Sculpt: Migrate Box Hide to common gesture code #119040

Sean Kim · 2024-03-04T03:33:11+01:00

Sean Kim commented

2024-03-04 03:33:11 +01:00

This PR migrates the existing PAINT_OT_hide_show operator to the previously extracted gesture namespace performed in #118881

Changes

The current operator has support for choosing whether the hide / show action is performed on every vertex inside or outside the selection area. This has been added to the common gesture functionality too, but no common operator property has been exposed for it for the other Lasso / Box tools to use.

Previously, the PAINT_OT_hide_show operator handled three main cases (exposed as the VisArea enum):

All vertices of a mesh - ALL
All masked vertices of a mesh above a hardcoded threshold of 0.5 - MASKED
Either all selected vertices selected by the box tool (INSIDE) or all unselected vertices (OUTSIDE)

The existing code has been refactored to handle the first two options separately from the selection usecase to more clearly delineate a separation of concerns.

Open Questions

Is it useful / necessary for this tool to support the Front Faces Only setting that other box tools support?
Should the inside / outside option be exposed across all Box tools without requiring the user to edit the operator via the keymap?

Testing

Manual testing of the Box Hide operator with both INSIDE and OUTSIDE functionality

Prerequisite for #80390

This PR migrates the existing `PAINT_OT_hide_show` operator to the previously extracted `gesture` namespace performed in #118881 ## Changes The current operator has support for choosing whether the hide / show action is performed on every vertex inside or outside the selection area. This has been added to the common gesture functionality too, but no common operator property has been exposed for it for the other Lasso / Box tools to use. Previously, the `PAINT_OT_hide_show` operator handled three main cases (exposed as the `VisArea` `enum`): * All vertices of a mesh - `ALL` * All masked vertices of a mesh above a hardcoded threshold of `0.5` - `MASKED` * Either all *selected* vertices selected by the box tool (`INSIDE`) or all *unselected* vertices (`OUTSIDE`) The existing code has been refactored to handle the first two options separately from the selection usecase to more clearly delineate a separation of concerns. ## Open Questions * Is it useful / necessary for this tool to support the `Front Faces Only` setting that other box tools support? * Should the inside / outside option be exposed across all Box tools without requiring the user to edit the operator via the keymap? ## Testing * Manual testing of the `Box Hide` operator with both `INSIDE` and `OUTSIDE` functionality Prerequisite for #80390

Sean Kim added 1 commit 2024-03-04 03:33:22 +01:00

e827aca772 Sculpt: Migrate Box Hide to common gesture code

* Switch Box Hide tool to use extracted gesture functionality
* Adds support for inside/outside selection into core gesture code
* Separate object-wide functionality from selection-based functionality

Sean Kim requested review from Hans Goudey 2024-03-04 03:34:35 +01:00

Hans Goudey reviewed 2024-03-04 19:19:22 +01:00

Hans Goudey left a comment

I like where you're heading here, but I'm finding the code a bit more confusing now. I left some comments inline with ideas.

