Cleanup: Sculpt: Avoid unnecessarily changing vertex normals #111470

Hans Goudey · 2023-08-24T14:38:27+02:00

Hans Goudey commented

2023-08-24 14:38:27 +02:00

Keeping a mutable reference to vertex normals for the entire lifetime
of the PBVH structure makes caching the normals and sharing the cache
harder than it should be. Generally code is safer when we reduce the
number of mutable references to an object.

Currently the normals are modified in two places. First is the sculpt
mesh normal recalculation. There we can just retrieve the normals from
the mesh each time. Second is the restore from an undo step. That is
unnecessary because the normals are marked for recalculation anyway.
It doesn't even make much sense to store the normals in an undo step
when we can easily recalculate them based on new positions.

This change helps with #110479. These were also the last place that
kept a mutable reference to normals. I tested undo and redo after
sculpting, and it works well for each PBVH type.

Keeping a mutable reference to vertex normals for the entire lifetime of the PBVH structure makes caching the normals and sharing the cache harder than it should be. Generally code is safer when we reduce the number of mutable references to an object. Currently the normals are modified in two places. First is the sculpt mesh normal recalculation. There we can just retrieve the normals from the mesh each time. Second is the restore from an undo step. That is unnecessary because the normals are marked for recalculation anyway. It doesn't even make much sense to store the normals in an undo step when we can easily recalculate them based on new positions. This change helps with #110479. These were also the last place that kept a mutable reference to normals. I tested undo and redo after sculpting, and it works well for each PBVH type.

Hans Goudey added 1 commit 2023-08-24 14:38:37 +02:00

9f25542080 Cleanup: Sculpt: Avoid unnecessarily changing vertex normals

Keeping a mutable reference to vertex normals for the entire lifetime
of the PBVH structure makes caching the normals and sharing the cache
harder than it should be. Generally code is safer when we reduce the
number of mutable references to an object.

Currently the normals are modified in two places. First is the sculpt
mesh normal recalculation. There we can just retrieve the normals from
the mesh each time. Second is the restore from an undo step. That is
unnecessary because the normals are marked for recalculation anyway.
It doesn't even make much sense to store the normals in an undo step
when we can easily recalculate them based on new positions anyway.

This change helps with #110479. These were also the last place that
kept a mutable reference to normals. I tested undo and redo after
sculpting, and it works well for each PBVH type.

Hans Goudey requested review from Sergey Sharybin 2023-08-24 14:38:51 +02:00

Hans Goudey requested review from Jacques Lucke 2023-08-24 14:38:51 +02:00

Sergey Sharybin commented

2023-08-24 15:10:59 +02:00

That is unnecessary because the normals are marked for recalculation anyway.

I can only clearly see this done when sculpting operates on a base mesh.
It is not really clear to me what ensures validity of normals after undo for multires and dynamic topology sculpting.

This part needs to be clarified, as locally the proposed state of the undo code seems wrong.

> That is unnecessary because the normals are marked for recalculation anyway. I can only clearly see this done when sculpting operates on a base mesh. It is not really clear to me what ensures validity of normals after undo for multires and dynamic topology sculpting. This part needs to be clarified, as locally the proposed state of the undo code seems wrong.

Hans Goudey commented

2023-08-24 15:47:06 +02:00

Dynamic topology and multires sculpting just update the normals of entire PBVH nodes (see BKE_pbvh_update_normals), so in this case the call to BKE_pbvh_node_mark_update is enough to recalculate all the necessary normals.

Dynamic topology and multires sculpting just update the normals of entire PBVH nodes (see `BKE_pbvh_update_normals`), so in this case the call to `BKE_pbvh_node_mark_update` is enough to recalculate all the necessary normals.

Sergey Sharybin commented

2023-08-24 15:54:16 +02:00

Ah, indeed, I see it now. Thanks for clarifying.

The remaining confusion from my side is the assignment of pbvh->vert_normals in the normals_update function. Why is it needed now?

From testing and rest of the code it seems all fine though.

Ah, indeed, I see it now. Thanks for clarifying. The remaining confusion from my side is the assignment of pbvh->vert_normals in the normals_update function. Why is it needed now? From testing and rest of the code it seems all fine though.

Hans Goudey commented

2023-08-24 15:56:11 +02:00

The remaining confusion from my side is the assignment of pbvh->vert_normals in the normals_update function. Why is it needed now?

Good question! I was thinking of reassigning the data from the runtime vector, but it shouldn't be reallocated here, so that's unnecessary, I'll remove it.

Thanks for the quick review!

>The remaining confusion from my side is the assignment of pbvh->vert_normals in the normals_update function. Why is it needed now? Good question! I was thinking of reassigning the data from the runtime vector, but it shouldn't be reallocated here, so that's unnecessary, I'll remove it. Thanks for the quick review!

👍 1

Hans Goudey added 2 commits 2023-08-24 15:57:24 +02:00

cc4ce957c5 Merge branch 'main' into remove-sculpt-normal-reference

2b2a1d61b2 Remove unnecessary reassignment of normals spans

Sergey Sharybin approved these changes 2023-08-24 15:58:22 +02:00

Hans Goudey merged commit 111e586424 into main

2023-08-24 16:48:02 +02:00

Hans Goudey referenced this issue from a commit

2023-08-24 16:48:03 +02:00

Cleanup: Sculpt: Avoid unnecessarily changing vertex normals

Hans Goudey deleted branch remove-sculpt-normal-reference

2023-08-24 16:48:04 +02:00