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

Allow self-crossing rings #30

Closed
mfogel opened this issue Aug 5, 2018 · 3 comments
Closed

Allow self-crossing rings #30

mfogel opened this issue Aug 5, 2018 · 3 comments
Labels
enhancement New feature or request performance Possible performance improvement
Milestone

Comments

@mfogel
Copy link
Owner

mfogel commented Aug 5, 2018

Self-crossing rings are ambiguous because they could be interpreted using either the non-zero rule or the even-odd rule. As such, the algorithm throws an exception, by design, when given a self-crossing ring. This behavior is covered by the test suite.

Right now, checking for the self-crossing rings is done by a separate loop over the output segments before the algorithm attempts to glue them together into output shapes.

It may be possible to eliminate this loop by doing this check right when a segment is split by another segment, and at that point if the two segments are from the same ring, and they do cross (not just touch) then throw the error there.

@mfogel mfogel added the performance Possible performance improvement label Aug 5, 2018
@rowanwins
Copy link
Contributor

Hi @mfogel

I was under the impression that the martinez algorithm was able to process polygons with self-intersections. Or is this about where holes cross the external boundary of the polygon?

Tied with all of this is and perhaps more overarching concern is how to cater for degenerate inputs... you could make the algorithm attempt to handle them by running a zillion checks, the downside being that it slows down performance for everyone. Or you could assume people pass in decent inputs and things run a bit quickly because minimal checks are done. Perhaps a middle of the road option is to provide some sort of clean function which people can use if their first pass throws an error...

@mfogel
Copy link
Owner Author

mfogel commented Aug 10, 2018

good point @rowanwins - specifically "attempt to handle them (degenerate inputs) by running a zillion checks, the downside being that it slows down performance for everyone". It's definitely possible to go to far to the side of trying to protect developers from themselves. In this particular case, this is probably currently a step too far in protecting developers from themselves - as there's a whole O(n) loop there just to make sure a nice error is thrown in this case.

The wikipedia page for non-zero rule has a nice image that shows how some self-crossing rings enclose different area depending on whether you interpret the ring using the non-zero rule or the even-odd rule. And if it's not clear what area the input rings enclose, then when forming the output rings at the end of the algorithm, it's not clear what area should be enclosed.

Another option would be to just choose either the even-odd rule or the non-zero rule, put that choice in the documentation and be done with it. That's probably the right answer if this error check can't be reduced from O(n) to O(1).

@mfogel mfogel changed the title Better way to detect self-crossing rings Allow self-crossing rings Aug 19, 2018
@mfogel mfogel added the enhancement New feature or request label Aug 19, 2018
@mfogel
Copy link
Owner Author

mfogel commented Aug 19, 2018

I can't see any way to perform this check in anything less than O(n). As such, I'm changing to this issue to permit self-crossing rings, and just going to document in the README how they'll be interpreted.

@mfogel mfogel closed this as completed in b35adf9 Aug 19, 2018
@mfogel mfogel added this to the 0.8 milestone Aug 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request performance Possible performance improvement
Projects
None yet
Development

No branches or pull requests

2 participants