Anim: change parameters of slots.new() RNA function

`Action.slots.new()` in the Python API previously took either an ID or nothing
as a parameter. In the former case it would create a slot with the appropriate
`id_root` and name for that ID. In the latter case it would create a default
slot with an unspecified `id_root` and default name.

This had several issues:

1. You couldn't create a slot with a specific `id_root` without already having
   an ID of that type. In theory this isn't a problem, but in practice in larger
   scripts/addons you don't necessarily have such an ID on hand at the call
   site.
2. You couldn't directly create a slot with a desired name without an existing
   ID with that name. This isn't so important, since you can always just set the
   name afterwards. But it's a bit annoying.
3. Most other `new()` APIs in Blender *require* you to specify the name of the
   item being created. So calling this with no parameters was violating that
   norm.
4. Ideally, we want to eliminate unspecified `id_root`s, since they cause other
   weirdness in the API such as slot identifiers changing upon slot assignment.

To resolve these issues, and just generally to make the API more
straightforward, this PR changes `slots.new()` to take two required parameters:
an ID type and a name. For example:

`slots.new(id_type='CAMERA', name="My Camera Data Slot")`.

This fully specifies everything needed for the slot identifier upon creation,
and doesn't require any outside data items to create a slot with the desired
type and name.

In the future if we decide we still want a `for_id`-style slot creation API, we
can reintroduce it as a separate function.

Ref: #130892
Pull Request: https://projects.blender.org/blender/blender/pulls/130970

scripts/modules/bpy_extras/anim_utils.py

