Modeling: Connect Vertex Pair does not work when coordinates are large #113111

New Issue

Peng Yan · 2023-10-01T13:09:38+02:00

Peng Yan commented

2023-10-01 13:09:38 +02:00

This bug is found by @1D_inc. Thank him for providing the .blend file.

System Information
Operating system: Windows 10
Graphics card: NVIDIA RTX 2060

Blender Version
Broken: 3.5 / 4.1 alpha

Short description of error
connect vertex pair/connect vertex path does not work if the coordinates of the vertex pair are large

Exact steps for others to reproduce the error
1.Open the attached .blend file, two vertices are already selected in edit mode
2.use shortcut J to connect them, you'll get a "could not connect vertices" error message.
3. if you move all vertices a little bit (in edit mode!), the operator may or may not work.
4.try some other vertex pairs in the file, the operator may or may not work.

This bug is found by @1D_inc. Thank him for providing the .blend file. **System Information** Operating system: Windows 10 Graphics card: NVIDIA RTX 2060 **Blender Version** Broken: 3.5 / 4.1 alpha **Short description of error** connect vertex pair/connect vertex path does not work if the coordinates of the vertex pair are large **Exact steps for others to reproduce the error** 1.Open the attached .blend file, two vertices are already selected in edit mode 2.use shortcut J to connect them, you'll get a "could not connect vertices" error message. 3. if you move all vertices a little bit (in edit mode!), the operator may or may not work. 4.try some other vertex pairs in the file, the operator may or may not work.

Connect vertex path err.blend

2.6 MiB

👍 1

Peng Yan added the

labels 2023-10-01 13:09:39 +02:00

Peng Yan commented

2023-10-01 13:15:54 +02:00

The bug is caused by the constant CONNECT_EPS in bmo_connect_pair.cc. CONNECT_EPS is currently set to 0.0001. It is a threshold checking if the path passes through a vertex exactly. When the coordinates are large ( > 1000), float is not enough to accommodate so many decimal places. Sometimes the threshold will be exceeded, so the path may be unable to find the end point.

Peng Yan commented

2023-10-01 13:26:14 +02:00

I've written a python script to move the objects around and test this operator. Here are some scatter plot data I got. Note that Diff in the plots is the float variable defined in state_isect_co_exact(). It is the distance between the path and the vertex. Every plot gets 100 × 100 points in an xy grid.

From the first plot, the offset is set to -0.01 in x and -0.01 in y. The Diff does not show a clear feature. If you move the object around, you'll get 0.0000, +0.00024, -0.00024 as Diff.

In the second plot, the offset is set to -10 in x and 30 in y. In the third plot, the offset is set to 200 in x and 200 in y. It shows that if the coordinates are larger, the abs(Diff) is likely to be larger.

I've written a python script to move the objects around and test this operator. Here are some scatter plot data I got. Note that **Diff** in the plots is the float variable defined in state_isect_co_exact(). It is the distance between the path and the vertex. Every plot gets 100 × 100 points in an xy grid. From the first plot, the offset is set to -0.01 in x and -0.01 in y. The Diff does not show a clear feature. If you move the object around, you'll get 0.0000, +0.00024, -0.00024 as Diff. In the second plot, the offset is set to -10 in x and 30 in y. In the third plot, the offset is set to 200 in x and 200 in y. It shows that if the coordinates are larger, the abs(Diff) is likely to be larger.

dataplot-0-01.jpg

162 KiB

dataplot-200.jpg

74 KiB

dataplot-10-30.jpg

88 KiB

Peng Yan commented

2023-10-01 13:30:41 +02:00

A reasonable fix is to make CONNECT_EPS proportional to the (max abs) coordinate of the vertex pair. Again thanks @1D_inc for the idea. I'll work on this.

Pratik Borhade commented

2023-10-02 12:04:19 +02:00

Hi, thanks for the report. If I understand correctly, operator is expected to split faces by connecting selected data with edges.
As long as selected vertices are not sharing any faces (so line can pass through it), the operation will fail, I believe.

does not work if the coordinates of the vertex pair are large

I brought the mesh near origin but it is still failing. Does it work for you when cords of vertices are near world origin?
Guess this is not a bug.

Hi, thanks for the report. If I understand correctly, operator is expected to split faces by connecting selected data with edges. As long as selected vertices are not sharing any faces (so line can pass through it), the operation will fail, I believe. > does not work if the coordinates of the vertex pair are large I brought the mesh near origin but it is still failing. Does it work for you when cords of vertices are near world origin? Guess this is not a bug.

