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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Weird GeometryFixer behavior #959

Open
sheinbergon opened this issue Feb 25, 2023 · 3 comments
Open

Weird GeometryFixer behavior #959

sheinbergon opened this issue Feb 25, 2023 · 3 comments

Comments

@sheinbergon
Copy link

sheinbergon commented Feb 25, 2023

So, first of all, let me begin by acknowledging all of your hard work and effort in maintaining this library. Amazing stuff 馃憦

To the point, it seems GeometryFixer algorithm can sometimes return... unexpected results.

Here are two examples:

Input PostGIS ST_MakeValid Result Geometry Fixer Result (keepCollapsed=false) Geometry Fixer Result (keepCollapsed=true)
POLYGON((0 0, 1 1, 1 2, 1 1, 0 0)) MULTILINESTRING((0 0,1 1),(1 1,1 2)) POLYGON EMPTY LINESTRING (0 0, 1 1, 1 2, 1 1, 0 0)
POLYGON((0 0, 10 0, 10 10, 0 10, 0 0), (2 2, 2 8, 8 8, 8 2, 2 2), (3 3, 3 7, 7 7, 7 3, 3 3)) MULTIPOLYGON(((10 10,10 0,0 0,0 10,10 10),(8 8,2 8,2 2,8 2,8 8)),((7 7,7 3,3 3,3 7,7 7))) POLYGON ((0 10, 10 10, 10 0, 0 0, 0 10), (2 2, 8 2, 8 8, 2 8, 2 2) POLYGON ((0 10, 10 10, 10 0, 0 0, 0 10), (2 2, 8 2, 8 8, 2 8, 2 2)

In the first row, it decides to return an empty polygon instead of a line/multiline.
In the second case, it completely discards one of the interior rings.

I'm willing to dive in and provide PRs, assuming you'll confirm these are in fact, bugs

@dr-jts
Copy link
Contributor

dr-jts commented Feb 25, 2023

Thanks for the kudos, and for the nicely detailed issue report.

The semantics of GeometryFixer are slightly different to the PostGIS ST_MakeValid. Both of these cases are working as designed.

GeometryFixer tries to preserve the topology of the input, but ensuring the output is valid. So in the first case of a completely collapsed polygon, POLYGON EMPTY is returned. The keepCollapsed option allows recovering the collapsed linework if required. A LINESTRING seems more appropriate than the MULTILINESTRING returned by ST_MakeValid.

In the second case, the assumption is that holes are voids in the containing polygon. Two nested holes still form a single void, as constructed. The ST_MakeValid result is an alternate interpretation of the linework, but perhaps more suited as the result of a polygonize operation.

@sheinbergon
Copy link
Author

sheinbergon commented Feb 25, 2023

So, for the first case, you confirmed my assumption. Agreed.

For the second case, while I understand your claim, I do feel PostGIS interpretation makes more sense, as it maintains more of the original structure, while returning a valid outcome. I think that at least having the ability to opt in such a "keep nested holes" feature would be good to have.

WDYT?

@dr-jts
Copy link
Contributor

dr-jts commented Feb 27, 2023

I think that at least having the ability to opt in such a "keep nested holes" feature would be good to have.

Maybe. Have to look and see how to implement that behaviour.

Another difference with the PostGIS semantic is the behaviour for overlapping holes. For consistency that might need to be addressed as well.

image
GeometryFixer treats all holes as actual holes

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

No branches or pull requests

2 participants