Page MenuHome

Cavity Automasking Mode
ClosedPublic

Authored by Joseph Eagar (joeedh) on Jun 4 2022, 4:17 AM.

Details

Summary

This patch adds a new cavity automasking mode. Cavity masking is a great way to quickly add detail in crevices and the like. It's meant to be used with the Paint brush in color attribute mode.

Differences from the sculpt-dev implementation:

  • It uses the word "cavity." When I first implemented this I wasn't aware this feature existed in other software (and other paint modes in Blender), and for reasons that escape me today I initially decided to call it a concave or concavity mask.
  • The cavity factor works a bit differently. It's no longer non-linear and functions as a simple scale around 0.5f.
  • Supports custom curves.
  • Supports blurring.

There is also a new automask -> mask baking tool.

Diff Detail

Repository
rB Blender
Branch
temp-sculpt-cavity-mask
Build Status
Buildable 22426
Build 22426: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Add automask baking tool

Tools lives in the Mask menu and also a subpanel in
the Options panel in the workspace properties.

Joseph Eagar (joeedh) edited the summary of this revision. (Show Details)Jun 29 2022, 9:54 AM

Remove blur mode 1. Seems like the consensus was that mode 2 was better.

Merge with latest master

Merge with master.

Julien Kaspar (JulienKaspar) requested changes to this revision.EditedAug 4 2022, 11:33 AM

Some more notes after testing the latest version of the patch, and from recent module meeting discussions:

  • Further performance improvements are needed. It was mentioned that these are depending on D15496 to be committed.
  • The inverted cavity auto-masking needs to be added in an abstracted way to the pie menu

The behaviour we discussed is that the "Cavity" and "Inverted Cavity" pie menu toggles are mutually exclusive.
Toggling Cavity will enable the cavity setting and disable the Invert sub-setting in the options.
Toggling Inverted Cavity will enabled the Cavity setting and enable the Invert sub-setting in the options.

This behaviour is unusual for pie menus in Blender but would lead to the most intuitive workflow.

  • The "Dirty Mask" operator in the header menu and "Bake Automask" panel in the options need to be replaced with a "Mask from Cavity" operator in the header menu.

This operator should make use of the Adjust Last Operation panel and expose the same options as the auto-mask.
By default a toggle at the top should be checked to "Use Auto-Mask settings"

  • The Invert setting of the cavity auto-mask should also invert the effect of the "Use Curve" setting.

This way the inverted cavity auto-mask will be correctly inverted. This should be more convenient and avoid unnecessary manual inverting of the curve.

  • Holding Ctrl while dragging the Factor slider to snap to increments doesn't work. It will always snap to 0 or 1.
This revision now requires changes to proceed.Aug 4 2022, 11:33 AM

patch update

  • Merge master
  • Invert curve behavior in invert mode
  • Invert and normal cavity mask are now seperate modes and mutually exclusive.
  • Automask bake operator now has option to use scene defaults.
  • The automask boolean properties are now defined in a EnumPropertyItem list to avoid three sets of duplicate code. We might want to consider making automasking flags a bitflag enum property in the future. For now the EnumPropertyList is simply read to build the boolean properties.
  • Added a "Mask from cavity" menu operator.
