diff --git a/CHANGELOG.md b/CHANGELOG.md index 08afb87cc..d33353a04 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,7 @@ Released: - JSON parsing: unified error_handler across all grammars - Improved unit test coverage - Raster scaling: fixed nodata handling, acurracy when working with small floats and clipping floats by \[0; 255\] (https://github.com/mapnik/mapnik/pull/3147) + - Centroid algorithm: fixed invalid input handling, particularly empty geometries (https://github.com/mapnik/mapnik/pull/3185) ## 3.0.8 diff --git a/include/mapnik/geometry_centroid.hpp b/include/mapnik/geometry_centroid.hpp index 8162d6ec6..590ba4898 100644 --- a/include/mapnik/geometry_centroid.hpp +++ b/include/mapnik/geometry_centroid.hpp @@ -27,6 +27,7 @@ #include #include #include +#include namespace mapnik { namespace geometry { @@ -58,60 +59,64 @@ struct geometry_centroid result_type operator() (point const& geom) const { - boost::geometry::centroid(geom, pt_); - return true; + return centroid_simple(geom); } result_type operator() (line_string const& geom) const { - if (mapnik::geometry::is_empty(geom)) - { - return false; - } - boost::geometry::centroid(geom, pt_); - return true; + return centroid_simple(geom); } result_type operator() (polygon const& geom) const { - if (mapnik::geometry::is_empty(geom)) - { - return false; - } - boost::geometry::centroid(geom, pt_); - return true; + return centroid_simple(geom); } result_type operator() (multi_point const& geom) const { - if (mapnik::geometry::is_empty(geom)) - { - return false; - } - boost::geometry::centroid(geom, pt_); - return true; + return centroid_simple(geom); } result_type operator() (multi_line_string const& geom) const { - if (mapnik::geometry::is_empty(geom) || mapnik::geometry::has_empty(geom)) - { - return false; - } - boost::geometry::centroid(geom, pt_); - return true; + return centroid_multi(geom); } result_type operator() (multi_polygon const& geom) const { - if (mapnik::geometry::is_empty(geom) || mapnik::geometry::has_empty(geom)) + return centroid_multi(geom); + } + + point & pt_; + +private: + template + result_type centroid_simple(Geom const & geom) const + { + try + { + boost::geometry::centroid(geom, pt_); + return true; + } + catch (boost::geometry::centroid_exception const & e) { return false; } - boost::geometry::centroid(geom, pt_); - return true; } - point & pt_; + + template + result_type centroid_multi(Geom const & geom) const + { +// https://github.com/mapnik/mapnik/issues/3169 +#if BOOST_VERSION <= 105900 + if (mapnik::geometry::has_empty(geom)) + { + Geom stripped = mapnik::geometry::remove_empty(geom); + return centroid_simple(stripped); + } +#endif + return centroid_simple(geom); + } }; } diff --git a/include/mapnik/geometry_remove_empty.hpp b/include/mapnik/geometry_remove_empty.hpp new file mode 100644 index 000000000..9838b5a61 --- /dev/null +++ b/include/mapnik/geometry_remove_empty.hpp @@ -0,0 +1,77 @@ +/***************************************************************************** + * + * This file is part of Mapnik (c++ mapping toolkit) + * + * Copyright (C) 2015 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 + * + *****************************************************************************/ + +#ifndef MAPNIK_GEOMETRY_REMOVE_EMPTY_HPP +#define MAPNIK_GEOMETRY_REMOVE_EMPTY_HPP + +#include +#include + +namespace mapnik { namespace geometry { + +namespace detail { + +struct geometry_remove_empty +{ + mapnik::geometry::multi_line_string operator() (mapnik::geometry::multi_line_string const & geom) const + { + return remove_empty(geom); + } + + mapnik::geometry::multi_polygon operator() (mapnik::geometry::multi_polygon const & geom) const + { + return remove_empty(geom); + } + + template + T operator() (T const & geom) const + { + return geom; + } + +private: + template + T remove_empty(T const & geom) const + { + T new_geom; + for (auto const & g : geom) + { + if (!g.empty()) + { + new_geom.emplace_back(g); + } + } + return new_geom; + } +}; + +} + +template +inline GeomType remove_empty(GeomType const& geom) +{ + return detail::geometry_remove_empty()(geom); +} + +}} + +#endif // MAPNIK_GEOMETRY_REMOVE_EMPTY_HPP diff --git a/test/unit/geometry/centroid.cpp b/test/unit/geometry/centroid.cpp index 5f080fffc..1e58aad4e 100644 --- a/test/unit/geometry/centroid.cpp +++ b/test/unit/geometry/centroid.cpp @@ -132,7 +132,9 @@ SECTION("multi-linestring: one component empty") { geom.emplace_back(std::move(line)); geom.emplace_back(); mapnik::geometry::point centroid; - REQUIRE(!mapnik::geometry::centroid(geom, centroid)); + REQUIRE(mapnik::geometry::centroid(geom, centroid)); + REQUIRE(centroid.x == 0); + REQUIRE(centroid.y == 25); } SECTION("empty multi-linestring") { @@ -189,7 +191,9 @@ SECTION("multi-polygon: one component empty") { geom.emplace_back(); mapnik::geometry::point centroid; - REQUIRE(!mapnik::geometry::centroid(geom, centroid)); + REQUIRE(mapnik::geometry::centroid(geom, centroid)); + REQUIRE(centroid.x == 0.5); + REQUIRE(centroid.y == 0.5); } SECTION("empty multi-polygon") { diff --git a/test/unit/geometry/remove_empty.cpp b/test/unit/geometry/remove_empty.cpp new file mode 100644 index 000000000..15237542f --- /dev/null +++ b/test/unit/geometry/remove_empty.cpp @@ -0,0 +1,53 @@ +#include "catch.hpp" + +#include + +TEST_CASE("geometry remove_empty") { + +SECTION("point") { + + using geom_type = mapnik::geometry::point; + geom_type pt(10, 10); + geom_type pt2 = mapnik::geometry::remove_empty(pt); + REQUIRE(pt.x == pt2.x); + REQUIRE(pt.y == pt2.y); +} + +SECTION("multi-linestring") { + + using geom_type = mapnik::geometry::multi_line_string; + geom_type geom; + mapnik::geometry::line_string line; + line.add_coord(0, 0); + line.add_coord(0, 25); + line.add_coord(0, 50); + geom.emplace_back(std::move(line)); + geom.emplace_back(); + + REQUIRE(geom.size() == 2); + geom_type geom2 = mapnik::geometry::remove_empty(geom); + REQUIRE(geom2.size() == 1); + REQUIRE(geom2[0].size() == 3); +} + +SECTION("multi-polygon") { + + using geom_type = mapnik::geometry::multi_polygon; + geom_type geom; + mapnik::geometry::polygon poly; + mapnik::geometry::linear_ring ring; + ring.add_coord(0, 0); + ring.add_coord(1, 0); + ring.add_coord(1, 1); + ring.add_coord(0, 1); + ring.add_coord(0, 0); + poly.set_exterior_ring(std::move(ring)); + geom.emplace_back(std::move(poly)); + geom.emplace_back(); + + REQUIRE(geom.size() == 2); + geom_type geom2 = mapnik::geometry::remove_empty(geom); + REQUIRE(geom2.size() == 1); + REQUIRE(geom2[0].exterior_ring.size() == 5); +} +}