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

[FEATURE] Log problematic geometry causing JTS exceptions #700

Open
bdon opened this issue Oct 30, 2023 · 22 comments · Fixed by #703
Open

[FEATURE] Log problematic geometry causing JTS exceptions #700

bdon opened this issue Oct 30, 2023 · 22 comments · Fixed by #703

Comments

@bdon
Copy link
Contributor

bdon commented Oct 30, 2023

Is your feature request related to a problem? Please describe.

A couple dozen times per run I'll see JTS exceptions that may be related to robustness in OverlayNG polygon boolops.

Example:

2:04:53 ERR [archive:encode] - Caught error postprocessing features buildings {x=8588 y=5397 z=14}
org.locationtech.jts.geom.TopologyException: side location conflict: arg 1 [ (242.875, 235.55901699437496, NaN) ]
        at org.locationtech.jts.operation.overlayng.OverlayLabeller.propagateAreaLocations(OverlayLabeller.java:140)
        at org.locationtech.jts.operation.overlayng.OverlayLabeller.labelAreaNodeEdges(OverlayLabeller.java:80)
        at org.locationtech.jts.operation.overlayng.OverlayLabeller.computeLabelling(OverlayLabeller.java:52)
        at org.locationtech.jts.operation.overlayng.OverlayNG.labelGraph(OverlayNG.java:566)
        at org.locationtech.jts.operation.overlayng.OverlayNG.computeEdgeOverlay(OverlayNG.java:503)
        at org.locationtech.jts.operation.overlayng.OverlayNG.getResult(OverlayNG.java:483)
        at org.locationtech.jts.operation.overlayng.OverlayNG.overlay(OverlayNG.java:277)
        at org.locationtech.jts.operation.overlayng.OverlayNGRobust.overlay(OverlayNGRobust.java:137)
        at org.locationtech.jts.operation.union.CascadedPolygonUnion$1.union(CascadedPolygonUnion.java:62)
        at org.locationtech.jts.operation.union.CascadedPolygonUnion.unionActual(CascadedPolygonUnion.java:338)
        at org.locationtech.jts.operation.union.CascadedPolygonUnion.unionSafe(CascadedPolygonUnion.java:321)
        at org.locationtech.jts.operation.union.CascadedPolygonUnion.binaryUnion(CascadedPolygonUnion.java:246)
        at org.locationtech.jts.operation.union.CascadedPolygonUnion.binaryUnion(CascadedPolygonUnion.java:227)
        at org.locationtech.jts.operation.union.CascadedPolygonUnion.unionTree(CascadedPolygonUnion.java:191)
        at org.locationtech.jts.operation.union.CascadedPolygonUnion.union(CascadedPolygonUnion.java:179)
        at org.locationtech.jts.operation.union.CascadedPolygonUnion.union(CascadedPolygonUnion.java:94)
        at org.locationtech.jts.operation.union.UnaryUnionOp.union(UnaryUnionOp.java:219)
        at org.locationtech.jts.operation.union.UnaryUnionOp.union(UnaryUnionOp.java:107)
        at org.locationtech.jts.geom.GeometryOverlay.union(GeometryOverlay.java:165)
        at org.locationtech.jts.geom.Geometry.union(Geometry.java:1439)
        at com.onthegomap.planetiler.FeatureMerge.union(FeatureMerge.java:438)
        at com.onthegomap.planetiler.FeatureMerge.bufferUnionUnbuffer(FeatureMerge.java:431)
        at com.onthegomap.planetiler.FeatureMerge.mergeNearbyPolygons(FeatureMerge.java:325)
        at com.onthegomap.planetiler.FeatureMerge.mergeNearbyPolygons(FeatureMerge.java:360)

It would be ideal to log these so the exception can be reproduced and reported upstream in the JTS issue tracker.

Describe the solution you'd like
Add a configuration flag like --log-jts-exceptions. When an exception happens, it serializes the input geometries to the JTS operation to GeoJSON on disk so that the exception can be reproduced later. TBD how to name these output files.

@bdon bdon changed the title [FEATURE] Log problematic geometry that results in JTS exceptions [FEATURE] Log problematic geometry causing JTS exceptions Oct 30, 2023
@msbarry
Copy link
Contributor

msbarry commented Oct 30, 2023

@bdon what do you think of this approach: #703 ? When you add --log-jts-exceptions it prints out WKT-encoded geometries in the logs along with the error message so we have more context on where the failure occurred. That only adds it for the buffer/union/unbuffer stack trace you shared, are there other entrypoints printing issues?

