Page MenuHome

Sculpt Face Sets
Needs ReviewPublic

Authored by Pablo Dobarro (pablodp606) on Oct 15 2019, 4:55 PM.
Tokens
"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

This is a new system to manage sculpt mode visibility. You can create groups of faces dynamically and hide/show them directly while sculpting without using masking tools or box selections.

The sculpt face grouips are also used in other sculpting features like the automasking system and mesh filters. It can also be preserved when remeshing the sculpt with the voxel remesher.
Face Groups are stored in the polys instead of in the vertices. This way we can subidivide it with multires in a predictable way.

This patch includes all basic features:

  • Sculpt Face Groups data structures
  • Undo support
  • Remesher reprojection support
  • Automasking and Mesh filter support
  • Sculpt Mode Face Groups and Visibility API
  • Sculpt Face Groups creation and visibility operators
  • New keymap

Know Issues:

  • We can change the name of this feature to avoid confusion with the mesh facemaps for rigging. If in the future we are going to use Vertex groups, Edges groups and Face groups we can keep this name for this feature. Done: The feature will be renamed to sculpt face groups
  • The facemap colors are drawn by the PBVH. We need to add something to change the visibility of the facemap and mix it with the new sculpt vertex colors. I'm not sure if we can use an overlay for this without affecting sculpt mode performance, but that would be the ideal solution. It also misses smooth shading support and opacity controls. Done: Face groups have their own overlay and opacity control
  • It needs multires support. When using dyntopo, the sculpt facemap should be disabled.
  • It gives an error related to the new datalayer.
  • I think we should store mesh visibility in sculpt mode in polys instead of in vertices. This should be an easy change. Right now I'm using both and it is starting to become a mess. Some facemap tools have bugs related to keeping in sync facemap visibility and vertex visibility. Done: visibility can be stored per face or per vertex. This way we can still support hiding parts of the mesh with tools not related to the facemap. If this still causes problems we can fully switch to face visibility by rewriting the current visibility operators, which is not hard to do (box hide and mask hide)
  • Colors are generated using a random seed that is generated each time the PBVH is built. This way we always get different colors, but colors will also change when entering sculpt mode. Is there something else I can use a seed? Maybe store it in the mesh datablock? Done: I will add an operator in a later patch to generate new colors updating the seed, which is stored per mesh.

Diff Detail

Repository
rB Blender
Branch
sculpt-facemap (branched from master)
Build Status
Buildable 6774
Build 6774: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
source/blender/makesdna/DNA_view3d_types.h
213

I don't see any versioning or defaults for this option

source/blender/makesrna/intern/rna_mesh.c
3037

Not sure we need to mention Sculpt here.

This revision now requires changes to proceed.Nov 27 2019, 10:46 AM
Pablo Dobarro (pablodp606) marked 26 inline comments as done.
  • Review update
  • Rebase
  • Fix bug in remesh reproject code
  • Add operator to randomize the face groups colors
  • Fix face groups rendering with smooth shading
source/blender/editors/sculpt_paint/sculpt.c
290

The visibility is encoded in the sign of the int. Positive numbers are visible and negative are not visible. The absolute value is the ID of the face group. This way, we can easily operate with visibility and IDs at the same time when working with vertices, as checking the max int of the connected faces will return the most recent, visible face group. The seed only affects the color when rendering and that does not have any meaning. I will add an operator to update the seed in case you are getting random colors that are too similar in adjacent face groups.

The problem that could happen is if someone overflows the int by creating face groups. We can add a code that recalculates the IDs to a lower value when calculating the next face group instead of just adding 1, but that code is going to run only if someone creates a new face group in the same mesh two billion times.

Way awarded a token.Dec 1 2019, 5:31 PM
  • Rebase
  • Use Overlay Engine in Face Groups

This change has rather significant implications and needs broader discussion. Adding the UI team as blocking reviewer.
First off: I don't want to stop the progress - in principle this patch adds a hugely useful feature that would be great to have. It is more the process that is problematic, and it's a general issue we have coming with the 2.8 dynamics.


