Sculpt: Reuse existing mesh triangles cache in sculpt mode #123638

Hans Goudey · 2024-06-23T14:57:29+02:00

Hans Goudey commented

2024-06-23 14:57:29 +02:00

Addresses #121240.
Instead of allocating a new array and recalculating mesh triangulation
every time the user enters sculpt mode, reuse the mesh's existing cache.
Currently in order to avoid recalculating triangulation on every brush update
(which would typically be necessary because the triangulation direction
depends on vertex positions), add a mechanism to "freeze" the cache to
skip recalculations until the user exits sculpt mode. That even avoids
recalculation if vertex positions aren't affected. This is necessary because
we can't use the cache in a dirty state; tagging the cache dirty frees the
triangulation array.

Removing the duplicate triangles array reduces memory usage by 384 MB
in a 16 million vertex sculpt, and makes entering sculpt mode 125ms faster
(tested on a Ryzen 7840u).

In the long term, I hope we find a different solution that's a bit more
transparent and hopefully more integrated with the caching system
in general. In the meantime, this is a relatively safe low impact change
that helps document the needs for such a system anyway.

Addresses #121240. Instead of allocating a new array and recalculating mesh triangulation every time the user enters sculpt mode, reuse the mesh's existing cache. Currently in order to avoid recalculating triangulation on every brush update (which would typically be necessary because the triangulation direction depends on vertex positions), add a mechanism to "freeze" the cache to skip recalculations until the user exits sculpt mode. That even avoids recalculation if vertex positions aren't affected. This is necessary because we can't use the cache in a dirty state; tagging the cache dirty frees the triangulation array. Removing the duplicate triangles array reduces memory usage by 384 MB in a 16 million vertex sculpt, and makes entering sculpt mode 125ms faster (tested on a Ryzen 7840u). In the long term, I hope we find a different solution that's a bit more transparent and hopefully more integrated with the caching system in general. In the meantime, this is a relatively safe low impact change that helps document the needs for such a system anyway.

🎉 3

Hans Goudey added 1 commit 2024-06-23 14:57:38 +02:00

Sculpt: Reuse existing mesh triangles cache in sculpt mode

buildbot/vexp-code-patch-lint Build done.

Details

buildbot/vexp-code-patch-linux-x86_64 Build done.

Details

buildbot/vexp-code-patch-darwin-x86_64 Build done.

Details

buildbot/vexp-code-patch-darwin-arm64 Build done.

Details

buildbot/vexp-code-patch-windows-amd64 Build done.

Details

buildbot/vexp-code-patch-coordinator Build done.

Details

a1c9b8a85b

Addresses #121240.
Instead of allocating a new array and recalculating mesh triangulation
every time the user enters sculpt mode, reuse the existing cache. In
order to avoid recalculating triangulation on every brush update (which
would typically be necessary because the triangulation direction depends
on vertex positions), add a mechanism to "freeze" the cache to skip
recalculations until the user exits sculpt mode. That even avoids
recalculation if vertex positions aren't affected.

This reduces memory usage by 384 MB in a 16 million vertex sculpt, and
makes entering sculpt mode 125ms faster (tested on a Ryzen 7840u).

Hans Goudey added this to the Sculpt, Paint & Texture project 2024-06-23 14:57:52 +02:00

Hans Goudey commented

2024-06-23 14:57:57 +02:00

@blender-bot build

Hans Goudey requested review from Sergey Sharybin 2024-06-23 16:42:34 +02:00

Hans Goudey requested review from Jacques Lucke 2024-06-23 16:42:35 +02:00

Sergey Sharybin requested changes 2024-06-24 12:23:02 +02:00

Dismissed

Sergey Sharybin left a comment

I am confused by the explanation in the current code, and in this PR.

I don't understand how we can re-calculate triangulation of a mesh without changing anything else in the BVH. So the related part of the current comment /* Owned by the #PBVH, because after deformations they have to be recomputed. */ seems odd. I also don't see any calls to the corner_tris_calc_impl when sculpting on a base mesh (without modifiers). So do we really re-calculate anything?

The part in the new code around

  /* Re-triangulating the mesh for position changes in sculpt mode isn't worth the performance
   * impact, so delay triangulation updates until the user exits sculpt mode. */

has a couple of issues (at least from the wording and presentation perspectives):

It is not only about performance. Having difference in shading between sculpt mode and object node is not to be taken lightly.
From the sounds of it, one would expect performance improvement during the stroke, as something is delayed until exiting the sculpt mode. But the description of the PR does not mention anything about it. And for the non-deformed base mesh sculpting triangulation is not happening anyway.
For the sculpting on the deformed mesh the weird thing is setting this flag on the original mesh.

