Anim: emit liboverride on slot handle when action is changed

Emit a 'diff' for the `animdata.slot_handle` property whenever the
`.action` property is changed through a library override.

The slot handle is only meaningful within the context of the assigned
action. So when a liboverride changes the assigned action, the slot
handle should also get an override.

This is necessary even when the numerical value of the slot handle
happens to be the same in both actions, as the newly chosen slot is
different from the slot that was chosen in the library file.

This applies to direct Action assignment, NLA strips, and Action
constraints.

Pull Request: https://projects.blender.org/blender/blender/pulls/133727

source/blender/makesrna/intern/rna_action_tools.hh

@@ -53,4 +53,24 @@ void rna_generic_action_slot_handle_set(blender::animrig::slot_handle_t slot_han
 void rna_iterator_generic_action_suitable_slots_begin(CollectionPropertyIterator *iter,
                                                       bAction *assigned_action);
 
+/**
+ * Generic function for handling library overrides on Action slot handle properties.
+ *
+ * This is used for `id.animation_data.action_slot_handle`, and similar properties. These
+ * properties determine which Action Slot is assigned. The reason this needs special code is that
+ * the assigned slot is determined by two properties: the assigned Action, and the slot handle. So
+ * even when the slot handle itself is numerically identical in the library file and the override,
+ * if the Action assignment is overridden, that number indicates a different, unrelated slot.
+ *
+ * In the above case, when the library overrides get applied, first the new Action is assigned.
+ * This will make Blender auto-select a slot, which may fail, resulting in having no slot assigned.
+ * To ensure that the intended slot is assigned after this, this function will emit a library
+ * override operation for the slot handle as well. That way, after the Action is assigned, an
+ * explicit slot will be assigned.
+ */
+void rna_generic_action_slot_handle_override_diff(Main *bmain,
+                                                  RNAPropertyOverrideDiffContext &rnadiff_ctx,
+                                                  const bAction *action_a,
+                                                  const bAction *action_b);
+
 #endif /* RNA_RUNTIME */

source/blender/makesrna/intern/rna_animation.cc

@@ -12,6 +12,7 @@
 
 #include "BLT_translation.hh"
 
+#include "BKE_lib_override.hh"
 #include "BKE_nla.hh"
 
 #include "RNA_define.hh"
@@ -135,6 +136,63 @@ static void rna_AnimData_dependency_update(Main *bmain, Scene *scene, PointerRNA
   rna_AnimData_update(bmain, scene, ptr);
 }
 
+void rna_generic_action_slot_handle_override_diff(Main *bmain,
+                                                  RNAPropertyOverrideDiffContext &rnadiff_ctx,
+                                                  const bAction *action_a,
+                                                  const bAction *action_b)
+{
+  rna_property_override_diff_default(bmain, rnadiff_ctx);
+
+  if (rnadiff_ctx.comparison || (rnadiff_ctx.report_flag & RNA_OVERRIDE_MATCH_RESULT_CREATED)) {
+    /* Default diffing found a difference, no need to go further. */
+    return;
+  }
+
+  if (action_a == action_b) {
+    /* Action is unchanged, it's fine to mark the slot handle as unchanged as well. */
+    return;
+  }
+
+  /* Sign doesn't make sense here, as the numerical values are the same. */
+  rnadiff_ctx.comparison = 1;
+
+  /* The remainder of this function was taken from rna_property_override_diff_default(). It's just
+   * formatted a little differently to allow for early returns. */
+
+  const bool do_create = rnadiff_ctx.liboverride != nullptr &&
+                         (rnadiff_ctx.liboverride_flags & RNA_OVERRIDE_COMPARE_CREATE) != 0 &&
+                         rnadiff_ctx.rna_path != nullptr;
+  if (!do_create) {
+    /* Not enough info to create an override operation, so bail out. */
+    return;
+  }
+
+  /* Create the override operation. */
+  bool created = false;
+  IDOverrideLibraryProperty *op = BKE_lib_override_library_property_get(
+      rnadiff_ctx.liboverride, rnadiff_ctx.rna_path, &created);
+
+  if (op && created) {
+    BKE_lib_override_library_property_operation_get(
+        op, LIBOVERRIDE_OP_REPLACE, nullptr, nullptr, {}, {}, -1, -1, true, nullptr, nullptr);
+    rnadiff_ctx.report_flag |= RNA_OVERRIDE_MATCH_RESULT_CREATED;
+  }
+}
+
+/**
+ * Emit a 'diff' for the .slot_handle property whenever the .action property differs.
+ *
+ * \see rna_generic_action_slot_handle_override_diff()
+ */
+static void rna_AnimData_slot_handle_override_diff(Main *bmain,
+                                                   RNAPropertyOverrideDiffContext &rnadiff_ctx)
+{
+  const AnimData *adt_a = static_cast<AnimData *>(rnadiff_ctx.prop_a->ptr->data);
+  const AnimData *adt_b = static_cast<AnimData *>(rnadiff_ctx.prop_b->ptr->data);
+
+  rna_generic_action_slot_handle_override_diff(bmain, rnadiff_ctx, adt_a->action, adt_b->action);
+}
+
 static int rna_AnimData_action_editable(const PointerRNA *ptr, const char ** /*r_info*/)
 {
   BLI_assert(ptr->type == &RNA_AnimData);
@@ -1682,6 +1740,8 @@ static void rna_def_animdata(BlenderRNA *brna)
                            "A number that identifies which sub-set of the Action is considered "
                            "to be for this data-block");
   RNA_def_property_override_flag(prop, PROPOVERRIDE_OVERRIDABLE_LIBRARY);
+  RNA_def_property_override_funcs(
+      prop, "rna_AnimData_slot_handle_override_diff", nullptr, nullptr);
   RNA_def_property_update(prop, NC_ANIMATION | ND_NLA_ACTCHANGE, "rna_AnimData_dependency_update");
 
   prop = RNA_def_property(srna, "last_slot_identifier", PROP_STRING, PROP_NONE);

source/blender/makesrna/intern/rna_constraint.cc

@@ -788,6 +788,23 @@ static void rna_ActionConstraint_action_slot_handle_set(
                                      acon->last_slot_identifier);
 }
 