Pratik Borhade added

Status

Needs Information from User

and removed

Status

Needs Triage

labels 2023-10-02 12:04:31 +02:00

Peng Yan commented

2023-10-02 12:41:46 +02:00

As long as selected vertices are not sharing any faces (so line can pass through it), the operation will fail, I believe.

The operator (Vertex/Connect Vertex Path or Vertex/Connect Vertex Pair) is not simply splitting a single face by connecting two vertices. It can find a linear path between two vertices and split all faces on the path. It is powered by BMesh Operator "connect_vert_pair" in bmo_connect_pair.cc.

I brought the mesh near origin but it is still failing. Does it work for you when cords of vertices are near world origin?

Sorry I didn't explain it clearly. You can move all vertices to the world origin in edit mode. When the intrinsic coordinates of the vertices get smaller, the operator works well.

> As long as selected vertices are not sharing any faces (so line can pass through it), the operation will fail, I believe. The operator (Vertex/Connect Vertex Path or Vertex/Connect Vertex Pair) is not simply splitting a single face by connecting two vertices. It can find a linear path between two vertices and split all faces on the path. It is powered by BMesh Operator "connect_vert_pair" in bmo_connect_pair.cc. > I brought the mesh near origin but it is still failing. Does it work for you when cords of vertices are near world origin? Sorry I didn't explain it clearly. You can move all vertices to the world origin in edit mode. When the intrinsic coordinates of the vertices get smaller, the operator works well.

Pratik Borhade added

Status

Needs Triage

and removed

Status

Needs Information from User

labels 2023-10-02 13:03:18 +02:00

Philipp Oeser commented

2023-10-04 12:28:05 +02:00

Confirming this for now (even if in the grey-zone between bug and request for improved behavior)

A reasonable fix is to make CONNECT_EPS proportional to the (max abs) coordinate of the vertex pair. Again thanks @1D_inc for the idea. I'll work on this.

Seems reasonable.

Something like this seems to work just fine:


diff --git a/source/blender/bmesh/operators/bmo_connect_pair.cc b/source/blender/bmesh/operators/bmo_connect_pair.cc
index a651ed5345a..2b2c22c4a17 100644
--- a/source/blender/bmesh/operators/bmo_connect_pair.cc
+++ b/source/blender/bmesh/operators/bmo_connect_pair.cc
@@ -40,7 +40,7 @@
  * - run the standard connect operator.
  */
 
-#define CONNECT_EPS 0.0001f
+#define CONNECT_EPS 0.00001f
 #define VERT_OUT 1
 #define VERT_EXCLUDE 2
 
