Animation: New Euler filter implementation

This new discontinuity filter performs actions on the entire Euler
rotation, rather than only on the individual X/Y/Z channels. This makes
it fix a wider range of discontinuities, for example those in T52744.

The filter now runs twice on the selected channels, in this order:
- New: Convert X+Y+Z rotation to matrix, then back to Euler angles.
- Old: Add/remove factors of 360° to minimize jumps.

The messaging is streamlined; it now reports how many channels were
filtered, and only warns (instead of errors) when there was an actual
problem with the selected channels (like selecting three or more
channels, but without X/Y/Z triplet).

A new kernel function `BKE_fcurve_keyframe_move_value_with_handles()` is
introduced, to make it possible to move a keyframe's value and move its
handles at the same time.

Manifest Task: T52744

Reviewed By: looch

Differential Revision: https://developer.blender.org/D9602

5 files changed, 309 insertions, 68 deletions

diff --git a/source/blender/blenkernel/BKE_fcurve.h b/source/blender/blenkernel/BKE_fcurve.h
index f527f40d0d7..4569e68ea4a 100644
--- a/source/blender/blenkernel/BKE_fcurve.h
+++ b/source/blender/blenkernel/BKE_fcurve.h
@@ -248,6 +248,10 @@ bool BKE_fcurve_calc_bounds(struct FCurve *fcu,
 void BKE_fcurve_active_keyframe_set(struct FCurve *fcu, const struct BezTriple *active_bezt);
 int BKE_fcurve_active_keyframe_index(const struct FCurve *fcu);
 
+/* Move the indexed keyframe to the given value, and move the handles with it to ensure the slope
+ * remains the same. */
+void BKE_fcurve_keyframe_move_value_with_handles(struct BezTriple *keyframe, float new_value);
+
 /* .............. */
 
 /* Are keyframes on F-Curve of any use (to final result, and to show in editors)? */
diff --git a/source/blender/blenkernel/intern/fcurve.c b/source/blender/blenkernel/intern/fcurve.c
index e6d3696b198..bcd2fe098b7 100644
--- a/source/blender/blenkernel/intern/fcurve.c
+++ b/source/blender/blenkernel/intern/fcurve.c
@@ -884,6 +884,14 @@ int BKE_fcurve_active_keyframe_index(const FCurve *fcu)
 
 /** \} */
 
+void BKE_fcurve_keyframe_move_value_with_handles(struct BezTriple *keyframe, const float new_value)
+{
+  const float value_delta = new_value - keyframe->vec[1][1];
+  keyframe->vec[0][1] += value_delta;
+  keyframe->vec[1][1] = new_value;
+  keyframe->vec[2][1] += value_delta;
+}
+
 /* -------------------------------------------------------------------- */
 /** \name Status Checks
  * \{ */
diff --git a/source/blender/blenkernel/intern/fcurve_test.cc b/source/blender/blenkernel/intern/fcurve_test.cc
index fb6ce02d146..7d6b5fd88be 100644
--- a/source/blender/blenkernel/intern/fcurve_test.cc
+++ b/source/blender/blenkernel/intern/fcurve_test.cc
@@ -331,4 +331,35 @@ TEST(fcurve_active_keyframe, ActiveKeyframe)
   BKE_fcurve_free(fcu);
 }
 