Joseph Eagar (joeedh) marked an inline comment as done.Aug 6 2022, 12:32 AM
Joseph Eagar (joeedh) added inline comments.
source/blender/blenkernel/BKE_paint.h
665 ↗(On Diff #52797)

I'd prefer to not do that since in the future this will go through a temporary attribute API. I'd have to either add yet another CustomData type or pack them into a CD_PROP_VEC2.

source/blender/editors/sculpt_paint/sculpt_automasking.cc
131 ↗(On Diff #52797)

I'll go with length_sum.

Joseph Eagar (joeedh) marked an inline comment as done.Aug 7 2022, 1:58 PM
Joseph Eagar (joeedh) added inline comments.
source/blender/blenkernel/BKE_paint.h
665 ↗(On Diff #52797)

Sorry I had a queued comment. I did in fact decide to do this.

Julien Kaspar (JulienKaspar) requested changes to this revision.EditedAug 8 2022, 11:54 AM

@Joseph Eagar (joeedh) I tried out the updated patch:

  • Update the inverted curve behavior

My note on inverting the curve was probably misunderstood. The way it is now in the patch, it will apparently flip the curve horizontally, which just causes the Inverted cavity to be inverted back to normal.
That is not what I meant. I'm not sure what the inversion is called that I mean but it's essentially the effect of changing all curve point locations to 1-x and 1-y.
Both the normal and inverted cavity mask added together should result in a fully masked surface.

  • Remove Bake Auto-Mask

I think we should also not use a "Bake Auto-mask" operator and instead make a focused "Mask from Cavity" operator. This will keep the settings focused and easy to understand.
There is no clear use in baking the auto mask for face sets, topology or boundaries, since this can be done much faster and more reliably with Expand.
So instead of a whole Bake Auto-mask dropdown in the options, we can add a "Convert to Mask" button at the bottom of the Cavity auto-mask options.
This would then execute the Mask From Cavity operator with the auto-mask settings.

  • Add Curve UI to Redo Panel

In the redo panel for Mask from Cavity there is also no Curve UI yet. This should be possible to add right? The Bevel operator also has a Curve exposed int he redo panel.

  • Rename "Use Default Settings"

I would also suggest to rename the "Use Default Settings" to "Use Auto-Mask Settings". This is just clearer since "default" suggests that it will switch to a Blender default values.
Instead it actually switches to the user defined settings in the sculpt mode options, as the tooltip states.

An open question I have is: If the redo settings are changed while the "Use Auto-Mask Settings" toggle is enabled, can the auto-mask settings be updated based on those changes?

This revision now requires changes to proceed.Aug 8 2022, 11:54 AM

Make requested patch changes:

  • Add curve to redo last panel
  • Renamed bake_automask operator to make_cavity and removed options for other automask types.
  • Renamed 'Use Default Settings" to "Use Automask Settings."
  • Added a second automask cavity curve to Sculpt struct for use by operators. Note: not used if "Use Automask Settings" is on.
  • Fixed curve inversion behavior.
  • Renamed e_len to valence and make it an integer.
  • Renamed various counter variables from _num to _len

Fix OSX compile error.

Julien Kaspar (JulienKaspar) requested changes to this revision.EditedAug 9 2022, 10:36 AM

Great! The updates work good! Only a few more notes:

  • The Blur steps in the redo panel seem to only start at a value of 2. The value of 0 and 1 has the same effect.
  • The Curve UI in the redo panel has a bugged element. It shows and enabled "X" toggle in the top left corner instead of the zooming icons.
  • The "Mask From Cavity" operator in the header is always defaulting to not "Use Auto-Mask Settings". That's fine for the first time it's used. But if the user enables the toggle to use the auto-mask settings, the operator needs to remember that settings for future operations.

EDIT: Ths note was not solved like suggested but instead all settings are hidden when "Use Auto-Mask Settings" is enabled. This solves the issue at least somewhat.

  • The "Bake Cavity Mask" in the options ignores the Invert and Use Curve settings. This needs to be fixed.
  • Since the "Bake Cavity Mask" UI is only relevant if the cavity auto-masking is enabled, we should make that context clearer.

I'd suggest moving the dropdown up so it is directly below the auto-masking section. We should also hide it when no cavity auto-masking is used.

  • The operator for creating the cavity mask should also ideally be renamed to "Mask From Cavity", or something along those lines, like the menu operator, instead of "Bake Cavity"

It's only really being "baked", when put in context with the auto-masking settings. Baking is also typically associated with the Baking section in the render properties.
It's fine to keep the naming of "Bake" in the sculpt mode options UI and button right under the auto-masking settings IMO. This note is just about the name of the operator in general and the title in the redo panel.

This revision now requires changes to proceed.Aug 9 2022, 10:36 AM

The tooltips also still need a bit of adjusting.
The "Cavity" and "Cavity (Inverted)" auto-masking toggles also still don't have tooltips.
Cavity = "Do not affect vertices within crevices, based on the surface curvature."
Cavity (Inverted) = "Do not affect vertices on peaks, based on the surface curvature."

The Adjust Last Operation panel also is still lacking tooltips for the settings. There we can reuse the existing tooltips in the options.

source/blender/editors/sculpt_paint/sculpt_ops.c
1173 ↗(On Diff #54528)

This is not true when used from the header operator.
I already suggested to rename the operator to something like "Mask From Cavity", but the tooltip should also reflect what the operator does:
"Creates a mask based on the curvature of the surface"

source/blender/makesrna/intern/rna_sculpt_paint.c
875

Let's instead write: "The amount of times the cavity mask is blurred."

884

We should make the tooltips more descriptive. Like:
"The contrast of the cavity mask."

  • Rename "Bake Cavity" to "Mask From Cavity"
  • Mask from cavity operator's redo panel now edits automasking properties if "Use Automasking Settings" is on.
  • Bake cavity panel now emulates its operator UI.
    • So if use_automasking is on the operator last properties are shown, otherwise automasking properties are.
  • Renamed panel to "Mask from cavity"
  • Fix bug with blur steps starting from 1 instead of 0
Julien Kaspar (JulienKaspar) requested changes to this revision.EditedAug 18 2022, 12:29 PM

Some more issues that I've found, some of them caused by recent updates:

  • The naming of toggles in the Adjust Last Operation panel changes depending on the "Use Auto-masking Settings" toggle.

These should stay consistent. I'd suggest to use the naming from when the "Use Automask Settings" is toggled off.

  • Toggling "Invert" in the Mask From Cavity operator options, toggles the Cavity (Invert) auto-masking mode, if the toggle "Use Automask Settings" is enabled

We should avoid this unintentionally toggling auto-masking modes. This should only affect the settings of the auto-masks. Not if they are used.
So I'd suggest that the "Invert" toggle in the Mask From Cavity operator does not affect any setting in the auto-masking options, even if "Use Automask Settings" is enabled.

  • The cavity auto-masking settings in the brush settings need to be completely independent from the settings in the options panel.

This means that settings like Cavity Factor, Cavity Blur and the Use Curve toggle and the Cavity Curve must not be synced.
The brush settings needs to be able to set their own properties, to make the brush behavior predictable.

  • We need to rethink the Mask From Cavity panel in the options.

This part of the UI is supposed to always use the auto-mask settings. Like mentioned in a previous note, this should be made clearer.
A big issue right now is also that it duplicates the same UI even further in the properties.

Here's my suggestion:

Instead of displaying the entire options from the Mask From Cavity operator as a sub-panel, I suggest to only expose the operator as a button, directly under the cavity auto-masking options.
It's also important that:

  • The Mask From Cavity button is only displayed if either Cavity or Cavity (Inverted) is enabled
  • The operator, when used in the options panel, will always default to have "Use Auto-Masking settings" enabled
  • If the user wants to change the settings of the operation, like mix Mode or Mix Factor, they can do so in the Adjust Last Operation popup.
This revision now requires changes to proceed.Aug 18 2022, 12:29 PM

temp-sculpt-cavity-mask: Reuse cavity mask across strokes

  • Cavity mask cache can now be reused across strokes. This is done by hashing the automasking settings to see if anything's changed. Note: reuse only happens for sculpt tools that don't modify the mesh (e.g. paint).
  • Cavity cache uses new attribute API.

temp-sculpt-cavity-mask: Make requested changes

  • Brush now has its own cavity mask settings
    • I modified the UI a bit to make it clearer when a brush automasking setting is overriding a scene one.
  • Merge remote-tracking branch 'origin' into temp-sculpt-cavity-mask
Julien Kaspar (JulienKaspar) requested changes to this revision.EditedSep 23 2022, 10:33 AM

Some last issues I've found.

  • Pie Menu arrangment is broken (Now there are two Topology options and no Inverted Cavity)

I modified the UI a bit to make it clearer when a brush automasking setting is overriding a scene one.

  • Let's leave this out of this patch. This makes the UI very messy at the moment and iterating on this would just block the patch from landing in master further.

This includes greying out toggles and UI areas and changing the toggle names in the properties and pie menus. We can just handle all of this as a separate patch.

  • A small note is just that the button "Mask From Cavity" needs a bit more spacing below it.

Also please adress the inline comments about the tooltips.

This revision now requires changes to proceed.Sep 23 2022, 10:33 AM

When it comes to performance improvements and better quality blurring of the cavity mask, this should be adressed after the patch has landed and it is tested more.

Joseph Eagar (joeedh) marked 3 inline comments as done.
  • Revert UI changes.
  • Add tooltips.
  • Add versioning code.

Replace a few float[3]s with float3.

Update to latest master

I'd say this patch is ready to land. Any small issues that remain can be tested and found when in master.

The only thing I'd suggest to still add is this note on the tooltips:

The "Cavity" and "Cavity (Inverted)" auto-masking toggles also still don't have tooltips.
Cavity = "Do not affect vertices within crevices, based on the surface curvature."
Cavity (Inverted) = "Do not affect vertices on peaks, based on the surface curvature."

Code overall looks fine, mostly some code style issues.

release/scripts/startup/bl_ui/space_view3d_toolbar.py
1008

Revert whitespace change

source/blender/blenkernel/intern/brush.cc
1716 ↗(On Diff #56081)

Revert whitespace change

source/blender/blenloader/intern/versioning_300.cc
3538 ↗(On Diff #56081)

According to the defaults, we should do this for any brush, not only when in sculptmode
automasking_cavity_factor

source/blender/editors/sculpt_paint/sculpt.c
5415 ↗(On Diff #56081)

Not found of the name. Consider to use SCULPT_stroke_id_next or write increase; inc can be confusing as it is an abbreviation that isn't industry standard.

source/blender/editors/sculpt_paint/sculpt_automasking.cc
369 ↗(On Diff #56081)

You could use a bitwise conversion in stead of introducing precision issues.

567 ↗(On Diff #56081)

Use BLI_assert_unreachable. Message can be added as a comment.

Or write down as an assert

BLI_assert_msg(BKE_pbvh_type(ss->pbvh) != PBVH_FACES || ss->pmap, "...")

source/blender/editors/sculpt_paint/sculpt_ops.c
1211 ↗(On Diff #56081)

Remove commented out code.

source/blender/makesrna/RNA_access.h
908 ↗(On Diff #56081)

Naming is not clear and would not add it to RNA_access as this is what is used to access core features.
Perhaps add a function in editors/sculpt that gives the reference and move the enum items there.

Jeroen Bakker (jbakker) requested changes to this revision.Sep 26 2022, 11:25 AM
This revision now requires changes to proceed.Sep 26 2022, 11:25 AM
Joseph Eagar (joeedh) marked 8 inline comments as done.

Make requested patch changes

Just a small code-style, but patch seems good to go.

release/scripts/startup/bl_ui/space_view3d_toolbar.py
1008

Add empty line (without whitespaces) to comply to python code-style.
Each top level definition should be separated with 2 empty lines.

This revision is now accepted and ready to land.Sep 28 2022, 10:51 AM
This revision was automatically updated to reflect the committed changes.