Bit media frames physics #6003
Bit media frames physics #6003
Conversation
keianhzo
force-pushed
the
bit-media-frames-physics
branch
from
4999ec4
to
ca6b79b
Compare
There was a problem hiding this comment.
I reviewed the PR mainly for bitecs design and readability. Most my comments are for future clean up and discussion, and don't block merging.
I haven't reviewed the core logic because I'm not familiar with media frames physics implementation. It may be ok if it keeps working because we focus on migration now. Or another reviewer may need to check.
Honestly bit-systems/media-loading.ts, systems/bit-media-frames (and others) are getting complex and harder to be maintained. But we may refactor them later because our development is at the migration phase now.
There was a problem hiding this comment.
This is looking good. I am happy to see the media frame system didn't need to change all that much and most of the code changes there were are just around generating the preview and getting the object bounds (which is what you would expect)
I made a bunch of little suggestions and observations, but none are sticking out to me as extreme blockers. (I think the disposal stuff probably sticks out the most, but its mostly an issue of "correctness", since in reality media frames are very rarely removed except for at scene transition time when the objects they are previewing are also going away).
The object bounding box stuff still strikes me as incredibly more complicated than I would expect (not a slight on your code, its happened ever time we try and write code like it). I have a very strong suspicion we are just tripping over ourselves because of some underlying complexity, and my best bet is that its in shape generation code in the physics system. When I think back I sort of remember the "just write the obvious thing" version of that code working except "but the physics shape doesn't line up now"... Been a long time since I looked at it though so I could be miss-remembering. I think once we are down to 1 codepath for all this stuff and we have had time to rethink physics a bit, its worth a revisit.
| @@ -19,6 +20,39 @@ import { loadAudio } from "../utils/load-audio"; | |||
| import { MediaType, mediaTypeName, resolveMediaInfo } from "../utils/media-utils"; | |||
| import { EntityID } from "../utils/networking-types"; | |||
|
|
|||
| export const MediaContentBounds = (MediaLoaded as any).contentBounds as Map<EntityID, Vector3>; | |||
|
|
|||
| const getBox = (() => { | |||
There was a problem hiding this comment.
This code definitely feels pretty funky, and I remember having similar feelings the first time similar code was written for original media. I think a lot of it was around getting physics shapes to line up, but I suspect thats all actually just a sympton f the physics code being wrong and then we bend over backwards to accommodate those weird assumptions in the rest of our code. Definitely wroth revisiting when we get around to a cleanup pass on physics.
src/systems/bit-media-frames.js
Outdated
| const isFrameDeleting = findAncestorWithComponent(world, Deleting, frame); | ||
| const isFrameOwned = hasComponent(world, Owned, frame); | ||
|
|
||
| if (captured && isCapturedOwned && !isCapturedHeld && !isFrameDeleting && colliding) { |
There was a problem hiding this comment.
Was slightly uneasy when I saw the addition of the Deleting component at the start of reviewing this PR. I see here we are using it to prevent scaling the object down as the media frame it was in gets shrunk during the delete animation. It definitely seems really odd to have to think about that delete animation in systems and this definitely wont be the last time we have to... Don't have a nicer solution off the top of my head but we should definitely brainstorm on this more.
| @@ -229,10 +316,16 @@ function mediaTypesOf(world, entities) { | |||
| return mask; | |||
| } | |||
|
|
|||
| export function cleanupMediaFrame(obj) { | |||
| obj.traverse(child => { | |||
There was a problem hiding this comment.
No need for the wrapping function.
That said not sure this is correct. The only thing the media frame "owns" in terms of disposable resources is the geometry for the guide. The cloned preview object is all just references to geometries/textures that something else owns. Its actually incorrect to try and .dispose() a geometry/texture for an object we happened to be previewing just because this media frame is going away, since the object that actually "owns" those things is not going away.
With the way threejs resource management works you won't actually notice this issue since it will lazily re-create everything. So I think what will happen in practice is when you delete a media frame capturing an object we will delete the GPU side resources for them and then re-upload them to the GPU next frame.
There was a problem hiding this comment.
Oof, actually looking at the code some more, for videos we actually are creating a new Geometry so we technically do want to clean that up. We actually don't need a new geometry for each media frame and I am fine with statically keeping that around for the lifetime of the application so we shouldn't need to clean that up. Will add a note above about that as well.
There was a problem hiding this comment.
This should only be disposing the guide as the preview is not a child of the media frame and is removed when hidden so it shouldn't be affected by the cleanup. But you are right, the video geometry is not being cleaned up correctly. I didn't want to pre-allocate that to avoid allocating resources for something that can potentially never be used but maybe that's better that disposing it every time a preview is hidden so I'm going o do that.
src/systems/bit-media-frames.js
Outdated
| previewMaterial.transparent = true; | ||
| previewMaterial.opacity = 0.5; | ||
|
|
||
| const geometry = createPlaneBufferGeometry(1, 1, 1, 1, TEXTURES_FLIP_Y); |
There was a problem hiding this comment.
I noted in the cleanup code, we would technically need to clean this up (since we are the ones creating a disposable resource), but we don't want to dipose of the previewMesh's geometry for the other paths (since we are just referencing them. Since we don't actually mutate this geometry at all we can just statically define it once and share it among all media frames, and never dispose() of it.
Built on top of #5978. Only the last commit applies.
As commented in a previous PR,