+/**
+ * Emit a 'diff' for the .action_slot_handle property whenever the .action property differs.
+ *
+ * \see rna_generic_action_slot_handle_override_diff()
+ */
+static void rna_ActionConstraint_action_slot_handle_override_diff(
+    Main *bmain, RNAPropertyOverrideDiffContext &rnadiff_ctx)
+{
+  const bConstraint *con_a = static_cast<bConstraint *>(rnadiff_ctx.prop_a->ptr->data);
+  const bConstraint *con_b = static_cast<bConstraint *>(rnadiff_ctx.prop_b->ptr->data);
+
+  const bActionConstraint *act_con_a = static_cast<bActionConstraint *>(con_a->data);
+  const bActionConstraint *act_con_b = static_cast<bActionConstraint *>(con_b->data);
+
+  rna_generic_action_slot_handle_override_diff(bmain, rnadiff_ctx, act_con_a->act, act_con_b->act);
+}
+
 static PointerRNA rna_ActionConstraint_action_slot_get(PointerRNA *ptr)
 {
   bConstraint *con = (bConstraint *)ptr->data;
@@ -1987,6 +2004,8 @@ static void rna_def_constraint_action(BlenderRNA *brna)
                            "A number that identifies which sub-set of the Action is considered "
                            "to be for this Action Constraint");
   RNA_def_property_override_flag(prop, PROPOVERRIDE_OVERRIDABLE_LIBRARY);
+  RNA_def_property_override_funcs(
+      prop, "rna_ActionConstraint_action_slot_handle_override_diff", nullptr, nullptr);
   RNA_def_property_update(prop, NC_ANIMATION | ND_NLA_ACTCHANGE, "rna_Constraint_update");
 
   prop = RNA_def_property(srna, "last_slot_identifier", PROP_STRING, PROP_NONE);

source/blender/makesrna/intern/rna_nla.cc

@@ -493,6 +493,20 @@ static void rna_NlaStrip_action_slot_handle_set(
                                      strip->last_slot_identifier);
 }
 
+/**
+ * Emit a 'diff' for the .action_slot_handle property whenever the .action property differs.
+ *
+ * \see rna_generic_action_slot_handle_override_diff()
+ */
+static void rna_NlaStrip_action_slot_handle_override_diff(
+    Main *bmain, RNAPropertyOverrideDiffContext &rnadiff_ctx)
+{
+  const NlaStrip *strip_a = static_cast<NlaStrip *>(rnadiff_ctx.prop_a->ptr->data);
+  const NlaStrip *strip_b = static_cast<NlaStrip *>(rnadiff_ctx.prop_b->ptr->data);
+
+  rna_generic_action_slot_handle_override_diff(bmain, rnadiff_ctx, strip_a->act, strip_b->act);
+}
+
 static PointerRNA rna_NlaStrip_action_slot_get(PointerRNA *ptr)
 {
   NlaStrip *strip = (NlaStrip *)ptr->data;
@@ -914,6 +928,8 @@ static void rna_def_nlastrip(BlenderRNA *brna)
                            "A number that identifies which sub-set of the Action is considered "
                            "to be for this NLA strip");
   RNA_def_property_override_flag(prop, PROPOVERRIDE_OVERRIDABLE_LIBRARY);
+  RNA_def_property_override_funcs(
+      prop, "rna_NlaStrip_action_slot_handle_override_diff", nullptr, nullptr);
   RNA_def_property_update(prop, NC_ANIMATION | ND_NLA_ACTCHANGE, "rna_NlaStrip_dependency_update");
 
   prop = RNA_def_property(srna, "last_slot_identifier", PROP_STRING, PROP_NONE);

tests/python/CMakeLists.txt

