From c053682711027793df8eaa55178e94e38e5d05a1 Mon Sep 17 00:00:00 2001 From: artemp Date: Tue, 20 Oct 2015 11:12:11 +0100 Subject: [PATCH 1/9] geojson.input - assign incremental feature_id in geojson_index_featureset and geojson_memory_index_featureset (ref #3134) --- plugins/input/geojson/geojson_index_featureset.cpp | 2 +- plugins/input/geojson/geojson_index_featureset.hpp | 1 + plugins/input/geojson/geojson_memory_index_featureset.cpp | 2 +- plugins/input/geojson/geojson_memory_index_featureset.hpp | 2 +- 4 files changed, 4 insertions(+), 3 deletions(-) diff --git a/plugins/input/geojson/geojson_index_featureset.cpp b/plugins/input/geojson/geojson_index_featureset.cpp index f5d4249fb..285691607 100644 --- a/plugins/input/geojson/geojson_index_featureset.cpp +++ b/plugins/input/geojson/geojson_index_featureset.cpp @@ -93,7 +93,7 @@ mapnik::feature_ptr geojson_index_featureset::next() static const mapnik::json::feature_grammar grammar(tr); using namespace boost::spirit; standard::space_type space; - mapnik::feature_ptr feature(mapnik::feature_factory::create(ctx_,1)); + mapnik::feature_ptr feature(mapnik::feature_factory::create(ctx_, feature_id_++)); if (!qi::phrase_parse(start, end, (grammar)(boost::phoenix::ref(*feature)), space) || start != end) { throw std::runtime_error("Failed to parse geojson feature"); diff --git a/plugins/input/geojson/geojson_index_featureset.hpp b/plugins/input/geojson/geojson_index_featureset.hpp index e683df28d..ea7b9127d 100644 --- a/plugins/input/geojson/geojson_index_featureset.hpp +++ b/plugins/input/geojson/geojson_index_featureset.hpp @@ -56,6 +56,7 @@ private: using file_ptr = std::unique_ptr; file_ptr file_; #endif + std::size_t feature_id_ = 1; mapnik::context_ptr ctx_; std::vector positions_; std::vector::iterator itr_; diff --git a/plugins/input/geojson/geojson_memory_index_featureset.cpp b/plugins/input/geojson/geojson_memory_index_featureset.cpp index 14a1a6c14..99a79ff39 100644 --- a/plugins/input/geojson/geojson_memory_index_featureset.cpp +++ b/plugins/input/geojson/geojson_memory_index_featureset.cpp @@ -69,7 +69,7 @@ mapnik::feature_ptr geojson_memory_index_featureset::next() static const mapnik::json::feature_grammar grammar(tr); using namespace boost::spirit; standard::space_type space; - mapnik::feature_ptr feature(mapnik::feature_factory::create(ctx_,1)); + mapnik::feature_ptr feature(mapnik::feature_factory::create(ctx_, feature_id_++)); if (!qi::phrase_parse(start, end, (grammar)(boost::phoenix::ref(*feature)), space) || start != end) { throw std::runtime_error("Failed to parse geojson feature"); diff --git a/plugins/input/geojson/geojson_memory_index_featureset.hpp b/plugins/input/geojson/geojson_memory_index_featureset.hpp index 154460c58..25f4a2f3c 100644 --- a/plugins/input/geojson/geojson_memory_index_featureset.hpp +++ b/plugins/input/geojson/geojson_memory_index_featureset.hpp @@ -42,7 +42,7 @@ public: private: file_ptr file_; - + std::size_t feature_id_ = 1; const array_type index_array_; array_type::const_iterator index_itr_; array_type::const_iterator index_end_; From daaf2ee9d586d02c3112c422e3fc651227217917 Mon Sep 17 00:00:00 2001 From: artemp Date: Tue, 20 Oct 2015 11:22:42 +0100 Subject: [PATCH 2/9] unit test (geojson) - de-dupe code --- test/unit/datasource/geojson.cpp | 120 ++++--------------------------- 1 file changed, 14 insertions(+), 106 deletions(-) diff --git a/test/unit/datasource/geojson.cpp b/test/unit/datasource/geojson.cpp index 86c3a09a4..1732abd5f 100644 --- a/test/unit/datasource/geojson.cpp +++ b/test/unit/datasource/geojson.cpp @@ -73,20 +73,9 @@ TEST_CASE("geojson") { { SECTION("GeoJSON Point") { + for (auto cache_features : {true, false}) { - // cache features in memory - auto feature = detail::fetch_first_feature("./test/data/json/point.json", true); - // test - auto const& geometry = feature->get_geometry(); - REQUIRE(mapnik::geometry::geometry_type(geometry) == mapnik::geometry::Point); - auto const& pt = mapnik::util::get >(geometry); - REQUIRE(pt.x == 100); - REQUIRE(pt.y == 0); - } - { - // on-fly in-memory r-tree index + read features from disk - auto feature = detail::fetch_first_feature("./test/data/json/point.json", false); - // test + auto feature = detail::fetch_first_feature("./test/data/json/point.json", cache_features); auto const& geometry = feature->get_geometry(); REQUIRE(mapnik::geometry::geometry_type(geometry) == mapnik::geometry::Point); auto const& pt = mapnik::util::get >(geometry); @@ -97,14 +86,9 @@ TEST_CASE("geojson") { SECTION("GeoJSON LineString") { - mapnik::parameters params; - params["type"] = "geojson"; - params["file"] = "./test/data/json/linestring.json"; - + for (auto cache_features : {true, false}) { - // cache features in memory - auto feature = detail::fetch_first_feature("./test/data/json/linestring.json", true); - // test + auto feature = detail::fetch_first_feature("./test/data/json/linestring.json", cache_features); auto const& geometry = feature->get_geometry(); REQUIRE(mapnik::geometry::geometry_type(geometry) == mapnik::geometry::LineString); auto const& line = mapnik::util::get >(geometry); @@ -112,28 +96,13 @@ TEST_CASE("geojson") { REQUIRE(mapnik::geometry::envelope(line) == mapnik::box2d(100,0,101,1)); } - { - // on-fly in-memory r-tree index + read features from disk - auto feature = detail::fetch_first_feature("./test/data/json/linestring.json", false); - // test - auto const& geometry = feature->get_geometry(); - REQUIRE(mapnik::geometry::geometry_type(geometry) == mapnik::geometry::LineString); - auto const& line = mapnik::util::get >(geometry); - REQUIRE(line.size() == 2); - REQUIRE(mapnik::geometry::envelope(line) == mapnik::box2d(100,0,101,1)); - } } SECTION("GeoJSON Polygon") { - mapnik::parameters params; - params["type"] = "geojson"; - params["file"] = "./test/data/json/polygon.json"; - + for (auto cache_features : {true, false}) { - // cache features in memory - auto feature = detail::fetch_first_feature("./test/data/json/polygon.json", true); - // test + auto feature = detail::fetch_first_feature("./test/data/json/polygon.json", cache_features); auto const& geometry = feature->get_geometry(); REQUIRE(mapnik::geometry::geometry_type(geometry) == mapnik::geometry::Polygon); auto const& poly = mapnik::util::get >(geometry); @@ -144,37 +113,13 @@ TEST_CASE("geojson") { REQUIRE(mapnik::geometry::envelope(poly) == mapnik::box2d(100,0,101,1)); } - { - // on-fly in-memory r-tree index + read features from disk - auto feature = detail::fetch_first_feature("./test/data/json/polygon.json", false); - // test - auto const& geometry = feature->get_geometry(); - REQUIRE(mapnik::geometry::geometry_type(geometry) == mapnik::geometry::Polygon); - auto const& poly = mapnik::util::get >(geometry); - REQUIRE(poly.num_rings() == 2); - REQUIRE(poly.exterior_ring.size() == 5); - REQUIRE(poly.interior_rings.size() == 1); - REQUIRE(poly.interior_rings[0].size() == 5); - REQUIRE(mapnik::geometry::envelope(poly) == mapnik::box2d(100,0,101,1)); - } } SECTION("GeoJSON MultiPoint") { + for (auto cache_features : {true, false}) { - // cache features in memory - auto feature = detail::fetch_first_feature("./test/data/json/multipoint.json", true); - // test - auto const& geometry = feature->get_geometry(); - REQUIRE(mapnik::geometry::geometry_type(geometry) == mapnik::geometry::MultiPoint); - auto const& multi_pt = mapnik::util::get >(geometry); - REQUIRE(multi_pt.size() == 2); - REQUIRE(mapnik::geometry::envelope(multi_pt) == mapnik::box2d(100,0,101,1)); - } - { - // on-fly in-memory r-tree index + read features from disk - auto feature = detail::fetch_first_feature("./test/data/json/multipoint.json", false); - // test + auto feature = detail::fetch_first_feature("./test/data/json/multipoint.json", cache_features); auto const& geometry = feature->get_geometry(); REQUIRE(mapnik::geometry::geometry_type(geometry) == mapnik::geometry::MultiPoint); auto const& multi_pt = mapnik::util::get >(geometry); @@ -185,10 +130,9 @@ TEST_CASE("geojson") { SECTION("GeoJSON MultiLineString") { + for (auto cache_features : {true, false}) { - // cache features in memory - auto feature = detail::fetch_first_feature("./test/data/json/multilinestring.json", true); - // test + auto feature = detail::fetch_first_feature("./test/data/json/multilinestring.json", cache_features); auto const& geometry = feature->get_geometry(); REQUIRE(mapnik::geometry::geometry_type(geometry) == mapnik::geometry::MultiLineString); auto const& multi_line = mapnik::util::get >(geometry); @@ -198,25 +142,13 @@ TEST_CASE("geojson") { REQUIRE(mapnik::geometry::envelope(multi_line) == mapnik::box2d(100,0,103,3)); } - { - // on-fly in-memory r-tree index + read features from disk - auto feature = detail::fetch_first_feature("./test/data/json/multilinestring.json", false); - // test - auto const& geometry = feature->get_geometry(); - REQUIRE(mapnik::geometry::geometry_type(geometry) == mapnik::geometry::MultiLineString); - auto const& multi_line = mapnik::util::get >(geometry); - REQUIRE(multi_line.size() == 2); - REQUIRE(multi_line[0].size() == 2); - REQUIRE(multi_line[1].size() == 2); - REQUIRE(mapnik::geometry::envelope(multi_line) == mapnik::box2d(100,0,103,3)); - } } SECTION("GeoJSON MultiPolygon") { + for (auto cache_features : {true, false}) { - // cache features in memory - auto feature = detail::fetch_first_feature("./test/data/json/multipolygon.json", true); + auto feature = detail::fetch_first_feature("./test/data/json/multipolygon.json", cache_features); // test auto const& geometry = feature->get_geometry(); REQUIRE(mapnik::geometry::geometry_type(geometry) == mapnik::geometry::MultiPolygon); @@ -227,37 +159,13 @@ TEST_CASE("geojson") { REQUIRE(mapnik::geometry::envelope(multi_poly) == mapnik::box2d(100,0,103,3)); } - { - // on-fly in-memory r-tree index + read features from disk - auto feature = detail::fetch_first_feature("./test/data/json/multipolygon.json", false); - // test - auto const& geometry = feature->get_geometry(); - REQUIRE(mapnik::geometry::geometry_type(geometry) == mapnik::geometry::MultiPolygon); - auto const& multi_poly = mapnik::util::get >(geometry); - REQUIRE(multi_poly.size() == 2); - REQUIRE(multi_poly[0].num_rings() == 1); - REQUIRE(multi_poly[1].num_rings() == 2); - REQUIRE(mapnik::geometry::envelope(multi_poly) == mapnik::box2d(100,0,103,3)); - } } SECTION("GeoJSON GeometryCollection") { + for (auto cache_features : {true, false}) { - // cache features in memory - auto feature = detail::fetch_first_feature("./test/data/json/geometrycollection.json", true); - // test - auto const& geometry = feature->get_geometry(); - REQUIRE(mapnik::geometry::geometry_type(geometry) == mapnik::geometry::GeometryCollection); - auto const& collection = mapnik::util::get >(geometry); - REQUIRE(collection.size() == 2); - REQUIRE(mapnik::geometry::geometry_type(collection[0]) == mapnik::geometry::Point); - REQUIRE(mapnik::geometry::geometry_type(collection[1]) == mapnik::geometry::LineString); - REQUIRE(mapnik::geometry::envelope(collection) == mapnik::box2d(100,0,102,1)); - } - { - // cache features in memory - auto feature = detail::fetch_first_feature("./test/data/json/geometrycollection.json", false); + auto feature = detail::fetch_first_feature("./test/data/json/geometrycollection.json", cache_features); // test auto const& geometry = feature->get_geometry(); REQUIRE(mapnik::geometry::geometry_type(geometry) == mapnik::geometry::GeometryCollection); From c952db979e7a98ee86805149edf352ebfab95f0d Mon Sep 17 00:00:00 2001 From: artemp Date: Tue, 20 Oct 2015 11:45:39 +0100 Subject: [PATCH 3/9] use mapnik::value_integer for feature id's --- plugins/input/geojson/geojson_index_featureset.hpp | 2 +- plugins/input/geojson/geojson_memory_index_featureset.hpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/plugins/input/geojson/geojson_index_featureset.hpp b/plugins/input/geojson/geojson_index_featureset.hpp index ea7b9127d..2cd934c25 100644 --- a/plugins/input/geojson/geojson_index_featureset.hpp +++ b/plugins/input/geojson/geojson_index_featureset.hpp @@ -56,7 +56,7 @@ private: using file_ptr = std::unique_ptr; file_ptr file_; #endif - std::size_t feature_id_ = 1; + mapnik::value_integer feature_id_ = 1; mapnik::context_ptr ctx_; std::vector positions_; std::vector::iterator itr_; diff --git a/plugins/input/geojson/geojson_memory_index_featureset.hpp b/plugins/input/geojson/geojson_memory_index_featureset.hpp index 25f4a2f3c..a29751d67 100644 --- a/plugins/input/geojson/geojson_memory_index_featureset.hpp +++ b/plugins/input/geojson/geojson_memory_index_featureset.hpp @@ -42,7 +42,7 @@ public: private: file_ptr file_; - std::size_t feature_id_ = 1; + mapnik::value_integer feature_id_ = 1; const array_type index_array_; array_type::const_iterator index_itr_; array_type::const_iterator index_end_; From c4ec93a5f25976838cc2c1b892993ece39863ab4 Mon Sep 17 00:00:00 2001 From: artemp Date: Tue, 20 Oct 2015 11:46:39 +0100 Subject: [PATCH 4/9] geojson unit test - run 'FeatureCollection' test using all permutations of disk_index and cache_features settings. --- test/unit/datasource/geojson.cpp | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/test/unit/datasource/geojson.cpp b/test/unit/datasource/geojson.cpp index 1732abd5f..dbe482a8b 100644 --- a/test/unit/datasource/geojson.cpp +++ b/test/unit/datasource/geojson.cpp @@ -177,16 +177,26 @@ TEST_CASE("geojson") { } } - SECTION("GeoJSON FeatureCollection *.index") + SECTION("GeoJSON FeatureCollection") { std::string filename("./test/data/json/featurecollection.json"); - if (detail::create_disk_index(filename, true) == 0) + + for (auto create_index : { true, false }) { - if (mapnik::util::exists(filename + ".index")) + if (create_index) { - mapnik::parameters params; - params["type"] = "geojson"; - params["file"] = filename; + REQUIRE(detail::create_disk_index(filename, true) == 0); + CHECK(mapnik::util::exists(filename + ".index")); + } + + mapnik::parameters params; + params["type"] = "geojson"; + params["file"] = filename; + + for (auto cache_features : {true, false}) + { + params["cache_features"] = cache_features; + auto ds = mapnik::datasource_cache::instance().create(params); auto fields = ds->get_descriptor().get_descriptors(); mapnik::query query(ds->envelope()); @@ -197,7 +207,7 @@ TEST_CASE("geojson") { auto features = ds->features(query); auto bounding_box = ds->envelope(); mapnik::box2d bbox; - std::size_t count = 0; + mapnik::value_integer count = 0; while (true) { auto feature = features->next(); @@ -205,9 +215,13 @@ TEST_CASE("geojson") { if (!bbox.valid()) bbox = feature->envelope(); else bbox.expand_to_include(feature->envelope()); ++count; + REQUIRE(feature->id() == count); } REQUIRE(count == 3); REQUIRE(bounding_box == bbox); + } + if (mapnik::util::exists(filename + ".index")) + { CHECK(mapnik::util::remove(filename + ".index")); } } From aa537e6254e4672159f02537c8a597489c16dd26 Mon Sep 17 00:00:00 2001 From: artemp Date: Tue, 20 Oct 2015 12:04:32 +0100 Subject: [PATCH 5/9] geojson unit test - further simplify and increase coverage --- test/unit/datasource/geojson.cpp | 128 ++++++++++++------------------- 1 file changed, 48 insertions(+), 80 deletions(-) diff --git a/test/unit/datasource/geojson.cpp b/test/unit/datasource/geojson.cpp index dbe482a8b..d1d2e9f15 100644 --- a/test/unit/datasource/geojson.cpp +++ b/test/unit/datasource/geojson.cpp @@ -177,6 +177,30 @@ TEST_CASE("geojson") { } } + SECTION("GeoJSON Feature") + { + // Create datasource + mapnik::parameters params; + params["type"] = "geojson"; + params["file"] = "./test/data/json/feature.json"; + for (auto cache_features : {true, false}) + { + params["cache-features"] = cache_features; + auto ds = mapnik::datasource_cache::instance().create(params); + REQUIRE(bool(ds)); + auto fields = ds->get_descriptor().get_descriptors(); + mapnik::query query(ds->envelope()); + for (auto const& field : fields) + { + query.add_property_name(field.get_name()); + } + auto features = ds->features(query); + REQUIRE(features != nullptr); + auto feature = features->next(); + REQUIRE(feature != nullptr); + } + } + SECTION("GeoJSON FeatureCollection") { std::string filename("./test/data/json/featurecollection.json"); @@ -227,99 +251,43 @@ TEST_CASE("geojson") { } } - SECTION("json feature cache-feature=\"true\"") - { - // Create datasource - mapnik::parameters params; - params["type"] = "geojson"; - params["file"] = "./test/data/json/feature.json"; - params["cache-features"] = true; - auto ds = mapnik::datasource_cache::instance().create(params); - REQUIRE(bool(ds)); - auto fields = ds->get_descriptor().get_descriptors(); - mapnik::query query(ds->envelope()); - for (auto const& field : fields) - { - query.add_property_name(field.get_name()); - } - auto features = ds->features(query); - REQUIRE(features != nullptr); - auto feature = features->next(); - REQUIRE(feature != nullptr); - } - - SECTION("json feature cache-feature=\"false\"") - { - mapnik::parameters params; - params["type"] = "geojson"; - params["file"] = "./test/data/json/feature.json"; - params["cache-features"] = false; - auto ds = mapnik::datasource_cache::instance().create(params); - REQUIRE(bool(ds)); - auto fields = ds->get_descriptor().get_descriptors(); - mapnik::query query(ds->envelope()); - for (auto const& field : fields) - { - query.add_property_name(field.get_name()); - } - auto features = ds->features(query); - REQUIRE(features != nullptr); - auto feature = features->next(); - REQUIRE(feature != nullptr); - } - - SECTION("json extra properties cache-feature=\"true\"") + SECTION("GeoJSON extra properties") { // Create datasource mapnik::parameters params; params["type"] = "geojson"; params["file"] = "./test/data/json/feature_collection_extra_properties.json"; - params["cache-features"] = true; - auto ds = mapnik::datasource_cache::instance().create(params); - REQUIRE(bool(ds)); - auto fields = ds->get_descriptor().get_descriptors(); - mapnik::query query(ds->envelope()); - for (auto const& field : fields) + for (auto cache_features : {true, false}) { - query.add_property_name(field.get_name()); + params["cache-features"] = cache_features; + auto ds = mapnik::datasource_cache::instance().create(params); + REQUIRE(bool(ds)); + auto fields = ds->get_descriptor().get_descriptors(); + mapnik::query query(ds->envelope()); + for (auto const& field : fields) + { + query.add_property_name(field.get_name()); + } + auto features = ds->features(query); + REQUIRE(features != nullptr); + auto feature = features->next(); + REQUIRE(feature != nullptr); + REQUIRE(feature->envelope() == mapnik::box2d(123,456,123,456)); } - auto features = ds->features(query); - REQUIRE(features != nullptr); - auto feature = features->next(); - REQUIRE(feature != nullptr); - REQUIRE(feature->envelope() == mapnik::box2d(123,456,123,456)); } - SECTION("json extra properties cache-feature=\"false\"") - { - // Create datasource - mapnik::parameters params; - params["type"] = "geojson"; - params["file"] = "./test/data/json/feature_collection_extra_properties.json"; - params["cache-features"] = false; - auto ds = mapnik::datasource_cache::instance().create(params); - REQUIRE(bool(ds)); - auto fields = ds->get_descriptor().get_descriptors(); - mapnik::query query(ds->envelope()); - for (auto const& field : fields) - { - query.add_property_name(field.get_name()); - } - auto features = ds->features(query); - REQUIRE(features != nullptr); - auto feature = features->next(); - REQUIRE(feature != nullptr); - REQUIRE(feature->envelope() == mapnik::box2d(123,456,123,456)); - } - SECTION("json - ensure input fully consumed and throw exception otherwise") + SECTION("GeoJSON ensure input fully consumed and throw exception otherwise") { mapnik::parameters params; params["type"] = "geojson"; params["file"] = "./test/data/json/points-malformed.geojson"; // mismatched parentheses - params["cache-features"] = false; - REQUIRE_THROWS(mapnik::datasource_cache::instance().create(params)); - params["cache-features"] = true; - REQUIRE_THROWS(mapnik::datasource_cache::instance().create(params)); + for (auto cache_features : {true, false}) + { + params["cache-features"] = cache_features; + REQUIRE_THROWS(mapnik::datasource_cache::instance().create(params)); + params["cache-features"] = true; + REQUIRE_THROWS(mapnik::datasource_cache::instance().create(params)); + } } } } From 4822477d40bd2302eb5f551b1b949543fdfecb32 Mon Sep 17 00:00:00 2001 From: artemp Date: Tue, 20 Oct 2015 12:44:45 +0100 Subject: [PATCH 6/9] don't rely on std::system return value (implementation defined) --- test/unit/datasource/geojson.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/datasource/geojson.cpp b/test/unit/datasource/geojson.cpp index d1d2e9f15..31e07b0f7 100644 --- a/test/unit/datasource/geojson.cpp +++ b/test/unit/datasource/geojson.cpp @@ -209,7 +209,7 @@ TEST_CASE("geojson") { { if (create_index) { - REQUIRE(detail::create_disk_index(filename, true) == 0); + detail::create_disk_index(filename, true); CHECK(mapnik::util::exists(filename + ".index")); } From 5de3a3776d6681113a1dcfd631b132adbbf68dc0 Mon Sep 17 00:00:00 2001 From: artemp Date: Tue, 20 Oct 2015 13:50:54 +0100 Subject: [PATCH 7/9] travis - std::system() calls fails --- test/unit/datasource/geojson.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/test/unit/datasource/geojson.cpp b/test/unit/datasource/geojson.cpp index 31e07b0f7..6fec8483d 100644 --- a/test/unit/datasource/geojson.cpp +++ b/test/unit/datasource/geojson.cpp @@ -210,7 +210,6 @@ TEST_CASE("geojson") { if (create_index) { detail::create_disk_index(filename, true); - CHECK(mapnik::util::exists(filename + ".index")); } mapnik::parameters params; From ab2d86c617c2cdb8d523973d518caf4bf6014f61 Mon Sep 17 00:00:00 2001 From: artemp Date: Tue, 20 Oct 2015 14:18:17 +0100 Subject: [PATCH 8/9] shape.input: remove set_id() method as it's no longer required ref #1020 #1019 --- include/mapnik/feature.hpp | 4 +--- plugins/input/shape/shape_featureset.cpp | 7 +++---- plugins/input/shape/shape_index_featureset.cpp | 11 ++++------- plugins/input/shape/shape_io.hpp | 1 + 4 files changed, 9 insertions(+), 14 deletions(-) diff --git a/include/mapnik/feature.hpp b/include/mapnik/feature.hpp index fff06a232..42bd10bdb 100644 --- a/include/mapnik/feature.hpp +++ b/include/mapnik/feature.hpp @@ -110,8 +110,6 @@ public: inline mapnik::value_integer id() const { return id_;} - inline void set_id(mapnik::value_integer id) { id_ = id;} - template inline void put(context_type::key_type const& key, T const& val) { @@ -259,7 +257,7 @@ public: } private: - mapnik::value_integer id_; + const mapnik::value_integer id_; context_ptr ctx_; cont_type data_; geometry::geometry geom_; diff --git a/plugins/input/shape/shape_featureset.cpp b/plugins/input/shape/shape_featureset.cpp index 4cf9dbf0e..44f58ce84 100644 --- a/plugins/input/shape/shape_featureset.cpp +++ b/plugins/input/shape/shape_featureset.cpp @@ -69,7 +69,8 @@ feature_ptr shape_featureset::next() { int offset = shape_.shx().read_xdr_integer(); int record_length = shape_.shx().read_xdr_integer(); - shape_.move_to(2*offset); + shape_.move_to(2 * offset); + mapnik::value_integer feature_id = shape_.id(); assert(record_length == shape_.reclength_); shape_file::record_type record(record_length * 2); shape_.shp().read_record(record); @@ -78,7 +79,7 @@ feature_ptr shape_featureset::next() // skip null shapes if (type == shape_io::shape_null) continue; - feature_ptr feature(feature_factory::create(ctx_, shape_.id_)); + feature_ptr feature(feature_factory::create(ctx_, feature_id)); switch (type) { case shape_io::shape_point: @@ -133,8 +134,6 @@ feature_ptr shape_featureset::next() return feature_ptr(); } - // FIXME: https://github.com/mapnik/mapnik/issues/1020 - feature->set_id(shape_.id_); if (attr_ids_.size()) { shape_.dbf().move_to(shape_.id_); diff --git a/plugins/input/shape/shape_index_featureset.cpp b/plugins/input/shape/shape_index_featureset.cpp index 32f261979..e352efd48 100644 --- a/plugins/input/shape/shape_index_featureset.cpp +++ b/plugins/input/shape/shape_index_featureset.cpp @@ -85,10 +85,11 @@ feature_ptr shape_index_featureset::next() while ( itr_ != offsets_.end()) { shape_ptr_->move_to(*itr_++); + mapnik::value_integer feature_id = shape_ptr_->id(); shape_file::record_type record(shape_ptr_->reclength_ * 2); shape_ptr_->shp().read_record(record); int type = record.read_ndr_integer(); - feature_ptr feature(feature_factory::create(ctx_,shape_ptr_->id_)); + feature_ptr feature(feature_factory::create(ctx_, feature_id)); switch (type) { @@ -141,18 +142,14 @@ feature_ptr shape_index_featureset::next() return feature_ptr(); } - // FIXME: https://github.com/mapnik/mapnik/issues/1020 - feature->set_id(shape_ptr_->id_); if (attr_ids_.size()) { shape_ptr_->dbf().move_to(shape_ptr_->id_); - std::vector::const_iterator itr = attr_ids_.begin(); - std::vector::const_iterator end = attr_ids_.end(); try { - for (; itr!=end; ++itr) + for (auto id : attr_ids_) { - shape_ptr_->dbf().add_attribute(*itr, *tr_, *feature); + shape_ptr_->dbf().add_attribute(id, *tr_, *feature); } } catch (...) diff --git a/plugins/input/shape/shape_io.hpp b/plugins/input/shape/shape_io.hpp index cc8f2584c..ab28f9999 100644 --- a/plugins/input/shape/shape_io.hpp +++ b/plugins/input/shape/shape_io.hpp @@ -75,6 +75,7 @@ public: return (index_ && index_->is_open()); } + inline int id() const { return id_;} void move_to(std::streampos pos); static void read_bbox(shape_file::record_type & record, mapnik::box2d & bbox); static mapnik::geometry::geometry read_polyline(shape_file::record_type & record); From bfbc72c4846309ecaa78ff917b7d31fa1a7201c2 Mon Sep 17 00:00:00 2001 From: artemp Date: Tue, 20 Oct 2015 14:36:19 +0100 Subject: [PATCH 9/9] feature : put back set_id() for upstream compatibility --- include/mapnik/feature.hpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/mapnik/feature.hpp b/include/mapnik/feature.hpp index 42bd10bdb..769ca0579 100644 --- a/include/mapnik/feature.hpp +++ b/include/mapnik/feature.hpp @@ -109,7 +109,7 @@ public: raster_() {} inline mapnik::value_integer id() const { return id_;} - + inline void set_id(mapnik::value_integer id) { id_ = id;} template inline void put(context_type::key_type const& key, T const& val) { @@ -257,7 +257,7 @@ public: } private: - const mapnik::value_integer id_; + mapnik::value_integer id_; context_ptr ctx_; cont_type data_; geometry::geometry geom_;