Cleanup: Extract sculpt gesture functionality to separate file #118881

Sean Kim · 2024-02-29T03:48:30+01:00

Sean Kim commented

2024-02-29 03:48:30 +01:00

This PR extracts the common sculpt gesture code from paint_mask.cc into a separate file in preparation for further changes to use it within the existing PAINT_OT_hide_show box gesture operator and later for adding a new Lasso Hide operator for #80390

Testing

Basic sanity check of Trim Box operator to ensure that nothing went wrong while extracting code.

This PR extracts the common sculpt gesture code from paint_mask.cc into a separate file in preparation for further changes to use it within the existing `PAINT_OT_hide_show` box gesture operator and later for adding a new *Lasso Hide* operator for #80390 ## Testing * Basic sanity check of *Trim Box* operator to ensure that nothing went wrong while extracting code.

Sean Kim added 1 commit 2024-02-29 03:48:39 +01:00

af1f5cfdb1 Cleanup: Extract sculpt gesture functionality to separate file

Sean Kim requested review from Hans Goudey 2024-02-29 03:48:59 +01:00

Hans Goudey reviewed 2024-02-29 16:44:16 +01:00

Hans Goudey left a comment

Seems like a good place to start!

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

						
				@ -63,6 +63,8 @@

				namespace blender::ed::sculpt_paint::mask {

				using namespace blender::ed::sculpt_paint::gesture;

Hans Goudey commented

2024-02-29 16:42:35 +01:00

I'd rather avoid these using namespace. Ideally namespaces are helpful when reading to show where different code comes from and separate concerns.

I'd rather avoid these `using namespace`. Ideally namespaces are helpful when reading to show where different code comes from and separate concerns.

👍 1

Sean Kim commented

2024-02-29 18:31:21 +01:00

Ah, yeah this and your other comment make sense - I'll do a renaming of the associated types and methods and remove the usage of using

Ah, yeah this and your other comment make sense - I'll do a renaming of the associated types and methods and remove the usage of `using`

Sean-Kim marked this conversation as resolved

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

						
				@ -1667,0 +1759,4 @@

				};

				/* Determines whether or not a gesture action should be applied. */

				bool sculpt_gesture_is_effected(SculptGestureContext *sgcontext,

Hans Goudey commented

2024-02-29 16:43:44 +01:00

As an example about the namespace function naming thing, all the Sculpt in these function and type names are unnecessary, since the code is already in the sculpt_paint namespace. Not sure it's worth changing in this PR, but that's the direction we can go in anyway.

As an example about the namespace function naming thing, all the `Sculpt` in these function and type names are unnecessary, since the code is already in the `sculpt_paint` namespace. Not sure it's worth changing in this PR, but that's the direction we can go in anyway.

Sean-Kim marked this conversation as resolved

Sean Kim added 1 commit 2024-02-29 19:53:45 +01:00

3d89232665 Remove using declaration and update naming

Sean Kim reviewed 2024-02-29 19:59:22 +01:00

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

						
				@ -1667,0 +1698,4 @@

				struct Operation;

				/* Common data used for executing a gesture operation. */

				struct Context {

Sean Kim commented

2024-02-29 19:59:22 +01:00

Unsure if this naming without the SculptGesture prefix is now too similar to the common bContext. I have some slight concerns about this naming later with free too - thoughts on both of these @HooglyBoogly ?

Unsure if this naming without the `SculptGesture` prefix is now too similar to the common `bContext`. I have some slight concerns about this naming later with `free` too - thoughts on both of these @HooglyBoogly ?

Hans Goudey commented

2024-03-01 00:39:02 +01:00

Thanks for trying that out! I think those are valid concerns. Maybe free_data would work? And maybe GestureData is okay. I like something like cloth::SimulationData, but I can't think of another word besides Gesture for that. Not a big deal to just add a bit of redundancy in these cases though.

Thanks for trying that out! I think those are valid concerns. Maybe `free_data` would work? And maybe `GestureData` is okay. I like something like `cloth::SimulationData`, but I can't think of another word besides `Gesture` for that. Not a big deal to just add a bit of redundancy in these cases though.

👍 1

Sean-Kim marked this conversation as resolved

Sean Kim added 1 commit 2024-03-01 01:12:20 +01:00