Make `VariableCurve` into curves #13105

mweatherley · 2024-04-25T22:51:29Z

Objective

Presently, the main notion of curve in bevy_animation is VariableCurve, which is essentially a way of organizing an imported glTF animation. This RFC demonstrates a reorganization of this data so that each Transform component and MorphWeights curve is actually a Curve, with its glTF interpolation modes reified by curve structs that implement them on the underlying data buffers.

This has several advantages:

It makes Bevy's representations of glTF animations more portable and reusable.
It cleans up the code internally surrounding animation modes by dispatching them based on separate types.
It lays the foundation for more general curve-based animation by making the current animation data conform more closely with those notions.
It makes curve importing and sampling more robust, closing avenues where illegal values could sneak in.

Solution

VariableCurve, along with the code that loads it and uses it, has been completely overhauled.

VariableCurve is still an enum, but it is split up in a different way:

/// A curve for animating either a the component of a [`Transform`] (translation, rotation, scale)
/// or the [`MorphWeights`] of morph targets for a mesh.
///
/// Each variant yields a [`Curve`] over the data that it parametrizes.
pub enum VariableCurve {
    /// A [`TranslationCurve`] for animating the `translation` component of a [`Transform`].
    Translation(TranslationCurve),

    /// A [`RotationCurve`] for animating the `rotation` component of a [`Transform`].
    Rotation(RotationCurve),

    /// A [`ScaleCurve`] for animating the `scale` component of a [`Transform`].
    Scale(ScaleCurve),

    /// A [`WeightsCurve`] for animating [`MorphWeights`] of a mesh.
    Weights(WeightsCurve),
}

Each of the Transform component curve types here is actually a Curve, but they are still enums, broken down by mode of interpolation; for example, here is RotationCurve:

/// A curve specifying the scale component of a [`Transform`] in animation. The variants are
/// broken down by interpolation mode (with the exception of `Constant`, which never interpolates).
///
/// This type is, itself, a `Curve<Quat>`, and it internally uses the provided sampling modes; each
/// variant "knows" its own interpolation mode.
#[derive(Clone, Debug, Reflect)]
pub enum RotationCurve {
    /// A curve which takes a constant value over its domain. Notably, this is how animations with
    /// only a single keyframe are interpreted.
    Constant(ConstantCurve<Quat>),

    /// A curve which uses spherical linear interpolation between keyframes.
    SphericalLinear(UnevenSampleAutoCurve<Quat>),

    /// A curve which interpolates between keyframes in steps.
    Step(SteppedKeyframeCurve<Quat>),

    /// A curve which interpolates between keyframes by using auxiliary tangent data to join
    /// adjacent keyframes with a cubic Hermite spline. For quaternions, this means interpolating
    /// the underlying 4-vectors, sampling, and normalizing the result.
    CubicSpline(CubicKeyframeCurve<Vec4>),
}

Some of the curve representations that appear here are new, such as SteppedKeyframeCurve and CubicKeyframeCurve — these belong to bevy_animation, and are built on the data structures from bevy::math::curve::cores, which handles the data storage and access patterns. Others, like UnevenSampleAutoCurve and ConstantCurve, are taken "off-the-shelf" from the Curve API itself.

