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

'fromPoints' is O(n), 'simpleFromPoints' is O(n log n). #74

Merged
merged 4 commits into from
Dec 29, 2020

Conversation

lemmih
Copy link
Collaborator

@lemmih lemmih commented Dec 28, 2020

Hi @noinia. How do you feel about this naming scheme where simple* functions have an (Ord r, Fractional r) constraint and check for self-intersections (ie if the polygons are actually simple)?

@lemmih lemmih added the review Ready for review and comments label Dec 28, 2020
@noinia
Copy link
Owner

noinia commented Dec 28, 2020

Hi @noinia. How do you feel about this naming scheme where simple* functions have an (Ord r, Fractional r) constraint and check for self-intersections (ie if the polygons are actually simple)?

Sounds good to me. Note that I do think we can actually check if the polygon has self-intersectsions with only (Ord r, Num r) actually (but they would still run in O(n log n) time.

@lemmih
Copy link
Collaborator Author

lemmih commented Dec 29, 2020

Sounds good to me. Note that I do think we can actually check if the polygon has self-intersectsions with only (Ord r, Num r) actually (but they would still run in O(n log n) time.

The way I did it before (by basically using Data.Ratio without the Integral constraint) had robustness issues when used with Doubles. Some polygons would trigger the intersection check when I used my trick but then had no intersections when using Doubles directly.

@lemmih lemmih merged commit 9b2d237 into noinia:master Dec 29, 2020
@lemmih lemmih deleted the feature-simple-polygon branch December 29, 2020 01:41
@noinia
Copy link
Owner

noinia commented Dec 29, 2020

Sounds good to me. Note that I do think we can actually check if the polygon has self-intersectsions with only (Ord r, Num r) actually (but they would still run in O(n log n) time.

The way I did it before (by basically using Data.Ratio without the Integral constraint) had robustness issues when used with Doubles. Some polygons would trigger the intersection check when I used my trick but then had no intersections when using Doubles directly.

We should be able to use the 4 ccw tests to test for intersection. But maybe we can let that solve itself after fixing #77.

A separate discussion is what to do about Doubles/Floating point numbers. With the better onSegment test in the visibility-polygon branch some of the isSelfIntersecting Poygon checks with doubles now fail. I'm not entirely sure what to do about issues like this. On one hand it would be nice if the algorithms could do something reasonable with Doubles, but I'm not sure how much effort to put in this since it is clear that we won't be able to get correct results for all situations anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review Ready for review and comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants