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 animation auto play #16098

Merged
merged 3 commits into from
Jan 23, 2025
Merged

Conversation

ryantrem
Copy link
Member

With some recent refactoring I broke animation-auto-play. Previously it was doing some juggling with responsibilities split across the Viewer and the ViewerElement. With this change, I moved all the responsibility to the Viewer, dependent on load options. This makes it much simpler to ensure the logic is correct. An alternative would be to make the ViewerElement entirely responsible, and have it sequentially load the model, then set the selectedAnimation, then play the animation. The only reason I don't want to do it this way is because changing the selectedAnimation animates the camera pose (based on computed bounds), and when the model is first being loaded, we don't want this animation. So it's either add load options for the Viewer, or expose the selectAnimation function so the ViewerElement can select an animation without animating the camera pose. But since other view framework layers would want the same behavior, I think it's better to do it in the Viewer.

@ryantrem ryantrem requested review from RaananW and sebavan January 22, 2025 22:37
@ryantrem ryantrem enabled auto-merge (squash) January 22, 2025 22:38
@bjsplat
Copy link
Collaborator

bjsplat commented Jan 22, 2025

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@bjsplat
Copy link
Collaborator

bjsplat commented Jan 22, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Jan 22, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Jan 22, 2025

@ryantrem ryantrem merged commit 48b263e into BabylonJS:master Jan 23, 2025
14 checks passed
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