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

Hide LOD progressive loading mode behind the flag #5795

Merged
merged 1 commit into from
Nov 28, 2022

Conversation

takahirox
Copy link
Contributor

From the internal discussion with @netpro2k and @johnshaughnessy

Currently LOD progressive loading mode is chosen by default but it may be useless without rangerequests that is hidden by flag because without rangerequests the glTF loader starts with loading entire file and just lazily parse higher levels on demand.
Just lazy parsing isn't worth. It just makes the loading path complex.

So better to hide LOD progressive loading mode behind the flag. Enable both range requests and LOD progressive loading mode if rangerequests query strings is in URL.

Currently LOD progressive loading mode is chosen
by default but it may be useless without rangerequests
that is hidden by flag because without rangerequests
the glTF loader starts with loading entire file
and just lazily parse higher levels on demand.
Just lazy parsing isn't worth. It just makes the
loading path complex.

So better to hide LOD progressive loading mode
behind the flag. Enable both range requests and
LOD progressive loading mode if rangerequests
query strings is in URL.
@@ -134,7 +134,7 @@
"semver": "^7.3.2",
"three": "github:mozillareality/three.js#56e7c46d991cc16bff82bdbb03c7bfba4620567f",
"three-ammo": "github:mozillareality/three-ammo",
"three-gltf-extensions": "^0.0.13",
"three-gltf-extensions": "^0.0.14",
Copy link
Contributor Author

@takahirox takahirox Nov 15, 2022

Choose a reason for hiding this comment

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

For reviewers: Found a bug in "all" loading mode so needed to fix and upgrade.

takahirox/three-gltf-extensions#96

@johnshaughnessy
Copy link
Contributor

johnshaughnessy commented Nov 28, 2022

It looks like the test-and-deploy-storybook job failed with a network error:

npm ERR! npm ERR! code ECONNRESET
npm ERR! npm ERR! network aborted
npm ERR! npm ERR! network This is a problem related to network connectivity.
npm ERR! npm ERR! network In most cases you are behind a proxy or have bad network settings.
npm ERR! npm ERR! network 
npm ERR! npm ERR! network If you are behind a proxy, please make sure that the
npm ERR! npm ERR! network 'proxy' config is set properly.  See: 'npm help config'

I'm going to rerun it to see if it was just a blip.
Original: https://github.com/mozilla/hubs/actions/runs/3474092916/jobs/5806865372
Rerun: https://github.com/mozilla/hubs/actions/runs/3474092916/jobs/5997089308

[Update: The rerun succeeded.]

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.

lgtm

@takahirox takahirox merged commit 1fc6826 into master Nov 28, 2022
@takahirox takahirox deleted the LODProgressiveLoadingBehindFlag branch November 28, 2022 21:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants