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

Implement the AnimationGraph, allowing for multiple animations to be blended together. #11989

Merged
merged 44 commits into from
Mar 7, 2024

Conversation

pcwalton
Copy link
Contributor

@pcwalton pcwalton commented Feb 20, 2024

This is an implementation of RFC #51: https://github.com/bevyengine/rfcs/blob/main/rfcs/51-animation-composition.md

Note that the implementation strategy is different from the one outlined in that RFC, because two-phase animation has now landed.

Objective

Bevy needs animation blending. The RFC for this is RFC 51.

Solution

This is an implementation of the RFC. Note that the implementation strategy is different from the one outlined there, because two-phase animation has now landed.

This is just a draft to get the conversation started. Currently we're missing a few things:

  • A fully-fleshed-out mechanism for transitions
  • A serialization format for AnimationGraphs
  • Examples are broken, other than animated_fox
  • Documentation

Changelog

Added

  • The AnimationPlayer has been reworked to support blending multiple animations together through an AnimationGraph, and as such will no longer function unless a Handle<AnimationGraph> has been added to the entity containing the player. See RFC 51 for more details.

  • Transition functionality has moved from the AnimationPlayer to a new component, AnimationTransitions, which works in tandem with the AnimationGraph.

Migration Guide

  • AnimationPlayers can no longer play animations by themselves and need to be paired with a Handle<AnimationGraph>. Code that was using AnimationPlayer to play animations will need to create an AnimationGraph asset first, add a node for the clip (or clips) you want to play, and then supply the index of that node to the AnimationPlayer's play method.

  • The AnimationPlayer::play_with_transition() method has been removed and replaced with the AnimationTransitions component. If you were previously using AnimationPlayer::play_with_transition(), add all animations that you were playing to the AnimationGraph, and create an AnimationTransitions component to manage the blending between them.

blended together.

This is an implementation of RFC bevyengine#51: https://github.com/bevyengine/rfcs/blob/main/rfcs/51-animation-composition.md

Note that the implementation strategy is different from the one outlined
in that RFC, because two-phase animation has now landed.
@james7132 james7132 self-requested a review February 20, 2024 06:14
@alice-i-cecile alice-i-cecile added C-Enhancement A new feature A-Editor Graphical tools to make Bevy games X-Controversial There is active debate or serious implications around merging this PR labels Feb 20, 2024
@pcwalton pcwalton removed the X-Controversial There is active debate or serious implications around merging this PR label Feb 20, 2024
@pcwalton
Copy link
Contributor Author

I removed the Controversial label as this is implementing an approved RFC.

@alice-i-cecile alice-i-cecile added the D-Complex Quite challenging from either a design or technical perspective. Ask for help! label Feb 20, 2024
Copy link
Contributor

@djeedai djeedai left a comment

Choose a reason for hiding this comment

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

Looks good overall, no special concern or comment

crates/bevy_animation/src/graph.rs Outdated Show resolved Hide resolved
crates/bevy_animation/src/graph.rs Outdated Show resolved Hide resolved
Copy link
Contributor

The generated examples/README.md is out of sync with the example metadata in Cargo.toml or the example readme template. Please run cargo run -p build-templated-pages -- update examples to update it, and commit the file change.

1 similar comment
Copy link
Contributor

The generated examples/README.md is out of sync with the example metadata in Cargo.toml or the example readme template. Please run cargo run -p build-templated-pages -- update examples to update it, and commit the file change.

Copy link
Contributor

The generated examples/README.md is out of sync with the example metadata in Cargo.toml or the example readme template. Please run cargo run -p build-templated-pages -- update examples to update it, and commit the file change.

@pcwalton pcwalton requested a review from djeedai March 1, 2024 09:27
@pcwalton pcwalton marked this pull request as ready for review March 5, 2024 05:29
@pcwalton
Copy link
Contributor Author

pcwalton commented Mar 5, 2024

OK, I'm much happier with this. I've solved the problems of animation ordering and removed the "scheduled animation" overhead. This should be ready for re-review.

@pcwalton
Copy link
Contributor Author

pcwalton commented Mar 6, 2024

Improved the example a bit, thanks to @rparrett.

Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

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

I'm overall happy with the current state of this PR. Given the size of the PR, I'm opting to skip the smaller nits and opt to iteratively improve upon the implementation.