@bdon
Copy link
Contributor Author

bdon commented Oct 30, 2023

maybe it's safer to use WKB instead of WKT? Wouldn't writing WKT transform floating-point representation into fixed-precision text; if it's parsed back into floating-point later it might be a different number, which might make robustness problems disappear? Will inspect the logs for different classes of errors now.

@dr-jts
Copy link

dr-jts commented Oct 30, 2023

maybe it's safer to use WKB instead of WKT?

Generally it is the safest to use WKB. However, I've seen lots (well, some) of OverlayNG issues which can be reproduced using WKT. So either way is likely to provide good feedback.

cheers @bdon !

@bdon
Copy link
Contributor Author

bdon commented Oct 30, 2023

Another class of exception from the same run:

ERR [archive:encode] - Caught error postprocessing features natural {x=562 y=727 z=11}
org.locationtech.jts.geom.TopologyException: found non-noded intersection between LINESTRING ( -0.000000000000000000000000000000007607423280329582 165.08210678118655, -0.000000000000000000000000000000003803711640164791 165.08210678118655 ) and LINESTRING ( 0 165.08210678118655, -0.000000000000000000000000000000007607423280329582 165.08210678118655 ) [ (-7.607423280329582E-33, 165.08210678118655, NaN) ]
        at org.locationtech.jts.noding.FastNodingValidator.checkValid(FastNodingValidator.java:139)
        at org.locationtech.jts.noding.ValidatingNoder.validate(ValidatingNoder.java:61)
        at org.locationtech.jts.noding.ValidatingNoder.computeNodes(ValidatingNoder.java:56)
        at org.locationtech.jts.operation.overlayng.EdgeNodingBuilder.node(EdgeNodingBuilder.java:186)
        at org.locationtech.jts.operation.overlayng.EdgeNodingBuilder.build(EdgeNodingBuilder.java:165)
        at org.locationtech.jts.operation.overlayng.OverlayNG.nodeEdges(OverlayNG.java:541)
        at org.locationtech.jts.operation.overlayng.OverlayNG.computeEdgeOverlay(OverlayNG.java:495)
        at org.locationtech.jts.operation.overlayng.OverlayNG.getResult(OverlayNG.java:483)
        at org.locationtech.jts.operation.overlayng.OverlayNG.overlay(OverlayNG.java:277)
        at org.locationtech.jts.operation.overlayng.OverlayNGRobust.overlay(OverlayNGRobust.java:137)
        at org.locationtech.jts.operation.union.CascadedPolygonUnion$1.union(CascadedPolygonUnion.java:62)
        at org.locationtech.jts.operation.union.CascadedPolygonUnion.unionActual(CascadedPolygonUnion.java:338)
        at org.locationtech.jts.operation.union.CascadedPolygonUnion.unionSafe(CascadedPolygonUnion.java:321)
        at org.locationtech.jts.operation.union.CascadedPolygonUnion.binaryUnion(CascadedPolygonUnion.java:246)
        at org.locationtech.jts.operation.union.CascadedPolygonUnion.binaryUnion(CascadedPolygonUnion.java:227)
        at org.locationtech.jts.operation.union.CascadedPolygonUnion.unionTree(CascadedPolygonUnion.java:191)
        at org.locationtech.jts.operation.union.CascadedPolygonUnion.reduceToGeometries(CascadedPolygonUnion.java:286)
        at org.locationtech.jts.operation.union.CascadedPolygonUnion.unionTree(CascadedPolygonUnion.java:189)
        at org.locationtech.jts.operation.union.CascadedPolygonUnion.reduceToGeometries(CascadedPolygonUnion.java:286)
        at org.locationtech.jts.operation.union.CascadedPolygonUnion.unionTree(CascadedPolygonUnion.java:189)
        at org.locationtech.jts.operation.union.CascadedPolygonUnion.union(CascadedPolygonUnion.java:179)
        at org.locationtech.jts.operation.union.CascadedPolygonUnion.union(CascadedPolygonUnion.java:94)
        at org.locationtech.jts.operation.union.UnaryUnionOp.union(UnaryUnionOp.java:219)
        at org.locationtech.jts.operation.union.UnaryUnionOp.union(UnaryUnionOp.java:107)
        at org.locationtech.jts.geom.GeometryOverlay.union(GeometryOverlay.java:165)
        at org.locationtech.jts.geom.Geometry.union(Geometry.java:1439)
        at com.onthegomap.planetiler.FeatureMerge.union(FeatureMerge.java:438)
        at com.onthegomap.planetiler.FeatureMerge.bufferUnionUnbuffer(FeatureMerge.java:431)
        at com.onthegomap.planetiler.FeatureMerge.mergeNearbyPolygons(FeatureMerge.java:325)
        at com.onthegomap.planetiler.FeatureMerge.mergeNearbyPolygons(FeatureMerge.java:360)
        at com.protomaps.basemap.layers.Natural.postProcess(Natural.java:59)

