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.

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

New TriangleSplitter #194

Draft
wants to merge 75 commits into
base: main
Choose a base branch
from
Draft

New TriangleSplitter #194

wants to merge 75 commits into from

Conversation

gkjohnson
Copy link
Owner

@gkjohnson gkjohnson commented Jan 3, 2024

Related to #51, #97

TODO

Follow On

  • Potentially able to determine coplanar triangles much more easily (triangles within a coplanar triangle)
  • We have half-edge connectivity and "required" edges
    • These can be used to more quickly determine connected triangle sets to add together and reduce raycasts
    • Because it will be easier to track the original half edge sibling we can even use the sibling edges raycast result
    • We can use these to update the half edge structure instead of rebuilding it from scratch
  • Potentially enables easier water-tight detection
  • Potentially able to choose edge direction that keep triangles fairly large

src/core/splitter/TriangleGraph.js Fixed Show fixed Hide fixed
src/core/splitter/TriangleGraph.js Fixed Show fixed Hide fixed
src/core/splitter/TriangleGraph.js Fixed Show fixed Hide fixed
src/core/splitter/TriangleGraph.js Fixed Show fixed Hide fixed
src/core/splitter/EdgeGraph.js Fixed Show fixed Hide fixed
src/core/splitter/EdgeGraph.js Fixed Show fixed Hide fixed

removeTriangle( t ) {

const { points, edges, triangles } = this;

Check notice

Code scanning / CodeQL

Unused variable, import, function or class Note

Unused variable points.
}

// if the final edge is degenerate then we haven't intersected
if ( target.distance() < EPS ) {

Choose a reason for hiding this comment

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

I have been trying to get this to work for us locally and I believe this will fail if the triangles only intersect at a single point:

__
|  /
| /*----
|/ |
   |

Since they are intersecting at that point you still need to insert it into your graph, otherwise the graph will not know about that point

This logic then needs to be moved to the two places you call getTriangleLineIntersection and choose to add a point if it is a single point edge.

Choose a reason for hiding this comment

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

Causes the issue in the fiddle here:
#210

for ( let j = 0, l = _edgesToSwap.length; j < l; j ++ ) {

const other = _edgesToSwap[ j ];
this.swapEdge( other );

Choose a reason for hiding this comment

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

Another issue we ran into.

Swapping this edge can lead to one of the triangles to be a 0 area triangle, which then makes it not able to clean up.

I'm not sure if there is a downstream place to fix it, but we discovered this would lead to some invalid geometry.

If I check if the resulting triangles are not 0 area this ends up not being a problem

Choose a reason for hiding this comment

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

Actually I looked into this more and the problem is a bit more generic.

This function swaps edges, which can lead to an invalid triangle or a flipped triangle.

So we need to detect if the swapped edge will interect or flip over the none change edge for both of the attached triangles:

I can improve this by adding this to swapEdge

const otherEdgeIndex = ( t0SwapIndex + 1 ) % 3;
		const otherEdgeIndexReverse = ( t1SwapIndex + 1 ) % 3;
		const otherEdge = triangle.edges[ otherEdgeIndex ].edge;
		const otherEdgeReverse = reverseTriangle.edges[ otherEdgeIndexReverse ].edge;
		const travlingEdge = { start: triangle.points[ t0SwapIndex ], end: reverseTriangle.points[ t1SwapIndex ] };
		const res = { x: NaN, y: NaN };
		const resReverse = { x: NaN, y: NaN };
		 closestPointLineToLine( otherEdge, travlingEdge, res );
		 if ( res.x <= 0 || res.x >= 1 ) {

			edge.required = true;

			return false;

		}

		 closestPointLineToLine( otherEdgeReverse, travlingEdge, resReverse );
		 if ( resReverse.x <= EPSILON || resReverse.x >= 1 - EPSILON ) {

			edge.required = true;

			return false;

		}
		
		```

@ToyboxZach
Copy link

I have started using this branch and overall its a huge improvement over the old triangle splitter. I was able to find a couple small issues.
https://github.com/ToyboxZach/three-bvh-csg/tree/FixTriangleSplit

I made three fixes:

  1. In the triangle splitter, if a triangle splits a triangle and ends directly on another edge the package used to ignore that point, we now correctly handle that.
  2. In edge sape, it was possible for those faces to be invalid this change fixes it so its much more unlikely to generate an invalid face.
  3. If there is a 0 area triangle we just don't add it to the final mesh, this was creating a few cases of extra edges/bounding box weirdness that was not necessary. I think the real fix is probably to not generate these 0 area triangles, but I need to move on to something else so I didn't dive too deeply into the issues.

I ended up finding most of these issues using various orientations of coplanar Boxes and TextGeomtery which gives a whole bunch of orientations

This is what was happening with this PR:
image

This is after my small fixes:
image


const containingTriangle = triangles.findIndex( t => {

return t.getArea() !== 0 && t.containsPoint( point );

Choose a reason for hiding this comment

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

This should probably check > EPSILON

}

// try for a few iterations to swap edges until they work
for ( let i = 0; i < SWAP_ITERATIONS; i ++ ) {

Choose a reason for hiding this comment

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

Could you explain why 3 was chosen? I ended up with a case where I needed a few more iterations, and _edgesToSwapLength ended up being enough

Copy link
Owner Author

Choose a reason for hiding this comment

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

I think 3 is just the number I was using for my testing at the time.

@gkjohnson
Copy link
Owner Author

Thanks for taking a deeper look @ToyboxZach. As mentioned in #210 - there are still some issues with this approach. Specifically just "swapping edges" isn't good enough to generate a good topology. The paper referenced in the original comment instead requires that something like earcut triangulation be performed when we find that edges need to be swapped. Unfortunately the defacto javascript earch implementation (from MapboxGL) isn't robust to some scenarios and will produce bad triangulation. I think a CDL implementation (referenced in #210) would be easier to work with.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants