Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

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

Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
29b81f5
[FIX] Coplanarity test - uncertainty issue - see https://github.com/g…
xavierjs Mar 8, 2024
2a5b549
[QUAL] Fix Semicolon lint issues on TriangleSplitter
xavierjs Mar 11, 2024
1863162
[LINT]
xavierjs Mar 12, 2024
7371d72
[FIX] Restore package and package lock
xavierjs Mar 12, 2024
dd9350b
[QUAL] Apply @gkjohnson PR reviews
xavierjs Mar 15, 2024
40332a3
[QUAL] Clean, lower epsilons [DOC] Add some comments
xavierjs Mar 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 43 additions & 6 deletions src/core/TriangleSplitter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,17 @@ 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;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

@gkjohnson gkjohnson Mar 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


const _edge = new Line3();
const _foundEdge = new Line3();
const _vec = new Vector3();
const _triangleNormal = new Vector3();
const _planeNormal = new Vector3();
const _plane = new Plane();
const _splittingTriangle = new ExtendedTriangle();
const _planeCenter = new Vector3();


// A pool of triangles to avoid unnecessary triangle creation
class TrianglePool {
Expand Down Expand Up @@ -110,6 +114,23 @@ export class TriangleSplitter {
const { normal, triangles } = this;
triangle.getNormal( _triangleNormal ).normalize();

// compute triangleMinEdgeSize:
let triangleMinEdgeSizeSq = Infinity;
const arr = [ triangle.a, triangle.b, triangle.c ];
gkjohnson marked this conversation as resolved.
Show resolved Hide resolved
for ( let i = 0; i < 3; i ++ ) {

const nexti = ( i + 1 ) % 3;
const v0 = arr[ i ];
const v1 = arr[ nexti ];
const edgeSizeSq = v0.distanceToSquared( v1 );
triangleMinEdgeSizeSq = Math.min( triangleMinEdgeSizeSq, edgeSizeSq );

}

const triangleMinEdgeSize = Math.sqrt( triangleMinEdgeSizeSq );
let minEdgeSize = triangleMinEdgeSize;


if ( Math.abs( 1.0 - Math.abs( _triangleNormal.dot( normal ) ) ) < PARALLEL_EPSILON ) {

this.coplanarTriangleUsed = true;
Expand All @@ -133,25 +154,30 @@ export class TriangleSplitter {
// plane positive direction is toward triangle center
_vec.subVectors( v1, v0 ).normalize();
_planeNormal.crossVectors( _triangleNormal, _vec );
minEdgeSize = Math.min( triangleMinEdgeSize, v1.distanceTo( v0 ) );
_plane.setFromNormalAndCoplanarPoint( _planeNormal, v0 );
_planeCenter.copy( v0 );

this.splitByPlane( _plane, triangle );
// we need to provide planeCenter and minEdgeSize to evaluate the plane uncertainty
// the smaller minEdgeSize is, the higher is the uncertainty
// the larger from planeCenter we are, the higher is the uncertainty
this.splitByPlane( _plane, _planeCenter, minEdgeSize, triangle );

}

} else {

// otherwise split by the triangle plane
triangle.getPlane( _plane );
this.splitByPlane( _plane, triangle );
this.splitByPlane( _plane, _planeCenter, minEdgeSize, triangle );

}

}

// Split the triangles by the given plan. If a triangle is provided then we ensure we
// intersect the triangle before splitting the plane
splitByPlane( plane, clippingTriangle ) {
splitByPlane( plane, planeCenter, planeEdgeSize, clippingTriangle ) {

const { triangles, trianglePool } = this;

Expand Down Expand Up @@ -189,7 +215,17 @@ export class TriangleSplitter {
// so we can use that information to determine whether to split later.
const startDist = plane.distanceToPoint( _edge.start );
const endDist = plane.distanceToPoint( _edge.end );
if ( Math.abs( startDist ) < COPLANAR_EPSILON && Math.abs( endDist ) < COPLANAR_EPSILON ) {
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 );
gkjohnson marked this conversation as resolved.
Show resolved Hide resolved
Comment on lines +218 to +219
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


coPlanarEpsilonStart = Math.min( coPlanarEpsilonStart, COPLANAR_EPSILON_MAX );
coPlanarEpsilonEnd = Math.min( coPlanarEpsilonEnd, COPLANAR_EPSILON_MAX );

if ( Math.abs( startDist ) < coPlanarEpsilonStart && Math.abs( endDist ) < coPlanarEpsilonEnd ) {

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

@gkjohnson gkjohnson Mar 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


coplanarEdge = true;
break;
Expand All @@ -207,8 +243,9 @@ export class TriangleSplitter {
}

// we only don't consider this an intersection if the start points hits the plane
if ( Math.abs( startDist ) < COPLANAR_EPSILON ) {
if ( Math.abs( startDist ) < coPlanarEpsilonStart ) {

plane.projectPoint( _edge.start, arr[ t ] );
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain this addition?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

@gkjohnson gkjohnson Mar 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

continue;

}
Expand All @@ -218,7 +255,7 @@ export class TriangleSplitter {
// Because we ignore the start point intersection above we have to make sure we check the end
// point intersection here.
let didIntersect = ! ! plane.intersectLine( _edge, _vec );
if ( ! didIntersect && Math.abs( endDist ) < COPLANAR_EPSILON ) {
if ( ! didIntersect && Math.abs( endDist ) < coPlanarEpsilonEnd ) {

_vec.copy( _edge.end );
didIntersect = true;
Expand Down