Mesh: Share normals caches by splitting vertex and face calculation #110479

Hans Goudey · 2023-07-25T22:51:20+02:00

Hans Goudey commented

2023-07-25 22:51:20 +02:00

Since vertex and face normals can be calculated separately, it simplifies
things to further separate the two caches. This makes it easier to use
SharedCache to avoid recalculating normals when copying meshes.

Sharing vertex normal caches with meshes with the same positions and
topology allows completely skipping recomputation as meshes are
copied. The effects are similar to e8f4010611, but normals are much
more expensive, so the benefit is larger.

In a simple test changing a large grid's generic attribute with geometry
nodes, I observed a performance improvement from 12 to 17 FPS.
Most real world situations will have smaller changes though.

Completely splitting face and vertex calculation is slightly slower
when face normals aren't already calculated, so I kept the option
to recalculate them together as well.

This simplifies investigating the changes in #105920 which resolve
non-determinism in the vertex normal calculation. If we can make the
topology map creation fast enough, that might allow simplifying this
code more in the future.

Since vertex and face normals can be calculated separately, it simplifies things to further separate the two caches. This makes it easier to use `SharedCache` to avoid recalculating normals when copying meshes. Sharing vertex normal caches with meshes with the same positions and topology allows completely skipping recomputation as meshes are copied. The effects are similar to e8f4010611e7, but normals are much more expensive, so the benefit is larger. In a simple test changing a large grid's generic attribute with geometry nodes, I observed a performance improvement from 12 to 17 FPS. Most real world situations will have smaller changes though. Completely splitting face and vertex calculation is slightly slower when face normals aren't already calculated, so I kept the option to recalculate them together as well. This simplifies investigating the changes in #105920 which resolve non-determinism in the vertex normal calculation. If we can make the topology map creation fast enough, that might allow simplifying this code more in the future.

Hans Goudey force-pushed mesh-vert-face-normals-split from 5820eb501c to 1b5f7de7c2

2023-07-27 14:14:10 +02:00

Compare

Hans Goudey added 2 commits 2023-08-12 07:50:08 +02:00

8f87226e21 Merge branch 'main' into mesh-vert-face-normals-split

a3f65c3265 Use shared caches for mesh normals

Hans Goudey changed title from ~~Mesh: Split vertex and face normal calculation~~ to Mesh: Share normals caches by splitting vertex and face calculation

2023-08-12 07:50:19 +02:00

Jacques Lucke commented

2023-08-23 11:18:03 +02:00

Generally seems fine, but would of course be fine if we could avoid the slowdown if possible. Right now, it's not clear to me if it's at all possible to have no slowdown while still making normal calculation deterministic like you did in #105920. It would be good if you could describe a bit better why there is non-determinism in the first place and how it impacts other parts of Blender.

Ignoring the non-determinism issue, it seems like it might be possible to keep the same performance while still having two separate shared caches, at the cost of a bit more complexity. This could be achieved by calling face_normals_cache.ensure directly from Mesh::vert_normals(). Then one could have a specialized implementation that computes the vertex and face normals at the same time if the face normals are not available yet.

Generally seems fine, but would of course be fine if we could avoid the slowdown if possible. Right now, it's not clear to me if it's at all possible to have no slowdown while still making normal calculation deterministic like you did in #105920. It would be good if you could describe a bit better why there is non-determinism in the first place and how it impacts other parts of Blender. Ignoring the non-determinism issue, it seems like it might be possible to keep the same performance while still having two separate shared caches, at the cost of a bit more complexity. This could be achieved by calling `face_normals_cache.ensure` directly from `Mesh::vert_normals()`. Then one could have a specialized implementation that computes the vertex and face normals at the same time if the face normals are not available yet.

Campbell Barton commented

2023-08-24 09:39:35 +02:00

The changes seem fine but it would be good if the slowdown could be avoided for the common code path for animation playback.

Could there be a function that calculates vertex & face normals at once which is called from BKE_mesh_ensure_normals_for_display for e.g.

From my own tests this can slow down animation playback ~6%, tested with a single modifier an 2.5million quads.
While not a large amount, it's noticeable and would be good to avoid if possible.

The changes seem fine but it would be good if the slowdown could be avoided for the common code path for animation playback. Could there be a function that calculates vertex & face normals at once which is called from `BKE_mesh_ensure_normals_for_display` for e.g. --- From my own tests this can slow down animation playback ~6%, tested with a single modifier an 2.5million quads. While not a large amount, it's noticeable and would be good to avoid if possible.

Hans Goudey referenced this issue from a commit

2023-08-24 13:37:51 +02:00

Cleanup: Separate sculpt undo restore into types

Hans Goudey referenced this pull request

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

Cleanup: Sculpt: Avoid unnecessarily changing vertex normals #111470

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 added 5 commits 2023-08-24 19:18:04 +02:00

8d0c6aeb00 Merge branch 'main' into mesh-vert-face-normals-split

4fa7f58bfd Merge branch 'main' into mesh-vert-face-normals-split

038c172b9c Update PBVH faces normal calculation

df9ea8b4c4 Add back code to calculate face and vertex normals at the same time

d47ea956d5 Merge branch 'main' into mesh-vert-face-normals-split

Hans Goudey commented

2023-08-24 19:23:40 +02:00

Thanks for the comments! I added back the code calculating both vertex and face normals together, in addition to the option of separating them. I think this is now ready, though I'll run some more performance tests on my desktop when I'm back home, just to get some "X times faster" numbers.

Hans Goudey requested review from Campbell Barton 2023-08-24 19:24:01 +02:00

Hans Goudey requested review from Jacques Lucke 2023-08-24 19:24:02 +02:00

Hans Goudey added this to the 4.0 milestone 2023-08-24 19:24:08 +02:00

Campbell Barton approved these changes 2023-08-25 12:08:19 +02:00

Campbell Barton left a comment

Re-tested, if anything this is a little faster now (~1-3%).
Some notes inline but nothing blocking.

Re-tested, if anything this is a little faster now (~1-3%). Some notes inline but nothing blocking.

source/blender/blenkernel/BKE_mesh.hh Outdated

						
				@ -83,3 +81,1 @@

				                            Span<int> corner_verts,

				                            MutableSpan<float3> face_normals,

				                            MutableSpan<float3> vert_normals);

				void normals_calc_verts(Span<float3> vert_positions,

Campbell Barton commented

2023-08-25 12:07:14 +02:00

Worth adding a note about normals_calc_faces_verts in the doc-string as it may be preferred.

Worth adding a note about `normals_calc_faces_verts` in the doc-string as it may be preferred.

HooglyBoogly marked this conversation as resolved

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

						
				@ -289,3 +275,1 @@

				      }

				    });

				  }

				static void normals_calc_faces_verts(const Span<float3> positions,

Campbell Barton commented

2023-08-25 12:00:36 +02:00

suggestion, but this reads a bit like face-verts which is sometimes used for faces-of-verts: I find this makes it clearer that the function is calculating both the values at once: normals_calc_faces_and_verts.
(this knit-pick applies to the old function too).

*suggestion*, but this reads a bit like face-verts which is sometimes used for faces-of-verts: I find this makes it clearer that the function is calculating both the values at once: `normals_calc_faces_and_verts`. (this knit-pick applies to the old function too).

👍 1

HooglyBoogly marked this conversation as resolved

Jacques Lucke approved these changes 2023-08-25 12:48:37 +02:00

Hans Goudey added 3 commits 2023-08-25 16:25:57 +02:00

4ca5696b74 Merge branch 'main' into mesh-vert-face-normals-split

5ab103f45a Make faces and verts function name clearer

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

43d955c7c4 Add comment

Hans Goudey commented

2023-08-25 18:12:49 +02:00

@blender-bot build

Hans Goudey added 1 commit 2023-08-25 22:28:07 +02:00

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

fd473c0ac7 Fix tests

`face_normal_calc` gives different results than `normal_calc_ngon` which
is used by the existing vertex normal calculation. That can be looked
into separately. Until then, switch to the latter function.

Hans Goudey commented

2023-08-25 22:28:35 +02:00

@blender-bot build

Hans Goudey merged commit 383a145a19 into main

2023-08-25 23:06:12 +02:00

Hans Goudey referenced this issue from a commit

2023-08-25 23:06:12 +02:00

Mesh: Share normals caches by splitting vertex and face calculation

Hans Goudey deleted branch mesh-vert-face-normals-split

2023-08-25 23:06:14 +02:00

Stefan Werner referenced this issue from a commit

2023-09-07 15:02:35 +02:00