turf icon indicating copy to clipboard operation
turf copied to clipboard

turf-nearest-point-on-line: fix for points far away from the segment endpoints

Open mlc opened this issue 10 months ago • 2 comments

Fixes #2870; see that ticket for a description of the observed bug.

This patch fixes the described bug by adjusting how nearestPointOnSegment picks an endpoint when it determines that the nearest intersection of the great circles is not on the segment. Rather than doing work with angles, it just computes the distance to the two endpoints and picks the closer one.

I think this is safe—it does not break any existing tests—and should not lead to a performance reduction (there are just as many calls to distance as there were before, and some vector math has been removed), but the algorithms here are a little bit outside of my wheelhouse, so I would certainly appreciate any feedback if there is something wrong. A more natural way of fixing the issue might be to correctly choose the right value of I in the section above the one I modified, but I can not quite figure out how to make that work.

Thanks so much for turf and for your attention to this!

Please provide the following when creating a PR:

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

mlc avatar Apr 22 '25 21:04 mlc

Ah sorry @mlc - I missed this when creating #2940. I had the same issue but took a bit of a different approach to fixing and also found a few other edge cases in the process. #2940 will conflict with this but fixes additional issues and also speeds up the algorithm. Happy to discuss.

bratter avatar Nov 04 '25 16:11 bratter

@bratter I don't feel strongly about my solution—if yours is better, I'm all for it.

mlc avatar Nov 04 '25 16:11 mlc