From 995d3044a466bc9d862700d7949a6dbf93dae42b Mon Sep 17 00:00:00 2001 From: Artem Pavlenko Date: Mon, 26 Feb 2024 14:01:09 +0000 Subject: [PATCH 1/7] GeoJSON - allow empty arrays in "coordinates" element for Multi* geometries (ref #4431) --- include/mapnik/json/create_geometry.hpp | 36 +++++++++++++++++++ .../json/geometry_generator_grammar_impl.hpp | 6 ++-- test/unit/datasource/geojson.cpp | 14 +++++--- 3 files changed, 48 insertions(+), 8 deletions(-) diff --git a/include/mapnik/json/create_geometry.hpp b/include/mapnik/json/create_geometry.hpp index 9df2d063e..707f5afa6 100644 --- a/include/mapnik/json/create_geometry.hpp +++ b/include/mapnik/json/create_geometry.hpp @@ -64,6 +64,10 @@ struct create_linestring { mapnik::geometry::line_string line; std::size_t size = points.size(); + //if (size < 2) + //{ + // throw std::runtime_error("RFC 7946: For type \"LineString\", the \"coordinates\" member is an array of two or more positions."); + //} line.reserve(size); for (auto&& pt : points) { @@ -108,6 +112,12 @@ struct create_polygon mapnik::geometry::correct(geom_); } + void operator()(ring const& points) const + { + //POLYGON EMPTY + geom_ = std::move(mapnik::geometry::polygon{}); + } + template void operator()(T const&) const { @@ -170,6 +180,12 @@ struct create_multilinestring geom_ = std::move(multi_line); } + void operator()(ring const& points) const + { + //MULTILINESTRING EMPTY + geom_ = std::move(mapnik::geometry::multi_line_string{}); + } + template void operator()(T const&) const { @@ -213,6 +229,26 @@ struct create_multipolygon mapnik::geometry::correct(geom_); } + void operator()(rings const& rngs) const + { + //MULTIPOLYGON + mapnik::geometry::multi_polygon multi_poly; + mapnik::geometry::polygon poly; + std::size_t num_rings = rngs.size(); + for (std::size_t i = 0; i < num_rings; ++i) + { + // POLYGON EMPTY + multi_poly.emplace_back(mapnik::geometry::polygon{}); + } + geom_ = std::move(multi_poly); + } + + void operator()(ring const& points) const + { + //MULTIPOLYGON EMPTY + geom_ = std::move(mapnik::geometry::multi_polygon{}); + } + template void operator()(T const&) const { diff --git a/include/mapnik/json/geometry_generator_grammar_impl.hpp b/include/mapnik/json/geometry_generator_grammar_impl.hpp index 5f003c2f1..2940f6ee9 100644 --- a/include/mapnik/json/geometry_generator_grammar_impl.hpp +++ b/include/mapnik/json/geometry_generator_grammar_impl.hpp @@ -83,16 +83,16 @@ geometry_generator_grammar::geometry_generator_grammar linear_ring_coord = lit('[') << -(point_coord % lit(',')) << lit(']')//linestring_coord.alias() ; - polygon_coord = lit('[') << linear_ring_coord % lit(',') << lit(']') + polygon_coord = lit('[') << -(linear_ring_coord % lit(',')) << lit(']') ; multi_point_coord = lit('[') << -(point_coord % lit(',')) << lit(']');//linestring_coord.alias() ; - multi_linestring_coord = lit('[') << linestring_coord % lit(',') << lit(']') + multi_linestring_coord = lit('[') << -(linestring_coord % lit(',')) << lit(']') ; - multi_polygon_coord = lit('[') << polygon_coord % lit(',') << lit("]") + multi_polygon_coord = lit('[') << -(polygon_coord % lit(',')) << lit("]") ; geometries = geometry % lit(',') diff --git a/test/unit/datasource/geojson.cpp b/test/unit/datasource/geojson.cpp index 308e51cff..afd8d84c6 100644 --- a/test/unit/datasource/geojson.cpp +++ b/test/unit/datasource/geojson.cpp @@ -125,11 +125,15 @@ TEST_CASE("geojson") SECTION("GeoJSON empty Geometries handling") { auto valid_empty_geometries = {"null", // Point can't be empty - "{ \"type\": \"LineString\", \"coordinates\": [] }", - "{ \"type\": \"Polygon\", \"coordinates\": [ [ ] ] } ", - "{ \"type\": \"MultiPoint\", \"coordinates\": [ ] }", - "{ \"type\": \"MultiLineString\", \"coordinates\": [ [] ] }", - "{ \"type\": \"MultiPolygon\", \"coordinates\": [[ []] ] }"}; + "{ \"type\": \"LineString\", \"coordinates\":[]}", + "{ \"type\": \"Polygon\", \"coordinates\":[]} ", + "{ \"type\": \"Polygon\", \"coordinates\":[[]]} ", + "{ \"type\": \"MultiPoint\", \"coordinates\":[]}", + "{ \"type\": \"MultiLineString\", \"coordinates\":[]}", + "{ \"type\": \"MultiLineString\", \"coordinates\":[[]]}", + "{ \"type\": \"MultiPolygon\", \"coordinates\":[]}", + "{ \"type\": \"MultiPolygon\", \"coordinates\":[[]]}", + "{ \"type\": \"MultiPolygon\", \"coordinates\": [[[]]] }"}; for (auto const& in : valid_empty_geometries) { From 127f9ba143c827a1e68cdc8d7b05073d569b7652 Mon Sep 17 00:00:00 2001 From: Artem Pavlenko Date: Mon, 26 Feb 2024 14:39:41 +0000 Subject: [PATCH 2/7] add missing copy --- test/unit/geometry/is_empty.cpp | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/test/unit/geometry/is_empty.cpp b/test/unit/geometry/is_empty.cpp index d1d449a57..0692b2c9d 100644 --- a/test/unit/geometry/is_empty.cpp +++ b/test/unit/geometry/is_empty.cpp @@ -1,3 +1,26 @@ +/***************************************************************************** + * + * This file is part of Mapnik (c++ mapping toolkit) + * + * Copyright (C) 2024 Artem Pavlenko + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA + * + *****************************************************************************/ + + #include "catch.hpp" #include From b911464472f2394fba02ba40ddaa9fbe91b74261 Mon Sep 17 00:00:00 2001 From: Artem Pavlenko Date: Mon, 26 Feb 2024 14:40:36 +0000 Subject: [PATCH 3/7] correct 'is_empty' implementation for multi geometries --- include/mapnik/geometry/is_empty.hpp | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/include/mapnik/geometry/is_empty.hpp b/include/mapnik/geometry/is_empty.hpp index fccf7eae0..e39a5b9f7 100644 --- a/include/mapnik/geometry/is_empty.hpp +++ b/include/mapnik/geometry/is_empty.hpp @@ -39,18 +39,36 @@ struct geometry_is_empty bool operator()(mapnik::geometry::point const&) const { return false; } - bool operator()(mapnik::geometry::line_string const& geom) const { return geom.empty(); } + bool operator()(mapnik::geometry::line_string const& line) const { return line.empty(); } - bool operator()(mapnik::geometry::polygon const& geom) const + bool operator()(mapnik::geometry::polygon const& poly) const { - return geom.empty() || geom.front().empty(); + for (auto const& ring : poly) + { + if (!ring.empty()) return false; + } + return true; } bool operator()(mapnik::geometry::multi_point const& geom) const { return geom.empty(); } - bool operator()(mapnik::geometry::multi_line_string const& geom) const { return geom.empty(); } + bool operator()(mapnik::geometry::multi_line_string const& mline) const + { + for (auto const& line : mline) + { + if (!line.empty()) return false; + } + return true; + } - bool operator()(mapnik::geometry::multi_polygon const& geom) const { return geom.empty(); } + bool operator()(mapnik::geometry::multi_polygon const& mpoly) const + { + for (auto const& poly : mpoly) + { + if (!operator()(poly)) return false; + } + return true; + } bool operator()(mapnik::geometry::geometry_collection const& geom) const { return geom.empty(); } From 0e9f8b06a31495c5e82aac56ca6180da67dad1fc Mon Sep 17 00:00:00 2001 From: Artem Pavlenko Date: Mon, 26 Feb 2024 14:44:19 +0000 Subject: [PATCH 4/7] Fix `is_empty` unit test e.g if multi geometry consists of empty geometries it is "empty". --- test/unit/geometry/is_empty.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/unit/geometry/is_empty.cpp b/test/unit/geometry/is_empty.cpp index 0692b2c9d..63caef803 100644 --- a/test/unit/geometry/is_empty.cpp +++ b/test/unit/geometry/is_empty.cpp @@ -118,7 +118,7 @@ TEST_CASE("geometry is_empty") mapnik::geometry::multi_line_string geom; mapnik::geometry::line_string line; geom.emplace_back(std::move(line)); - REQUIRE(!mapnik::geometry::is_empty(geom)); + REQUIRE(mapnik::geometry::is_empty(geom)); } } @@ -134,7 +134,7 @@ TEST_CASE("geometry is_empty") mapnik::geometry::linear_ring ring; poly.push_back(std::move(ring)); geom.emplace_back(std::move(poly)); - REQUIRE(!mapnik::geometry::is_empty(geom)); + REQUIRE(mapnik::geometry::is_empty(geom)); } } } From c067399041756bc65f6325b6d68bf375bce8c2f0 Mon Sep 17 00:00:00 2001 From: Artem Pavlenko Date: Mon, 26 Feb 2024 14:45:59 +0000 Subject: [PATCH 5/7] GeoJSON unit tests - require expected geometries are "empty". --- test/unit/datasource/geojson.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/unit/datasource/geojson.cpp b/test/unit/datasource/geojson.cpp index afd8d84c6..7c35e7f1b 100644 --- a/test/unit/datasource/geojson.cpp +++ b/test/unit/datasource/geojson.cpp @@ -28,6 +28,7 @@ #include #include #include +#include #include #include #include @@ -140,6 +141,11 @@ TEST_CASE("geojson") std::string json(in); mapnik::geometry::geometry geom; CHECK(mapnik::json::from_geojson(json, geom)); + if (!mapnik::geometry::is_empty(geom)) + { + std::cerr << json << std::endl; + } + //REQUIRE(mapnik::geometry::is_empty(geom)); // round trip std::string json_out; CHECK(mapnik::util::to_geojson(json_out, geom)); From 38193817defe3dfef32dd7a58b5cd6724c7d3b05 Mon Sep 17 00:00:00 2001 From: Artem Pavlenko Date: Mon, 26 Feb 2024 14:48:49 +0000 Subject: [PATCH 6/7] pre-commit run --show-diff-on-failure --color=always --all-files [skip ci] --- include/mapnik/geometry/is_empty.hpp | 9 ++++++--- include/mapnik/json/create_geometry.hpp | 15 ++++++++------- test/unit/datasource/geojson.cpp | 4 ++-- test/unit/geometry/is_empty.cpp | 1 - 4 files changed, 16 insertions(+), 13 deletions(-) diff --git a/include/mapnik/geometry/is_empty.hpp b/include/mapnik/geometry/is_empty.hpp index e39a5b9f7..71c7ad70b 100644 --- a/include/mapnik/geometry/is_empty.hpp +++ b/include/mapnik/geometry/is_empty.hpp @@ -45,7 +45,8 @@ struct geometry_is_empty { for (auto const& ring : poly) { - if (!ring.empty()) return false; + if (!ring.empty()) + return false; } return true; } @@ -56,7 +57,8 @@ struct geometry_is_empty { for (auto const& line : mline) { - if (!line.empty()) return false; + if (!line.empty()) + return false; } return true; } @@ -65,7 +67,8 @@ struct geometry_is_empty { for (auto const& poly : mpoly) { - if (!operator()(poly)) return false; + if (!operator()(poly)) + return false; } return true; } diff --git a/include/mapnik/json/create_geometry.hpp b/include/mapnik/json/create_geometry.hpp index 707f5afa6..de9cab58d 100644 --- a/include/mapnik/json/create_geometry.hpp +++ b/include/mapnik/json/create_geometry.hpp @@ -64,10 +64,11 @@ struct create_linestring { mapnik::geometry::line_string line; std::size_t size = points.size(); - //if (size < 2) + // if (size < 2) //{ - // throw std::runtime_error("RFC 7946: For type \"LineString\", the \"coordinates\" member is an array of two or more positions."); - //} + // throw std::runtime_error("RFC 7946: For type \"LineString\", the \"coordinates\" member is an array of + // two or more positions."); + // } line.reserve(size); for (auto&& pt : points) { @@ -114,7 +115,7 @@ struct create_polygon void operator()(ring const& points) const { - //POLYGON EMPTY + // POLYGON EMPTY geom_ = std::move(mapnik::geometry::polygon{}); } @@ -182,7 +183,7 @@ struct create_multilinestring void operator()(ring const& points) const { - //MULTILINESTRING EMPTY + // MULTILINESTRING EMPTY geom_ = std::move(mapnik::geometry::multi_line_string{}); } @@ -231,7 +232,7 @@ struct create_multipolygon void operator()(rings const& rngs) const { - //MULTIPOLYGON + // MULTIPOLYGON mapnik::geometry::multi_polygon multi_poly; mapnik::geometry::polygon poly; std::size_t num_rings = rngs.size(); @@ -245,7 +246,7 @@ struct create_multipolygon void operator()(ring const& points) const { - //MULTIPOLYGON EMPTY + // MULTIPOLYGON EMPTY geom_ = std::move(mapnik::geometry::multi_polygon{}); } diff --git a/test/unit/datasource/geojson.cpp b/test/unit/datasource/geojson.cpp index 7c35e7f1b..a8812c7d4 100644 --- a/test/unit/datasource/geojson.cpp +++ b/test/unit/datasource/geojson.cpp @@ -145,8 +145,8 @@ TEST_CASE("geojson") { std::cerr << json << std::endl; } - //REQUIRE(mapnik::geometry::is_empty(geom)); - // round trip + // REQUIRE(mapnik::geometry::is_empty(geom)); + // round trip std::string json_out; CHECK(mapnik::util::to_geojson(json_out, geom)); json.erase(std::remove_if(std::begin(json), diff --git a/test/unit/geometry/is_empty.cpp b/test/unit/geometry/is_empty.cpp index 63caef803..2307ae66d 100644 --- a/test/unit/geometry/is_empty.cpp +++ b/test/unit/geometry/is_empty.cpp @@ -20,7 +20,6 @@ * *****************************************************************************/ - #include "catch.hpp" #include From 5b08c21eb092d6df6d8c6b47d884e54dd57d0488 Mon Sep 17 00:00:00 2001 From: Artem Pavlenko Date: Mon, 26 Feb 2024 16:25:40 +0000 Subject: [PATCH 7/7] Actually enable 'is_empty' test + cleanup --- test/unit/datasource/geojson.cpp | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/test/unit/datasource/geojson.cpp b/test/unit/datasource/geojson.cpp index a8812c7d4..30522d12b 100644 --- a/test/unit/datasource/geojson.cpp +++ b/test/unit/datasource/geojson.cpp @@ -141,11 +141,7 @@ TEST_CASE("geojson") std::string json(in); mapnik::geometry::geometry geom; CHECK(mapnik::json::from_geojson(json, geom)); - if (!mapnik::geometry::is_empty(geom)) - { - std::cerr << json << std::endl; - } - // REQUIRE(mapnik::geometry::is_empty(geom)); + REQUIRE(mapnik::geometry::is_empty(geom)); // round trip std::string json_out; CHECK(mapnik::util::to_geojson(json_out, geom));