blender
/
blender
136
407
Fork 589
Code Issues 6.3k Pull Requests 777 Projects 19 Wiki Activity

Cleanup: Perform cleanup on sculpt_automasking.cc #117651

Merged
Hans Goudey merged 4 commits from Sean-Kim/blender:cleanup-automask into main 2024-01-31 21:43:12 +01:00
Conversation 2 Commits 4 Files Changed 5 +6 -8
2 changed files with 6 additions and 8 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 41c651d872 - Show all commits

8
source/blender/editors/sculpt_paint/sculpt_automasking.cc
View File

@ -204,9 +204,6 @@ static bool needs_factors_cache(const Sculpt *sd, const Brush *brush)
return true;
}
/* 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
* exists. */
if (automasking_flags & BRUSH_AUTOMASKING_VIEW_NORMAL) {
return brush && brush->automasking_boundary_edges_propagation_steps != 1;
Sean-Kim marked this conversation as resolved Outdated
Hans Goudey commented 2024-01-30 04:02:37 +01:00
Outdated
Review
Copy Link

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.

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
}
@ -822,6 +819,11 @@ bool tool_can_reuse_automask(int sculpt_tool)
SCULPT_TOOL_DRAW_FACE_SETS);
}
std::unique_ptr<Cache> cache_init(const Sculpt *sd, Object *ob)
{
return cache_init(sd, nullptr, ob);
}
std::unique_ptr<Cache> cache_init(const Sculpt *sd, const Brush *brush, Object *ob)
{
SculptSession *ss = ob->sculpt;

6
source/blender/editors/sculpt_paint/sculpt_intern.hh
View File

@ -1290,12 +1290,8 @@ Cache *active_cache_get(SculptSession *ss);
*
* 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
* \param brush: the brush being used (optional)
* \param ob: the object being operated on
* \return the automask cache
*/
std::unique_ptr<Cache> cache_init(const Sculpt *sd, Object *ob);
Sean-Kim marked this conversation as resolved Outdated
Hans Goudey commented 2024-01-29 23:40:36 +01:00
Outdated
Review
Copy Link

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

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
Outdated
Review
Copy Link

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
Outdated
Review
Copy Link

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

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
Outdated
Review
Copy Link

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
Outdated
Review
Copy Link

An overload seems even better!

An overload seems even better!
std::unique_ptr<Cache> cache_init(const Sculpt *sd, const Brush *brush, Object *ob);
void cache_free(Cache *automasking);