Implement minimal animation graphs in order to support animation blending. #11670

pcwalton · 2024-02-03T05:51:51Z

Objective

This is a minimum viable implementation of bevyengine/rfcs#51. It allows Bevy to support animation blending.

Solution

This is just to start the conversation.

james7132 · 2024-02-03T06:21:45Z

crates/bevy_animation/src/graph.rs

+    transforms: Query<&mut Transform>,
+    morph_weights: Query<&mut MorphWeights>,
+    parents: Query<(Has<AnimationPlayer>, Has<AnimationGraph>, Option<&Parent>)>,
+    mut animation_graphs: Query<(Entity, Option<&Parent>, &mut AnimationGraph)>,


This seems to drop the parallelization that is currently supported, and may be a significant performance regression for scenes with a large number of animated rigs.

Is there any reason we cannot just use .par_iter_mut().for_each here? Seems like it would work.

It does look like par_iter would bring this in line with how things are currently dispatched in animation_player. Same exact query more or less.

james7132 · 2024-02-03T06:33:36Z

crates/bevy_animation/src/graph.rs

+
+        let node = &mut self.animation_graph.graph[*node_index];
+        if let Some(ref mut playing_animation) = node.animation {
+            crate::apply_animation(


Each time apply_animation is called, the query for each bone within the hierarchy is redone, which involves a hierarchy traversal and multiple string comparisons. This works, but the total overhead scales with the number of clips in the graph. This isn't a significant departure from what we currently have, so I don't want to block on this, but we need to address this sooner rather than later.

james7132 · 2024-02-03T06:35:23Z

crates/bevy_animation/src/graph.rs

+    'mwq,
+    'mwa,
+    'mwb,
+    'mwc,


These lifetimes definitely need to be addressed before we merge this. This is a little ridiculous.

Can these not share the same lifetime bound? See for example here and here.

I was able to appease the borrow-checker with the following:

struct AnimationContext<'l, 'w, 's, 'a> { animation_graph: &'l mut AnimationGraph, time: &'l Time, root_entity: Entity, animation_clips: &'l Assets<AnimationClip>, parent: Option<&'w Parent>, parents: &'l Query< 'w, 's, ( Has<AnimationPlayer>, Has<AnimationGraph>, Option<&'a Parent>, ), >, children: &'l Query<'w, 's, &'a Children>, names: &'l Query<'w, 's, &'a Name>, transforms: &'l Query<'w, 's, &'a mut Transform>, morph_weights: &'l Query<'w, 's, &'a mut MorphWeights>, } pub fn evaluate_animation_graphs<'a>( time: Res<Time>, animation_clips: Res<Assets<AnimationClip>>, children: Query<&'a Children>, names: Query<&'a Name>, transforms: Query<&'a mut Transform>, morph_weights: Query<&'a mut MorphWeights>, parents: Query<( Has<AnimationPlayer>, Has<AnimationGraph>, Option<&'a Parent>, )>, mut animation_graphs: Query<(Entity, Option<&'a Parent>, &'a mut AnimationGraph)>, )

I don't think it is possible to reduce this further, and I'm not 100% confident this ends up the same as the original. Maybe 'a is actually static idk. Not currently sure what AnimationContext is for but I trust the process.

pcwalton · 2024-02-04T20:18:03Z

I'm closing this as there will need to be a lot of changes.

# Objective Bevy's animation system currently does tree traversals based on `Name` that aren't necessary. Not only do they require in unsafe code because tree traversals are awkward with parallelism, but they are also somewhat slow, brittle, and complex, which manifested itself as way too many queries in #11670. # Solution Divide animation into two phases: animation *advancement* and animation *evaluation*, which run after one another. *Advancement* operates on the `AnimationPlayer` and sets the current animation time to match the game time. *Evaluation* operates on all animation bones in the scene in parallel and sets the transforms and/or morph weights based on the time and the clip. To do this, we introduce a new component, `AnimationTarget`, which the asset loader places on every bone. It contains the ID of the entity containing the `AnimationPlayer`, as well as a UUID that identifies which bone in the animation the target corresponds to. In the case of glTF, the UUID is derived from the full path name to the bone. The rule that `AnimationTarget`s are descendants of the entity containing `AnimationPlayer` is now just a convention, not a requirement; this allows us to eliminate the unsafe code. # Migration guide * `AnimationClip` now uses UUIDs instead of hierarchical paths based on the `Name` component to refer to bones. This has several consequences: - A new component, `AnimationTarget`, should be placed on each bone that you wish to animate, in order to specify its UUID and the associated `AnimationPlayer`. The glTF loader automatically creates these components as necessary, so most uses of glTF rigs shouldn't need to change. - Moving a bone around the tree, or renaming it, no longer prevents an `AnimationPlayer` from affecting it. - Dynamically changing the `AnimationPlayer` component will likely require manual updating of the `AnimationTarget` components. * Entities with `AnimationPlayer` components may now possess descendants that also have `AnimationPlayer` components. They may not, however, animate the same bones. * As they aren't specific to `TypeId`s, `bevy_reflect::utility::NoOpTypeIdHash` and `bevy_reflect::utility::NoOpTypeIdHasher` have been renamed to `bevy_reflect::utility::NoOpHash` and `bevy_reflect::utility::NoOpHasher` respectively.