Its implementation of Curve<Quat> essentially just matches over the variants. (The last one requires special handling to do quaternion normalization, but that's about it.)

Allocation-free `Curve<Vec<T>>`

Since the definitions implicit in the Curve trait would require that anything that looks like a Curve<Vec<T>> allocates to produce owned output, this PR introduces an offshoot trait which mitigates this problem — IterableCurve:

/// A curve which provides samples in the form of [`Iterator`]s.
///
/// This is an abstraction that provides an interface for curves which look like `Curve<Vec<T>>`
/// but side-stepping issues with allocation on sampling. This happens when the size of an output
/// array cannot be known statically.
pub trait IterableCurve<T> {
    /// The interval over which this curve is parametrized.
    fn domain(&self) -> Interval;

    /// Sample this curve at a specified time `t`, producing an iterator over sampled values.
    fn sample_iter<'a>(&self, t: f32) -> impl Iterator<Item = T>
    where
        Self: 'a;
}

This is used in concert with the core data structures from the Curve API to sample from keyframes valued in morph weights without ever allocating, all backed by a contiguous buffer of output data.

Performance

This is probably one of the biggest concerns about substantial changes to VariableCurve, so let me be proactive in addressing this.

First of all, VariableCurve has not changed in size (still 64 bytes); this is because the backing data for every curve is at most a pair of vectors — 48 bytes in total, plus 16 bytes for enum discriminants. This is important for caching reasons.

Secondly, proactive measures have been taken to ensure that the curves are designed with good cache locality properties, internally using a Vec<f32> for keyframe times paired with a contiguous buffer of sample data Vec<T> which is sliced up to actually perform sampling. One nice thing is that bevy_animation is not doing any very fancy gymnastics here; it's mostly just using the bevy_math::curve::core APIs as someone would in user-space. The IterableCurve abstraction mentioned above allows these niceties to extend to the case of morph weights without allocation concerns.

Finally, preliminary performance data from tracing looks fine (huge grain of salt — just my machine, on this one example, etc.). On my machine, the difference (if any) on many_foxes appears to be basically the same, with the PR branch running within +-5% of main on animate_targets.

One thing that I really want is a broader base of examples to pull from for performance benchmarking our animation systems, to get a better idea of the potential impact under more realistic circumstances.

Future direction

I think it's unlikely that this representation will serve as the be-all and end-all for encoding character animations — to the contrary, it's likely in the future that bevy_animation will want to include things like compression, at which point these curve constructions will no longer (in an ideal world) see much direct use. On the other hand, I believe that the Curve API itself might provide some value in accomplishing feats like compression, and it would be especially nice if the tools for such things could actually be made reusable across domains. To me, this change helps facilitate that, in addition to just providing a nicer internal representation for glTF animations.

…y_ref

nzhao95 · 2024-05-02T21:41:54Z

Hey cool stuff. However I think it would be important to keep a baked animation data at all times. With curves it's really conveniant to work and modify animations which I would love but to actually play them having to compute curve values every frame would be loss of performence compared to baked animation.
Once the curve animation is all set it should be converted to baked animation before being sent to the animation graph that would only read a value and update on engine ticks. Here it feels like you are having variablecurves that only hold CubicSpline for instance

mweatherley · 2024-05-02T21:47:16Z

Hey cool stuff. However I think it would be important to keep a baked animation data at all times. With curves it's really conveniant to work and modify animations which I would love but to actually play them having to compute curve values every frame would be loss of performence compared to baked animation. Once the curve animation is all set it should be converted to baked animation before being sent to the animation graph that would only read a value and update on engine ticks. Here it feels like you are having variablecurves that only hold CubicSpline for instance

This is pretty much a direct drop-in replacement for how animation currently works in Bevy (which is to say, there is no baking). It also doesn't really preclude things like baking AnimationGraph output in the future; it's more-or-less just making the parts of VariableCurve able to stand on their own as data.

nzhao95 · 2024-05-02T22:14:21Z

This is pretty much a direct drop-in replacement for how animation currently works in Bevy (which is to say, there is no baking). It also doesn't really preclude things like baking AnimationGraph output in the future; it's more-or-less just making the parts of VariableCurve able to stand on their own as data.

I believe right now the transform values are lerped between previous and next key which will always be the case because of variable frame durations. The lerp function comes from the glam crate which is using SIMd making this operation very fast.
So I think it would be a nice addition in the future to cache the baked animation and avoid as many operations as possible. I didn't quite look at your code yet but I'll try to do that to see if I can make any useful suggestions ^^

mweatherley · 2024-05-02T22:24:03Z

I believe right now the transform values are lerped between previous and next key which will always be the case because of variable frame durations. The lerp function comes from the glam crate which is using SIMd making this operation very fast. So I think it would be a nice addition in the future to cache the baked animation and avoid as many operations as possible. I didn't quite look at your code yet but I'll try to do that to see if I can make any useful suggestions ^^

Actually, I think that bevy_animation doesn't use glam's Vec3A for interpolation, but maybe we should switch at some point. In any case, the keyframe interpolation in this implementation works in pretty much exactly the same way, just with the interpolation modes reified to trait implementations of types (vector types just use lerp for this). It may be the case that in doing performance optimizations, however, that we will want to collapse some of these abstractions (in favor, for example, of using SoA); in any case, it would remain true that, for example, TranslationCurve would remain a unified Curve<Vec3>.

One advantage of this is that if we do ever actually want to bake anything, the Curve interface makes this quite standard, since we can just call resample on anything that is a curve and then extract the result. For example, if we get to the point where the animation graph output is a Curve, then we can also bake the result pretty easily. Of course, this is probably overlooking a number of organizational and technical challenges that would intervene along the way 😄

nzhao95 · 2024-05-02T22:37:15Z

I believe right now the transform values are lerped between previous and next key which will always be the case because of variable frame durations. The lerp function comes from the glam crate which is using SIMd making this operation very fast. So I think it would be a nice addition in the future to cache the baked animation and avoid as many operations as possible. I didn't quite look at your code yet but I'll try to do that to see if I can make any useful suggestions ^^

Actually, I think that bevy_animation doesn't use glam's Vec3A for interpolation, but maybe we should switch at some point. In any case, the keyframe interpolation in this implementation works in pretty much exactly the same way, just with the interpolation modes reified to trait implementations of types (vector types just use lerp for this). It may be the case that in doing performance optimizations, however, that we will want to collapse some of these abstractions (in favor, for example, of using SoA); in any case, it would remain true that, for example, TranslationCurve would remain a unified Curve<Vec3>.

One advantage of this is that if we do ever actually want to bake anything, the Curve interface makes this quite standard, since we can just call resample on anything that is a curve and then extract the result. For example, if we get to the point where the animation graph output is a Curve, then we can also bake the result pretty easily. Of course, this is probably overlooking a number of organizational and technical challenges that would intervene along the way 😄

I was looking at lib.rs in bevy_animation : fn apply_single_keyframe() I believe it's the function used for apply animations but I'm not sure I just guess it by the name :D But this function is indeed using the Quat::slerp and Vec3::lerp which are coming from glam.
And yeah I think it's awesome to be able to store anims in curves but I was just saying that for the actual display it would be nice to cache it and make the frame iterations lower.
Would be easier to tell with some use cases. Where is the interpolation done at the end to be played by the AnimationPlayer ?

mweatherley · 2024-05-02T22:43:41Z

I was looking at lib.rs in bevy_animation : fn apply_single_keyframe() I believe it's the function used for apply animations but I'm not sure I just guess it by the name :D But this function is indeed using the Quat::slerp and Vec3::lerp which are coming from glam. And yeah I think it's awesome to be able to store anims in curves but I was just saying that for the actual display it would be nice to cache it and make the frame iterations lower. Would be easier to tell with some use cases. Where is the interpolation done at the end to be played by the AnimationPlayer ?

Ah; apply_single_keyframe is only used in the case that the loaded glTF animation only consists of a single keyframe, and the lerp and slerp there is better described as "blending" rather than interpolation (it comes from weights in the animation graph); the actual interpolation logic is in apply_tweened_keyframe. In this proposal, both of these would be largely replaced just by calling sample on the associated curves (the constant curve variants playing the role of apply_single_keyframe).

nzhao95 · 2024-05-03T08:38:53Z

Ok I see, we should probably look into code optimization on this topic later. Even the tweened keyframe function is doing a few unecessary copies :

                let tangent_out_start = keyframes[step_start * 3 + 2];
                let tangent_in_end = keyframes[(step_start + 1) * 3];
                let value_end = keyframes[(step_start + 1) * 3 + 1];
                let result = cubic_spline_interpolation(
                    value_start,
                    tangent_out_start,
                    tangent_in_end,
                    value_end,
                    lerp,
                    duration,
                );

The keyframe update functions should be very fast compared to data management functions which can take there time. But yeah lots of things to do ^^ Having curves is already a huge plus. Would be interesting to reimplement all the actual curve evaluations in SIMd as well

# Objective Finish what we started in #14630. The Curve RFC is [here](https://github.com/bevyengine/rfcs/blob/main/rfcs/80-curve-trait.md). ## Solution This contains the rest of the library from my branch. The main things added here are: - Bulk sampling / resampling methods on `Curve` itself - Data structures supporting the above - The `cores` submodule that those data structures use to encapsulate sample interpolation The weirdest thing in here is probably `ChunkedUnevenCore` in `cores`, which is not used by anything in the Curve library itself but which is required for efficient storage of glTF animation curves. (See #13105.) We can move it into a different PR if we want to; I don't have strong feelings either way. ## Testing New tests related to resampling are included. As I write this, I realize we could use some tests in `cores` itself, so I will add some on this branch before too long. --------- Co-authored-by: Alice Cecile <[email protected]> Co-authored-by: Robert Walter <[email protected]>

…ve::cores

…ation

NthTensor · 2024-08-19T21:48:54Z

Very cool! Obligatory note that we cannot merge this without profiling.

mweatherley · 2024-08-19T22:00:46Z

Very cool! Obligatory note that we cannot merge this without profiling.

Well, I did profile it, but as noted in the description, more profiling would definitely be great.

Sirmadeira · 2024-08-20T15:57:34Z

Really cool, please accept the merge this will make the logic behind blended mask nodes. Way easier to implement

RobWalt

Haven't gotten very far yet. To be continued and here's at least one comment. Already looks really cool! :)