@@ -476,6 +476,7 @@ add_blender_test(
   --python ${CMAKE_CURRENT_LIST_DIR}/bl_animation_action.py
   --
   --testdir "${TEST_SRC_DIR}/animation"
+  --output-dir "${TEST_OUT_DIR}/bl_animation_action"
 )
 
 add_blender_test(

tests/python/bl_animation_action.py

@@ -613,6 +613,114 @@ class VersioningTest(unittest.TestCase):
             self.assertEqual(fcurve.group.name, "Bone.001")
 
 
+class SlotHandleLibraryOverridesTest(unittest.TestCase):
+    @classmethod
+    def setUpClass(cls):
+        args.output_dir.mkdir(parents=True, exist_ok=True)
+
+        cls.libfile = args.testdir.resolve() / "liboverride-action-slot-libfile.blend"
+        cls.workfile = args.output_dir.resolve() / "liboverride-action-slot.blend"
+
+    @classmethod
+    def tearDownClass(cls):
+        cls.workfile.unlink(missing_ok=True)
+
+    def test_liboverride_slot_handle(self):
+        # Whenever a liboverride changes the assigned Action, there should be a
+        # liboverride on the slot handle as well. Even when the assigned slot in
+        # the original data numerically has the same handle as the overridden
+        # slot.
+
+        self._create_test_file()
+        self._load_test_file()
+        self._check_assumptions()
+        self._perform_test()
+
+    def _create_test_file(self):
+        """Create the test file.
+
+        This has to happen every time the test runs, because it's about the
+        creation of library override operations. Creating the file once, storing
+        it with the rest of the test files, and opening it here to test it, will
+        just repeat the test on a once-written-correctly file, and not test the
+        currently-running Blender.
+        """
+
+        bpy.ops.wm.read_homefile(use_factory_startup=True, use_empty=True)
+
+        # Link Suzanne into the file and then into the scene.
+        with bpy.data.libraries.load(str(self.libfile), link=True, relative=False) as (data_from, data_to):
+            data_to.objects = ['Library Suzanne']
+        orig_lib_suzanne = data_to.objects[0]
+        bpy.context.scene.collection.objects.link(orig_lib_suzanne)
+
+        # Create a library override on Suzanne.
+        with bpy.context.temp_override(active_object=orig_lib_suzanne):
+            bpy.ops.object.make_override_library()
+
+        # Create a local Action to assign.
+        local_action = bpy.data.actions.new("Local Action")
+        local_slot = local_action.slots.new('OBJECT', "Local Slot")
+        layer = local_action.layers.new("Layer")
+        strip = layer.strips.new(type='KEYFRAME')
+        cbag = strip.channelbags.new(local_slot)
+        fcurve = cbag.fcurves.new('location', index=2)
+        fcurve.keyframe_points.insert(1, -5)
+        fcurve.keyframe_points.insert(20, 5)
+
+        # Grab the overridden Suzanne, and assign the local Action + a slot from that Action.
+        override_suzanne = bpy.data.objects['Library Suzanne', None]
+        override_suzanne.animation_data.action = local_action
+        override_suzanne.animation_data.action_slot = local_slot
+
+        # Save the file to disk.
+        bpy.ops.wm.save_as_mainfile(filepath=str(self.workfile), check_existing=False)
+
+    def _load_test_file(self):
+        bpy.ops.wm.read_homefile(use_factory_startup=True)  # Just to be sure.
+        bpy.ops.wm.open_mainfile(filepath=str(self.workfile), load_ui=False)
+
+    def _check_assumptions(self):
+        """Check that the test data is indeed as expected."""
+
+        # The library Action and the local Action should have the same handle on
+        # the first slot. If the slot handles are different, Blender's default
+        # library override diffing code would create an override operation, and
+        # this test will produce a false positive.
+        self.assertEqual(
+            bpy.data.actions['Library Action'].slots[0].handle,
+            bpy.data.actions['Local Action'].slots[0].handle,
+        )
+
+        # The library & local Action slots should have different identifiers.
+        # Otherwise the slot assignment will be correct regardless of library
+        # overrides, and this test will produce a false positive.
+        self.assertNotEqual(
+            bpy.data.actions['Library Action'].slots[0].identifier,
+            bpy.data.actions['Local Action'].slots[0].identifier,
+        )
+
+        # Check the Action assignments before we trust a check for the action slot.
+        libpath = bpy.data.libraries['liboverride-action-slot-libfile.blend'].filepath
+        orig_lib_suzanne = bpy.data.objects['Library Suzanne', libpath]
+        override_suzanne = bpy.data.objects['Library Suzanne', None]
+
+        self.assertEqual(bpy.data.actions['Library Action'], orig_lib_suzanne.animation_data.action)
+        self.assertEqual(bpy.data.actions['Local Action'], override_suzanne.animation_data.action)
+
+    def _perform_test(self):
+        override_suzanne = bpy.data.objects['Library Suzanne', None]
+
+        # === The actual test ===
+        self.assertEqual(bpy.data.actions['Local Action'].slots[0], override_suzanne.animation_data.action_slot)
+
+        # Set Suzanne's Z position to something large, and go the first frame to
+        # let the animation system evaluation overwrite it.
+        bpy.context.scene.frame_set(1)
+        self.assertLess(override_suzanne.location.z,
+                        -1, "Suzanne should be significantly below Z=0 when animated by the library Action")
+
+
 def main():
     global args
     import argparse
@@ -623,6 +731,15 @@ def main():
 
     parser = argparse.ArgumentParser()
     parser.add_argument('--testdir', required=True, type=pathlib.Path)
+    parser.add_argument(
+        "--output-dir",
+        dest="output_dir",
+        type=pathlib.Path,
+        default=pathlib.Path("."),
+        help="Where to output temp saved blendfiles",
+        required=False,
+    )
+
     args, remaining = parser.parse_known_args(argv)
 
     unittest.main(argv=remaining)