diff --git a/source/blender/animrig/ANIM_nla.hh b/source/blender/animrig/ANIM_nla.hh index dc2bd1c246e..73c3b89c9c4 100644 --- a/source/blender/animrig/ANIM_nla.hh +++ b/source/blender/animrig/ANIM_nla.hh @@ -22,7 +22,7 @@ namespace blender::animrig::nla { * * \see blender::animrig::assign_action * - * \returns whether a slot was automatically assigned. + * \returns whether the assignment was ok. */ bool assign_action(NlaStrip &strip, Action &action, ID &animated_id); diff --git a/source/blender/animrig/intern/nla.cc b/source/blender/animrig/intern/nla.cc index bb59c062e7c..4776474f7b0 100644 --- a/source/blender/animrig/intern/nla.cc +++ b/source/blender/animrig/intern/nla.cc @@ -18,52 +18,47 @@ namespace blender::animrig::nla { bool assign_action(NlaStrip &strip, Action &action, ID &animated_id) { - if (strip.act == &action) { - /* Already assigned, so just leave the slot as-is. */ + if (!generic_assign_action( + animated_id, &action, strip.act, strip.action_slot_handle, strip.action_slot_name)) + { return false; } - unassign_action(strip, animated_id); - - /* Assign the Action. */ - strip.act = &action; - id_us_plus(&action.id); - - /* Find a slot with the previously-used slot name. */ - if (strip.action_slot_name[0]) { - Slot *slot = action.slot_find_by_name(strip.action_slot_name); - if (slot && assign_action_slot(strip, slot, animated_id) == ActionSlotAssignmentResult::OK) { - return true; + /* For the NLA, the auto slot selection gets one more fallback option (compared to the generic + * code). This is to support the following scenario: + * + * - Python script creates an Action, and adds some F-Curves via the legacy API. + * - This creates a slot 'XXSlot'. + * - The script creates multiple NLA strips for that Action. + * - The desired result is that these strips get the same Slot assigned as well. + * + * The generic code doesn't work for this. The first strip assignment would see the slot + * `XXSlot`, and because it has never been used, just use it. This would change its name to, for + * example, `OBSlot`. The second strip assignment would not see a 'virgin' slot, and thus not + * auto-select `OBSlot`. This behaviour makes sense when assigning Actions in the Action editor + * (it shouldn't automatically pick the first slot of matching ID type), but for the NLA I + * (Sybren) feel that it could be a bit more 'enthousiastic' in auto-picking a slot. + */ + if (strip.action_slot_handle == Slot::unassigned && action.slots().size() == 1) { + Slot *first_slot = action.slot(0); + if (first_slot->is_suitable_for(animated_id)) { + const ActionSlotAssignmentResult result = assign_action_slot(strip, first_slot, animated_id); + BLI_assert_msg(result == ActionSlotAssignmentResult::OK, + "Assigning a slot that we know is suitable should work"); + UNUSED_VARS_NDEBUG(result); } } - /* As a last resort, search for the ID name. */ - Slot *slot = action.slot_find_by_name(animated_id.name); - if (slot && assign_action_slot(strip, slot, animated_id) == ActionSlotAssignmentResult::OK) { - return true; - } - - return false; + /* Regardless of slot auto-selection, the Action assignment worked just fine. */ + return true; } void unassign_action(NlaStrip &strip, ID &animated_id) { - if (!strip.act) { - /* No Action was assigned, so nothing to do here. */ - BLI_assert_msg(strip.action_slot_handle == Slot::unassigned, - "When there is no Action assigned, the slot handle should also be set to " - "'unassigned'"); - return; - } - - /* Unassign any previously-assigned slot. */ - if (strip.action_slot_handle != Slot::unassigned) { - assign_action_slot(strip, nullptr, animated_id); - } - - /* Unassign the Action. */ - id_us_min(&strip.act->id); - strip.act = nullptr; + const bool ok = generic_assign_action( + animated_id, nullptr, strip.act, strip.action_slot_handle, strip.action_slot_name); + BLI_assert_msg(ok, "Un-assigning an Action from an NLA strip should always work."); + UNUSED_VARS_NDEBUG(ok); } ActionSlotAssignmentResult assign_action_slot(NlaStrip &strip, diff --git a/source/blender/animrig/intern/nla_test.cc b/source/blender/animrig/intern/nla_test.cc index 29b0ade1468..d9ea096dc0b 100644 --- a/source/blender/animrig/intern/nla_test.cc +++ b/source/blender/animrig/intern/nla_test.cc @@ -81,15 +81,16 @@ TEST_F(NLASlottedActionTest, assign_slot_to_nla_strip) EXPECT_EQ(strip->act, nullptr); EXPECT_EQ(action->id.us, 0); - /* Assign an Action with an unrelated slot. This should not be picked. */ - action->slot_add(); + /* Assign an Action with a never-assigned slot. This should be picked automatically. */ + Slot &virgin_slot = action->slot_add(); /* Assign the Action. */ - EXPECT_FALSE(nla::assign_action(*strip, *action, cube->id)); - EXPECT_EQ(strip->action_slot_handle, Slot::unassigned); - EXPECT_STREQ(strip->action_slot_name, ""); + EXPECT_TRUE(nla::assign_action(*strip, *action, cube->id)); + EXPECT_EQ(strip->action_slot_handle, virgin_slot.handle); + EXPECT_STREQ(strip->action_slot_name, virgin_slot.name); EXPECT_EQ(action->id.us, 1); EXPECT_EQ(strip->act, action); + EXPECT_EQ(virgin_slot.idtype, GS(cube->id.name)); /* Unassign the Action. */ nla::unassign_action(*strip, cube->id); @@ -141,17 +142,21 @@ TEST_F(NLASlottedActionTest, assign_slot_to_multiple_strips) nla::unassign_action(*strip1, cube->id); nla::unassign_action(*strip2, cube->id); - /* Create an unrelated slot, it should not be auto-picked. */ + /* Create a virgin slot, it should be auto-picked. */ Slot &slot = action->slot_add(); - EXPECT_FALSE(nla::assign_action(*strip1, *action, cube->id)); - EXPECT_EQ(strip1->action_slot_handle, Slot::unassigned); - - /* Assign the slot 'manually'. */ - EXPECT_EQ(nla::assign_action_slot(*strip1, &slot, cube->id), ActionSlotAssignmentResult::OK); + EXPECT_TRUE(nla::assign_action(*strip1, *action, cube->id)); EXPECT_EQ(strip1->action_slot_handle, slot.handle); + EXPECT_STREQ(strip1->action_slot_name, slot.name); + EXPECT_EQ(slot.idtype, ID_OB); + + /* Assign another slot slot 'manually'. */ + Slot &other_slot = action->slot_add(); + EXPECT_EQ(nla::assign_action_slot(*strip1, &other_slot, cube->id), + ActionSlotAssignmentResult::OK); + EXPECT_EQ(strip1->action_slot_handle, other_slot.handle); /* Assign the Action + slot to the second strip.*/ - EXPECT_FALSE(nla::assign_action(*strip2, *action, cube->id)); + EXPECT_TRUE(nla::assign_action(*strip2, *action, cube->id)); EXPECT_EQ(nla::assign_action_slot(*strip2, &slot, cube->id), ActionSlotAssignmentResult::OK); /* The cube should be registered as user of the slot. */ diff --git a/source/blender/makesrna/intern/rna_nla.cc b/source/blender/makesrna/intern/rna_nla.cc index 3207c2e75f7..828f882971c 100644 --- a/source/blender/makesrna/intern/rna_nla.cc +++ b/source/blender/makesrna/intern/rna_nla.cc @@ -435,6 +435,30 @@ static void rna_NlaStrip_use_auto_blend_set(PointerRNA *ptr, bool value) } } +static void rna_NlaStrip_action_set(PointerRNA *ptr, PointerRNA value, ReportList *reports) +{ + using namespace blender::animrig; + BLI_assert(ptr->owner_id); + BLI_assert(ptr->data); + + ID &animated_id = *ptr->owner_id; + NlaStrip &strip = *static_cast(ptr->data); + Action *action = static_cast(value.data); + + if (!action) { + nla::unassign_action(strip, animated_id); + return; + } + + if (!nla::assign_action(strip, *action, animated_id)) { + BKE_reportf(reports, + RPT_ERROR, + "Could not assign action %s to NLA strip %s", + action->id.name + 2, + strip.name); + } +} + static int rna_NlaStrip_action_editable(const PointerRNA *ptr, const char ** /*r_info*/) { NlaStrip *strip = (NlaStrip *)ptr->data; @@ -875,8 +899,9 @@ static void rna_def_nlastrip(BlenderRNA *brna) /* Action */ prop = RNA_def_property(srna, "action", PROP_POINTER, PROP_NONE); RNA_def_property_pointer_sdna(prop, nullptr, "act"); - RNA_def_property_pointer_funcs(prop, nullptr, nullptr, nullptr, "rna_Action_id_poll"); - RNA_def_property_flag(prop, PROP_EDITABLE | PROP_ID_REFCOUNT); + RNA_def_property_pointer_funcs( + prop, nullptr, "rna_NlaStrip_action_set", nullptr, "rna_Action_id_poll"); + RNA_def_property_flag(prop, PROP_EDITABLE); RNA_def_property_editable_func(prop, "rna_NlaStrip_action_editable"); RNA_def_property_ui_text(prop, "Action", "Action referenced by this strip"); RNA_def_property_update( diff --git a/tests/python/bl_animation_nla_strip.py b/tests/python/bl_animation_nla_strip.py index 12ad35994c2..dd0f59c5778 100644 --- a/tests/python/bl_animation_nla_strip.py +++ b/tests/python/bl_animation_nla_strip.py @@ -13,6 +13,16 @@ import sys import unittest +def enable_experimental_animation_baklava(): + bpy.context.preferences.view.show_developer_ui = True + bpy.context.preferences.experimental.use_animation_baklava = True + + +def disable_experimental_animation_baklava(): + bpy.context.preferences.view.show_developer_ui = False + bpy.context.preferences.experimental.use_animation_baklava = False + + class AbstractNlaStripTest(unittest.TestCase): """ Sets up a series of strips in one NLA track. """ @@ -113,6 +123,55 @@ class NlaStripBoundaryTest(AbstractNlaStripTest): self.assertFrameValue(4.1, 1.0) +class NLAStripActionSlotSelectionTest(AbstractNlaStripTest): + + def setUp(self): + enable_experimental_animation_baklava() + return super().setUp() + + def tearDown(self) -> None: + disable_experimental_animation_baklava() + return super().tearDown() + + def test_two_strips_for_same_action(self): + action = bpy.data.actions.new("StripAction") + action.slots.new() + 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") + + strip2 = track.strips.new("name", 10, action) + self.assertEqual(action.slots[0], strip2.action_slot) + + def test_switch_action_via_assignment(self): + action1 = bpy.data.actions.new("StripAction 1") + action1.slots.new() + self.assertTrue(action1.is_action_layered) + self.assertEqual(1, len(action1.slots)) + + action2 = bpy.data.actions.new("StripAction 2") + action2.slots.new() + self.assertTrue(action2.is_action_layered) + self.assertEqual(1, len(action2.slots)) + + track = self.nla_tracks.new() + + strip = track.strips.new("name", 1, action1) + self.assertEqual(action1.slots[0], strip.action_slot) + self.assertEqual('OBJECT', action1.slots[0].id_root, "Slot of Action 1 should have been rooted to object") + + strip.action = action2 + self.assertEqual(action2.slots[0], strip.action_slot) + self.assertEqual('OBJECT', action2.slots[0].id_root, "Slot of Action 2 should have been rooted to object") + + if __name__ == "__main__": # Drop all arguments before "--", or everything if the delimiter is absent. Keep the executable path. unittest.main(argv=sys.argv[:1] + (sys.argv[sys.argv.index("--") + 1:] if "--" in sys.argv else []))