Rework animation to be done in two phases. #11707
Conversation
This eliminates all the unsafe code, increases parallelism, and reduces the number of queries to just two.
There was a problem hiding this comment.
Setting aside performance, this is a big win in both safety and maintainability. The documentation still needs to be improved, but it is already so much easier to tell what is going on.
Aside from one new exclusive system I'm not seeing anything (structural) that would make me worry about performance, and even that is a work-around we should eventually be able to remove. I will maybe run some benchmarks next week.
I really really like this.
This patch places all entity paths into a shared table so that comparing them is as cheap as a pointer comparison. We don't use the pre-existing `Interner` type because that leaks all strings added to it, and I was uncomfortable with leaking names, as Bevy apps might dynamically generate them. Instead, I reference count all the names and use a linked list to stitch together paths into a tree. The interner uses a weak hash set from the [`weak_table`] crate. This patch is especially helpful for the two-phase animation PR bevyengine#11707, because two-phase animation gets rid of the cache from name to animation-specific slot, thus increasing the load on the hash table that maps paths to bone indices. Note that the interned table is a global variable behind a `OnceLock` instead of a resource. This is because it must be accessed from the glTF `AssetLoader`, which unfortunately has no access to Bevy resources.
This patch places all entity paths into a shared table so that comparing them is as cheap as a pointer comparison. We don't use the pre-existing `Interner` type because that leaks all strings added to it, and I was uncomfortable with leaking names, as Bevy apps might dynamically generate them. Instead, I reference count all the names and use a linked list to stitch together paths into a tree. The interner uses a weak hash set from the [`weak-table`] crate. This patch is especially helpful for the two-phase animation PR bevyengine#11707, because two-phase animation gets rid of the cache from name to animation-specific slot, thus increasing the load on the hash table that maps paths to bone indices. Note that the interned table is a global variable behind a `OnceLock` instead of a resource. This is because it must be accessed from the glTF `AssetLoader`, which unfortunately has no access to Bevy resources. [`weak-table`]: https://crates.io/crates/weak-table
|
on the many_foxes examples, this PR is slower than main |
|
I would expect the problem to be #11711. I also haven't looked at performance at all. |
|
By the way, I don't know how to implement the Animatable trait, which is an approved RFC, without something like this or an explosion in unsafe code. |
alice-i-cecile
added
C-Performance
There was a problem hiding this comment.
Just wanted to leave an early comment that this overall looks good to me now, but I would like to do some profiling to dig into why @NthTensor found little improvement with this change (see here for some potential ideas on what they might be).
james7132
added
S-Ready-For-Final-Review
|
example |
mockersf
removed
the
S-Ready-For-Final-Review
|
https://github.com/KhronosGroup/glTF-Sample-Assets/tree/main/Models/InterpolationTest doesn't work either with this PR |
|
OK, I allowed |
|
Both |
|
|
@mockersf You were running Both of those issues are problems with the examples, not with the Bevy logic. |
|
Those should be working now and should be ready for review and merging afterward now that 0.13 has shipped. |
Yup!
Thanks! It's good practice to fix the examples with the PR that breaks them, it avoids having to investigate without the context. |
mockersf
added
the
S-Ready-For-Final-Review
# 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.
# 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.
Objective
Bevy's animation system currently does tree traversals based on
Namethat 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
AnimationPlayerand 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 theAnimationPlayer, 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 thatAnimationTargets are descendants of the entity containingAnimationPlayeris now just a convention, not a requirement; this allows us to eliminate the unsafe code.Migration guide
AnimationClipnow uses UUIDs instead of hierarchical paths based on theNamecomponent to refer to bones. This has several consequences:AnimationTarget, should be placed on each bone that you wish to animate, in order to specify its UUID and the associatedAnimationPlayer. The glTF loader automatically creates these components as necessary, so most uses of glTF rigs shouldn't need to change.AnimationPlayerfrom affecting it.AnimationPlayercomponent will likely require manual updating of theAnimationTargetcomponents.AnimationPlayercomponents may now possess descendants that also haveAnimationPlayercomponents. They may not, however, animate the same bones.TypeIds,bevy_reflect::utility::NoOpTypeIdHashandbevy_reflect::utility::NoOpTypeIdHasherhave been renamed tobevy_reflect::utility::NoOpHashandbevy_reflect::utility::NoOpHasherrespectively.