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

check root node for animations #9407

Merged
merged 10 commits into from
Aug 28, 2023
Merged

Conversation

robtfm
Copy link
Contributor

@robtfm robtfm commented Aug 10, 2023

Objective

fixes #8357

gltf animations can affect multiple "root" nodes (i.e. top level nodes within a gltf scene).

the current loader adds an AnimationPlayer to each root node which is affected by any animation. when a clip which affects multiple root nodes is played on a root node player, the root node name is not checked, leading to potentially incorrect weights being applied.
also, the AnimationClip::compatible_with method will never return true for those clips, as it checks that all paths start with the root node name - not all paths start with the same name so it can't return true.

Solution

  • check the first path node name matches the given root
  • change compatible_with to return true if any match is found

a better alternative would probably be to attach the player to the scene root instead of the first child, and then walk the full path from there. this would be breaking (and would stop multiple animations that don't overlap from being played concurrently), but i'm happy to modify to that if it's preferred.

@robtfm robtfm added C-Bug An unexpected or incorrect behavior A-Animation Make things move and change over time labels Aug 10, 2023
@nicopap
Copy link
Contributor

nicopap commented Aug 10, 2023

If I'm not mistaken this "Fixes #8357"?

Comment on lines +302 to +307
let Some((_, root_name)) = parts.next() else {
return None;
};
if names.get(current_entity) != Ok(root_name) {
return None;
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let Some((_, root_name)) = parts.next() else {
return None;
};
if names.get(current_entity) != Ok(root_name) {
return None;
}
if names.get(current_entity).ok() != parts.next().map(|(_, part)| part) {
return None;
};

Just a style nit, but I would prefer this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's slightly different semantically - this would pass if parts is empty and the node has no name. neither of those things can happen via the gltf loader but could happen for clips/players constructed another way.

could maybe go with if names.get(current_entity) != Ok(parts.next()?.1) { ... i'm not sure it's nicer.

robtfm added a commit to decentraland/bevy-explorer that referenced this pull request Aug 16, 2023
features
- `AvatarAttach`
- `Visibility`
- `PointerResults`:
  - add normal
  - add position
  - add scene entity to hit result for events sent to scene roots
- collisions
  - add DisableCollisions marker, apply to `AvatarAttach`ed items
- mock js modules

bugfixes
- animations
  - fix path bug (inc bevy patch [#9407](bevyengine/bevy#9407)) that caused anims to play on the wrong nodes
  - play gltf first animation if no animator present
  - play animation resets the anim (only if not looping)
- fix main.crdt buffer overrun
- fix collider pipeline use-before-init crash
- system log chat tab now updates live without forcing reload
- chat tab wraps correctly
- send transform updates to scene only if they are changed
- don't validate the `main_file` for texture wearables, just look for pngs
- AudioSource 
  - only plays in current scene
  - use manual spatial calc to preserve requested volume
  
tweaks
- increase base move speed
- vsync off by default
- update to latest protobuf messages
@nicopap nicopap added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Aug 20, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Aug 28, 2023
Merged via the queue into bevyengine:main with commit 25e5239 Aug 28, 2023
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
# Objective

fixes bevyengine#8357 

gltf animations can affect multiple "root" nodes (i.e. top level nodes
within a gltf scene).

the current loader adds an AnimationPlayer to each root node which is
affected by any animation. when a clip which affects multiple root nodes
is played on a root node player, the root node name is not checked,
leading to potentially incorrect weights being applied.
also, the `AnimationClip::compatible_with` method will never return true
for those clips, as it checks that all paths start with the root node
name - not all paths start with the same name so it can't return true.

## Solution

- check the first path node name matches the given root
- change compatible_with to return true if `any` match is found

a better alternative would probably be to attach the player to the scene
root instead of the first child, and then walk the full path from there.
this would be breaking (and would stop multiple animations that *don't*
overlap from being played concurrently), but i'm happy to modify to that
if it's preferred.

---------

Co-authored-by: Nicola Papale <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Animation Make things move and change over time C-Bug An unexpected or incorrect behavior S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AnimationPlayer can play unrelated AnimationClips
4 participants