From eda2f11f7a4bef34c5176d0f3e2bfacf2b0394af Mon Sep 17 00:00:00 2001 From: Nathan Vegdahl Date: Tue, 4 Feb 2025 13:39:50 +0100 Subject: [PATCH] Anim: change RNA Action.id_root to have backwards-compatible behavior Most of the old Animato properties on an Action (e.g. FCurve list, Channel Groups) already act as proxies for the data for the first slot in the first strip of the first layer. (Say that three times fast!) However, this was not yet the case for `Action.id_root`. This PR changes `Action.id_root` to act as a proxy for the first Slot's `target_id_type` property, both for reading and writing. If the Action has no Slots, then reading always returns 'UNSPECIFIED', and writing will create a Slot and set its `target_id_type`. Note that the ability to write to the first Slot's `target_id_type` via `Action.id_root` conflicts with `target_id_type` supposedly only being writable when it's still 'UNSPECIFIED' (#133883). Although that's certainly a little weird, practically speaking this doesn't break anything for now, and is a temporary kludge to keep `id_root` working until we can remove it in Blender 5.0. `id_root` will be removed entirely in 5.0, resolving this inconsistency. Pull Request: https://projects.blender.org/blender/blender/pulls/133823 --- source/blender/animrig/ANIM_action_legacy.hh | 8 +++ .../blender/animrig/intern/action_legacy.cc | 25 +++++--- source/blender/makesrna/intern/rna_action.cc | 52 ++++++++++++++- tests/python/bl_animation_action.py | 64 ++++++++++++++++++- 4 files changed, 137 insertions(+), 12 deletions(-) diff --git a/source/blender/animrig/ANIM_action_legacy.hh b/source/blender/animrig/ANIM_action_legacy.hh index c37df2aaeb7..a9428597815 100644 --- a/source/blender/animrig/ANIM_action_legacy.hh +++ b/source/blender/animrig/ANIM_action_legacy.hh @@ -21,6 +21,14 @@ namespace blender::animrig::legacy { constexpr const char *DEFAULT_LEGACY_SLOT_NAME = "Legacy Slot"; constexpr const char *DEFAULT_LEGACY_LAYER_NAME = "Legacy Layer"; +/** + * Ensure that a Slot exists, for legacy Python API shims that need one. + * + * \return The first Slot if one already exists, or a newly created "Legacy + * Slot" otherwise. + */ +Slot &slot_ensure(Action &action); + /** * Return the Channelbag for compatibility with the legacy Python API. * diff --git a/source/blender/animrig/intern/action_legacy.cc b/source/blender/animrig/intern/action_legacy.cc index e4f2ab944ae..6b436e54c41 100644 --- a/source/blender/animrig/intern/action_legacy.cc +++ b/source/blender/animrig/intern/action_legacy.cc @@ -26,6 +26,19 @@ static Strip *first_keyframe_strip(Action &action) return nullptr; } +Slot &slot_ensure(Action &action) +{ + assert_baklava_phase_1_invariants(action); + + if (!action.slots().is_empty()) { + return *action.slot(0); + } + + Slot &slot = action.slot_add(); + action.slot_display_name_define(slot, DATA_(DEFAULT_LEGACY_SLOT_NAME)); + return slot; +} + Channelbag *channelbag_get(Action &action) { if (action.slots().is_empty()) { @@ -44,15 +57,7 @@ Channelbag &channelbag_ensure(Action &action) { assert_baklava_phase_1_invariants(action); - /* Ensure a Slot. */ - Slot *slot; - if (action.slots().is_empty()) { - slot = &action.slot_add(); - action.slot_display_name_define(*slot, DATA_(DEFAULT_LEGACY_SLOT_NAME)); - } - else { - slot = action.slot(0); - } + Slot &slot = slot_ensure(action); /* Ensure a Layer + keyframe Strip. * @@ -68,7 +73,7 @@ Channelbag &channelbag_ensure(Action &action) Strip &keystrip = *action.layer(0)->strip(0); /* Ensure a Channelbag. */ - return keystrip.data(action).channelbag_for_slot_ensure(*slot); + return keystrip.data(action).channelbag_for_slot_ensure(slot); } /* Lots of template args to support transparent non-const and const versions. */ diff --git a/source/blender/makesrna/intern/rna_action.cc b/source/blender/makesrna/intern/rna_action.cc index fea15678a7a..621cb7fd107 100644 --- a/source/blender/makesrna/intern/rna_action.cc +++ b/source/blender/makesrna/intern/rna_action.cc @@ -1571,6 +1571,52 @@ static void rna_ActionSlot_target_id_type_set(PointerRNA *ptr, int value) action.slot_idtype_define(slot, ID_Type(value)); } +/* For API backwards compatability with pre-layered-actions (Blender 4.3 and + * earlier), we treat `Action.id_root` as a proxy for the `target_id_type` + * property (`idtype` in DNA) of the Action's first Slot. + * + * If the Action has no slots, then we fallback to returning 'unspecified' (0). + */ +static int rna_Action_id_root_get(PointerRNA *ptr) +{ + animrig::Action &action = reinterpret_cast(ptr->owner_id)->wrap(); + + if (action.slots().is_empty()) { + return 0; + } + + return action.slot(0)->idtype; +} + +/* For API backwards compatability with pre-layered-actions (Blender 4.3 and + * earlier), we treat `Action.id_root` as a proxy for the `target_id_type` + * property (`idtype` in DNA) of the Action's first Slot. + * + * If the Action has no slots, then a legacy slot is created and its + * `target_id_type` is set. */ +static void rna_Action_id_root_set(PointerRNA *ptr, int value) +{ + animrig::Action &action = reinterpret_cast(ptr->owner_id)->wrap(); + + animrig::Slot &slot = animrig::legacy::slot_ensure(action); + action.slot_idtype_define(slot, ID_Type(value)); +} + +static void rna_Action_id_root_update(Main *bmain, Scene *, PointerRNA *ptr) +{ + animrig::Action &action = rna_action(ptr); + + if (action.slots().is_empty()) { + /* Nothing to do: id_root can't be set without at least one slot, so no + * change was possible that would necessitate an update. */ + return; + } + + /* Setting id_root actually sets the target ID type of the first slot, so it's + * the resulting changes to the first slot that we need to propagate. */ + action.slot_identifier_propagate(*bmain, *action.slot(0)); +} + #else static void rna_def_dopesheet(BlenderRNA *brna) @@ -2728,7 +2774,11 @@ static void rna_def_action_legacy(BlenderRNA *brna, StructRNA *srna) prop = RNA_def_property(srna, "id_root", PROP_ENUM, PROP_NONE); RNA_def_property_enum_sdna(prop, nullptr, "idroot"); RNA_def_property_enum_items(prop, default_ActionSlot_target_id_type_items); - RNA_def_property_enum_funcs(prop, nullptr, nullptr, "rna_ActionSlot_target_id_type_itemf"); + RNA_def_property_enum_funcs(prop, + "rna_Action_id_root_get", + "rna_Action_id_root_set", + "rna_ActionSlot_target_id_type_itemf"); + RNA_def_property_update(prop, NC_ANIMATION | ND_ANIMCHAN, "rna_Action_id_root_update"); RNA_def_property_flag(prop, PROP_ENUM_NO_CONTEXT); RNA_def_property_ui_text(prop, "ID Root Type", diff --git a/tests/python/bl_animation_action.py b/tests/python/bl_animation_action.py index 086a260bbbb..361a102efc0 100644 --- a/tests/python/bl_animation_action.py +++ b/tests/python/bl_animation_action.py @@ -324,7 +324,7 @@ class LegacyAPIOnLayeredActionTest(unittest.TestCase): - curve_frame_range - fcurves - groups - - id_root (should always be 0 for layered Actions) + - id_root - flip_with_pose(object) """ @@ -450,6 +450,68 @@ class LegacyAPIOnLayeredActionTest(unittest.TestCase): self.assertEqual([group], channelbag.groups[:]) + def test_id_root_on_layered_action(self) -> None: + # When there's at least one slot, action.id_root should simply act as a + # proxy for the first slot's target_id_type. This should work for both + # reading and writing. + + slot_1 = self.action.slots.new('OBJECT', "Slot 1") + slot_2 = self.action.slots.new('CAMERA', "Slot 2") + bpy.data.objects['Cube'].animation_data_create() + bpy.data.objects['Cube'].animation_data.action = self.action + bpy.data.objects['Cube'].animation_data.action_slot = slot_1 + + self.assertEqual(self.action.id_root, 'OBJECT') + self.assertEqual(self.action.slots[0].target_id_type, 'OBJECT') + self.assertEqual(self.action.slots[0].identifier, 'OBSlot 1') + self.assertEqual(self.action.slots[1].target_id_type, 'CAMERA') + self.assertEqual(self.action.slots[1].identifier, 'CASlot 2') + self.assertEqual(bpy.data.objects['Cube'].animation_data.last_slot_identifier, 'OBSlot 1') + + self.action.id_root = 'MATERIAL' + + self.assertEqual(self.action.id_root, 'MATERIAL') + self.assertEqual(self.action.slots[0].target_id_type, 'MATERIAL') + self.assertEqual(self.action.slots[0].identifier, 'MASlot 1') + self.assertEqual(self.action.slots[1].target_id_type, 'CAMERA') + self.assertEqual(self.action.slots[1].identifier, 'CASlot 2') + self.assertEqual(bpy.data.objects['Cube'].animation_data.last_slot_identifier, 'MASlot 1') + + def test_id_root_on_layered_action_for_identifier_uniqueness(self) -> None: + # When setting id_root such that the first slot's identifier would + # become a duplicate, the name portion of the identifier should be + # automatically renamed to be unique. + + slot_1 = self.action.slots.new('OBJECT', "Foo") + slot_2 = self.action.slots.new('CAMERA', "Foo") + + self.assertEqual(self.action.id_root, 'OBJECT') + self.assertEqual(self.action.slots[0].target_id_type, 'OBJECT') + self.assertEqual(self.action.slots[0].identifier, 'OBFoo') + self.assertEqual(self.action.slots[1].target_id_type, 'CAMERA') + self.assertEqual(self.action.slots[1].identifier, 'CAFoo') + + self.action.id_root = 'CAMERA' + + self.assertEqual(self.action.id_root, 'CAMERA') + self.assertEqual(self.action.slots[0].target_id_type, 'CAMERA') + self.assertEqual(self.action.slots[0].identifier, 'CAFoo.001') + self.assertEqual(self.action.slots[1].target_id_type, 'CAMERA') + self.assertEqual(self.action.slots[1].identifier, 'CAFoo') + + def test_id_root_on_empty_action(self) -> None: + # When there are no slots, setting action.id_root should create a legacy + # slot and set its target_id_type. + + self.assertEqual(self.action.id_root, 'UNSPECIFIED') + self.assertEqual(len(self.action.slots), 0) + + self.action.id_root = 'OBJECT' + + self.assertEqual(self.action.id_root, 'OBJECT') + self.assertEqual(len(self.action.slots), 1) + self.assertEqual(self.action.slots[0].target_id_type, 'OBJECT') + class ChannelbagsTest(unittest.TestCase): def setUp(self):