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

Impose a more sensible ordering for animation graph evaluation. #15589

Merged

Conversation

pcwalton
Copy link
Contributor

@pcwalton pcwalton commented Oct 2, 2024

This is an updated version of #15530. Review comments were addressed.

This commit changes the animation graph evaluation to be operate in a more sensible order and updates the semantics of blend nodes to conform to the animation composition RFC. Prior to this patch, a node graph like this:

	    ┌─────┐
	    │     │
	    │  1  │
	    │     │
	    └──┬──┘
	       │
       ┌───────┴───────┐
       │               │
       ▼               ▼
    ┌─────┐         ┌─────┐
    │     │         │     │
    │  2  │         │  3  │
    │     │         │     │
    └──┬──┘         └──┬──┘
       │               │
   ┌───┴───┐       ┌───┴───┐
   │       │       │       │
   ▼       ▼       ▼       ▼
┌─────┐ ┌─────┐ ┌─────┐ ┌─────┐
│     │ │     │ │     │ │     │
│  4  │ │  6  │ │  5  │ │  7  │
│     │ │     │ │     │ │     │
└─────┘ └─────┘ └─────┘ └─────┘

Would be evaluated as (((4 ⊕ 5) ⊕ 6) ⊕ 7), with the blend (lerp/slerp) operation notated as ⊕. As quaternion multiplication isn't commutative, this is very counterintuitive and will especially lead to trouble with the forthcoming additive blending feature (#15198).

This patch fixes the issue by changing the evaluation order to postorder, with children of a node evaluated in ascending order by node index.

To do so, this patch revamps AnimationCurve to be based on an evaluation stack and a blend register. During target evaluation, the graph evaluator traverses the graph in postorder. When encountering a clip node, the evaluator pushes the possibly-interpolated value onto the evaluation stack. When encountering a blend node, the evaluator pops values off the stack into the blend register, accumulating weights as appropriate. When the graph is completely evaluated, the top element on the stack is committed to the property of the component.

A new system, the graph threading system, is added in order to cache the sorted postorder traversal to avoid the overhead of sorting children at animation evaluation time. Mask evaluation has been moved to this system so that the graph only has to be traversed at most once per frame. Unlike the ActiveAnimation list, the threaded graph is cached from frame to frame and only has to be regenerated when the animation graph asset changes.

This patch currently regresses the animate_target performance in many_foxes by around 50%, resulting in an FPS loss of about 2-3 FPS. I'd argue that this is an acceptable price to pay for a much more intuitive system. In the future, we can mitigate the regression with a fast path that avoids consulting the graph if only one animation is playing. However, in the interest of keeping this patch simple, I didn't do so here.

Objective

  • Describe the objective or issue this PR addresses.
  • If you're fixing a specific issue, say "Fixes #X".

Solution

  • Describe the solution used to achieve the objective above.

Testing

  • Did you test these changes? If so, how?
  • Are there any parts that need more testing?
  • How can other people (reviewers) test your changes? Is there anything specific they need to know?
  • If relevant, what platforms did you test these changes on, and are there any important ones you can't test?

Showcase

This section is optional. If this PR does not include a visual change or does not add a new feature, you can delete this section.

  • Help others understand the result of this PR by showcasing your awesome work!
  • If this PR adds a new feature or public API, consider adding a brief pseudo-code snippet of it in action
  • If this PR includes a visual change, consider adding a screenshot, GIF, or video
    • If you want, you could even include a before/after comparison!
  • If the Migration Guide adequately covers the changes, you can delete this section

While a showcase should aim to be brief and digestible, you can use a toggleable section to save space on longer showcases:

Click to view showcase
println!("My super cool code.");

Migration Guide

This section is optional. If there are no breaking changes, you can delete this section.

  • If this PR is a breaking change (relative to the last release of Bevy), describe how a user might need to migrate their code to support these changes
  • Simply adding new functionality is not a breaking change.
  • Fixing behavior that was definitely a bug, rather than a questionable design choice is not a breaking change.

@pcwalton
Copy link
Contributor Author

pcwalton commented Oct 2, 2024

r? @aecsocket

@pcwalton pcwalton added this to the 0.15 milestone Oct 2, 2024
@pcwalton pcwalton added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Animation Make things move and change over time C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 2, 2024
@pcwalton pcwalton force-pushed the animation-graph-ordering-reform branch from 07542f9 to 576d2dd Compare October 2, 2024 06:11
This commit changes the animation graph evaluation to be operate in a more
sensible order and updates the semantics of blend nodes to conform to [the
animation composition RFC]. Prior to this patch, a node graph like this:

	    ┌─────┐
	    │     │
	    │  1  │
	    │     │
	    └──┬──┘
	       │
       ┌───────┴───────┐
       │               │
       ▼               ▼
    ┌─────┐         ┌─────┐
    │     │         │     │
    │  2  │         │  3  │
    │     │         │     │
    └──┬──┘         └──┬──┘
       │               │
   ┌───┴───┐       ┌───┴───┐
   │       │       │       │
   ▼       ▼       ▼       ▼
┌─────┐ ┌─────┐ ┌─────┐ ┌─────┐
│     │ │     │ │     │ │     │
│  4  │ │  6  │ │  5  │ │  7  │
│     │ │     │ │     │ │     │
└─────┘ └─────┘ └─────┘ └─────┘

Would be evaluated as (((4 ⊕ 5) ⊕ 6) ⊕ 7), with the blend (lerp/slerp)
operation notated as ⊕. As quaternion multiplication isn't commutative, this is
very counterintuitive and will especially lead to trouble with the forthcoming
additive blending feature (bevyengine#15198).

This patch fixes the issue by changing the evaluation order to postorder, with
children of a node evaluated in ascending order by node index.

To do so, this patch revamps `AnimationCurve` to be based on an *evaluation
stack* and a *blend register*. During target evaluation, the graph evaluator
traverses the graph in postorder. When encountering a clip node, the evaluator
pushes the possibly-interpolated value onto the evaluation stack. When
encountering a blend node, the evaluator pops values off the stack into the
blend register, accumulating weights as appropriate. When the graph is
completely evaluated, the top element on the stack is *committed* to the
property of the component.

A new system, the *graph threading* system, is added in order to cache the
sorted postorder traversal to avoid the overhead of sorting children at
animation evaluation time. Mask evaluation has been moved to this system so
that the graph only has to be traversed at most once per frame. Unlike the
`ActiveAnimation` list, the *threaded graph* is cached from frame to frame and
only has to be regenerated when the animation graph asset changes.

This patch currently regresses the `animate_target` performance in `many_foxes`
by around 50%, resulting in an FPS loss of about 2-3 FPS.  I'd argue that this
is an acceptable price to pay for a much more intuitive system. In the future,
we can mitigate the regression with a fast path that avoids consulting the
graph if only one animation is playing. However, in the interest of keeping
this patch simple, I didn't do so here.

[the animation composition RFC]: https://github.com/bevyengine/rfcs/blob/main/rfcs/51-animation-composition.md
@pcwalton pcwalton force-pushed the animation-graph-ordering-reform branch from 576d2dd to e248239 Compare October 2, 2024 08:02
@mweatherley
Copy link
Contributor

mweatherley commented Oct 2, 2024

This is great work. Unfortunately, I think the approach in AnimationEvaluationState of using the curve types to key the evaluators is also erroneous because of the introduction of generics in the move, e.g. TranslationKeyframes -> TranslationCurve<C>. That is, previously it was always the case that TranslationKeyframes was a single type, but now TranslationCurve<C> is a family of types, which means that if two different curve types are driving translation in the same animation graph, they will each receive separate evaluators. (Let me know if my understanding of this is incorrect.)

Here is a sketch of how things might be restructured to fix this:

  1. Key the maps in AnimationEvaluationState on the AnimatableProperty types instead of curve types. This is more correct and more semantically meaningful. (Continue for details about how this interacts with Transform.)
  2. Move the dispatch of boxed AnimationCurveEvaluators from AnimationCurve to AnimatableProperty. Note that, because all special Transform logic now lives in the evaluators, this also means that Transform properties can simply become named properties that we export (e.g. TransformTranslation or something, with TranslationCurve<C> becoming simply an alias for AnimatablePropertyCurve<C, TransformTranslation>. We can probably also make this an automatic method that defaults to the standard evaluator for AnimatableProperty types.
  3. Remove all type generics of curves from AnimationCurveEvaluator types.
  4. To support the previous, augment AnimationCurve with the following:
    i. A method animated_property_id that returns the TypeId of the AnimatableProperty that the curve animates. This allows the correct evaluator to be dispatched indirectly.
    ii. A method which allows the curve to be sampled dynamically given extrinsic knowledge of its output type:
    /// Downcast to a curve with output type `T` by reference. Fails unless the
    /// contained curve is of output type `T`.
    fn downcast_curve_ref<T: Any>(&self) -> Option<&dyn Curve<T>>;
    This allows evaluators to operate on dyn AnimationCurve as they do now without knowing the intermediate curve type, which is imperative for this whole idea to work.
  5. To support (4)(ii), the Curve library should export a safe wrapper that allows this downcasting to be actually implemented. Concretely, the implementations would look something like this (e.g. for translations):
if TypeId::of::<Vec3>() == TypeId::of::<T>() {
    unsafe {
        let inner: &dyn Curve<Vec3> = &self.0;
        mem::transmute(Some(inner))
    }
} else {
    None
}

I'm pretty sure this is sound just because you're transmuting something to the type that it already is. To my knowledge, there is no way to do this without a hint of unsafe Rust.

Of course, this also goes far beyond just "rebasing". If you agree with this approach and want me to work on this and submit it either as a PR on your fork or as a PR on Bevy adopting this, I'd be more than happy to.

EDIT: I realize now that the above approach can't work verbatim because it invalidates the object-safety of AnimationCurve. I'll think about it a little more.

Copy link
Contributor

@mweatherley mweatherley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(See above)

crates/bevy_animation/src/animation_curves.rs Outdated Show resolved Hide resolved
@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 2, 2024
@pcwalton pcwalton added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Oct 2, 2024
Copy link
Contributor

@mweatherley mweatherley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the concerns about evaluator mapping addressed, this is excellent. The animation graph threading organizes animate_targets and advance_animations much better, as well.

};
match animation_graph_node.clip {
None => {
// This is a blend node.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// This is a blend node.
// This is a blend node. Note that the threaded traversal order guarantees that each
// `AnimationCurveEvaluator` required to blend inputs at this node will have already been
// added in the other match branch.

Copy link
Contributor Author

@pcwalton pcwalton Oct 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't actually true because if all the weights we've seen thus far are zero then the AnimationCurveEvaluator won't have been created yet. In that case we do nothing, which is the correct behavior.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! That's good to know.

@alice-i-cecile
Copy link
Member

Great docs and well motivated. Once you and @mweatherley decide what to do with TransformCurve (see #15598) and resolve the merge conflicts, ping me and I'll merge this in ASAP.

@alice-i-cecile
Copy link
Member

Yeah you can just delete the entirety of TransformCurve and TransformCurveEvaluator afaik

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Oct 3, 2024
Merged via the queue into bevyengine:main with commit ca8dd06 Oct 3, 2024
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Animation Make things move and change over time C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants