Page MenuHome

Fix T78242: Crash when using a Sculpt color tools that needs connectivity for the first time
ClosedPublic

Authored by Pablo Dobarro (pablodp606) on Jun 26 2020, 9:08 PM.

Details

Summary

When there is no color layer available,
BKE_sculpt_update_object_for_edit creates a new one and tags the mesh
with ID_RECLAC_GEOMETRY, so this layer is inmediatly available when the
tool starts. This also deletes the PBVH and when it is created again in
BKE_sculpt_update_object_after_eval, the pmap is not initialized, making
the tool crash.

Most of the tools need the pmap, so I would rather have it always
initialized and remove all checks for pmap all over the code, (also,
multires and dyntopo have connectivity info always available), but I'm
not sure how doing this will affect performance/memory ussage. For now,
this always request the connectivity info when updating objects after
eval, so this should not happen again.

Diff Detail

Repository
rB Blender
Branch
T78242 (branched from master)
Build Status
Buildable 8735
Build 8735: arc lint + arc unit

Event Timeline

Pablo Dobarro (pablodp606) requested review of this revision.Jun 26 2020, 9:08 PM
Pablo Dobarro (pablodp606) created this revision.

To me it seems that the actual source of problem is that BKE_sculpt_update_object_for_edit() leaves object in a non-consistent state, despite the fact that it is expected that PBVH and session should be fully usable after the call of this function. It also feels a wrong place to perform object modifications.

Agree, this is working around the problem and the fix should address the cause directly.

  • Review Update: Add a separate function to create the color layer

With this fix it is necessary to also create the color layer before using BKE_sculpt_update_object_for_edit, but the mask datalayer creation (both for mesh and grids) is still handled in that function. I would say that this needs a bigger refactor after adding multires support to the color layers because this approach is causing other problems that are not related to this crash (for example, the mask data is only copied to the first level grids when using BKE_sculpt_update_object_for_edit for the first time, but it is never updated again)

Sergey Sharybin (sergey) requested changes to this revision.Thu, Jul 16, 10:58 AM
Sergey Sharybin (sergey) added inline comments.
source/blender/blenkernel/intern/paint.c
1687

This is a weird semantic. Ensure is "make sure", "guarantee". Conditional behavior based on experimental feature set enabled does not fit this definition.

Consider something like this instead:

/* Create new color layer on object if it doesn't have one and if experimental feature set has sculpt vertex color enabled.
 * Returns truth if new layer has been added, false otherwise. */
bool BKE_sculpt_color_layer_create_if_needed(Object *object)  {
}

P.S. After reading the rest of the patch, I don't see where the return value is used. Is still fine to have it, but can as well replace with void.

source/blender/editors/sculpt_paint/sculpt.c
7108–7112 ↗(On Diff #26833)

Add a note that CTX_data_ensure_evaluated_depsgraph is only to be used at the end to prop;erly update with possible earlier steps modifying original data.

This revision now requires changes to proceed.Thu, Jul 16, 10:58 AM
Pablo Dobarro (pablodp606) marked 2 inline comments as done.
  • Review Update: add comments
source/blender/editors/sculpt_paint/sculpt_filter_color.c
287 ↗(On Diff #26874)

This function is not defined in master and in this patch. Did you compile the patch?

289 ↗(On Diff #26874)

Can add same comment as above.

Pablo Dobarro (pablodp606) marked 2 inline comments as done.
  • Review update: missing file
This revision is now accepted and ready to land.Mon, Jul 20, 9:45 AM