Fix #120761: Handle vert to face flushing in vert_hide_update #121678

Sean Kim · 2024-05-11T01:38:54+02:00

Sean Kim commented

2024-05-11 01:38:54 +02:00

This PR changes the vert_hide_update function to flush visibility from the selected vertices to their corresponding faces on the node level instead of on the mesh level. This ensures that in certain cases where vertices exist along the border of a nodes are selected that all corresponding faces are updated and their PBVH nodes are tagged for updating appropriately.

Additionally, this provides a roughly 15ms improvement over the current implementation operating on a subdivided sphere of 32 million vertices when selecting a relatively small portion of the mesh. (50ms -> 35ms).

Spawned from a discussion on #120798

This PR changes the `vert_hide_update` function to flush visibility from the selected vertices to their corresponding faces on the node level instead of on the mesh level. This ensures that in certain cases where vertices exist along the border of a nodes are selected that all corresponding faces are updated and their PBVH nodes are tagged for updating appropriately. Additionally, this provides a roughly 15ms improvement over the current implementation operating on a subdivided sphere of 32 million vertices when selecting a relatively small portion of the mesh. (50ms -> 35ms). Spawned from a discussion on #120798

Sean Kim added 1 commit 2024-05-11 01:39:05 +02:00

Fix #120761 : Handle vert to face flushing in vert_hide_update ba32739de7

Sean Kim added this to the Sculpt, Paint & Texture project 2024-05-11 01:39:20 +02:00

Sean Kim requested review from Hans Goudey 2024-05-11 01:39:28 +02:00

Hans Goudey reviewed 2024-05-13 05:59:20 +02:00

Hans Goudey left a comment

Looks great, thanks!

source/blender/editors/sculpt_paint/paint_hide.cc Outdated

						
				@ -200,0 +204,4 @@

				                           const Span<bool> hide_vert,

				                           MutableSpan<bool> hide_face)

				{

				  for (const int face_idx : node_faces.index_range()) {

Hans Goudey commented

2024-05-13 05:56:37 +02:00

How about this?

  for (const int i : node_faces) {
    const Span<int> face_verts = corner_verts.slice(faces[node_faces[i]]);
    hide_face[i] = std::any_of(
        face_verts.begin(), face_verts.end(), [&](const int i) { return hide_vert[i]; });
  }

This avoids the distinction between face and face_idx and writes the assignment more concisely.

How about this? ```Cpp for (const int i : node_faces) { const Span<int> face_verts = corner_verts.slice(faces[node_faces[i]]); hide_face[i] = std::any_of( face_verts.begin(), face_verts.end(), [&](const int i) { return hide_vert[i]; }); } ``` This avoids the distinction between `face` and `face_idx` and writes the assignment more concisely.

👍 1

Sean Kim commented

2024-05-14 01:42:43 +02:00

Actually, we need the index_range() here because this is following the same scatter / gather pattern that's used in the other callbacks like calc_hide - node_faces and hide_face are expected to be the same length:

  for (const int i : node_faces.index_range()) {
    Span<int> face_verts = corner_verts.slice(faces[node_faces[i]]);
    hide_face[i] = std::any_of(face_verts.begin(), face_verts.end(), [&](const int v) { return hide_vert[v]; });
  }

Actually, we need the `index_range()` here because this is following the same `scatter` / `gather` pattern that's used in the other callbacks like `calc_hide` - `node_faces` and `hide_face` are expected to be the same length: ```Cpp for (const int i : node_faces.index_range()) { Span<int> face_verts = corner_verts.slice(faces[node_faces[i]]); hide_face[i] = std::any_of(face_verts.begin(), face_verts.end(), [&](const int v) { return hide_vert[v]; }); } ```

👍 1

Sean-Kim marked this conversation as resolved

source/blender/editors/sculpt_paint/paint_hide.cc Outdated

						
				@ -200,0 +259,4 @@

				}

				/* Updates all of a mesh's edge visibility based on vertex visibility. */

				static void flush_edge_changes(Mesh &mesh, Span<bool> hide_vert)

Hans Goudey commented

2024-05-13 05:59:02 +02:00

How about calling the existing edge_hide_from_vert in some way instead of implementing the same thing here?

How about calling the existing `edge_hide_from_vert` in some way instead of implementing the same thing here?

Sean Kim commented

2024-05-13 22:02:09 +02:00

Whoops, yeah that's definitely a better option. Will do.

Sean-Kim marked this conversation as resolved

Sean Kim added 1 commit 2024-05-14 02:00:30 +02:00

Address PR feedback 189d48476a

Iliya Katushenock reviewed 2024-05-14 14:41:39 +02:00

source/blender/blenkernel/BKE_mesh.hh Outdated

						
				@ -339,0 +339,4 @@

				/**

				 * Calculate edge visibility based on vertex visibility, hides an edge when either of its

				 * vertices are hidden. */

				void edge_hide_from_vert(const Span<int2> edges,

Iliya Katushenock commented

2024-05-14 14:41:39 +02:00

const Span<int2> edges -> Span<int2> edges

`const Span<int2> edges` -> `Span<int2> edges`

👍 1

Hans Goudey commented

2024-05-14 15:51:21 +02:00

Also add the mesh_ prefix since this is only in the blender::bke namespace

Also add the `mesh_` prefix since this is only in the `blender::bke` namespace

👍 1

Sean-Kim marked this conversation as resolved

Hans Goudey approved these changes 2024-05-14 15:52:57 +02:00

Hans Goudey left a comment

Thanks, looks good, pending two little changes.

source/blender/editors/sculpt_paint/paint_hide.cc Outdated

						
				@ -200,0 +259,4 @@

				  bke::SpanAttributeWriter<bool> hide_edge = attributes.lookup_or_add_for_write_only_span<bool>(

				      ".hide_edge", bke::AttrDomain::Edge);

				  Span<int2> edges = mesh.edges();

				  bke::edge_hide_from_vert(edges, hide_vert, hide_edge.span);

Hans Goudey commented

2024-05-14 15:52:38 +02:00

Fine to just put mesh.edges() in the call here IMO, without the separate variable.

Fine to just put `mesh.edges()` in the call here IMO, without the separate variable.

Sean-Kim marked this conversation as resolved

Sean Kim added 1 commit 2024-05-14 20:40:51 +02:00

PR Feedback

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

36d711080e

Sean Kim commented

2024-05-14 20:51:04 +02:00

@blender-bot build

Sean Kim merged commit 7017091272 into main

2024-05-14 21:25:09 +02:00

Sean Kim deleted branch fix-120761-option-2

2024-05-14 21:25:12 +02:00

Sean Kim referenced this issue from a commit

2024-05-14 21:25:13 +02:00

Fix #120761: Handle vert to face flushing in vert_hide_update

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

Fix #120761: Handle vert to face flushing in vert_hide_update #121678