Skip to content
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

Lazy load media on scene entry #4135

Merged
merged 11 commits into from
Oct 13, 2021
Merged

Lazy load media on scene entry #4135

merged 11 commits into from
Oct 13, 2021

Conversation

keianhzo
Copy link
Contributor

@keianhzo keianhzo commented Apr 9, 2021

Related #3565

This PR adds initial support for lazy loading media upon scene loading to avoid waiting for all the media to be loaded to enter the scene.

┆Issue is synchronized with this Jira Task

@takahirox
Copy link
Contributor

Nice work! Let me test soon later.

One thing I talked with @netpro2k on Discord is there may be a chance that we need to change the loading cube effect because the current one may be annoying to users if there a lot of loading cubes in a viewport especially if media objects are big. This situation can happen on entry with this change.

@takahirox
Copy link
Contributor

takahirox commented Apr 12, 2021

One more thing. Do you think there is a use case that scene owner wants to disable lazy load on room entry, meaning the owner wants users to enter a room when all the assets are ready? (eg. Artistic purpose.) If so we may add an option to disable it in scene settings or somewhere else.

And for the automatic performance test I have been working on, I may want a capability to disable it or an event which is fired on all the assets are ready.

@keianhzo keianhzo force-pushed the lazy-loading-media branch 2 times, most recently from 1a7e226 to d0e757c Compare April 14, 2021 21:49
@keianhzo
Copy link
Contributor Author

@takahirox I've updated to enable/disable that through an admin config feature "Lazy load media". You should be able to override that from a script doing a configs.APP_CONFIG.features["lazy_load_media"] = true/false;

Regarding the loading cube, if you both agree I'd rather consulting that with DPX and opening another issue to replace it in case they consider that's necessary, otherwise we might be blocking this for too long.

@keianhzo keianhzo marked this pull request as ready for review April 14, 2021 21:49
@takahirox
Copy link
Contributor

takahirox commented Apr 14, 2021

Thanks for the update. Just in case I would like to know if other devs agree lazy_media_load configuration (for example, some of them might want to always enable it).

Opening another issue for loading cube effect sounds good to me.

I don't think I understand the scene entry flow so I would like other devs to review the code more deeply.

(And I would like to check the user experience of the case where a lot of medias are in a room but I couldn't do yet due to lack of time..)

@keianhzo
Copy link
Contributor Author

Now that you mention that, it might be interesting to also allow set it in the room settings and not at an admin level. Maybe @dom has a better sense of what might be more useful.

@keianhzo keianhzo requested review from johnshaughnessy and removed request for netpro2k September 8, 2021 15:38
@keianhzo
Copy link
Contributor Author

keianhzo commented Sep 8, 2021

I've rebased this and fixed a couple of issues:

  • The loading cube blend mode (was alphaBlend for some reason)
  • Since the networking refactor we don't get the didConnectToNetworkedScene event when the adapter is connected anymore. I've added events to the adapter to handle the connected event correctly as we rely on it for the scene loading UI.

@keianhzo keianhzo requested review from netpro2k and removed request for johnshaughnessy September 20, 2021 20:40
Copy link
Contributor

@netpro2k netpro2k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This mostly lgtm.

The moving of didConnectToNetworkedScene seems pretty fishy, it should be the reticulum connection and NAF adapter that have an opinion about this not dialog.

Also, it seems like this PR removes LoadingObject_Atom.glb ... That doesn't seem correct. Was there supposed to be a replacement? This would be a good time to grab the updated model from Jim.

Made a few minor comments on the react code, but those are mostly just informational.

src/components/media-video.js Outdated Show resolved Hide resolved
src/hub.js Outdated Show resolved Hide resolved
src/react-components/room/useRoomLoadingState.js Outdated Show resolved Hide resolved
src/react-components/room/useRoomLoadingState.js Outdated Show resolved Hide resolved
src/react-components/room/useRoomLoadingState.js Outdated Show resolved Hide resolved
src/react-components/room/useRoomLoadingState.js Outdated Show resolved Hide resolved
src/hub.js Outdated Show resolved Hide resolved
@keianhzo
Copy link
Contributor Author

@netpro2k Thanks for the review, there were a few thing that were wrong or not optimal. I've have updated with a few changes.

Copy link
Contributor

@netpro2k netpro2k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes lgtm

@keianhzo keianhzo merged commit 8461d01 into master Oct 13, 2021
@keianhzo keianhzo deleted the lazy-loading-media branch October 13, 2021 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants