From f97c54ff76f2d7ad5f380e6769cfa94c32f73f31 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sybren=20A=2E=20St=C3=BCvel?= Date: Tue, 28 Jan 2025 17:20:59 +0100 Subject: [PATCH] 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 --- .../makesrna/intern/rna_action_tools.hh | 20 +++ .../blender/makesrna/intern/rna_animation.cc | 60 +++++++++ .../blender/makesrna/intern/rna_constraint.cc | 19 +++ source/blender/makesrna/intern/rna_nla.cc | 16 +++ tests/data | 2 +- tests/python/CMakeLists.txt | 1 + tests/python/bl_animation_action.py | 117 ++++++++++++++++++ 7 files changed, 234 insertions(+), 1 deletion(-) diff --git a/source/blender/makesrna/intern/rna_action_tools.hh b/source/blender/makesrna/intern/rna_action_tools.hh index d5444d4b92e..f036abaae7d 100644 --- a/source/blender/makesrna/intern/rna_action_tools.hh +++ b/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 */ diff --git a/source/blender/makesrna/intern/rna_animation.cc b/source/blender/makesrna/intern/rna_animation.cc index 5616aea2304..b5906338bf3 100644 --- a/source/blender/makesrna/intern/rna_animation.cc +++ b/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(rnadiff_ctx.prop_a->ptr->data); + const AnimData *adt_b = static_cast(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); diff --git a/source/blender/makesrna/intern/rna_constraint.cc b/source/blender/makesrna/intern/rna_constraint.cc index c00f77769c8..722d402c465 100644 --- a/source/blender/makesrna/intern/rna_constraint.cc +++ b/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(rnadiff_ctx.prop_a->ptr->data); + const bConstraint *con_b = static_cast(rnadiff_ctx.prop_b->ptr->data); + + const bActionConstraint *act_con_a = static_cast(con_a->data); + const bActionConstraint *act_con_b = static_cast(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); diff --git a/source/blender/makesrna/intern/rna_nla.cc b/source/blender/makesrna/intern/rna_nla.cc index 71a4a85e391..14772347303 100644 --- a/source/blender/makesrna/intern/rna_nla.cc +++ b/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(rnadiff_ctx.prop_a->ptr->data); + const NlaStrip *strip_b = static_cast(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); diff --git a/tests/data b/tests/data index f2345afbf08..c9a50d5336e 160000 --- a/tests/data +++ b/tests/data @@ -1 +1 @@ -Subproject commit f2345afbf08a3239cf56f8d7a996a66f9042ade3 +Subproject commit c9a50d5336e81c240e93d1a8371af789122d97e3 diff --git a/tests/python/CMakeLists.txt b/tests/python/CMakeLists.txt index 4be8d0a3964..45615f239ad 100644 --- a/tests/python/CMakeLists.txt +++ b/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( diff --git a/tests/python/bl_animation_action.py b/tests/python/bl_animation_action.py index cf2d698bf91..fefa9fc1b8d 100644 --- a/tests/python/bl_animation_action.py +++ b/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)