Skip to content

Commit

Permalink
check root node for animations (#9407)
Browse files Browse the repository at this point in the history
# 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.

---------

Co-authored-by: Nicola Papale <nicopap@users.noreply.github.com>
  • Loading branch information
robtfm and nicopap committed Aug 28, 2023
1 parent b28f633 commit 25e5239
Showing 1 changed file with 19 additions and 3 deletions.
22 changes: 19 additions & 3 deletions crates/bevy_animation/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ impl AnimationClip {

/// Whether this animation clip can run on entity with given [`Name`].
pub fn compatible_with(&self, name: &Name) -> bool {
self.paths.keys().all(|path| &path.parts[0] == name)
self.paths.keys().any(|path| &path.parts[0] == name)
}
}

Expand Down Expand Up @@ -406,8 +406,18 @@ fn entity_from_path(
// PERF: finding the target entity can be optimised
let mut current_entity = root;
path_cache.resize(path.parts.len(), None);
// Ignore the first name, it is the root node which we already have
for (idx, part) in path.parts.iter().enumerate().skip(1) {

let mut parts = path.parts.iter().enumerate();

// check the first name is the root node which we already have
let Some((_, root_name)) = parts.next() else {
return None;
};
if names.get(current_entity) != Ok(root_name) {
return None;
}

for (idx, part) in parts {
let mut found = false;
let children = children.get(current_entity).ok()?;
if let Some(cached) = path_cache[idx] {
Expand Down Expand Up @@ -608,12 +618,14 @@ fn apply_animation(
return;
}

let mut any_path_found = false;
for (path, bone_id) in &animation_clip.paths {
let cached_path = &mut animation.path_cache[*bone_id];
let curves = animation_clip.get_curves(*bone_id).unwrap();
let Some(target) = entity_from_path(root, path, children, names, cached_path) else {
continue;
};
any_path_found = true;
// SAFETY: The verify_no_ancestor_player check above ensures that two animation players cannot alias
// any of their descendant Transforms.
//
Expand Down Expand Up @@ -702,6 +714,10 @@ fn apply_animation(
}
}
}

if !any_path_found {
warn!("Animation player on {root:?} did not match any entity paths.");
}
}
}

Expand Down

0 comments on commit 25e5239

Please sign in to comment.