Cleanup: Perform cleanup on sculpt_automasking.cc #117651

Sean Kim · 2024-01-29T23:36:43+01:00

Sean Kim commented

2024-01-29 23:36:43 +01:00

Adds documentation / comments
Applies const where possible to cache_init function codepath
Minor cleanup of conditionals and initialization

Contains changes that were deferred from #117316

* Adds documentation / comments * Applies const where possible to cache_init function codepath * Minor cleanup of conditionals and initialization Contains changes that were deferred from #117316

Sean Kim added 1 commit 2024-01-29 23:36:51 +01:00

637ba52d24 Cleanup: Perform cleanup on sculpt_automasking.cc

* Adds documentation / comments
* Applies const where possible to cache_init function
* Minor cleanup of conditionals and initialization

Sean Kim requested review from Hans Goudey 2024-01-29 23:37:17 +01:00

Hans Goudey requested changes 2024-01-30 04:03:50 +01:00

Hans Goudey left a comment

Thanks for the cahnges. I'm still being a bit picky inline, so we can align on cleanup goals, etc.

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

						
				@ -205,2 +205,4 @@

				  }

				  /* TODO: Unsure why the BRUSH_AUTOMASKING_VIEW_NORMAL mode requires a check against the

				   * propagation steps variable. This can probably be limited to checking if the brush itself

Hans Goudey commented

2024-01-30 04:02:37 +01:00

TBH I'm not too convinced by adding a TODO comment in a cleanup commit. This is more likely to become outdated, and maybe better handled as a discussion in chat or another PR that tries removing this.

👍 1

Sean-Kim marked this conversation as resolved

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

						
				@ -1290,0 +1291,4 @@

				 * For automasking modes that cannot be calculated in real time,

				 * data is also stored at the vertex level prior to the stroke starting.

				 *

				 * \param sd: the sculpt tool

Hans Goudey commented

2024-01-29 23:40:36 +01:00

Typically we won't add descriptions for parameters that are "obvious". IMO they just add noise. I would just remove the "param" and "return" comments here. The "brush can be null" part is better "documented" by making every non-null pointer into a reference

Sean Kim commented

2024-01-31 19:25:39 +01:00

I'll remove the boilerplate parameters - for the non-null pointer to reference, do you just mean changing the callers to be cache_init(s, &brush, ... ) and cache_init(s, nullptr, ...)?

I'll remove the boilerplate parameters - for the non-null pointer to reference, do you just mean changing the callers to be `cache_init(s, &brush, ... )` and `cache_init(s, nullptr, ...)`?

Hans Goudey commented

2024-01-31 19:39:32 +01:00

Yeah, that sounds good. Could be done here or later, as part of a more general "Use references instead of pointers in sculpt code" cleanup

Sean Kim commented

2024-01-31 19:52:11 +01:00

I suppose an alternate option would be to have some form of overload:

std::unique_ptr<Cache> cache_init(const Sculpt *sd, Object *ob);
std::unique_ptr<Cache> cache_init(const Sculpt *sd, const Brush *brush, Object *ob);

Where the former method just passes in nullptr to the latter. I'm mainly thinking about this from the POV of someone working on the sculpt_automasking.cc file where it may not be obvious that the null checks are required for brush but not sd

I suppose an alternate option would be to have some form of overload: ``` std::unique_ptr<Cache> cache_init(const Sculpt *sd, Object *ob); std::unique_ptr<Cache> cache_init(const Sculpt *sd, const Brush *brush, Object *ob); ``` Where the former method just passes in `nullptr` to the latter. I'm mainly thinking about this from the POV of someone working on the `sculpt_automasking.cc` file where it may not be obvious that the null checks are required for `brush` but not `sd`

Hans Goudey commented

2024-01-31 19:55:03 +01:00

An overload seems even better!

Sean-Kim marked this conversation as resolved

Sean Kim added 2 commits 2024-01-31 20:16:45 +01:00

41c651d872 Address PR comments

062eb1bcc3 Merge branch 'main' of projects.blender.org:blender/blender into cleanup-automask

Sean Kim requested review from Hans Goudey 2024-01-31 20:18:01 +01:00

Sean Kim commented

2024-01-31 20:19:05 +01:00

~~Oh, I just realized I didn't change the callers; ignore review request for a bit.~~

Edit: updated