RobWalt · 2024-08-21T20:36:39Z

crates/bevy_animation/src/curves.rs

+    /// The time of the last keyframe for this animation curve. If the curve is constant, None
+    /// is returned instead.
+    #[inline]
+    pub fn duration(&self) -> Option<f32> {


I feel like the doc comment and the name of the function are not aligned. In my head duration is more like the length of the domain whereas the comment and the implementation are about the end_time or however you want to call it. If the curve implicitly will always start at t = 0 then we should note that somewhere.

The same review comment applies to the other enums and their duration methods.

I agree. I expect duration() to return a std::time::duration like Time.delta() would.

Maybe last_keyframe() would be more appropriate?

RobWalt

Looks really good to me. Only a few minor things. Great work! 💚

RobWalt · 2024-08-22T07:15:58Z

crates/bevy_animation/src/curves.rs

+        cubic_spline_interpolation(
+            first[idx + width],
+            first[idx + (width * 2)],
+            second[idx + width],
+            second[idx],
+            s,
+            step_between,
+        )


Not really super deep into the code but comparing with the other occurrence of this function call we have

cubic_spline_interpolation(u[1], u[2], v[0], v[1], s, t1 - t0)

so in my mind I would think that we follow a similar pattern here. So to me it looks like the args for second got switched up and should be

cubic_spline_interpolation( first[idx + width], first[idx + (width * 2)], second[idx], second[idx + width], s, step_between, )

since I think that second[idx + width] is the end value and second[idx] is the in tangent. But maybe I'm missing something, just making sure that this is intended

RobWalt · 2024-08-22T07:23:36Z

crates/bevy_gltf/src/loader.rs

+                                        .map(|c| {
+                                            VariableCurve::Translation(TranslationCurve::Linear(c))
+                                        })


I'm wondering why clippy isn't complaining but (nit-picky and opinionated!!) I think this can be

Suggested change

.map(|c| {

VariableCurve::Translation(TranslationCurve::Linear(c))

})

.map(TranslationCurve::Linear)

.map(VariableCurve::Translation)

saving us the indentation, a line of code and generally easier read (probably just to me). This is basically applicaable everywhere below

RobWalt · 2024-08-22T07:29:01Z

crates/bevy_animation/src/curves.rs

+            | InterpolationDatum::RightTail((_, v)) => v[1],
+
+            InterpolationDatum::Between((t0, u), (t1, v), s) => {
+                cubic_spline_interpolation(u[1], u[2], v[0], v[1], s, t1 - t0)


I feel like this could use

a) well named variables for u[1], u[2], v[0] and v[1] to add some context again

b) a normal comment explaining what the values are

