Fix #114981 : Add box, circle and lasso selection tools to weight paint + bone select mode #115807

Daiki Hashimoto · 2023-12-05T16:04:38+01:00

Daiki Hashimoto commented

2023-12-05 16:04:38 +01:00

Issue

Currently, bone select mode in weight paint mode (added/made explicit in #115409) has a tweak tool, but lacks box, circle and lasso select tools, lacking consistency with the other two modes (face and vertex select modes) in Weight paint mode. This results in a lack of useful methods for selecting multiple bones in weight paint mode, which was mentioned as necessary in issue #114981 .

Note : This issue was mentioned in the Future Improvements section of PR #115409, and will address Issue #114981 together with PR #115409 which is already merged.

Solution

By adding a box, circle and lasso select tools with the following behavior, all selection tools can be added while respecting the current behavior of selecting bones using alt-click and LCS and right-click in RCS (referred to as click behavior):

Keep most of the behavior of selection tools in pose mode
When using the selection tools, cursor selecting in an empty area will not deselect bones (aligning with the current click behavior), while region selecting will (new behavior)
EDIT : When activating region select from hotkeys such as B for box select and C for circle select, region selecting in an empty area will not deselect bones (aligning with the behavior in Pose mode. This is different from the behavior above, but these two behaviors are consistent with the rest of Blender.)
In the case where multiple armatures were selected when going into weight paint mode, you cannot multi-select bones across armatures. (You can only multi-select bones on the current active armature. You can however switch the pose mode armature using cursor select. This aligns with the current additive click behavior.)

Note: this PR will also enable the B and C hotkeys for box select and circle select as the keymap from 3D View will fall through to weight paint mode.

Examples

Cursor picking with box and lasso tool will not deselect bones, but region picking will. (Clicking here will not deselect the bone, but box selecting from here to the right will. Same goes for the lasso tool.)

When an object has multiple armatures deforming it and both armatures were selected when going into weight paint mode, box selecting all the bones will only select the bones from the armature with the active bone. It is still possible to select the bone in the other armature by cursor picking.

(before box select)

(after box select)

(after clicking on a bone in the other armature)

Key areas for review

Behavior of region selecting an empty space

The ability to deselect all bones by region selecting an empty area felt reasonable, and might be useful in situations where selecting multiple bones in weight paint mode is useful. (To run a certain operation based on the selection of bones?) However, this does not align with the current behavior of alt-click bone select where clicking in an empty space will not deselect any bones. If aligning with this behavior feels more natural, then maybe region selecting an empty space should not deselect all bones.

Files

Example file used in screen shots: pr_115807_single_armature.blend, pr_115807_multiple_armatures.blend

Commit message for the squashed commit

Currently, bone select mode in weight paint mode has a tweak tool, but lacks box, circle and lasso select tools, lacking consistency with the other two modes (face and vertex select modes) in Weight paint mode. This results in a lack of useful methods for selecting multiple bones in weight paint mode.

By adding a box, circle and lasso select tools with the following behavior, all selection tools can be added while respecting the current behavior of selecting bones using alt-click and LCS and right-click in RCS (referred to as click behavior):

Keep most of the behavior of selection tools in pose mode
When using the selection tools, cursor selecting in an empty area will not deselect bones (aligning with the current click behavior), while region selecting wil.
When activating region select from hotkeys such as B for box select and C for circle select, region selecting in an empty area will not deselect bones (aligning with the behavior in Pose mode. This is different from the behavior above, but these two behaviors are consistent with the rest of Blender.)
In the case where multiple armatures were selected when going into weight paint mode, you cannot multi-select bones across armatures. (You can only multi-select bones on the current active armature. You can however switch the pose mode armature using cursor select. This aligns with the current additive click behavior.)

# Issue Currently, bone select mode in weight paint mode (added/made explicit in #115409) has a tweak tool, but lacks box, circle and lasso select tools, lacking consistency with the other two modes (face and vertex select modes) in Weight paint mode. This results in a lack of useful methods for selecting multiple bones in weight paint mode, which was mentioned as necessary in issue #114981 . **Note :** This issue was mentioned in the Future Improvements section of PR #115409, and will address Issue #114981 together with PR #115409 which is already merged. # Solution By adding a box, circle and lasso select tools with the following behavior, all selection tools can be added while respecting the current behavior of selecting bones using alt-click and LCS and right-click in RCS (referred to as click behavior): - Keep most of the behavior of selection tools in pose mode - When using the selection tools, cursor selecting in an empty area will not deselect bones (aligning with the current click behavior), while region selecting will (new behavior) - **EDIT** : When activating region select from hotkeys such as B for box select and C for circle select, region selecting in an empty area will not deselect bones (aligning with the behavior in Pose mode. This is different from the behavior above, but these two behaviors are consistent with the rest of Blender.) - In the case where multiple armatures were selected when going into weight paint mode, you cannot multi-select bones across armatures. (You can only multi-select bones on the current active armature. You can however switch the pose mode armature using cursor select. This aligns with the current additive click behavior.) **Note:** this PR will also enable the B and C hotkeys for box select and circle select as the keymap from 3D View will fall through to weight paint mode. **Examples** Cursor picking with box and lasso tool will not deselect bones, but region picking will. (Clicking here will not deselect the bone, but box selecting from here to the right will. Same goes for the lasso tool.) ![image](/attachments/241ee3d1-ed75-43ce-b6d2-3cedb50eb34b) When an object has multiple armatures deforming it and both armatures were selected when going into weight paint mode, box selecting all the bones will only select the bones from the armature with the active bone. It is still possible to select the bone in the other armature by cursor picking. ![image](/attachments/57ae411c-b8bc-4efb-a634-43cdc6d91b90) (before box select) ![image](/attachments/05b24ce1-a77a-41ba-88b5-23898acf79af) (after box select) ![image](/attachments/dc698883-602a-4a58-8893-086d56271b3f) (after clicking on a bone in the other armature) # Key areas for review ## Behavior of region selecting an empty space The ability to deselect all bones by region selecting an empty area felt reasonable, and might be useful in situations where selecting multiple bones in weight paint mode is useful. (To run a certain operation based on the selection of bones?) However, this does not align with the current behavior of alt-click bone select where clicking in an empty space will not deselect any bones. If aligning with this behavior feels more natural, then maybe region selecting an empty space should not deselect all bones. # Files Example file used in screen shots: [pr_115807_single_armature.blend](/attachments/73513382-6c02-4432-91fa-eab2b2030bb3), [pr_115807_multiple_armatures.blend](/attachments/a4aec492-5017-4372-8d53-2d30b19ff2be) # Commit message for the squashed commit Currently, bone select mode in weight paint mode has a tweak tool, but lacks box, circle and lasso select tools, lacking consistency with the other two modes (face and vertex select modes) in Weight paint mode. This results in a lack of useful methods for selecting multiple bones in weight paint mode. By adding a box, circle and lasso select tools with the following behavior, all selection tools can be added while respecting the current behavior of selecting bones using alt-click and LCS and right-click in RCS (referred to as click behavior): - Keep most of the behavior of selection tools in pose mode - When using the selection tools, cursor selecting in an empty area will not deselect bones (aligning with the current click behavior), while region selecting wil. - When activating region select from hotkeys such as B for box select and C for circle select, region selecting in an empty area will not deselect bones (aligning with the behavior in Pose mode. This is different from the behavior above, but these two behaviors are consistent with the rest of Blender.) - In the case where multiple armatures were selected when going into weight paint mode, you cannot multi-select bones across armatures. (You can only multi-select bones on the current active armature. You can however switch the pose mode armature using cursor select. This aligns with the current additive click behavior.)

pr_115807_single_armature.blend

999 KiB

pr_115807_multiple_armatures.blend

1019 KiB

image.png

736 KiB

image.png

734 KiB

image.png

693 KiB

image.png

735 KiB

image.png

734 KiB

image.png

735 KiB

Daiki Hashimoto added 5 commits 2023-12-05 16:04:47 +01:00

9a14bc7c6b In weight paint mode, add box, circle and lasso select tools to bone select mode

fb7a32b4a9 Add function to test if the given object is in weight paint mode AND bone select mode

fa0ce9f374 Handle select operators in bone select mode if pose mode armature exists

4702be37eb Pick bones using box, circle and lasso select

c6dfd6c199 Stay in bone select mode while using select tools when there are multiple armature modifiers

Iliya Katushenock added this to the Sculpt, Paint & Texture project 2023-12-05 16:07:21 +01:00

Daiki Hashimoto changed title from ~~WIP: Add box, circle and lasso selection tools to weight paint + bone select mode~~ to Fix #114981 : Add box, circle and lasso selection tools to weight paint + bone select mode

2023-12-06 13:47:46 +01:00

Christoph Lendenfeld requested review from Christoph Lendenfeld 2023-12-07 17:06:01 +01:00

Christoph Lendenfeld added the

Module

Animation & Rigging

label 2023-12-07 17:06:07 +01:00

Christoph Lendenfeld requested changes 2023-12-08 10:56:00 +01:00

Christoph Lendenfeld left a comment

minor comment style changes.

The bigger thing is the behavior when clicking in an empty space. It feels weird to me, and it is inconsistent with the behavior in object mode. But I feel like it's not up to me to decide.

Great job pointing that out in the PR description btw. And also good job testing the edge case of multiple armatures.

@Mets need you to have an eye on this. What should happen when clicking in an empty space with e.g. the box select tool.

minor comment style changes. The bigger thing is the behavior when clicking in an empty space. It feels weird to me, and it is inconsistent with the behavior in object mode. But I feel like it's not up to me to decide. Great job pointing that out in the PR description btw. And also good job testing the edge case of multiple armatures. @Mets need you to have an eye on this. What should happen when clicking in an empty space with e.g. the box select tool.

source/blender/blenkernel/BKE_paint.hh Outdated

						
				@ -208,2 +208,4 @@

				 */

				bool BKE_paint_select_vert_test(const Object *ob);

				// /**

				//  * Return true when in weight paint + bone-select mode?

Christoph Lendenfeld commented

2023-12-08 10:30:49 +01:00

The blender comment style is without the double slash (//) at the start

Also I feel like the comment should not be a question. I know this is the same as the functions above,
but it's confusing to read.
I assume its just a statement. As in it returns true when in weight paint + bone-select mode.

The blender comment style is without the double slash (//) at the start Also I feel like the comment should not be a question. I know this is the same as the functions above, but it's confusing to read. I assume its just a statement. As in it returns true when in weight paint + bone-select mode.

👍 1

Daiki Hashimoto commented

2023-12-08 14:26:18 +01:00

I applied the same change in 09a3f4b9b1 to the two functions above (change the question mark to a period) since I encountered them many times when coding for this PR, and were used as a statement to check for their respective modes.

I applied the same change in 09a3f4b9b1 to the two functions above (change the question mark to a period) since I encountered them many times when coding for this PR, and were used as a statement to check for their respective modes.

daisea3e1203 marked this conversation as resolved

source/blender/editors/space_view3d/view3d_select.cc

						
				@ -2914,0 +2919,4 @@

				      else if (BKE_paint_select_bone_test(vc.obact)) {

				        /* pass  */

				        /* During weight paint mode, when there are multiple armatures deforming a single mesh,

Christoph Lendenfeld commented

2023-12-08 10:45:45 +01:00

this comment is great. I love it

❤️ 1

daisea3e1203 marked this conversation as resolved

source/blender/editors/space_view3d/view3d_select.cc Outdated

						
				@ -5307,6 +5320,21 @@ static int view3d_circle_select_exec(bContext *C, wmOperator *op)

				    }

				    FOREACH_OBJECT_IN_MODE_END;

				  }

				  /* Select bones from pose mode armature in weight paint + bone-select mode */

Christoph Lendenfeld commented

2023-12-08 10:45:26 +01:00

comments end with a dot

👍 1

Daiki Hashimoto commented

2023-12-08 14:28:09 +01:00

Addressed in 05bb4f72e0
Thank you for spotting these mistakes! Will be careful with comments in future PRs 🙏

Addressed in 05bb4f72e0 Thank you for spotting these mistakes! Will be careful with comments in future PRs 🙏

daisea3e1203 marked this conversation as resolved

Demeter Dzadik commented

2023-12-08 11:51:20 +01:00

Thanks for the patch!

When an object has multiple armatures deforming it

To be clear, this is practically unsupported in Blender as it is, so the fact that it doesn't work perfectly is not the biggest issue imo.
However, if it did work, that would make life easier in a lovely but unlikely to be near future, where Armature deformation is done via GeoNodes, so the weights can come from as many armature objects as desired and still be normalized. But considering this is not on anybody's roadmap, I don't feel strongly about it.

The ability to deselect all bones by region selecting an empty area felt reasonable

I would stay consistent with other areas of Blender here. This sounds a bit esoteric to me.
Deselect All is Alt+A throughout Blender. Meanwhile, box/circle/lasso select is an additive selection throughout Blender. Having it clear the selection when there's nothing in the selected region feels like it will just result in confusion, so I wouldn't do it tbh.

Thanks for the patch! > When an object has multiple armatures deforming it To be clear, this is practically unsupported in Blender as it is, so the fact that it doesn't work perfectly is not the biggest issue imo. However, if it did work, that would make life easier in a lovely but unlikely to be near future, where Armature deformation is done via GeoNodes, so the weights can come from as many armature objects as desired and still be normalized. But considering this is not on anybody's roadmap, I don't feel strongly about it. > The ability to deselect all bones by region selecting an empty area felt reasonable I would stay consistent with other areas of Blender here. This sounds a bit esoteric to me. Deselect All is Alt+A throughout Blender. Meanwhile, box/circle/lasso select is an additive selection throughout Blender. Having it clear the selection when there's nothing in the selected region feels like it will just result in confusion, so I wouldn't do it tbh.

👍 1

Christoph Lendenfeld commented

2023-12-08 12:01:10 +01:00

I would stay consistent with other areas of Blender here. This sounds a bit esoteric to me.
Deselect All is Alt+A throughout Blender. Meanwhile, box/circle/lasso select is an additive selection throughout Blender. Having it clear the selection when there's nothing in the selected region feels like it will just result in confusion, so I wouldn't do it tbh.

this is another area where LCS and RCS are different. LCS definitely deselects by clicking an empty area. Will need to check RCS

> I would stay consistent with other areas of Blender here. This sounds a bit esoteric to me. > Deselect All is Alt+A throughout Blender. Meanwhile, box/circle/lasso select is an additive selection throughout Blender. Having it clear the selection when there's nothing in the selected region feels like it will just result in confusion, so I wouldn't do it tbh. this is another area where LCS and RCS are different. LCS definitely deselects by clicking an empty area. Will need to check RCS

Daiki Hashimoto commented

2023-12-08 12:20:00 +01:00

Thank you for the feedback! Will check the RCS behavior and append to the PR description.

❤️ 1

Demeter Dzadik commented

2023-12-08 12:35:29 +01:00

Oh wow, nevermind, I think it's not a left/right-click select thing, but a tool vs hotkey thing. Selection Tools, which I never used, behave differently from B/C/Ctrl+Click hotkeys... Wow. I learned something. Well, I think that's odd, but that ain't this patch's fault. But still, let's try to be consistent, and if this is consistent with the rest of Blender as it is, then so be it. 🙃

That said, I wish those hotkeys were available in weight paint mode, at least B and C. But I suppose that is a separate issue.

Oh wow, nevermind, I think it's not a left/right-click select thing, but a tool vs hotkey thing. Selection Tools, which I never used, behave differently from B/C/Ctrl+Click hotkeys... Wow. I learned something. Well, I think that's odd, but that ain't this patch's fault. But still, let's try to be consistent, and if this is consistent with the rest of Blender as it is, then so be it. 🙃 That said, I wish those hotkeys were available in weight paint mode, at least B and C. But I suppose that is a separate issue.

Daiki Hashimoto commented

2023-12-08 13:40:23 +01:00

I forgot to mention in the description (fixed), but B and C hotkeys will be available with this patch! (the respective keymap from 3d View will be applied automatically now that the selection tools are available in weight paint mode.)
Will check if the behavior of selection tools activated from hotkeys align with the behavior you mentioned 👍

I forgot to mention in the description (fixed), but B and C hotkeys will be available with this patch! (the respective keymap from 3d View will be applied automatically now that the selection tools are available in weight paint mode.) Will check if the behavior of selection tools activated from hotkeys align with the behavior you mentioned 👍

❤️ 1

Daiki Hashimoto added 2 commits 2023-12-08 14:20:19 +01:00

09a3f4b9b1 Clean comments: remove confusing question mark at end of comment.

05bb4f72e0 Clean comments: add period at end of comment sentence.

Daiki Hashimoto commented

2023-12-08 14:58:12 +01:00

@Mets I checked the behavior of B and C hotkeys in weight paint mode, and they aligned with what you mentioned 👍 (region selecting with hotkeys doesn't deselect bones while region selecting with selection tools does. This behavior is appended to the PR description.)

@ChrisLend Thank you for spotting the comment style mistakes! They have been addressed in the last two commits. The review from @Mets should also be resolved with the check on hotkey behaviors above!

@Mets I checked the behavior of B and C hotkeys in weight paint mode, and they aligned with what you mentioned 👍 (region selecting with hotkeys doesn't deselect bones while region selecting with selection tools does. This behavior is appended to the PR description.) @ChrisLend Thank you for spotting the comment style mistakes! They have been addressed in the last two commits. The review from @Mets should also be resolved with the check on hotkey behaviors above!

Christoph Lendenfeld commented

2023-12-08 15:52:30 +01:00

Since if the feedback from @Mets is to make it consistent with the rest of blender, I am comparing weigth paint with pose mode here.

RCS

-	Pose Mode	Weight Paint
Right click in empty space	deselects	nothing happens
box select in empty space	deselects	deselects
box select bones	only bones in box are selected	only bones in box are selected
using C or B to select	additive	additive

LCS

-	Pose Mode	Weight Paint
Left click in empty space	deselects	nothing happens
box select in empty space	deselects	deselects
box select bones	only bones in box are selected	only bones in box are selected
using C or B to select	additive	additive

so the only difference I could find was the deselection behavior which is inconsistent and I think should change

❤️ 1

Daiki Hashimoto commented

2023-12-11 09:03:49 +01:00

@ChrisLend
Do you think it would be clearer to create a separate PR for changing the behavior of clicking in an empty space during weight paint mode (row1 for LCS and RCS), and proceed with this PR as is?
The reason is that while the behaviors for rows 2-4 is related to the new box/circle select tools, changing row 1's behavior involves changing the existing behavior of not deselecting bones when right-clicking in an empty space during weight paint mode in RCS.

@ChrisLend Do you think it would be clearer to create a separate PR for changing the behavior of clicking in an empty space during weight paint mode (row1 for LCS and RCS), and proceed with this PR as is? The reason is that while the behaviors for rows 2-4 is related to the new box/circle select tools, changing row 1's behavior involves changing the existing behavior of not deselecting bones when right-clicking in an empty space during weight paint mode in RCS.

Christoph Lendenfeld commented

2023-12-12 09:24:01 +01:00

The reason is that while the behaviors for rows 2-4 is related to the new box/circle select tools, changing row 1's behavior involves changing the existing behavior of not deselecting bones when right-clicking in an empty space during weight paint mode in RCS.

@Mets your opinion on this? I don't mind having that as a separate PR if the behavior is already like that

> The reason is that while the behaviors for rows 2-4 is related to the new box/circle select tools, changing row 1's behavior involves changing the existing behavior of not deselecting bones when right-clicking in an empty space during weight paint mode in RCS. @Mets your opinion on this? I don't mind having that as a separate PR if the behavior is already like that

Demeter Dzadik commented

2023-12-12 11:54:47 +01:00

Ah, so Blender is already inconsistent as it is, and right-clicking in an empty space in weight paint mode won't deselect bones like it does in pose mode. Odd indeed.

I'd say that's a separate topic though, and I wouldn't hold back this PR because of it.

Ah, so Blender is already inconsistent as it is, and right-clicking in an empty space in weight paint mode won't deselect bones like it does in pose mode. Odd indeed. I'd say that's a separate topic though, and I wouldn't hold back this PR because of it.

Christoph Lendenfeld approved these changes 2023-12-12 12:56:10 +01:00

Christoph Lendenfeld left a comment

Just checked and it is consistent with the face/vertex mask selection behavior.
Thanks for pointing that out. In that case it looks good to land

Just checked and it is consistent with the face/vertex mask selection behavior. Thanks for pointing that out. In that case it looks good to land

Nathan Vegdahl commented

2023-12-12 13:13:49 +01:00

I don't know if I'm misunderstanding how these additions are supposed to work, but box/circle/lasso selecting isn't actually selecting bones in my testing. I can activate the tools just fine, and e.g. drag a box out. It's just that it doesn't actually affect the bone selection. Simply clicking does still select the bones, however.

I don't know if I'm misunderstanding how these additions are supposed to work, but box/circle/lasso selecting isn't actually selecting bones in my testing. I can activate the tools just fine, and e.g. drag a box out. It's just that it doesn't actually affect the bone selection. Simply clicking does still select the bones, however. <video src="/attachments/77138082-3050-409f-b20d-b03ab2e99369" title="box_select_not_working.mp4" controls></video>

box_select_not_working.mp4

201 KiB

Sybren A. Stüvel requested changes 2023-12-12 13:15:39 +01:00

Sybren A. Stüvel left a comment

Just popping in to nag about a small thing.

source/blender/blenkernel/intern/paint.cc

						
				@ -1046,1 +1046,4 @@

				bool BKE_paint_select_bone_test(const Object *ob)

				{

				  return ((ob != nullptr) && (ob->type == OB_MESH) && (ob->data != nullptr) &&

Sybren A. Stüvel commented

2023-12-12 13:05:25 +01:00

This will be clearer when written as separate expressions. I know the current code is consistent with the existing code, but I find the existing code really hard to read too, and I don't think we should duplicate that.

if (ob == nullptr || ob->type != OB_MESH || (ob->mode & OB_MODE_WEIGHT_PAINT) == 0) {
  return false;
}

Mesh *mesh = static_cast<Mesh *>(ob->data);
return mesh->editflag & ~(ME_EDIT_PAINT_FACE_SEL | ME_EDIT_PAINT_VERT_SEL);

This separates the precondition checks from the actual query.

I also think the query is incorrect, as it checks whether any flag is set, ignoring ME_EDIT_PAINT_FACE_SEL and ME_EDIT_PAINT_VERT_SEL. IMO it should check that these flags are not set. I suspect this is the right check, but please do verify

return (mesh->editflag & (ME_EDIT_PAINT_FACE_SEL | ME_EDIT_PAINT_VERT_SEL)) == 0;

Note that I didn't include the check for ob->data != nullptr, as this is implied by ob->type == OB_MESH. If a Mesh object doesn't have any data, there is something very wrong and other areas in the code will have stopped working already. This function is not the place to try and solve this problem.

This will be clearer when written as separate expressions. I know the current code is consistent with the existing code, but I find the existing code really hard to read too, and I don't think we should duplicate that. ```cpp if (ob == nullptr || ob->type != OB_MESH || (ob->mode & OB_MODE_WEIGHT_PAINT) == 0) { return false; } Mesh *mesh = static_cast<Mesh *>(ob->data); return mesh->editflag & ~(ME_EDIT_PAINT_FACE_SEL | ME_EDIT_PAINT_VERT_SEL); ``` This separates the precondition checks from the actual query. I also think the query is incorrect, as it checks whether any flag is set, ignoring `ME_EDIT_PAINT_FACE_SEL` and `ME_EDIT_PAINT_VERT_SEL`. IMO it should check that these flags are *not* set. I suspect this is the right check, but please do verify ```cpp return (mesh->editflag & (ME_EDIT_PAINT_FACE_SEL | ME_EDIT_PAINT_VERT_SEL)) == 0; ``` Note that I didn't include the check for `ob->data != nullptr`, as this is implied by `ob->type == OB_MESH`. If a Mesh object doesn't have any data, there is something very wrong and other areas in the code will have stopped working already. This function is not the place to try and solve this problem.

❤️ 1

Daiki Hashimoto commented

2023-12-12 14:32:13 +01:00

Thank you very much for the review and the insights about mesh data 🙏

The query is incorrect as you have pointed out, and have probably lead to the tools not functioning properly in some situations as @nathanvegdahl has demonstrated below. This has been fixed in e11fce8298 .

I have also refactored the two functions above in a similar fashion in 93a58c7601, but if you think this change should occur in a different PR or occasion, please let me know.

Thank you very much for the review and the insights about mesh data 🙏 The query is incorrect as you have pointed out, and have probably lead to the tools not functioning properly in some situations as @nathanvegdahl has demonstrated below. This has been fixed in e11fce8298 . I have also refactored the two functions above in a similar fashion in 93a58c7601, but if you think this change should occur in a different PR or occasion, please let me know.

Sybren A. Stüvel commented

2023-12-18 16:01:56 +01:00

This has been fixed in e11fce8298 .
👍

if you think this change should occur in a different PR or occasion, please let me know.
Yup, that's for a different PR.

Ideally, such cleanups would happen before this PR (first cleanup, then change functionality). In this case I don't think that would be worth the effort, though.

> This has been fixed in e11fce8298 . :+1: > if you think this change should occur in a different PR or occasion, please let me know. Yup, that's for a different PR. Ideally, such cleanups would happen *before* this PR (first cleanup, then change functionality). In this case I don't think that would be worth the effort, though.

Daiki Hashimoto commented

2023-12-19 15:33:55 +01:00

Thank you very much for the feedback!

Ideally, such cleanups would happen before this PR (first cleanup, then change functionality). In this case I don't think that would be worth the effort, though.

When I attempt to make a large change to the codebase in the future, I will make sure to create a separate clean up PR beforehand.

Thank you very much for the feedback! > Ideally, such cleanups would happen before this PR (first cleanup, then change functionality). In this case I don't think that would be worth the effort, though. When I attempt to make a large change to the codebase in the future, I will make sure to create a separate clean up PR beforehand.

Sybren A. Stüvel commented

2024-02-12 11:21:32 +01:00

Sorry for the back-and-forth, but looking back at this now, I do think that it's better to have this change as a small separate PR that we can land before this one. That way this little fix can be described by itself, and in the hypothetical case that it causes any issues, rolled back by itself, without having to deal with this entire PR.

Christoph Lendenfeld commented

2023-12-12 13:22:35 +01:00

I don't know if I'm misunderstanding how these additions are supposed to work, but box/circle/lasso selecting isn't actually selecting bones in my testing. I can activate the tools just fine, and e.g. drag a box out. It's just that it doesn't actually affect the bone selection. Simply clicking does still select the bones, however.

Can you try with factory preferences or share your file? I just tried on my end which has a similar setup and it's definitely working.

> I don't know if I'm misunderstanding how these additions are supposed to work, but box/circle/lasso selecting isn't actually selecting bones in my testing. I can activate the tools just fine, and e.g. drag a box out. It's just that it doesn't actually affect the bone selection. Simply clicking does still select the bones, however. Can you try with factory preferences or share your file? I just tried on my end which has a similar setup and it's definitely working.

Nathan Vegdahl commented

2023-12-12 13:57:16 +01:00

@ChrisLend Yeah, here's the blend file. (Note: in the video I only moved the bones outside the mesh to verify that it wasn't an occlusion issue.)

Resetting to factory preferences didn't make any difference for me.

@ChrisLend Yeah, here's the blend file. (Note: in the video I only moved the bones outside the mesh to verify that it wasn't an occlusion issue.) Resetting to factory preferences didn't make any difference for me.

weight_paint_bone_box_select.blend

954 KiB

Daiki Hashimoto added 3 commits 2023-12-12 14:23:21 +01:00

4b15675294 Merge branch 'main' into add-select-tools-to-bone-select-mode

e11fce8298 Fix BKE_paint_select_bone_test to be clearer and to query for the correct flags

93a58c7601 Refactor: clarify logic by separating the precondition check from the actual query

Daiki Hashimoto commented

2023-12-12 14:24:51 +01:00

@nathanvegdahl
Thank you so much for pointing this out! This is probably related to the flag check error @dr.sybren has pointed out, and should be fixed in the latest commit.

@nathanvegdahl Thank you so much for pointing this out! This is probably related to the flag check error @dr.sybren has pointed out, and should be fixed in the latest commit.

👍 1

Christoph Lendenfeld commented

2023-12-12 14:27:14 +01:00

@nathanvegdahl indeed I can reproduce the issue with your file, but I cannot see the difference to my file. Can you verify my test file works for you

Edit: didn't see your latest comment. Will check the latest commit
Edit 2: yes with the latest changes, it even works in nathans test file

@nathanvegdahl indeed I can reproduce the issue with your file, but I cannot see the difference to my file. Can you verify my test file works for you Edit: didn't see your latest comment. Will check the latest commit Edit 2: yes with the latest changes, it even works in nathans test file

weight_paint_test_simple.blend

853 KiB

❤️ 1

Daiki Hashimoto added 1 commit 2023-12-13 10:32:23 +01:00

0b0d45cb0c Format : remove redundant parentheses

Nathan Vegdahl commented

2023-12-14 10:07:56 +01:00

The latest change fixes the issue for me as well.

Nathan Vegdahl approved these changes 2023-12-14 10:15:48 +01:00

Nathan Vegdahl left a comment

I've only given the code a cursory code review, since @ChrisLend and @dr.sybren have already looked more closely. But it looks good to me. I spent a bit more time testing, and it now seems to work as expected as well, including when multiple armatures are selected.

❤️ 1

Daiki Hashimoto commented

2023-12-19 15:48:57 +01:00

@ChrisLend
The changes requested by @dr.sybren has been addressed and were reviewed in the conversation thread, so this PR is probably ready to land!

I have also added a "Commit message for the squashed commit" in the PR description since I noticed you had to edit the PR description into a commit message when creating the squashed commit in the last PR. I simply removed references to other issues/PRs, and revised the Issue and Solution section, editing in the information from EDITs and NOTEs.

@ChrisLend The changes requested by @dr.sybren has been addressed and were reviewed in the conversation thread, so this PR is probably ready to land! I have also added a "Commit message for the squashed commit" in the PR description since I noticed you had to edit the PR description into a commit message when creating the squashed commit in the last PR. I simply removed references to other issues/PRs, and revised the Issue and Solution section, editing in the information from EDITs and NOTEs.

Christoph Lendenfeld requested review from Sybren A. Stüvel 2023-12-19 16:18:52 +01:00

Christoph Lendenfeld commented

2023-12-19 16:19:44 +01:00

@daisea3e1203 I've re-requested review from @dr.sybren since he still had not given the green tick. once that is given I can land it.

Thanks for writing a commit message beforehand :)

@daisea3e1203 I've re-requested review from @dr.sybren since he still had not given the green tick. once that is given I can land it. Thanks for writing a commit message beforehand :)

👍 1

Sybren A. Stüvel reviewed 2024-02-12 11:29:19 +01:00

Sybren A. Stüvel left a comment

When an object has multiple armatures deforming it and both armatures were selected when going into weight paint mode, box selecting all the bones will only select the bones from the armature with the active bone. It is still possible to select the bone in the other armature by cursor picking.

What was the reasoning behind this choice?

About the commit message:

Currently, bone select mode in weight paint mode has a tweak tool, but lacks ...

I'd recommend describing what the commit does. Using a word like "currently" can be confusing, as as soon as that commit is made, the message does not describe Blender's current behaviour any more.

By adding a box, circle and lasso select tools with the following behavior, all selection tools can be added ...

By writing can be added, the message implies that they have not been added yet. The PR title (which is used as commit title) tells us that these tools are added by this commit, though. This would be another point of confusion.

I think the commit message can be simpler. The initial paragraph could be something like:

Add box, circle, and lasso selection tools to the bone select mode in Weight Paint mode. This makes the bone select mode consistent with vertex/face selection modes.

After that a more elaborate message can be written with more detailed changes, motivations of choices, etc.

> When an object has multiple armatures deforming it and both armatures were selected when going into weight paint mode, box selecting all the bones will only select the bones from the armature with the active bone. It is still possible to select the bone in the other armature by cursor picking. What was the reasoning behind this choice? About the commit message: > Currently, bone select mode in weight paint mode has a tweak tool, but lacks ... I'd recommend describing what the commit does. Using a word like "currently" can be confusing, as as soon as that commit is made, the message does not describe Blender's current behaviour any more. > By adding a box, circle and lasso select tools with the following behavior, all selection tools can be added ... By writing **can** be added, the message implies that they have not been added yet. The PR title (which is used as commit title) tells us that these tools are added by this commit, though. This would be another point of confusion. I think the commit message can be simpler. The initial paragraph could be something like: > Add box, circle, and lasso selection tools to the bone select mode in Weight Paint mode. This makes the bone select mode consistent with vertex/face selection modes. After that a more elaborate message can be written with more detailed changes, motivations of choices, etc.

Sybren A. Stüvel requested changes 2024-02-13 10:16:51 +01:00

Sybren A. Stüvel left a comment

I'm just clicking "Request Changes" to get this off my review queue.

This pull request has changes conflicting with the target branch.

scripts/startup/bl_ui/space_toolsystem_toolbar.py
source/blender/editors/space_view3d/view3d_select.cc

View command line instructions.

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u add-select-tools-to-bone-select-mode:daisea3e1203-add-select-tools-to-bone-select-mode

git checkout daisea3e1203-add-select-tools-to-bone-select-mode

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Fix #114981 : Add box, circle and lasso selection tools to weight paint + bone select mode #115807

Issue

Solution

Key areas for review

Behavior of region selecting an empty space

Files

Commit message for the squashed commit

RCS

LCS

Checkout