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

Animation Primitives #49

Draft
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

james7132
Copy link
Member

@james7132 james7132 commented Feb 11, 2022

RENDERED

This RFC details the absolute bare minimum viable API for setting up a flexible and extendable property based animation system in Bevy... or at least part of it. This gets probably about 1/3 of the way there.

@alice-i-cecile
Copy link
Member

Ping @lassade; I'm very curious on your opinions on this if you have the time and interest.

@james7132 james7132 marked this pull request as draft February 11, 2022 21:28
james7132 and others added 3 commits February 11, 2022 13:39
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
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.

Generally I'm very impressed. These are good abstractions that are powerful, flexible and build on each other.

rfcs/49-animation-primitives.md Show resolved Hide resolved
rfcs/49-animation-primitives.md Outdated Show resolved Hide resolved
rfcs/49-animation-primitives.md Show resolved Hide resolved
rfcs/49-animation-primitives.md Show resolved Hide resolved
rfcs/49-animation-primitives.md Outdated Show resolved Hide resolved
rfcs/49-animation-primitives.md Outdated Show resolved Hide resolved
rfcs/49-animation-primitives.md Outdated Show resolved Hide resolved
rfcs/49-animation-primitives.md Outdated Show resolved Hide resolved
rfcs/49-animation-primitives.md Outdated Show resolved Hide resolved
rfcs/49-animation-primitives.md Outdated Show resolved Hide resolved
james7132 and others added 3 commits February 11, 2022 13:55
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
@lassade
Copy link

lassade commented Feb 11, 2022

a few notes

  1. Not sure how curves and sampling could be decoupled into to separated traits, to me they are the same;
  2. Sampling also involves how data should be interpolated: step, linear and catmul-row; I propose to have 3 different types of curves like in my original implementation;
  3. For Quaternions there is a difference between lerp vs slerp, other than that generally the type it self defines how interpolate between values. You will almost never will want to use slerp by the way;
  4. Color interpolation will be a nightmare If the Colors is defined as a enum that supports sRGB, LinSRGB, HSL and others, because each color space gives a different interpolation result; If you let the user choose the interpolation color space it might have a heavy performance cost, probably many times greater than using slerp;
  5. Sampling should include some form of cursor (cache locality, think a curve with many keyframes) and should and use binary search (faster keyframe search). But you should only use the latter to make a simpler initial implementation;

@lassade
Copy link

lassade commented Feb 12, 2022