As for this specific patch, it introduces the concept of face groups and exposes that in the UI. From a user's perspective, this sculpt mode specific thing, seems to be a quite different, yet similar concept to our other geometry groups: vertex groups, edge groups and face maps (see T54989). So it introduces a non-trivial concept without considering how it integrates with other parts of Blender.
Currently what you defined as face groups seems to be special selection type, or maybe reusable selection-sets. Not to sure though, I lack the time to investigate details right now.

There is also no rationale mentioned here, which is the first thing every proposal should mention. Not that I don't see the benefit of this (which are great!). However it does seem like you start with a design (possibly taken from other applications), rather than thinking about the problems to solve, the possible ways to go about it, exploring and evaluating designs, etc.
Without getting the right people to sign these changes off (just Jeroen is not enough), we further risk adding different kinds of technical debt (e.g. design limitations because earlier changes were done without considering the bigger picture).
Blender has a long history of adding such debt, I personally fight with it every day. We should try to do better for the future.


Management wise, we might be in a difficult situation now: This patch got large public attention, the term "face groups" was spread on Twitter before any design discussion was done in our usual channels. Somebody (in this case me) will have to be a party pooper and it looks like this person just holds back progress on an awesome feature.

Note that it's not just me raising saying these issues, and it's not only about this single patch. I'm just playing the messenger here, ideally our managers could deal with these kinds of things (CCing @Dalai Felinto (dfelinto)). We had a developer meeting here at the Blender Institute and agreed that a process like this is quite problematic for the longer term, as unsatisfying as it sounds.

release/scripts/startup/bl_ui/space_view3d.py
5023–5024

Why use the term "facemap" here?

Related: Sybren published a little requirement proposal for feature/patch descriptions, which should help addressing what I've just explained.
Again, this is a general issue in Blender development, you're going to hear more about this in the coming weeks.

This change has rather significant implications and needs broader discussion.

I don't think this patch introduces a change that big that needs a broader discussion worth delaying the feature. We are usually committing patches that introduce changes way bigger than this as bug fixes, and that usually seems not to be a problem. Even if we want to reimplement the data structure that is used to store and manipulate the face groups in the future it is not going to be a huge change. We can also consider adding this and the sculpt vertex colors as experimental features.

However it does seem like you start with a design (possibly taken from other applications), rather than thinking about the problems to solve, the possible ways to go about it, exploring and evaluating designs, etc.

This is the 4th implementation I made for a better visibility system (that is why it was named face maps in the first version of the patch. I was trying to reuse the facemaps in an early implementation). I know that I'm not the best expressing and discussing ideas in design documents and I'm sorry for that. I'm probably not following the usual development cycle, where you get the requirements from another person and you try to design a solution for that problem. I code these patches to fix and improve the issues I find while making art myself, so I think we can consider that all work related to problem identification, design and use cases is already included. Every single detail of the design and behavior of these features is carefully considered and everything has a reason to work the way it does (that is why these patches get so popular, because they solve real problems that artists are facing constantly in the most straightforward way).
We can discuss IU, implementation, code quality, optimizations and integration with other areas, but if we require a discussion or design document, starting from something as basic as what are the problems that these patches are trying to solve, we will have a high risk of losing a huge amount of time from a lot of developers going through work that was already done.

Management wise, we might be in a difficult situation now: This patch got large public attention, the term "face groups" was spread on Twitter before any design discussion was done in our usual channels

There was a discussion about this feature in the official channels. Jeroen was aware of this for a while and we discussed this with William when I was at the Blender Institute, mentioning all the issues with the vertex/edge/face groups patches. In fact, I asked William in blender.chat about renaming this to face groups before doing that change in the patch. I can change the name again, that is not the problem. The problem is that right now we have a sculpt mode with highly advanced features and a visibility system that only has box hide and mask hide. That is a product management issue, not a software development issue. I'm always seeing discussions about development management but almost none about product management, which is also affecting the project the same ways as a technical limitation, a bug or the patch review process.

Just a humble opinion from a long-time Blender user:

I think merging Sculpt Face Groups and Face Maps would benefit clarity and cross-mode usability in Blender.

