Fix @turf/nearest-point-on-line endpoint selection and degenerate input cases

[bratter]

Summary

This pull request addresses several issues with @turf/nearest-point-on-line, specifically:

• Resolves Bug: nearestPointOnSegment returns incorrect endpoint when in opposite hemispheres #2934 where the nearest endpoint was sometimes incorrect due to issues with the angular comparison
• Resolves @turf/nearestPointOnLine Coordinates must contain numbers #2939 where the short circuit introduced in turf-nearest-point-on-line: Fix issue #2808 #2849 only catches exact equality, but precision loss / calculation issues can mean exact equality is too strict to avoid all degenerate cases
• The reworked short circuit logic still ensures fix from turf-nearest-point-on-line: Fix issue #2808 #2849 remains
• Added additional checks for possible degenerate cases (see commit notes for details)
• Clamped input to asin to prevent calling asin with z > 1 due to floating point imprecision
• Introduced throw to catch degenerate case where line segment is constructed from antipodal points

In addition to behavioral fixes, the PR removed some unused calculations and used some cheaper comparisons that avoided trig functions. Overall benchmark performance increased by ~60%.

Potential Discussion Topic

The snippet below shows the catch for degenerate cases. This introduces an explicit exception when the input line segment's points are exactly antipodal. This is highly unlikely to happen in practice as radian conversion tends to throw even simple input cases slightly out at the bit level, and also seems to not be the usage paradigm for this function, but I guess it is at least possible.

I went with the exception, but it also would have been possible to just return the point being exactly on the segment. This is sort of valid as there are infinite great circles through antipodal points, and one of them will also cover the target point. This approach could lead to some strange behavior, but is maybe better than injecting an exception.

Open to changing if consensus is to take the point as on the line.

  if (segmentAxis[0] === 0 && segmentAxis[1] === 0 && segmentAxis[2] === 0) {
    if (dot(A, B) > 0) {
      return [[...posB], true];
    } else {
      throw new Error(
        `Undefined arc segment, line segment endpoints [[${posA}], [${posB}]] are antipodes`
      );
    }
  }

Testing Note

#2939 was reproducible in the browser but not in node due to some floating point issues described in the issue. To make sure there was a working test (that failed without the fix) I monkey patched Math.cos to make the test work. It isn't great, but it works.

Checklist

Please provide the following when creating a PR:

• Meaningful title, including the name of the package being modified.
• Summary of the changes.
• Heads up if this is a breaking change.
• Any issues this resolves.
• Inclusion of your details in the contributors field of package.json - you've earned it! 👏
• Confirmation you've read the steps for preparing a pull request.

[HarelM]

I'm not sure I fully understand this part.
the bottom line from my perspective is that I don't expect nearest-point-on-line to ever throw.
So if this is caused by ambiguity (if I understand what you wrote above) then I would expect this to return the first one, even if it's not deterministic, I would still prefer that over a method that throws...
I might have completely misunderstood what you wrote, but I also couldn't see a test to over this code flow, so do let me know what I'm missing.

[bratter]

Yeah, I totally get what you are saying and could go either way. In case its helpful, I'll lay out all the conflicting thought process:

This case should only be hit when the input line segment is made up of two points that are antipodal. Theoretically, for instance, [0, 0] and [180, 0], but practically doesn't quite work like that because of floating point. This is mathematically undefined because the is an infinite number of valid great circles that pass through antipodal points.

So the two options are:

1. Throw for undefined behavior:
   1. This give stricter mathematical correctness, which is good in some cases but may not be necessary when the focus is pragmatism.
   2. Gives the caller an opportunity to handle in the best way for the application - for instance us picking the right answer may not be the best in animation scenarios where it might cause snapping etc.
   3. Downside is of course that it throws and throwing is always less than ideal, people won't handle it, and might crash the application if not caught.
2. Assume the point lies on the line:
   1. More permissively, we can simply choose one of the possible great circles. The only one that really makes sense is the one that also passes through the test point, making the point exactly on the line. I don't think there are any other options that would possibly make sense.
   2. This avoids any exceptions in nearest-point-on-line, which as you point out seems like a reasonable expectation.
   3. Determining that the point on the line in this case is not entirely logically unsound, and would likely be a reasonable result in almost all circumstances, especially given this antipodes case is unlikely to occur in practice.
   4. Antipodal points in this kind of method are kind of crazy anyway, and I doubt are seen at all in practice - and if it mattered, you'd think the geometry would include an intermediate point to disambiguate.

So I can see advantages each way. Wanted to drop the more correct version in for the first pass, but definitely open to changing.

[HarelM]

If someone needs to know that this might be problematic you could export a method called isAntiPodal to allow testing it if someone is concerned about the results of this method.
This avoid throwing and allows to decide what to do in this case if this interested you.
Another option is simply to call it out in the method docs so that people might know this is a "know limitation".

[bratter]

Makes sense, adding to the docs would work.. I like not throwing. I'll do in the function signature for here now, and then can do in the website docs later.

Noticed another issue with one of the comparisons that I didn't catch before so need to make a further update, so will amend this PR with both fixes shortly.

[HarelM]

Looks good!

[HarelM]

The math is a bit too complicated for me to dive into, but if this passes all the previous test cases and also the new test cases from the linked issues I believe this is a good improvement.
Thanks for this!
Would be great to have a new version with these fixes.

[bratter]

Updated to fix another issue and replace the throw with an early return.

[mfedderly]

Thank you! One comment seems to have been abandoned but overall looks good.

[mfedderly]

Hanging Note that in the comment

[bratter]

Ah whoops. I'll fix that when I get a chance and also look at the point-on-polygon tests that failed. These were succeeding when I tried earlier, but are worthwhile checking into due to the floating point funniness.

[bratter]

I was able to confirm that the failed tests in turf-point-to-polygon-distance are due to a single bit difference in the latitude of the nearest point between the old version and the new version. See below for an example from the first failing test:

Latitude from the nearest failing point
Old: 52.47479359036524 => 0 10000000100 1010 00111100 11000110 00001001 01001111 01111100 00001100
New: 52.47479359036523 => 0 10000000100 1010 00111100 11000110 00001001 01001111 01111100 00001011

Then as the result is in meters, the single bit difference creeps up. Yay for floating point :-).

Given that a single bit floating point difference is inside even the boundaries of trig functions in different JS engines I see no issue in just changing the expected distance, although it does beg a larger questions as to whether those tests should test with a tolerance like some others do... problem for another time.

I will amend and update the PR.

[mfedderly]

Thanks for the detailed readout from this investigation!

[HarelM]

Nice! Do let me know when a new version is available.

[mfedderly]

Ah the test failure is just a merge conflict with a new test that was just introduced. Almost certainly a similar issue with the floating point representation. Sorry!

[bratter]

Yes indeed - sorry for the messy commit history.