Animatable trait for interpolation and blending #4482

james7132 · 2022-04-15T12:36:03Z

Objective

Allow animation of types other than translation, scale, and rotation on Transforms.

Solution

Add a base trait for all values that can be animated by the animation system. This provides the basic operations for sampling and blending animation values for more than just translation, rotation, and scale.

This implements part of bevyengine/rfcs#51, but is missing the implementations for Range<T> and Color. This also does not fully integrate with the existing AnimationPlayer yet, just setting up the trait.

crates/bevy_animation/src/util.rs

Co-authored-by: Kirillov Kirill <[email protected]>

crates/bevy_animation/src/util.rs

Co-authored-by: François <[email protected]>

Use slerp for Quat interpolation

crates/bevy_animation/src/animatable.rs

atlv24 · 2024-01-29T10:58:14Z

crates/bevy_animation/src/animatable.rs

+            if input.additive {
+                translation += input.weight * Vec3A::from(input.value.translation);
+                scale += input.weight * Vec3A::from(input.value.scale);
+                rotation = rotation.slerp(input.value.rotation, input.weight);


Suggested change

rotation = rotation.slerp(input.value.rotation, input.weight);

// Important: additively animating quaternions means we first have to partially apply the input rotation.

// Quaternion multiplication order is important here. Additionally, do not use nlerp here. These rotations

// can be very different, so we must use slerp explicitly.

// Reference:

// "Additive blend" section in <https://animcoding.com/post/animation-tech-intro-part-3-blending>

// "Here’s how to apply an additive pose on top of the regular pose. It differs from the regular blend.

// If the weight is one, then we can multiply transforms. If the weight is non-one, then we need to first blend

// between identity and the additive transform, then apply this blended transform on the transform in the pose."

rotation = rotation * Quat::IDENTITY.slerp(input.value.rotation, input.weight);

This is the critical fix right? We should add a comment describing why this is done, ideally with a reference.

Yes this is the critical fix. Everything else in my review is 'fluff'. added a comment

atlv24 · 2024-01-29T11:01:43Z

crates/bevy_animation/src/animatable.rs

+                    input.weight,
+                );
+                scale = Vec3A::interpolate(&scale, &Vec3A::from(input.value.scale), input.weight);
+                rotation = Quat::interpolate(&rotation, &input.value.rotation, input.weight);


~~nit: can be more explicit about the kind of interpolation~~

~~rotation = rotation.slerp(&input.value.rotation, input.weight);~~

nlerp is wanted here for commutativity on the blend stack

What does that mean? What's supposed to be commutative? What's the blend stack? Blending operations are not commutative in general, you need to apply them in order. And even if they are, what do we gain by reordering?

the blend stack is the sequence of animations being blended. commutativity makes reasoning about the operations simpler in the general case, as you dont need to have logic to ensure they are always ordered the same way for the result to be consistent. consider two animations A and B, which can each start and stop at any moment. if you start A, then B, you will have A applied first then B applied on top. if you start B then A, they will be on the stack the other way around and will look different. This gets more complicated if you can cancel and restart, or pause animations. You start needing an animation priority system to determine which get applied first, and that can be confusing. Besides all that, its slower for not much gain quality-wise, as far as i understand. Slerp is not needed here, refer to the document linked in the comments above interpolate

if you start A, then B, you will have A applied first then B applied on top.

Only if B is additive, because otherwise you should stop A first, you can't apply two non-additive animations to the same target. Also pause etc. is about animation transitions and creating the stack, not about blending (how to consume the stack). And if B is additive then I argue that the operations are not commutative. It's not a question of quality, it's a question of producing the correct result.

atlv24 · 2024-01-29T11:04:37Z

crates/bevy_animation/src/animatable.rs

+    fn interpolate(a: &Self, b: &Self, t: f32) -> Self {
+        Self {
+            translation: Vec3::interpolate(&a.translation, &b.translation, t),
+            rotation: Quat::interpolate(&a.rotation, &b.rotation, t),


~~nit: can be more explicit about the kind of interpolation~~
this definitely needs to explicitly be slerp

Suggested change

rotation: Quat::interpolate(&a.rotation, &b.rotation, t),

rotation: a.rotation.slerp(&b.rotation, t),

Because interpolate should be nlerp by default and transforms can be very different rotations and thus need slerp.

Ah you mean that for rotations the interpolation should be fixed to always be slerp because others will not provide decent results in general, right? I agree that we should probably limit the number of different interpolations we support, explicitly. See my other long comment about the design.

atlv24 · 2024-01-29T11:07:38Z

crates/bevy_animation/src/animatable.rs

+}
+
+impl Animatable for Quat {
+    /// Performs an nlerp, because it's cheaper and easier to combine with other animations,


These docs are dissonant. The code is not doing an nlerp.

This should be doing an nlerp

Your other comment says it should be doing a slerp() no? Because nlerp() for very different rotations might be way off.

atlv24

I've gone over the usages of interpolate and slerp again after reading more. In general we want to use nlerp in most places, except for additive blending and transform interpolation. The rest we prefer nlerp.

pcwalton · 2024-01-29T20:58:46Z

I don't have much to add to the review other than what @rodolphito said, looks good otherwise.

djeedai · 2024-01-30T07:31:51Z

crates/bevy_animation/src/animatable.rs

+pub trait Animatable: Reflect + Sized + Send + Sync + 'static {
+    /// Interpolates between `a` and `b` with  a interpolation factor of `time`.
+    ///
+    /// The `time` parameter here may not be clamped to the range `[0.0, 1.0]`.


What does this comment mean? How is an implementor supposed to understand interpolating with t=1.5 (which sounds like an extrapolation rather)? Are we sure this is even possible in the general case? Because this seems to strongly imply lerp here / a linear transform.

atlv24 · 2024-01-31T05:31:10Z

There's a bigger problem with switching to nlerp i've discovered: Glam's nlerp is not commutative, nor can it easily be made commutative. This is because it is normalizing after interpolating, which makes sense for blending two quaternions, but it also means that its no longer a truly linear operation. To commutatively blend an arbitrary number of quaternions, one would need to defer the normalization til the very end. This is problematic however because the intermediary values are not true rotation quaternions because they are not unit length. We'd want to encode this in the type system somehow. I think it can be done with a PR to glam, but it might be contentious as it adds complexity and would need some extra implementations. There would need to be a DenormalizedQuaternion type that implements Into<Quaternion>, and have specialized lerp methods that take combinations of DenormalizedQuaternions and Quaternions to linearly interpolate them. Then at the end it would be silently normalized and converted into a Quaternion as soon as it gets used.

For this reason I am opting to leave slerp in for the moment, because this would need further discussion and upstream work in glam.

atlv24 · 2024-01-31T05:50:17Z

I've realized there actually is a simpler way forward that can be implemented now without changes to glam.

atlv24 · 2024-01-31T06:08:19Z

BlendInput says additive determines "Whether or not to additively blend this input into the final result" which is actually not what its doing. Additive BlendInputs are being applied in the order they are given in the blend stack, meaning if a non-additive BlendInput is present after an additive one, the additive one is not blended into the final result as claimed. This will look wrong and be unintuitive, compared to most animation solutions ive worked with

djeedai

See the long comment over the abstraction shape. We can discuss on Discord, but this was easier I think to make a comment here with code example.

djeedai · 2024-01-31T20:55:48Z

crates/bevy_animation/src/animatable.rs

+use bevy_utils::FloatOrd;
+
+/// An individual input for [`Animatable::blend`].
+pub struct BlendInput<T> {


I've thought a bit more about this abstraction. I think it's useful to look at what our goals are, especially in terms of how things will be used, and how this is trying to achieve those goals.

As I recall, the base idea of BlendInput<T> is to "preprocess" all the various blend sources into a single common data structure per type (one for f32, one for Vec4, etc.) and then run a tight loop on blending all sources for that data type, which is then easier because you iterate only on BlendInput<T> and not a collection of heterogenous sources.

Now, this is clearly a step in the right direction, in the sense that we're thinking about the data usage pattern to optimize it and leverage e.g. SIMD. Where I think this abstraction falls short is in two ways:

It iterates over the blend sources in the inner loop, as seen in the signature of Animatable::blend() which takes a set of sources to blend together. How many different sources do we expect will blend that way? Even for an advanced scenario with say 4 base animations blending together (walk, run, jump, and crouch), and let's say 4 more additive ones on top (look left/right, up/down, and whatever else), that makes 8 sources to blend into a final vertex value. Compare that with how many vertices we need to blend for a single 3D character in a modern game, which is in the order of thousands. And those will be iterated over in an outer loop. We've got the loops reversed from a performance point of view.

The data structure is not optimal for packing and memory accesses. First the packing: depending on T, and let's take any VecN as example, the compiler will most likely put the VecN first, aligned on 4 bytes (or 16 bytes for SIMD), followed by weight (4B size/align), followed by additive (1B). This means we have 3 bytes of padding for every 12B (f32) to 24B (Vec4) of total size, which is a lot. The access pattern, which is also to load the value and weight into separate (SIMD) registers to perform some lerp() (which is the most common case; let's ignore rotations for a minute) is also not helped here because we can't load more than 1 source at once (SIMD register load only accepts a continuous block of memory). If, on the other hand, we were transforming this "array of struct" into a "struct of array", we could load more data sources at once, and blend multiple of them via SIMD. Better even, we can even pack all lerp() sources together (that is, including packing f32 with Vec3 and Vec4) since the SIMD instructions to lerp are the same and operate component-wise. This means a very tight loop on a long array of data without any branching etc. which is the absolute optimal for SIMD.

So the way I'd see a better abtraction for the blending step is:

explicitly support lerp / nlerp / slerp with separate optimized codepaths, because they require different SIMD instructions;

possibly add a fallback where we invoke something like the current Animatable::interpolate(), but only for "exotic" interpolations, and I'm not even sure we want that (I don't have an example);

pack all inputs and do lerp(lhs[], rhs[], weights[]) instead of lerp(lhs,rhs,weight)[].

pub enum Interpolation<T> { Linear, NormalizedLinear, SphericalLinear, // See how this immediately become messy due to T, maybe we can skip this entirely Custom(Box<dyn Fn(&T, &T, f32) -> T), } impl Interpolation<f32> { // Blend two arrays of values with given weights fn blend(&self, src: &mut [f32], rhs: &[f32], weights: &[f32], additive: bool) { match *self { Interpolation::<f32>::Linear => { // Branch _before_ we start the hot loop if additive { // This loop can get partially unrolled, use SIMD, etc. for i in 0..src.len() { src[i] += rhs[i] * weights[i]; } } else { // Same here for i in 0..src.len() { src[i] = (1.0 - weights[i]) * src[i] + rhs[i] * weights[i]; } } } // etc. } } } // Now, we need to know how to pack e.g. Vec3 and f32 and Vec4 together: pub trait Animatable: [...] { type Unit; fn prepare(&self) -> &[Unit]; } impl Animatable for Vec3 { type Unit = f32; fn prepare(&self) -> &[f32] { self.as_ref() } } // etc. for other types... fn animation_system(anims: &[...]) { // Read and prepare values from component (here, Transform) let mut src = vec![]; for tr in &transforms { // Assuming here the case where both translation and scale use Interpolation::Linear // to keep the example short: src.extend_from_slice(tr.translation.prepare()); src.extend_from_slice(tr.scale.prepare()); } // Blend all anims into src directly let mut rhs = vec![]; for anim in &anims { // Grab one animation. Ideally we can even pre-process this in the anim format, // and completely remove this block. rhs.clear(); for tr in &anim.transforms { rhs.extend_from_slice(tr.translation.prepare()); rhs.extend_from_slice(tr.scale.prepare()); } // Do the actual blend for every transform at once Interpolation::<f32>::blend(&mut src, &rhs, &anim.weights, false); } // Store back final result let mut offset = 0; for tr in &mut transforms { tr.translation.copy_from_slice(&src[offset..offset+3]); offset += 3; tr.scale.copy_from_slice(&src[offset..offset+3]); offset += 3; } }

djeedai · 2024-01-31T20:58:22Z

crates/bevy_animation/src/animatable.rs

+    fn interpolate(a: &Self, b: &Self, t: f32) -> Self {
+        Self {
+            translation: Vec3::interpolate(&a.translation, &b.translation, t),
+            rotation: Quat::interpolate(&a.rotation, &b.rotation, t),


Ah you mean that for rotations the interpolation should be fixed to always be slerp because others will not provide decent results in general, right? I agree that we should probably limit the number of different interpolations we support, explicitly. See my other long comment about the design.

djeedai · 2024-01-31T20:59:22Z

crates/bevy_animation/src/animatable.rs

+}
+
+impl Animatable for Quat {
+    /// Performs an nlerp, because it's cheaper and easier to combine with other animations,


Your other comment says it should be doing a slerp() no? Because nlerp() for very different rotations might be way off.

pcwalton · 2024-02-02T00:49:11Z

@rodolphito I've been told that what this PR does actually matches what Unity does. In Unity override layers cancel out the results of previous additive layers.

djeedai

Discussed a lot on Discord. There's relative consensus with @pcwalton and @james7132 that this is good enough for now, so let's move forward.

Some resources:

alice-i-cecile

I'm content with this for now. Like djee said, there's been a lot of discussion and we have broad consensus. Ongoing improvements will be much easier to do in follow-ups :)

james7132 · 2024-02-02T21:47:44Z

For posterity, the discussion that led to this being merged can be found here: https://discord.com/channels/691052431525675048/774027865020039209/1202358388466384956

# Objective Allow animation of types other than translation, scale, and rotation on `Transforms`. ## Solution Add a base trait for all values that can be animated by the animation system. This provides the basic operations for sampling and blending animation values for more than just translation, rotation, and scale. This implements part of bevyengine/rfcs#51, but is missing the implementations for `Range<T>` and `Color`. This also does not fully integrate with the existing `AnimationPlayer` yet, just setting up the trait. --------- Co-authored-by: Kirillov Kirill <[email protected]> Co-authored-by: François <[email protected]> Co-authored-by: irate <[email protected]> Co-authored-by: Alice Cecile <[email protected]> Co-authored-by: Alice Cecile <[email protected]>

Add Animatable trait

6a209f1

github-actions bot added the S-Needs-Triage This issue needs to be labelled label Apr 15, 2022

james7132 added C-Feature A new feature, making something new possible A-Animation Make things move and change over time and removed S-Needs-Triage This issue needs to be labelled labels Apr 15, 2022

kirusfg reviewed Apr 15, 2022

View reviewed changes

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

more accurate approx_sqrt

ef0963f

Co-authored-by: Kirillov Kirill <[email protected]>

alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Apr 22, 2022

Update comment about implementation, appease clippy

bec4c6d

tim-blackbird reviewed Apr 28, 2022

View reviewed changes

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

Weibye added the S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! label Aug 10, 2022

james7132 removed the S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! label Nov 7, 2022

james7132 added 3 commits November 7, 2022 01:51

Merge branch 'main' into animatable

1601c5d

Make post_process safe

acddae3

Remove the inline(always) that are unnecessary

77eb93f

james7132 requested review from tim-blackbird and kirusfg and removed request for tim-blackbird and kirusfg November 7, 2022 10:03

Fix CI

08fe08a

mockersf reviewed Nov 7, 2022

View reviewed changes

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

mockersf reviewed Nov 7, 2022

View reviewed changes

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

Fix docs.

8e69ca9

Co-authored-by: François <[email protected]>

Merge pull request #8 from alice-i-cecile/quat-interpolation

71d3dfc

Use slerp for Quat interpolation

alice-i-cecile reviewed Jan 15, 2024

View reviewed changes

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

alice-i-cecile mentioned this pull request Jan 24, 2024

bevy_animation: Abstract away interpolation #11501

Closed

alice-i-cecile modified the milestones: 0.13, 0.14 Jan 24, 2024

atlv24 suggested changes Jan 29, 2024

View reviewed changes

atlv24 reviewed Jan 29, 2024

View reviewed changes

djeedai reviewed Jan 30, 2024

View reviewed changes

djeedai suggested changes Jan 31, 2024

View reviewed changes

djeedai approved these changes Feb 2, 2024

View reviewed changes

alice-i-cecile modified the milestones: 0.14, 0.13 Feb 2, 2024

alice-i-cecile approved these changes Feb 2, 2024

View reviewed changes

alice-i-cecile 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 2, 2024

alice-i-cecile and others added 2 commits February 2, 2024 16:01

Merge branch 'main' into animatable

c6876cc

Cargo fmt

da5f7a9

alice-i-cecile enabled auto-merge February 2, 2024 21:07

alice-i-cecile added this pull request to the merge queue Feb 2, 2024

Merged via the queue into bevyengine:main with commit 602515d Feb 2, 2024
23 checks passed

james7132 deleted the animatable branch March 10, 2024 08:05

djeedai mentioned this pull request Jun 4, 2024

Smooth following via generalized interpolation #13613

Closed

djeedai mentioned this pull request Jun 17, 2024

Additive blending of Quats looks suspect #13832

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Animatable trait for interpolation and blending #4482

Animatable trait for interpolation and blending #4482

james7132 commented Apr 15, 2022 •

edited

Loading

atlv24 Jan 29, 2024 •

edited

Loading

alice-i-cecile Jan 29, 2024

atlv24 Jan 29, 2024 •

edited

Loading

atlv24 Jan 29, 2024 •

edited

Loading

atlv24 Jan 29, 2024

djeedai Jan 30, 2024

atlv24 Jan 31, 2024

djeedai Jan 31, 2024

atlv24 Jan 29, 2024 •

edited

Loading

djeedai Jan 30, 2024

atlv24 Jan 31, 2024

djeedai Jan 31, 2024

atlv24 Jan 29, 2024

atlv24 Jan 29, 2024

djeedai Jan 31, 2024

atlv24 left a comment

pcwalton commented Jan 29, 2024

djeedai Jan 30, 2024

atlv24 commented Jan 31, 2024

atlv24 commented Jan 31, 2024

atlv24 commented Jan 31, 2024

djeedai left a comment

djeedai Jan 31, 2024

djeedai Jan 31, 2024

djeedai Jan 31, 2024

pcwalton commented Feb 2, 2024

djeedai left a comment •

edited

Loading

alice-i-cecile left a comment

james7132 commented Feb 2, 2024

-                rotation = rotation.slerp(input.value.rotation, input.weight);
+                // Important: additively animating quaternions means we first have to partially apply the input rotation.
+                // Quaternion multiplication order is important here. Additionally, do not use nlerp here. These rotations
+                // can be very different, so we must use slerp explicitly.
+                // Reference:
+                // "Additive blend" section in <https://animcoding.com/post/animation-tech-intro-part-3-blending>
+                // "Here’s how to apply an additive pose on top of the regular pose. It differs from the regular blend.
+                // If the weight is one, then we can multiply transforms. If the weight is non-one, then we need to first blend
+                // between identity and the additive transform, then apply this blended transform on the transform in the pose."
+                rotation = rotation * Quat::IDENTITY.slerp(input.value.rotation, input.weight);

	rotation: Quat::interpolate(&a.rotation, &b.rotation, t),
	rotation: a.rotation.slerp(&b.rotation, t),

Animatable trait for interpolation and blending #4482

Animatable trait for interpolation and blending #4482

Conversation

james7132 commented Apr 15, 2022 • edited Loading

Objective

Solution

atlv24 Jan 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atlv24 Jan 29, 2024 • edited Loading

Choose a reason for hiding this comment

atlv24 Jan 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atlv24 Jan 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atlv24 left a comment

Choose a reason for hiding this comment

pcwalton commented Jan 29, 2024

Choose a reason for hiding this comment

atlv24 commented Jan 31, 2024

atlv24 commented Jan 31, 2024

atlv24 commented Jan 31, 2024

djeedai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pcwalton commented Feb 2, 2024

djeedai left a comment • edited Loading

Choose a reason for hiding this comment

alice-i-cecile left a comment

Choose a reason for hiding this comment

james7132 commented Feb 2, 2024

james7132 commented Apr 15, 2022 •

edited

Loading

atlv24 Jan 29, 2024 •

edited

Loading

atlv24 Jan 29, 2024 •

edited

Loading

atlv24 Jan 29, 2024 •

edited

Loading

atlv24 Jan 29, 2024 •

edited

Loading

djeedai left a comment •

edited

Loading