fix topojson parsing (work-in-progress)

include/mapnik/json/topojson_grammar.hpp

@@ -70,8 +70,6 @@ private:
     // properties
     qi::rule<Iterator, space_type, mapnik::topojson::properties()> properties;
     qi::rule<Iterator, space_type, mapnik::topojson::properties()> attributes;
-    // id
-    qi::rule<Iterator,space_type> id;
 };
 
 }}

include/mapnik/json/topojson_grammar_impl.hpp

@@ -71,13 +71,13 @@ BOOST_FUSION_ADAPT_STRUCT(
 
 BOOST_FUSION_ADAPT_STRUCT(
     mapnik::topojson::linestring,
-    (mapnik::topojson::index_type, ring)
+    (std::vector<mapnik::topojson::index_type>, rings)
     (boost::optional<mapnik::topojson::properties>, props)
     )
 
 BOOST_FUSION_ADAPT_STRUCT(
     mapnik::topojson::multi_linestring,
-    (std::vector<mapnik::topojson::index_type>, rings)
+    (std::vector<std::vector<mapnik::topojson::index_type> >, lines)
     (boost::optional<mapnik::topojson::properties>, props)
     )
 
@@ -101,6 +101,8 @@ BOOST_FUSION_ADAPT_STRUCT(
     (boost::optional<mapnik::topojson::bounding_box>, bbox)
    )
 
+
+
 namespace mapnik { namespace topojson {
 
 namespace qi = boost::spirit::qi;
@@ -128,7 +130,6 @@ topojson_grammar<Iterator, ErrorHandler>::topojson_grammar()
 
     // error handler
     boost::phoenix::function<ErrorHandler> const error_handler;
-
     // generic JSON types
     json.value = json.object | json.array | json.string_ | json.number
         ;
@@ -180,7 +181,7 @@ topojson_grammar<Iterator, ErrorHandler>::topojson_grammar()
         >> lit('{')
         >> -((omit[json.string_]
               >> lit(':')
-              >>  (geometry_collection(_val) | geometry)) % lit(','))
+              >>  (geometry_collection(_val) | geometry[push_back(_val, _1)]) % lit(',')))
         >> lit('}')
         ;
 
@@ -195,17 +196,21 @@ topojson_grammar<Iterator, ErrorHandler>::topojson_grammar()
         ;
 
     geometry_collection =  lit('{')
-               >> lit("\"type\"") >> lit(':') >> lit("\"GeometryCollection\"")
+        >> lit("\"type\"") >> lit(':') >> lit("\"GeometryCollection\"")
-               >> -(lit(',') >> omit[bbox])
+        >> -(lit(',') >> omit[bbox])
-               >> lit(',') >> lit("\"geometries\"") >> lit(':') >> lit('[') >> -(geometry[push_back(_r1, _1)] % lit(','))
+        >> lit(',') >> lit("\"geometries\"") >> lit(':') >> lit('[') >> -(geometry[push_back(_r1, _1)] % lit(','))
-               >> lit(']')
+        >> lit(']')
-               >> lit('}')
+        >> lit('}')
         ;
+
     point = lit('{')
         >> lit("\"type\"") >> lit(':') >> lit("\"Point\"")
         >> -(lit(',') >> omit[bbox])
         >> ((lit(',') >> lit("\"coordinates\"") >> lit(':') >> coordinate)
-            ^ (lit(',') >> properties) /*^ (lit(',') >> omit[id])*/)
+            ^
+            (lit(',') >> properties)
+            ^
+            (lit(',') >> omit[json.key_value]))
         >> lit('}')
         ;
 
@@ -214,23 +219,30 @@ topojson_grammar<Iterator, ErrorHandler>::topojson_grammar()
         >> -(lit(',') >> omit[bbox])
         >> ((lit(',') >> lit("\"coordinates\"") >> lit(':')
              >> lit('[') >> -(coordinate % lit(',')) >> lit(']'))
-            ^ (lit(',') >> properties) ^ (lit(',') >> omit[id]))
+            ^
+            (lit(',') >> properties) ^ (lit(',') >> omit[json.key_value]))
         >> lit('}')
         ;
 
     linestring = lit('{')
         >> lit("\"type\"") >> lit(':') >> lit("\"LineString\"")
