From 5dfbe1af2198c67d73166ae0bb0c7ed05321e6ec Mon Sep 17 00:00:00 2001 From: Christoph Lendenfeld Date: Tue, 29 Jul 2025 14:37:35 +0200 Subject: [PATCH] Refactor: Replace PBONE_SELECTED macro with functions This PR changes the uses of `PBONE_SELECTED` and all the places where it *should* have been used with a new function `bone_is_selected` or `...editbone` and `...pose_bone` specializations. No functional changes intended. Do note that there are still places in the code where this function should probably be called, but this PR is very careful not to change any behavior, even if the current behavior is probably wrong. In preparation for storing pose bone selection state on the `bPoseChannel` Related to #138482 Pull Request: https://projects.blender.org/blender/blender/pulls/139496 --- source/blender/animrig/ANIM_armature.hh | 19 +++++++++++++++++++ source/blender/blenkernel/BKE_armature.hh | 3 --- source/blender/blenkernel/intern/action.cc | 8 ++------ source/blender/blenkernel/intern/armature.cc | 4 +++- .../blenkernel/intern/armature_selection.cc | 2 +- .../editors/animation/anim_asset_ops.cc | 8 ++------ .../blender/editors/animation/keyframing.cc | 9 ++------- .../blender/editors/armature/armature_add.cc | 16 ++++------------ .../blender/editors/armature/armature_edit.cc | 15 ++++++--------- .../editors/armature/armature_naming.cc | 14 ++++++-------- .../editors/armature/armature_relations.cc | 4 +--- source/blender/editors/armature/pose_lib_2.cc | 2 +- .../blender/editors/armature/pose_select.cc | 8 +++++--- .../editors/armature/pose_transform.cc | 4 +--- 14 files changed, 53 insertions(+), 63 deletions(-) diff --git a/source/blender/animrig/ANIM_armature.hh b/source/blender/animrig/ANIM_armature.hh index e049ba90843..931926f43ac 100644 --- a/source/blender/animrig/ANIM_armature.hh +++ b/source/blender/animrig/ANIM_armature.hh @@ -36,4 +36,23 @@ inline bool bone_is_visible_editbone(const bArmature *armature, const EditBone * return bone_itself_visible && ANIM_bonecoll_is_visible_editbone(armature, ebone); } +/** + * Returns true if the bone is selected. This includes a visibility check + * because invisible bones cannot be selected, no matter their flag. + */ +inline bool bone_is_selected(const bArmature *armature, const Bone *bone) +{ + return (bone->flag & BONE_SELECTED) && bone_is_visible(armature, bone); +} + +inline bool bone_is_selected(const bArmature *armature, const bPoseChannel *pchan) +{ + return (pchan->bone->flag & BONE_SELECTED) && bone_is_visible_pchan(armature, pchan); +} + +inline bool bone_is_selected(const bArmature *armature, const EditBone *ebone) +{ + return (ebone->flag & BONE_SELECTED) && bone_is_visible_editbone(armature, ebone); +} + } // namespace blender::animrig diff --git a/source/blender/blenkernel/BKE_armature.hh b/source/blender/blenkernel/BKE_armature.hh index b3655c12067..d2cfe707d06 100644 --- a/source/blender/blenkernel/BKE_armature.hh +++ b/source/blender/blenkernel/BKE_armature.hh @@ -571,9 +571,6 @@ void BKE_pchan_bbone_deform_segment_index(const bPoseChannel *pchan, #define PBONE_SELECTABLE(arm, bone) \ (blender::animrig::bone_is_visible(arm, bone) && !((bone)->flag & BONE_UNSELECTABLE)) -#define PBONE_SELECTED(arm, bone) \ - (((bone)->flag & BONE_SELECTED) & blender::animrig::bone_is_visible(arm, bone)) - /* context.selected_pose_bones */ #define FOREACH_PCHAN_SELECTED_IN_OBJECT_BEGIN(_ob, _pchan) \ for (bPoseChannel *_pchan = (bPoseChannel *)(_ob)->pose->chanbase.first; _pchan; \ diff --git a/source/blender/blenkernel/intern/action.cc b/source/blender/blenkernel/intern/action.cc index 085d39af345..0ba7c82602f 100644 --- a/source/blender/blenkernel/intern/action.cc +++ b/source/blender/blenkernel/intern/action.cc @@ -1227,17 +1227,13 @@ bPoseChannel *BKE_pose_channel_active_or_first_selected(Object *ob) } bPoseChannel *pchan = BKE_pose_channel_active_if_bonecoll_visible(ob); - if (pchan && (pchan->bone->flag & BONE_SELECTED) && - blender::animrig::bone_is_visible_pchan(arm, pchan)) - { + if (pchan && blender::animrig::bone_is_selected(arm, pchan)) { return pchan; } LISTBASE_FOREACH (bPoseChannel *, pchan, &ob->pose->chanbase) { if (pchan->bone != nullptr) { - if ((pchan->bone->flag & BONE_SELECTED) && - blender::animrig::bone_is_visible_pchan(arm, pchan)) - { + if (blender::animrig::bone_is_selected(arm, pchan)) { return pchan; } } diff --git a/source/blender/blenkernel/intern/armature.cc b/source/blender/blenkernel/intern/armature.cc index 2ee9a5abb45..1baec4c8d62 100644 --- a/source/blender/blenkernel/intern/armature.cc +++ b/source/blender/blenkernel/intern/armature.cc @@ -3215,10 +3215,12 @@ std::optional> BKE_pose_minmax(const Object *ob if (!pchan->bone) { continue; } + /* Despite `bone_is_selected` also checking for visibility we need to check visbility + * manually due to `use_select` potentially ignoring selection state.*/ if (!blender::animrig::bone_is_visible_pchan(arm, pchan)) { continue; } - if (use_select && !(pchan->bone->flag & BONE_SELECTED)) { + if (use_select && !blender::animrig::bone_is_selected(arm, pchan)) { continue; } diff --git a/source/blender/blenkernel/intern/armature_selection.cc b/source/blender/blenkernel/intern/armature_selection.cc index 0a28c2f488e..f613e455833 100644 --- a/source/blender/blenkernel/intern/armature_selection.cc +++ b/source/blender/blenkernel/intern/armature_selection.cc @@ -23,7 +23,7 @@ void find_selected_bones__visit_bone(const bArmature *armature, SelectedBonesResult &result, Bone *bone) { - const bool is_selected = PBONE_SELECTED(armature, bone); + const bool is_selected = blender::animrig::bone_is_selected(armature, bone); result.all_bones_selected &= is_selected; result.no_bones_selected &= !is_selected; diff --git a/source/blender/editors/animation/anim_asset_ops.cc b/source/blender/editors/animation/anim_asset_ops.cc index 418d8cd5397..072f878d94f 100644 --- a/source/blender/editors/animation/anim_asset_ops.cc +++ b/source/blender/editors/animation/anim_asset_ops.cc @@ -136,9 +136,7 @@ static blender::animrig::Action &extract_pose(Main &bmain, } LISTBASE_FOREACH (bPoseChannel *, pose_bone, &pose_object->pose->chanbase) { - if (!(pose_bone->bone->flag & BONE_SELECTED) || - !blender::animrig::bone_is_visible(armature, pose_bone->bone)) - { + if (!blender::animrig::bone_is_selected(armature, pose_bone)) { continue; } PointerRNA bone_pointer = RNA_pointer_create_discrete( @@ -516,9 +514,7 @@ static Vector generate_path_values(Object &pose_object) Vector path_values; const bArmature *armature = static_cast(pose_object.data); LISTBASE_FOREACH (bPoseChannel *, pose_bone, &pose_object.pose->chanbase) { - if (!(pose_bone->bone->flag & BONE_SELECTED) || - !blender::animrig::bone_is_visible(armature, pose_bone->bone)) - { + if (!blender::animrig::bone_is_selected(armature, pose_bone)) { continue; } PointerRNA bone_pointer = RNA_pointer_create_discrete( diff --git a/source/blender/editors/animation/keyframing.cc b/source/blender/editors/animation/keyframing.cc index 5bdb021c50b..ad690e04ab0 100644 --- a/source/blender/editors/animation/keyframing.cc +++ b/source/blender/editors/animation/keyframing.cc @@ -843,13 +843,8 @@ static bool can_delete_key(FCurve *fcu, Object *ob, ReportList *reports) if ((pchan) && (pchan->bone)) { bArmature *arm = static_cast(ob->data); - /* Invisible bones should not be modified. */ - if (!blender::animrig::bone_is_visible_pchan(arm, pchan)) { - return false; - } - - /* selection flag... */ - if ((pchan->bone->flag & BONE_SELECTED) == 0) { + /* Only selected bones should be affected. */ + if (!blender::animrig::bone_is_selected(arm, pchan)) { return false; } } diff --git a/source/blender/editors/armature/armature_add.cc b/source/blender/editors/armature/armature_add.cc index 60d73165bd6..58fc590ba90 100644 --- a/source/blender/editors/armature/armature_add.cc +++ b/source/blender/editors/armature/armature_add.cc @@ -1134,9 +1134,7 @@ static wmOperatorStatus armature_duplicate_selected_exec(bContext *C, wmOperator /* Select mirrored bones */ if (arm->flag & ARM_MIRROR_EDIT) { LISTBASE_FOREACH (EditBone *, ebone_iter, arm->edbo) { - if (blender::animrig::bone_is_visible_editbone(arm, ebone_iter) && - (ebone_iter->flag & BONE_SELECTED)) - { + if (blender::animrig::bone_is_selected(arm, ebone_iter)) { EditBone *ebone; ebone = ED_armature_ebone_get_mirrored(arm->edbo, ebone_iter); @@ -1152,9 +1150,7 @@ static wmOperatorStatus armature_duplicate_selected_exec(bContext *C, wmOperator ebone_iter && ebone_iter != ebone_first_dupe; ebone_iter = ebone_iter->next) { - if (blender::animrig::bone_is_visible_editbone(arm, ebone_iter) && - (ebone_iter->flag & BONE_SELECTED)) - { + if (blender::animrig::bone_is_selected(arm, ebone_iter)) { EditBone *ebone; char new_bone_name_buff[MAXBONENAME]; const char *new_bone_name = ebone_iter->name; @@ -1183,9 +1179,7 @@ static wmOperatorStatus armature_duplicate_selected_exec(bContext *C, wmOperator ebone_iter && ebone_iter != ebone_first_dupe; ebone_iter = ebone_iter->next) { - if (blender::animrig::bone_is_visible_editbone(arm, ebone_iter) && - (ebone_iter->flag & BONE_SELECTED)) - { + if (blender::animrig::bone_is_selected(arm, ebone_iter)) { EditBone *ebone = ebone_iter->temp.ebone; if (!ebone_iter->parent) { @@ -1384,9 +1378,7 @@ static wmOperatorStatus armature_symmetrize_exec(bContext *C, wmOperator *op) ebone_iter && ebone_iter != ebone_first_dupe; ebone_iter = ebone_iter->next) { - if (blender::animrig::bone_is_visible_editbone(arm, ebone_iter) && - (ebone_iter->flag & BONE_SELECTED)) - { + if (blender::animrig::bone_is_selected(arm, ebone_iter)) { if (ebone_iter->temp.ebone != nullptr) { /* This will be set if the mirror bone already exists (no need to make a new one) * but we do need to make sure that the 'pchan' settings (constraints etc) diff --git a/source/blender/editors/armature/armature_edit.cc b/source/blender/editors/armature/armature_edit.cc index 6c5b66f3059..40e31ccaa77 100644 --- a/source/blender/editors/armature/armature_edit.cc +++ b/source/blender/editors/armature/armature_edit.cc @@ -1231,8 +1231,7 @@ static bool armature_delete_ebone_cb(const char *bone_name, void *arm_p) EditBone *ebone; ebone = ED_armature_ebone_find_name(arm->edbo, bone_name); - return (ebone && (ebone->flag & BONE_SELECTED) && - blender::animrig::bone_is_visible_editbone(arm, ebone)); + return (ebone && blender::animrig::bone_is_selected(arm, ebone)); } /* previously delete_armature */ @@ -1261,14 +1260,12 @@ static wmOperatorStatus armature_delete_selected_exec(bContext *C, wmOperator * for (curBone = static_cast(arm->edbo->first); curBone; curBone = ebone_next) { ebone_next = curBone->next; - if (blender::animrig::bone_is_visible_editbone(arm, curBone)) { - if (curBone->flag & BONE_SELECTED) { - if (curBone == arm->act_edbone) { - arm->act_edbone = nullptr; - } - ED_armature_ebone_remove(arm, curBone); - changed = true; + if (blender::animrig::bone_is_selected(arm, curBone)) { + if (curBone == arm->act_edbone) { + arm->act_edbone = nullptr; } + ED_armature_ebone_remove(arm, curBone); + changed = true; } } diff --git a/source/blender/editors/armature/armature_naming.cc b/source/blender/editors/armature/armature_naming.cc index 2364ed8c050..ec8fc752cde 100644 --- a/source/blender/editors/armature/armature_naming.cc +++ b/source/blender/editors/armature/armature_naming.cc @@ -451,15 +451,13 @@ static wmOperatorStatus armature_flip_names_exec(bContext *C, wmOperator *op) ListBase bones_names = {nullptr}; LISTBASE_FOREACH (EditBone *, ebone, arm->edbo) { - if (blender::animrig::bone_is_visible_editbone(arm, ebone)) { - if (ebone->flag & BONE_SELECTED) { - BLI_addtail(&bones_names, BLI_genericNodeN(ebone->name)); + if (blender::animrig::bone_is_selected(arm, ebone)) { + BLI_addtail(&bones_names, BLI_genericNodeN(ebone->name)); - if (arm->flag & ARM_MIRROR_EDIT) { - EditBone *flipbone = ED_armature_ebone_get_mirrored(arm->edbo, ebone); - if ((flipbone) && !(flipbone->flag & BONE_SELECTED)) { - BLI_addtail(&bones_names, BLI_genericNodeN(flipbone->name)); - } + if (arm->flag & ARM_MIRROR_EDIT) { + EditBone *flipbone = ED_armature_ebone_get_mirrored(arm->edbo, ebone); + if ((flipbone) && !(flipbone->flag & BONE_SELECTED)) { + BLI_addtail(&bones_names, BLI_genericNodeN(flipbone->name)); } } } diff --git a/source/blender/editors/armature/armature_relations.cc b/source/blender/editors/armature/armature_relations.cc index 7e338042a70..81d6210a184 100644 --- a/source/blender/editors/armature/armature_relations.cc +++ b/source/blender/editors/armature/armature_relations.cc @@ -651,9 +651,7 @@ static void separate_armature_bones(Main *bmain, Object *ob, const bool is_selec curbone = ED_armature_ebone_find_name(arm->edbo, pchan->name); /* check if bone needs to be removed */ - if (is_select == (blender::animrig::bone_is_visible_editbone(arm, curbone) && - (curbone->flag & BONE_SELECTED))) - { + if (is_select == blender::animrig::bone_is_selected(arm, curbone)) { /* Clear the bone->parent var of any bone that had this as its parent. */ LISTBASE_FOREACH (EditBone *, ebo, arm->edbo) { diff --git a/source/blender/editors/armature/pose_lib_2.cc b/source/blender/editors/armature/pose_lib_2.cc index ed5c44da7eb..f3f56eac219 100644 --- a/source/blender/editors/armature/pose_lib_2.cc +++ b/source/blender/editors/armature/pose_lib_2.cc @@ -151,7 +151,7 @@ static void poselib_keytag_pose(bContext *C, Scene *scene, PoseBlendData *pbd) return; } if (BKE_pose_backup_is_selection_relevant(pbd->pose_backup) && - !PBONE_SELECTED(armature, pchan->bone)) + !blender::animrig::bone_is_selected(armature, pchan)) { return; } diff --git a/source/blender/editors/armature/pose_select.cc b/source/blender/editors/armature/pose_select.cc index dbf4693a563..87e169a19ad 100644 --- a/source/blender/editors/armature/pose_select.cc +++ b/source/blender/editors/armature/pose_select.cc @@ -950,7 +950,7 @@ static blender::Set get_selected_pose_bones(Object *pose_object) blender::Set selected_pose_bones; bArmature *arm = static_cast((pose_object) ? pose_object->data : nullptr); LISTBASE_FOREACH (bPoseChannel *, pchan, &pose_object->pose->chanbase) { - if (PBONE_SELECTED(arm, pchan->bone)) { + if (blender::animrig::bone_is_selected(arm, pchan)) { selected_pose_bones.add(pchan); } } @@ -1060,7 +1060,7 @@ static bool pose_select_siblings(bContext *C, const bool extend) BLI_assert(arm); blender::Set parents_of_selected; LISTBASE_FOREACH (bPoseChannel *, pchan, &pose_object->pose->chanbase) { - if (PBONE_SELECTED(arm, pchan->bone)) { + if (blender::animrig::bone_is_selected(arm, pchan)) { parents_of_selected.add(pchan->parent); } } @@ -1073,7 +1073,9 @@ static bool pose_select_siblings(bContext *C, const bool extend) } /* Checking if the bone is already selected so `changed_any_selection` stays true to its * word. */ - if (parents_of_selected.contains(pchan->parent) && !PBONE_SELECTED(arm, pchan->bone)) { + if (parents_of_selected.contains(pchan->parent) && + !blender::animrig::bone_is_selected(arm, pchan)) + { pose_do_bone_select(pchan, SEL_SELECT); changed_any_selection = true; } diff --git a/source/blender/editors/armature/pose_transform.cc b/source/blender/editors/armature/pose_transform.cc index 6d5c05bcb53..f64fe1aa76a 100644 --- a/source/blender/editors/armature/pose_transform.cc +++ b/source/blender/editors/armature/pose_transform.cc @@ -537,9 +537,7 @@ static wmOperatorStatus pose_visual_transform_apply_exec(bContext *C, wmOperator int i; LISTBASE_FOREACH_INDEX (bPoseChannel *, pchan, &ob->pose->chanbase, i) { - if (!((pchan->bone->flag & BONE_SELECTED) && - blender::animrig::bone_is_visible_pchan(arm, pchan))) - { + if (!blender::animrig::bone_is_selected(arm, pchan)) { pchan_xform_array[i].is_set = false; continue; }