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

Fix edge case for bridge #630

Closed

Conversation

Ethan-Russell
Copy link

Fixes #629

If the nearest points to be bridged are identical, I made the outer ring continue by simply using the point rather than creating two shifted versions. Technically, it makes the resulting polygon intersect itself at that point. If we require polygons to not intersect themselves, I can amend it so that one of the vertices is shifted, then it continues with the algorithm offsetting the vertices to make a non-intersecting polygon.

@juliohm
Copy link
Member

juliohm commented Nov 9, 2023

That is a very clever fix @Ethan-Russell , thanks for taking the time to read the implementation. I am just concerned that this fix only handles the case where the intersection occurs at a point. What if the inner ring touches the outer ring on a full edge? Maybe we should try to fix the more general case with the "enlargement" idea to fix the issue once and for all?

My suggestion is that we close this PR, and start a fresh one with a new Repair transform that takes a PolyArea and "enlarges" the outer ring to make sure there are no intersections. We could then use this Repair transform inside the Bridge transform like we do with the Repair{9}. What do you think?

The list of currently implemented repairs is here:

https://github.com/JuliaGeometry/Meshes.jl/blob/master/src/transforms/repair.jl

@juliohm
Copy link
Member

juliohm commented Nov 9, 2023

Another reason that I am concerned with this fix here is that discretization algorithms like Dehn1889 assume simple polygons. In particular, they assume that there are no overlapping edges. When we introduce a single point in and out of the hole, we introduce two overlapping edges.

@juliohm
Copy link
Member

juliohm commented Nov 13, 2023

@Ethan-Russell I will close this as discussed above.

Please let us know if you will have some time to dedicate to this fix.

@juliohm juliohm closed this Nov 13, 2023
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.

Bridge returns NaNs for PolyArea with intersecting Rings
2 participants