don't inherit multi_point from line_string

[jfirebaugh]

Can multi_point inherit from std::vector< point<T> > directly, rather than line_string<T>? Semantically, it is not a line string and should not auto-convert to one.

[artemp]

@jfirebaugh - good call! Agree, deriving from line_string is confusing.

[artemp]

Digging a bit deeper, the reason for multi_point inheriting from line_string was less duplication/verbosity when used in algorithms:

diff --git a/include/mapnik/proj_transform.hpp b/include/mapnik/proj_transform.hpp
index 3247a15..82b732f 100644
--- a/include/mapnik/proj_transform.hpp
+++ b/include/mapnik/proj_transform.hpp
@@ -26,12 +26,13 @@
 // mapnik
 #include <mapnik/config.hpp>
 #include <mapnik/util/noncopyable.hpp>
+// stl
+#include <vector>

 namespace mapnik {

 namespace geometry {
 template <typename T> struct point;
-template <typename T> struct line_string;
 }
 class projection;
 template <typename T> class box2d;
@@ -50,8 +51,8 @@ public:
     bool backward (double *x, double *y , double *z, int point_count, int offset = 1) const;
     bool forward (geometry::point<double> & p) const;
     bool backward (geometry::point<double> & p) const;
-    unsigned int forward (geometry::line_string<double> & ls) const;
-    unsigned int backward (geometry::line_string<double> & ls) const;
+    unsigned int forward (std::vector<geometry::point<double>> & ls) const;
+    unsigned int backward (std::vector<geometry::point<double>> & ls) const;
     bool forward (box2d<double> & box) const;
     bool backward (box2d<double> & box) const;
     bool forward (box2d<double> & box, int points) const;
diff --git a/include/mapnik/well_known_srs.hpp b/include/mapnik/well_known_srs.hpp
index e8349e7..d175ef5 100644
--- a/include/mapnik/well_known_srs.hpp
+++ b/include/mapnik/well_known_srs.hpp
@@ -26,7 +26,7 @@
 // mapnik
 #include <mapnik/global.hpp> // for M_PI on windows
 #include <mapnik/enumeration.hpp>
-#include <mapnik/geometry.hpp>
+#include <mapnik/geometry/point.hpp>

 #pragma GCC diagnostic push
 #include <mapnik/warning_ignore.hpp>
@@ -35,6 +35,7 @@

 // stl
 #include <cmath>
+#include <vector>

 namespace mapnik {

@@ -93,9 +94,9 @@ static inline bool merc2lonlat(double * x, double * y , int point_count)
     return true;
 }

-static inline bool lonlat2merc(geometry::line_string<double> & ls)
+static inline bool lonlat2merc(std::vector<geometry::point<double>> & ls)
 {
-    for(auto & p : ls)
+    for (auto& p : ls)
     {
         if (p.x > 180) p.x = 180;
         else if (p.x < -180) p.x = -180;
@@ -108,7 +109,7 @@ static inline bool lonlat2merc(geometry::line_string<double> & ls)
     return true;
 }

-static inline bool merc2lonlat(geometry::line_string<double> & ls)
+static inline bool merc2lonlat(std::vector<geometry::point<double>> & ls)
 {
     for (auto & p : ls)
     {
diff --git a/src/proj_transform.cpp b/src/proj_transform.cpp
index 108e3f9..a418d5d 100644
--- a/src/proj_transform.cpp
+++ b/src/proj_transform.cpp
@@ -102,7 +102,7 @@ bool proj_transform::forward (geometry::point<double> & p) const
     return forward(&(p.x), &(p.y), &z, 1);
 }

-unsigned int proj_transform::forward (geometry::line_string<double> & ls) const
+unsigned int proj_transform::forward (std::vector<geometry::point<double>> & ls) const
 {
     std::size_t size = ls.size();
     if (size == 0) return 0;
@@ -242,7 +242,7 @@ bool proj_transform::backward (geometry::point<double> & p) const
     return backward(&(p.x), &(p.y), &z, 1);
 }

-unsigned int proj_transform::backward (geometry::line_string<double> & ls) const
+unsigned int proj_transform::backward (std::vector<geometry::point<double>> & ls) const
 {
     std::size_t size = ls.size();
     if (size == 0) return 0;
diff --git a/test/unit/geometry/geometry_equal.hpp b/test/unit/geometry/geometry_equal.hpp
index 4ff80ab..6a510ce 100644
--- a/test/unit/geometry/geometry_equal.hpp
+++ b/test/unit/geometry/geometry_equal.hpp
@@ -119,7 +119,7 @@ struct geometry_equal_visitor
     }

     template <typename T>
-    void operator() (line_string<T> const& ls1, line_string<T> const& ls2) const
+    void operator() (std::vector<point<T>> const& ls1, std::vector<point<T>> const& ls2) const
     {
         if (ls1.size() != ls2.size())
         {
@@ -150,11 +150,18 @@ struct geometry_equal_visitor
     }

     template <typename T>
+    void operator() (line_string<T> const& ls1, line_string<T> const& ls2) const
+    {
+        (*this)(static_cast<std::vector<point<T>> const&>(ls1), static_cast<std::vector<point<T>> const&>(ls2));
+    }
+
+    template <typename T>
     void operator() (multi_point<T> const& mp1, multi_point<T> const& mp2) const
     {
-        (*this)(static_cast<line_string<T> const&>(mp1), static_cast<line_string<T> const&>(mp2));
+        (*this)(static_cast<std::vector<point<T>> const&>(mp1), static_cast<std::vector<point<T>> const&>(mp2));
     }

+
     template <typename T>
     void operator() (multi_line_string<T> const& mls1, multi_line_string<T> const& mls2) const
     {

Maybe we should add a common base class but for now lets derive from container directly.

Here is the latest multi_point implementation

template <typename T, template <typename...> class Cont = std::vector>
struct multi_point : Cont<point<T>>
{
    using point_type = point<T>;
    using container_type = Cont<point_type>;
    multi_point() = default;
    explicit multi_point(std::size_t size)
        : container_type(size) {}
    inline std::size_t num_points() const { return container_type::size(); }
    inline void add_coord(T x, T y) { container_type::template emplace_back(x, y);}
};

The main difference is a template template parameter for container type. std::vector is great but there are some others, too.

mapbox::geometry::multi_point<double>; // default to std::vector
mapbox::geometry::multi_point<double, boost::container::vector>; // use Boost.Container vector implementation.
mapbox::geometry::multi_point<float, std::deque>; // deque

/cc @jfirebaugh @kkaefer @flippmoke @springmeyer

[jfirebaugh]

@artemp Can you explain what you mean by "duplication/verbosity when used in algorithms", or what's going on in the diff you posted? Not sure I understand.

I'm 👍 on the current implementation, deriving from container directly.

[artemp]

@jfirebaugh - sorry it wasn't very clear, but that diff ^ demonstrates how @flippmoke was using the fact that from data structure point of view line_string and multi_point are essentially the same.

(*this)(static_cast<line_string<T> const&>(mp1), static_cast<line_string<T> const&>(mp2));

After converting mapnik to use new multi_point I realized that verbosity is really not an issue, so lets stick with current implementation.

[artemp]

ok this is in master, closing

[springmeyer]

@artemp - you forgot to actually close ;)

[artemp]

@springmeyer - :) cheers