Details

Reviewers

Sergey Sharybin (sergey)
Brecht Van Lommel (brecht)

Maniphest Tasks

T78242: Sculpt - smear brush crash

Commits

rB4f3233dd536e: Fix T78242: Crash when using a Sculpt color tools that needs connectivity for…

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

Event Timeline

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

Pablo Dobarro (pablodp606) created this revision.

Pablo Dobarro (pablodp606) added a reviewer: Sergey Sharybin (sergey).Jun 26 2020, 9:09 PM

Harbormaster completed remote builds in B8735: Diff 26326.Jun 26 2020, 11:35 PM

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)

Harbormaster completed remote builds in B9023: Diff 26833.Jul 15 2020, 6:02 PM

Sergey Sharybin (sergey) requested changes to this revision.Jul 16 2020, 10:58 AM

Sergey Sharybin (sergey) added inline comments.

source/blender/blenkernel/intern/paint.c
1680	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
7166–7170	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.