Rework animation to be done in two phases. #11707

pcwalton · 2024-02-05T00:53:49Z

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 AnimationTargets 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 TypeIds, bevy_reflect::utility::NoOpTypeIdHash and bevy_reflect::utility::NoOpTypeIdHasher have been renamed to bevy_reflect::utility::NoOpHash and bevy_reflect::utility::NoOpHasher respectively.

This eliminates all the unsafe code, increases parallelism, and reduces the number of queries to just two.

crates/bevy_animation/src/lib.rs

NthTensor

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

mockersf · 2024-02-05T05:58:03Z

on the many_foxes examples, this PR is slower than main

pcwalton · 2024-02-05T06:00:08Z

I would expect the problem to be #11711. I also haven't looked at performance at all.

pcwalton · 2024-02-05T06:02:57Z

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.

pcwalton · 2024-02-05T06:15:37Z

I've verified that with #11711 applied on top of this, the many_foxes example is faster than main with this PR. Without #11711, this PR alone is indeed slower because of all the hashing.

djeedai

Quite a few comments but overall looks very good, easier to read and more maintainable. Thanks!

crates/bevy_animation/src/lib.rs

NthTensor

Nit: Naming could be more consistent. Render target or bone, it's one or the other. Are we going to talk about paths on functions that now accept render target ids?

I'm pretty much sold.

crates/bevy_animation/src/lib.rs

mockersf

could you restore the behaviour of the animated_transform example?

mockersf · 2024-02-09T20:09:10Z

example morph_targets is not working with this PR. pcwalton#1 fixes it but breaks one of the assumptions of the PR

mockersf · 2024-02-09T20:45:09Z

https://github.com/KhronosGroup/glTF-Sample-Assets/tree/main/Models/InterpolationTest doesn't work either with this PR

entity

pcwalton · 2024-02-10T02:54:31Z

OK, I allowed AnimationTarget and AnimationPlayer to coexist on the same entity. The fact that they couldn't is a relic of when I was using EntityMut on AnimationTargets, so they would lock out the AnimationPlayers. This is no longer necessary.

NthTensor · 2024-02-14T17:35:22Z

Both animated_transform and morph_targets seem to be working. @mockersf

mockersf · 2024-02-14T19:22:34Z

Both animated_transform and morph_targets seem to be working. @mockersf

animated_transform doesn't behave as before and https://github.com/KhronosGroup/glTF-Sample-Assets/tree/main/Models/InterpolationTest doesn't work

pcwalton · 2024-02-19T01:44:18Z

@mockersf You were running InterpolationTest through scene_viewer, right? Because the way that scene_viewer plays animations is pretty broken, and the fact that we now use multiple AnimationPlayers in this PR broke it. I'll fix it as part of this PR.

Both of those issues are problems with the examples, not with the Bevy logic.

pcwalton · 2024-02-19T03:18:29Z

Those should be working now and should be ready for review and merging afterward now that 0.13 has shipped.

mockersf · 2024-02-19T14:21:52Z

@mockersf You were running InterpolationTest through scene_viewer, right?

Yup!

Because the way that scene_viewer plays animations is pretty broken, and the fact that we now use multiple AnimationPlayers in this PR broke it. I'll fix it as part of this PR.

Thanks! It's good practice to fix the examples with the PR that breaks them, it avoids having to investigate without the context.

# 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 `EntityHash` and related types were moved from `bevy_utils` to `bevy_ecs` in #11498, but seemed to have been accidentally reintroduced a week later in #11707. ## Solution Remove the old leftover code. --- ## Migration Guide - Uses of `bevy::utils::{EntityHash, EntityHasher, EntityHashMap, EntityHashSet}` now have to be imported from `bevy::ecs::entity`.

Rework animation to be done in two phases.

a17b396

This eliminates all the unsafe code, increases parallelism, and reduces the number of queries to just two.

NthTensor reviewed Feb 5, 2024

View reviewed changes

crates/bevy_animation/src/lib.rs Outdated Show resolved Hide resolved

Remove AnimationTarget from disconnected subtrees

7fc17b1

NthTensor reviewed Feb 5, 2024

View reviewed changes

Fix some missing cases in change detection

c2548b5

pcwalton mentioned this pull request Feb 5, 2024

Intern entity paths for faster comparisons. #11711

Closed

djeedai reviewed Feb 5, 2024

View reviewed changes

alice-i-cecile added C-Performance A change motivated by improving speed, memory usage or compile times C-Code-Quality A section of code that is hard to understand or change A-Animation Make things move and change over time labels Feb 5, 2024

pcwalton added 2 commits February 5, 2024 14:36