for future developers that look at the code in 1+ year even though (!!) it's already perfectly explained a few lines below in the new functions. imo we can't guarantee that people will find the other docs and if they don't this is harder to figure out.

RobWalt · 2024-08-22T07:31:00Z

crates/bevy_math/src/curve/cores.rs

+        if values.is_empty() {
+            return Err(ChunkedUnevenCoreError::ZeroWidth);
+        }


We could move this up a bit as it is cheap to check and we could potentially skip filter_sort_dedup_times and exit early.

RobWalt · 2024-08-22T07:32:40Z

crates/bevy_math/src/curve/iterable.rs

+    fn sample_iter_unchecked<'a>(&self, t: f32) -> impl Iterator<Item = T>
+    where
+        Self: 'a;


What is the lifetime for here?

tigerplush · 2024-08-25T08:00:53Z

crates/bevy_gltf/src/loader.rs

+                    continue;
+                }
+
+                let maybe_curve: Option<VariableCurve> = if let Some(outputs) =


I feel this has a lot of cyclomatic complexity - if we do

let Some(outputs) = reader.read_outputs() else { warn!("Animations without a sampler output are not supported"); return Err(GltfError::MissingAnimationSampler(animation.index())); };

we could achieve the same thing but with more readability?
The same review comment applies to the following let ... = if let Some(...) = ... passages.

mweatherley · 2024-09-25T22:45:57Z

Subsumed by #15434.

mweatherley added 6 commits April 11, 2024 19:43

First draft

708645f

Added Interval and refactored to use it as the curve domain

71ab763

Ensured object safety, added Deref blanket impl supported by method b…

df4629c

…y_ref

Refactored interval steps to a dedicated function

ded34c2

Comment change

f0abd38

Curve replacements for the parts of VariableCurve

33e12e3

mweatherley changed the title ~~IntegrateCurve with bevy_animation~~ Integrate Curve with bevy_animation Apr 25, 2024

alice-i-cecile added A-Animation Make things move and change over time C-Feature A new feature, making something new possible X-Controversial There is active debate or serious implications around merging this PR labels Apr 25, 2024

Added WeightsCurve, MultiCurve subtrait

34222f7

mweatherley mentioned this pull request Apr 25, 2024

A curve trait for general interoperation bevyengine/rfcs#80

Merged

Iteration on MultiCurve

ec13959

mweatherley added 10 commits May 4, 2024 16:00

Change UnevenSampleCurve to SoA

c7fde9e

Derive Error on error types

b9e113d

Lints

b0c5f44

Restrict Interpolable constraint to resampling methods

9083553

Address early review comments

02de000

Explicitly clamp index in UnevenSampleCurve::sample

6ea3416

Reorganized, added some tests

36476b1

Fixes to docs/lints, some re-exports

715fdca

Added IterableCurve abstraction

eabbd08

Merge curve-trait into curves-in-animation

7f9634b

mweatherley added 2 commits June 11, 2024 21:56

Merge branch 'curve-trait' into curves-in-animation

28b5ad2

Slight rework of multisample curves. VariableCurve -> 64 bytes

412ecb4

mweatherley changed the title ~~Integrate Curve with bevy_animation~~ Make VariableCurve into curves Jun 12, 2024

This was referenced Aug 10, 2024

A Curve trait for general interoperation — Part II #14700

Merged

bevy_animation::animate_targets crashes with "subtract with overflow" if there is an empty curve on AnimationClip #14766

Open

mweatherley mentioned this pull request Aug 16, 2024

Tracking Issue: Curves Working Group Tasks #13213

Open

15 tasks

mweatherley added 5 commits August 16, 2024 13:47

Move IterableCurve into its own module

cc558ed

Merge main into curves-in-animation

9b2f7dc

Delete outdated Reflect stubs

987d2b4

Obliterate unchecked curve constructions

22491e1

Fix doc links

1fe2f4d

mweatherley added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label Aug 18, 2024

mweatherley marked this pull request as ready for review August 18, 2024 17:28

mweatherley marked this pull request as draft August 18, 2024 17:28

mweatherley added 5 commits August 19, 2024 06:13

Replace keyframe tests in bevy_animation with tests in bevy_math::cur…

52d65c8

…ve::cores

Add sampling variants for IterableCurve, use clamped sampling in anim…

b23b8e8

…ation

Lint

1ed2925

Merge branch 'main' into curves-in-animation

eb5549a

Return to unchecked sampling for perf

4e68dd0

mweatherley marked this pull request as ready for review August 19, 2024 16:15

RobWalt reviewed Aug 21, 2024

View reviewed changes

RobWalt reviewed Aug 22, 2024

View reviewed changes

tigerplush reviewed Aug 25, 2024

View reviewed changes

notmd mentioned this pull request Sep 18, 2024

Allow animation clips to animate arbitrary properties. #15282

Merged

mweatherley closed this Sep 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `VariableCurve` into curves #13105

Make `VariableCurve` into curves #13105

mweatherley commented Apr 25, 2024 •

edited

Loading

nzhao95 commented May 2, 2024

mweatherley commented May 2, 2024

nzhao95 commented May 2, 2024 •

edited

Loading

mweatherley commented May 2, 2024

nzhao95 commented May 2, 2024

mweatherley commented May 2, 2024

nzhao95 commented May 3, 2024 •

edited

Loading

NthTensor commented Aug 19, 2024

mweatherley commented Aug 19, 2024

Sirmadeira commented Aug 20, 2024

RobWalt left a comment

RobWalt Aug 21, 2024

tigerplush Aug 25, 2024 •

edited

Loading

RobWalt left a comment

RobWalt Aug 22, 2024

RobWalt Aug 22, 2024

RobWalt Aug 22, 2024

RobWalt Aug 22, 2024

RobWalt Aug 22, 2024

tigerplush Aug 25, 2024

mweatherley commented Sep 25, 2024

Make VariableCurve into curves #13105

Make VariableCurve into curves #13105

Conversation

mweatherley commented Apr 25, 2024 • edited Loading

Objective

Solution

Allocation-free Curve<Vec<T>>

Performance

Future direction

nzhao95 commented May 2, 2024

mweatherley commented May 2, 2024

nzhao95 commented May 2, 2024 • edited Loading

mweatherley commented May 2, 2024

nzhao95 commented May 2, 2024

mweatherley commented May 2, 2024

nzhao95 commented May 3, 2024 • edited Loading

NthTensor commented Aug 19, 2024

mweatherley commented Aug 19, 2024

Sirmadeira commented Aug 20, 2024

RobWalt left a comment

Choose a reason for hiding this comment

RobWalt Aug 21, 2024

Choose a reason for hiding this comment

tigerplush Aug 25, 2024 • edited Loading

Choose a reason for hiding this comment

RobWalt left a comment

Choose a reason for hiding this comment

RobWalt Aug 22, 2024

Choose a reason for hiding this comment

RobWalt Aug 22, 2024

Choose a reason for hiding this comment

RobWalt Aug 22, 2024

Choose a reason for hiding this comment

RobWalt Aug 22, 2024

Choose a reason for hiding this comment

RobWalt Aug 22, 2024

Choose a reason for hiding this comment

tigerplush Aug 25, 2024

Choose a reason for hiding this comment

mweatherley commented Sep 25, 2024

Make `VariableCurve` into curves #13105

Make `VariableCurve` into curves #13105

mweatherley commented Apr 25, 2024 •

edited

Loading

Allocation-free `Curve<Vec<T>>`

nzhao95 commented May 2, 2024 •

edited

Loading

nzhao95 commented May 3, 2024 •

edited

Loading

tigerplush Aug 25, 2024 •

edited

Loading