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.

Sign up for GitHub

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

check root node for animations #9407

Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
3332915
anim-check-root
robtfm Aug 10, 2023
c377ee1
dont unwrap
robtfm Aug 10, 2023
1ef51b9
duh
robtfm Aug 10, 2023
4ba07d2
add warning
robtfm Aug 10, 2023
f24ac9e
fmt
robtfm Aug 10, 2023
0acc31e
Update crates/bevy_animation/src/lib.rs
robtfm Aug 10, 2023
f7ccc25
Merge remote-tracking branch 'upstream/main' into anim-check-root
robtfm Aug 26, 2023
6966b9b
let-else fmt = conflict apparently
robtfm Aug 26, 2023
e97c0d4
Merge branch 'main' into anim-check-root
robtfm Aug 26, 2023
0646218
fmt fmt
robtfm Aug 26, 2023
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,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 @@ -301,8 +301,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 @@ -507,12 +517,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 @@ -601,6 +613,10 @@ fn apply_animation(
}
}
}

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

Expand Down