In the storage domain you should include how streamed data will be handled and at mention available animation formats namely ACL (https://github.com/nfrechette/acl) in the prior art.

@james7132
Copy link
Member Author

james7132 commented Feb 20, 2022

Almost completely revised the RFC end to end:

  • Removed the mention of animation graph and mixing entirely, going to have a separate RFC for this
  • Dropped the idea of Sample<T>, and instead opting for defining the base characteristics required for an animatable property instead, which will be the base for the composition API
  • Dove deeper into the implementation details and memory layout.
  • Addressed review comments from @alice-i-cecile and @lassade

Still quite a few TODOs, namely discussing a strategy for implementing AnimationClip and it's serialization.


To respond to @lassade's comments (next time please leave them inline so I can address them 1-by-1 instead of a giant block like this).

  1. Not sure how curves and sampling could be decoupled into to separated traits, to me they are the same;

Good point, retracted that idea after discussing this with you on Discord and reading your Animator implementation. I've changed it now to instead have a trait on the sampled value named Animatable, which takes your Lerp and logically extends it one step further for allowing anyone to make their own blendable value. This will be used to blend sampled values from curves the next Animation Graph attempt I'm testing instead of relying on generic nodes as previously proposed. This should remove generics from everywhere except for the Curve<T> trait itself.

  1. Sampling also involves how data should be interpolated: step, linear and catmul-row; I propose to have 3 different types of curves like in my original implementation;

I'm tentatively addressing this by trying to use a newtype and implement Animatable on it instead. Having T be a generic parameter to Curve instead of an associated type actually allows us to have multiple Curve<T> implementations for a given type, and we can unwrap the newtype in a blanket impl if we need to. Having multiple curve types was originally always in the plan, just didn't have time to write down the full details, but if the underlying structure of the curve does not change fundamentally change, but the interpolation behavior changes, we can just newtype the value type instead. There's an argument to be made here about SoA vs AoS in wrapping values like this, which is why the CurveVariable in your implementation is the way it is, but I'm not sure if SoA is the answer here, particularly since we're not speed-iterating through the curve, but occasionally sampling a value from it, sometimes randomly.

  1. For Quaternions there is a difference between lerp vs slerp, other than that generally the type it self defines how interpolate between values. You will almost never will want to use slerp by the way;

This can be addressed by the newtype pattern that I mentioned above. Though I'm curious why we wouldn't want to use slerp other than performance.

  1. Color interpolation will be a nightmare If the Colors is defined as a enum that supports sRGB, LinSRGB, HSL and others, because each color space gives a different interpolation result; If you let the user choose the interpolation color space it might have a heavy performance cost, probably many times greater than using slerp;

Right, and that's sort of an implementation detail that we need to address, and is sort of separate from the design detailed here. I'm well aware this is an issue, but it's something we need to resolve in general, not just for animation.

  1. Sampling should include some form of cursor (cache locality, think a curve with many keyframes) and should and use binary search (faster keyframe search). But you should only use the latter to make a simpler initial implementation;

I added a section on this, though I'm not 100% sold on it. After reading your implementation, I get the distinct impression that this very heavily complicates the implementation as the cursor must be mutably saved somewhere, and you might have the same curve/clip being sampled in different locations by multiple entities (or even within the same entity). That's not to say I think we should definitely not add it in, but it does seem like an optimization we can add in after we deliver a MVP on this.

In the storage domain you should include how streamed data will be handled and at mention available animation formats namely ACL (https://github.com/nfrechette/acl) in the prior art.

Great point! Added that a mention of ACL in. I need to rewrite that section to mention the other efforts in the Rust community and other public literature about the approaches to this.

bors bot pushed a commit to bevyengine/bevy that referenced this pull request Apr 2, 2022
# Objective

- Add a basic animation player
  - Single track
  - Not generic, can only animate `Transform`s
  - With plenty of possible optimisations available
  - Close-ish to bevyengine/rfcs#49
- https://discord.com/channels/691052431525675048/774027865020039209/958820063148929064

## Solution

- Can play animations
  - looping or not
- Can pause animations
- Can seek in animation
- Can alter speed of animation
- I also removed the previous gltf animation example

https://user-images.githubusercontent.com/8672791/161051887-e79283f0-9803-448a-93d0-5f7a62acb02d.mp4
Copy link

@Weibye Weibye left a comment

Choose a reason for hiding this comment

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

To me this looks like flexible and powerful primitives to build on top of. I particularly like how this would build animation in bevy from the ground up to be able to animate anything and not just skeletal animation.

  • Obvious outstanding TODOs
  • Don't feel I have enough experience to give feedback on the suggested technical implementation details
  • It is clearly and concisely written, so even with limited technical knowledge it is easy to read and understand

Overall this is great work, thank you!

@ValorZard
Copy link

Looking at the curve system as explained in the RFC, it reminds me of how Godot does animations, specifically how Godot lets you animate literally everything including UI all using the same UI graph. I'm wondering, will the Curve syntax work for everything, or only for certain things like skeletal animation?

aevyrie pushed a commit to aevyrie/bevy that referenced this pull request Jun 7, 2022
# Objective

- Add a basic animation player
  - Single track
  - Not generic, can only animate `Transform`s
  - With plenty of possible optimisations available
  - Close-ish to bevyengine/rfcs#49
- https://discord.com/channels/691052431525675048/774027865020039209/958820063148929064

## Solution

- Can play animations
  - looping or not
- Can pause animations
- Can seek in animation
- Can alter speed of animation
- I also removed the previous gltf animation example

https://user-images.githubusercontent.com/8672791/161051887-e79283f0-9803-448a-93d0-5f7a62acb02d.mp4
Copy link
Member

@Aceeri Aceeri left a comment

Choose a reason for hiding this comment

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

I like the concept, I'm going to think about it for a bit but here are a couple of nitpick-y grammar/etc.

A curve always covers the time range of `[0, duration]`, but must return valid
values when sampled at any non-negative time. What a curve does beyond this range
is implementation specifc. This typically will just return the value that would
be sampled at time 0 or `duration`, whichever is closer, but it doesn't have to
Copy link
Member

Choose a reason for hiding this comment

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

If it can only be sampled with non-negative time then wouldn't it only be clamped to duration here?

buffer like `Vec<T>`. However, it stores the exact keyframe times in a parallel
`Vec<T>`, allowing for the time between keyframes to be variable. This comes at a
cost: sampling the curve requires performing a binary search to find the
surroudning keyframe before sampling a value from the curve itself. This can
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
surroudning keyframe before sampling a value from the curve itself. This can
surrounding keyframe before sampling a value from the curve itself. This can

of keyframes and the representation of said keyframes, and a typical game with
multiple character animations may several hundreds or thousands of curves to
sample from. To both help cut down on memory usage and minimize cache misses,
it's useful to compress the most usecases seen by a
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
it's useful to compress the most usecases seen by a
it's useful to compress the usecases most likely to be seen by a user.

A rough outline of how such a `PropertyPath` struct might look like:

```rust
#[derive(Debug, Clone, PartialEq, Eq, Hash, Ord, ParitialOrd)]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#[derive(Debug, Clone, PartialEq, Eq, Hash, Ord, ParitialOrd)]
#[derive(Debug, Clone, PartialEq, Eq, Hash, Ord, PartialOrd)]

parts: Box<[Name]>,
}

#[derive(Debug, Clone, PartialEq, Eq, Hash, Ord, ParitialOrd)]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#[derive(Debug, Clone, PartialEq, Eq, Hash, Ord, ParitialOrd)]
#[derive(Debug, Clone, PartialEq, Eq, Hash, Ord, PartialOrd)]

PropertyPaths are written out as raw strings, and parsed on deserialization.

### `AnimationClip`
A `AnimationClip` is a serializable asset type that encompasses a mapping of
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
A `AnimationClip` is a serializable asset type that encompasses a mapping of
An `AnimationClip` is a serializable asset type that encompasses a mapping of

Bevy absolutely needs some form of a first-party animation system, no modern game
engine can be called production ready without one. Having this exist solely as a
third-party ecosystem crate is unacceptable as it would promote a
facturing of the ecosystem with multiple incompatible baseline animation system
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
facturing of the ecosystem with multiple incompatible baseline animation system
fracturing of the ecosystem with multiple incompatible baseline animation system


## Unresolved questions
- Can we safely interleave multiple curves together so that we do not incur
mulitple cache misses when sampling from animation clips?
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
mulitple cache misses when sampling from animation clips?
multiple cache misses when sampling from animation clips?

bors bot pushed a commit to bevyengine/bevy that referenced this pull request Jan 9, 2023
# Objective

- Fixes #6338

This PR allows for smooth transitions between different animations.

## Solution

- This PR uses very simple linear blending of animations.
- When starting a new animation, you can give it a duration, and throughout that duration, the previous and the new animation are being linearly blended, until only the new animation is running.
- I'm aware of bevyengine/rfcs#49 and bevyengine/rfcs#51, which are more complete solutions to this problem, but they seem still far from being implemented. Until they're ready, this PR allows for the most basic use case of blending, i.e. smoothly transitioning between different animations.

## Migration Guide

- no bc breaking changes
james7132 pushed a commit to james7132/bevy that referenced this pull request Jan 21, 2023
# Objective

- Fixes bevyengine#6338

This PR allows for smooth transitions between different animations.

## Solution

- This PR uses very simple linear blending of animations.
- When starting a new animation, you can give it a duration, and throughout that duration, the previous and the new animation are being linearly blended, until only the new animation is running.
- I'm aware of bevyengine/rfcs#49 and bevyengine/rfcs#51, which are more complete solutions to this problem, but they seem still far from being implemented. Until they're ready, this PR allows for the most basic use case of blending, i.e. smoothly transitioning between different animations.

## Migration Guide

- no bc breaking changes
alradish pushed a commit to alradish/bevy that referenced this pull request Jan 22, 2023
# Objective

- Fixes bevyengine#6338

This PR allows for smooth transitions between different animations.

## Solution

- This PR uses very simple linear blending of animations.
- When starting a new animation, you can give it a duration, and throughout that duration, the previous and the new animation are being linearly blended, until only the new animation is running.
- I'm aware of bevyengine/rfcs#49 and bevyengine/rfcs#51, which are more complete solutions to this problem, but they seem still far from being implemented. Until they're ready, this PR allows for the most basic use case of blending, i.e. smoothly transitioning between different animations.

## Migration Guide

- no bc breaking changes
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

- Fixes bevyengine#6338

This PR allows for smooth transitions between different animations.

## Solution

- This PR uses very simple linear blending of animations.
- When starting a new animation, you can give it a duration, and throughout that duration, the previous and the new animation are being linearly blended, until only the new animation is running.
- I'm aware of bevyengine/rfcs#49 and bevyengine/rfcs#51, which are more complete solutions to this problem, but they seem still far from being implemented. Until they're ready, this PR allows for the most basic use case of blending, i.e. smoothly transitioning between different animations.

## Migration Guide

- no bc breaking changes
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

- Add a basic animation player
  - Single track
  - Not generic, can only animate `Transform`s
  - With plenty of possible optimisations available
  - Close-ish to bevyengine/rfcs#49
- https://discord.com/channels/691052431525675048/774027865020039209/958820063148929064

## Solution

- Can play animations
  - looping or not
- Can pause animations
- Can seek in animation
- Can alter speed of animation
- I also removed the previous gltf animation example

https://user-images.githubusercontent.com/8672791/161051887-e79283f0-9803-448a-93d0-5f7a62acb02d.mp4
@mweatherley
Copy link
Contributor

I might be a couple years late to the party here, but I have some thoughts on Curve<T> as associated with other curve-related matters that have recently been on my mind.

In investigating matters related to, e.g. mesh generation by curve extrusion, I have also come up against curve sampling and similar questions related to what appear here as CurveFixed etc. For instance, in order to perform acceptable mesh extrusion for curves in general, one really needs at least the following in addition to the points on the curve:

  1. Tangents — ideally with regularity assumptions, so that, for example, T = Dir3 is supported for many curves we construct in addition to T = Vec3 (or equivalent)
  2. Arclength (T = f32 or similar)
  3. Rotation-minimizing frames, given by T = Mat3, T = Quat (interpreted as orientation quaternions) or similar, with something like spherical interpolation papering over the gaps

Notably, for curves constructed by splines, (1) is available algebraically (hence for arbitrary parameter values with no interpolation necessary), while (2) and (3) must generally be constructed by sampling (although for some simple classes of curves they could also be algebraic). Here are what I see as the main differences:

  • For these kinds of application in particular, sampling arbitrary time parameters is often less important, since the resolution can often be bound to the task at hand directly — if I want to generate a mesh with some given curve-space resolution, I can just sample the values I need at that frequency (albeit perhaps with some substeps to avoid numerical error) in the first place
  • Insofar as interpolation is needed, it would generally consist only of Animatable::interpolate
  • Concrete Curve implementations will often blend discrete data (possibly interpolated) with data that can be exactly computed from underlying algebraic structures

I think the good news is that these differences are mostly easy to accommodate. For the first, it is not really much of a rift, as long as the Curve<T> implementor has its resolution specified at the time of construction anyway.

For the third, I see this mostly as creating difficulties when serializing and deserializing. For example, something like type erasing the underlying algebraic structure, if necessary, could require passage from exact information to discrete samples; luckily, if we come to that bridge, the other kinds of data that we need are still "compatible" in the sense that they could be recomputed from discrete data anyway (that is to say that RMF, for instance, really only needs a Curve<(Vec3, Dir3)> in its construction).

The second is where I wanted to raise a minor point. I am against the exact definition of Curve<T> in this proposal only because it requires T: Animatable rather than something more general (especially with a name as all-encompassing as Curve). Concretely, what I would prefer is to do the following:

  • Introduce a weaker trait, Interpolable (or similar) in bevy_math, with Animatable: Interpolable in bevy_animation
  • Introduce the trait Curve<T: Interpolable> in bevy_math and implement it for sensible values of T with our spline output curves
  • Have animation depend on Curve<T> where T: Animatable explicitly

I think that this would provide a solid common baseline (at least at the level of API) for the distinct areas.

(By the way, thank you for writing this, and sorry if I am verbose.)

dekirisu pushed a commit to dekirisu/bevy_gltf_trait that referenced this pull request Jul 7, 2024
# Objective

- Add a basic animation player
  - Single track
  - Not generic, can only animate `Transform`s
  - With plenty of possible optimisations available
  - Close-ish to bevyengine/rfcs#49
- https://discord.com/channels/691052431525675048/774027865020039209/958820063148929064

## Solution

- Can play animations
  - looping or not
- Can pause animations
- Can seek in animation
- Can alter speed of animation
- I also removed the previous gltf animation example

https://user-images.githubusercontent.com/8672791/161051887-e79283f0-9803-448a-93d0-5f7a62acb02d.mp4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants