Page MenuHome

Fix T78747: Fix mesh boundary detection and automasking
ClosedPublic

Authored by Pablo Dobarro (pablodp606) on Fri, Jul 10, 2:22 AM.

Details

Summary

This issue was produced by a hack in the sculpt mode code from 2.80 when the sculpt API for connectivity info was not available. The smooth brush was the only brush that needed connectivity info, so there were 3 different smooth functions with the connectivity queries implemented for dyntopo, meshes and grids. The mesh version of smoothing was checking the number of connected faces to a vertex to mask the mesh boundaries, which was not covering all cases and was hardcoded in the smooth function itself.

This patch removes all those legacy functions and unifies all smooth functions into a single one using the new API and the automasking system. In order to achieve this, there were needed some extra changes:

  • The smooth brush now does not automasks the boundaries by default, so its default preset needs to be updated to enable automasking
  • The mesh boundary info is extracted using BMesh and cached int a bitmap, similar to the disconnected elements IDs. This makes boundary detection work as expected in all cases, solving a lot of known issues with the smooth brush. In multires, this info is extracted and cached only at the base mesh level, so it is much more memory efficient than the previous automasking system.
  • In order to keep the brushes responsive as they were before, the automasking system can now skip creating the cache when it is not needed for the requested options. This means that for high poly meshes and simple automasking options the brushes won't lag on start.

Diff Detail

Repository
rB Blender

Event Timeline

Pablo Dobarro (pablodp606) requested review of this revision.Fri, Jul 10, 2:22 AM
Sergey Sharybin (sergey) requested changes to this revision.Mon, Jul 13, 9:44 AM
Sergey Sharybin (sergey) added inline comments.
source/blender/editors/sculpt_paint/sculpt_automasking.c
115–128

Reverse the condition, unindent the body.

312–324

You can simplify all this by doing something like this:

int SCULPT_automasking_mode_effective_bits(const Sculpt *sculpt, const Brush *brush) {
  return sculpt->automasking_flags | brush->automasking_flags;
}

....

ss->cache->automask_settings.flags = SCULPT_automasking_mode_effective_bits(sculpt, brush);

After doing this, I'd also say that SCULPT_automasking_needs_cache can become more clean:

const int automasking_flags = SCULPT_automasking_mode_effective_bits(...);
if (automasking_flags & FOO) {
  ...
}
if (automasking_flags & BAR) {
  ..
}
333–338
  • Use MEM_calloc_arrayN / MEM_malloc_arrayN
  • Don't use calloc semantic if you're fully initializing the memotry.
  • You are allocating SCULPT_vertex_count_get(ss) elements but iterating over totvert elements. While it is fine in this case because they are the same value, it is still confusing to read ans is more error-prone.
source/blender/editors/sculpt_paint/sculpt_filter_mesh.c
184

Always make no-functional-changes in the separate commit.

There is no need to introduce functional changes to qualify for making a change which makes naming more generic and prepares foundation for the further development.

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

If it is a single bit, use typed enum. If it is a bitmask, call it flags.

870

This field is inside of StrokeCache, making you to access cache->automask_cache which is redundant.

It is more interesting to know:

  • Who owns this memory.
  • What it is indexed with, what are the elements.

Based on what elements are you can also choose better naming, which will facilitate understanding what you're accessing without jumping to the declaration with comment,

Generally follow the rule: code should be locally readable and understandable (by others and future-you).

source/blender/editors/sculpt_paint/sculpt_smooth.c
83

You do not need to cast both operands for floating point division. 1.0 / int will give you what you want.

This revision now requires changes to proceed.Mon, Jul 13, 9:44 AM
Pablo Dobarro (pablodp606) marked 7 inline comments as done.
Pablo Dobarro (pablodp606) edited the summary of this revision. (Show Details)
  • Review update
Sergey Sharybin (sergey) requested changes to this revision.Tue, Jul 14, 12:36 PM

Please treat BMesh creation as something absolutely expensive, similar to designing and sending new rocket to space. It does a lot of allocations, bumping the peak memory a lot, and is fdoing a lot of other initializations.

Here if you really can not re-use any existing information from Sculpt, you'd better allocate one single array indexed by edge index, where element indicates how many faces are adjacent to this edge, then loop faces and their loops once, updating elements of this array. Then loop through edges and if the array element is one, tag edge vertices as boundary.

Since there is more information stored in Sculpt, you can think of using adjacency and/or pmap information. Maybe that could be used as well, to avoid any extra temporary allocations/initializaitons (assuming, they are available at the time you need this function to run).

This revision now requires changes to proceed.Tue, Jul 14, 12:36 PM
  • Review Update: Create boundary info without bmesh
source/blender/editors/sculpt_paint/sculpt_filter_mesh.c
533–537

const

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

SculptSession can be const.

741

Add a note that flags are from eAutomasking_flag.

Pablo Dobarro (pablodp606) marked 3 inline comments as done.
  • Review update
This revision is now accepted and ready to land.Wed, Jul 15, 4:25 PM

@Sergey Sharybin (sergey) Not related to this patch, but if the Mesh - BMesh conversion is that expensive maybe we can check the main unsubdivide function because it does the conversion multiple times just to use the bmesh iterators