Hook up invert and smooth mode to weight and vertex paint
ClosedPublic
Actions

Authored by Sebastian Parborg (zeddb) on Jan 17 2022, 5:59 PM.

Details

Reviewers

Campbell Barton (campbellbarton)

Commits

rBa215d7e230d3: Hook up invert and smooth mode to weight and vertex paint

Summary

Previously weight paint wasn't hooked up to the "smooth" and "invert" modes.
I have added code so this is the case now. (And hooked up "smooth" for vertex paint as well)

What is not in this current patch is default keymap changes that I think we might need to discuss a bit.

My idea is that we bring these more in line with sculpt mode.
So holding shift will switch to the smooth brush and holding control will invert.

Currently in weight paint mode, shift and control is used for sampling weights, I think we should move these to S so we match it with the "sample color" short cut from other painting modes.

For vertex paint mode, only the shift key short cut needs to be added which will not "steal" any of the current shortcut keys in that mode.

This way I feel our short cuts will be more consistent between painting modes.

Diff Detail

Event Timeline

Sebastian Parborg (zeddb) requested review of this revision.Jan 17 2022, 5:59 PM

Sebastian Parborg (zeddb) created this revision.

Daniel Ulrich (dulrich) awarded a token.Jan 17 2022, 7:33 PM

Generally fine, some things that can be improved on though.

Try move these set/clear blocks into a function, so the logic to set / restore values remains close, easier to follow and isn't as likely to get out of sync.

If the paint type is passes as an argument both vertex & weight paint could share the same function by the looks of it too.

Depending on the brush name "Blur" isn't so good, in general it's good to avoid depending on names in the users blend file (even if users mostly keep these names unchanged).

Suggest to use BKE_paint_toolslots_brush_get(paint, WPAINT_TOOL_BLUR) - which will get the brush that would be used if the user selects the blur brush, so they're not limited to "Blur". Note that it's possible these slots need to be initialized (if the return value isn't set). In that case BKE_paint_toolslots_brush_ensure can be added, with logic extracted from toolsystem_ref_link (see the lines that call BKE_brush_add).

This revision now requires changes to proceed.Jan 19 2022, 12:35 PM

Thanks for the feedback, I'll implement your suggestions as they makes sense to me :)

Before I do though:
I mostly just copy pasted the logic from sculpt mode, do you feel that I should also update that part as well with your proposed changes?
Specifically the change to move away from looking up brushes with BKE_libblock_find_name in this case.

Perhaps as a separate clean up commit, IE:

Sculpt brush cleanup commit
This review task commit with the suggested changes applied.

@Sebastian Parborg (zeddb) agree, making these changes for sculpt mode in a separate commit would be better, adding a BKE_paint_toolslots_brush_ensure function could also be done in a separate commit.

Updated patch with the provided feedback.

To me it seems like I didn't need to write any _ensure function as the toolslots seems to be initialized when the BRUSH_STROKE_SMOOTH is set with a keybinding.
So I'm guessing it will always be implicitly initialized in our use cases. I didn't manage to break it during my testing at least.

I haven't do the sculpt mode changes as I wanted to get a thumbs up on this new revision before I "sync" up the code there as well.

Accepting, only requesting minor changes.

source/blender/editors/sculpt_paint/paint_vertex.c
1476–1509	Suggest a single function `smooth_brush_toggle_set(..)`, since context vars are mostly similar and one case does the reverse of the other.
source/blender/makesdna/DNA_brush_enums.h
545 ↗	(On Diff #47323)	Avoid using relative locations (they get outdated), reference `#eBrushVertexPaintTool` instead.