From 24cfa26a0608bc3b439f5fe21ee95ed2941b8d7c Mon Sep 17 00:00:00 2001 From: Casper van der Wel Date: Fri, 23 Jun 2023 14:08:22 +0200 Subject: [PATCH 1/2] Remove NaN point special case logic --- NEWS.md | 2 ++ include/geos/geom/CoordinateSequence.h | 14 ----------- src/geom/GeometryFactory.cpp | 33 ++++---------------------- tests/unit/geom/PointTest.cpp | 2 +- tests/unit/io/GeoJSONWriterTest.cpp | 12 ++++++++++ tests/unit/io/WKTReaderTest.cpp | 16 +++++++++++++ tests/unit/io/WKTWriterTest.cpp | 3 +-- 7 files changed, 36 insertions(+), 46 deletions(-) diff --git a/NEWS.md b/NEWS.md index 47efeb8a62..66f372c0a0 100644 --- a/NEWS.md +++ b/NEWS.md @@ -29,11 +29,13 @@ xxxx-xx-xx - CoverageUnion now requires valid inputs to produce valid outputs and may return invalid outputs silently when fed invalid inputs. Use CoverageValidator first if you do not know the validity of your data. + - WKTReader: Points with all-NaN coordinates are not considered empty anymore (#..., Casper van der Wel) - Fixes/Improvements: - WKTReader: Fix parsing of Z and M flags in WKTReader (#676 and GH-669, Dan Baston) - WKTReader: Throw exception on inconsistent geometry dimension (#1080, Dan Baston) - WKTReader: Throw exception if WKT contains extra text after end of geometry (#1095, Dan Baston) + - WKTReader: Points with all-NaN coordinates are written as such (#..., Casper van der Wel) - GEOSIntersects: Fix crash with empty point inputs (#1110, Dan Baston) - GEOSIntersects: Improve performance/robustness by using PreparedGeometry algorithm (GH-775, Dan Baston) - LineMerger: Recursively collect all components from GeometryCollections (#401, Dan Baston) diff --git a/include/geos/geom/CoordinateSequence.h b/include/geos/geom/CoordinateSequence.h index 3070ec834a..34a725bd3d 100644 --- a/include/geos/geom/CoordinateSequence.h +++ b/include/geos/geom/CoordinateSequence.h @@ -197,20 +197,6 @@ class GEOS_DLL CoordinateSequence { return m_vect.empty(); } - /// Returns true if there is 1 coordinate and if it is null. - bool isNullPoint() const { - if (size() != 1) { - return false; - } - switch(getCoordinateType()) { - case CoordinateType::XY: return getAt(0).isNull(); - case CoordinateType::XYZ: return getAt(0).isNull(); - case CoordinateType::XYZM: return getAt(0).isNull(); - case CoordinateType::XYM: return getAt(0).isNull(); - default: return false; - } - } - /** \brief * Tests whether an a {@link CoordinateSequence} forms a ring, * by checking length and closure. Self-intersection is not checked. diff --git a/src/geom/GeometryFactory.cpp b/src/geom/GeometryFactory.cpp index d30ee4906a..881189fc20 100644 --- a/src/geom/GeometryFactory.cpp +++ b/src/geom/GeometryFactory.cpp @@ -226,8 +226,6 @@ GeometryFactory::createPoint(std::unique_ptr&& coords) const { if (!coords) { return createPoint(); - } else if ((*coords).isNullPoint()) { - return createPoint((*coords).getDimension()); } return std::unique_ptr(new Point(std::move(*coords), this)); } @@ -235,55 +233,32 @@ GeometryFactory::createPoint(std::unique_ptr&& coords) const std::unique_ptr GeometryFactory::createPoint(const CoordinateXY& coordinate) const { - if(coordinate.isNull()) { - return createPoint(2); - } - else { - return std::unique_ptr(new Point(coordinate, this)); - } + return std::unique_ptr(new Point(coordinate, this)); } /*public*/ std::unique_ptr GeometryFactory::createPoint(const Coordinate& coordinate) const { - if(coordinate.isNull()) { - return createPoint(3); - } - else { - return std::unique_ptr(new Point(coordinate, this)); - } + return std::unique_ptr(new Point(coordinate, this)); } std::unique_ptr GeometryFactory::createPoint(const CoordinateXYM& coordinate) const { - if(coordinate.isNull()) { - return createPoint(4); // can't do XYM! - } - else { - return std::unique_ptr(new Point(coordinate, this)); - } + return std::unique_ptr(new Point(coordinate, this)); } std::unique_ptr GeometryFactory::createPoint(const CoordinateXYZM& coordinate) const { - if(coordinate.isNull()) { - return createPoint(4); - } - else { - return std::unique_ptr(new Point(coordinate, this)); - } + return std::unique_ptr(new Point(coordinate, this)); } /*public*/ std::unique_ptr GeometryFactory::createPoint(const CoordinateSequence& fromCoords) const { - if (fromCoords.isNullPoint()) { - return createPoint(fromCoords.getDimension()); - } CoordinateSequence newCoords(fromCoords); return std::unique_ptr(new Point(std::move(newCoords), this)); diff --git a/tests/unit/geom/PointTest.cpp b/tests/unit/geom/PointTest.cpp index 77245af0a6..b19de45fe5 100644 --- a/tests/unit/geom/PointTest.cpp +++ b/tests/unit/geom/PointTest.cpp @@ -597,7 +597,7 @@ void object::test<46> coords->add(Coordinate(geos::DoubleNotANumber, geos::DoubleNotANumber)); auto point = factory_->createPoint(std::move(coords)); - ensure("point->isEmpty()", point->isEmpty()); + ensure("point->isEmpty()", !point->isEmpty()); ensure("point->getCoordinateDimension() == 2", point->getCoordinateDimension() == 2); } diff --git a/tests/unit/io/GeoJSONWriterTest.cpp b/tests/unit/io/GeoJSONWriterTest.cpp index 4a3cf67f6f..30862b168e 100644 --- a/tests/unit/io/GeoJSONWriterTest.cpp +++ b/tests/unit/io/GeoJSONWriterTest.cpp @@ -306,5 +306,17 @@ void object::test<19> ensure_equals(result, "{\"type\":\"LineString\",\"coordinates\":[[0.0,0.0],[1.0,1.0],[1.0,0.0],[0.0,0.0]]}"); } +// Write a point with all-nan coordinates +// https://github.com/libgeos/geos/issues/885 +template<> +template<> +void object::test<20> +() +{ + GeomPtr geom(wktreader.read("POINT (NaN NaN)")); + std::string result = geojsonwriter.write(geom.get()); + ensure_equals(result, "{\"type\":\"Point\",\"coordinates\":[null,null]}"); +} + } diff --git a/tests/unit/io/WKTReaderTest.cpp b/tests/unit/io/WKTReaderTest.cpp index 3180a66869..b247ba844e 100644 --- a/tests/unit/io/WKTReaderTest.cpp +++ b/tests/unit/io/WKTReaderTest.cpp @@ -447,4 +447,20 @@ void object::test<22> ensure_dimension("MULTIPOINT ZM ((0 0 4 3), (1 2 4 5))", true, true); } +// accept NaN coordinates +template<> +template<> +void object::test<23> +() +{ + GeomPtr geom(wktreader.read("POINT(NaN NaN)")); + auto coords = geom->getCoordinates(); + + ensure(!coords->isEmpty()); + ensure(coords->getDimension() == 2); + ensure(std::isnan(coords->getX(0))); + ensure(std::isnan(coords->getY(0))); +} + + } // namespace tut diff --git a/tests/unit/io/WKTWriterTest.cpp b/tests/unit/io/WKTWriterTest.cpp index c22853be26..2292275ecc 100644 --- a/tests/unit/io/WKTWriterTest.cpp +++ b/tests/unit/io/WKTWriterTest.cpp @@ -223,7 +223,7 @@ void object::test<7> auto point = factory_->createPoint(std::move(coords)); std::string result = wktwriter.write(point.get()); - ensure_equals(result, std::string("POINT EMPTY")); + ensure_equals(result, std::string("POINT (NaN NaN)")); } @@ -406,4 +406,3 @@ void object::test<14> } } // namespace tut - From 48caa3a89f0c5c78fd13850d91277164d4c3f542 Mon Sep 17 00:00:00 2001 From: Casper van der Wel Date: Sat, 24 Jun 2023 10:46:16 +0200 Subject: [PATCH 2/2] Fix GEOSInterpolate --- capi/geos_ts_c.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/capi/geos_ts_c.cpp b/capi/geos_ts_c.cpp index 9c42927fc2..69ea4f5107 100644 --- a/capi/geos_ts_c.cpp +++ b/capi/geos_ts_c.cpp @@ -3785,7 +3785,7 @@ extern "C" { geos::linearref::LengthIndexedLine lil(g); geos::geom::Coordinate coord = lil.extractPoint(d); const GeometryFactory* gf = handle->geomFactory; - auto point = gf->createPoint(coord); + auto point = coord.isNull() ? gf->createPoint(g->getCoordinateDimension()) : gf->createPoint(coord); point->setSRID(g->getSRID()); return point.release(); });