griefith/test
1
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

Anim: Fix NLA Strip Action assignment

Browse Source 
When creating a new NLA strip for an action, as well as when setting
`strip.action` via RNA, use the generic action-assignment code. This
ensures that the slot selection follows the same logic as other Action
assignments.

If the generic slot selection doesn't find a suitable slot, and there is
a single slot on that Action of a suitable ID type, always assign it.
This is to support the following scenario:

- Python script creates an Action and adds 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, because:

- The first strip assignment would see the slot `XXSlot` (`XX`
  indicating "not bound to any ID type yet"). Because that slot has
  never been used, it will be assigned (which is good). This assignment
  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 (assigning an Action that already
  animates 'Cube' to 'Suzanne' should not assign the 'OBCube' slot to
  Suzanne), but for the NLA I feel that it could be a bit more
  'enthousiastic' in auto-picking a slot to support the above case.

This is preparation for the removal of the 'Slotted Actions'
experimental flag, and getting the new code to run as compatibly as
possible with the legacy code.

The return value of `animrig::nla::assign_action()` has changed a bit.
It used to indicate whether a slot was auto-selected; it now indicates
whether the Action assignment was successful. Whether a slot was
assigned or not can be seen at `strip.action_slot`.

Pull Request: https://projects.blender.org/blender/blender/pulls/128892
This commit is contained in:
Sybren A. Stüvel
2024-10-11 10:38:24 +02:00
parent ee3aea4caf
commit 76bddb41df
5 changed files with 135 additions and 51 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
source/blender/animrig/ANIM_nla.hh
View File

@@ -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);

67
source/blender/animrig/intern/nla.cc
View File

@@ -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,

29
source/blender/animrig/intern/nla_test.cc
View File

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

29
source/blender/makesrna/intern/rna_nla.cc
View File

@@ -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<NlaStrip *>(ptr->data);
Action *action = static_cast<Action *>(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(

59
tests/python/bl_animation_nla_strip.py
View File

@@ -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 []))