Page MenuHome

Sculpt Face Sets
ClosedPublic

Authored by Pablo Dobarro (pablodp606) on Oct 15 2019, 4:55 PM.
Tokens
"Yellow Medal" token, awarded by neotam."Love" token, awarded by bnzs."Like" token, awarded by ThinkingPolygons."Party Time" token, awarded by roman13."Love" token, awarded by hitrpr."Love" token, awarded by pablovazquez."Love" token, awarded by Shimoon."Like" token, awarded by Constantina32."Love" token, awarded by oobma."Party Time" token, awarded by Alumx."Love" token, awarded by Kronk."Love" token, awarded by jmztn."Love" token, awarded by mankysee."Love" token, awarded by Way."Love" token, awarded by rbx775."Yellow Medal" token, awarded by ostapblender."Love" token, awarded by S_Jockey."Love" token, awarded by Anubis."Love" token, awarded by marcoalvares."Love" token, awarded by RyanJEC."Love" token, awarded by amonpaike."Love" token, awarded by monio."Love" token, awarded by michaelknubben."Love" token, awarded by andruxa696."Love" token, awarded by Dusty_Shoe."Love" token, awarded by julperado."Love" token, awarded by Brandon777."Like" token, awarded by knightknight."Love" token, awarded by MetinSeven."Love" token, awarded by iWaraxe."Love" token, awarded by tiagoffcruz."Love" token, awarded by erickblender."Love" token, awarded by TheFlow."Like" token, awarded by Alrob."Yellow Medal" token, awarded by Zino."Mountain of Wealth" token, awarded by TheRedWaxPolice."Love" token, awarded by xrg."100" token, awarded by Frozen_Death_Knight.

Details

Summary

Face Sets are the new system to control the visibility state of the mesh in sculpt and paint modes. They are designed to work in modes where brushes are the primary way of interaction and they provide much more control when working with meshes with complex shapes and overlapping surfaces.

This initial commit includes:

  • Sculpt Face Sets data structures and PBVH rendering.
  • Face Set overlay and opacity controls.
  • Sculpt Undo support.
  • Remesher reprojection support. The visibility state of the mesh is also preserved when remeshing.
  • Automasking and Mesh filter support.
  • Mask expand operator mode to expand Face Sets (Shift + W) and flood fill areas by connectivity (press Ctrl while expanding).
  • Sculpt Mode Face Sets and Visibility API.
  • Sculpt Face Sets creation and visibility management operators.
  • Operator to randomize the Face Sets colors.
  • Draw Face Sets brush tool to create and edit the Face Sets. Drawing on the mesh creates a new Face Set. Pressing Ctrl before drawing modifies the Face Set under the brush at the beginning of the stroke.
  • Updated keymap and menu to work with Face Sets from Sculpt Mode (H to toggle visibility, Alt + H to show all, Shit + H to hide).
  • Pie menu on the W key with Face common Sets operations.

Know limitations:

  • Multires support. The Face Sets and Visibility API needs to be implemented for Multires.

Diff Detail

Repository
rB Blender

Event Timeline

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

Yes, my bad. Sculpt Face Maps was suggested

So should I rename it to Face Maps or Face Sets?

Everyone in the UI team seems to agree "Face Sets" is ok. While "Face Maps" is perhaps also a bit unclear since it's not mapping one thing to another the way UV maps or face maps for rigging do.

So, let's go with that "Face Sets".

  • Rename to Face Sets
Pablo Dobarro (pablodp606) retitled this revision from Sculpt Face Masks to Sculpt Face Sets.Feb 25 2020, 2:02 PM
Julian Eisel (Severin) requested changes to this revision.EditedFeb 25 2020, 6:27 PM

Played with this for a bit to see where it stands. Found some issues that need to be looked into. Quickly going over RNA, DNA and UI changes, I didn't see anything obviously wrong.

If the issues mentioned below can be fixed soon, I think this can also go in soon.

  • I get a memory leak from using the face set tool, grows to > 1GB after a while of sculpting. (allocated by MEM_lockfree_reallocN_id() call in BKE_pbvh_node_add_proxy()).
  • Starting Blender with --debug-memory and ASAN, I get a crash with the following steps:
    • Replace default cube with monkey
    • Ctrl+2 to add subdiv modifier (level 2 matters for some reason...)
    • Apply modifier
    • Switch to sculpt mode and start sculpting
  • There are some warnings on GCC with default settings: P1274.
  • Without visual feedback, the behavior felt quite random and broken to me. It took me a way too long time to find the Face Sets Opacity slider (in fact, I only found it through code). Maybe we can always draw face sets by default (somehow) when the face set drawing tool is active?
  • When setting the brush strength to 0, I'd expect it to not paint, currently it always does.
  • This uses H to toggle visibility, we usually use H to hide and Alt+H to unhide.
  • There are custom data warnings printed, but I assume those are the ones you refer to in the description.
  • There are some other glitches when toggling visibility, but they are difficult to reproduce and may be caused by mentioned memory issues. We can look into them if they are still there after the memory issues are fixed.
  • There is no explanation of what a face set is in the UI. I think the tool's tooltip should have a few words doing that.

@William Reynish (billreynish) could you give some feedback too? Also for the keymap changes.

source/blender/draw/engines/overlay/shaders/sculpt_mask_vert.glsl
16–18

face_set_color is just a copy of fset. From the code this seems unnecessary?

source/blender/draw/intern/draw_manager_data.c
826

use_facesets

source/blender/draw/modes/shaders/sculpt_facemap_vert.glsl
1 ↗(On Diff #22035)

Add the shader to source/blender/draw/CMakeLists.txt so IDEs can add it to their project files.

The shader also still needs to be renamed (facemap->faceset).

14 ↗(On Diff #22035)

Can simplify to

finalColor = mix(vec4(fgrp, 1.0), vec4(1.0), 1.0 - fgrpOpacity);

Also, fgrp should be changed, I suggest to face_set (there's no good reason to abbreviate this here, there are good reasons to avoid useless abbreviations).

source/blender/makesdna/DNA_mesh_types.h
203

Rather than adding a new padding var, you can reuse _pad1 and give it a size of 7.

This revision now requires changes to proceed.Feb 25 2020, 6:27 PM
Pablo Dobarro (pablodp606) marked 5 inline comments as done.
  • Rebase
  • Review update

@Julian Eisel (Severin) I think most of the issues you mentioned are now fixed. Almost all artifacts in hide/show operations should have been fixed in D6767.
Regarding the viewport display, I can hack something to ignore the viewport settings when a tool is selected, but I think that is not supported by design. All the masking tools are in the same situation, you can use all masking operators with the overlays disabled.

When in the master? I'd like this feature to appear in release 2.83.If like all the questions have been solved.

@Pablo Dobarro (pablodp606) I was testing and got some crashes, but could not really find the source.

  • By default the face set overlay is set to zero, what makes it hard for users to start working with the tool. Are you missing do version code
  • Note that Face masks aren't visible when modifiers are active, should we document that as a known issue?
  • Not all options are discoverable via the menu's perhaps we should also add them to the menus. Pie menus are not easy for starters when you don't know about them.
  • Rebase
  • Add visibility toggle
  • Add Face Sets menu to Sculpt Mode
  • Show Face Sets by default
  • Tweak threshold for Face Sets from Masked
  • Add Randomize Face Sets Colors to the UI

@Jeroen Bakker (jbakker) Face sets are not visible with modifiers, just like the sculpt mask. That is still a TODO.

Julian Eisel (Severin) requested changes to this revision.Mar 3 2020, 7:28 PM

I wouldn't mind accepting this even if there is a known bug or two. It should still be checked if they can somehow end up corrupting files though.

  • I still get the mentioned crash (monkey with applied subdiv modifier). This is a memory error so it should be checked if that can corrupt files before committing.
  • There should probably also be a shortcut for toggling the face sets in the industry compatible keymap.
  • Enabling face-sets by default is a quite drastic measure. That means there's always a weird color visible, rather than the set shading. @William Reynish (billreynish) opinion?
  • The patch still uses H to toggle visibility, rather than H and Alt+H. That is an inconsistency that should be discussed first. Again, something for @William Reynish (billreynish) to check.
source/blender/editors/sculpt_paint/sculpt_intern.h
247–249

These are defined twice now, giving warnings on GCC.

This revision now requires changes to proceed.Mar 3 2020, 7:28 PM

I tested the patch a bit and I found visual bugd when having a vertex color layer, drawing face sets in a color mode other than vertex colors (like random or material) and then switching to vertex colors.
The colors just completely glitch out. Can someone else confirm this.
I also noticed that sometimes face sets are still visible as wires or in white when being toggled and take a moment to catch up.

Enabling face-sets by default is a quite drastic measure. That means there's always a weird color visible, rather than the set shading. @William Sitton (william) Reynish (billreynish) opinion?>

@Julian Eisel (Severin) Is it possible to set the default color of the face sets to be white? Then the face sets are always visible but not noticeable unless the user starts drawing or randomising the face sets.

The patch still uses H to toggle visibility, rather than H and Alt+H. That is an inconsistency that should be discussed first. Again, something for @William Sitton (william) Reynish (billreynish) to check.

This has me torn since the current shortcuts are far easier to use but it is going against conventions.

@Pablo Dobarro (pablodp606) Another proposal for the functionality I have is that the traditional "Hide Masked" or "Hide Bounding Box" is not working together with the face sets. It would be great if the user could point at either a face set color or a masked area and press Shift + H to hide that area.
Same for inverting the visibility. If you hide geometry based on a box or mask and invert the visibility of face sets, it just ignores the previously hidden geometry.

  • Rebase
  • Fix Face Set viewport glitch with vertex colors

@Julien Kaspar (JulienKaspar) Making always the first face set to be white is possible, but I think it is going to add a lot of hacks through the code.
Currently, the old and the new visibility systems are working at the same time, that is why the old hide box and hide masked operators are not aware of the face set visibility. This operators are going to be rewritten in the future to use face sets directly (after making them compatible with multires), so those problems will be fixed.
@Julian Eisel (Severin) I also think that the current shortcuts are better for this use case as here you are not working with selections. It does not make much sense to remove the toggle and just leave Alt + H to show all (that already works) because the default operation is "hide everything but the active face set"

Pablo Dobarro (pablodp606) updated this revision to Diff 22368.EditedMar 4 2020, 1:33 PM
  • Mask Expand: Press Ctrl to flood fill and area using connectivity when expanding.

This allows to quickly isolate disconnected elements of the mesh into its own face set. It works for both face sets and mask.

  • Avoid locking the cursor when using mask expand, better UX when creating face sets
  • PBVH functions cleanup
  • Fix visual glitches on Face Sets visibility undo
  • Fix crash on Face Set undo after a geometry update operation
  • Fix typo and comment formatting
  • Fix memory leak on face set expand
  • Fix mask being removed on face set expand
  • Fix error 42 - can't be written to file on file saving

@Jeroen Bakker (jbakker) @Julian Eisel (Severin) @Brecht Van Lommel (brecht) I think this is now ready for master if the loading/saving code is OK and it won't corrupt the files. It may have some remaining bugs and glitches related to the visibility state when using the undo code from the old system as it already was in a bad state or some UX/UI behaviors that can be polished. I think the sooner this can be on master to start getting bugs reports and files to fix those issues, the better, as they are quite easy to fix.

Pablo Dobarro (pablodp606) edited the summary of this revision. (Show Details)
  • Set Face Set opacity to 0 in versioning

Think it's fine to merge this, although I'd ask to change to using Alt+H for unhiding first. Then we are consistent "by default" and can check on a exception to use H to toggle after merging.

Also +1 on setting face set opacity to 0 by default, at least until we know a better way to handle their visiblity.
My proposal would be to only draw the face set colors for actually existing face sets, not for faces that are not explicitly assigned to any face set. But again, this can be worked on after the merge.

If you say the memory errors don't corrupt files, I'll just trust you on that ;)

@Julian Eisel (Severin) Alt + H for unhiding is already in the keymap. If you don't press H twice to use the toggle, H and Alt + H should be consistent with other modes

I see, didn't catch that from the keymap. In that case pressing H over an already hidden face should do nothing. In other words, the mode for H should not be TOGGLE.

I also think it's better to just be consistent. We should just use H and Alt-H like elsewhere.

@Julian Eisel (Severin) You need to take into account how this feature will be used. When using face sets, you usually want to quickly alternate between 2 overlapping face sets multiple times (like two fingers in a hand or an arm touching the body). If we keep H as a toggle, you always know that pressing that shortcut will bring you to the other visibility state automatically, where you can select another face set and continue working from there. If we remove the toggle, you need to analyze first if you are in an partially hidden state and you want to show all to select another face set (press Alt H) or if you want to hide everything but the active face set (press H) to create this quick select -> hide -> select cycle. It is an UX annoyance that should not be there, specially if the other option is having a key that does nothing.

I also think it's better to just be consistent. We should just use H and Alt-H like elsewhere.

In that case enabling the "Select All Toggles" in user preference will make H toggle? Because many users (like me) prefer the Toggle workflow instead of H , ALT+H

Why exactly wouldn't you want H to toggle hide/unhide? Frankly, it should be the default behaviour across all of Blender. Toggling with a single button when possible is more preferable to users than having to switch buttons every time. I even changed the keybind on my own because it was more convenient than the default behaviour.

@Julian Eisel (Severin) @William Reynish (billreynish) @Brecht Van Lommel (brecht) I can change the default H operation in this patch before committing, the other operation is already implemented, It is just a matter of changing the operator mode from TOGGLE to SHOW_ACTIVE in the keymap.
I just want to say that the intended behavior of this feature and the way all functionality is designed is to be used like a local view, not like hide/unhide selections in object or edit mode. We don't have an keymap entry/menu option to enter local view and another one to exit, it is a toggle and it works just like the H shortcut here. Maybe I can change the name of the TOGGLE operator to make this more clear.

The whole point of this feature is to be able to work on areas like this, where you need to be constantly switching visibility areas in order to understand what you are doing. The TOGGLE mode of the operator does exactly that and takes care of the visibility state and the next operation for you, so you don't need to think if you need to hide or unhide to continue working in the next area. I'm doing all this visibility changes just by pressing the H key and it always does what it is intended to do, it never gets locked in any state waiting for the user to press the correct shortcut. And, in any case, the Alt - H shortcut is always available to go back to the default state. Just try it for yourself by removing the toggle option and see how many times you press the wrong shorcut and get stuck when changing between areas this fast.

Pablo is right, this is much closer in behavior of the "Toggle Local View" ( Numpad / ) operator than the Hide/Unhide (H, ALT+H). Just call it "Toggle Face Set" and keep the H a toggle.

Probably a naive suggestion, but could you get around this impasse regarding hiding by adding a separate operator to "toggle visibility"?

@Harley Acheson (harley) I can do that, but the change visibility operator implementation needs quite some code to prepare and update the mesh when changing the visibility state. I added a single operator with different modes to be able to share all this code and make it easier to debug possible visibility state issues, as all visibility changes go through the exact same code. This is also important if multires or textures needs special code to handle these updates, as the code will need to be updated in a single place. Spiting this operation into a different operator or keeping this as it is (an operator with different operation modes you can select from) won't affect the user at all, but will make things harder to maintain.

So this minor hotkey issue is the thing preventing this from being committed? 👀

Accepting as UI team, shortcut keys can still be changed after commit if needed.

This revision is now accepted and ready to land.Mar 5 2020, 9:01 PM
This revision was automatically updated to reflect the committed changes.