Switch to using UUIDs for bones

56bed0d

Merge remote-tracking branch 'origin/main' into two-phase-animation

af2e114

NthTensor reviewed Feb 5, 2024

View reviewed changes

crates/bevy_animation/src/lib.rs Outdated Show resolved Hide resolved

NthTensor reviewed Feb 5, 2024

View reviewed changes

alice-i-cecile added this to the 0.14 milestone Feb 5, 2024

pcwalton added 4 commits February 5, 2024 16:29

Write documentation and use no-op hashing

39aff53

No bevy_internal usage

4392fa7

Address review comment

65c3f2e

Rename update_animations to advance_animations

6d28e75

NthTensor reviewed Feb 6, 2024

View reviewed changes

crates/bevy_animation/src/lib.rs Outdated Show resolved Hide resolved

NthTensor reviewed Feb 6, 2024

View reviewed changes

crates/bevy_animation/src/lib.rs Show resolved Hide resolved

pcwalton added 2 commits February 5, 2024 17:05

Address review comments

ee92b8f

Address review comment

b08435c

pcwalton marked this pull request as ready for review February 6, 2024 01:09

james7132 added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help labels Feb 8, 2024

mockersf requested changes Feb 9, 2024

View reviewed changes

mockersf removed the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Feb 9, 2024

pcwalton added 2 commits February 9, 2024 18:08

Merge remote-tracking branch 'origin/main' into two-phase-animation

dfd7819

Allow AnimationPlayer and AnimationTarget to coexist on the same

ee7154e

entity

pcwalton added 2 commits February 18, 2024 16:39

Merge remote-tracking branch 'origin/main' into two-phase-animation

8958770

Make animated_transform behave as before

3928dcb

Update the scene viewer. This fixes the InterpolationTest.

26f1fb1

pcwalton added 2 commits February 19, 2024 01:29

Merge remote-tracking branch 'origin/main' into two-phase-animation

4a9ee8e

Clippy police

7fc0421

pcwalton requested a review from mockersf February 19, 2024 09:41

mockersf approved these changes Feb 19, 2024

View reviewed changes

mockersf added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Feb 19, 2024

mockersf added this pull request to the merge queue Feb 19, 2024

Merged via the queue into bevyengine:main with commit 5f1dd39 Feb 19, 2024
27 of 28 checks passed

james7132 mentioned this pull request Mar 29, 2024

Add NameLookup Type to make looking up entities by name easier #5976

Closed

ItsDoot mentioned this pull request Sep 4, 2024

Remove remnant EntityHash and related types from bevy_utils #15039

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework animation to be done in two phases. #11707

Rework animation to be done in two phases. #11707

pcwalton commented Feb 5, 2024 •

edited

Loading

NthTensor left a comment •

edited

Loading

mockersf commented Feb 5, 2024

pcwalton commented Feb 5, 2024

pcwalton commented Feb 5, 2024

pcwalton commented Feb 5, 2024

djeedai left a comment

NthTensor left a comment

mockersf left a comment

mockersf commented Feb 9, 2024

mockersf commented Feb 9, 2024

pcwalton commented Feb 10, 2024

NthTensor commented Feb 14, 2024

mockersf commented Feb 14, 2024

pcwalton commented Feb 19, 2024 •

edited

Loading

pcwalton commented Feb 19, 2024 •

edited

Loading

mockersf commented Feb 19, 2024

Rework animation to be done in two phases. #11707

Rework animation to be done in two phases. #11707

Conversation

pcwalton commented Feb 5, 2024 • edited Loading

Objective

Solution

Migration guide

NthTensor left a comment • edited Loading

Choose a reason for hiding this comment

mockersf commented Feb 5, 2024

pcwalton commented Feb 5, 2024

pcwalton commented Feb 5, 2024

pcwalton commented Feb 5, 2024

djeedai left a comment

Choose a reason for hiding this comment

NthTensor left a comment

Choose a reason for hiding this comment

mockersf left a comment

Choose a reason for hiding this comment

mockersf commented Feb 9, 2024

mockersf commented Feb 9, 2024

pcwalton commented Feb 10, 2024

NthTensor commented Feb 14, 2024

mockersf commented Feb 14, 2024

pcwalton commented Feb 19, 2024 • edited Loading

pcwalton commented Feb 19, 2024 • edited Loading

mockersf commented Feb 19, 2024

pcwalton commented Feb 5, 2024 •

edited

Loading

NthTensor left a comment •

edited

Loading

pcwalton commented Feb 19, 2024 •

edited

Loading

pcwalton commented Feb 19, 2024 •

edited

Loading