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
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 @@ -295,8 +295,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;
}
robtfm marked this conversation as resolved.
Show resolved Hide resolved
Comment on lines +308 to +313
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.


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 @@ -497,10 +507,12 @@ 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 @@ -587,6 +599,10 @@ fn apply_animation(
}
}
}

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

Expand Down
Loading