Fix #125646: Resolve edge-slide performance regression #128016

Campbell Barton · 2024-09-23T10:23:40+02:00

Campbell Barton commented

2024-09-23 10:23:40 +02:00

Partially revert 0 which replaced BMBVHTree with SnapObjectContext.

While SnapObjectContext is a comprehensive method of performing
ray-casts, the purpose of edge-slide visibility checks is mainly to
exclude back-facing edges from the direction calculation,
when the selected edge only has some of it's faces front-facing.

SnapObjectContext 3x ray-casts where each call would traverse into
node-groups, constructing & freeing dupli-lists which could make
Blender hang while sliding edges.

Resolve be restoring simpler self-occlusion check.

NOTE: even if SnapObjectContext could be optimized for this case, I don't think it's useful for the entire scene to be taken into account when calculating edge-slide direction.

Partially revert [0] which replaced BMBVHTree with SnapObjectContext. While SnapObjectContext is a comprehensive method of performing ray-casts, the purpose of edge-slide visibility checks is mainly to exclude back-facing edges from the direction calculation, when the selected edge only has some of it's faces front-facing. SnapObjectContext 3x ray-casts where each call would traverse into node-groups, constructing & freeing dupli-lists which could make Blender hang while sliding edges. Resolve be restoring simpler self-occlusion check. [0]: 2d50a41d77374cb37553f3a3a067845140e13aad ---- NOTE: even if `SnapObjectContext` could be optimized for this case, I don't think it's useful for the entire scene to be taken into account when calculating edge-slide direction.

Campbell Barton requested review from Germano Cavalcante 2024-09-23 10:24:30 +02:00

Campbell Barton referenced this pull request

2024-09-23 10:26:29 +02:00

Snapping related performance regression in 4.2 #125646

Germano Cavalcante commented

2024-09-23 22:41:47 +02:00

In fact, using the SnapObjectContext to test occlusion is slower, mainly because it iterates over all objects in the scene, creating linked lists and additional BVH trees for objects in the way.

However, I would prefer to avoid using BMeshes in this code, as the edge slide code is now generic and works with UVs, and may later support other types of objects like Curves and Lattices. Currently, sv->td->extra is not always a BMVert.

Also, using BM_ITER_ELEM(e, &iter_other, v, BM_EDGES_OF_VERT) to test occlusion of all unselected edges connected to a vertex seems a bit excessive, since only one or two edges are chosen to slide. The mval_dir and loop_dir calculated at the end may not be the most ideal.

(...) could make Blender hang while sliding edges.

As far as I can check, this code is only called once during the operation. Even with "navigate while transform," it is not called again.

As an alternative solution, I would prefer to create a new eSnapTargetOP, something like SCE_SNAP_TARGET_ONLY_EDITED or SCE_SNAP_TARGET_ONLY_ACTIVE, to avoid iterating over all objects in the scene.

In fact, using the `SnapObjectContext` to test occlusion is slower, mainly because it iterates over all objects in the scene, creating linked lists and additional BVH trees for objects in the way. However, I would prefer to avoid using BMeshes in this code, as the edge slide code is now generic and works with UVs, and may later support other types of objects like Curves and Lattices. Currently, `sv->td->extra` is not always a `BMVert`. Also, using `BM_ITER_ELEM(e, &iter_other, v, BM_EDGES_OF_VERT)` to test occlusion of all unselected edges connected to a vertex seems a bit excessive, since only one or two edges are chosen to slide. The `mval_dir` and `loop_dir` calculated at the end may not be the most ideal. > (...) could make Blender hang while sliding edges. As far as I can check, this code is only called once during the operation. Even with "navigate while transform," it is not called again. --- As an alternative solution, I would prefer to create a new `eSnapTargetOP`, something like `SCE_SNAP_TARGET_ONLY_EDITED` or `SCE_SNAP_TARGET_ONLY_ACTIVE`, to avoid iterating over all objects in the scene.

Campbell Barton force-pushed pr-125646-fix from 94af2579b8 to 754c3d513d

2024-09-24 02:43:59 +02:00

Compare

Campbell Barton force-pushed pr-125646-fix from cf9b0f54ce to 26b55430ad

2024-09-24 02:55:32 +02:00

Compare

Campbell Barton commented

2024-09-24 03:12:41 +02:00

@mano-wii updated the patch to make the mesh check explicit and restored some of updates lost when reverting.

Checking further and old code has an advantage over use of the new snapping logic - which is that an edge never occludes itself - because the BMBVH ray-cast excludes faces connected to the edge.
I managed to find cases where the current snap logic incorrectly detects edges as occluded although they required using large scaled meshes.

Since the old logic has been in Blender for a long time, I think we could apply the patch for 4.2 LTS.
Then develop a different method separately.

@mano-wii updated the patch to make the mesh check explicit and restored some of updates lost when reverting. Checking further and old code has an advantage over use of the new snapping logic - which is that an edge never occludes itself - because the BMBVH ray-cast excludes faces connected to the edge. I managed to find cases where the current snap logic incorrectly detects edges as occluded although they required using large scaled meshes. Since the old logic has been in Blender for a long time, I think we could apply the patch for 4.2 LTS. Then develop a different method separately.

Germano Cavalcante requested changes 2024-09-24 03:21:48 +02:00

Dismissed

source/blender/editors/transform/transform_mode_edge_slide.cc Outdated

						
				@ -290,1 +230,3 @@

				    }

				    BMIter iter_other;

				    BMEdge *e;

				    BMVert *v = static_cast<BMVert *>(sv->td->extra);

Germano Cavalcante commented

2024-09-24 03:17:52 +02:00

Does this work if we use Edge Slide on a UV?

Campbell Barton commented

2024-09-24 03:28:03 +02:00

I've since split the BMesh part out, so UV's are handled in a separate loop.

ideasman42 marked this conversation as resolved

source/blender/editors/transform/transform_mode_edge_slide.cc Outdated

						
				@ -314,0 +249,4 @@

				      /* Screen-space coords. */

				      float2 sco_a, sco_b;

				      sld->project(sv, sco_a, sco_b);

Germano Cavalcante commented

2024-09-24 03:20:49 +02:00

We are re-projecting and re-testing the same sliding coordinates for each linked edge.

The is_visible logic should be moved to a function in order to break the loop at the first visible edge and only test once.

We are re-projecting and re-testing the same sliding coordinates for each linked edge. The `is_visible` logic should be moved to a function in order to break the loop at the first visible edge and only test once.

ideasman42 marked this conversation as resolved

Campbell Barton force-pushed pr-125646-fix from 12e5989d9c to 5eab0a7eb1

2024-09-24 03:24:17 +02:00

Compare

Campbell Barton added 1 commit 2024-09-24 03:30:43 +02:00

Address comments based on review. 52155a161c

Germano Cavalcante requested changes 2024-09-24 03:32:37 +02:00

Dismissed

source/blender/editors/transform/transform_mode_edge_slide.cc Outdated

						
				@ -304,1 +243,3 @@

				        sld->curr_sv_index = i;

				        if (BM_elem_flag_test(e, BM_ELEM_SELECT)) {

				          continue;

				        }

Germano Cavalcante commented

2024-09-24 03:32:06 +02:00

There is no need to retest the same vertex several times.
This should be moved to a function like this:

bool is_sv_visible(...) {
	  BM_ITER_ELEM (e, &iter_other, v, BM_EDGES_OF_VERT) {
      if (BM_elem_flag_test(e, BM_ELEM_SELECT)) {
        continue;
      }

      /* This test is only relevant if object is not wire-drawn! See #32068. */
      bool is_visible = !use_occlude_geometry || BMBVH_EdgeVisible(bmbvh, e, t->depsgraph, region, v3d, tc->obedit);

      if (is_visible) {
          return true;
      }
  }
  return false;
}

There is no need to retest the same vertex several times. This should be moved to a function like this: ``` bool is_sv_visible(...) { BM_ITER_ELEM (e, &iter_other, v, BM_EDGES_OF_VERT) { if (BM_elem_flag_test(e, BM_ELEM_SELECT)) { continue; } /* This test is only relevant if object is not wire-drawn! See #32068. */ bool is_visible = !use_occlude_geometry || BMBVH_EdgeVisible(bmbvh, e, t->depsgraph, region, v3d, tc->obedit); if (is_visible) { return true; } } return false; } ```

ideasman42 marked this conversation as resolved

Campbell Barton added 1 commit 2024-09-24 03:43:17 +02:00

Move edge visibility check into a function, avoid redundant calculations d233f9a438

Germano Cavalcante approved these changes 2024-09-24 03:49:31 +02:00

Campbell Barton referenced this issue from a commit

2024-09-24 07:41:55 +02:00

Fix #125646: Resolve edge-slide performance regression when snapping

Campbell Barton changed title from ~~Fix #125646: Resolve edge-slide performance regression when snapping~~ to Fix #125646: Resolve edge-slide performance regression

2024-09-24 07:42:48 +02:00

Campbell Barton manually merged commit a13513ab25 into main

2024-09-24 07:42:57 +02:00

Philipp Oeser referenced this issue from a commit

2024-10-10 14:20:38 +02:00

Fix #125646: Resolve edge-slide performance regression when snapping

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 #125646: Resolve edge-slide performance regression #128016