Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Robustify compare segments #13

Merged
merged 24 commits into from
Mar 9, 2020

Conversation

bluenote10
Copy link
Contributor

@bluenote10 bluenote10 commented Mar 1, 2020

I think I have a reasonable bundle for another update (although there are still open issues...).

This PR fixes a few issues around ordering segments in the sweep line. The most challenging issue was the following problem, already alluded to here:

2020-03-01 19 47 34-2

Normally when comparing two line segments it suffices to look at the left point, and if it is below or above the other segments, the segment is below/above. Special handling is only triggered when the left point falls perfectly on the line (according to the signed_area == 0 check). This leaves an ugly edge case:

  1. The signed area check is non-zero (like here for the left point of B). In the example, the algorithm sees segment B below segment A.
  2. Later in the algorithm intersections are computed w.r.t. the neighbors in the sweep line, i.e., A and B are checked for intersections. If an intersection does not fall onto an endpoint, the segment gets sub-divided, and the subdivisions become separate segments in the sweep line. The problem: For segment B the intersection falls onto its endpoint, so it cannot be subdivided. This leaves segment B in its original form, and its position in the sweep line is wrong: still below A, although it really should be above A.

My ideas to work-around this inconsistency were:

  • Give up the limitation that segments cannot be subdivided into a 0-length / 100%-length ratio (solution in stage 2): This seems to be like an obvious solution, just push another version of B that is above. But I'm not sure if it could be done easily. The challenge is to avoid the infinite loop that a naive implementation would produce: If we push exactly the same segment B into the event queue, in the next iteration we would simply process exactly the same -- nothing changes => infinite loop. I haven't really looked for solution to that because zero-length segments would lead to other trouble.
  • Backtrack from wrong order (also a solution in stage 2): After computing the intersection point, and making the "is segment actually dividable check" we could add a consistency check, if the two segments at hand actually have the correct order. This seems dangerous and difficult though, because we would at least temporarily have an wrongly ordered sweep line, and modifying the order by other means than direct comparison calls for trouble.
  • Upfront check (solution in stage 1): When inserting the segments (initial comparison), we could already compute the intersection point, see the problem and switch to the comparison logic identical to the "left point on line" case.

Solution 3 is most straightforward, so I went for that. Obviously this adds a bit in terms of run time. I made sure to compute the intersection lazily only if necessary to keep the impact small. Currently I see a 5% - 10% performance impact. However, I already have a few ideas how to optimize performance eventually (only really makes sense once the logic has fewer bugs), and the algorithm is crazy fast anyway -- less than 1 ms total run time even on somewhat complex polygons.

Other bug fixes / improvements:

  • Fix for Wrong result for two simple polygons w8r/martinez#110: Kind of the smaller brother of the issue above where the left endpoint falls onto the line perfectly. The JS version has a long standing bug of always returning "smaller" in that case (i.e., it only gets the upside-down case right).
  • Another problem of the JS implementation in compare_segments: The comparison returned "equals" on equal values, which was messing up test cases with self-overlapping edges. I feel like it is easier to support self-overlapping edges properly by using "equals" only for identity. Fixes Incorrect union on small example #12 (the main reported issue).
  • Another problem of the JS implementation that looks a bit like typo: The two recompute-fields-lines (in subdivide_segments in the intersection with "next" branch) call compute_fields two times on event itself. It occurs to me that it should be called once for event and once for next, similar to how the branch looks like in the "prev" case. This is required to fix the other issue mentioned in Incorrect union on small example #12 and test cases overlapping_segment2 and overlapping_segment3.
  • In general I came across test cases which worked in its current version, but when swapping the roles of the subject/clipping polygons they were broken (I mean for commutative operations union/intersection/xor). I took this as an opportunity to extend the test suite to not only test for op(A, B) == result but also op(B, A) == result. Currently there are still 2 test cases which are broken with swapped operands (marked as such in the PDFs), TODO for later.

Rust specific changes:

  • An issue in the Rust implementation: The "other event" access in possible_itersection had a minor problem, because it didn't account for changes of the "other event" pointer. Fix is required for overlapping_segment2.
  • The generic test case execution now uses std::panic::catch_unwind. This means the tests will no longer fail/panic on the first failing test, but rather run through all the test cases and report a failure summary. This is super convenient for getting a better understanding of the algorithm, because one can add e.g. debug asserts and see how many / which test cases would be affected.
  • To simplify the debugging work I decided to introduce a feature flag, which is used internally for conditional compilation of debug output. In general I try to keep my code free from debugging code, but the algorithm is so hard to debug that I spend a significant amount of time of adding/removing/optimizing debug code. I think a feature flag makes sense, because this allows to keep the debug stuff as clean as possible, and users of the library are not affected at all.

Fixed / new test cases: test_cases.pdf

@bluenote10 bluenote10 changed the title Feature/robustify compare segments Robustify compare segments Mar 1, 2020
@bluenote10
Copy link
Contributor Author

CC @untoldwind (I assume you don't get notified by PRs since you are not among the Watchers, and for some reason I can't assign you as a reviewer this time).

As a note to myself:

Give up the limitation that segments cannot be subdivided into a 0-length / 100%-length ratio: [...] The challenge is to avoid the infinite loop ...

Probably this can be solved by introducing a flag for each segment with the semantics "second attempt" or rather "don't trust the left endpoint". If we encounter the case of an intersection on the left endpoint in the second stage, we could re-push the segment into the sweep line with the flag set to true. And in this second attempt compare_segment would switch to a different logic and sort the segment according to its right endpoint this time.

The solution is still a bit more error prone, because when ordering a segment in a second attempt we still have to order the same segment properly with respect to other segments (where we have to use the left endpoint for the comparison to get a correct result), and only use the right endpoint in the particular case shown above. Clearly something for a future experiment.

@untoldwind untoldwind merged commit 7f522fd into 21re:master Mar 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect union on small example
2 participants