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

@@ -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'}
 
 

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.
  */

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<animrig::Channelbag>(__func__, *source_cbag);
+  target_cbag.slot_handle = target_slot_handle;
+
+  grow_array_and_append<ActionChannelbag *>(
+      &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<ActionSlot *>(&to) = *reinterpret_cast<ActionSlot *>(&from);
+  *reinterpret_cast<ActionSlot *>(&to) = *reinterpret_cast<const ActionSlot *>(&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 *>(&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

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<StripKeyframeData>(*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<StripKeyframeData>(*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. */

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;
 

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<bAction *>(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<std::string> 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)