-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
BitECS media loading fixes #6119
Conversation
src/bit-systems/media-loading.ts
Outdated
@@ -99,6 +100,9 @@ function resizeAndRecenter(world: HubsWorld, media: EntityID, eid: EntityID) { | |||
const mediaObj = world.eid2obj.get(media)!; | |||
const box = new Box3(); | |||
box.setFromObject(mediaObj); | |||
if (box.isEmpty()) { | |||
box.setFromCenterAndSize(new Vector3(0, 0, 0), new Vector3(1, 1, 1)); | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering in what scenarios box can be empty...
If I'm right, aren't visible objects always assigned to media entities by Hubs Client (media loaders) for loaded medias (eg: Plane Mesh for videos and audios)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example when a glTF is loaded that has a media component attached, when resizeAndRecenter
is called at that point there is no actual geometry in the gltf hierarchy as the video plane is added when the video is resolved and so the AABB is not set. As we resize and recenter the media based on that size that causes the media to be positioned at (0,0,0).
This will happen with any gltf that has a component that triggers an async media fetch: model, video, audio, image and link.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A better solution would be to resize and center when all the depending medias have been resolved but we don't have a good way of knowing that so that would be a more complicated solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW The main reason for this is that it works correctly in the AFrame loader so I'm focusing merely on parity here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example when a glTF is loaded that has a media component attached, when resizeAndRecenter is called at that point there is no actual geometry in the gltf hierarchy as the video plane is added when the video is resolved
Isn't this resizeAndRecenter()
called after video (or other media) is resolved?
And box seems to be set from resolved media object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but:
- We only recenter the spawned interactable not the media:
Interactable:
https://github.com/mozilla/hubs/blob/c5b17010a8aa2e7522ed3506e3c7e2a621d3f344/src/load-media-on-paste-or-drop.ts#L26-L32
Media (video example):
https://github.com/mozilla/hubs/blob/c5b17010a8aa2e7522ed3506e3c7e2a621d3f344/src/inflators/video-loader.ts#L14-L20
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I noticed that I had overlooked this part
This will happen with any gltf that has a component that triggers an async media fetch: model, video, audio, image and link.
Would you mind adding a comment about how here box can be empty? Other people may be confused, too, like I was. Or I would like to add a comment in another PR.
A better solution would be to resize and center when all the depending medias have been resolved but we don't have a good way of knowing that so that would be a more complicated solution.
Sounds like it is an ideal solution but yes I can understand it can be complicated. So the approach in this PR is reasonable to me. And we may revisit later if better solution will be really necessary.
src/bit-systems/media-loading.ts
Outdated
}); | ||
|
||
addComponent(world, MediaLoaded, media); | ||
removeComponent(world, MediaLoader, eid); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just in case, this changes the timing of when MediaLoaded
is added to an entity. It was added right after when media is loaded (eg: before animateScale()
starts) while with this change it is added everything is finished (eg: after animateScale()
ends). Did you confirm this timing change won't affect any other system? (Sorry I didn't have time to check the other codes by myself.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This affects when media playback starts: https://github.com/mozilla/hubs/blob/95a4deefafc548ab0104b6af32bf5ddb77d96454/src/bit-systems/video-system.ts#L49 but my understanding is that this is a more desirable behavior as we probably want a video playback to start when it's fully visible rather than when it's still scaling.
It also affects when a media-frame can capture an entity https://github.com/mozilla/hubs/blob/95a4deefafc548ab0104b6af32bf5ddb77d96454/src/systems/bit-media-frames.js#L114 and I also think that it makes sense that works this way unless I'm missing something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a commit to don't recenter/resize for objects without volume, I think that makes more sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a commit to don't recenter/resize for objects without volume, I think that makes more sense.
I agree with that it makes more sense.
Can you elaborate it for me please? There might be a chance that the problem should be fixed on such systems end. |
src/bit-systems/media-loading.ts
Outdated
const box = getBox(world, eid, media); | ||
addComponent(world, MediaContentBounds, eid); | ||
box.getSize(tmpVector); | ||
MediaContentBounds.bounds[eid].set(tmpVector.toArray()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The behavior is changed here in case media is failed to be loaded. Without this change MediaContentBounds
is not set (because MediaLoaded
is not set) while with this change it is set. Is this change intentional? (Honestly I'm not so sure about MediaContentBounds
.
0b18eff
to
4c4e61a
Compare
@takahirox To avoid confusion I've limited the scope of this PR to just not recentering/resizing the interactable in case it has no volume and I will address other issues in separate PRs. |
[MediaType.HTML]: (world: HubsWorld, { canonicalUrl, thumbnail }: { canonicalUrl: string, thumbnail: string }) => | ||
[MediaType.AUDIO]: (world: HubsWorld, { accessibleUrl }: { accessibleUrl: string }) => | ||
loadAudio(world, accessibleUrl), | ||
[MediaType.HTML]: (world: HubsWorld, { canonicalUrl, thumbnail }: { canonicalUrl: string; thumbnail: string }) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Just to confirm, this change is for linting, isn't it? Personally I prefer to separate updates for linting/formatting from bug fix/feature addition PRs for easier review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's only linting. As it was a really small linter update I thought it was fine to leave it here. I'll keep that in mind for the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid confusion I've limited the scope of this PR to just not recentering/resizing the interactable in case it has no volume and I will address other issues in separate PRs.
That sounds good to me.
This PR addresses some media issues an refactors some code:
Screen.Recording.2023-06-12.at.16.00.23.mp4
(You can test this using a glTF file that has an empty root and triggers a media loading. ie. a video loading media-video-flat.glb.zip)
MediaLoaded
instead ofMediaLoading
to know when a media is loaded.mixerAnimatableSystem
) crash if the loading cube is inmmediatly removed after adding it.