It is only these two classes of exception that happen, numbering about 10-20 per run for the 100GB output tileset.

@bdon
Copy link
Contributor Author

bdon commented Oct 30, 2023

We want a separate out-of-band check on the output tileset to check for self-intersection JTS silently creates - my preference is to use https://github.com/mapbox/wagyu

@dr-jts
Copy link

dr-jts commented Oct 30, 2023

In the non-noded intersection error, the two linestrings in the error message are VERY short:

LINESTRING ( -0.000000000000000000000000000000007607423280329582 165.08210678118655, -0.000000000000000000000000000000003803711640164791 165.08210678118655 ) 
LINESTRING ( 0 165.08210678118655, -0.000000000000000000000000000000007607423280329582 165.08210678118655 )

It seems like some heuristic code to eliminate very short line segments might avoid these errors. If these are in the input (which seems unlikely) this could be done by some preprocessing. If they are being created during the union process, I'll need some reproducing data to try and identify a fix.

@dr-jts
Copy link

dr-jts commented Oct 30, 2023

I wonder if there's a better (ie. faster, more robust) solution than FeatureMerge.bufferUnionUnbuffer ? Buffering/unbuffering is quite likely to introduce line segment artifacts which may cause issues down the road. Plus, it's slow.

Perhaps ConcaveHullOfPolygons might be an option?

@msbarry
Copy link
Contributor

msbarry commented Oct 31, 2023

Thanks or the feedback @dr-jts! ConcaveHullOfPolygons sounds promising, I tried it out in place of bufferUnionUnbufer and ran into a couple of issues:

image

Is there a different sequence of operations you think might help here instead?

@msbarry
Copy link
Contributor

msbarry commented Oct 31, 2023

@bdon it looks like the same spot in planetiler code will catch that exception as well. I'll print out WKT and base64-encoded WKB just to be safe.

@msbarry msbarry reopened this Oct 31, 2023
@bdon
Copy link
Contributor Author

bdon commented Oct 31, 2023

Attached are text files with the full WKT and WKB inputs along with stack traces for the 6 exceptions from last night's build:

exception_6.txt
exception_5.txt
exception_4.txt
exception_3.txt
exception_2.txt
exception_1.txt

@dr-jts
Copy link

dr-jts commented Nov 1, 2023

It looks like all these errors are due to the "buffer" result being invalid, due to self-intersecting rings. I'm not sure why this is - probably a bug in the buffer algorithm. Here's an example from one of the error cases:
image
image

Can you provide the distance used to buffer? That will help in looking into this.

A workaround is to check the buffered value for validity, and then run GeometryFixer to fix invalid polygons. To reduce overhead, this could be done only when an exception is encountered.

Another possible fix: buffering essentially acts as a union in itself. So possibly in bufferUnionUnbuffer the input polygons can simply all be buffered together, and then the intermediate union can be skipped? However, there might be a performance implication here for very large input sets, since 'buffer can sometimes take a long time on large complex inputs.

@msbarry
Copy link
Contributor

msbarry commented Nov 1, 2023

Thanks @dr-jts! Looks like the buffer distance is 0.5.

since 'buffer can sometimes take a long time on large complex inputs

Yes that's what I initially saw while implementing this, they performed about the same until it got to merging a lot of tiny densely packed building polygons and would hang for a long time.

@dr-jts
Copy link

dr-jts commented Nov 1, 2023

I did some more testing on this case (example_6):

GEOMETRYCOLLECTION (POLYGON ((240.6875 234.25, 241.5625 233.8125, 242.25 233.5, 243 235.0625, 242.875 235, 242.375 235.25, 242.375 235.375, 241.4375 235.875, 240.6875 234.25)), POLYGON ((242.1875 232.9375, 241.9375 232.9375, 243.0625 232.5625, 243.5 233.5, 242.6875 233.875, 242.1875 232.9375)))

