DNM: Updates to Segment.intersection Method

[sayas01]

Description:

This PR updates the Segment.intersection method to match the changes in the Segment.intersects method introduced in the PR #373, to fix the issue #395.

The point of intersection is calculated as before except that if the new intersects method returns true and the check for intersection point in the Segment.intersection returns false, then the point is calculated by a new method, intersectionAtEnd, with the same logic as intersection, but with longs and dm7.
Note:
Since the PR #373 was to address the case when two segments intersect at end points, returning the end point of either of the segments should be enough if the intersection method returns null but intersects method returns true. I tried out a few examples which suggested the same. I can update it to return the end point if Jack confirms the same as he has a better understanding of the edge cases that resulted in the updates.

Potential Impact:

Fixes the issue osmlab#395

Unit Test Approach:

Added a unit test to test the enhancement.

Test Results:

All tests passing.

---

In doubt: Contributing Guidelines

[Bentleysb]

Is there some reason we would not always want to use this version of the function?

[sayas01]

I had tested it out on a few cases and the result with degrees returns the most accurate intersection than with long. Like I mentioned in the PR description, returning the end point would be ideal instead of actually calling this function, but there may be some cases I am not aware of.

[Bentleysb]

It sounds like the next step should be to confer with Jack, then.

[danielduhh]

This PR has a lot of potential impact, so I'm temporarily blocking this until we have..

1. expansive tests, and test results that verify no side effects (may need original edge cases here)
2. sign off from jack (we could just have him review it afterwards)

Also, the PR description can use a little work. Do you mind elaborating a bit on why you're using Longs and dm7? Thanks @sayas01 !

[jklamer]

Yeah, the long version should be the default, and only in the case of overflow should you use the double version of the function. But the general idea here is looking good. The only reason I say long version first is because in real map data the overflow is very unlikely, and thats the only function where we can differentiate between the edge case and the segments that are truly not intersecting. Right now for every segment pair that doesn't intersect this runs the calc twice because we can't tell if its an edge case or not, which is expensive given how often this function is called.

To do the overflow checking, you can use the Math.subtractExact in the s and t value calculations, and catch the ArithmeticException, at which point, do the double version of the function. Then you also won't need to do the intersectscall at the jump I believe.

[jklamer]

please remove when possible (details in full review)

[jklamer]

Can you add and intersection call to the overflowing example as well? The intersection should be Null Island.

[sayas01]

Hi @jklamer, since the only changes to the Segment.intersect method was to use long and dm7 as the default and fall back to double only when the arithmetic exception occurred, I had first updated the Segment.intesection method to use long and dm7 as default and fall back to double on null. But since the svalue and tvalue is computed by arithmetic division, using long will always produce zero; in which case, the intersection point will be one of the location of the intersecting segments. For proper division, I can cast the values to double in which case, the intersection location will be calculated using Latitude.degrees() and Longitude.degrees(). But it results in values outside -90 degrees -> 90 degrees.
Using degrees gives precise location and if the intersection happens at any of the end location, then the value can be computed using long. Let me know what you think about this approach.

[jklamer]

So if you cast the longs to double you find you get values outside -900000000 900000000 ?

I suppose I would be okay if we used the long method only if the double method produced s and t values very close to 0 and 1 but outside the inclusive range. Right now it looks like it always try the long version.