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

[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 all 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
69 changes: 61 additions & 8 deletions src/core/TriangleSplitter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,19 @@ import { isTriDegenerate } from './utils/triangleUtils.js';
// NOTE: these epsilons likely should all be the same since they're used to measure the
// distance from a point to a plane which needs to be done consistently
const EPSILON = 1e-10;
const COPLANAR_EPSILON = 1e-10;
const COPLANAR_EPSILON = 1e-12;
const PARALLEL_EPSILON = 1e-10;
const COPLANAR_EPSILON_MAX = 1e-8;

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,22 @@ 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 );


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

this.coplanarTriangleUsed = true;
Expand All @@ -122,7 +142,6 @@ export class TriangleSplitter {
}

// if the triangle is coplanar then split by the edge planes
const arr = [ triangle.a, triangle.b, triangle.c ];
for ( let i = 0; i < 3; i ++ ) {

const nexti = ( i + 1 ) % 3;
Expand All @@ -134,24 +153,28 @@ export class TriangleSplitter {
_vec.subVectors( v1, v0 ).normalize();
_planeNormal.crossVectors( _triangleNormal, _vec );
_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, triangleMinEdgeSize, triangle );

}

} else {

// otherwise split by the triangle plane
triangle.getPlane( _plane );
this.splitByPlane( _plane, triangle );
this.splitByPlane( _plane, _planeCenter, triangleMinEdgeSize, 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 +212,37 @@ 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 ) {

// we scale COPLANAR_EPSILON since a very small edge leads to more uncertainty in the true plane position:
// see https://github.com/gkjohnson/three-bvh-csg/issues/199#issuecomment-1986287165
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 );


const isStartCoplanar = ( Math.abs( startDist ) < coPlanarEpsilonStart );
const isEndCoplanar = ( Math.abs( endDist ) < coPlanarEpsilonEnd );

// 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
Comment on lines +228 to +231
Copy link
Owner

Choose a reason for hiding this comment

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

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.

if ( isStartCoplanar ) {

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

}

if ( isEndCoplanar ) {

plane.projectPoint( _edge.end, arr[ tNext ] );

}


if ( isStartCoplanar && isEndCoplanar ) {

coplanarEdge = true;
break;
Expand All @@ -207,7 +260,7 @@ 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 ( isStartCoplanar ) {

continue;

Expand All @@ -218,7 +271,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 && isEndCoplanar ) {

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