Page MenuHome

Sculpt: Topology automasking
ClosedPublic

Authored by Pablo Dobarro (pablodp606) on Sep 1 2019, 12:43 AM.
Tags
None
Subscribers
None
Tokens
"Love" token, awarded by johnsyed."Like" token, awarded by CandleComet."Love" token, awarded by TheAngerSpecialist."Love" token, awarded by brilliant_ape.

Details

Summary

The sculpt automasking feature assigns a factor to each vertex before starting the stroke. This can be used for isolating disconnected meshes, masking cavities, mesh boundary edges or creating topological falloffs.

This patch implements automasking in all brushes and topology automasking without topology falloff.

Diff Detail

Repository
rB Blender

Event Timeline

Brecht Van Lommel (brecht) requested changes to this revision.Sep 6 2019, 5:41 PM

I think you can do the automasking as part of tex_strength()? It seems to be missing now from these places:

  • do_topology_rake_bmesh_task_cb_ex: I think topology rake should only affect areas that the sculpt tool affected, so should take into account automask here too.
  • do_gravity_task_cb_ex: similar to topology rake, should affect same areas as the sculpt tool.
  • do_smooth_brush_multires_task_cb_ex: not supported for multires now, but should be ok to have it here anyway and do nothing.
source/blender/editors/sculpt_paint/sculpt.c
325

Mutex locks like this are very slow when you have many cores, especially on multi-socket system where synchronizing is costly. Doing this for all vertices in the radius can make the operation slower than using a single core.

Instead what I suggest is to compute the nearest vertex per PBVH node. And then at the end of this function compare it to the global nearest vertex. For that you can use a spin mutex, since this is a relatively fast operation.

For performance you could also track the nearest_vertex_distance_squared instead of recomputing the length every time, or at least used len_squared_v3v3.

363

for (int i = 0

367

len_squared_v3v3(location, vertex) < radius * radius

2051

This test is too slow to perform for every vertex. Is checking if (ss->cache->automask) enough, or can you set some other flag that is quick to check?

2078

Naming conventions: VertexTopologyIterator

2083

Add some comments in this function explaining what the various parts do, for example:

/* Add active vertex and symmetric vertices to queue. */
...
/* Flood fill automask to connected vertices. */
2102

Use less obscure name.

2107–2108

Can we put this condition in a utility function? I see it duplicated in a few places and it's hard to understand.

2109

Above it's (char)i, can we be consistent?

2126

Using a BLI_gsqueue for this is rather slow because of the amount of memory allocations it does, it's not implemented efficiently for this case.

BLI_stack will be more efficient here and the same amount of code.

2159–2162

Commented this already in other code reviews, but would be good to move these 4 lines into a utility function.

It can also be put in sculpt_topology_automasking_init, since it's only needed there, and not when BRUSH_AUTOMASKING_TOPOLOGY is off?

source/blender/makesrna/intern/rna_brush.c
1974

Topology automasking -> Topology Automasking

Automaks disconnected vertices -> Don't affect vertices not connected to the active vertex under the brush

This revision now requires changes to proceed.Sep 6 2019, 5:41 PM
source/blender/makesrna/intern/rna_brush.c
1974

Or avoiding the double negative: Affect only vertices connected to the active vertex under the brush

Pablo Dobarro (pablodp606) marked 13 inline comments as done.
  • Review update
Brecht Van Lommel (brecht) added inline comments.
source/blender/editors/sculpt_paint/sculpt.c
1170

tool requires it*/ -> tool requires it. */

source/blender/makesrna/intern/rna_brush.c
1599–1602

This is unused so might give a compiler warning, leave out of the commit.

This revision is now accepted and ready to land.Sep 8 2019, 11:41 AM
This revision was automatically updated to reflect the committed changes.