+TEST(BKE_fcurve, BKE_fcurve_keyframe_move_value_with_handles)
+{
+  FCurve *fcu = BKE_fcurve_create();
+
+  insert_vert_fcurve(fcu, 1.0f, 7.5f, BEZT_KEYTYPE_KEYFRAME, INSERTKEY_NO_USERPREF);
+  insert_vert_fcurve(fcu, 8.0f, 15.0f, BEZT_KEYTYPE_KEYFRAME, INSERTKEY_NO_USERPREF);
+  insert_vert_fcurve(fcu, 14.0f, 8.2f, BEZT_KEYTYPE_KEYFRAME, INSERTKEY_NO_USERPREF);
+
+  EXPECT_FLOAT_EQ(fcu->bezt[1].vec[0][0], 5.2671194f);
+  EXPECT_FLOAT_EQ(fcu->bezt[1].vec[0][1], 15.0f);
+
+  EXPECT_FLOAT_EQ(fcu->bezt[1].vec[1][0], 8.0f);
+  EXPECT_FLOAT_EQ(fcu->bezt[1].vec[1][1], 15.0f);
+
+  EXPECT_FLOAT_EQ(fcu->bezt[1].vec[2][0], 10.342469f);
+  EXPECT_FLOAT_EQ(fcu->bezt[1].vec[2][1], 15.0f);
+
+  BKE_fcurve_keyframe_move_value_with_handles(&fcu->bezt[1], 47.0f);
+
+  EXPECT_FLOAT_EQ(fcu->bezt[1].vec[0][0], 5.2671194f) << "Left handle should not move in time";
+  EXPECT_FLOAT_EQ(fcu->bezt[1].vec[0][1], 47.0f) << "Left handle value should have been updated";
+
+  EXPECT_FLOAT_EQ(fcu->bezt[1].vec[1][0], 8.0f) << "Frame should not move in time";
+  EXPECT_FLOAT_EQ(fcu->bezt[1].vec[1][1], 47.0f) << "Frame value should have been updated";
+
+  EXPECT_FLOAT_EQ(fcu->bezt[1].vec[2][0], 10.342469f) << "Right handle should not move in time";
+  EXPECT_FLOAT_EQ(fcu->bezt[1].vec[2][1], 47.0f) << "Right handle value should have been updated";
+
+  BKE_fcurve_free(fcu);
+}
+
 }  // namespace blender::bke::tests
diff --git a/source/blender/editors/space_graph/graph_edit.c b/source/blender/editors/space_graph/graph_edit.c
index 09440ee7d98..bcd3adec1ab 100644
--- a/source/blender/editors/space_graph/graph_edit.c
+++ b/source/blender/editors/space_graph/graph_edit.c
@@ -2610,6 +2610,14 @@ typedef struct tEulerFilter {
   const char *rna_path;
 } tEulerFilter;
 
+static bool keyframe_time_differs(BezTriple *keyframes[3])
+{
+  const float precision = 1e-5;
+  return fabs(keyframes[0]->vec[1][0] - keyframes[1]->vec[1][0]) > precision ||
+         fabs(keyframes[1]->vec[1][0] - keyframes[2]->vec[1][0]) > precision ||
+         fabs(keyframes[0]->vec[1][0] - keyframes[2]->vec[1][0]) > precision;
+}
+
 /* Find groups of `rotation_euler` channels. */
 static ListBase /*tEulerFilter*/ euler_filter_group_channels(
     const ListBase /*bAnimListElem*/ *anim_data, ReportList *reports, int *r_num_groups)
@@ -2666,68 +2674,151 @@ static ListBase /*tEulerFilter*/ euler_filter_group_channels(
   return euler_groups;
 }
 
