Failing GeoTools unit test due to JTS 1.18.0-SNAPSHOT - GeometryPrecisionReducer

[jnh5y]

When trying to use JTS 1.18.0-SNAPSHOT with GeoTools, I encountered "IllegalArgumentException("Reduction failed, possible invalid input")"s from the ProjectionHandlerTest (https://github.com/geotools/geotools/blob/master/modules/library/main/src/test/java/org/geotools/renderer/crs/ProjectionHandlerTest.java).

The ProjectionHandler is reducing a geometry here: https://github.com/geotools/geotools/blob/master/modules/library/main/src/main/java/org/geotools/renderer/crs/ProjectionHandler.java#L799.

The PrecisionModel is "Fixed" with scale of 1.0E8. The geometry is

POLYGON ((-95.52890749421356 -4.069715451545125, -95.27515365199997 -3.2845930729999395, -95.25786707899994 -3.248801323999942, -95.29606749299995 -3.3384279029999675, -95.07870692499995 -2.904322486999945, -94.72751957499997 -2.3949212689999513, -93.81029733999998 -1.365497904999927, -92.67859800799994 -0.4329676369999333, -92.25250601399995 0.0000000000000568, -101.16745957499995 -0.0000002099999392, -101.18492052799996 -2.4971886659999427, -99.57820110399996 -2.478882963999979, -98.06016130299997 -2.6162888649999445, -97.51964490799998 -2.774348894999946, -97.14769993999994 -3.029418001999943, -96.76723317899996 -3.438246832999937, -96.64209401999994 -3.6365530179999723, -96.51550988699995 -3.955361244999949, -96.36923798099997 -4.767552936999948, -96.37915015299996 -5.713956061999966, -96.16663660199998 -6.543433900999958, -96.05244410099999 -6.76917928599994, -95.91559129799998 -6.94036688999995, -95.66501679599997 -7.135677733999955, -95.53245184999997 -7.186370558999954, -95.43489939399996 -7.105049019999967, -95.35258101199997 -6.864051166999957, -95.37351480299998 -6.728515880999964, -95.63786922599996 -6.004913795999926, -95.69662724099999 -5.648719570999958, -95.66243977999994 -4.892913767999971, -95.52890749421356 -4.069715451545125))

[jnh5y]

Did the semantics around precision get flipped? I do see the GeoTools code do this:

                        new GeometryPrecisionReducer(new PrecisionModel(1 / precision));
                Geometry reduced = reducer.reduce(geometry);```

[jnh5y]

When I changed that bit to be ...new PrecisionModel(precision)... instead, test conditions which encoded previous failures in topological operations started to fail.

I take that both as a "win" and a sign that some of the OverlayNG changes/benefits are outside of the control of the jts.overlay flag. (Which may be ok; we should make sure to be clear about it.)

[dr-jts]

The 1 / precision code is fine - there has been no change to the semantics of precision scale factor.

What has changed is that GeometryPrecisionReducer now does not use buffer(0), because it caused problems. However, this makes it less tolerance of invalid input. In this case the input is clearly invalid, due to a self-intersection:

I guess it depends what the GeoTools test is trying to do. Is it trying to do a MakeValid operation on input which has become invalid due to reprojection? If so, then GeometryPrecisionReducer is not the right way to do that. JTS may soon get a MakeValid capability, but in the meantime buffer(0) is about the best there is.

[dr-jts]

Maybe the original input geometry is invalid (which would not be unlikely for a renderer). it looks like this is a clipping routine? Unfortunately invalid geometry can't be handled by OverlayNG (or the original overlay). If this is the case, GeoTools will need to provide it's own strategy for making the geometry valid enough to be clipped (perhaps via a test for validity first, with a MakeValid based on buffer(0), or on another more robust approach I can provide.)

[dr-jts]

When I changed that bit to be ...new PrecisionModel(precision)... instead, test conditions which encoded previous failures in topological operations started to fail.

That's a pretty big change - it should be obvious which one is right. Sounds like the original.

I take that both as a "win" and a sign that some of the OverlayNG changes/benefits are outside of the control of the jts.overlay flag. (Which may be ok; we should make sure to be clear about it.)

What do you suggest? I could note this in an API Changes section in the version history. In fact, I will!

[jnh5y]

The two obvious options are 1) document it or 2) keep the old behavior (even if undesirable) and control it with the flag.

It seems like unfortunate timing to leave renderers in the situation where they have to (re)implement MakeValid functionality to adopt this upgrade. That said, this is definitely a bit of a corner case.

[dr-jts]

And nothing says that GeoTools can transition to a new release with zero changes, right? It's going to change sometime...

[jnh5y]

Oh yeah, I'm fine with that idea in general, and it's the right thing for JTS to do.

That said, I'm nervous about changing kludge-y code which has been "working" for years without being 100% on exactly what it does...:)

[dr-jts]

I guess you could vendor in the old implementation? It's fairly simple.

[jnh5y]

This issue was fixed on the GeoTools side by using the 'buffer(0)' approach to clean up the geometry.