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

signed_distance returns different answer if polygon is explicitly closed or not #57

Open
skygering opened this issue Feb 13, 2024 · 7 comments
Assignees
Labels
bug Something isn't working

Comments

@skygering
Copy link
Collaborator

import GeometryOps as GO
import GeoInterface as GI

pt = (1.1, 1.0)
rect_open = GI.Polygon([[(1.0, 1.0), (1.0, 2.0), (2.0, 2.0), (2.0, 1.0)]])
rect_closed = GI.Polygon([[(1.0, 1.0), (1.0, 2.0), (2.0, 2.0), (2.0, 1.0), (1.0, 1.0)]])

GO.signed_distance(pt, rect_open) # return 0.1
GO.signed_distance(pt, rect_closed) # return 0.0

@skygering skygering added the bug Something isn't working label Feb 13, 2024
@skygering skygering self-assigned this Feb 13, 2024
@asinghvi17
Copy link
Member

Ah oops! I imagine we should probably filter that based on first + last coordinate...

@skygering
Copy link
Collaborator Author

It's annoying because GI.Polygon doesn't enforce repeated points, but we should still assume they're closed. This is related to #46, but an example of it biting me in the butt 😆

@rafaqz
Copy link
Member

rafaqz commented Feb 13, 2024

Not so easy to enforce repeats because we can wrap other polygon types

We could have a repeat bit we set at construction that adds an extra point to getpoint...

@asinghvi17
Copy link
Member

Or just add the extra point to the end if necessary

@rafaqz
Copy link
Member

rafaqz commented Feb 14, 2024

Hmm this means both GI.getpoint and GI.npoint for GI.LinearRing do a comparison of the first and last points and add the extra point or the +1 when necessary?

Wonder how expensive that is - mostly what it does to the iterator GI.getpoint(::LineString) having a conditional in there for every point access. Probably significant compared to directly accessing a vector.

@asinghvi17
Copy link
Member

Ah yeah, that would be expensive...I was thinking we could do this at construction, but it wouldn't solve the problem for external types. I suppose it would add a bit more complexity since most methods would be expecting a closed polygon as well, so they'd have to check for that each time they were called, which is suboptimal.

@rafaqz
Copy link
Member

rafaqz commented Feb 14, 2024

Probably we can add a close argument to apply that will close any LineaRingTrait it encounters (and LineString when its in a Polygon for some extra complexity)?

Then we could to tuples(obj; close=true) and just take the conversion hit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants