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

Refactor connect edges #10

Merged
merged 23 commits into from
Feb 3, 2020

Conversation

bluenote10
Copy link
Contributor

@bluenote10 bluenote10 commented Jan 30, 2020

This PR follows the ideas of w8r/martinez#113, combined with a few more fixes, and translated into a more Rust friendly structure. Basically the JS implementation was missing a key step of the Martinez algorithm in the second stage -- the implementation of the 4 possible cases for determining parent contours (Fig. 4 in the paper):

image

The idea is that for each start point of a contour we look at the lower ("previous in result") contour and check:

  • if there is no contour below => current contour must be an exterior contour (case 1).
  • if there is a lower contour, but it is an inside-outside transition => current contour must also be an exterior contour (case 3).
  • if there is a lower contour, and it is an outside-inside transition => in this case, the current contour is a hole, and depending on whether the lower contour is also a hole or an exterior contour we can establish the parent/hole relationship (cases 2 & 4).

I also tried to document the ideas with code comments as best I could.

This PR fixes / affects:

Here is a visualization of all new / modified test cases:

test_cases.pdf

A few minor changes:

  • The biggest debugging struggle for me was caused by the inability to display/debug print the Float trait. Omitting the Debug/Display traits from Float doesn't do anyone a favor who gets to debug Float based code. Hope it is fine to extend the trait, because Float will be almost exclusively instantiated by f32 or f64 anyhow.
  • I've added the Python plotting script I use for visualizing the test cases in a scripts subfolder. It is ~200 LOC and I thought it might be useful for anyone testing the algrithm. I have also documented how the test scheme works in a short readme.

@bluenote10 bluenote10 requested a review from untoldwind January 30, 2020 13:31
@untoldwind untoldwind merged commit a99cac8 into 21re:master Feb 3, 2020
@untoldwind
Copy link
Contributor

I admit that I do not entirely get the changes to the algorithm, but that's what test-cases are for ;)

I did a minor code-cleanup in master, mostly involving formatting and clippy.
On a side-note:
It's always a good idea to run
cargo fmt
and
cargo clippy
regularly.
Especially the latter gives a lot of insights

@agersant
Copy link

agersant commented Feb 9, 2020

Thanks a lot for working on this @bluenote10! These bugs were the roadblock for me to consider replacing my usage of gpc with a Rust solution.

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.

3 participants