-        >> ((lit(',') >> lit("\"arcs\"") >> lit(':') >> lit('[') >> int_ >> lit(']'))
+        >> (lit(',') >> (lit("\"arcs\"") >> lit(':') >> ring)
-            ^ (lit(',') >> properties) ^ (lit(',') >> omit[id]))
+            ^
+            (lit(',') >> properties))
+        //^
+        //  (lit(',') >> omit[json.key_value]))
         >> lit('}')
         ;
 
     multi_linestring = lit('{')
         >> lit("\"type\"") >> lit(':') >> lit("\"MultiLineString\"")
         >> -(lit(',') >> omit[bbox])
-        >> ((lit(',') >> lit("\"arcs\"") >> lit(':') >> lit('[')
+        >> ((lit(',') >> lit("\"arcs\"") >> lit(':')
-             >> -((lit('[') >> int_ >> lit(']')) % lit(',')) >> lit(']'))
+             >> lit('[') >> -(ring % lit(',')) >> lit(']'))
-            ^ (lit(',') >> properties) ^ (lit(',') >> omit[id]))
+            ^
+            (lit(',') >> properties)
+            ^
+            (lit(',') >> omit[json.key_value]))
         >> lit('}')
         ;
 
@@ -239,7 +251,8 @@ topojson_grammar<Iterator, ErrorHandler>::topojson_grammar()
         >> -(lit(',') >> omit[bbox])
         >> ((lit(',') >> lit("\"arcs\"") >> lit(':')
              >> lit('[') >> -(ring % lit(',')) >> lit(']'))
-            ^ (lit(',') >> properties) ^ (lit(',') >> omit[id]))
+            ^ (lit(',') >> properties))
+            //^ (lit(',') >> omit[json.key_value]))
         >> lit('}')
         ;
 
@@ -249,19 +262,16 @@ topojson_grammar<Iterator, ErrorHandler>::topojson_grammar()
         >> ((lit(',') >> lit("\"arcs\"") >> lit(':')
              >> lit('[')
              >> -((lit('[') >> -(ring % lit(',')) >> lit(']')) % lit(','))
-             >> lit(']')) ^ (lit(',') >> properties) ^ (lit(',') >> omit[id]))
+             >> lit(']')) ^ (lit(',') >> properties) ^ (lit(',') >> omit[json.key_value]))
         >> lit('}')
         ;
 
-    id = lit("\"id\"") >> lit(':') >> omit[json.value]
-        ;
-
     ring = lit('[') >> -(int_ % lit(',')) >> lit(']')
         ;
 
     properties = lit("\"properties\"")
         >> lit(':')
-        >> (( lit('{') >> attributes >> lit('}')) | json.object)
+        >> lit('{') >> attributes >> lit('}')
         ;
 
     attributes = (json.string_ >> lit(':') >> json.value) % lit(',')

include/mapnik/json/topojson_utils.hpp

@@ -40,6 +40,12 @@ struct bounding_box_visitor
         : topo_(topo),
           num_arcs_(topo_.arcs.size()) {}
 
