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 useprogress multi instanced and improved the related story #304

Merged
merged 3 commits into from
Feb 16, 2021
Merged

fix useprogress multi instanced and improved the related story #304

merged 3 commits into from
Feb 16, 2021

Conversation

RenaudRohlinger
Copy link
Member

@RenaudRohlinger RenaudRohlinger commented Feb 14, 2021

Why

fix #302

What

It seems that threejs is by default always using the DefaultLoadingManager so using a new instance of the LoadingManager won't work.
Because it uses the DefaultLoadingManager values are never reset.
I fixed it and added an offset that decrements the value for the new elements loaded when the previous loading was finished.

Before it would do :
0 -> 100% (let's say 1 -> 10 items where loaded)
Then for any new element loaded :
90% -> 100% ( 10 -> 11 items)

Now it will do the progress based on only the new element that is loading (10 - (last total loaded 10) -> 11 = 1).

I improved the story using multiple dynamic loading with Knobs to prevent errors.

The best solution would be to add a reset method to the LoadingManager in threejs.

Checklist

  • Storybook entry added
  • Ready to merge

Demo :
https://drei-git-fork-renaudrohlinger-fix-useprogress.pmndrs.vercel.app/?path=/story/misc-useprogress--use-progress-scene-st

@vercel
Copy link

vercel bot commented Feb 14, 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/83bo4svwy
✅ Preview: https://drei-git-fork-renaudrohlinger-fix-useprogress.pmndrs.vercel.app

@codesandbox-ci
Copy link

codesandbox-ci bot commented Feb 14, 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 4cedbdf:

Sandbox Source
brave-frost-jhp3w Configuration
drei reflector Configuration
arc-x-pmndrs-colors Configuration
react-three-fiber starter Issue #302

Copy link
Member

@joshuaellis joshuaellis left a comment

Choose a reason for hiding this comment

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

This just needs clearing up for me. Do we need to amend the documentation also?

src/core/useProgress.tsx Outdated Show resolved Hide resolved
@RenaudRohlinger
Copy link
Member Author

@joshuaellis I set back the use of DefaultLoadingManager and I just checked, nothing needs to be changed in the documentation. It should be safe and ready to merge.

Demo:
https://drei-git-fork-renaudrohlinger-fix-useprogress.pmndrs.vercel.app/?path=/story/misc-useprogress--use-progress-scene-st

Copy link
Member

@joshuaellis joshuaellis left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for fixing this!

@joshuaellis joshuaellis merged commit 707fb48 into pmndrs:master Feb 16, 2021
@github-actions
Copy link

🎉 This PR is included in version 3.8.1 🎉

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

useProgress is broken
2 participants