source/blender/editors/sculpt_paint/paint_hide.cc Outdated

						
				@ -277,1 +267,4 @@

				static void partialvis_update_mesh(gesture::GestureData *gesture_data)

				{

				  HideShowOperation *operation = (HideShowOperation *)gesture_data->operation;

Hans Goudey commented

2024-03-04 19:10:21 +01:00

Use C++ reinterpret_cast rather than C-style casts

Use C++ `reinterpret_cast` rather than C-style casts

Sean-Kim marked this conversation as resolved

source/blender/editors/sculpt_paint/paint_hide.cc Outdated

						
				@ -594,2 +666,3 @@

				  switch (pbvh_type) {

				  Vector<PBVHNode *> nodes = bke::pbvh::search_gather(pbvh,

				                                                      [&](PBVHNode & /* node */) { return true; });

Hans Goudey commented

2024-03-04 19:17:47 +01:00

You can just pas {} to gather all nodes.

You can just pas `{}` to gather all nodes.

👍 1

Sean-Kim marked this conversation as resolved

source/blender/editors/sculpt_paint/paint_hide.cc Outdated

						
				@ -650,9 +733,12 @@ void PAINT_OT_hide_show(wmOperatorType *ot)

				  ot->idname = "PAINT_OT_hide_show";

				  ot->description = "Hide/show some vertices";

				  /* Because this operator handles both interactive viewport functionality and predefined menu

Hans Goudey commented

2024-03-04 19:17:03 +01:00

Because this operator handles both interactive viewport functionality and predefined menu actions

This seems to make the code quite a bit more complicated honestly. It's sort of hard to understand the diff and the code when it mixes these things. Especially with all the switches with defaults now, it's hard to tell where everything is handled at a glance. What do you think about splitting this into more operators? Something like:

PAINT_OT_hide_masked
PAINT_OT_hide_all

>Because this operator handles both interactive viewport functionality and predefined menu actions This seems to make the code quite a bit more complicated honestly. It's sort of hard to understand the diff and the code when it mixes these things. Especially with all the switches with defaults now, it's hard to tell where everything is handled at a glance. What do you think about splitting this into more operators? Something like: - `PAINT_OT_hide_masked` - `PAINT_OT_hide_all`

Sean Kim commented

2024-03-04 19:23:47 +01:00

More operators was my first idea, but I was worried if doing so would be a breaking change. I’ll work on splitting them up.

I can see it being confusing too because of the function naming, I’ll spend some time thinking about that as well.

More operators was my first idea, but I was worried if doing so would be a breaking change. I’ll work on splitting them up. I can see it being confusing too because of the function naming, I’ll spend some time thinking about that as well.

Sean-Kim marked this conversation as resolved

source/blender/editors/sculpt_paint/sculpt_intern.hh Outdated

						
				@ -1671,6 +1671,11 @@ enum eShapeType {

				  SCULPT_GESTURE_SHAPE_LINE,

				};

				enum eSelectionType {

Hans Goudey commented

2024-03-04 19:16:16 +01:00

enum eSelectionType -> enum class SelectionType

And it can have Inside and Outside as names. Combined with other changes I suggested, this could completely replace VisArea too.

`enum eSelectionType` -> `enum class SelectionType` And it can have `Inside` and `Outside` as names. Combined with other changes I suggested, this could completely replace `VisArea` too.

👍 1

Sean-Kim marked this conversation as resolved

Sean Kim added 1 commit 2024-03-05 00:30:21 +01:00

ca40008b9d Address PR feedback

* Use reinterpret_cast
* Split existing operator into two new operators
* Fix call to search_gather
* Use enum class

Hans Goudey requested changes 2024-03-05 02:59:47 +01:00

Dismissed

Hans Goudey left a comment

Really nice! Just a few small comments.

source/blender/editors/sculpt_paint/paint_hide.cc

						
				@ -276,0 +268,4 @@

				  vert_hide_update(*object, nodes, [&](const Span<int> verts, MutableSpan<bool> hide) {

				    for (const int i : verts.index_range()) {

				      if (gesture::is_affected(gesture_data, positions[verts[i]], normals[verts[i]]))

				        hide[i] = value;

Hans Goudey commented

2024-03-05 02:53:50 +01:00

Missing curly braces here. We use them around all if statements

👍 1

Sean-Kim marked this conversation as resolved

source/blender/editors/sculpt_paint/paint_hide.cc

						
				@ -531,3 +518,1 @@

				  ViewContext vc = ED_view3d_viewcontext_init(C, depsgraph);

				  BoundBox bb;

				  ED_view3d_clipping_calc(&bb, clip_planes, vc.region, vc.obact, rect);

				  partialvis_update_bmesh_nodes(ob, nodes, action, {});

Hans Goudey commented

2024-03-05 02:56:56 +01:00

Rather than checking whether should_update is empty in the callback, maybe it's cleaner to pass [](const BMVert &vert) { return true; } here. That way the price isn't payed for other tests where it isn't necessary

Rather than checking whether `should_update` is empty in the callback, maybe it's cleaner to pass `[](const BMVert &vert) { return true; }` here. That way the price isn't payed for other tests where it isn't necessary

Sean-Kim marked this conversation as resolved

source/blender/editors/sculpt_paint/sculpt_intern.hh Outdated

						
				@ -1671,6 +1671,11 @@ enum eShapeType {

				  SCULPT_GESTURE_SHAPE_LINE,

				};

				enum class eSelectionType {

Hans Goudey commented

2024-03-05 02:59:33 +01:00

enum class eSelectionType {
  Inside = 0,
  Outside = 1,
};

Sorry this is a bit nitpicky, the e is something we want to move away from though, and they sort of just look better without all caps ;) I'll commit more cleanups to move in that directions in more of the surrounding code later.

``` enum class eSelectionType { Inside = 0, Outside = 1, }; ``` Sorry this is a bit nitpicky, the `e` is something we want to move away from though, and they sort of just look better without all caps ;) I'll commit more cleanups to move in that directions in more of the surrounding code later.

Sean Kim commented

2024-03-05 05:11:22 +01:00

Oh, was unaware of the change in standards - I think the handbook style guide is a bit out of date with those regards then

Sean-Kim marked this conversation as resolved

Sean Kim added 1 commit 2024-03-05 05:09:22 +01:00

5a00795e47 Address PR feedback

Sean Kim added 1 commit 2024-03-05 05:13:29 +01:00

buildbot/vexp-code-patch-lint Build done. Details

buildbot/vexp-code-patch-linux-x86_64 Build done. Details

buildbot/vexp-code-patch-windows-amd64 Build done. Details

buildbot/vexp-code-patch-coordinator Build done. Details

1abf8b3ca5 Add missed optimization for functionref

Sean Kim requested review from Hans Goudey 2024-03-05 05:22:29 +01:00

Hans Goudey approved these changes 2024-03-05 13:51:20 +01:00

Dismissed

Hans Goudey approved these changes 2024-03-05 13:53:01 +01:00

Hans Goudey left a comment

What do you think about "hide_show_all" vs "hide_show_global"? I think it's a reasonable idea, but I didn't get used to "global" yet. It just has so many other connotations. "All" might be clear enough here.

Other than that, this looks good to go.

What do you think about "hide_show_all" vs "hide_show_global"? I think it's a reasonable idea, but I didn't get used to "global" yet. It just has so many other connotations. "All" might be clear enough here. Other than that, this looks good to go.

Hans Goudey commented

2024-03-05 13:53:09 +01:00

@blender-bot build

Sean Kim commented

2024-03-05 17:37:46 +01:00

What do you think about "hide_show_all" vs "hide_show_global"? I think it's a reasonable idea, but I didn't get use to "global" yet. It just has so many other connotations. "All" might be clear enough here.

Other than that, this looks good to go.

I was debating the same naming myself, I think all is fine, global does imply that it would apply to multiple objects.

> What do you think about "hide_show_all" vs "hide_show_global"? I think it's a reasonable idea, but I didn't get use to "global" yet. It just has so many other connotations. "All" might be clear enough here. > > Other than that, this looks good to go. I was debating the same naming myself, I think `all` is fine, `global` does imply that it would apply to multiple objects.

👍 1

Sean Kim added 3 commits 2024-03-06 01:12:23 +01:00

8a5a1288b9 Renaming "*_global" to "*_all"

733fd5ea1f Merge branch 'main' of projects.blender.org:blender/blender into migrate-hide-show

buildbot/vexp-code-patch-lint Build done. Details

buildbot/vexp-code-patch-linux-x86_64 Build done. Details

buildbot/vexp-code-patch-darwin-arm64 Build done. Details

buildbot/vexp-code-patch-darwin-x86_64 Build done. Details

buildbot/vexp-code-patch-windows-amd64 Build done. Details

buildbot/vexp-code-patch-coordinator Build done. Details

54803c0523 Add missed file

Sean Kim commented

2024-03-06 01:14:20 +01:00

Renamed the files and merged from main. I ran the tests locally and they should be passing on buildbot now, just get the usual expected failures due to running in developer mode. PR should be good to go now @HooglyBoogly