test and fix for #154

[ondras]

This commit introduces a trivial fix for #154. All line segments are now tested. A unit test is included as well.

[Fil]

Thank you for the fix! (I'm the culprit here.)

I think we can make a few more changes:

1. Optimize by not computing the same distance twice for each point.

2. In the tests, it would be nice to add points that do not belong to the line's coordinates (some of which should be contained, some which shouldn't).

3. Finally, seeing Mike's drawing and thinking again about the formula:

It appears that, when the point tested is close to a vertex, the value A = ao+bo-ab is close to the distance from the point to the line (d), but if we are "in between" it is more like the square of that distance (A ~= d**2) — so our current test is very laxist and will say that a point belongs to a line segment if its distance is less than ~sqrt(epsilon), i.e. 1e-3.

I don't want to derive the exact formula in spherical coordinates, but, if I'm not mistaken, a better approximation of the distance would be

d**2 ~= (ao + bo - ab) * (ab**2 - (ao - bo)**2) / ab

meaning that, if we want to check that "O is between A and B" and "d < ~epsilon", we should test with

(ao + bo - ab) * (ab**2 - (ao - bo) ** 2) < epsilon2 * ab

(and make sure that ao <= ab and bo <= ab)

I've set up a notebook here to explore the formula
https://beta.observablehq.com/d/39e5f445a82cbff8

[ondras]

Hi @Fil ,

thanks for your comments!

1. Optimize by not computing the same distance twice for each point.

Makes sense. I will try to refactor, although that would break the current clean functional containsLineSegment separation :)

2. In the tests, it would be nice to add points that do not belong to the line's coordinates (some of which should be contained, some which shouldn't).

Why not. However, this also holds for other tests for this D3 module. Perhaps a separate issue/PR shall be open for this?

3. Finally, seeing Mike's drawing and thinking again about the formula:

I provided my own (simpler, probably less correct) approximation in #153 (comment); I'm going to fork your observable notebook to see how my formula turns out.

[Fil]

I have just added a few changes as per my previous comment. Would you like to review them? The containsLineSegment was nice but I feel it's better to avoid doubling the number of distance computations.