+    box2d<double> operator() (mapnik::topojson::empty const&) const
+    {
+        std::cerr << "EMPTY FAIL" << std::endl;
+        return box2d<double>();
+    }
+
     box2d<double> operator() (mapnik::topojson::point const& pt) const
     {
         double x = pt.coord.x;
@@ -82,50 +88,15 @@ struct bounding_box_visitor
     box2d<double> operator() (mapnik::topojson::linestring const& line) const
     {
         box2d<double> bbox;
+        bool first = true;
         if (num_arcs_ > 0)
         {
-            index_type index = line.ring;
+            for (auto index : line.rings)
-            index_type arc_index = index < 0 ? std::abs(index) - 1 : index;
-            if (arc_index >= 0 && arc_index < static_cast<int>(num_arcs_))
-            {
-                bool first = true;
-                double px = 0, py = 0;
-                auto const& arcs = topo_.arcs[arc_index];
-                for (auto pt : arcs.coordinates)
-                {
-                    double x = pt.x;
-                    double y = pt.y;
-                    if (topo_.tr)
-                    {
-                        x =  (px += x) * (*topo_.tr).scale_x + (*topo_.tr).translate_x;
-                        y =  (py += y) * (*topo_.tr).scale_y + (*topo_.tr).translate_y;
-                    }
-                    if (first)
-                    {
-                        first = false;
-                        bbox.init(x, y, x, y);
-                    }
-                    else
-                    {
-                        bbox.expand_to_include(x, y);
-                    }
-                }
-            }
-        }
-        return bbox;
-    }
-
-    box2d<double> operator() (mapnik::topojson::multi_linestring const& multi_line) const
-    {
-        box2d<double> bbox;
-        if (num_arcs_ > 0)
-        {
-            bool first = true;
-            for (auto index : multi_line.rings)
             {
                 index_type arc_index = index < 0 ? std::abs(index) - 1 : index;
                 if (arc_index >= 0 && arc_index < static_cast<int>(num_arcs_))
                 {
+
                     double px = 0, py = 0;
                     auto const& arcs = topo_.arcs[arc_index];
                     for (auto pt : arcs.coordinates)
@@ -153,6 +124,47 @@ struct bounding_box_visitor
         return bbox;
     }
 
+    box2d<double> operator() (mapnik::topojson::multi_linestring const& multi_line) const
+    {
+        box2d<double> bbox;
+        if (num_arcs_ > 0)
+        {
+            bool first = true;
+            for (auto const& line : multi_line.lines)
+            {
+                for (auto index : line)
+                {
+                    index_type arc_index = index < 0 ? std::abs(index) - 1 : index;
+                    if (arc_index >= 0 && arc_index < static_cast<int>(num_arcs_))
+                    {
+                        double px = 0, py = 0;
+                        auto const& arcs = topo_.arcs[arc_index];
+                        for (auto pt : arcs.coordinates)
+                        {
+                            double x = pt.x;
+                            double y = pt.y;
+                            if (topo_.tr)
+                            {
+                                x =  (px += x) * (*topo_.tr).scale_x + (*topo_.tr).translate_x;
+                                y =  (py += y) * (*topo_.tr).scale_y + (*topo_.tr).translate_y;
+                            }
+                            if (first)
+                            {
+                                first = false;
+                                bbox.init(x, y, x, y);
+                            }
+                            else
+                            {
+                                bbox.expand_to_include(x, y);
+                            }
+                        }
+                    }
+                }
+            }
+        }
+        return bbox;
+    }
+
     box2d<double> operator() (mapnik::topojson::polygon const& poly) const
     {
         box2d<double> bbox;
@@ -314,29 +326,31 @@ struct feature_generator
         mapnik::feature_ptr feature(mapnik::feature_factory::create(ctx_,feature_id_));
         if (num_arcs_ > 0)
         {
-            index_type index = line.ring;
+            mapnik::geometry::line_string<double> line_string;
-            index_type arc_index = index < 0 ? std::abs(index) - 1 : index;
-            if (arc_index >= 0 && arc_index < static_cast<int>(num_arcs_))
-            {
-                auto const& arcs = topo_.arcs[arc_index];
-                double px = 0, py = 0;
-                mapnik::geometry::line_string<double> line_string;
-                line_string.reserve(arcs.coordinates.size());
 
-                for (auto pt : arcs.coordinates)
+            for (auto index : line.rings)
+            {
+                index_type arc_index = index < 0 ? std::abs(index) - 1 : index;
+                if (arc_index >= 0 && arc_index < static_cast<int>(num_arcs_))
                 {
-                    double x = pt.x;
+                    auto const& arcs = topo_.arcs[arc_index];
-                    double y = pt.y;
+                    double px = 0, py = 0;
-                    if (topo_.tr)
+                    line_string.reserve(line_string.size() + arcs.coordinates.size());
+                    for (auto pt : arcs.coordinates)
                     {
-                        x =  (px += x) * (*topo_.tr).scale_x + (*topo_.tr).translate_x;
+                        double x = pt.x;
-                        y =  (py += y) * (*topo_.tr).scale_y + (*topo_.tr).translate_y;
+                        double y = pt.y;
+                        if (topo_.tr)
+                        {
+                            x =  (px += x) * (*topo_.tr).scale_x + (*topo_.tr).translate_x;
+                            y =  (py += y) * (*topo_.tr).scale_y + (*topo_.tr).translate_y;
+                        }
+                        line_string.add_coord(x,y);
                     }
-                    line_string.add_coord(x,y);
                 }
-                feature->set_geometry(std::move(line_string));
-                assign_properties(*feature, line, tr_);
             }
+            feature->set_geometry(std::move(line_string));
+            assign_properties(*feature, line, tr_);
         }
         return feature;
     }
@@ -348,30 +362,34 @@ struct feature_generator
         {
             mapnik::geometry::multi_line_string<double> multi_line_string;
             bool hit = false;
-            multi_line_string.reserve(multi_line.rings.size());
+            for (auto const& line : multi_line.lines)
-            for (auto const& index : multi_line.rings)
             {
-                index_type arc_index = index < 0 ? std::abs(index) - 1 : index;
+                multi_line_string.reserve(multi_line_string.size() + line.size());
-                if (arc_index >= 0 && arc_index < static_cast<int>(num_arcs_))
+                mapnik::geometry::line_string<double> line_string;
+                for (auto index : line)
                 {
-                    hit = true;
+                    index_type arc_index = index < 0 ? std::abs(index) - 1 : index;
-                    double px = 0, py = 0;
+                    if (arc_index >= 0 && arc_index < static_cast<int>(num_arcs_))
-                    mapnik::geometry::line_string<double> line_string;
-                    auto const& arcs = topo_.arcs[arc_index];
-                    line_string.reserve(arcs.coordinates.size());
-                    for (auto pt : arcs.coordinates)
                     {
-                        double x = pt.x;
+                        hit = true;
-                        double y = pt.y;
+                        double px = 0, py = 0;
-                        if (topo_.tr)
+                        auto const& arcs = topo_.arcs[arc_index];
+                        line_string.reserve(line_string.size() + arcs.coordinates.size());
+                        for (auto pt : arcs.coordinates)
                         {
-                            x =  (px += x) * (*topo_.tr).scale_x + (*topo_.tr).translate_x;
+                            double x = pt.x;
-                            y =  (py += y) * (*topo_.tr).scale_y + (*topo_.tr).translate_y;
+                            double y = pt.y;
+                            if (topo_.tr)
+                            {
+                                x =  (px += x) * (*topo_.tr).scale_x + (*topo_.tr).translate_x;
+                                y =  (py += y) * (*topo_.tr).scale_y + (*topo_.tr).translate_y;
+                            }
+                            line_string.add_coord(x, y);
                         }
-                        line_string.add_coord(x, y);
+
                     }
-                    multi_line_string.push_back(std::move(line_string));
                 }
+                multi_line_string.push_back(std::move(line_string));
             }
             if (hit)
             {

include/mapnik/json/topology.hpp

@@ -62,13 +62,13 @@ struct multi_point
 
 struct linestring
 {
-    index_type ring ;
+    std::vector<index_type> rings ;
     boost::optional<properties> props;
 };
 
 struct multi_linestring
 {
-    std::vector<index_type> rings;
+    std::vector<std::vector<index_type> > lines;
     boost::optional<properties> props;
 };
 
@@ -84,7 +84,10 @@ struct multi_polygon
     boost::optional<properties> props;
 };
 
-using geometry =  util::variant<point,
+struct empty {};
+
+using geometry =  util::variant<empty,
+                                point,
                                 linestring,
                                 polygon,
                                 multi_point,

plugins/input/topojson/topojson_datasource.cpp

@@ -66,7 +66,6 @@ struct attr_value_converter
     {
         return mapnik::Boolean;
     }
-
     // string, object, array
     template <typename T>
     mapnik::eAttributeType operator() (T const& /*val*/) const
@@ -101,6 +100,11 @@ struct geometry_type_visitor
     {
         return static_cast<int>(mapnik::datasource_geometry_t::Polygon);
     }
+    template <typename T>
+    int operator() (T const& ) const
+    {
+        return 0;
+    }
 };
 
 struct collect_attributes_visitor
@@ -109,6 +113,9 @@ struct collect_attributes_visitor
     collect_attributes_visitor(mapnik::layer_descriptor & desc):
       desc_(desc) {}
 
+    // no-op
+    void operator() (mapnik::topojson::empty) {}
+    //
     template <typename GeomType>
     void operator() (GeomType const& g)
     {

test/data

@@ -1 +1 @@
-Subproject commit 7657658330c7b95f2d431fa199de8b7629b41fab
+Subproject commit b2ec613fb5fba85e6f810dabe2436c9e5b2d90b0

test/unit/datasource/ds_test_util.hpp

@@ -110,6 +110,7 @@ using attr = std::tuple<std::string, mapnik::value>;
 #define REQUIRE_ATTRIBUTES(feature, attrs) \
     REQUIRE(bool(feature)); \
     for (auto const &kv : attrs) { \
+        std::cerr << std::get<0>(kv) << std::endl; \
         REQUIRE(feature->has_key(std::get<0>(kv))); \
         CHECK(feature->get(std::get<0>(kv)) == std::get<1>(kv)); \
     } \

test/unit/datasource/topojson.cpp

@@ -21,6 +21,7 @@
  *****************************************************************************/
 
 #include "catch.hpp"
+#include "ds_test_util.hpp"
 
 #include <mapnik/util/fs.hpp>
 #include <mapnik/util/file_io.hpp>
@@ -59,6 +60,7 @@ TEST_CASE("topology")
         mapnik::transcoder tr("utf8");
         for (auto const& path : mapnik::util::list_directory("test/data/topojson/"))
         {
+            std::cerr << path << std::endl;
             mapnik::topojson::topology topo;
             REQUIRE(parse_topology(path, topo));
             for (auto const& geom : topo.geometries)
@@ -72,4 +74,42 @@ TEST_CASE("topology")
             }
         }
     }
+
+    SECTION("TopoJSON properties are properly expressed")
+    {
+        std::string filename("./test/data/topojson/escaped.topojson");
+        mapnik::context_ptr ctx = std::make_shared<mapnik::context_type>();
+        mapnik::transcoder tr("utf8");
+        mapnik::topojson::topology topo;
+        REQUIRE(parse_topology(filename, topo));
+        mapnik::value_integer feature_id = 0;
+        for (auto const& geom : topo.geometries)
+        {
+            mapnik::box2d<double> bbox = mapnik::util::apply_visitor(mapnik::topojson::bounding_box_visitor(topo), geom);
+            CHECK(bbox.valid());
+            mapnik::topojson::feature_generator<mapnik::context_ptr> visitor(ctx, tr, topo, feature_id);
+            mapnik::feature_ptr feature = mapnik::util::apply_visitor(visitor, geom);
+            CHECK(feature);
+            CHECK(feature->envelope() == bbox);
+            std::initializer_list<attr> attrs = {
+                attr{"name", mapnik::value_unicode_string("Test")},
+                attr{"NOM_FR", mapnik::value_unicode_string("Québec")},
+                attr{"boolean", mapnik::value_bool("true")},
+                attr{"description", mapnik::value_unicode_string("Test: \u005C")},
+                attr{"double", mapnik::value_double(1.1)},
+                attr{"int", mapnik::value_integer(1)},
+                attr{"object", mapnik::value_unicode_string("{name:\"waka\",spaces:\"value with spaces\",int:1,double:1.1,boolean:false"
+                                                            ",NOM_FR:\"Québec\",array:[\"string\",\"value with spaces\",3,1.1,null,true"
+                                                            ",\"Québec\"],another_object:{name:\"nested object\"}}")},
+                attr{"spaces", mapnik::value_unicode_string("this has spaces")},
+                attr{"array", mapnik::value_unicode_string("[\"string\",\"value with spaces\",3,1.1,null,true,"
+                                                           "\"Québec\",{name:\"object within an array\"},"
+                                                           "[\"array\",\"within\",\"an\",\"array\"]]")},
+                attr{"empty_array", mapnik::value_unicode_string("[]")},
+                attr{"empty_object", mapnik::value_unicode_string("{}")},
+            };
+            REQUIRE_ATTRIBUTES(feature, attrs);
+        }
+    }
+
 }