-
-
Notifications
You must be signed in to change notification settings - Fork 65
[FIX] Coplanarity test - uncertainty issue - see https://github.com/g… #203
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
29b81f5
2a5b549
1863162
7371d72
dd9350b
40332a3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 { | ||
|
|
@@ -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 ]; | ||
| 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; | ||
|
|
@@ -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; | ||
|
|
@@ -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; | ||
|
|
||
|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
|
@@ -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; | ||
|
|
||
|
|
@@ -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; | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.