JTS 1.19 produces the invalid buffer (with mitre join) for the first element as you are seeing, which causes the error in union:
image

However, JTS 1.20 (unreleased - using the current dev code) produces a correct buffer:
image

This unions successfully:
image

example_1 shows the same pattern.

There have been several changes in the codebase that might have improved the buffer output, but I'm not exactly sure what has solved this particular issue.

So - you could try building from the source of master, and see if that fixes the problems.

@dr-jts
Copy link

dr-jts commented Nov 1, 2023

As for the low utlity of ConcaveHullOfPolygons on your data, that's too bad, since it's the theoretically correct solution. Clearly, more research required! For now it seems like your buffer approach is best. I'm always impressed at how many problems buffer can solve!

@msbarry
Copy link
Contributor

msbarry commented Nov 2, 2023

Thanks @dr-jts that's great news! @bdon should be able to force his build to use the latest 1.20.0-SNAPSHOT build to test out if this is fixed.

It would be great if we could use ConcaveHullOfPolygons eventually, I played with it a bit more and it seems faster on average than buffer/union/unbuffer, it just chokes on the jakarta buildings that are all much closer than the distance threshold - maybe some optimization would eventually help that case? A couple other issues I ran into:

  • it throws a "Unable to find shell join index with interior join line" error processing polygons like
    buildings.wkb.txt with concaveHullByLength(geom, 0.5, true, true) - is there some additional preprocessing you need to do besides buffer(0) to join the overlapping polygons?
  • there are a few cases where the output is not valid, for example concaveHullByLength(geom, 0.5, true, true) on buildings2.wbk.txt - is that expected? Or a bug in ConcaveHullOfPolygons ?

@bdon
Copy link
Contributor Author

bdon commented Nov 2, 2023

Attached are all 3 exceptions that arose with JTS 1.20.0-SNAPSHOT.

Note: this runs on slightly different data than before (since it's OSM)

There is a new class of exception that did not appear in the 6 before:

org.locationtech.jts.geom.TopologyException: unable to assign free hole to a shell [ (177.09719405932674, 102.53619843452306, NaN) ]

jts_1_20_exception_3.txt
jts_1_20_exception_2.txt
jts_1_20_exception_1.txt

@dr-jts
Copy link

dr-jts commented Nov 2, 2023

It looks like these errors are all due to the initial buffer producing invalid output. Specifically, there's a bug that causes the inward buffer of holes to "invert" and be output as a separate (overlapping) polygon.

The workaround is to check buffer output for validity, and if invalid run GeometryFixer on it. Hopefully this doesn't introduce too much of a performance penalty.

I'll log this as a JTS issue. It's probably gnarly to fix though, so I doubt I'll have a fix soon.

@msbarry
Copy link
Contributor

msbarry commented Nov 2, 2023

Would it be sufficient to only do the fix if an exception gets thrown?

@dr-jts
Copy link

dr-jts commented Nov 7, 2023

Would it be sufficient to only do the fix if an exception gets thrown?

Yes, that's a good idea.

@msbarry
Copy link
Contributor

msbarry commented Nov 7, 2023

OK I added these test cases and the GeometryFixer fix in #713 - it seems to resolve all of the issues. One other thing that I notice is that the output of the unbuffer operation is often invalid (~12k times per planet render) and gets fixed by this code in snapAndFixPolygon - that seems to resolve the issue but is that a kind of JTS issue that you want examples of to debug? Currently it just increments a counter and dumps the count at the end.

Thanks for the help @dr-jts !

@dr-jts
Copy link

dr-jts commented Nov 7, 2023

OK I added these test cases and the GeometryFixer fix in #713 - it seems to resolve all of the issues.

Excellent. Good that GeometryFixer can clean things up.

One other thing that I notice is that the output of the unbuffer operation is often invalid (~12k times per planet render) and gets fixed by this code in snapAndFixPolygon - that seems to resolve the issue but is that a kind of JTS issue that you want examples of to debug? Currently it just increments a counter and dumps the count at the end.

Yes, a few examples of unbuffer invalid output would be good. I will then open a JTS issue for this - to be worked on at some future date. It's gnarly...

@msbarry
Copy link
Contributor

msbarry commented Nov 8, 2023

Here are some geometries that result in invalid output when you do geometry.buffer(-0.1): unbuffer-0.1.zip

And here are some geometries that result in invalid output when you do geometry.unbuffer(-0.5): unbuffer-0.5.zip

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 a pull request may close this issue.

3 participants