Also, being able to have overlapping sculpt face groups in multiple Face Map channels would make the feature more powerful than the comparable ZBrush Polygroups.

One universally accessible face group feature would benefit third-party add-ons like Quad Remesher as well, which is the same as ZRemesher, and extensively uses Polygroups.

Just a humble opinion from a long-time Blender user:
I think merging Sculpt Face Groups and Face Maps would benefit clarity and cross-mode usability in Blender.
Also, being able to have overlapping sculpt face groups in multiple Face Map channels would make the feature more powerful than the comparable ZBrush Polygroups.
One universally accessible face group feature would benefit third-party add-ons like Quad Remesher as well, which is the same as ZRemesher, and extensively uses Polygroups.

Please let keep the discussion here about codes review otherwise all of us who have opinion will make mess out of this tickets.

This change has rather significant implications and needs broader discussion.

I don't think this patch introduces a change that big that needs a broader discussion worth delaying the feature. We are usually committing patches that introduce changes way bigger than this as bug fixes, and that usually seems not to be a problem. Even if we want to reimplement the data structure that is used to store and manipulate the face groups in the future it is not going to be a huge change. We can also consider adding this and the sculpt vertex colors as experimental features.

However it does seem like you start with a design (possibly taken from other applications), rather than thinking about the problems to solve, the possible ways to go about it, exploring and evaluating designs, etc.

This is the 4th implementation I made for a better visibility system (that is why it was named face maps in the first version of the patch. I was trying to reuse the facemaps in an early implementation). I know that I'm not the best expressing and discussing ideas in design documents and I'm sorry for that. I'm probably not following the usual development cycle, where you get the requirements from another person and you try to design a solution for that problem. I code these patches to fix and improve the issues I find while making art myself, so I think we can consider that all work related to problem identification, design and use cases is already included. Every single detail of the design and behavior of these features is carefully considered and everything has a reason to work the way it does (that is why these patches get so popular, because they solve real problems that artists are facing constantly in the most straightforward way).
We can discuss IU, implementation, code quality, optimizations and integration with other areas, but if we require a discussion or design document, starting from something as basic as what are the problems that these patches are trying to solve, we will have a high risk of losing a huge amount of time from a lot of developers going through work that was already done.

Management wise, we might be in a difficult situation now: This patch got large public attention, the term "face groups" was spread on Twitter before any design discussion was done in our usual channels

There was a discussion about this feature in the official channels. Jeroen was aware of this for a while and we discussed this with William when I was at the Blender Institute, mentioning all the issues with the vertex/edge/face groups patches. In fact, I asked William in blender.chat about renaming this to face groups before doing that change in the patch. I can change the name again, that is not the problem. The problem is that right now we have a sculpt mode with highly advanced features and a visibility system that only has box hide and mask hide. That is a product management issue, not a software development issue. I'm always seeing discussions about development management but almost none about product management, which is also affecting the project the same ways as a technical limitation, a bug or the patch review process.

If all this is just about changing the name, i say well said Pablo you got my blessings on this patch,

Pablo Dobarro (pablodp606) marked an inline comment as done.
  • Rebase
  • Fix Naming

Due to the recent changes in the overlay code, I think the face groups color glows too much because it is using alpha blending instead of multiplying the color. I think the old version (the one from the videos) looks better, but it depends on the color mode and the matcap you are using (alpha blend may look better with EEVEE enabled).

I agree with Severin that everything has to be part of something bigger. But Pablo in a few months is giving life to changes that we had been asking for years and that never arrived because of design decisions.

Pablo's virtue is, from my point of view, to be a developer who is also an advanced user and artist. That gives him a perspective of what he needs, what is important, and the way to implement it that I understand some people don't understand. But as a user I understand perfectly the way he works and I have never worried about giving him any feedback in UX because he has never let me down.

I have always blamed blender for the difficulty it has in implementing small useful tools, small changes that involve giant steps in the daily work. Remember the controversy with the weld modifier that recently occurred in the community. Right now I'm working with a version 2.82 of blender, and is that basically the patches he has added save me more time than the possible crashes of the program. I think it's better to let the development flow in a natural way and from time to time order a little what is necessary to have a strict development that only blocks the creation of tools.