@@ -386,7 +386,7 @@ def bake_action_iter(
     else:
         # When baking into the current action, a slot needs to be assigned.
         if not atd.action_slot:
-            slot = action.slots.new(for_id=obj)
+            slot = action.slots.new(obj.id_type, obj.name)
             atd.action_slot = slot
 
     # Only leave tweak mode if we actually need to modify the action (#57159)
@@ -397,7 +397,7 @@ def bake_action_iter(
 
         atd.action = action
         if action.is_action_layered:
-            slot = action.slots.new(for_id=obj)
+            slot = action.slots.new(obj.id_type, obj.name)
             atd.action_slot = slot
 
     # Baking the action only makes sense in Replace mode, so force it (#69105)

scripts/startup/bl_operators/anim.py

@@ -700,7 +700,7 @@ class ANIM_OT_slot_new_for_id(Operator):
         animated_id = context.animated_id
 
         action = animated_id.animation_data.action
-        slot = action.slots.new(for_id=animated_id)
+        slot = action.slots.new(animated_id.id_type, animated_id.name)
         animated_id.animation_data.action_slot = slot
         return {'FINISHED'}
 

source/blender/animrig/ANIM_action.hh

@@ -144,6 +144,18 @@ class Action : public ::bAction {
   Slot *slot_for_handle(slot_handle_t handle);
   const Slot *slot_for_handle(slot_handle_t handle) const;
 
+  /**
+   * Set the slot display name (the part of the identifier after the two-letter
+   * ID prefix), ensure the resulting identifier is unique, and propagate the
+   * new identifier to all data-blocks that use it
+   *
+   * This has to be done on the Action level to ensure each slot has a unique
+   * identifier within the Action.
+   *
+   * \see #Action::slot_identifier_set
+   */
+  void slot_display_name_set(Main &bmain, Slot &slot, StringRefNull new_display_name);
+
   /**
    * Set the slot identifier, ensure it is unique, and propagate the new identifier to
    * all data-blocks that use it.
@@ -190,6 +202,13 @@ class Action : public ::bAction {
    */
   Slot &slot_add();
 
+  /**
+   * Create a new, unused Slot for the given ID type.
+   *
+   * The returned slot will only be suitable for the specified ID type.
+   */
+  Slot &slot_add_for_id_type(ID_Type idtype);
+
   /**
    * Create a new slot, named after the given ID, and limited to the ID's type.
    *

source/blender/animrig/intern/action.cc

@@ -399,6 +399,19 @@ static void slot_identifier_ensure_unique(Action &action, Slot &slot)
       check_name_is_used, &check_data, "", '.', slot.identifier, sizeof(slot.identifier));
 }
 
+void Action::slot_display_name_set(Main &bmain, Slot &slot, StringRefNull new_display_name)
+{
+  BLI_assert_msg(StringRef(new_display_name).size() >= 1,
+                 "Action Slot display names must not be empty");
+  BLI_assert_msg(StringRef(slot.identifier).size() >= 2,
+                 "Action Slot's existing identifier lacks the two-character type prefix, which "
+                 "would make the display name copy meaningless due to early null termination.");
+
+  BLI_strncpy_utf8(slot.identifier + 2, new_display_name.c_str(), ARRAY_SIZE(slot.identifier) - 2);
+  slot_identifier_ensure_unique(*this, slot);
+  this->slot_identifier_propagate(bmain, slot);
+}
+
 void Action::slot_identifier_set(Main &bmain, Slot &slot, const StringRefNull new_identifier)
 {
   /* TODO: maybe this function should only set the 'identifier without prefix' aka the 'display
@@ -498,6 +511,21 @@ Slot &Action::slot_add()
   return slot;
 }
 
+Slot &Action::slot_add_for_id_type(const ID_Type idtype)
+{
+  Slot &slot = this->slot_add();
+
+  slot.idtype = idtype;
+  slot.identifier_ensure_prefix();
+  BLI_strncpy_utf8(slot.identifier + 2, DATA_(slot_default_name), ARRAY_SIZE(slot.identifier) - 2);
+  slot_identifier_ensure_unique(*this, slot);
+
+  /* No need to call anim.slot_identifier_propagate() as nothing will be using
+   * this brand new Slot yet. */
+
+  return slot;
+}
+
 Slot &Action::slot_add_for_id(const ID &animated_id)
 {
   Slot &slot = this->slot_add();

source/blender/animrig/intern/action_test.cc

@@ -517,22 +517,27 @@ TEST_F(ActionLayersTest, rename_slot)
   EXPECT_STREQ(slot_cube.identifier, cube->adt->last_slot_identifier)
       << "The slot identifier should be copied to the adt";
 
-  action->slot_identifier_define(slot_cube, "New Slot Name");
-  EXPECT_STREQ("New Slot Name", slot_cube.identifier);
+  action->slot_identifier_define(slot_cube, "OBNew Slot Name");
+  EXPECT_STREQ("OBNew Slot Name", slot_cube.identifier);
   /* At this point the slot identifier will not have been copied to the cube
    * AnimData. However, I don't want to test for that here, as it's not exactly
    * desirable behavior, but more of a side-effect of the current
    * implementation. */
 
   action->slot_identifier_propagate(*bmain, slot_cube);
-  EXPECT_STREQ("New Slot Name", cube->adt->last_slot_identifier);
+  EXPECT_STREQ("OBNew Slot Name", cube->adt->last_slot_identifier);
+
+  /* Rename via the display name, which should propagate to the ADT. */
+  action->slot_display_name_set(*bmain, slot_cube, "Slot's New Display Name");
+  EXPECT_STREQ("OBSlot's New Display Name", slot_cube.identifier);
+  EXPECT_STREQ("OBSlot's New Display Name", cube->adt->last_slot_identifier);
 
   /* Finally, do another rename, do NOT call the propagate function, then
    * unassign. This should still result in the correct slot name being stored
    * on the ADT. */
-  action->slot_identifier_define(slot_cube, "Even Newer Name");
+  action->slot_identifier_define(slot_cube, "OBEven Newer Name");
   EXPECT_TRUE(unassign_action(cube->id));
-  EXPECT_STREQ("Even Newer Name", cube->adt->last_slot_identifier);
+  EXPECT_STREQ("OBEven Newer Name", cube->adt->last_slot_identifier);
 }
 
 TEST_F(ActionLayersTest, slot_identifier_ensure_prefix)

source/blender/makesrna/intern/rna_action.cc

@@ -179,13 +179,10 @@ static void rna_ActionSlots_active_set(PointerRNA *ptr,
   }
 }
 
-static ActionSlot *rna_Action_slots_new(bAction *dna_action,
-                                        bContext *C,
-                                        ReportList *reports,
-                                        ID *id_for_slot)
+static ActionSlot *rna_Action_slots_new(
+    bAction *dna_action, Main *bmain, bContext *C, ReportList *reports, int type, const char *name)
 {
   animrig::Action &action = dna_action->wrap();
-  animrig::Slot *slot;
 
   if (!action.is_action_layered()) {
     BKE_reportf(reports,
@@ -195,13 +192,14 @@ static ActionSlot *rna_Action_slots_new(bAction *dna_action,
     return nullptr;
   }
 
-  if (id_for_slot) {
-    slot = &action.slot_add_for_id(*id_for_slot);
-  }
-  else {
-    slot = &action.slot_add();
+  if (name[0] == 0) {
+    BKE_reportf(reports, RPT_ERROR, "Invalid slot name '%s': name must not be empty.", name);
+    return nullptr;
   }
 
+  animrig::Slot *slot = &action.slot_add_for_id_type(ID_Type(type));
+  action.slot_display_name_set(*bmain, *slot, name);
+
   WM_event_add_notifier(C, NC_ANIMATION | ND_ANIMCHAN, nullptr);
   return slot;
 }
@@ -1899,17 +1897,26 @@ 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 Action");
-  RNA_def_function_flag(func, FUNC_USE_CONTEXT | FUNC_USE_REPORTS);
-  parm = RNA_def_pointer(
+  RNA_def_function_flag(func, FUNC_USE_MAIN | FUNC_USE_CONTEXT | FUNC_USE_REPORTS);
+  parm = RNA_def_enum(
       func,
-      "for_id",
-      "ID",
-      "Data-Block",
-      "If given, the new slot will be named after this data-block, and limited to animating "
-      "data-blocks of its type. If ommitted, limiting the ID type will happen as soon as the "
-      "slot is assigned.");
-  /* Clear out the PARM_REQUIRED flag, which is set by default for pointer parameters. */
-  RNA_def_parameter_flags(parm, PropertyFlag(0), ParameterFlag(0));
+      "id_type",
+      rna_enum_id_type_items,
+      ID_OB,
+      "Data-block Type",
+      "The data-block type that the slot is intended for. This is combined with the slot name to "
+      "create the slot's unique identifier, and is also used to limit (on a best-effort basis) "
+      "which data-blocks the slot can be assigned to.");
+  RNA_def_parameter_flags(parm, PropertyFlag(0), PARM_REQUIRED);
+  parm = RNA_def_string(
+      func,
+      "name",
+      nullptr,
+      /* Minus 2 for the ID-type prefix. */
+      sizeof(ActionSlot::identifier) - 2,
+      "Name",
+      "Name of the slot. This will be made unique within the Action among slots of the same type");
+  RNA_def_parameter_flags(parm, PropertyFlag(0), PARM_REQUIRED);
 
   parm = RNA_def_pointer(func, "slot", "ActionSlot", "", "Newly created action slot");
   RNA_def_function_return(func, parm);

tests/python/bl_animation_action.py

@@ -13,6 +13,61 @@ blender -b --factory-startup --python tests/python/bl_animation_action.py
 """
 
 
+class ActionSlotCreationTest(unittest.TestCase):
+    """Test creating action slots & their resulting identifiers and id roots."""
+
+    def setUp(self) -> None:
+        bpy.ops.wm.read_homefile(use_factory_startup=True)
+
+        self.action = bpy.data.actions.new('Action')
+
+    def test_same_name_different_type(self):
+        slot1 = self.action.slots.new('OBJECT', "Bob")
+        slot2 = self.action.slots.new('CAMERA', "Bob")
+        slot3 = self.action.slots.new('LIGHT', "Bob")
+
+        self.assertEqual("OBBob", slot1.identifier)
+        self.assertEqual('OBJECT', slot1.id_root)
+
+        self.assertEqual("CABob", slot2.identifier)
+        self.assertEqual('CAMERA', slot2.id_root)
+
+        self.assertEqual("LABob", slot3.identifier)
+        self.assertEqual('LIGHT', slot3.id_root)
+
+    def test_same_name_same_type(self):
+        slot1 = self.action.slots.new('OBJECT', "Bob")
+        slot2 = self.action.slots.new('OBJECT', "Bob")
+        slot3 = self.action.slots.new('OBJECT', "Bob")
+
+        self.assertEqual("OBBob", slot1.identifier)
+        self.assertEqual('OBJECT', slot1.id_root)
+
+        self.assertEqual("OBBob.001", slot2.identifier)
+        self.assertEqual('OBJECT', slot2.id_root)
+
+        self.assertEqual("OBBob.002", slot3.identifier)
+        self.assertEqual('OBJECT', slot3.id_root)
+
+    def test_invalid_arguments(self):
+        with self.assertRaises(TypeError):
+            # ID type parameter is required.
+            self.action.slots.new('Hello')
+
+        with self.assertRaises(TypeError):
+            # Name parameter is required.
+            self.action.slots.new('OBJECT')
+
+        with self.assertRaises(RuntimeError):
+            # Name parameter must not be empty.
+            self.action.slots.new('OBJECT', "")
+
+        with self.assertRaises(TypeError):
+            # Creating slots with unspecified ID type is
+            # not supported in the Python API.
+            self.action.slots.new('UNSPECIFIED', "Bob")
+
+
 class ActionSlotAssignmentTest(unittest.TestCase):
     """Test assigning actions & check reference counts."""
 
@@ -53,13 +108,13 @@ class ActionSlotAssignmentTest(unittest.TestCase):
         cube = bpy.data.objects['Cube']
         cube_adt = cube.animation_data_create()
         cube_adt.action = action
-        slot_cube = action.slots.new(for_id=cube)
+        slot_cube = action.slots.new(cube.id_type, cube.name)
         cube_adt.action_slot_handle = slot_cube.handle
         self.assertEqual(cube_adt.action_slot_handle, slot_cube.handle)
 
         # Assign the Action to the camera as well.
         camera = bpy.data.objects['Camera']
-        slot_camera = action.slots.new(for_id=camera)
+        slot_camera = action.slots.new(camera.id_type, camera.name)
         camera_adt = camera.animation_data_create()
         camera_adt.action = action
         self.assertEqual(camera_adt.action_slot_handle, slot_camera.handle)
@@ -69,13 +124,13 @@ class ActionSlotAssignmentTest(unittest.TestCase):
         self.assertEqual(cube_adt.last_slot_identifier, slot_cube.identifier)
 
         # It should not be possible to set the slot handle while the Action is unassigned.
-        slot_extra = action.slots.new()
+        slot_extra = action.slots.new('OBJECT', "Slot")
         cube_adt.action_slot_handle = slot_extra.handle
         self.assertNotEqual(cube_adt.action_slot_handle, slot_extra.handle)
 
         # Slots from another Action should be gracefully rejected.
         other_action = bpy.data.actions.new("That Other Action")
-        slot = other_action.slots.new()
+        slot = other_action.slots.new('OBJECT', "Slot")
         cube_adt.action = action
         cube_adt.action_slot = slot_cube
         with self.assertRaises(RuntimeError):
@@ -147,7 +202,7 @@ class LegacyAPIOnLayeredActionTest(unittest.TestCase):
         self.action = bpy.data.actions.new('LayeredAction')
 
     def test_fcurves_on_layered_action(self) -> None:
-        slot = self.action.slots.new(for_id=bpy.data.objects['Cube'])
+        slot = self.action.slots.new(bpy.data.objects['Cube'].id_type, bpy.data.objects['Cube'].name)
 
         layer = self.action.layers.new(name="Layer")
         strip = layer.strips.new(type='KEYFRAME')
@@ -247,8 +302,7 @@ class ChannelBagsTest(unittest.TestCase):
 
         self.action = bpy.data.actions.new('TestAction')
 
-        self.slot = self.action.slots.new()
-        self.slot.identifier = 'OBTest'
+        self.slot = self.action.slots.new('OBJECT', "Test")
 
         self.layer = self.action.layers.new(name="Layer")
         self.strip = self.layer.strips.new(type='KEYFRAME')
@@ -285,7 +339,7 @@ class ChannelBagsTest(unittest.TestCase):
 
         # Removing an unrelated F-Curve should fail, even when an F-Curve with
         # the same RNA path and array index exists.
-        other_slot = self.action.slots.new()
+        other_slot = self.action.slots.new('OBJECT', "Slot")
         other_cbag = self.strip.channelbags.new(other_slot)
         other_fcurve = other_cbag.fcurves.new('location', index=1)
         with self.assertRaises(RuntimeError):
@@ -354,7 +408,7 @@ class ChannelBagsTest(unittest.TestCase):
 
         # Attempting to remove a channel group that belongs to a different
         # channel bag should fail.
-        other_slot = self.action.slots.new()
+        other_slot = self.action.slots.new('OBJECT', "Slot")
         other_cbag = self.strip.channelbags.new(other_slot)
         other_group = other_cbag.groups.new('group1')
         with self.assertRaises(RuntimeError):
@@ -378,8 +432,7 @@ class DataPathTest(unittest.TestCase):
     def test_repr(self):
         action = bpy.data.actions.new('TestAction')
 
-        slot = action.slots.new()
-        slot.identifier = 'OBTest'
+        slot = action.slots.new('OBJECT', "Test")
         self.assertEqual("bpy.data.actions['TestAction'].slots[\"OBTest\"]", repr(slot))
 
         layer = action.layers.new(name="Layer")

tests/python/bl_animation_nla_strip.py

@@ -116,14 +116,12 @@ class NlaStripBoundaryTest(AbstractNlaStripTest):
 class NLAStripActionSlotSelectionTest(AbstractNlaStripTest):
     def test_two_strips_for_same_action(self):
         action = bpy.data.actions.new("StripAction")
-        action.slots.new()
+        action.slots.new('OBJECT', "Slot")
         self.assertTrue(action.is_action_layered)
         self.assertEqual(1, len(action.slots))
 
         track = self.nla_tracks.new()
 
-        self.assertEqual('UNSPECIFIED', action.slots[0].id_root)
-
         strip1 = track.strips.new("name", 1, action)
         self.assertEqual(action.slots[0], strip1.action_slot)
         self.assertEqual('OBJECT', action.slots[0].id_root, "Slot should have been rooted to object")
@@ -133,12 +131,12 @@ class NLAStripActionSlotSelectionTest(AbstractNlaStripTest):
 
     def test_switch_action_via_assignment(self):
         action1 = bpy.data.actions.new("StripAction 1")
-        action1.slots.new()
+        action1.slots.new('OBJECT', "Slot")
         self.assertTrue(action1.is_action_layered)
         self.assertEqual(1, len(action1.slots))
 
         action2 = bpy.data.actions.new("StripAction 2")
-        action2.slots.new()
+        action2.slots.new('OBJECT', "Slot")
         self.assertTrue(action2.is_action_layered)
         self.assertEqual(1, len(action2.slots))
 

tests/python/bl_constraints.py

@@ -450,7 +450,7 @@ class ActionConstraintTest(AbstractConstraintTests):
 
     def test_assign_action_slot_virgin(self):
         action = bpy.data.actions.new("Slotted")
-        slot = action.slots.new()
+        slot = action.slots.new('OBJECT', "Slot")
 
         con = self.constraint()
         con.action = action