Page MenuHome

Sculpt Face Groups
Needs ReviewPublic

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

Reviewers
Jeroen Bakker (jbakker)
Group Reviewers
User Interface
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 5390
Build 5390: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Pablo Dobarro (pablodp606) edited the summary of this revision. (Show Details)
Pablo Dobarro (pablodp606) edited the summary of this revision. (Show Details)
  • Fix bug

Wow, painting Sculpt Face Groups, that's very cool! 👏

It'd be fantastic to be able to smooth the borders as well. 🙏

Automatically generate Face Maps from Loose Parts, materials, surface curvature, UV islands ... I think it would be useful.

As Metin Seven says, an option to smooth the borders, even to flange them with a bevel, or block them would also be very good.

Flipo con cada commit, oooOOOHHHHUUUuuuu.

  • Rebase
  • Rename Facemap to Face Groups
Pablo Dobarro (pablodp606) retitled this revision from Sculpt Facemap to Sculpt Face Groups.Tue, Nov 26, 1:11 AM
Pablo Dobarro (pablodp606) edited the summary of this revision. (Show Details)
Jeroen Bakker (jbakker) requested changes to this revision.Wed, Nov 27, 10:46 AM
Jeroen Bakker (jbakker) added inline comments.
source/blender/blenkernel/BKE_pbvh.h
416

use BM_elem_flag_test_bool

source/blender/blenkernel/intern/customdata.c
1673

We should rename this to "CDSculptFaceGroup" for consistency.

source/blender/blenkernel/intern/paint.c
1229

face group

source/blender/blenkernel/intern/pbvh.c
1002

show_sculpt_face_groups

1175

unclear name could be is_visible or has_visibility

1182

We can break by the first hit. To break the iterator move to a utility function and break using a return.

1204

show_sculpt_face_groups

2753

pbvh_has_face_groups

source/blender/blenkernel/intern/pbvh_intern.h
139

face_group_seed

source/blender/draw/modes/sculpt_mode.c
42

rename the file to sculpt_face_groups_vert_glsl

101

This code needs to be adjusted to fit the structure of D6296: Overlay Engine: Refactor & Cleanup that will be merged in this week. I would say to adjust the sculpt face groups patch after that and we do not want to commit face groups before that patch.

184

would be better support a single overlay for masks and face groups and solve the composite in the vert shader. No need to draw twice. Especially as sculpt drawing currently is doing its own thing.

source/blender/draw/modes/shaders/sculpt_facemap_vert.glsl
3

use a more descriptive name for uniforms no need to shorten

source/blender/editors/object/object_remesh.c
148

I still think we should reduce the number of times that BKE_mesh_runtime_clear_geometry is called.

source/blender/editors/sculpt_paint/sculpt.c
217

Use SET_FLAG_FROM_TEST

285

This means that the face_groups[i] highest bit must never be used, otherwise this results to undefined behavior.
If this is the face_group_seed we should bit mask the seed when generated and add comments with the reason why.

We should document the ss->face_groups structure it seems to does some encoding, and I cannot tell that the encoding and decoding is applied correctly as I am assuming that it needs to be decoded but I don't see this in this patch.

325

See comment at sculpt_face_groups_visibility_all_set

351

Does this needs to be an absolute value?

3215

move out of the for loop

5470–5475

Isn't this supported, or does it use a different behavior. Anyhow we should at least comment why this is an exception

8531

This overwrites the previous one....MESH_FILTER_RELAX

8696

define this SCULPT_FACE_GROUP_INACTIVE and use this as a check elsewhere int the code?

10025

Naming should be clearer towards users.

From Mask, From Visible, Complete/Whole/Full Mesh

10125

Do we create a single or multiple face groups

10173

Update text

source/blender/editors/sculpt_paint/sculpt_intern.h
227

face_group

source/blender/editors/sculpt_paint/sculpt_undo.c
340

false

source/blender/gpu/intern/gpu_buffers.c
295

Gives a linear rec709 color,
Perhaps you wanted srgb

best way to do the conversion in the shader from linearrgb to srgb in the shader.
But this should also be clearly documented in the shader

source/blender/makesdna/DNA_customdata_types.h
78

pad can be removed?

source/blender/makesdna/DNA_mesh_types.h
198

face_group_color_seed

source/blender/makesdna/DNA_view3d_types.h
206

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

324

or remove _pad3

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

Not sure we need to mention Sculpt here.

This revision now requires changes to proceed.Wed, Nov 27, 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
285

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.Sun, Dec 1, 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
4884–4885

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.