From a962780d279f5444e9adca921196d60519fbb37f Mon Sep 17 00:00:00 2001 From: Peter Stace Date: Fri, 15 Sep 2023 09:51:34 +1000 Subject: [PATCH] Don't validate results from GEOS Recent versions of GEOS use OverlayNG (NG stands for New Generation) for set operations between geometries (Intersection, Union, etc.). With the introduction of OverlayNG, invalid geometries are no longer frequently produced by GEOS. They are either non-existent, or very rare. Before this change, the `simplefeatures` GEOS wrapper validates results from GEOS. This feels like a misalignment: - Validation isn't free, so validating GEOS results seems like the wrong default. - The GEOS wrapper is doing more than strictly wrapping the GEOS library (it is also validating the result). This blurs the lines between what simplefeatures is doing and what GEOS is doing. This change removes that validation. Users can of course still validate manually by calling the `Validate` method on the result. --- geos/benchmark_test.go | 19 +---------- geos/doc.go | 4 +++ geos/entrypoints.go | 61 +++++++++++++++++++++--------------- geos/entrypoints_test.go | 2 +- geos/generic_operations.go | 6 ++-- geos/handle.go | 10 +++--- internal/perf/set_op_test.go | 2 +- 7 files changed, 49 insertions(+), 55 deletions(-) diff --git a/geos/benchmark_test.go b/geos/benchmark_test.go index 756cda28..cc0670a2 100644 --- a/geos/benchmark_test.go +++ b/geos/benchmark_test.go @@ -33,23 +33,6 @@ func regularPolygon(center geom.XY, radius float64, sides int) geom.Polygon { return poly } -func BenchmarkIntersectionWithoutValidation(b *testing.B) { - for _, sz := range []int{10, 100, 1000, 10000} { - b.Run(fmt.Sprintf("n=%d", sz), func(b *testing.B) { - inputA := regularPolygon(geom.XY{X: 0, Y: 0}, 1.0, sz).AsGeometry() - inputB := regularPolygon(geom.XY{X: 1, Y: 0}, 1.0, sz).AsGeometry() - b.ResetTimer() - - for i := 0; i < b.N; i++ { - _, err := Intersection(inputA, inputB, geom.DisableAllValidations) - if err != nil { - b.Fatal(err) - } - } - }) - } -} - func BenchmarkNoOp(b *testing.B) { for _, sz := range []int{10, 100, 1000, 10000} { b.Run(fmt.Sprintf("n=%d", sz), func(b *testing.B) { @@ -57,7 +40,7 @@ func BenchmarkNoOp(b *testing.B) { b.ResetTimer() for i := 0; i < b.N; i++ { - _, err := noop(input, geom.DisableAllValidations) + _, err := noop(input) if err != nil { b.Fatal(err) } diff --git a/geos/doc.go b/geos/doc.go index 1b5b5b89..7c34cf25 100644 --- a/geos/doc.go +++ b/geos/doc.go @@ -4,6 +4,10 @@ // Its purpose is to provide functionality that has been implemented in GEOS, // but is not yet available natively in the simplefeatures library. // +// Results from functions in this package are returned from GEOS unvalidated +// and as-is. Users may call the Validate method on results if they wish to +// check result validity. +// // The operations in this package ignore Z and M values if they are present. // // To use this package, you will need to install the GEOS library. diff --git a/geos/entrypoints.go b/geos/entrypoints.go index f56d1e0e..dd1e599e 100644 --- a/geos/entrypoints.go +++ b/geos/entrypoints.go @@ -179,8 +179,10 @@ func Overlaps(a, b geom.Geometry) (bool, error) { // Union returns a geometry that is the union of the input geometries. // Formally, the returned geometry will contain a particular point X if and // only if X is present in either geometry (or both). -func Union(a, b geom.Geometry, opts ...geom.ConstructorOption) (geom.Geometry, error) { - g, err := binaryOpG(a, b, opts, func(ctx C.GEOSContextHandle_t, a, b *C.GEOSGeometry) *C.GEOSGeometry { +// +// The validity of the result is not checked. +func Union(a, b geom.Geometry) (geom.Geometry, error) { + g, err := binaryOpG(a, b, func(ctx C.GEOSContextHandle_t, a, b *C.GEOSGeometry) *C.GEOSGeometry { return C.GEOSUnion_r(ctx, a, b) }) return g, wrap(err, "executing GEOSUnion_r") @@ -189,8 +191,10 @@ func Union(a, b geom.Geometry, opts ...geom.ConstructorOption) (geom.Geometry, e // Intersection returns a geometry that is the intersection of the input // geometries. Formally, the returned geometry will contain a particular point // X if and only if X is present in both geometries. -func Intersection(a, b geom.Geometry, opts ...geom.ConstructorOption) (geom.Geometry, error) { - g, err := binaryOpG(a, b, opts, func(ctx C.GEOSContextHandle_t, a, b *C.GEOSGeometry) *C.GEOSGeometry { +// +// The validity of the result is not checked. +func Intersection(a, b geom.Geometry) (geom.Geometry, error) { + g, err := binaryOpG(a, b, func(ctx C.GEOSContextHandle_t, a, b *C.GEOSGeometry) *C.GEOSGeometry { return C.GEOSIntersection_r(ctx, a, b) }) return g, wrap(err, "executing GEOSIntersection_r") @@ -204,7 +208,6 @@ type bufferOptionSet struct { endCapStyle int joinStyle int mitreLimit float64 - ctorOpts []geom.ConstructorOption } func newBufferOptionSet(opts []BufferOption) bufferOptionSet { @@ -278,19 +281,13 @@ func BufferJoinStyleBevel() BufferOption { } } -// BufferConstructorOption sets constructor option that are used when -// reconstructing the buffered geometry that is returned from the GEOS lib. -func BufferConstructorOption(opts ...geom.ConstructorOption) BufferOption { - return func(bos *bufferOptionSet) { - bos.ctorOpts = append(bos.ctorOpts, opts...) - } -} - // Buffer returns a geometry that contains all points within the given radius // of the input geometry. +// +// The validity of the result is not checked. func Buffer(g geom.Geometry, radius float64, opts ...BufferOption) (geom.Geometry, error) { optSet := newBufferOptionSet(opts) - result, err := unaryOpG(g, optSet.ctorOpts, func(ctx C.GEOSContextHandle_t, gh *C.GEOSGeometry) *C.GEOSGeometry { + result, err := unaryOpG(g, func(ctx C.GEOSContextHandle_t, gh *C.GEOSGeometry) *C.GEOSGeometry { return C.GEOSBufferWithStyle_r( ctx, gh, C.double(radius), C.int(optSet.quadSegments), @@ -306,8 +303,10 @@ func Buffer(g geom.Geometry, radius float64, opts ...BufferOption) (geom.Geometr // Douglas-Peucker algorithm. Topological invariants may not be maintained, // e.g. polygons can collapse into linestrings, and holes in polygons may be // lost. -func Simplify(g geom.Geometry, tolerance float64, opts ...geom.ConstructorOption) (geom.Geometry, error) { - result, err := unaryOpG(g, opts, func(ctx C.GEOSContextHandle_t, gh *C.GEOSGeometry) *C.GEOSGeometry { +// +// The validity of the result is not checked. +func Simplify(g geom.Geometry, tolerance float64) (geom.Geometry, error) { + result, err := unaryOpG(g, func(ctx C.GEOSContextHandle_t, gh *C.GEOSGeometry) *C.GEOSGeometry { return C.GEOSSimplify_r(ctx, gh, C.double(tolerance)) }) return result, wrap(err, "executing GEOSSimplify_r") @@ -315,8 +314,10 @@ func Simplify(g geom.Geometry, tolerance float64, opts ...geom.ConstructorOption // Difference returns the geometry that represents the parts of input geometry // A that are not part of input geometry B. -func Difference(a, b geom.Geometry, opts ...geom.ConstructorOption) (geom.Geometry, error) { - result, err := binaryOpG(a, b, opts, func(ctx C.GEOSContextHandle_t, a, b *C.GEOSGeometry) *C.GEOSGeometry { +// +// The validity of the result is not checked. +func Difference(a, b geom.Geometry) (geom.Geometry, error) { + result, err := binaryOpG(a, b, func(ctx C.GEOSContextHandle_t, a, b *C.GEOSGeometry) *C.GEOSGeometry { return C.GEOSDifference_r(ctx, a, b) }) return result, wrap(err, "executing GEOSDifference_r") @@ -324,8 +325,10 @@ func Difference(a, b geom.Geometry, opts ...geom.ConstructorOption) (geom.Geomet // SymmetricDifference returns the geometry that represents the parts of the // input geometries that are not part of the other input geometry. -func SymmetricDifference(a, b geom.Geometry, opts ...geom.ConstructorOption) (geom.Geometry, error) { - result, err := binaryOpG(a, b, opts, func(ctx C.GEOSContextHandle_t, a, b *C.GEOSGeometry) *C.GEOSGeometry { +// +// The validity of the result is not checked. +func SymmetricDifference(a, b geom.Geometry) (geom.Geometry, error) { + result, err := binaryOpG(a, b, func(ctx C.GEOSContextHandle_t, a, b *C.GEOSGeometry) *C.GEOSGeometry { return C.GEOSSymDifference_r(ctx, a, b) }) return result, wrap(err, "executing GEOSSymDifference_r") @@ -336,13 +339,15 @@ func SymmetricDifference(a, b geom.Geometry, opts ...geom.ConstructorOption) (ge // geometry that is valid and similar (but not the same as) the original // invalid geometry. If the input geometry is valid, then it is returned // unaltered. -func MakeValid(g geom.Geometry, opts ...geom.ConstructorOption) (geom.Geometry, error) { +// +// The validity of the result is not checked. +func MakeValid(g geom.Geometry) (geom.Geometry, error) { if C.MAKE_VALID_MISSING != 0 { return geom.Geometry{}, unsupportedGEOSVersionError{ C.MAKE_VALID_MIN_VERSION, "MakeValid", } } - result, err := unaryOpG(g, opts, func(ctx C.GEOSContextHandle_t, g *C.GEOSGeometry) *C.GEOSGeometry { + result, err := unaryOpG(g, func(ctx C.GEOSContextHandle_t, g *C.GEOSGeometry) *C.GEOSGeometry { return C.GEOSMakeValid_r(ctx, g) }) return result, wrap(err, "executing GEOSMakeValid_r") @@ -362,13 +367,15 @@ func MakeValid(g geom.Geometry, opts ...geom.ConstructorOption) (geom.Geometry, // and return an error, but this is not guaranteed, and an invalid result may // be returned without an error. It is the responsibility of the caller to // ensure that the constraints are met before using this function. -func CoverageUnion(g geom.Geometry, opts ...geom.ConstructorOption) (geom.Geometry, error) { +// +// The validity of the result is not checked. +func CoverageUnion(g geom.Geometry) (geom.Geometry, error) { if C.COVERAGE_UNION_MISSING != 0 { return geom.Geometry{}, unsupportedGEOSVersionError{ C.COVERAGE_UNION_MIN_VERSION, "CoverageUnion", } } - result, err := unaryOpG(g, opts, func(ctx C.GEOSContextHandle_t, g *C.GEOSGeometry) *C.GEOSGeometry { + result, err := unaryOpG(g, func(ctx C.GEOSContextHandle_t, g *C.GEOSGeometry) *C.GEOSGeometry { return C.GEOSCoverageUnion_r(ctx, g) }) return result, wrap(err, "executing GEOSCoverageUnion_r") @@ -377,8 +384,10 @@ func CoverageUnion(g geom.Geometry, opts ...geom.ConstructorOption) (geom.Geomet // UnaryUnion is a single argument version of Union. It is most useful when // supplied with a GeometryCollection, resulting in the union of the // GeometryCollection's child geometries. -func UnaryUnion(g geom.Geometry, opts ...geom.ConstructorOption) (geom.Geometry, error) { - result, err := unaryOpG(g, opts, func(ctx C.GEOSContextHandle_t, g *C.GEOSGeometry) *C.GEOSGeometry { +// +// The validity of the result is not checked. +func UnaryUnion(g geom.Geometry) (geom.Geometry, error) { + result, err := unaryOpG(g, func(ctx C.GEOSContextHandle_t, g *C.GEOSGeometry) *C.GEOSGeometry { return C.GEOSUnaryUnion_r(ctx, g) }) return result, wrap(err, "executing GEOSUnaryUnion_r") diff --git a/geos/entrypoints_test.go b/geos/entrypoints_test.go index 6afdf28c..c4d605cc 100644 --- a/geos/entrypoints_test.go +++ b/geos/entrypoints_test.go @@ -579,7 +579,7 @@ type BinaryOperationTestCase struct { Out string } -func RunBinaryOperationTest(t *testing.T, fn func(a, b geom.Geometry, opts ...geom.ConstructorOption) (geom.Geometry, error), cases []BinaryOperationTestCase) { +func RunBinaryOperationTest(t *testing.T, fn func(a, b geom.Geometry) (geom.Geometry, error), cases []BinaryOperationTestCase) { for i, c := range cases { t.Run(strconv.Itoa(i), func(t *testing.T) { run := func(rev bool) func(t *testing.T) { diff --git a/geos/generic_operations.go b/geos/generic_operations.go index df2c4c5e..6539556d 100644 --- a/geos/generic_operations.go +++ b/geos/generic_operations.go @@ -43,7 +43,6 @@ func binaryOpE( func binaryOpG( g1, g2 geom.Geometry, - opts []geom.ConstructorOption, op func(C.GEOSContextHandle_t, *C.GEOSGeometry, *C.GEOSGeometry) *C.GEOSGeometry, ) (geom.Geometry, error) { var result geom.Geometry @@ -54,7 +53,7 @@ func binaryOpG( } defer C.GEOSGeom_destroy(resultGH) var err error - result, err = h.decode(resultGH, opts) + result, err = h.decode(resultGH) return wrap(err, "decoding result") }) return result, err @@ -95,7 +94,6 @@ func unaryOpE(g geom.Geometry, op func(*handle, *C.GEOSGeometry) error) error { func unaryOpG( g geom.Geometry, - opts []geom.ConstructorOption, op func(C.GEOSContextHandle_t, *C.GEOSGeometry) *C.GEOSGeometry, ) (geom.Geometry, error) { var result geom.Geometry @@ -111,7 +109,7 @@ func unaryOpG( defer C.GEOSGeom_destroy(resultGH) } var err error - result, err = h.decode(resultGH, opts) + result, err = h.decode(resultGH) return wrap(err, "decoding result") }) return result, err diff --git a/geos/handle.go b/geos/handle.go index c7eacc1d..ad94e722 100644 --- a/geos/handle.go +++ b/geos/handle.go @@ -37,8 +37,8 @@ import ( // noop returns the geometry unaltered, via conversion to and from GEOS. This // function is only for benchmarking purposes, hence it is not exported or used // outside of benchmark tests. -func noop(g geom.Geometry, opts ...geom.ConstructorOption) (geom.Geometry, error) { - result, err := unaryOpG(g, opts, func(ctx C.GEOSContextHandle_t, g *C.GEOSGeometry) *C.GEOSGeometry { +func noop(g geom.Geometry) (geom.Geometry, error) { + result, err := unaryOpG(g, func(ctx C.GEOSContextHandle_t, g *C.GEOSGeometry) *C.GEOSGeometry { return C.noop(ctx, g) }) return result, wrap(err, "executing noop") @@ -158,7 +158,7 @@ func (h *handle) boolErr(c C.char) (bool, error) { } } -func (h *handle) decode(gh *C.GEOSGeometry, opts []geom.ConstructorOption) (geom.Geometry, error) { +func (h *handle) decode(gh *C.GEOSGeometry) (geom.Geometry, error) { var ( isWKT C.char size C.size_t @@ -171,10 +171,10 @@ func (h *handle) decode(gh *C.GEOSGeometry, opts []geom.ConstructorOption) (geom if isWKT != 0 { wkt := C.GoStringN(serialised, C.int(size)) - g, err := geom.UnmarshalWKT(wkt, opts...) + g, err := geom.UnmarshalWKT(wkt, geom.DisableAllValidations) return g, wrap(err, "failed to unmarshal GEOS WKT result") } wkb := C.GoBytes(unsafe.Pointer(serialised), C.int(size)) - g, err := geom.UnmarshalWKB(wkb, opts...) + g, err := geom.UnmarshalWKB(wkb, geom.DisableAllValidations) return g, wrap(err, "failed to unmarshal GEOS WKB result") } diff --git a/internal/perf/set_op_test.go b/internal/perf/set_op_test.go index f3718253..db196db2 100644 --- a/internal/perf/set_op_test.go +++ b/internal/perf/set_op_test.go @@ -43,7 +43,7 @@ func BenchmarkSetOperation(b *testing.B) { } func adaptGEOSSetOp( - setOp func(_, _ geom.Geometry, _ ...geom.ConstructorOption) (geom.Geometry, error), + setOp func(_, _ geom.Geometry) (geom.Geometry, error), ) func(_, _ geom.Geometry) (geom.Geometry, error) { return func(g1, g2 geom.Geometry) (geom.Geometry, error) { return setOp(g1, g2)