From 66cdc112fc573bdcfcbc050c55d004dbb657bd34 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sybren=20A=2E=20St=C3=BCvel?= Date: Fri, 5 Jan 2024 16:51:26 +0100 Subject: [PATCH] Anim: Bone Collections, replace `.move_to_parent()` with `.child_number` Instead of moving bone collections by absolute index, they can now be moved by manipulating `.child_number`. This is the relative index of the bone collection within the list of its siblings. This replaces the much more cumbersome `collections.move_to_parent()` function. Since that function is now no longer necessary (it's been replaced by assignment to `.parent` and `.child_number`), it's removed from RNA. Note that this function was never part of even a beta build of Blender. --- .../blender/animrig/ANIM_bone_collections.hh | 22 ++++++ .../animrig/intern/bone_collections.cc | 49 +++++++++++++ .../animrig/intern/bone_collections_test.cc | 53 ++++++++++++++ .../blender/makesrna/intern/rna_armature.cc | 23 ++++++ .../makesrna/intern/rna_armature_api.cc | 72 ------------------- tests/python/bl_animation_armature.py | 5 +- 6 files changed, 150 insertions(+), 74 deletions(-) diff --git a/source/blender/animrig/ANIM_bone_collections.hh b/source/blender/animrig/ANIM_bone_collections.hh index 3be9a5938e1..6caa5727aef 100644 --- a/source/blender/animrig/ANIM_bone_collections.hh +++ b/source/blender/animrig/ANIM_bone_collections.hh @@ -327,6 +327,28 @@ int armature_bonecoll_find_index(const bArmature *armature, const ::BoneCollecti */ int armature_bonecoll_find_parent_index(const bArmature *armature, int bcoll_index); +/** + * Find the child number of this bone collection. + * + * This is the offset of this collection relative to the parent's first child. + * In other words, the first child has number 0, second child has number 1, etc. + * + * This requires a scan of the array, hence the function is called 'find' and not 'get'. + */ +int armature_bonecoll_child_number_find(const bArmature *armature, const ::BoneCollection *bcoll); + +/** + * Move this bone collection to a new child number. + * + * \return the new absolute index of the bone collection, or -1 if the new child number was not + * valid. + * + * \see armature_bonecoll_child_number_find + */ +int armature_bonecoll_child_number_set(bArmature *armature, + ::BoneCollection *bcoll, + int new_child_number); + bool armature_bonecoll_is_root(const bArmature *armature, int bcoll_index); bool armature_bonecoll_is_child_of(const bArmature *armature, diff --git a/source/blender/animrig/intern/bone_collections.cc b/source/blender/animrig/intern/bone_collections.cc index 3558ebded8c..84b6593960a 100644 --- a/source/blender/animrig/intern/bone_collections.cc +++ b/source/blender/animrig/intern/bone_collections.cc @@ -1075,6 +1075,55 @@ int armature_bonecoll_find_parent_index(const bArmature *armature, const int bco return -1; } +int armature_bonecoll_child_number_find(const bArmature *armature, const ::BoneCollection *bcoll) +{ + const int bcoll_index = armature_bonecoll_find_index(armature, bcoll); + const int parent_index = armature_bonecoll_find_parent_index(armature, bcoll_index); + return bonecoll_child_number(armature, parent_index, bcoll_index); +} + +int armature_bonecoll_child_number_set(bArmature *armature, + ::BoneCollection *bcoll, + int new_child_number) +{ + const int bcoll_index = armature_bonecoll_find_index(armature, bcoll); + const int parent_index = armature_bonecoll_find_parent_index(armature, bcoll_index); + + BoneCollection fake_armature_parent = {}; + fake_armature_parent.child_count = armature->collection_root_count; + + BoneCollection *parent_bcoll; + if (parent_index < 0) { + parent_bcoll = &fake_armature_parent; + } + else { + parent_bcoll = armature->collection_array[parent_index]; + } + + /* Bounds checks. */ + if (new_child_number >= parent_bcoll->child_count) { + return -1; + } + if (new_child_number < 0) { + new_child_number = parent_bcoll->child_count - 1; + } + + /* Store the parent's child_index, as that might move if to_index is the first child + * (bonecolls_move_to_index() will keep it pointing at that first child). */ + const int old_parent_child_index = parent_bcoll->child_index; + const int to_index = parent_bcoll->child_index + new_child_number; + internal::bonecolls_move_to_index(armature, bcoll_index, to_index); + + parent_bcoll->child_index = old_parent_child_index; + + /* Make sure that if this was the active bone collection, its index also changes. */ + if (armature->runtime.active_collection_index == bcoll_index) { + ANIM_armature_bonecoll_active_index_set(armature, to_index); + } + + return to_index; +} + bool armature_bonecoll_is_root(const bArmature *armature, const int bcoll_index) { BLI_assert(bcoll_index >= 0); diff --git a/source/blender/animrig/intern/bone_collections_test.cc b/source/blender/animrig/intern/bone_collections_test.cc index 8005cfee18a..ca15d0201d7 100644 --- a/source/blender/animrig/intern/bone_collections_test.cc +++ b/source/blender/animrig/intern/bone_collections_test.cc @@ -1426,6 +1426,59 @@ TEST_F(ANIM_armature_bone_collections_testlist, move_before_after_index__to_root EXPECT_EQ(-1, armature_bonecoll_find_parent_index(&arm, 1)); } +TEST_F(ANIM_armature_bone_collections_testlist, child_number_set__roots) +{ + /* Test with only one root. */ + EXPECT_EQ(0, armature_bonecoll_child_number_set(&arm, root, 0)); + EXPECT_TRUE(expect_bcolls({"root", "child0", "child1", "child2", "child1_0"})); + + /* Move to "after the last child", which is the one root itself. */ + EXPECT_EQ(0, armature_bonecoll_child_number_set(&arm, root, -1)); + EXPECT_TRUE(expect_bcolls({"root", "child0", "child1", "child2", "child1_0"})); + EXPECT_EQ(0, armature_bonecoll_child_number_set(&arm, root, 0)); + EXPECT_TRUE(expect_bcolls({"root", "child0", "child1", "child2", "child1_0"})); + + /* Going beyond the number of children is not allowed. */ + EXPECT_EQ(-1, armature_bonecoll_child_number_set(&arm, root, 1)); + EXPECT_TRUE(expect_bcolls({"root", "child0", "child1", "child2", "child1_0"})); + + /* Add two roots to be able to play. */ + ANIM_armature_bonecoll_new(&arm, "root1"); + ANIM_armature_bonecoll_new(&arm, "root2"); + EXPECT_TRUE(expect_bcolls({"root", "root1", "root2", "child0", "child1", "child2", "child1_0"})); + + /* Move the old root in between the two new ones. */ + EXPECT_EQ(1, armature_bonecoll_child_number_set(&arm, root, 1)); + EXPECT_TRUE(expect_bcolls({"root1", "root", "root2", "child0", "child1", "child2", "child1_0"})); + + /* And to the last one. */ + EXPECT_EQ(2, armature_bonecoll_child_number_set(&arm, root, 2)); + EXPECT_TRUE(expect_bcolls({"root1", "root2", "root", "child0", "child1", "child2", "child1_0"})); +} + +TEST_F(ANIM_armature_bone_collections_testlist, child_number_set__siblings) +{ + /* Move child0 to itself. */ + EXPECT_EQ(1, armature_bonecoll_child_number_set(&arm, child0, 0)); + EXPECT_TRUE(expect_bcolls({"root", "child0", "child1", "child2", "child1_0"})); + + /* Move child2 to itself. */ + EXPECT_EQ(3, armature_bonecoll_child_number_set(&arm, child2, -1)); + EXPECT_TRUE(expect_bcolls({"root", "child0", "child1", "child2", "child1_0"})); + + /* Going beyond the number of children is not allowed. */ + EXPECT_EQ(-1, armature_bonecoll_child_number_set(&arm, child0, 3)); + EXPECT_TRUE(expect_bcolls({"root", "child0", "child1", "child2", "child1_0"})); + + /* Move child0 to in between child1 and child2. */ + EXPECT_EQ(2, armature_bonecoll_child_number_set(&arm, child0, 1)); + EXPECT_TRUE(expect_bcolls({"root", "child1", "child0", "child2", "child1_0"})); + + /* Move child0 to the last spot. */ + EXPECT_EQ(3, armature_bonecoll_child_number_set(&arm, child0, 2)); + EXPECT_TRUE(expect_bcolls({"root", "child1", "child2", "child0", "child1_0"})); +} + class ANIM_armature_bone_collections_liboverrides : public ANIM_armature_bone_collections_testlist { protected: diff --git a/source/blender/makesrna/intern/rna_armature.cc b/source/blender/makesrna/intern/rna_armature.cc index 72c3823d4d6..1c4716e60c4 100644 --- a/source/blender/makesrna/intern/rna_armature.cc +++ b/source/blender/makesrna/intern/rna_armature.cc @@ -425,6 +425,20 @@ static int rna_BoneCollection_index_get(PointerRNA *ptr) return blender::animrig::armature_bonecoll_find_index(arm, bcoll); } +static int rna_BoneCollection_child_number_get(PointerRNA *ptr) +{ + bArmature *arm = reinterpret_cast(ptr->owner_id); + BoneCollection *bcoll = static_cast(ptr->data); + return blender::animrig::armature_bonecoll_child_number_find(arm, bcoll); +} +static void rna_BoneCollection_child_number_set(PointerRNA *ptr, const int new_child_number) +{ + bArmature *arm = reinterpret_cast(ptr->owner_id); + BoneCollection *bcoll = static_cast(ptr->data); + blender::animrig::armature_bonecoll_child_number_set(arm, bcoll, new_child_number); + WM_main_add_notifier(NC_OBJECT | ND_BONE_COLLECTION, nullptr); +} + /* BoneCollection.bones iterator functions. */ static void rna_BoneCollection_bones_begin(CollectionPropertyIterator *iter, PointerRNA *ptr) @@ -2348,6 +2362,15 @@ static void rna_def_bonecollection(BlenderRNA *brna) "Index of this bone collection in the armature.collections.all array. Note that finding " "this index requires a scan of all the bone collections, so do access this with care"); + prop = RNA_def_property(srna, "child_number", PROP_INT, PROP_NONE); + RNA_def_property_int_funcs( + prop, "rna_BoneCollection_child_number_get", "rna_BoneCollection_child_number_set", nullptr); + RNA_def_property_ui_text( + prop, + "Child Number", + "Index of this collection into its parent's list of children. Note that finding " + "this index requires a scan of all the bone collections, so do access this with care"); + RNA_api_bonecollection(srna); } diff --git a/source/blender/makesrna/intern/rna_armature_api.cc b/source/blender/makesrna/intern/rna_armature_api.cc index 30bccd418b2..6ad7b5ceabc 100644 --- a/source/blender/makesrna/intern/rna_armature_api.cc +++ b/source/blender/makesrna/intern/rna_armature_api.cc @@ -171,35 +171,6 @@ static bool rna_BoneCollection_unassign(BoneCollection *bcoll, ANIM_armature_bonecoll_unassign_editbone); } -static int rna_BoneCollection_move_to_parent(ID *owner_id, - BoneCollection *self, - bContext *C, - ReportList *reports, - BoneCollection *to_parent, - const int to_child_num) -{ - using namespace blender::animrig; - - bArmature *armature = (bArmature *)owner_id; - - const int from_bcoll_index = armature_bonecoll_find_index(armature, self); - const int from_parent_index = armature_bonecoll_find_parent_index(armature, from_bcoll_index); - const int to_parent_index = armature_bonecoll_find_index(armature, to_parent); - - if (to_parent_index == from_bcoll_index || - armature_bonecoll_is_decendent_of(armature, from_bcoll_index, to_parent_index)) - { - BKE_report(reports, RPT_ERROR, "Cannot make a bone collection a decendent of itself"); - return from_bcoll_index; - } - - const int new_bcoll_index = armature_bonecoll_move_to_parent( - armature, from_bcoll_index, to_child_num, from_parent_index, to_parent_index); - - WM_event_add_notifier(C, NC_OBJECT | ND_BONE_COLLECTION, nullptr); - return new_bcoll_index; -} - #else void RNA_api_armature_edit_bone(StructRNA *srna) @@ -349,49 +320,6 @@ void RNA_api_bonecollection(StructRNA *srna) "Whether the bone was actually removed; will be false if the bone was " "not a member of the collection to begin with"); RNA_def_function_return(func, parm); - - /* collection.move_to_parent(parent, child_num) */ - func = RNA_def_function(srna, "move_to_parent", "rna_BoneCollection_move_to_parent"); - RNA_def_function_ui_description( - func, - "Change the hierarchy, by moving this bone collection to become a child of a different " - "parent. Use `parent=None` to make this collection a root collection"); - RNA_def_function_flag(func, FUNC_USE_SELF_ID | FUNC_USE_CONTEXT | FUNC_USE_REPORTS); - - parm = RNA_def_pointer(func, - "to_parent", - "BoneCollection", - "Parent Collection", - "The bone collection becomes a child of this collection. If `None`, the " - "bone collection becomes a root"); - RNA_def_parameter_flags(parm, PropertyFlag(0), PARM_REQUIRED); - - parm = RNA_def_int(func, - "to_child_num", - -1, - -1, - INT_MAX, - "Child Number", - "Place the bone collection before this child; `child_num=0` makes it the " - "first child, `child_num=1` the second, etc. Both `child_num=-1` and " - "`child_num=child_count` will move the bone collection to be the last child " - "of the new parent", - -1, - INT_MAX); - - /* Return value. */ - - parm = RNA_def_int(func, - "new_index", - -1, - -1, - INT_MAX, - "New Index", - "Resulting index of the bone collection, after the move. This can be used " - "for manipulating the active bone collection index", - -1, - INT_MAX); - RNA_def_function_return(func, parm); } #endif diff --git a/tests/python/bl_animation_armature.py b/tests/python/bl_animation_armature.py index 7bc3efdc0fe..dcd12cfacab 100644 --- a/tests/python/bl_animation_armature.py +++ b/tests/python/bl_animation_armature.py @@ -87,7 +87,7 @@ class BoneCollectionTest(unittest.TestCase): self.assertEqual([root1, root2, r1_child1, r1_child1_001, r2_child1, r2_child2], list(bcolls.all)) # Move root2 to become the child of r1_child1. - self.assertEqual(5, root2.move_to_parent(r1_child1)) + root2.parent = r1_child1 # Check the hierarchy. self.assertEqual([root1], list(bcolls), 'armature.collections should reflect only the roots') @@ -100,7 +100,8 @@ class BoneCollectionTest(unittest.TestCase): self.assertEqual([root1, r1_child1, r1_child1_001, r2_child1, r2_child2, root2], list(bcolls.all)) # Move root2 between r1_child1 and r1_child1_001. - self.assertEqual(2, root2.move_to_parent(root1, to_child_num=1)) + root2.parent = root1 + root2.child_num = 1 # Check the hierarchy. self.assertEqual([root1], list(bcolls), 'armature.collections should reflect only the roots')