-static int euler_filter_perform_filter(ListBase /*tEulerFilter*/ *eulers, ReportList *reports)
+/* Perform discontinuity filter based on conversion to matrix and back.
+ * Return true if the curves were filtered (which may have been a no-op), false otherwise. */
+static bool euler_filter_multi_channel(tEulerFilter *euf, ReportList *reports)
+{
+  /* Sanity check: ensure that there are enough F-Curves to work on in this group. */
+  if (ELEM(NULL, euf->fcurves[0], euf->fcurves[1], euf->fcurves[2])) {
+    /* Report which components are missing. */
+    BKE_reportf(reports,
+                RPT_INFO,
+                "Missing %s%s%s component(s) of euler rotation for ID='%s' and RNA-Path='%s'",
+                (euf->fcurves[0] == NULL) ? "X" : "",
+                (euf->fcurves[1] == NULL) ? "Y" : "",
+                (euf->fcurves[2] == NULL) ? "Z" : "",
+                euf->id->name,
+                euf->rna_path);
+    return false;
+  }
+
+  FCurve *fcu_rot_x = euf->fcurves[0];
+  FCurve *fcu_rot_y = euf->fcurves[1];
+  FCurve *fcu_rot_z = euf->fcurves[2];
+  if (fcu_rot_x->totvert != fcu_rot_y->totvert || fcu_rot_y->totvert != fcu_rot_z->totvert) {
+    BKE_reportf(reports,
+                RPT_INFO,
+                "XYZ rotations not equally keyed for ID='%s' and RNA-Path='%s'",
+                euf->id->name,
+                euf->rna_path);
+    return false;
+  }
+
+  if (fcu_rot_x->totvert < 2) {
+    /* Empty curves and single keyframes are trivially "filtered". */
+    return false;
+  }
+
+  float filtered_euler[3] = {
+      fcu_rot_x->bezt[0].vec[1][1],
+      fcu_rot_y->bezt[0].vec[1][1],
+      fcu_rot_z->bezt[0].vec[1][1],
+  };
+
+  for (int keyframe_index = 1; keyframe_index < fcu_rot_x->totvert; ++keyframe_index) {
+    BezTriple *keyframes[3] = {
+        &fcu_rot_x->bezt[keyframe_index],
+        &fcu_rot_y->bezt[keyframe_index],
+        &fcu_rot_z->bezt[keyframe_index],
+    };
+
+    if (keyframe_time_differs(keyframes)) {
+      /* The X-coordinates of the keyframes are different, so we cannot correct this key. */
+      continue;
+    }
+
+    const float unfiltered_euler[3] = {
+        keyframes[0]->vec[1][1],
+        keyframes[1]->vec[1][1],
+        keyframes[2]->vec[1][1],
+    };
+
+    /* The conversion back from matrix to Euler angles actually performs the filtering. */
+    float matrix[3][3];
+    eul_to_mat3(matrix, unfiltered_euler);
+    mat3_normalized_to_compatible_eul(filtered_euler, filtered_euler, matrix);
+
+    /* TODO(Sybren): it might be a nice touch to compare `filtered_euler` with `unfiltered_euler`,
+     * to see if there was actually a change. This could improve reporting for the artist. */
+
+    BKE_fcurve_keyframe_move_value_with_handles(keyframes[0], filtered_euler[0]);
+    BKE_fcurve_keyframe_move_value_with_handles(keyframes[1], filtered_euler[1]);
+    BKE_fcurve_keyframe_move_value_with_handles(keyframes[2], filtered_euler[2]);
+  }
+
+  return true;
+}
+
+/* Remove 360-degree flips from a single FCurve.
+ * Return true if the curve was modified, false otherwise. */
+static bool euler_filter_single_channel(FCurve *fcu)
 {
-  int failed = 0;
+  /* Simple method: just treat any difference between keys of greater than 180 degrees as being a
+   * flip. */
+  BezTriple *bezt, *prev;
+  uint i;
 
-  LISTBASE_FOREACH (tEulerFilter *, euf, eulers) {
-    /* Sanity check: ensure that there are enough F-Curves to work on in this group. */
-    /* TODO: also enforce assumption that there be a full set of keyframes
-     * at each position by ensuring that totvert counts are same? (Joshua Leung 2011) */
-    if (ELEM(NULL, euf->fcurves[0], euf->fcurves[1], euf->fcurves[2])) {
-      /* Report which components are missing. */
-      BKE_reportf(reports,
-                  RPT_WARNING,
-                  "Missing %s%s%s component(s) of euler rotation for ID='%s' and RNA-Path='%s'",
-                  (euf->fcurves[0] == NULL) ? "X" : "",
-                  (euf->fcurves[1] == NULL) ? "Y" : "",
-                  (euf->fcurves[2] == NULL) ? "Z" : "",
-                  euf->id->name,
-                  euf->rna_path);
-
-      /* Keep track of number of failed sets, and carry on to next group. */
-      failed++;
+  /* Skip if not enough verts to do a decent analysis. */
+  if (fcu->totvert <= 2) {
+    return false;
+  }
+
+  /* Prev follows bezt, bezt = "current" point to be fixed. */
+  /* Our method depends on determining a "difference" from the previous vert. */
+  bool is_modified = false;
+  for (i = 1, prev = fcu->bezt, bezt = fcu->bezt + 1; i < fcu->totvert; i++, prev = bezt++) {
+    const float sign = (prev->vec[1][1] > bezt->vec[1][1]) ? 1.0f : -1.0f;
+
+    /* >= 180 degree flip? */
+    if ((sign * (prev->vec[1][1] - bezt->vec[1][1])) < (float)M_PI) {
       continue;
     }
 
-    /* Simple method: just treat any difference between
-     * keys of greater than 180 degrees as being a flip. */
-    /* FIXME: there are more complicated methods that
-     * will be needed to fix more cases than just some */
-    for (int f = 0; f < 3; f++) {
-      FCurve *fcu = euf->fcurves[f];
-      BezTriple *bezt, *prev;
-      uint i;
-
-      /* Skip if not enough vets to do a decent analysis of.... */
-      if (fcu->totvert <= 2) {
-        continue;
-      }
+    /* 360 degrees to add/subtract frame value until difference
+     * is acceptably small that there's no more flip. */
+    const float fac = sign * 2.0f * (float)M_PI;
 
-      /* Prev follows bezt, bezt = "current" point to be fixed. */
-      /* Our method depends on determining a "difference" from the previous vert. */
-      for (i = 1, prev = fcu->bezt, bezt = fcu->bezt + 1; i < fcu->totvert; i++, prev = bezt++) {
-        const float sign = (prev->vec[1][1] > bezt->vec[1][1]) ? 1.0f : -1.0f;
+    while ((sign * (prev->vec[1][1] - bezt->vec[1][1])) >= (float)M_PI) {
+      bezt->vec[0][1] += fac;
+      bezt->vec[1][1] += fac;
+      bezt->vec[2][1] += fac;
+    }
 
-        /* >= 180 degree flip? */
-        if ((sign * (prev->vec[1][1] - bezt->vec[1][1])) < (float)M_PI) {
-          continue;
-        }
+    is_modified = true;
+  }
 
-        /* 360 degrees to add/subtract frame value until difference
-         * is acceptably small that there's no more flip. */
-        const float fac = sign * 2.0f * (float)M_PI;
+  return is_modified;
+}
 
-        while ((sign * (prev->vec[1][1] - bezt->vec[1][1])) >= (float)M_PI) {
-          bezt->vec[0][1] += fac;
-          bezt->vec[1][1] += fac;
-          bezt->vec[2][1] += fac;
-        }
+static void euler_filter_perform_filter(ListBase /*tEulerFilter*/ *eulers,
+                                        ReportList *reports,
+                                        int *r_curves_filtered,
+                                        int *r_curves_seen)
+{
+  *r_curves_filtered = 0;
+  *r_curves_seen = 0;
+
+  LISTBASE_FOREACH (tEulerFilter *, euf, eulers) {
+    int curves_filtered_this_group = 0;
+
+    if (euler_filter_multi_channel(euf, reports)) {
+      curves_filtered_this_group = 3;
+    }
+
+    for (int channel_index = 0; channel_index < 3; channel_index++) {
+      FCurve *fcu = euf->fcurves[channel_index];
+      if (fcu == NULL) {
+        continue;
+      }
+      ++*r_curves_seen;
+
+      if (euler_filter_single_channel(fcu)) {
+        ++curves_filtered_this_group;
       }
     }
-  }
 
-  return failed;
+    *r_curves_filtered += min_ii(3, curves_filtered_this_group);
+  }
 }
 
 static int graphkeys_euler_filter_exec(bContext *C, wmOperator *op)
@@ -2764,30 +2855,43 @@ static int graphkeys_euler_filter_exec(bContext *C, wmOperator *op)
   /* Step 2: go through each set of curves, processing the values at each keyframe.
    * - It is assumed that there must be a full set of keyframes at each keyframe position.
    */
-  const int failed = euler_filter_perform_filter(&eulers, op->reports);
-  BLI_freelistN(&eulers);
+  int curves_filtered;
+  int curves_seen;
+  euler_filter_perform_filter(&eulers, op->reports, &curves_filtered, &curves_seen);
 
+  BLI_freelistN(&eulers);
   ANIM_animdata_update(&ac, &anim_data);
   ANIM_animdata_freelist(&anim_data);
 
-  /* Updates + finishing warnings. */
-  if (failed == groups) {
-    BKE_report(
-        op->reports,
-        RPT_ERROR,
-        "No Euler Rotations could be corrected, ensure each rotation has keys for all components, "
-        "and that F-Curves for these are in consecutive XYZ order and selected");
+  if (curves_filtered == 0) {
+    if (curves_seen < 3) {
+      /* Showing the entire error message makes no sense when the artist is only trying to filter
+       * one or two curves. */
+      BKE_report(op->reports, RPT_WARNING, "No Euler Rotations could be corrected.");
+    }
+    else {
+      BKE_report(op->reports,
+                 RPT_ERROR,
+                 "No Euler Rotations could be corrected, ensure each rotation has keys for all "
+                 "components, "
+                 "and that F-Curves for these are in consecutive XYZ order and selected");
+    }
     return OPERATOR_CANCELLED;
   }
 
-  if (failed) {
-    BKE_report(
-        op->reports,
-        RPT_ERROR,
-        "Some Euler Rotations could not be corrected due to missing/unselected/out-of-order "
-        "F-Curves, "
-        "ensure each rotation has keys for all components, and that F-Curves for these are in "
-        "consecutive XYZ order and selected");
+  if (curves_filtered != curves_seen) {
+    BLI_assert(curves_filtered < curves_seen);
+    BKE_reportf(op->reports,
+                RPT_INFO,
+                "%d of %d rotation channels were filtered. See the Info window for details.",
+                curves_filtered,
+                curves_seen);
+  }
+  else if (curves_seen == 1) {
+    BKE_report(op->reports, RPT_INFO, "The rotation channel was filtered.");
+  }
+  else {
+    BKE_reportf(op->reports, RPT_INFO, "All %d rotation channels were filtered.", curves_seen);
   }
 
   /* Set notifier that keyframes have changed. */
diff --git a/tests/python/bl_animation_fcurves.py b/tests/python/bl_animation_fcurves.py
index b5772b8d335..2ec04749d70 100644
--- a/tests/python/bl_animation_fcurves.py
+++ b/tests/python/bl_animation_fcurves.py
@@ -25,11 +25,13 @@ blender -b -noaudio --factory-startup --python tests/python/bl_animation_fcurves
 import pathlib
 import sys
 import unittest
+from math import degrees, radians
+from typing import List
 
 import bpy
 
 
-class FCurveEvaluationTest(unittest.TestCase):
+class AbstractAnimationTest:
     @classmethod
     def setUpClass(cls):
         cls.testdir = args.testdir
@@ -38,6 +40,7 @@ class FCurveEvaluationTest(unittest.TestCase):
         self.assertTrue(self.testdir.exists(),
                         'Test dir %s should exist' % self.testdir)
 
+class FCurveEvaluationTest(AbstractAnimationTest, unittest.TestCase):
     def test_fcurve_versioning_291(self):
         # See D8752.
         bpy.ops.wm.open_mainfile(filepath=str(self.testdir / "fcurve-versioning-291.blend"))
@@ -73,6 +76,97 @@ class FCurveEvaluationTest(unittest.TestCase):
         self.assertAlmostEqual(1.0, fcurve.evaluate(10))
 
 
+class EulerFilterTest(AbstractAnimationTest, unittest.TestCase):
+    def setUp(self):
+        super().setUp()
+        bpy.ops.wm.open_mainfile(filepath=str(self.testdir / "euler-filter.blend"))
+
+    def test_multi_channel_filter(self):
+        """Test fixing discontinuities that require all X/Y/Z rotations to work."""
+
+        self.activate_object('Three-Channel-Jump')
+        fcu_rot = self.active_object_rotation_channels()
+
+        ## Check some pre-filter values to make sure the file is as we expect.
+        # Keyframes before the "jump". These shouldn't be touched by the filter.
+        self.assertEqualAngle(-87.5742, fcu_rot[0], 22)
+        self.assertEqualAngle(69.1701, fcu_rot[1], 22)
+        self.assertEqualAngle(-92.3918, fcu_rot[2], 22)
+        # Keyframes after the "jump". These should be updated by the filter.
+        self.assertEqualAngle(81.3266, fcu_rot[0], 23)
+        self.assertEqualAngle(111.422, fcu_rot[1], 23)
+        self.assertEqualAngle(76.5996, fcu_rot[2], 23)
+
+        bpy.ops.graph.euler_filter(self.get_context())
+
+        # Keyframes before the "jump". These shouldn't be touched by the filter.
+        self.assertEqualAngle(-87.5742, fcu_rot[0], 22)
+        self.assertEqualAngle(69.1701, fcu_rot[1], 22)
+        self.assertEqualAngle(-92.3918, fcu_rot[2], 22)
+        # Keyframes after the "jump". These should be updated by the filter.
+        self.assertEqualAngle(-98.6734, fcu_rot[0], 23)
+        self.assertEqualAngle(68.5783, fcu_rot[1], 23)
+        self.assertEqualAngle(-103.4, fcu_rot[2], 23)
+
+    def test_single_channel_filter(self):
+        """Test fixing discontinuities in single channels."""
+
+        self.activate_object('One-Channel-Jumps')
+        fcu_rot = self.active_object_rotation_channels()
+
+        ## Check some pre-filter values to make sure the file is as we expect.
+        # Keyframes before the "jump". These shouldn't be touched by the filter.
+        self.assertEqualAngle(360, fcu_rot[0], 15)
+        self.assertEqualAngle(396, fcu_rot[1], 21)  # X and Y are keyed on different frames.
+        # Keyframes after the "jump". These should be updated by the filter.
+        self.assertEqualAngle(720, fcu_rot[0], 16)
+        self.assertEqualAngle(72, fcu_rot[1], 22)
+
+        bpy.ops.graph.euler_filter(self.get_context())
+
+        # Keyframes before the "jump". These shouldn't be touched by the filter.
+        self.assertEqualAngle(360, fcu_rot[0], 15)
+        self.assertEqualAngle(396, fcu_rot[1], 21)  # X and Y are keyed on different frames.
+        # Keyframes after the "jump". These should be updated by the filter.
+        self.assertEqualAngle(360, fcu_rot[0], 16)
+        self.assertEqualAngle(432, fcu_rot[1], 22)
+
+    def assertEqualAngle(self, angle_degrees: float, fcurve: bpy.types.FCurve, frame: int) -> None:
+        self.assertAlmostEqual(
+            radians(angle_degrees),
+            fcurve.evaluate(frame),
+            4,
+            "Expected %.3f degrees, but FCurve %s[%d] evaluated to %.3f on frame %d" % (
+                angle_degrees, fcurve.data_path, fcurve.array_index, degrees(fcurve.evaluate(frame)), frame,
+            )
+        )
+
+    @staticmethod
+    def get_context():
+        ctx = bpy.context.copy()
+
+        for area in bpy.context.window.screen.areas:
+            if area.type != 'GRAPH_EDITOR':
+                continue
+
+            ctx['area'] = area
+            ctx['space'] = area.spaces.active
+            break
+
+        return ctx
+
+    @staticmethod
+    def activate_object(object_name: str) -> None:
+        ob = bpy.data.objects[object_name]
+        bpy.context.view_layer.objects.active = ob
+
+    @staticmethod
+    def active_object_rotation_channels() -> List[bpy.types.FCurve]:
+        ob = bpy.context.view_layer.objects.active
+        action = ob.animation_data.action
+        return [action.fcurves.find('rotation_euler', index=idx) for idx in range(3)]
+
+
 def main():
     global args
     import argparse