# Objective Bevy's animation system currently does tree traversals based on `Name` that aren't necessary. Not only do they require in unsafe code because tree traversals are awkward with parallelism, but they are also somewhat slow, brittle, and complex, which manifested itself as way too many queries in bevyengine#11670. # Solution Divide animation into two phases: animation *advancement* and animation *evaluation*, which run after one another. *Advancement* operates on the `AnimationPlayer` and sets the current animation time to match the game time. *Evaluation* operates on all animation bones in the scene in parallel and sets the transforms and/or morph weights based on the time and the clip. To do this, we introduce a new component, `AnimationTarget`, which the asset loader places on every bone. It contains the ID of the entity containing the `AnimationPlayer`, as well as a UUID that identifies which bone in the animation the target corresponds to. In the case of glTF, the UUID is derived from the full path name to the bone. The rule that `AnimationTarget`s are descendants of the entity containing `AnimationPlayer` is now just a convention, not a requirement; this allows us to eliminate the unsafe code. # Migration guide * `AnimationClip` now uses UUIDs instead of hierarchical paths based on the `Name` component to refer to bones. This has several consequences: - A new component, `AnimationTarget`, should be placed on each bone that you wish to animate, in order to specify its UUID and the associated `AnimationPlayer`. The glTF loader automatically creates these components as necessary, so most uses of glTF rigs shouldn't need to change. - Moving a bone around the tree, or renaming it, no longer prevents an `AnimationPlayer` from affecting it. - Dynamically changing the `AnimationPlayer` component will likely require manual updating of the `AnimationTarget` components. * Entities with `AnimationPlayer` components may now possess descendants that also have `AnimationPlayer` components. They may not, however, animate the same bones. * As they aren't specific to `TypeId`s, `bevy_reflect::utility::NoOpTypeIdHash` and `bevy_reflect::utility::NoOpTypeIdHasher` have been renamed to `bevy_reflect::utility::NoOpHash` and `bevy_reflect::utility::NoOpHasher` respectively.

Mostly-complete implementation of animation graphs

8163c6c

james7132 requested changes Feb 3, 2024

View reviewed changes

alice-i-cecile added C-Feature A new feature, making something new possible A-Animation Make things move and change over time labels Feb 3, 2024

pcwalton closed this Feb 4, 2024

pcwalton mentioned this pull request Feb 7, 2024

Rework animation to be done in two phases. #11707

Merged

lee-orr mentioned this pull request Mar 18, 2024

Multiple Animations on the same Mesh/Skeleton which can run Simultaneously. #6562

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement minimal animation graphs in order to support animation blending. #11670

Implement minimal animation graphs in order to support animation blending. #11670

pcwalton commented Feb 3, 2024

james7132 Feb 3, 2024

mbrea-c Feb 3, 2024

NthTensor Feb 4, 2024

james7132 Feb 3, 2024

james7132 Feb 3, 2024

mbrea-c Feb 3, 2024

NthTensor Feb 4, 2024 •

edited

Loading

pcwalton commented Feb 4, 2024

Implement minimal animation graphs in order to support animation blending. #11670

Implement minimal animation graphs in order to support animation blending. #11670

Conversation

pcwalton commented Feb 3, 2024

Objective

Solution

james7132 Feb 3, 2024

Choose a reason for hiding this comment

mbrea-c Feb 3, 2024

Choose a reason for hiding this comment

NthTensor Feb 4, 2024

Choose a reason for hiding this comment

james7132 Feb 3, 2024

Choose a reason for hiding this comment

james7132 Feb 3, 2024

Choose a reason for hiding this comment

mbrea-c Feb 3, 2024

Choose a reason for hiding this comment

NthTensor Feb 4, 2024 • edited Loading

Choose a reason for hiding this comment

pcwalton commented Feb 4, 2024

NthTensor Feb 4, 2024 •

edited

Loading