My only wish is that Pablo continues to implement at the same tempo as now and that someday he will be interested in hardsurfaces or multiresolution/layers.

This change has rather significant implications and needs broader discussion. Adding the UI team as blocking reviewer.
..
Currently what you defined as face groups seems to be special selection type, or maybe reusable selection-sets. Not to sure though, I lack the time to investigate details right now.
..
There is also no rationale mentioned here, which is the first thing every proposal should mention. Not that I don't see the benefit of this (which are great!). However it does seem like you start with a design (possibly taken from other applications), rather than thinking about the problems to solve, the possible ways to go about it, exploring and evaluating designs, etc.
Without getting the right people to sign these changes off (just Jeroen is not enough), we further risk adding different kinds of technical debt (e.g. design limitations because earlier changes were done without considering the bigger picture).
Blender has a long history of adding such debt, I personally fight with it every day. We should try to do better for the future.
...


Management wise, we might be in a difficult situation now: This patch got large public attention, the term "face groups" was spread on Twitter before any design discussion was done in our usual channels. Somebody (in this case me) will have to be a party pooper and it looks like this person just holds back progress on an awesome feature.

Beforehand: I don't mean to sound personal but it will be some kind of personal.

There are quite a few things that is very very wrong with your comment.

First off: Yes, you are kind of a party pooper. To extend your analogy -- late to the party, moaning about the drinks and why is everyone is having fun without him.

Let me explain. You are literally jump on board yesterday, confessed yourself that you haven't investigate the patch but at the same time you are complaining about a number of things, high public attention, lack of paper work and expressing your privilege on blocking due to UI. I see that as a bureaucracy in it's worse.

This patch was in the works like for a half a year and was discussed with various developers before so the least that you can do is to investigate _before_ you jump in, just to respect the work that was put into it.

As for a paperwork and puplic attention you are so displeased -- public attention to this patch happened exactly because it's a useful feature that solves a very important problem for those of us who use sculpting. And instead of moaning about the lack of formal paperwork you should(and I think clearly must) put some efforts into understanding what is all fuss is about instead of working as a gatekeeper just because you _suspect_ something wrong with a patch that has no connection to your area and you have no expertise in it(these face groups have connection with UX more than with anything else leave alone UI and sculpt UX is the thing that Pablo clearly know very well).

I beg your pardon I come of rude or too edgy, I didn't mean this as an attack at you personally but I find the approach you've taken to this patch very very troubling since instead of finding a common ground you are resorting to expressing power without any shown efforts to explore the subject beforehand.

Just an outside voice here: I think I agree that there should be some consistency and group discussion going on that hasn't happened as much with Pablo's work. I think it's easy to run with the excuse that "well, at least it's something!" and just let things slide in like this because you are so grateful for some progress. so, I do think it would benefit from some more conversion. One thing I see happening a lot is things getting named without talking to native English speakers first. "Face groups" my be what they are (just groups of faces) but it doesn't really describe what they actually are used for. I think "Visibility groups/sets" makes more sense to me. You can use them for other things like excluding other geo from some process or tool, but mainly it's used for keeping sets of geo to hide or reveal.

I can't be 100% objective since I've worked on facemaps, but this feels, smells, barks and looks like facemaps. In fact I came here expecting to see the code reusing them and was quite surprised that it didn't.

First, some explanation as to what a facemap is: Facemap is a concept that allows a face to belong to one, and only one, set of faces. This is different to vertex groups, where a vertex can belong to multiple sets, hence I thought that a change of name would illustrate this more clearly. In computer science, when we link a key to a value, it is a map, so "facemap" sounded a good idea to my old self.

I am not too attached to names though, which I find bike-shedding of the ultimate order. The key point here is that each face corresponds to one and only one set of faces.
And this is exactly what this patch tries to do. If I remember correctly, the initial design of facemaps even allows multiple facemaps per mesh, which can be quite useful for this feature.

I believe there are multiple positives in reusing facemaps/facegroups here. These groups can be reused for different things (imagine sculpting a shoe part on the mesh and reuse the same group as a bone group for rigging, for instance). Also I love some of the functionality in this patch (for instance coloring of face groups with different colors), which would be great to expose elsewhere, say in edit mode.

