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

Robust checks for orientation #19

Merged
merged 11 commits into from
Oct 11, 2021
Merged

Robust checks for orientation #19

merged 11 commits into from
Oct 11, 2021

Conversation

andreesteve
Copy link
Collaborator

This PR addresses the robustness issue tracked in #15, based on mapbox/delaunator/pull/68

Changes

  • Takes dependency on robust, a pure rust implementation of Shewchuk's robust predicates
  • Uses robust::orient2d implementation for the orient() checks
  • Split the test fixtures from a single test case into separate tests to make it easier to see errors when running cargo test
  • Adds a example that generates a svg of the triangulation
  • Brought in the additional fixture tests from the javascript implementation

Benchmark

The robust checks are slower by about ~8-10%

triangulate/100         time:   [18.330 us 18.350 us 18.392 us]
                        change: [+6.7385% +7.6582% +8.8075%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 1 outliers among 20 measurements (5.00%)
  1 (5.00%) high severe
triangulate/1000        time:   [275.33 us 276.51 us 277.46 us]
                        change: [+12.091% +12.617% +13.055%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 2 outliers among 20 measurements (10.00%)
  2 (10.00%) high mild
triangulate/10000       time:   [3.4929 ms 3.4974 ms 3.5028 ms]
                        change: [+8.8572% +9.3444% +9.7613%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 2 outliers among 20 measurements (10.00%)
  2 (10.00%) high severe
triangulate/100000      time:   [50.839 ms 50.982 ms 51.150 ms]
                        change: [+6.2803% +6.6616% +7.0349%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 1 outliers among 20 measurements (5.00%)
  1 (5.00%) high mild

Commentary

During testing, I was not able to reproduce issue mapbox/delaunator/issues/61 with or without this change.

For example, this is what the triangulation output looks on current master:

image

This is what it looks like with the new orient2d checks:

image

And this is what it looks as reported on the original issue:

image

While testing, I came across #18, so I added the checks for connectivity on the test case but left commented out for now, until we can address that issue (which is unrelated to this).

Additionally, this change may lead to supporting the ask in #4, at least for f32 if we move from our Point struct to the Coord struct defined in the robust crate.

Overall, the performance impact is significant but worth it in my mind to guarantee correctness. I am just unsure about these changes because I couldn't repro the original issue yet. Please let me know your thoughts.

@andreesteve andreesteve requested a review from mourner June 17, 2021 05:24
@mourner
Copy link
Owner

mourner commented Jun 17, 2021

@andreesteve this is great! The perf hit is definitely worth it, although it seems to be much bigger than the JS equivalent — perhaps an issue with the robust crate code which might have room for optimization.

I'm not sure why the original issue isn't reproducible, but it's worth checking whether all the other "robustness" test cases pass the validation without the robust orientation, and additionally the test case reported in #10.

Not sure where the connectivity issue comes from — needs investigation (debugging the same case side by side with the JS version to see what goes differently).

@andreesteve
Copy link
Collaborator Author

@mourner - to make sure I am not missing anything, when you talk about the robustness checks you are talking about the triangle vs hull area error check done in https://github.com/mapbox/delaunator/blob/master/test/test.js? I see we have the same checks for our test cases.

Thanks for pointing to issue #10 - I added a test case to cover it. Interestingly enough I get a visually correct triangulation with and without the orient2d changes in this PR. Also the area checks and half-edge connection checks pass in both master and this robust branch.

I have added all the robustness test inputs from the JS:

     Running tests/tests.rs (target/debug/deps/tests-9dd7cff8b40a6e26)

running 15 tests
test bad_input ... ok
test issue11js ... ok
test issue24js ... ok
test issue43js ... ok
test issue13js ... ok
test robust6 ... ok
test robust5 ... ok
test issue10 ... ok
test robust2 ... ok
test unordered_collinear_points_input ... ok
test robustness ... ok
test robust4 ... ok
test basic ... ok
test ukraine ... ok
test robust3 ... ok

test result: ok. 15 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.04s

   Doc-tests delaunator

running 1 test
test src/lib.rs - (line 6) ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.17s

I was wondering if this could be hardware specific, but I got the same output on the VM I host my blog. I'll ping the person that reported the bug and see if they would be able to try that input with this new changes.

For the connectivity, I'll investigate that and send a separate PR.

@andreesteve
Copy link
Collaborator Author

Hi @mourner ! Sorry for the hiatus. I got time to get back to this and tackle the last open items from #15 related to robustness:

I also validated that these changes fix #10 - I added all the details and screenshots on the issue's discussion page.

With this, #15 is almost done. The only thing left is adding the update method, but I'd prefer to do that on a separate PR since this is too big already!

I think this is good to merge. Please let me know your feedback. Thanks!

Copy link
Owner

@mourner mourner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andreesteve amazing! Thanks so much for working on this!

@mourner mourner merged commit 6bcc939 into master Oct 11, 2021
@mourner mourner deleted the robust branch October 11, 2021 09:20
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.

None yet

2 participants