crates/bevy_animation/src/lib.rs Show resolved Hide resolved
crates/bevy_animation/src/lib.rs Show resolved Hide resolved
crates/bevy_animation/src/graph.rs Show resolved Hide resolved
@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it C-Needs-Release-Note Work that should be called out in the blog due to impact labels Mar 7, 2024
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Really pleased with the documentation and general care shown here. I'm content with the level of review from subject matter experts, and the examples appear to work correctly for me.

There's definitely some general-purpose abstractions missing on the asset management side, but I'm content with this for now. It'll be useful to have this in place to help us figure out a design there.

However, I'm getting a bunch of warnings like:

2024-03-07T15:32:36.708431Z WARN bevy_animation: Couldn't find the animation player 53v1 for the target entity 49v1 (Some("b_LeftLeg02_016"))

on both the animated_fox and animation_graph examples. This doesn't appear to be present on main. I suspect a system ordering problem.

Once those are fixed up I'm happy to approve and merge.

@rparrett
Copy link
Contributor

rparrett commented Mar 7, 2024

2024-03-07T15:32:36.708431Z WARN bevy_animation: Couldn't find the animation player 53v1 for the target entity 49v1 (Some("b_LeftLeg02_016"))

These started showing up after 283a4d0.

I have some more ideas for the examples, but I will follow up later. They're definitely in a good enough state.

So "approved," but I only looked at the examples.

and is harmless, and fix the system order in the examples
@pcwalton
Copy link
Contributor Author

pcwalton commented Mar 7, 2024

I demoted the warnings to traces because they're relatively common and harmless now. They'll occur whenever you load a glTF and haven't added an AnimationGraph. glTF has no concept of animation blending, so Bevy has no way to figure out what you want, and you have to add it manually. Until you do, animations won't play. This is documented in the migration guide. (It might be convenient to have some sort of default AnimationGraph, but that can be a followup.)

I also switched the system order to hopefully avoid 1-frame lag (and avoid those traces) in the examples.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Mar 7, 2024
Merged via the queue into bevyengine:main with commit dfdf2b9 Mar 7, 2024
29 checks passed
spectria-limina pushed a commit to spectria-limina/bevy that referenced this pull request Mar 9, 2024
…e blended together. (bevyengine#11989)

This is an implementation of RFC bevyengine#51:
https://github.com/bevyengine/rfcs/blob/main/rfcs/51-animation-composition.md

Note that the implementation strategy is different from the one outlined
in that RFC, because two-phase animation has now landed.

# Objective

Bevy needs animation blending. The RFC for this is [RFC 51].

## Solution

This is an implementation of the RFC. Note that the implementation
strategy is different from the one outlined there, because two-phase
animation has now landed.

This is just a draft to get the conversation started. Currently we're
missing a few things:

- [x] A fully-fleshed-out mechanism for transitions
- [x] A serialization format for `AnimationGraph`s
- [x] Examples are broken, other than `animated_fox`
- [x] Documentation

---

## Changelog

### Added

* The `AnimationPlayer` has been reworked to support blending multiple
animations together through an `AnimationGraph`, and as such will no
longer function unless a `Handle<AnimationGraph>` has been added to the
entity containing the player. See [RFC 51] for more details.

* Transition functionality has moved from the `AnimationPlayer` to a new
component, `AnimationTransitions`, which works in tandem with the
`AnimationGraph`.

## Migration Guide

* `AnimationPlayer`s can no longer play animations by themselves and
need to be paired with a `Handle<AnimationGraph>`. Code that was using
`AnimationPlayer` to play animations will need to create an
`AnimationGraph` asset first, add a node for the clip (or clips) you
want to play, and then supply the index of that node to the
`AnimationPlayer`'s `play` method.

* The `AnimationPlayer::play_with_transition()` method has been removed
and replaced with the `AnimationTransitions` component. If you were
previously using `AnimationPlayer::play_with_transition()`, add all
animations that you were playing to the `AnimationGraph`, and create an
`AnimationTransitions` component to manage the blending between them.

[RFC 51]:
https://github.com/bevyengine/rfcs/blob/main/rfcs/51-animation-composition.md

---------

Co-authored-by: Rob Parrett <robparrett@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Editor Graphical tools to make Bevy games C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide C-Enhancement A new feature C-Needs-Release-Note Work that should be called out in the blog due to impact D-Complex Quite challenging from either a design or technical perspective. Ask for help! S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants