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

fix: Makes Loader go from 0 to 100 instead of 0 to 1 #280

Merged
merged 3 commits into from
Feb 3, 2021

Conversation

gsimone
Copy link
Member

@gsimone gsimone commented Feb 1, 2021

Come on.

@vercel
Copy link

vercel bot commented Feb 1, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/pmndrs/drei/h0ad1l1zo
✅ Preview: https://drei-git-gsimone-minor-fixes.pmndrs.vercel.app

@codesandbox-ci
Copy link

codesandbox-ci bot commented Feb 1, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit cdb216b:

Sandbox Source
trusting-rosalind-0qjbh Configuration
drei reflector Configuration
arc-x-pmndrs-colors Configuration

@joshuaellis
Copy link
Member

It's funny, I saw this all last week, and told myself "I must fix it" and I never did, what would we do without you?

@gsimone
Copy link
Member Author

gsimone commented Feb 1, 2021

Thankfully I'm here, to fix the stuff that REALLY matters, like this U_U

@gsimone gsimone requested a review from drcmda February 1, 2021 19:18
@joshuaellis
Copy link
Member

Do you think we still need Travis? I'm not actually clear on what it does.

@RenaudRohlinger
Copy link
Member

RenaudRohlinger commented Feb 2, 2021

@joshuaellis @gsimone @drcmda This one makes me remember of one bug. When we want to load something new, the loader assets will stack because we use the same instance of LoadingManager. So it means for example it will do 0 -> 1 on the first load. Then 0.5 -> 1 etc.... because this one is never reset :
https://github.com/mrdoob/three.js/blob/master/src/loaders/LoadingManager.js#L6

So I guess we should use a fresh instance of LoadingManager instead of DefaultLoadingManager like this :
https://github.com/RenaudRohlinger/drei/blob/master/src/core/useProgress.tsx
(I don't want to PR yet because I can't test yet but it should work)

@joshuaellis
Copy link
Member

Are you talking about if there are multiple suspense wrappers in an app?

@gsimone
Copy link
Member Author

gsimone commented Feb 2, 2021

Makes sense to use multiple Loaders, since this component is intended to "hide" the whole app - so it implies that it's only ever used one at a time

@joshuaellis
Copy link
Member

Can we write that in the docs then, please? I know it's a bit trivial, I do remember someone having an issue with that, and I did have a similar scenario myself and was confused about what was going on till I really thought about it 👀

@joshuaellis joshuaellis changed the base branch from master to beta February 3, 2021 18:19
@joshuaellis joshuaellis merged commit 683adaa into beta Feb 3, 2021
@joshuaellis joshuaellis deleted the gsimone-minor-fixes branch February 3, 2021 18:19
@github-actions
Copy link

github-actions bot commented Feb 3, 2021

🎉 This PR is included in version 3.3.0-beta.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions github-actions bot added the released on @beta released in beta for review label Feb 3, 2021
@github-actions
Copy link

github-actions bot commented Feb 3, 2021

🎉 This PR is included in version 3.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released on @beta released in beta for review released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants