From 42e0771633eeb9e8c1eb62d7e0671dd03f2da66d Mon Sep 17 00:00:00 2001
From: Matty <weatherleymatthew@gmail.com>
Date: Sat, 5 Oct 2024 16:03:10 -0400
Subject: [PATCH] Fix additive blending of quaternions (#15662)

# Objective

Fixes #13832

## Solution

Additively blend quaternions like this:
```rust
rotation = Quat::slerp(Quat::IDENTITY, incoming_rotation, weight) * rotation;
```

## Testing

Ran `animation_masks`, which behaves the same as before. (In the context
of an animation being blended only onto the base pose, there is no
difference.)

We should create some examples that actually exercise more of the
capabilities of the `AnimationGraph` so that issues like this can become
more visible in general. (On the other hand, I'm quite certain this was
wrong before.)

## Migration Guide

This PR changes the implementation of `Quat: Animatable`, which was not
used internally by Bevy prior to this release version. If you relied on
the old behavior of additive quaternion blending in manual applications,
that code will have to be updated, as the old behavior was incorrect.
---
 crates/bevy_animation/src/animatable.rs       | 16 +++++++++++---
 crates/bevy_animation/src/animation_curves.rs | 21 +++++++------------
 2 files changed, 20 insertions(+), 17 deletions(-)

diff --git a/crates/bevy_animation/src/animatable.rs b/crates/bevy_animation/src/animatable.rs
index 298d0125a208c..6af653d3077b4 100644
--- a/crates/bevy_animation/src/animatable.rs
+++ b/crates/bevy_animation/src/animatable.rs
@@ -149,7 +149,8 @@ impl Animatable for Transform {
             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);
+                rotation =
+                    Quat::slerp(Quat::IDENTITY, input.value.rotation, input.weight) * rotation;
             } else {
                 translation = Vec3A::interpolate(
                     &translation,
@@ -181,8 +182,17 @@ impl Animatable for Quat {
     #[inline]
     fn blend(inputs: impl Iterator<Item = BlendInput<Self>>) -> Self {
         let mut value = Self::IDENTITY;
-        for input in inputs {
-            value = Self::interpolate(&value, &input.value, input.weight);
+        for BlendInput {
+            weight,
+            value: incoming_value,
+            additive,
+        } in inputs
+        {
+            if additive {
+                value = Self::slerp(Self::IDENTITY, incoming_value, weight) * value;
+            } else {
+                value = Self::interpolate(&value, &incoming_value, weight);
+            }
         }
         value
     }
diff --git a/crates/bevy_animation/src/animation_curves.rs b/crates/bevy_animation/src/animation_curves.rs
index e4a6e46734e14..2ca23727b57f9 100644
--- a/crates/bevy_animation/src/animation_curves.rs
+++ b/crates/bevy_animation/src/animation_curves.rs
@@ -196,8 +196,7 @@ pub struct AnimatableCurve<P, C> {
 ///
 /// You shouldn't ordinarily need to instantiate one of these manually. Bevy
 /// will automatically do so when you use an [`AnimatableCurve`] instance.
-#[derive(Reflect, FromReflect)]
-#[reflect(from_reflect = false)]
+#[derive(Reflect)]
 pub struct AnimatableCurveEvaluator<P>
 where
     P: AnimatableProperty,
@@ -346,8 +345,7 @@ pub struct TranslationCurve<C>(pub C);
 ///
 /// You shouldn't need to instantiate this manually; Bevy will automatically do
 /// so.
-#[derive(Reflect, FromReflect)]
-#[reflect(from_reflect = false)]
+#[derive(Reflect)]
 pub struct TranslationCurveEvaluator {
     evaluator: BasicAnimationCurveEvaluator<Vec3>,
 }
@@ -444,8 +442,7 @@ pub struct RotationCurve<C>(pub C);
 ///
 /// You shouldn't need to instantiate this manually; Bevy will automatically do
 /// so.
-#[derive(Reflect, FromReflect)]
-#[reflect(from_reflect = false)]
+#[derive(Reflect)]
 pub struct RotationCurveEvaluator {
     evaluator: BasicAnimationCurveEvaluator<Quat>,
 }
@@ -542,8 +539,7 @@ pub struct ScaleCurve<C>(pub C);
 ///
 /// You shouldn't need to instantiate this manually; Bevy will automatically do
 /// so.
-#[derive(Reflect, FromReflect)]
-#[reflect(from_reflect = false)]
+#[derive(Reflect)]
 pub struct ScaleCurveEvaluator {
     evaluator: BasicAnimationCurveEvaluator<Vec3>,
 }
@@ -636,8 +632,7 @@ impl AnimationCurveEvaluator for ScaleCurveEvaluator {
 #[reflect(from_reflect = false)]
 pub struct WeightsCurve<C>(pub C);
 
-#[derive(Reflect, FromReflect)]
-#[reflect(from_reflect = false)]
+#[derive(Reflect)]
 struct WeightsCurveEvaluator {
     /// The values of the stack, in which each element is a list of morph target
     /// weights.
@@ -825,8 +820,7 @@ impl AnimationCurveEvaluator for WeightsCurveEvaluator {
     }
 }
 
-#[derive(Reflect, FromReflect)]
-#[reflect(from_reflect = false)]
+#[derive(Reflect)]
 struct BasicAnimationCurveEvaluator<A>
 where
     A: Animatable,
@@ -835,8 +829,7 @@ where
     blend_register: Option<(A, f32)>,
 }
 
-#[derive(Reflect, FromReflect)]
-#[reflect(from_reflect = false)]
+#[derive(Reflect)]
 struct BasicAnimationCurveEvaluatorStackElement<A>
 where
     A: Animatable,