Fix race condition when connecting coincident holes #120
Conversation
src/earcut.js
Outdated
| @@ -344,7 +344,8 @@ function findHoleBridge(hole, outerNode) { | |||
|
|
|||
| tan = Math.abs(hy - p.y) / (hx - p.x); // tangential | |||
|
|
|||
| if ((tan < tanMin || (tan === tanMin && p.x > m.x)) && locallyInside(p, hole)) { | |||
| if ((tan < tanMin || (tan === tanMin && (p.x > m.x || area(m, m.next, p.next) > 0))) && | |||
There was a problem hiding this comment.
I think the issue is that if the tangent is vertical (tanMin == +-Inf), and there are multiple possible bride points on that tangent, there is an issue, because it chooses the first one, instead of the closest. Instead of p.x > m.x, it should probably check the distance p.x*p.x+p.y*p.y > m.x*m.x+m.y*m.y. I'll look into it later
There was a problem hiding this comment.
No, it's not that. See this picture of when it fails:
For the last hole, there are multiple nodes in the same spot, with edges going into different directions. We're picking the node that belonged to the second hole (producing invalid connection), rather than picking the node that has the diagonal edge. So, to fix that, for multiple candidates with the same exact coordinates, we want to pick one with an edge closest to the hole point when going counter-clockwise. So I think the fix direction is right, I'm just not confident it covers all similar cases. But better than nothing.
|
Hold on, this fix introduces some regressions — will share more test cases... |
|
@mrgreywater here's a set of clean polygons that are failing for I'm fried for today but will keep cracking on this tomorrow... Any help appreciated though. |
|
Merging now because we need this fix urgently in production, but I'll be glad to accept any PRs that improve / simplify the fix while keeping the tests happy. |
|
I have looked at these PRs and while I can't see anything wrong with them per se, I don't like the fact that I can't really judge the impact on all edge cases. If you tested them all on a 190k dataset and see an improvement, I'm all for it. Personally I think we should have some kind of pseudo-random test case generator, so we can get some idea if we're working in the right direction. |
|
@mrgreywater yeah, I understand — sorry for the rapid changes! Alternatively we could establish a set of real world test cases we can use — e.g. take some very difficult open dataset like terrain contours, slice it into vector tiles with Tippecanoe with a defined set of options (so that we multiply the cases with clipping and simplification / grid snapping across zoom levels), and run earcut-reduce against it (the tool I currently use on that private dataset I can't share in full). |
Closes #119. When performing hole elimination, there was a rare edge case where there were multiple candidate points for connection that are all visible to the hole point, but some lead to incorrect winding when connected. This PR should make sure the topology is correct when picking connection points.
I'm not entirely sure this is the best fix, but it fixes the bug we encountered in real data while not regressing the test suite, so I'm inclined to merge.
cc @mrgreywater