Fix T95185: Invalid normals after undo in sculpt mode
ClosedPublic
Actions

Authored by Campbell Barton (campbellbarton) on Feb 1 2022, 1:56 AM.

Details

Reviewers

Hans Goudey (HooglyBoogly)

Maniphest Tasks

T95185: Invalid normals after undo in sculpt mode

Summary

Since d9c6ceb3b88b partial updates to normals in sculpt-mode were
accumulating into the current normal instead of a zeroed value.

Zero vertex normal values tagged for calculation before accumulation.

Notes:

This patch could allocate a new array as was done before, however it seems unnecessary as the existing memory can be used in-place.

Zeroing the normals could be multi-threaded, although as zeroing isn't an expensive operation. This raises the question of thread safety for assigning the same value to floats. From what I can tell this is technically undefined behavior, but works in practice (as long as threads complete before reading). I'd be interested to know if this is something done elsewhere in Blender (for non-byte values which we do already).

Diff Detail

Repository

rB Blender

Branch

TEMP-SCULPT-FIX-T95185 (branched from master)

Build Status

Buildable 20267
Build 20267: arc lint + arc unit

Event Timeline

Campbell Barton (campbellbarton) requested review of this revision.Feb 1 2022, 1:56 AM

Campbell Barton (campbellbarton) created this revision.

Harbormaster completed remote builds in B20262: Diff 47802.Feb 1 2022, 1:57 AM

Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)Feb 1 2022, 1:58 AM

Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)Feb 1 2022, 2:02 AM

Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)

Campbell Barton (campbellbarton) mentioned this in D13939: Fix T95185: Invalid normals after undo in sculpt mode.Feb 1 2022, 2:11 AM

Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)

Campbell Barton (campbellbarton) added a task: T95185: Invalid normals after undo in sculpt mode.Feb 1 2022, 2:15 AM

Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)Feb 1 2022, 2:26 AM

This is great, thanks a lot for looking into it. I'm a bit embarrassed that I latched onto the "undo" cause without actually testing the values in the array. A lesson learned there..

Accepting with a couple comments inline.

source/blender/blenkernel/intern/pbvh.c
1139–1147	I think this part would be simpler (and require loading less memory) if written like this (like in `pbvh_update_normals_store_task_cb`): const int verts = node->vert_indices; const int totvert = node->uniq_verts; for (int i = 0; i < totvert; i++) { const int v = verts[i]; MVert mvert = &pbvh->verts[v]; if (mvert->flag & ME_VERT_PBVH_UPDATE) { zero_v3(data.vnors[v]); } }
source/blender/editors/sculpt_paint/sculpt.c
181 ↗	(On Diff #47802)	A great cleanup here. But might it make sense to commit this separately? I don't think the changes depend on each other.

This revision is now accepted and ready to land.Feb 1 2022, 2:56 AM

Update with suggested improvements

Harbormaster completed remote builds in B20264: Diff 47804.Feb 1 2022, 3:29 AM

Use const mvert

Harbormaster completed remote builds in B20265: Diff 47805.Feb 1 2022, 3:31 AM

Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)Feb 1 2022, 3:41 AM

Campbell Barton (campbellbarton) added inline comments.Feb 1 2022, 3:46 AM

source/blender/blenkernel/intern/pbvh.c
1139–1147	Thanks, I hadn't touched sculpt code in a while and missed the possibility of looping over verts directly. In this case though, there are no problems accessing from multiple threads.