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

SnapRoundingNoder fix #504

Merged
merged 3 commits into from
Nov 19, 2021
Merged

SnapRoundingNoder fix #504

merged 3 commits into from
Nov 19, 2021

Conversation

mngr777
Copy link
Contributor

@mngr777 mngr777 commented Nov 17, 2021

Fix for "GEOSGeom_setPrecision returns error for valid geometry" https://trac.osgeo.org/geos/ticket/1127

nearnessTol calculation moved from SnapRoundingIntersectionAdder to SnapRoundingNoder; same tolerance value is passed to MCIndexNoder. This allows to detect all cases of vertices positioned close enough to segments to be added as intersections by SnapRoundingIntersectionAdder::processNearVertex. Currently closely positioned segments can be filtered out in MonotoneChain::computeOverlaps() by MonotoneChain::overlaps() called with zero overlapTolerance.

@Komzpa
Copy link
Contributor

Komzpa commented Nov 17, 2021

@mngr777 can you please add the minimized test case to the suite so it won't break in the future?
Thanks.

@pramsey
Copy link
Member

pramsey commented Nov 17, 2021

@dr-jts is going to check this fix on JTS too, just to make sure. Seems pretty magical.

@dr-jts
Copy link
Contributor

dr-jts commented Nov 19, 2021

NIce work on finding this bug. I fixed it in SnappingNoder, but of course SnapRoundingNoder needs the same thing. Glad the fix is so simple (and not some fundamental design flaw in the Snap-Rounding approach!).

@dr-jts dr-jts merged commit 1a09e78 into libgeos:main Nov 19, 2021
@mngr777 mngr777 deleted the snap-rounding-noder-fix branch November 19, 2021 19:19
@dr-jts dr-jts added the Bug label Nov 19, 2021
@dr-jts
Copy link
Contributor

dr-jts commented Nov 19, 2021

Unfortunately it looks to me like this fix (although correct) does not solve 2 of the 3 test cases for in the original GEOS issue. @mngr777 can you confirm if you are still seeing this issue in GeometryPrecisionReducer? If so, we should reopen an issue for this, and capture the failing test cases.

(Note: the fix I suggested of reducing the precision of the scale factor still works, although it is awkward to do using the current C API).

@mngr777
Copy link
Contributor Author

mngr777 commented Nov 19, 2021

@dr-jts Yes, 2 cases still produce exceptions. Please reopen the issue.

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

Successfully merging this pull request may close these issues.

4 participants