@Julian Eisel (Severin) Any date when this will be implemented? Face groups are very important to the sculpt process and I don't understand that we should be waiting months for something that is already implemented.

Agree, @Julian Eisel (Severin) @Jeroen Bakker (jbakker) this is a super important feature for the sculpt process, more even than for the edit mode process.

If something has to be modified to have this merged for 2.83 could be great to define it, the feature is there, it's working, if it has to use the default Face Maps and that's the only problem, then define it so @Pablo Dobarro (pablodp606) can modify it and get it ready for merge.

Please let's make this merged for 2.83 :)

This change has rather significant implications and needs broader discussion. Adding the UI team as blocking reviewer.
First off: I don't want to stop the progress - in principle this patch adds a hugely useful feature that would be great to have. It is more the process that is problematic, and it's a general issue we have coming with the 2.8 dynamics.

Afaik this has no further consequences in UI, so I don't understand why the UI team should be blocking it.
If you think there is a general issue with 2.8 dynamics, solve it and bring @Dalai Felinto (dfelinto) into this, but don't use this patch as an example please, because it's clear that this tool has something that usually is not present in other features development, a developer that is also an artist, not just an engineer, and this ensures a pretty good UX, specially because it's being tested at the same time it's being developed, of course it needs further testing, but so far all the people I showed and tried the feature are pretty happy as it is right now, as I said, it may need an internal modification to use Face Maps, but I'm not sure if this is good because performance of classic meshes with millions of polygons is super slow, so if performance is not good yet, this may be implemented as is, and interchange Face Groups with Face Maps when mesh performance is good enough.

I cannot understand this lock, because if this is the case many other patches should be locked, like the change @Brecht Van Lommel (brecht) just did removing Embree from the CMAKE options, it could seems a very simple change, but it has deeper implications since Embree may provoque problems with some addons, for example, I'm not saying it's not a good change, just that the logic followed to lock this patch does not make sense to me.

Or even Face Maps should not be there right now, because they are totally useless, they cannot be used with anything, it's just an isolated feature, the concept is awesome, but if it cannot be used in anything, why is it there? That feature should have been blocked by the UI team too, because it adds clutter to the UI, generates some expectations in the users that later cannot be fullfilled. Once again I like the feature, I think it should be there, but it's incomplete, if we follow the same logic as you stated here @Julian Eisel (Severin) many things should be blocked by the UI team.

Please guys, let's make an effort to unlock a feature that is very important for the sculpt process :)

Agree, @Julian Eisel (Severin) @Jeroen Bakker (jbakker) this is a super important feature for the sculpt process, more even than for the edit mode process.
If something has to be modified to have this merged for 2.83 could be great to define it, the feature is there, it's working, if it has to use the default Face Maps and that's the only problem, then define it so @Pablo Dobarro (pablodp606) can modify it and get it ready for merge.
Please let's make this merged for 2.83 :)

This change has rather significant implications and needs broader discussion. Adding the UI team as blocking reviewer.
First off: I don't want to stop the progress - in principle this patch adds a hugely useful feature that would be great to have. It is more the process that is problematic, and it's a general issue we have coming with the 2.8 dynamics.

Afaik this has no further consequences in UI, so I don't understand why the UI team should be blocking it.
If you think there is a general issue with 2.8 dynamics, solve it and bring @Dalai Felinto (dfelinto) into this, but don't use this patch as an example please, because it's clear that this tool has something that usually is not present in other features development, a developer that is also an artist, not just an engineer, and this ensures a pretty good UX, specially because it's being tested at the same time it's being developed, of course it needs further testing, but so far all the people I showed and tried the feature are pretty happy as it is right now, as I said, it may need an internal modification to use Face Maps, but I'm not sure if this is good because performance of classic meshes with millions of polygons is super slow, so if performance is not good yet, this may be implemented as is, and interchange Face Groups with Face Maps when mesh performance is good enough.
I cannot understand this lock, because if this is the case many other patches should be locked, like the change @Brecht Van Lommel (brecht) just did removing Embree from the CMAKE options, it could seems a very simple change, but it has deeper implications since Embree may provoque problems with some addons, for example, I'm not saying it's not a good change, just that the logic followed to lock this patch does not make sense to me.
Or even Face Maps should not be there right now, because they are totally useless, they cannot be used with anything, it's just an isolated feature, the concept is awesome, but if it cannot be used in anything, why is it there? That feature should have been blocked by the UI team too, because it adds clutter to the UI, generates some expectations in the users that later cannot be fullfilled. Once again I like the feature, I think it should be there, but it's incomplete, if we follow the same logic as you stated here @Julian Eisel (Severin) many things should be blocked by the UI team.
Please guys, let's make an effort to unlock a feature that is very important for the sculpt process :)

