FIX: Fix polyfill bug when vertex latitude exactly matches cell center #603
Conversation
| if (lat == a.lat || lat == b.lat) { | ||
| lat += DBL_EPSILON; | ||
| } |
There was a problem hiding this comment.
What happens if the latitude is maximally north already? I assume there won't be any issue here?
There was a problem hiding this comment.
You may be correct - if we're checking the latlng point [90, 0] (i.e. the north pole), this algorithm will I think fail to contain it in any polygon. But because we're using Cartesian polygons to begin with, there's no pole-enclosing polygon anyway. This is all moot for the polyfill usage, however, because the north pole cell at res 15 has a center latitude of 89.99999581738042, so it's not possible for this issue to arise. If we offered point-in-poly as an external function, this might be an issue, but probably one we could address in documentation.
I suppose we could address this issue with
if (lat > M_PI_2) {
lat = M_PI_2 - DBL_EPSILON
}
but I don't think this is necessary for our current usage, and not worth the extra branch.
There was a problem hiding this comment.
So the lat and lng here is provided by the caller, so this is theoretically a problem, but we just use the centerpoint of the polygon in reality:
Line 1004 in ba89a19
There might be a problem with pointInsideLinkedGeoLoop, though, since it just uses the first point which could be the northernmost one:
Line 191 in adde6da
There was a problem hiding this comment.
The pointInsideLinkedGeoLoop function is looking at whether one loop is inside another. So for this issue to occur, you'd need a fairly pathological shape, where not only was one vertex of the outer loop on the north pole, but one vertex of the inner loop was also on the north pole, eg.
But again, the loops involved here are derived from H3 cell boundaries, and there is no cell with a center or a vertex on the north pole.
There was a problem hiding this comment.
Perhaps add a comment explaining the assumption that only cell centers or vertexes are expected to be compared here against the user input?
There was a problem hiding this comment.
Added a long comment noting the issue.
| if (lat == a.lat || lat == b.lat) { | ||
| lat += DBL_EPSILON; | ||
| } |
There was a problem hiding this comment.
Perhaps add a comment explaining the assumption that only cell centers or vertexes are expected to be compared here against the user input?
|
Ha! Taking the W. |
Fixes #595
The issue here (I think?) is that rays cast directly through a vertex of the polygon were being counted twice, once for each adjoining segment. This uses the same "bias in an arbitrary direction" approach we used for longitude matches, which should also ensure no overlap between contiguous polygons, which I think was previously an issue.