So for the base mesh sculpting what this change effectively is doing is use pre-calculated triangles from the input mesh, and that where the speedup is coming from. To achieve this speedup we can just use the input mesh triangulation in the PBVH.
The bigger unknown impact is the fact that the PBVH is referencing the array now. Ignoring the deformed mesh case it seems we can remove this field from PBVH, and pass it along just like corner_tris or corner_tri_faces.
For the sculpting on the deformed mesh the triangulation is strange already, as it triangulation from the non-deformed mesh. So, perhaps the strategy from above will also work for the deformed mesh sculpting as well.

But all that leads me to thinking: why do we even need this "freeze" idea for triangulation? What cases it covers which are not covered with a simpler approach from above?

I am confused by the explanation in the current code, and in this PR. I don't understand how we can re-calculate triangulation of a mesh without changing anything else in the BVH. So the related part of the current comment `/* Owned by the #PBVH, because after deformations they have to be recomputed. */` seems odd. I also don't see any calls to the `corner_tris_calc_impl` when sculpting on a base mesh (without modifiers). So do we really re-calculate anything? The part in the new code around ``` /* Re-triangulating the mesh for position changes in sculpt mode isn't worth the performance * impact, so delay triangulation updates until the user exits sculpt mode. */ ``` has a couple of issues (at least from the wording and presentation perspectives): - It is not only about performance. Having difference in shading between sculpt mode and object node is not to be taken lightly. - From the sounds of it, one would expect performance improvement during the stroke, as something is delayed until exiting the sculpt mode. But the description of the PR does not mention anything about it. And for the non-deformed base mesh sculpting triangulation is not happening anyway. - For the sculpting on the deformed mesh the weird thing is setting this flag on the original mesh. So for the base mesh sculpting what this change effectively is doing is use pre-calculated triangles from the input mesh, and that where the speedup is coming from. To achieve this speedup we can just use the input mesh triangulation in the PBVH. The bigger unknown impact is the fact that the PBVH is referencing the array now. Ignoring the deformed mesh case it seems we can remove this field from PBVH, and pass it along just like `corner_tris` or `corner_tri_faces`. For the sculpting on the deformed mesh the triangulation is strange already, as it triangulation from the non-deformed mesh. So, perhaps the strategy from above will also work for the deformed mesh sculpting as well. But all that leads me to thinking: why do we even need this "freeze" idea for triangulation? What cases it covers which are not covered with a simpler approach from above?

source/blender/blenkernel/intern/pbvh.cc Outdated

						
				@ -656,4 +656,3 @@

				  const int totvert = mesh->verts_num;

				  const int corner_tris_num = poly_to_tri_count(mesh->faces_num, mesh->corners_num);

				  MutableSpan<float3> vert_positions = mesh->vert_positions_for_write();

				  const OffsetIndices<int> faces = mesh->faces();

Sergey Sharybin commented

2024-06-24 11:41:34 +02:00

const OffsetIndices<int> faces = mesh->faces(); is now unused.

`const OffsetIndices<int> faces = mesh->faces();` is now unused.

HooglyBoogly marked this conversation as resolved

Hans Goudey added 2 commits 2024-06-24 14:06:47 +02:00

Merge branch 'main' into sculpt-triangles-freeze 6b25515200

Cleanup: Remove unused variable d8c3895549

Hans Goudey commented

2024-06-24 14:13:05 +02:00

Thanks for the in-depth comment. I think you're misunderstanding though, thinking that we are currently recalculating triangles during sculpting. We aren't, the triangulation is unchanged after brush strokes. This PR is about maintaining that fact without the need to duplicate the triangles into a separate array just for the PBVH/sculpt mode. It's implementing 3.iii. from #121240.

It is not only about performance. Having difference in shading between sculpt mode and object node is not to be taken lightly.

Right, I wouldn't take changing that lightly, but that's already how it is.

For the sculpting on the deformed mesh the weird thing is setting this flag on the original mesh.

Not so weird IMO. The PBVH draws data from the original mesh with positions retrieved from the evaluated mesh. And sculpting modifies the original mesh rather than the evaluated mesh.

The bigger unknown impact is the fact that the PBVH is referencing the array now. Ignoring the deformed mesh case it seems we can remove this field from PBVH, and pass it along just like corner_tris or corner_tri_faces.

Yes, removing the reference from the PBVH is the next step, but I'd commit that separately, it makes the change in this PR less clear.

But all that leads me to thinking: why do we even need this "freeze" idea for triangulation? What cases it covers which are not covered with a simpler approach from above?

Not sure what simpler approach you're describing. I think it comes from the same misunderstanding/miscommunication?

