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

More robust Triangle-Triangle intersection #734

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

TheBlek
Copy link

@TheBlek TheBlek commented Jan 23, 2025

Added #655 as a test case.
I've traced issue down to this line

if ( ( doesIntersect || startIntersects ) && ! isNearZero( tempPoint.distanceTo( end ) ) ) {

In first triangle there is a point with exact x coordinate of the plane where second triangle lies. When this point is the endpoint of a segment, there appears to be some precision errors when calculating intersection point of a segment with a plane. And intersection point is not quite equal to the end point (~3*10^-15 difference). Increasing epsilon most most sense to me.

Also moved a part of the code inside the if statement bc it was executing only if that branch was taken. I think that is more readable.

@TheBlek
Copy link
Author

TheBlek commented Jan 29, 2025

@gkjohnson Let me know if you think there should be a better solution

@gkjohnson
Copy link
Owner

Hey @TheBlek - thanks for the contribution and sorry for being slow on the response here. I've been preoccupied on other projects but I'll try to be more attentive here.

This triangle intersection code has been a significantly difficult problem and toying with epsilon values always seems like a losing battle (I've done my share of modifying this value, as well 😅). As with any adjustment like this my fear is that this will break other useful conditions. I'm wondering if there's a more robust triangle / triangle intersection function out there that we can use?

@TheBlek
Copy link
Author

TheBlek commented Feb 10, 2025

As I understand, current intersection algorithm is based on Moller's paper. I've done a bit of reading and there seems to be a more robust algorithm.
However, in the book Real Time Collision Detection and in his blog, Christer Ericson writes that since all those algorithms assume a generic case, all the degenerate cases should be handled beforehand.

Maybe we can explicitly check coplanarity of all points with another triangle's plane before entering the general algorithm? That would solve #655 (and maybe other problems that we do not see yet?) without changing epsilon value.

@gkjohnson
Copy link
Owner

I don't recall exactly where the current algorithm is from - but for the most part it was made with a focus on speed of execution. Though of course, at it turns out, these kinds of algorithms are particularly tricky. It would be best to start with a well-known, well-tested implementation & epsilons and deal with any other problems from there.

Unfortunately I don't have a specific algorithm or paper in mind to replace the algorithm with so any recommendations you have, like the one you've shared, are appreciated.

since all those algorithms assume a generic case, all the degenerate cases should be handled beforehand.

I expect we're going to see issues no matter what but hopefully we'll still see an improvement. Regarding degenerate cases - if there are canonical ways to test for these then perhaps we can add this functionality to the beginning of the function or in a wrapper for the method.

Thanks for looking into this!

@TheBlek TheBlek changed the title Increase epsilon constant More robust Triangle-Triangle intersection Feb 12, 2025
@TheBlek TheBlek marked this pull request as draft February 12, 2025 15:55
@TheBlek
Copy link
Author

TheBlek commented Feb 12, 2025

I've started working on explicitly handling degenerate cases before your comment and pushed results here if you want to check out how this might look anyway. I'm not particularly fond of the code I wrote but it gets the job done. And even the previously skipped test now passes.
Running the benchmark did not reveal any significant performance difference so this should be fine.
Nevertheless, I intend to check out a few algorithms from papers to see if they handle it better. Hence, draft status on the pull request.

@@ -1,4 +1,4 @@
import { Triangle, Vector3, Line3, Sphere, Plane } from 'three';
import { Triangle, Vector3, Vector2, Line3, Sphere, Plane } from 'three';

Check notice

Code scanning / CodeQL

Unused variable, import, function or class Note

Unused import Sphere.
@TheBlek
Copy link
Author

TheBlek commented Feb 16, 2025

I have implemented Moller's algorithm from here. It handles corner cases for intersections pretty well, but still with quite a number of ifs. Also, I've found a couple optimizations that can be used regardless of main intersection algo:

  1. ExtendedTriangle is constructing bounding sphere in .update() which seems to have never been used
  2. Coplanar case contained checks with SAT axes that are cross products of triangles' edges. But that means that these axes were normals to the plane where the triangle lie. Hence they never would have been separating axes

For general case intersection implementation in this PR appears to outperform slightly against current implementation:
Before:

{
  "name": "Math Functions",
  "results": [
    {
      "name": "IntersectTri w/o Target",
      "mean": 0.00026540000009117647,
      "median": 0.00029999998514540493,
      "min": 0.0001999999803956598,
      "max": 0.002099999983329326,
      "iterations": 1000
    },
    {
      "name": "IntersectTri w/ Target",
      "mean": 0.0005448999997170177,
      "median": 0.0005000000237487257,
      "min": 0.0004999999946448952,
      "max": 0.0011999999987892807,
      "iterations": 1000
    },
    {
      "name": "IntersectTri w/ Update",
      "mean": 0.000739700000412995,
      "median": 0.0007000000041443855,
      "min": 0.0005999999993946403,
      "max": 0.015699999989010394,
      "iterations": 1000
    }
  ]
},

After:

{
  "name": "Math Functions",
  "results": [
    {
      "name": "IntersectTri w/o Target",
      "mean": 0.00020410000017727725,
      "median": 0.00020000000949949026,
      "min": 0.00009999997564591467,
      "max": 0.0009999999892897904,
      "iterations": 1000
    },
    {
      "name": "IntersectTri w/ Target",
      "mean": 0.0006590000002179295,
      "median": 0.0005999999993946403,
      "min": 0.0004999999946448952,
      "max": 0.02829999997629784,
      "iterations": 1000
    },
    {
      "name": "IntersectTri w/ Update",
      "mean": 0.0007584999999962747,
      "median": 0.0007999999797903001,
      "min": 0.000699999975040555,
      "max": 0.0027999999874737114,
      "iterations": 1000
    }
  ]
},

But benchmarking general case with one triangle pair intersected repeatedly seems not sufficient for evaluating algorithm performance. More about this later vvv

For more complex benchmarks results are inconclusive:
Before:

{
  "name": "IntersectsGeometry w/ BVH",
  "mean": 2.469768100000394,
  "median": 2.4654999999911524,
  "min": 2.4144000000087544,
  "max": 3.7338999999919906,
  "iterations": 1000
},
{
  "name": "IntersectsGeometry w/o BVH",
  "mean": 38.769013178294834,
  "median": 38.8857999999891,
  "min": 37.89329999999609,
  "max": 46.952499999984866,
  "iterations": 387
},
{
  "name": "BVHCast",
  "mean": 58.34759186046406,
  "median": 58.29505000001518,
  "min": 57.19989999997779,
  "max": 66.85099999999511,
  "iterations": 258
}

After:

{
  "name": "IntersectsGeometry w/ BVH",
  "mean": 1.6625149999998103,
  "median": 1.6542999999946915,
  "min": 1.596399999980349,
  "max": 2.8959000000031665,
  "iterations": 1000
},
{
  "name": "IntersectsGeometry w/o BVH",
  "mean": 78.6234848167548,
  "median": 78.59789999999339,
  "min": 78.13939999998547,
  "max": 79.39339999997173,
  "iterations": 191
},
{
  "name": "BVHCast",
  "mean": 42.55036628895265,
  "median": 42.52310000002035,
  "min": 41.87100000001374,
  "max": 45.99540000001434,
  "iterations": 353
}

I have tried to profile both implementations in order to figure out the inefficiency in IntersectsGeometry w/o BVH. The only difference that I've managed to find was that degenerate triangle are going into general case code path for current implementation and coplanar for my new one. (there appear to be degenerate triangles in TorusGeometry)

I think it will be worthwhile to extend benchmarking code to intersect random triangles for each corner case e.g. random coplanar, random non-coplanar non-intersecting, random non-coplanar intersecting, degenerate triangles etc.

After that I think an informed choice can be made

P.S. Sorry for raw benchmarks with no comparison numbers, compare-bench-json.js gives me a cryptic error about incorrect json format in the file. Files themselves and the paths to them appear to be correct so I dunno what's wrong. Maybe you encountered something like that before?
image

@gkjohnson
Copy link
Owner

Amazing! Very nice work. Once it's cleaned up and ready for review I can take a look and give more detailed feedback.

ExtendedTriangle is constructing bounding sphere in .update() which seems to have never been used

Yes sounds like there may be some vestigial code left over from previous iterations of different checks. Thanks for taking a deeper look.

Coplanar case contained checks with SAT axes that are cross products of triangles' edges. But that means that these axes were normals to the plane where the triangle lie. Hence they never would have been separating axes

Can you explain this a bit more? The SAT axes code works by projecting points from both onto a given axis and checking of the min / max of the projected bounds are overlapping or not. If they're not they the shapes are not touching. In the case of the coplanar triangles the normal axis check should be able to check if the triangles are on the same plane (rather than having the same normal while on different planes - eg z = 0 and z = 1).

For general case intersection implementation in this PR appears to outperform slightly against current implementation:

Amazing!

For more complex benchmarks results are inconclusive:
...
IntersectsGeometry w/ BVH

For these more complex cases it can be hard consider this a 1:1 comparsion. If there are cases where the new code is (correctly or incorrectly) detecting more triangle intersections then the "intersectsGeometry" function may finish sooner or later. Basically it's not guaranteed that the exact same number of triangle-triangle intersections are being run.

The only difference that I've managed to find was that degenerate triangle are going into general case code path for current implementation and coplanar for my new one. (there appear to be mrdoob/three.js#30533)

We should probable check for these degenerate cases and decide what to do, as well. Normals and SAT axes will not be correct in these cases so it will need a separate strategy, I think. Regarding the torus case it's been useful to find this case. But perhaps we should consider changing the torus so degenerate faces are not present once this is figured out.

I think it will be worthwhile to extend benchmarking code to intersect random triangles for each corner case e.g. random coplanar, random non-coplanar non-intersecting, random non-coplanar intersecting, degenerate triangles etc.

After that I think an informed choice can be made

This sounds like a good plan. I really appreciate your thoroughness and rigor in working through. This is the kind of code that definitely needs that.

P.S. Sorry for raw benchmarks with no comparison numbers, compare-bench-json.js gives me a cryptic error about incorrect json format in the file. Files themselves and the paths to them appear to be correct so I dunno what's wrong. Maybe you encountered something like that before?

I just ran the compare-bench and didn't see this issue. Could it have something to do with your platform? I'm running on Mac OSX.

@TheBlek
Copy link
Author

TheBlek commented Feb 18, 2025

Can you explain this a bit more? The SAT axes code works by projecting points from both onto a given axis and checking of the min / max of the projected bounds are overlapping or not. If they're not they the shapes are not touching. In the case of the coplanar triangles the normal axis check should be able to check if the triangles are on the same plane (rather than having the same normal while on different planes - eg z = 0 and z = 1).

Yes. Since separating axes of triangles are plane normal and edges, we don't need to check all the cross products of those axes in a coplanar case. Cross products of triangles' edges are going to be collinear with the plane normal which we already checked. (In the new code normal axis can also be skipped since at that point we know triangles are coplanar).

Now that I think of it, not all cross products can be skipped either: we still need to check each edge x normal. Interestingly, tests pass even though my implementation skips those axes. I guess I will need to come up with a test that breaks my new implementation and fix the problem :)

For these more complex cases it can be hard consider this a 1:1 comparsion. If there are cases where the new code is (correctly or incorrectly) detecting more triangle intersections then the "intersectsGeometry" function may finish sooner or later. Basically it's not guaranteed that the exact same number of triangle-triangle intersections are being run.

Thats a great point, I will check the exact intersection counts to see if thats the problem in the benchmark

I just ran the compare-bench and didn't see this issue. Could it have something to do with your platform? I'm running on Mac OSX.

I'm running on Windows 11. I wouldn't be surprised if the paths are messed up on windows. I will investigate it more thoroughly then

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