While i agree with many things you have mentioned, me testing the patch show that the tool is not ready yet, I would love to see Pablo tackling those visible problems, like those pointed in the video https://youtu.be/4i0cz0jgD0Y

Please let's keep code reviews focused on the code and functionality. If you want to discuss process, do it on blender.chat, bf-committers or devtalk, this is not the place.

like the change @Brecht Van Lommel (brecht) just did removing Embree from the CMAKE options, it could seems a very simple change, but it has deeper implications

It doesn't.

@William Reynish (billreynish) will be in Amsterdam end of this week, we (the UI-team) will look into a decision for this design then. We'll keep you posted.
Pablo and I discussed this further in the Blender chat a while ago. He was fine with pausing work on this patch for a bit, which is why there weren't any updates since then. I'll make sure that he can continue on this soon.

Note: Adding the UI team as blocking reviewer does not mean rejecting the feature at all. It just makes it explicit that the UI team has to sign off the changes before they get into master. We often imply that (probably because most contributors/devs don't know about the feature), sometimes it's better to make it explicit.

Last Friday there was a UI meeting. One of the topics was how what to do with this Sculpt Face Groups. Here is the relevant information from that discussion for this feature.

  • There was a discussion about this patch using a generic data structure (Face Mask) vs a specific data structure (Current implementation). We decided that for this feature we keep it to a sculpt specific feature and not use current the generic Face Masks. The reason behind it is that the use-case of the Face Masks data structure isn't clear.
  • In Blender we use the term Mask for mutual exclusive data. We use the term Groups for not mutual exclusive data (eg Vertex Groups). For this project it means that the term Group should be renamed to Mask eg Sculpt Face Mask. This also means renaming the variable and function names, display etc.
  • Rebase
  • Fix rendering
  • Rename to Face Masks
  • More renaming and bug fixing
  • Sculpt: Support for expand face mask on mask expand (Shift + W)

Pablo Dobarro (pablodp606) retitled this revision from Sculpt Face Groups to Sculpt Face Masks.Tue, Feb 25, 12:15 AM

Are you guys sure face masks is a good name? That's confusing imo, since regular masks already exists.
I'd go with "face sets" or something along those lines. They are polygon groups after all.

  • There was a discussion about this patch using a generic data structure (Face Mask) vs a specific data structure (Current implementation). We decided that for this feature we keep it to a sculpt specific feature and not use current the generic Face Masks. The reason behind it is that the use-case of the Face Masks data structure isn't clear.
  • In Blender we use the term Mask for mutual exclusive data. We use the term Groups for not mutual exclusive data (eg Vertex Groups). For this project it means that the term Group should be renamed to Mask eg Sculpt Face Mask. This also means renaming the variable and function names, display etc.

I thought the suggestion was Face Maps, not Face Masks? That's a bit confusing since mask is usually meant for things that are floats.

Face Sets is also fine with me (and leans closer to selection set which is a related to this). It just needs to be different from Group due to existing Blender terminology.

I don’t have a strong opinion for the naming in the UI, but it is true that using “face masks” in the sculpt mode code makes the variable and function names confusing, specially in functions that need to work with the mask and face masks at the same time.

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.Tue, Feb 25, 2:02 PM
Julian Eisel (Severin) requested changes to this revision.EditedTue, Feb 25, 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.Tue, Feb 25, 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.