[FIX] Coplanarity test - uncertainty issue - see https://github.com/g… #203

xavierjs · 2024-03-08T19:35:25Z

#199 (comment)

src/core/TriangleSplitter.js

gkjohnson · 2024-03-12T00:49:15Z

You can more easily fix the lint issues by running

npx eslint \"./src/**/*.{js,ts}\" \"./examples/*.js\" --fix

I'll take a look later this week, thanks!

gkjohnson

Very much appreciate the PR and the consideration put into looking into the triangle clipping code. The inaccuracy of the derived plane due to tirangle edge size is an easy thing to overlook. I've added a few comments.

Possibly also worth noting that a coplanar check is done in three-mesh-bvh here to compute whether the triangles intersect. It's maybe a bit less critical but this function is used when collecting which triangles need to be clipped against others in this project.

src/core/TriangleSplitter.js

gkjohnson · 2024-03-14T11:33:26Z

src/core/TriangleSplitter.js


+					plane.projectPoint( _edge.start, arr[ t ] );


Can you explain this addition?

If we estimate that _edge.start belongs to plane, then we make sure _edge.start is on the plane to lower the uncertainty.
It is to avoid error propagation after multiple CSG operations on the same geometry.

The point of this condition block, though, is to skip interpreting the start point as an intersection if it's the only point that lies on the plane. This is so we avoid double counting the same vertex if the plane passes through a corner of the triangle. The _edge variable is then immediately overwritten on the next loop iteration.

I'm also seeing now that this is modifying the same vector that is on the triangle object being split (ie this line could be changing tri.a). Same with the other point projections above. ~~Is this intentional? My feeling is that "tri" should be treated as immutable in this context so we're not changing triangle vertices over the course of the function.~~

Edit: I see from #199 (comment) you say this is deliberate, now. Can you explain the rationale? These are things I think will need comments to explain so they're understood in the future.

In the last commit, I have made stuff a bit clearer by projecting the points on the plane if we estimate it belongs to the plane before going into further processing. However, I still don't know why it works. I just have a kind of intuition...

gkjohnson · 2024-03-14T11:36:22Z

src/core/TriangleSplitter.js

@@ -7,13 +7,23 @@ import { isTriDegenerate } from './utils/triangleUtils.js';
 const EPSILON = 1e-10;
 const COPLANAR_EPSILON = 1e-10;
 const PARALLEL_EPSILON = 1e-10;
+const COPLANAR_EPSILON_MAX = 1e-4;


Have you considered scaling the epsilon up based on the scale of the smallest triangle edge? Jumping straight to 1e-4 if one of the edges is too small is a large jump.

COPLANAR_EPSILON_MAX is just to avoid too large epsilon values. We don't jump to this value, we just scale COPLANAR_EPSILON but we make sure that it stays below COPLANAR_EPSILON_MAX.

Got it - I'm understanding now. How did you conclude that 1e-4 is a good max epsilon? It's a very large value and from looking at the code it is still possible to wind up using 1e-4 as an epsilon at most. Do smaller values still cause issues?

It would be nice to have a couple example operation codes that show how this is fixed.

In my specific case this value was reached sometimes. I did not tried to set a lower values. However when it is reached it means that the computation is somewhat undefined (we want to test if a point or edge belongs to a plane but the plane is defined with a very high uncertainty so the answer is not reliable at all...)

It might have been reached but that doesn't mean it's needed for the calculations. As it's currently implemented you are scaling the COPLANAR_EPSILON value which is already set to 1e-10, a fairly large value in itself, which is basically assuming that 1e-10 is the necessary error value at the "center" of the plane where it should actually be a much smaller value since we're not dealing with this lever arm effect. It's difficult to calculate how the error has compounded over calculations but it's possible that the COPLANAR_EPSILON could be set to something like 1e-20 or less, now, and then it could be scaled by the approach added in this PR.

I'd much rather set this to a smaller value and if we continue to have issues we can increase it, I think.

In the last commit I have lower the EPSILON_MAX to 1e-7, it works with this value

gkjohnson · 2024-03-14T11:41:03Z

src/core/TriangleSplitter.js

+				let coPlanarEpsilonStart = COPLANAR_EPSILON *	Math.max( 1, 0.5 * _edge.start.distanceTo( planeCenter ) / planeEdgeSize );
+				let coPlanarEpsilonEnd = COPLANAR_EPSILON *	Math.max( 1, 0.5 * _edge.end.distanceTo( planeCenter ) / planeEdgeSize );


Do you mind explaining the rationale behind these calculations? Including the thinking behind "planeCenter"? It seems as though the distance to an arbitrarily chosen point on the plane will have a significant impact.

We don't want to use an epsilon larger than COPLANAR_EPSILON, that's why we use the Math.max
Please look at the picture posted with #199 (comment) for the annotations:
We scale epsilon (e) to get epsilon scaled (E) by IP / AB.

AB is the min edge size

instead of using IM like in my draft we use IE or IF, with E and F start and end points of the edge _edge
This computation is from the Thales theorem.

Thanks I've taken a look at the diagrams and I understand the intent. Basically a very small edge leads to more uncertainty in the true plane position so you want to use a large epsilon when checking distances to such a plane. And you're effectively scaling the amount that we scale the epsilon by the distance to the point used to generate the plane since we know that point is exactly correct.

This may be a bit difficult to describe effectively but it would nice to have some comments in the code to describe what's happening here and why. Even if it includes a link to this comment thread for context.

Hi @gkjohnson I have added some comments and a link to the Github issue in the last commit

xavierjs · 2024-03-15T14:40:49Z

@gkjohnson I have applied most of your reviews in the latest commit.

gkjohnson · 2024-03-20T03:52:59Z

src/core/TriangleSplitter.js

+					// we project the edge in the plane
+					plane.projectPoint( _edge.start, arr[ t ] );
+					plane.projectPoint( _edge.end, arr[ tNext ] );


I also don't think these are needed - below this coplanarEdge is set to true which means that no clipping happens, so these point projections won't have any impact (other than modifying the triangle object).

See edit in https://github.com/gkjohnson/three-bvh-csg/pull/203/files#r1531482474

gkjohnson

Sorry for the delay on reviews - this is a complicated topic and I want to make sure I'm able to give a thoughtful response.

Also are you able to provide a test scenario or two that this PR fixes? I'd like to be able to test it and possibly add some test cases to test for in the future.

gkjohnson · 2024-03-31T05:50:19Z

src/core/TriangleSplitter.js

+				// reprojection:
+				// if we estimate that a point belongs to the plane
+				// we force it to belongs to the plane
+				// I cannot explain exactly why it works but it looks that it works


This bit makes me uncomfortable still, I think. It's something that could fix your case and break others. Likely what's happening is that it's shifting vertices which helps change the subsequent calculations just enough such that it produces a different result and isn't culled. But this isn't stable in the sense the order in which triangles are clipped would change the result. And you'd get a similar effect by just randomly jittering the vertices by a small amount, I believe.

Instead of modifying the original triangle vertices such that it impacts subsequent clipping calculations have you tried just project the edge points in the local scope for this calculation to see if that solves the issue? Ie do something like this:

if ( isStartCoplanar ) { plane.projectPoint( _edge.start, _edge.start ); } if ( isEndCoplanar ) { plane.projectPoint( _edge.end, _edge.end ); }

Then we're ensuring that at least the edge we're using is on the other triangle plane.

[FIX] Coplanarity test - uncertainty issue - see gkjohnson#199 (comment)

29b81f5

github-advanced-security bot found potential problems Mar 10, 2024

View reviewed changes

[QUAL] Fix Semicolon lint issues on TriangleSplitter

2a5b549

xavierjs added 2 commits March 12, 2024 09:50

[LINT]

1863162

[FIX] Restore package and package lock

7371d72

gkjohnson reviewed Mar 14, 2024

View reviewed changes

[QUAL] Apply @gkjohnson PR reviews

dd9350b

gkjohnson reviewed Mar 20, 2024

View reviewed changes

[QUAL] Clean, lower epsilons [DOC] Add some comments

40332a3

gkjohnson reviewed Mar 31, 2024

View reviewed changes

gkjohnson mentioned this pull request Apr 17, 2024

Tests: Add tests to validate triangle splitting #73

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIX] Coplanarity test - uncertainty issue - see https://github.com/g… #203

[FIX] Coplanarity test - uncertainty issue - see https://github.com/g… #203

xavierjs commented Mar 8, 2024 •

edited by gkjohnson

Loading

gkjohnson commented Mar 12, 2024

gkjohnson left a comment

gkjohnson Mar 14, 2024

xavierjs Mar 14, 2024

gkjohnson Mar 20, 2024 •

edited

Loading

xavierjs Mar 25, 2024

gkjohnson Mar 14, 2024

xavierjs Mar 14, 2024

gkjohnson Mar 20, 2024 •

edited

Loading

xavierjs Mar 20, 2024

gkjohnson Mar 22, 2024

xavierjs Mar 25, 2024

gkjohnson Mar 14, 2024

xavierjs Mar 14, 2024

gkjohnson Mar 20, 2024

xavierjs Mar 25, 2024

xavierjs commented Mar 15, 2024

gkjohnson Mar 20, 2024 •

edited

Loading

gkjohnson left a comment

gkjohnson Mar 31, 2024

		let coPlanarEpsilonStart = COPLANAR_EPSILON * Math.max( 1, 0.5 * _edge.start.distanceTo( planeCenter ) / planeEdgeSize );
		let coPlanarEpsilonEnd = COPLANAR_EPSILON * Math.max( 1, 0.5 * _edge.end.distanceTo( planeCenter ) / planeEdgeSize );

[FIX] Coplanarity test - uncertainty issue - see https://github.com/g… #203

Are you sure you want to change the base?

[FIX] Coplanarity test - uncertainty issue - see https://github.com/g… #203

Conversation

xavierjs commented Mar 8, 2024 • edited by gkjohnson Loading

gkjohnson commented Mar 12, 2024

gkjohnson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkjohnson Mar 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkjohnson Mar 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xavierjs commented Mar 15, 2024

gkjohnson Mar 20, 2024 • edited Loading

Choose a reason for hiding this comment

gkjohnson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xavierjs commented Mar 8, 2024 •

edited by gkjohnson

Loading

gkjohnson Mar 20, 2024 •

edited

Loading

gkjohnson Mar 20, 2024 •

edited

Loading

gkjohnson Mar 20, 2024 •

edited

Loading