Page MenuHome

Fix T78665: Face Set visibility reverted when chaning Multires Levels

Authored by Pablo Dobarro (pablodp606) on Jul 6 2020, 6:10 PM.



Face Sets where only set and updated on the PBVH after starting a sculpt
tool. In order to preserve the visibility they store when changing
levels, they need to be set and updated also on PBVH creation

Diff Detail

rB Blender
T78665 (branched from master)
Build Status
Buildable 9207
Build 9207: arc lint + arc unit

Event Timeline

Pablo Dobarro (pablodp606) requested review of this revision.Jul 6 2020, 6:11 PM
Sergey Sharybin (sergey) requested changes to this revision.Jul 7 2020, 10:11 AM
Sergey Sharybin (sergey) added inline comments.

Why subdiv_ccg is to be set when there are face sets?

395 ↗(On Diff #26562)

Expand the macro, makes it more readable: pbvh->verts[i].flag |= ME_HIDE

403 ↗(On Diff #26562)

Same as above (use different boolean bit operation though ;)

This revision now requires changes to proceed.Jul 7 2020, 10:11 AM
Pablo Dobarro (pablodp606) marked 3 inline comments as done.
  • Review Update

I merged all face set visibility sync functionality into the BKE_sculpt_sync_face_set_visibility function. This ensures the visibility of the face sets, mesh and grids is always consistent across all levels including level 0. Also, this function runs without the PBVH, so visibility can be updated before building it. This way we ensure that the PBVH always builds with the updated visibility state and visibility flags are set correctly when building the leaf nodes.


Split into two private functions called from here. One to update mesh, one to update CCG.
Localizes logic, avoids returns from the middle of the code flow.


This seems to be different from the Mesh case. For meshes face visibility is same as face set visibility.
Here grids are only becoming hidden and never becoming visible again.


Doesn't seem you're ever assigning it to subdiv_ccg->grid_hidden[i].

It also feels like such manipulation should become more accessible via SubbdivCCG API. For example, could be BKE_subdiv_ccg_grid_hidden_ensure();

Pablo Dobarro (pablodp606) marked 3 inline comments as done.
  • Review Update

const bool is_hidden = (face_sets[face_index] < 0)

And then single loop with BLI_BITMAP_SET(gh, y * gridsize + x, is_hidden);

Pablo Dobarro (pablodp606) marked an inline comment as done.
  • Review update

Seems something got lost here?

It is now indeed a single loop, which is good. But if face is visible it's possible that the loop below will write to a non-existing grid.

Pablo Dobarro (pablodp606) marked an inline comment as done.
  • Review Update

Now when looking into other areas how commonly the optimization of unused hidden grids is used, some deeper can of worms is opened. The subdiv_ccg_alloc_elements in subdiv_ccg.c allocates all grids, so the "sparse" nature of grids memory optimization doesn't really work.

To follow the old code, we can simply remove allocation of hidden grids in subdiv_ccg_alloc_elements. However, this has implications. Points to keep in mind:

  • I do not see code which would be de-allocating fully visible grids. So toggling visibility will make it so all hidden grids bitmasks are pre-allocated (the "sparse" memory optimization is lost).
  • It might actually be more memory efficient to allocate hidden grid bitmask as one continuous array: less prone to fragmentation, less slop memory used caused by padding and alignment. Cache coherency might also be a factor here.
  • Some code might get simplified by assuming hidden grids are always available.
  • Some latency might get reduced by avoiding on-demand allocation.

Surely, this is not to be addressed by this patch, is just something I've noticed while verifying the patch. However, there are decisions to be made about the memory management of hidden grids.

For this patch I suggest to address the final pass of cosmetic changes (didn't notice we have BLI_bitmap_set_all before), and put it to master. The memory topic worry about separately.




You can make it even more clear and explicit by continuing the sentence with something like: , this is because missing grid_hidden implies grid is fully visible.

See the non-inlined comment for further thoughs.


Should be able to replace with BLI_bitmap_set_all(gh, is_hidden, grid_area);

We need to consider that the only reason we are supporting per grid element visibility is to support the box hide and mask hide tools, all of the new tools work using the face sets of the base mesh. If we don't support per grid element visibility, we can make box hide and mask hide work with face sets also in Multires and then remove all sync functions and code that checks if an element is visibility or not depending on the state of the face sets and the grid_hidden data.
Doing that may be a problem if you are starting from a low resolution base mesh because you won't have enough resolution to control which parts are hidden (using multires in a plane with one face, for example), but given how multires works, sculpt mode with multires won't be really useful in that case as there won't be enough nodes, so performance is going to be terrible.

If we want to keep this functionality then I would rather have a single bitmap that is created only when box hide and mask hide is used. So, if these tools are never used, the multires visibility will be only stored and checked using the face sets and per element visibility will be checked only if it exists.

Pablo Dobarro (pablodp606) marked 2 inline comments as done.
  • Review update

Don't copy-paste ID of allocation, it is supposed to be unique.

Use any of the approach from those:

subdiv_ccg->grid_hidden[grid_index] = BLI_BITMAP_NEW(key.grid_area, "ccg_grid_hidden");


subdiv_ccg->grid_hidden[grid_index] = BLI_BITMAP_NEW(key.grid_area, __func__);
Pablo Dobarro (pablodp606) marked an inline comment as done.
  • Review Update
This revision is now accepted and ready to land.Jul 28 2020, 9:17 AM