Fix T75881: Animation, limitation of Bézier Handles

Relax limits of FCurve Bézier handles during evaluation. FCurve handles
can be scaled down to avoid the curve looping backward in time. This
scaling was done correctly but over-carefully, posing unnecessary
limitations on the possible slope of FCurves. This commit changes the
scaling approach such that the FCurve can become near-vertical.

Bump Blender's subversion from 291.0.1 to 291.0.2 to ensure that older
animation files are correctly updated.

Reviewed By: sybren

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

source/blender/blenkernel/BKE_blender_version.h

@@ -39,7 +39,7 @@ extern "C" {
 
 /* Blender file format version. */
 #define BLENDER_FILE_VERSION BLENDER_VERSION
-#define BLENDER_FILE_SUBVERSION 1
+#define BLENDER_FILE_SUBVERSION 2
 
 /* Minimum Blender version that supports reading file written with the current
  * version. Older Blender versions will test this and show a warning if the file

source/blender/blenkernel/intern/fcurve.c

@@ -1308,7 +1308,7 @@ bool test_time_fcurve(FCurve *fcu)
 /** \name F-Curve Calculations
  * \{ */
 
-/* The total length of the handles is not allowed to be more
+/* The length of each handle is not allowed to be more
  * than the horizontal distance between (v1-v4).
  * This is to prevent curve loops.
  */
@@ -1337,15 +1337,17 @@ void correct_bezpart(const float v1[2], float v2[2], float v3[2], const float v4
     return;
   }
 
-  /* the two handles cross over each other, so force them
-   * apart using the proportion they overlap
+  /* To prevent looping or rewinding, handles cannot
+   * exceed the adjacent's keyframe time position.
    */
-  if ((len1 + len2) > len) {
-    fac = len / (len1 + len2);
-
+  if (len1 > len) {
+    fac = len / len1;
     v2[0] = (v1[0] - fac * h1[0]);
     v2[1] = (v1[1] - fac * h1[1]);
+  }
 
+  if (len2 > len) {
+    fac = len / len2;
     v3[0] = (v4[0] - fac * h2[0]);
     v3[1] = (v4[1] - fac * h2[1]);
   }

source/blender/blenloader/intern/versioning_290.c

@@ -25,6 +25,7 @@
 #include "BLI_string.h"
 #include "BLI_utildefines.h"
 
+#include "DNA_anim_types.h"
 #include "DNA_brush_types.h"
 #include "DNA_cachefile_types.h"
 #include "DNA_constraint_types.h"
@@ -272,6 +273,54 @@ static void do_versions_point_attributes(CustomData *pdata)
   }
 }
 
+/* Move FCurve handles towards the control point in such a way that the curve itself doesn't
+ * change. Since 2.91 FCurves are computed slightly differently, which requires this update to keep
+ * the same animation result. Previous versions scaled down overlapping handles during evaluation.
+ * This function applies the old correction to the actual animation data instead. */
+static void do_versions_291_fcurve_handles_limit(FCurve *fcu)
+{
+  uint i = 1;
+  for (BezTriple *bezt = fcu->bezt; i < fcu->totvert; i++, bezt++) {
+    /* Only adjust bezier keyframes. */
+    if (bezt->ipo != BEZT_IPO_BEZ) {
+      continue;
+    }
+
+    BezTriple *nextbezt = bezt + 1;
+    const float v1[2] = {bezt->vec[1][0], bezt->vec[1][1]};
+    const float v2[2] = {bezt->vec[2][0], bezt->vec[2][1]};
+    const float v3[2] = {nextbezt->vec[0][0], nextbezt->vec[0][1]};
+    const float v4[2] = {nextbezt->vec[1][0], nextbezt->vec[1][1]};
+
+    /* If the handles have no length, no need to do any corrections. */
+    if (v1[0] == v2[0] && v3[0] == v4[0]) {
+      continue;
+    }
+
+    /* Calculate handle deltas. */
+    float delta1[2], delta2[2];
+    sub_v2_v2v2(delta1, v1, v2);
+    sub_v2_v2v2(delta2, v4, v3);
+
+    const float len1 = fabsf(delta1[0]); /* Length of handle of first key. */
+    const float len2 = fabsf(delta2[0]); /* Length of handle of second key. */
+
+    /* Overlapping handles used to be internally scaled down in previous versions.
+     * We bake the handles onto these previously virtual values. */
+    const float time_delta = v4[0] - v1[0];
+    const float total_len = len1 + len2;
+    if (total_len <= time_delta) {
+      continue;
+    }
+
+    const float factor = time_delta / total_len;
+    /* Current keyframe's right handle: */
+    madd_v2_v2v2fl(bezt->vec[2], v1, delta1, -factor); /* vec[2] = v1 - factor * delta1 */
+    /* Next keyframe's left handle: */
+    madd_v2_v2v2fl(nextbezt->vec[0], v4, delta2, -factor); /* vec[0] = v4 - factor * delta2 */
+  }
+}
+
 void blo_do_versions_290(FileData *fd, Library *UNUSED(lib), Main *bmain)
 {
   UNUSED_VARS(fd);
@@ -528,16 +577,7 @@ void blo_do_versions_290(FileData *fd, Library *UNUSED(lib), Main *bmain)
     }
   }
 
-  /**
-   * Versioning code until next subversion bump goes here.
-   *
-   * \note Be sure to check when bumping the version:
-   * - "versioning_userdef.c", #BLO_version_defaults_userpref_blend
-   * - "versioning_userdef.c", #do_versions_theme
-   *
-   * \note Keep this message at the bottom of the function.
-   */
-  {
+  if (!MAIN_VERSION_ATLEAST(bmain, 291, 2)) {
     for (Scene *scene = bmain->scenes.first; scene; scene = scene->id.next) {
       RigidBodyWorld *rbw = scene->rigidbody_world;
 
@@ -598,6 +638,28 @@ void blo_do_versions_290(FileData *fd, Library *UNUSED(lib), Main *bmain)
       }
     }
 
+    /* Fix fcurves to allow for new bezier handles behaviour (T75881 and D8752). */
+    for (bAction *act = bmain->actions.first; act; act = act->id.next) {
+      for (FCurve *fcu = act->curves.first; fcu; fcu = fcu->next) {
+        /* Only need to fix Bezier curves with at least 2 keyframes. */
+        if (fcu->totvert < 2 || fcu->bezt == NULL) {
+          return;
+        }
+        do_versions_291_fcurve_handles_limit(fcu);
+      }
+    }
+  }
+
+  /**
+   * Versioning code until next subversion bump goes here.
+   *
+   * \note Be sure to check when bumping the version:
+   * - "versioning_userdef.c", #BLO_version_defaults_userpref_blend
+   * - "versioning_userdef.c", #do_versions_theme
+   *
+   * \note Keep this message at the bottom of the function.
+   */
+  {
     /* Keep this block, even when empty. */
   }
 }

source/blender/blenloader/intern/versioning_userdef.c

@@ -216,6 +216,14 @@ static void do_versions_theme(const UserDef *userdef, bTheme *btheme)
     FROM_DEFAULT_V4_UCHAR(tui.transparent_checker_secondary);
     btheme->tui.transparent_checker_size = U_theme_default.tui.transparent_checker_size;
   }
+  if (!USER_VERSION_ATLEAST(291, 2)) {
+    /* The new defaults for the file browser theme are the same as
+     * the outliner's, and it's less disruptive to just copy them. */
+    copy_v4_v4_uchar(btheme->space_file.back, btheme->space_outliner.back);
+    copy_v4_v4_uchar(btheme->space_file.row_alternate, btheme->space_outliner.row_alternate);
+
+    FROM_DEFAULT_V4_UCHAR(space_image.grid);
+  }
 
   /**
    * Versioning code until next subversion bump goes here.
@@ -228,13 +236,6 @@ static void do_versions_theme(const UserDef *userdef, bTheme *btheme)
    */
   {
     /* Keep this block, even when empty. */
-
-    /* The new defaults for the file browser theme are the same as
-     * the outliner's, and it's less disruptive to just copy them. */
-    copy_v4_v4_uchar(btheme->space_file.back, btheme->space_outliner.back);
-    copy_v4_v4_uchar(btheme->space_file.row_alternate, btheme->space_outliner.row_alternate);
-
-    FROM_DEFAULT_V4_UCHAR(space_image.grid);
   }
 
 #undef FROM_DEFAULT_V4_UCHAR

tests/python/CMakeLists.txt

@@ -217,6 +217,15 @@ add_blender_test(
   --run-all-tests
 )
 
+# ------------------------------------------------------------------------------
+# ANIMATION TESTS
+add_blender_test(
+  bl_animation_fcurves
+  --python ${CMAKE_CURRENT_LIST_DIR}/bl_animation_fcurves.py
+  --
+  --testdir "${TEST_SRC_DIR}/animation"
+)
+
 # ------------------------------------------------------------------------------
 # IO TESTS
 

tests/python/bl_animation_fcurves.py

@@ -0,0 +1,92 @@
+# ##### BEGIN GPL LICENSE BLOCK #####
+#
+#  This program is free software; you can redistribute it and/or
+#  modify it under the terms of the GNU General Public License
+#  as published by the Free Software Foundation; either version 2
+#  of the License, or (at your option) any later version.
+#
+#  This program is distributed in the hope that it will be useful,
+#  but WITHOUT ANY WARRANTY; without even the implied warranty of
+#  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+#  GNU General Public License for more details.
+#
+#  You should have received a copy of the GNU General Public License
+#  along with this program; if not, write to the Free Software Foundation,
+#  Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
+#
+# ##### END GPL LICENSE BLOCK #####
+
+# <pep8 compliant>
+
+"""
+blender -b -noaudio --factory-startup --python tests/python/bl_animation_fcurves.py -- --testdir /path/to/lib/tests/animation
+"""
+
+import pathlib
+import sys
+import unittest
+
+import bpy
+
+class FCurveEvaluationTest(unittest.TestCase):
+    @classmethod
+    def setUpClass(cls):
+        cls.testdir = args.testdir
+      
+    def setUp(self):
+        self.assertTrue(self.testdir.exists(),
+                        'Test dir %s should exist' % self.testdir)
+
+    def test_fcurve_versioning_291(self):
+      # See D8752.
+      bpy.ops.wm.open_mainfile(filepath=str(self.testdir / "fcurve-versioning-291.blend"))
+      cube = bpy.data.objects['Cube']
+      fcurve = cube.animation_data.action.fcurves.find('location', index=0)
+
+      self.assertAlmostEqual(0.0, fcurve.evaluate(1))
+      self.assertAlmostEqual(0.019638698548078537, fcurve.evaluate(2))
+      self.assertAlmostEqual(0.0878235399723053, fcurve.evaluate(3))
+      self.assertAlmostEqual(0.21927043795585632, fcurve.evaluate(4))
+      self.assertAlmostEqual(0.41515052318573, fcurve.evaluate(5))
+      self.assertAlmostEqual(0.6332430243492126, fcurve.evaluate(6))
+      self.assertAlmostEqual(0.8106040954589844, fcurve.evaluate(7))
+      self.assertAlmostEqual(0.924369215965271, fcurve.evaluate(8))
+      self.assertAlmostEqual(0.9830065965652466, fcurve.evaluate(9))
+      self.assertAlmostEqual(1.0, fcurve.evaluate(10))
+
+    def test_fcurve_extreme_handles(self):
+      # See D8752.
+      bpy.ops.wm.open_mainfile(filepath=str(self.testdir / "fcurve-extreme-handles.blend"))
+      cube = bpy.data.objects['Cube']
+      fcurve = cube.animation_data.action.fcurves.find('location', index=0)
+
+      self.assertAlmostEqual(0.0, fcurve.evaluate(1))
+      self.assertAlmostEqual(0.004713400732725859, fcurve.evaluate(2))
+      self.assertAlmostEqual(0.022335870191454887, fcurve.evaluate(3))
+      self.assertAlmostEqual(0.06331237405538559, fcurve.evaluate(4))
+      self.assertAlmostEqual(0.16721539199352264, fcurve.evaluate(5))
+      self.assertAlmostEqual(0.8327845335006714, fcurve.evaluate(6))
+      self.assertAlmostEqual(0.9366875886917114, fcurve.evaluate(7))
+      self.assertAlmostEqual(0.9776642322540283, fcurve.evaluate(8))
+      self.assertAlmostEqual(0.9952865839004517, fcurve.evaluate(9))
+      self.assertAlmostEqual(1.0, fcurve.evaluate(10))
+
+
+def main():
+    global args
+    import argparse
+
+    if '--' in sys.argv:
+        argv = [sys.argv[0]] + sys.argv[sys.argv.index('--') + 1:]
+    else:
+        argv = sys.argv
+
+    parser = argparse.ArgumentParser()
+    parser.add_argument('--testdir', required=True, type=pathlib.Path)
+    args, remaining = parser.parse_known_args(argv)
+
+    unittest.main(argv=remaining)
+
+
+if __name__ == "__main__":
+    main()