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)