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

Implement alternative handling of polygons with holes #700

Merged
merged 1 commit into from
Nov 21, 2023

Conversation

mzur
Copy link
Member

@mzur mzur commented Nov 17, 2023

I'd like to propose an updated solution to handling non-simple polygons that does not require the "polish" function. Here is my reasoning behind this:

The polishing function was used to remove duplicate coordinates that were sometimes added. I found that these were probably "holes", because every time a polygon had such a duplicate coordinate, there was an entry in _holes when the polygon was logged with polygonizer.getGeometry() in jstsValidate() (now jstsSimplify()). The screenshot shows the geometry with a hole at the top and the OL coordinates at the bottom:

image

So in order to remove the holes, now getExteriorRing() is called for each polygon. To reproduce a polygon with a hole, draw a geometry like this:

Screencast.from.17.11.2023.11.09.13.webm

For a similar reason I removed the "getInteriorRing" stuff from jstsAddPolygon(). However, holes were still present in the polygons and had to be removed as described above. Everything still worked fine without the code, though, so I left it like that.

Finally, getGreatestPolygon() also works if there is only one polygon so I removed the if.

@lehecht Could you please have a good look at this and check if this still handles all the cases that you tested? You spent much more time with this than I did so you probably know all the edge cases. It's entirely possible that my implementation does not handle some cases that your version handles correctly.

References #634

@mzur mzur requested a review from lehecht November 17, 2023 10:17
Copy link
Contributor

@lehecht lehecht left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works just perfectly. Thanks! I could not find any edge case where your solution does not work, so we can use that.

@mzur
Copy link
Member Author

mzur commented Nov 21, 2023

Thanks for the review!

@mzur mzur merged commit fabfa21 into non-simple-polygon-bug Nov 21, 2023
4 of 5 checks passed
@mzur mzur deleted the non-simple-polygon-bug-patch-1 branch November 21, 2023 08:18
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.

2 participants