@@ -82,6 +82,7 @@ struct PathContext {
   HeapSimple *states;
   float matrix[3][3];
   float axis_sep;
+  float connect_eps;
 
   /* only to access BMO flags */
   BMesh *bm_bmoflag;
@@ -171,8 +172,8 @@ static int state_isect_co_pair(const PathContext *pc, const float co_a[3], const
   const float diff_a = dot_m3_v3_row_x(pc->matrix, co_a) - pc->axis_sep;
   const float diff_b = dot_m3_v3_row_x(pc->matrix, co_b) - pc->axis_sep;
 
-  const int test_a = (fabsf(diff_a) < CONNECT_EPS) ? 0 : (diff_a < 0.0f) ? -1 : 1;
-  const int test_b = (fabsf(diff_b) < CONNECT_EPS) ? 0 : (diff_b < 0.0f) ? -1 : 1;
+  const int test_a = (fabsf(diff_a) < pc->connect_eps) ? 0 : (diff_a < 0.0f) ? -1 : 1;
+  const int test_b = (fabsf(diff_b) < pc->connect_eps) ? 0 : (diff_b < 0.0f) ? -1 : 1;
 
   if ((test_a && test_b) && (test_a != test_b)) {
     return 1; /* on either side */
@@ -183,7 +184,7 @@ static int state_isect_co_pair(const PathContext *pc, const float co_a[3], const
 static int state_isect_co_exact(const PathContext *pc, const float co[3])
 {
   const float diff = dot_m3_v3_row_x(pc->matrix, co) - pc->axis_sep;
-  return (fabsf(diff) <= CONNECT_EPS);
+  return (fabsf(diff) <= pc->connect_eps);
 }
 
 static float state_calc_co_pair_fac(const PathContext *pc,
@@ -617,6 +618,7 @@ void bmo_connect_vert_pair_exec(BMesh *bm, BMOperator *op)
   {
     pc.states = BLI_heapsimple_new();
     pc.link_pool = BLI_mempool_create(sizeof(PathLink), 0, 512, BLI_MEMPOOL_NOP);
+    pc.connect_eps = CONNECT_EPS * len_v3v3(pc.v_pair[0]->co, pc.v_pair[1]->co);
   }
 
   /* calculate matrix */

@ideasman42 might be interested

Confirming this for now (even if in the grey-zone between bug and request for improved behavior) > A reasonable fix is to make CONNECT_EPS proportional to the (max abs) coordinate of the vertex pair. Again thanks @1D_inc for the idea. I'll work on this. Seems reasonable. Something like this seems to work just fine: ```Diff diff --git a/source/blender/bmesh/operators/bmo_connect_pair.cc b/source/blender/bmesh/operators/bmo_connect_pair.cc index a651ed5345a..2b2c22c4a17 100644 --- a/source/blender/bmesh/operators/bmo_connect_pair.cc +++ b/source/blender/bmesh/operators/bmo_connect_pair.cc @@ -40,7 +40,7 @@ * - run the standard connect operator. */ -#define CONNECT_EPS 0.0001f +#define CONNECT_EPS 0.00001f #define VERT_OUT 1 #define VERT_EXCLUDE 2 @@ -82,6 +82,7 @@ struct PathContext { HeapSimple *states; float matrix[3][3]; float axis_sep; + float connect_eps; /* only to access BMO flags */ BMesh *bm_bmoflag; @@ -171,8 +172,8 @@ static int state_isect_co_pair(const PathContext *pc, const float co_a[3], const const float diff_a = dot_m3_v3_row_x(pc->matrix, co_a) - pc->axis_sep; const float diff_b = dot_m3_v3_row_x(pc->matrix, co_b) - pc->axis_sep; - const int test_a = (fabsf(diff_a) < CONNECT_EPS) ? 0 : (diff_a < 0.0f) ? -1 : 1; - const int test_b = (fabsf(diff_b) < CONNECT_EPS) ? 0 : (diff_b < 0.0f) ? -1 : 1; + const int test_a = (fabsf(diff_a) < pc->connect_eps) ? 0 : (diff_a < 0.0f) ? -1 : 1; + const int test_b = (fabsf(diff_b) < pc->connect_eps) ? 0 : (diff_b < 0.0f) ? -1 : 1; if ((test_a && test_b) && (test_a != test_b)) { return 1; /* on either side */ @@ -183,7 +184,7 @@ static int state_isect_co_pair(const PathContext *pc, const float co_a[3], const static int state_isect_co_exact(const PathContext *pc, const float co[3]) { const float diff = dot_m3_v3_row_x(pc->matrix, co) - pc->axis_sep; - return (fabsf(diff) <= CONNECT_EPS); + return (fabsf(diff) <= pc->connect_eps); } static float state_calc_co_pair_fac(const PathContext *pc, @@ -617,6 +618,7 @@ void bmo_connect_vert_pair_exec(BMesh *bm, BMOperator *op) { pc.states = BLI_heapsimple_new(); pc.link_pool = BLI_mempool_create(sizeof(PathLink), 0, 512, BLI_MEMPOOL_NOP); + pc.connect_eps = CONNECT_EPS * len_v3v3(pc.v_pair[0]->co, pc.v_pair[1]->co); } /* calculate matrix */ ``` @ideasman42 might be interested

👍 1

Philipp Oeser added

and removed

labels 2023-10-04 12:28:26 +02:00

Peng Yan commented

2023-10-04 12:55:51 +02:00

Thank you for the code. I've also coded something but I think your version is better. I changed CONNECT_EPS into a static variable, this may not be consistent with Blender's code style.
Will you handle this fix? Or should I submit a pull request?

Thank you for the code. I've also coded something but I think your version is better. I changed CONNECT_EPS into a static variable, this may not be consistent with Blender's code style. Will you handle this fix? Or should I submit a pull request?

Philipp Oeser commented

2023-10-04 13:11:24 +02:00

I think @ideasman42 can handle (he had a suggestion for improvement)

👍 1

Philipp Oeser referenced a pull request that will close this issue

2023-10-04 13:56:36 +02:00

Fix #113111: Connect Vertex Pair precision issue on large models #113246

Philipp Oeser commented

2023-10-04 13:57:45 +02:00

OK, made !113246 so it doesnt get lost

👍 2

Peng Yan commented