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

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)? */

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
  * \{ */

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

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)
 {
-  int failed = 0;
+  /* 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;
+  }
 
-  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);
+  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;
+  }
 
-      /* Keep track of number of failed sets, and carry on to next group. */
-      failed++;
+  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;
     }
 
-    /* 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;
+    const float unfiltered_euler[3] = {
+        keyframes[0]->vec[1][1],
+        keyframes[1]->vec[1][1],
+        keyframes[2]->vec[1][1],
+    };
 
-      /* Skip if not enough vets to do a decent analysis of.... */
-      if (fcu->totvert <= 2) {
-        continue;
-      }
+    /* 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);
 
-      /* 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;
+    /* 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. */
 
-        /* >= 180 degree flip? */
-        if ((sign * (prev->vec[1][1] - bezt->vec[1][1])) < (float)M_PI) {
-          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;
-
-        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;
-        }
-      }
-    }
+    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 failed;
+  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)
+{
+  /* Simple method: just treat any difference between keys of greater than 180 degrees as being a
+   * flip. */
+  BezTriple *bezt, *prev;
+  uint i;
+
+  /* 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;
+    }
+
+    /* 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;
+
+    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;
+    }
+
+    is_modified = true;
+  }
+
+  return is_modified;
+}
+
+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;
+      }
+    }
+
+    *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. */

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