centroid: enable algorithm on multi-geometries with empty sub-geometries

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
 

include/mapnik/geometry_centroid.hpp

@@ -27,6 +27,7 @@
 #include <mapnik/geometry_adapters.hpp>
 #include <boost/geometry/algorithms/centroid.hpp>
 #include <mapnik/geometry_is_empty.hpp>
+#include <mapnik/geometry_remove_empty.hpp>
 
 namespace mapnik { namespace geometry {
 
@@ -58,60 +59,64 @@ struct geometry_centroid
 
     result_type operator() (point<T> const& geom) const
     {
-        boost::geometry::centroid(geom, pt_);
+        return centroid_simple(geom);
-        return true;
     }
 
     result_type operator() (line_string<T> const& geom) const
     {
-        if (mapnik::geometry::is_empty(geom))
+        return centroid_simple(geom);
-        {
-            return false;
-        }
-        boost::geometry::centroid(geom, pt_);
-        return true;
     }
 
     result_type operator() (polygon<T> const& geom) const
     {
-        if (mapnik::geometry::is_empty(geom))
+        return centroid_simple(geom);
-        {
-            return false;
-        }
-        boost::geometry::centroid(geom, pt_);
-        return true;
     }
 
     result_type operator() (multi_point<T> const& geom) const
     {
-        if (mapnik::geometry::is_empty(geom))
+        return centroid_simple(geom);
-        {
-            return false;
-        }
-        boost::geometry::centroid(geom, pt_);
-        return true;
     }
 
     result_type operator() (multi_line_string<T> const& geom) const
     {
-        if (mapnik::geometry::is_empty(geom) || mapnik::geometry::has_empty(geom))
+        return centroid_multi(geom);
-        {
-            return false;
-        }
-        boost::geometry::centroid(geom, pt_);
-        return true;
     }
 
     result_type operator() (multi_polygon<T> const& geom) const
     {
-        if (mapnik::geometry::is_empty(geom) || mapnik::geometry::has_empty(geom))
+        return centroid_multi(geom);
+    }
+
+    point<T> & pt_;
+
+private:
+    template <typename Geom>
+    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<T> & pt_;
+
+    template <typename Geom>
+    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);
+    }
 };
 
 }

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 <mapnik/geometry.hpp>
+#include <mapnik/geometry_is_empty.hpp>
+
+namespace mapnik { namespace geometry {
+
+namespace detail {
+
+struct geometry_remove_empty
+{
+    mapnik::geometry::multi_line_string<double> operator() (mapnik::geometry::multi_line_string<double> const & geom) const
+    {
+        return remove_empty(geom);
+    }
+
+    mapnik::geometry::multi_polygon<double> operator() (mapnik::geometry::multi_polygon<double> const & geom) const
+    {
+        return remove_empty(geom);
+    }
+
+    template <typename T>
+    T operator() (T const & geom) const
+    {
+        return geom;
+    }
+
+private:
+    template <typename T>
+    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 <typename GeomType>
+inline GeomType remove_empty(GeomType const& geom)
+{
+    return detail::geometry_remove_empty()(geom);
+}
+
+}}
+
+#endif // MAPNIK_GEOMETRY_REMOVE_EMPTY_HPP

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<double> 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<double> 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") {

test/unit/geometry/remove_empty.cpp

@@ -0,0 +1,53 @@
+#include "catch.hpp"
+
+#include <mapnik/geometry_remove_empty.hpp>
+
+TEST_CASE("geometry remove_empty") {
+
+SECTION("point") {
+
+    using geom_type = mapnik::geometry::point<double>;
+    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<double>;
+    geom_type geom;
+    mapnik::geometry::line_string<double> 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<double>;
+    geom_type geom;
+    mapnik::geometry::polygon<double> poly;
+    mapnik::geometry::linear_ring<double> 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);
+}
+}