Thanks for the in-depth comment. I think you're misunderstanding though, thinking that we are currently recalculating triangles during sculpting. We aren't, the triangulation is unchanged after brush strokes. This PR is about _maintaining_ that fact without the need to duplicate the triangles into a separate array just for the PBVH/sculpt mode. It's implementing 3.iii. from #121240. > - It is not only about performance. Having difference in shading between sculpt mode and object node is not to be taken lightly. Right, I wouldn't take changing that lightly, but that's already how it is. > - For the sculpting on the deformed mesh the weird thing is setting this flag on the original mesh. Not so weird IMO. The PBVH draws data from the original mesh with positions retrieved from the evaluated mesh. And sculpting modifies the original mesh rather than the evaluated mesh. > The bigger unknown impact is the fact that the PBVH is referencing the array now. Ignoring the deformed mesh case it seems we can remove this field from PBVH, and pass it along just like `corner_tris` or `corner_tri_faces`. Yes, removing the reference from the PBVH is the next step, but I'd commit that separately, it makes the change in this PR less clear. > But all that leads me to thinking: why do we even need this "freeze" idea for triangulation? What cases it covers which are not covered with a simpler approach from above? Not sure what simpler approach you're describing. I think it comes from the same misunderstanding/miscommunication?

Sergey Sharybin commented

2024-06-24 17:09:39 +02:00

think you're misunderstanding though, thinking that we are currently recalculating triangles during sculpting

This is not what I am thinking.
What I am thinking is that:

Recalculation of triangulation would be quite strange from the BVH perspective
Is not something I see in the debugger
Comments in the old code, new code, and the PR description makes it sound that we do re-calculate triangulation on brush updates:

Old code:
/* Owned by the #PBVH, because after deformations they have to be recomputed. */

New code:
/* Re-triangulating the mesh for position changes in sculpt mode isn't worth the performance
 * impact, so delay triangulation updates until the user exits sculpt mode. */

Description:
In order to avoid recalculating triangulation on every brush update

And now the comment:
I think you're misunderstanding though, thinking that we are currently recalculating triangles during sculpting. We aren't, the triangulation is unchanged after brush strokes.

To me those statements do not add up in a coherent state of the code and the change.

Not sure what simpler approach you're describing. I think it comes from the same misunderstanding/miscommunication?

Simpler solution: remove PBVH::corner_tris; WITHOUT adding the freezing mechanism.

> think you're misunderstanding though, thinking that we are currently recalculating triangles during sculpting This is not what I am thinking. What I am thinking is that: - Recalculation of triangulation would be quite strange from the BVH perspective - Is not something I see in the debugger - Comments in the old code, new code, and the PR description makes it sound that we do re-calculate triangulation on brush updates: ``` Old code: /* Owned by the #PBVH, because after deformations they have to be recomputed. */ New code: /* Re-triangulating the mesh for position changes in sculpt mode isn't worth the performance * impact, so delay triangulation updates until the user exits sculpt mode. */ Description: In order to avoid recalculating triangulation on every brush update And now the comment: I think you're misunderstanding though, thinking that we are currently recalculating triangles during sculpting. We aren't, the triangulation is unchanged after brush strokes. ``` To me those statements do not add up in a coherent state of the code and the change. > Not sure what simpler approach you're describing. I think it comes from the same misunderstanding/miscommunication? Simpler solution: remove `PBVH::corner_tris;` WITHOUT adding the freezing mechanism.

Hans Goudey commented

2024-06-24 18:29:33 +02:00

From talking in a video call, the missing bit of information was that it isn't possible to safely continue using the triangulation cache after tagging it dirty. SharedCache::tag_dirty() is implementing by deleting the data, which un-shares it. I rephrased the PR description to hopefully make that clearer. We hack around this with the update() function for normals, which need to be updated during sculpting. But in this case it's better to keep using the current tag_positions_changed and not introduce more complexity to the more public mesh API.

From talking in a video call, the missing bit of information was that it isn't possible to safely continue using the triangulation cache after tagging it dirty. `SharedCache::tag_dirty()` is implementing by deleting the data, which un-shares it. I rephrased the PR description to hopefully make that clearer. We hack around this with the `update()` function for normals, which need to be updated during sculpting. But in this case it's better to keep using the current `tag_positions_changed` and not introduce more complexity to the more public mesh API.

Hans Goudey requested review from Sergey Sharybin 2024-06-24 18:29:41 +02:00

Sergey Sharybin approved these changes 2024-06-25 14:57:45 +02:00

Sergey Sharybin left a comment

Thanks for the clarification, it helps a lot!

Hans Goudey merged commit 59d6eae116 into main

2024-06-26 03:05:06 +02:00

Hans Goudey referenced this issue from a commit

2024-06-26 03:05:08 +02:00

Sculpt: Reuse existing mesh triangles cache in sculpt mode

Hans Goudey deleted branch sculpt-triangles-freeze

2024-06-26 03:05:10 +02:00

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Sculpt: Reuse existing mesh triangles cache in sculpt mode #123638