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 handling of incomplete touching inners #343

Merged
merged 7 commits into from
Jan 28, 2021

Conversation

tyrasd
Copy link
Member

@tyrasd tyrasd commented Jan 27, 2021

Description

Fixes a bug which caused an infinite loop in the cutAtSement method (used in mergeTouchingRings routine) because it is called with non-closed linestrings instead of rings. Fixed by making sure that buildRings does only return valid rings.

This has the side effect that some errorneous multipolygons now get "cleaned" up slightly into (smaller) polygons, where they were returned as GeometryCollections before (prior to 0.6.0). But since the old (buffer(0), see #249) implementation also had similar side effects, I see no major harm in this change.

Checklist

@tyrasd tyrasd added bug Something isn't working as expected priority:high Should be addressed as soon as possible (next release) labels Jan 27, 2021
@tyrasd tyrasd added this to the release 0.7.0 milestone Jan 27, 2021
@tyrasd tyrasd force-pushed the fix-incomplete-touching-inners branch from 63e0395 to a1cf76c Compare January 27, 2021 14:27
@tyrasd tyrasd requested a review from joker234 January 27, 2021 14:27
@joker234
Copy link
Member

Is the Milestone 0.7 fitting to this issue even tough we want to fix that directly for the 0.6 version?

@tyrasd
Copy link
Member Author

tyrasd commented Jan 28, 2021

Is the Milestone 0.7 fitting to this issue even tough we want to fix that directly for the 0.6 version?

I wasn't super sure about that either, but adding it to the 0.6 milestone seemed "wrong" because it's already closed; maybe not adding it to any milestone would be better? My thinking was: ok, we fix this for the current next version (0.7, indicated by the milestone) and additionally backport it as well (to 0.6, indicated by the priority:high label). 🤔

@tyrasd tyrasd requested a review from joker234 January 28, 2021 09:35
@joker234
Copy link
Member

Is the Milestone 0.7 fitting to this issue even tough we want to fix that directly for the 0.6 version?

I wasn't super sure about that either, but adding it to the 0.6 milestone seemed "wrong" because it's already closed; maybe not adding it to any milestone would be better? My thinking was: ok, we fix this for the current next version (0.7, indicated by the milestone) and additionally backport it as well (to 0.6, indicated by the priority:high label). 🤔

I was thinking about not adding it to any milestone would be okay. But keeping it in 0.7 is okay as well.

@tyrasd tyrasd changed the title Fix incomplete touching inners Fix handling of incomplete touching inners Jan 28, 2021
@tyrasd tyrasd merged commit 3a40fba into master Jan 28, 2021
@tyrasd tyrasd deleted the fix-incomplete-touching-inners branch January 28, 2021 11:11
@tyrasd tyrasd removed this from the release 0.7.0 milestone Jan 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working as expected priority:high Should be addressed as soon as possible (next release)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants