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(useAnimations): passing root to mixer init if it is Object3D #1307

Merged

Conversation

casualWaist
Copy link
Contributor

@casualWaist casualWaist commented Feb 22, 2023

Fixes #429

Why

Trying to create a new AnimationClips in useAnimations mixer fails because mixer is instantiated with no reference to the root scene as described in issue #429

What

passed the root scene to the constructor function after and instanceof check

Checklist

  • Ready to be merged

This is my first ever pull request, the fix worked for me but I'm not sure if this is breaking some intentional design or I'm missing something else. Be GeNtLe :)

@vercel
Copy link

vercel bot commented Feb 22, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
drei ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 8, 2023 3:23am

@codesandbox-ci
Copy link

codesandbox-ci bot commented Feb 22, 2023

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 75226c1:

Sandbox Source
recursing-sid-gduryn Configuration
Ground reflections and video textures Configuration
arc-x-pmndrs-colors Configuration

@drcmda
Copy link
Member

drcmda commented Mar 13, 2023

sorry for the long delay.

i think useState is the wrong place, it should probably be a useLayoutEffect that sets the property? otherwise useState would be stale and couldn't react to changing the root from one to another?

@casualWaist
Copy link
Contributor Author

Okay, I think I see. You're saying if the root of the mixer changes via a direct call then the actualRef will be out of sync with the mixer and the useEffect cleanup won't work? I admit I didn't think to do any tests with multiple objects interacting with the mixer. Sorry, I will revisit this.

@casualWaist
Copy link
Contributor Author

I did some testing and I'm more confused. I don't understand when or why the root object would change as other objects can be passed as optional root for a new action or create a separate mixer. The class doesn't seem to have a function that changes it, even unchacheRoot didn't change it, so, I don't know. Please let me know what I should do. Should I delete the pull request?

@CodyJasonBennett CodyJasonBennett merged commit a590d3d into pmndrs:master May 8, 2023
5 checks passed
@github-actions
Copy link

github-actions bot commented May 8, 2023

🎉 This PR is included in version 9.66.6 🎉

The release is available on:

Your semantic-release bot 📦🚀

@casualWaist casualWaist deleted the bug-fix/429-set-root-fix branch May 8, 2023 03:34
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.

mixer from useAnimations doesn't set _root property
3 participants