Anim: add Action::slot_remove() method to remove an action slot

Add a method to remove a slot from an Action. This is also exposed as
`Action.slots.remove(slot)` to RNA/Python.

Removing a slot deletes all its associated animation data in the Action.
It also un-assigns the slot from any ID that was animated by it. The
Action reference is left untouched.

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

source/blender/animrig/ANIM_action.hh

@@ -199,6 +199,15 @@ class Action : public ::bAction {
    */
   Slot &slot_add_for_id(const ID &animated_id);
 
+  /**
+   * Remove a slot, and ALL animation data that belongs to it.
+   *
+   * After this call, the reference is no longer valid as the slot will have been freed.
+   *
+   * \return true when the layer was found & removed, false if it wasn't found.
+   */
+  bool slot_remove(Slot &slot_to_remove);
+
   /**
    * Set the active Slot, ensuring only one Slot is flagged as the Active one.
    *
@@ -276,6 +285,9 @@ class Action : public ::bAction {
   /** Return the layer's index, or -1 if not found in this Action. */
   int64_t find_layer_index(const Layer &layer) const;
 
+  /** Return the slot's index, or -1 if not found in this Action. */
+  int64_t find_slot_index(const Slot &slot) const;
+
  private:
   Slot &slot_allocate();
 
@@ -368,6 +380,13 @@ class Strip : public ::ActionStrip {
    * (negative for frame_start, positive for frame_end) are supported.
    */
   void resize(float frame_start, float frame_end);
+
+  /**
+   * Remove all data belonging to the given slot.
+   *
+   * This is typically only called from Layer::slot_data_remove().
+   */
+  void slot_data_remove(slot_handle_t slot_handle);
 };
 static_assert(sizeof(Strip) == sizeof(::ActionStrip),
               "DNA struct and its C++ wrapper must have the same size");
@@ -455,6 +474,13 @@ class Layer : public ::ActionLayer {
    */
   bool strip_remove(Strip &strip);
 
+  /**
+   * Remove all data belonging to the given slot.
+   *
+   * This is typically only called from Action::slot_remove().
+   */
+  void slot_data_remove(slot_handle_t slot_handle);
+
  protected:
   /** Return the strip's index, or -1 if not found in this layer. */
   int64_t find_strip_index(const Strip &strip) const;
@@ -659,6 +685,13 @@ class KeyframeStrip : public ::KeyframeActionStrip {
    */
   bool channelbag_remove(ChannelBag &channelbag_to_remove);
 
+  /**
+   * Remove all strip data for the given slot.
+   *
+   * Typically only called from Strip::slot_data_remove().
+   */
+  void slot_data_remove(slot_handle_t slot_handle);
+
   /** Return the channelbag's index, or -1 if there is none for this slot handle. */
   int64_t find_channelbag_index(const ChannelBag &channelbag) const;
 

source/blender/animrig/intern/action.cc

@@ -306,6 +306,17 @@ int64_t Action::find_layer_index(const Layer &layer) const
   return -1;
 }
 
+int64_t Action::find_slot_index(const Slot &slot) const
+{
+  for (const int64_t slot_index : this->slots().index_range()) {
+    const Slot *visit_slot = this->slot(slot_index);
+    if (visit_slot == &slot) {
+      return slot_index;
+    }
+  }
+  return -1;
+}
+
 blender::Span<const Slot *> Action::slots() const
 {
   return blender::Span<Slot *>{reinterpret_cast<Slot **>(this->slot_array), this->slot_array_num};
@@ -482,6 +493,48 @@ Slot &Action::slot_add_for_id(const ID &animated_id)
   return slot;
 }
 
+static void slot_ptr_destructor(ActionSlot **dna_slot_ptr)
+{
+  Slot &slot = (*dna_slot_ptr)->wrap();
+  MEM_delete(&slot);
+};
+
+bool Action::slot_remove(Slot &slot_to_remove)
+{
+  /* Check that this slot belongs to this Action. */
+  const int64_t slot_index = this->find_slot_index(slot_to_remove);
+  if (slot_index < 0) {
+    return false;
+  }
+
+  /* Remove the slot's data from each layer. */
+  for (Layer *layer : this->layers()) {
+    layer->slot_data_remove(slot_to_remove.handle);
+  }
+
+  /* Un-assign this slot from its users. Only do this if the list of users is valid, */
+  for (ID *user : slot_to_remove.runtime_users()) {
+    /* Sanity check: make sure the slot is still assigned, before un-assigning anything. */
+    std::optional<std::pair<Action *, Slot *>> action_and_slot = get_action_slot_pair(*user);
+    BLI_assert_msg(action_and_slot, "Slot user has no Action assigned");
+    BLI_assert_msg(action_and_slot->first == this, "Slot user has other Action assigned");
+    BLI_assert_msg(action_and_slot->second == &slot_to_remove,
+                   "Slot user has other Slot assigned");
+    if (!action_and_slot || action_and_slot->first != this ||
+        action_and_slot->second != &slot_to_remove)
+    {
+      continue;
+    }
+
+    this->assign_id(nullptr, *user);
+  }
+
+  /* Remove the actual slot. */
+  dna::array::remove_index(
+      &this->slot_array, &this->slot_array_num, nullptr, slot_index, slot_ptr_destructor);
+  return true;
+}
+
 void Action::slot_active_set(const slot_handle_t slot_handle)
 {
   for (Slot *slot : slots()) {
@@ -723,6 +776,13 @@ int64_t Layer::find_strip_index(const Strip &strip) const
   return -1;
 }
 
+void Layer::slot_data_remove(const slot_handle_t slot_handle)
+{
+  for (Strip *strip : this->strips()) {
+    strip->slot_data_remove(slot_handle);
+  }
+}
+
 /* ----- ActionSlot implementation ----------- */
 
 Slot::Slot()
@@ -895,6 +955,14 @@ void Slot::name_ensure_prefix()
   *reinterpret_cast<short *>(this->name) = this->idtype;
 }
 
+void Strip::slot_data_remove(const slot_handle_t slot_handle)
+{
+  switch (this->type()) {
+    case Type::Keyframe:
+      this->as<KeyframeStrip>().slot_data_remove(slot_handle);
+  }
+}
+
 /* ----- Functions  ----------- */
 
 bool assign_action(Action &action, ID &animated_id)
@@ -1224,6 +1292,15 @@ bool KeyframeStrip::channelbag_remove(ChannelBag &channelbag_to_remove)
   return true;
 }
 
+void KeyframeStrip::slot_data_remove(const slot_handle_t slot_handle)
+{
+  ChannelBag *channelbag = this->channelbag_for_slot(slot_handle);
+  if (!channelbag) {
+    return;
+  }
+  this->channelbag_remove(*channelbag);
+}
+
 const FCurve *ChannelBag::fcurve_find(const FCurveDescriptor fcurve_descriptor) const
 {
   return animrig::fcurve_find(this->fcurves(), fcurve_descriptor);

source/blender/animrig/intern/action_test.cc

@@ -244,6 +244,103 @@ TEST_F(ActionLayersTest, add_slot_multiple)
   EXPECT_EQ(2, slot_suzanne.handle);
 }
 
+TEST_F(ActionLayersTest, slot_remove)
+{
+  { /* Canary test: removing a just-created slot on an otherwise empty Action should work. */
+    Slot &slot = action->slot_add();
+    EXPECT_EQ(1, slot.handle);
+    EXPECT_EQ(1, action->last_slot_handle);
+
+    EXPECT_TRUE(action->slot_remove(slot));
+    EXPECT_EQ(1, action->last_slot_handle)
+        << "Removing a slot should not change the last-used slot handle.";
+    EXPECT_EQ(0, action->slot_array_num);
+  }
+
+  { /* Removing a non-existing slot should return false. */
+    Slot slot;
+    EXPECT_FALSE(action->slot_remove(slot));
+  }
+
+  { /* Removing a slot should remove its ChannelBag. */
+    Slot &slot = action->slot_add();
+    const slot_handle_t slot_handle = slot.handle;
+    EXPECT_EQ(2, slot.handle);
+    EXPECT_EQ(2, action->last_slot_handle);
+
+    /* Create an F-Curve in a ChannelBag for the slot. */
+    action->layer_keystrip_ensure();
+    KeyframeStrip &strip = action->layer(0)->strip(0)->as<KeyframeStrip>();
+    ChannelBag &channelbag = strip.channelbag_for_slot_ensure(slot);
+    channelbag.fcurve_create_unique(bmain, {"location", 1});
+
+    /* Remove the slot. */
+    EXPECT_TRUE(action->slot_remove(slot));
+    EXPECT_EQ(2, action->last_slot_handle)
+        << "Removing a slot should not change the last-used slot handle.";
+    EXPECT_EQ(0, action->slot_array_num);
+
+    /* Check that its channelbag is gone. */
+    ChannelBag *found_cbag = strip.channelbag_for_slot(slot_handle);
+    EXPECT_EQ(found_cbag, nullptr);
+  }
+
+  { /* Removing one slot should leave the other two in place. */
+    Slot &slot1 = action->slot_add();
+    Slot &slot2 = action->slot_add();
+    Slot &slot3 = action->slot_add();
+    EXPECT_EQ(3, slot1.handle);
+    EXPECT_EQ(4, slot2.handle);
+    EXPECT_EQ(5, slot3.handle);
+    EXPECT_EQ(5, action->last_slot_handle);
+
+    /* For referencing the slot handle after the slot is removed. */
+    const slot_handle_t slot2_handle = slot2.handle;
+
+    /* Create a Channelbag for each slot. */
+    action->layer_keystrip_ensure();
+    KeyframeStrip &strip = action->layer(0)->strip(0)->as<KeyframeStrip>();
+    strip.channelbag_for_slot_ensure(slot1);
+    strip.channelbag_for_slot_ensure(slot2);
+    strip.channelbag_for_slot_ensure(slot3);
+
+    /* Remove the slot. */
+    EXPECT_TRUE(action->slot_remove(slot2));
+    EXPECT_EQ(5, action->last_slot_handle);
+
+    /* Check the correct slot + channelbag are removed. */
+    EXPECT_EQ(action->slot_for_handle(slot1.handle), &slot1);
+    EXPECT_EQ(action->slot_for_handle(slot2_handle), nullptr);
+    EXPECT_EQ(action->slot_for_handle(slot3.handle), &slot3);
+
+    EXPECT_NE(strip.channelbag_for_slot(slot1.handle), nullptr);
+    EXPECT_EQ(strip.channelbag_for_slot(slot2_handle), nullptr);
+    EXPECT_NE(strip.channelbag_for_slot(slot3.handle), nullptr);
+  }
+
+  { /* Removing an in-use slot should un-assign it from its users. */
+    Slot &slot = action->slot_add_for_id(cube->id);
+    action->assign_id(&slot, cube->id);
+    ASSERT_TRUE(slot.runtime_users().contains(&cube->id));
+    ASSERT_EQ(cube->adt->slot_handle, slot.handle);
+
+    ASSERT_TRUE(action->slot_remove(slot));
+    EXPECT_EQ(cube->adt->slot_handle, Slot::unassigned);
+  }
+
+  { /* Creating a slot after removing one should not reuse its handle. */
+    action->last_slot_handle = 3; /* To create independence between sub-tests. */
+    Slot &slot1 = action->slot_add();
+    ASSERT_EQ(4, slot1.handle);
+    ASSERT_EQ(4, action->last_slot_handle);
+    ASSERT_TRUE(action->slot_remove(slot1));
+
+    Slot &slot2 = action->slot_add();
+    EXPECT_EQ(5, slot2.handle);
+    EXPECT_EQ(5, action->last_slot_handle);
+  }
+}
+
 TEST_F(ActionLayersTest, action_assign_id)
 {
   /* Assign to the only, 'virgin' Slot, should always work. */

source/blender/makesrna/intern/rna_action.cc

@@ -202,6 +202,23 @@ static ActionSlot *rna_Action_slots_new(bAction *dna_action,
   return slot;
 }
 
+void rna_Action_slots_remove(bAction *dna_action,
+                             bContext *C,
+                             ReportList *reports,
+                             PointerRNA *slot_ptr)
+{
+  animrig::Action &action = dna_action->wrap();
+  animrig::Slot &slot = rna_data_slot(slot_ptr);
+  if (!action.slot_remove(slot)) {
+    BKE_report(reports, RPT_ERROR, "This slot does not belong to this Action");
+    return;
+  }
+
+  RNA_POINTER_INVALIDATE(slot_ptr);
+  WM_event_add_notifier(C, NC_ANIMATION | ND_ANIMCHAN, nullptr);
+  DEG_id_tag_update(&action.id, ID_RECALC_ANIMATION);
+}
+
 static void rna_iterator_action_layers_begin(CollectionPropertyIterator *iter, PointerRNA *ptr)
 {
   animrig::Action &action = rna_action(ptr);
@@ -1689,7 +1706,7 @@ static void rna_def_action_slots(BlenderRNA *brna, PropertyRNA *cprop)
 
   /* Animation.slots.new(...) */
   func = RNA_def_function(srna, "new", "rna_Action_slots_new");
-  RNA_def_function_ui_description(func, "Add a slot to the animation");
+  RNA_def_function_ui_description(func, "Add a slot to the Action");
   RNA_def_function_flag(func, FUNC_USE_CONTEXT | FUNC_USE_REPORTS);
   parm = RNA_def_pointer(
       func,
@@ -1704,6 +1721,15 @@ static void rna_def_action_slots(BlenderRNA *brna, PropertyRNA *cprop)
 
   parm = RNA_def_pointer(func, "slot", "ActionSlot", "", "Newly created action slot");
   RNA_def_function_return(func, parm);
+
+  /* Animation.slots.remove(layer) */
+  func = RNA_def_function(srna, "remove", "rna_Action_slots_remove");
+  RNA_def_function_flag(func, FUNC_USE_CONTEXT | FUNC_USE_REPORTS);
+  RNA_def_function_ui_description(func,
+                                  "Remove the slot from the Action, including all animation that "
+                                  "is associated with that slot");
+  parm = RNA_def_pointer(func, "action_slot", "ActionSlot", "Action Slot", "The slot to remove");
+  RNA_def_parameter_flags(parm, PropertyFlag(0), PARM_REQUIRED | PARM_RNAPTR);
 }
 
 static void rna_def_action_layers(BlenderRNA *brna, PropertyRNA *cprop)