From 9b1a34e83ebbc0ebb847503ebebd54181c63acfa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sybren=20A=2E=20St=C3=BCvel?= Date: Mon, 7 Apr 2025 16:14:49 +0200 Subject: [PATCH] Fix #136947: Duplicate Action Slot doesn't duplicate animation data The "new/duplicate" button in the Action Slot selector did not actually duplicate, and always acted as a "new" button. This introduces the RNA function `ActionSlot.duplicate()`, which takes care of duplicating all the animation data associated with the slot as well. The semantics of this function should remain valid in the future, when Actions support multiple layers & strips. Note that this new function does not assign the slot, it just duplicates it and its data. The assignment of this duplicated slot is done in Python, through the already-existing API for this. Pull Request: https://projects.blender.org/blender/blender/pulls/137087 --- scripts/startup/bl_operators/anim.py | 14 +++- source/blender/animrig/ANIM_action.hh | 16 ++++ source/blender/animrig/intern/action.cc | 43 ++++++++++- source/blender/animrig/intern/action_test.cc | 77 ++++++++++++++++++++ source/blender/makesdna/DNA_action_types.h | 4 +- source/blender/makesrna/intern/rna_action.cc | 19 +++++ 6 files changed, 167 insertions(+), 6 deletions(-) diff --git a/scripts/startup/bl_operators/anim.py b/scripts/startup/bl_operators/anim.py index 5027a8142ca..ee56554d8fd 100644 --- a/scripts/startup/bl_operators/anim.py +++ b/scripts/startup/bl_operators/anim.py @@ -679,6 +679,10 @@ class ANIM_OT_slot_new_for_id(Operator): Note that _which_ ID should get this slot must be set in the 'animated_id' context pointer, using: >>> layout.context_pointer_set("animated_id", animated_id) + + When the ID already has a slot assigned, the newly-created slot will be + named after it (ensuring uniqueness with a numerical suffix) and any + animation data of the assigned slot will be duplicated for the new slot. """ bl_idname = "anim.slot_new_for_id" bl_label = "New Slot" @@ -703,10 +707,14 @@ class ANIM_OT_slot_new_for_id(Operator): def execute(self, context): animated_id = context.animated_id + adt = animated_id.animation_data - action = animated_id.animation_data.action - slot = action.slots.new(animated_id.id_type, animated_id.name) - animated_id.animation_data.action_slot = slot + if adt.action_slot: + slot = adt.action_slot.duplicate() + else: + slot = adt.action.slots.new(animated_id.id_type, animated_id.name) + + adt.action_slot = slot return {'FINISHED'} diff --git a/source/blender/animrig/ANIM_action.hh b/source/blender/animrig/ANIM_action.hh index 9e1f3cab6cc..c5b02686a11 100644 --- a/source/blender/animrig/ANIM_action.hh +++ b/source/blender/animrig/ANIM_action.hh @@ -1044,6 +1044,13 @@ class StripKeyframeData : public ::ActionStripKeyframeData { */ void slot_data_remove(slot_handle_t slot_handle); + /** + * Clone the channelbag belonging to the source slot, and assign it to the target slot. + * + * This is typically only called from #duplicate_slot(). + */ + void slot_data_duplicate(slot_handle_t source_slot_handle, slot_handle_t target_slot_handle); + /** * Return the index of `channelbag` in this strip data's channelbag array, or * -1 if `channelbag` doesn't exist in this strip data. @@ -1962,6 +1969,15 @@ Action *convert_to_layered_action(Main &bmain, const Action &legacy_action); */ void move_slot(Main &bmain, Slot &slot, Action &from_action, Action &to_action); +/** + * Duplicate a slot, and all its animation data. + * + * Data-blocks using the slot are not updated, so the returned slot will be unused. + * + * The `action` MUST own `slot`. + */ +Slot &duplicate_slot(Action &action, const Slot &slot); + /** * Deselect the keys of all actions in the Span. Duplicate entries are only visited once. */ diff --git a/source/blender/animrig/intern/action.cc b/source/blender/animrig/intern/action.cc index be2013e3028..c13b8fab08f 100644 --- a/source/blender/animrig/intern/action.cc +++ b/source/blender/animrig/intern/action.cc @@ -1846,6 +1846,23 @@ void StripKeyframeData::slot_data_remove(const slot_handle_t slot_handle) this->channelbag_remove(*channelbag); } +void StripKeyframeData::slot_data_duplicate(const slot_handle_t source_slot_handle, + const slot_handle_t target_slot_handle) +{ + BLI_assert(!this->channelbag_for_slot(target_slot_handle)); + + const Channelbag *source_cbag = this->channelbag_for_slot(source_slot_handle); + if (!source_cbag) { + return; + } + + Channelbag &target_cbag = *MEM_new(__func__, *source_cbag); + target_cbag.slot_handle = target_slot_handle; + + grow_array_and_append( + &this->channelbag_array, &this->channelbag_array_num, &target_cbag); +} + const FCurve *Channelbag::fcurve_find(const FCurveDescriptor &fcurve_descriptor) const { return animrig::fcurve_find(this->fcurves(), fcurve_descriptor); @@ -3075,11 +3092,11 @@ Action *convert_to_layered_action(Main &bmain, const Action &legacy_action) * slot handle and runtime data. This copies the identifier which might clash with other * identifiers on the action. Call `slot_identifier_ensure_unique` after. */ -static void clone_slot(Slot &from, Slot &to) +static void clone_slot(const Slot &from, Slot &to) { ActionSlotRuntimeHandle *runtime = to.runtime; slot_handle_t handle = to.handle; - *reinterpret_cast(&to) = *reinterpret_cast(&from); + *reinterpret_cast(&to) = *reinterpret_cast(&from); to.runtime = runtime; to.handle = handle; } @@ -3158,4 +3175,26 @@ void move_slot(Main &bmain, Slot &source_slot, Action &from_action, Action &to_a from_action.slot_remove(source_slot); } +Slot &duplicate_slot(Action &action, const Slot &slot) +{ + BLI_assert(action.slots().contains(const_cast(&slot))); + + /* Duplicate the slot itself. */ + Slot &cloned_slot = action.slot_add(); + clone_slot(slot, cloned_slot); + slot_identifier_ensure_unique(action, cloned_slot); + + /* Duplicate each Channelbag for the source slot. */ + for (int i = 0; i < action.strip_keyframe_data_array_num; i++) { + StripKeyframeData &strip_data = action.strip_keyframe_data_array[i]->wrap(); + strip_data.slot_data_duplicate(slot.handle, cloned_slot.handle); + } + + /* The ID has changed, and so it needs to be re-evaluated. Animation does not + * have to be flushed since nothing is using this slot yet. */ + DEG_id_tag_update(&action.id, ID_RECALC_ANIMATION_NO_FLUSH); + + return cloned_slot; +} + } // namespace blender::animrig diff --git a/source/blender/animrig/intern/action_test.cc b/source/blender/animrig/intern/action_test.cc index 5ccb0dceed2..a9844a1a44d 100644 --- a/source/blender/animrig/intern/action_test.cc +++ b/source/blender/animrig/intern/action_test.cc @@ -1300,6 +1300,83 @@ TEST_F(ActionLayersTest, action_move_slot_without_channelbag) ASSERT_EQ(action, suzanne->adt->action); } +TEST_F(ActionLayersTest, action_duplicate_slot) +{ + ASSERT_TRUE(action->is_empty()); + + Slot &slot_cube = action->slot_add(); + ASSERT_EQ(assign_action_and_slot(action, &slot_cube, cube->id), ActionSlotAssignmentResult::OK); + + PointerRNA cube_rna_pointer = RNA_id_pointer_create(&cube->id); + + action_fcurve_ensure_ex(bmain, action, "Test", &cube_rna_pointer, {"location", 0}); + action_fcurve_ensure_ex(bmain, action, "Test", &cube_rna_pointer, {"rotation_euler", 1}); + + ASSERT_EQ(action->layer_array_num, 1); + Layer *layer = action->layer(0); + + ASSERT_EQ(layer->strip_array_num, 1); + StripKeyframeData &strip_data = layer->strip(0)->data(*action); + + ASSERT_EQ(strip_data.channelbag_array_num, 1); + Channelbag *bag = strip_data.channelbag(0); + ASSERT_EQ(bag->fcurve_array_num, 2); + + /* Duplicate the slot and check it for uniqueness within the Action. */ + Slot &dupli_slot = duplicate_slot(*action, slot_cube); + EXPECT_NE(dupli_slot.identifier, slot_cube.identifier); + EXPECT_NE(dupli_slot.handle, slot_cube.handle); + ASSERT_EQ(action->slot_array_num, 2); + EXPECT_EQ(&dupli_slot, action->slot(1)); + + /* Check the channelbag has been duplicated correctly. */ + ASSERT_EQ(strip_data.channelbag_array_num, 2); + Channelbag *dupli_bag = strip_data.channelbag(1); + EXPECT_EQ(dupli_bag->slot_handle, dupli_slot.handle); + EXPECT_EQ(dupli_bag->fcurve_array_num, 2); + + /* Check the original channelbag is untouched. */ + EXPECT_EQ(bag->slot_handle, slot_cube.handle); + EXPECT_EQ(bag->fcurve_array_num, 2); + + /* The slot should NOT have been reassigned. */ + EXPECT_EQ(action, cube->adt->action); + EXPECT_EQ(slot_cube.handle, cube->adt->slot_handle); +} + +TEST_F(ActionLayersTest, action_duplicate_slot_without_channelbag) +{ + ASSERT_TRUE(action->is_empty()); + + Slot &slot_cube = action->slot_add(); + ASSERT_EQ(assign_action_and_slot(action, &slot_cube, cube->id), ActionSlotAssignmentResult::OK); + + /* Create a keyframe strip, but without any channelbags. */ + action->layer_keystrip_ensure(); + + ASSERT_EQ(action->layer_array_num, 1); + Layer *layer = action->layer(0); + + ASSERT_EQ(layer->strip_array_num, 1); + StripKeyframeData &strip_data = layer->strip(0)->data(*action); + + ASSERT_EQ(strip_data.channelbag_array_num, 0); + + /* Duplicate the slot and check it for uniqueness within the Action. */ + Slot &dupli_slot = duplicate_slot(*action, slot_cube); + EXPECT_NE(dupli_slot.identifier, slot_cube.identifier); + EXPECT_NE(dupli_slot.handle, slot_cube.handle); + ASSERT_EQ(action->slot_array_num, 2); + EXPECT_EQ(&dupli_slot, action->slot(1)); + + /* Check there are still no channelbags. */ + EXPECT_EQ(strip_data.channelbag_array_num, 0); + + /* The slot should NOT have been reassigned. */ + EXPECT_EQ(action, cube->adt->action); + EXPECT_EQ(slot_cube.handle, cube->adt->slot_handle); +} + /*-----------------------------------------------------------*/ /* Allocate fcu->bezt, and also return a unique_ptr to it for easily freeing the memory. */ diff --git a/source/blender/makesdna/DNA_action_types.h b/source/blender/makesdna/DNA_action_types.h index fdde0625fa7..b2789d69a74 100644 --- a/source/blender/makesdna/DNA_action_types.h +++ b/source/blender/makesdna/DNA_action_types.h @@ -771,7 +771,9 @@ typedef struct bAction { /* Storage for the underlying data of strips. Each strip type has its own * array, and strips reference this data with an enum indicating the strip - * type and an int containing the index in the array to use. */ + * type and an int containing the index in the array to use. + * + * NOTE: when adding new strip data arrays, also update `duplicate_slot()`. */ struct ActionStripKeyframeData **strip_keyframe_data_array; int strip_keyframe_data_array_num; diff --git a/source/blender/makesrna/intern/rna_action.cc b/source/blender/makesrna/intern/rna_action.cc index 8b5990eb911..38611d00bba 100644 --- a/source/blender/makesrna/intern/rna_action.cc +++ b/source/blender/makesrna/intern/rna_action.cc @@ -391,6 +391,15 @@ static CollectionVector rna_ActionSlot_users(struct ActionSlot *self, Main *bmai return vector; } +static ActionSlot *rna_ActionSlot_duplicate(ID *action_id, const ActionSlot *self) +{ + animrig::Action &action = reinterpret_cast(action_id)->wrap(); + const animrig::Slot &source_slot = self->wrap(); + + animrig::Slot &dupli_slot = animrig::duplicate_slot(action, source_slot); + return &dupli_slot; +} + static std::optional rna_ActionLayer_path(const PointerRNA *ptr) { animrig::Layer &layer = rna_data_layer(ptr); @@ -2195,6 +2204,16 @@ static void rna_def_action_slot(BlenderRNA *brna) parm = RNA_def_property(func, "users", PROP_COLLECTION, PROP_NONE); RNA_def_property_struct_type(parm, "ID"); RNA_def_function_return(func, parm); + + func = RNA_def_function(srna, "duplicate", "rna_ActionSlot_duplicate"); + RNA_def_function_ui_description( + func, "Duplicate this slot, including all the animation data associated with it"); + /* Return value. */ + parm = RNA_def_property(func, "slot", PROP_POINTER, PROP_NONE); + RNA_def_function_flag(func, FUNC_USE_SELF_ID); + RNA_def_property_struct_type(parm, "ActionSlot"); + RNA_def_property_ui_text(parm, "Duplicated Slot", "The slot created by duplicating this one"); + RNA_def_function_return(func, parm); } static void rna_def_ActionLayer_strips(BlenderRNA *brna, PropertyRNA *cprop)