From 2c2e8589c341e2940202b781f7eb539b82b3599d Mon Sep 17 00:00:00 2001 From: Mikhail Pozdnyakov Date: Wed, 24 Jul 2019 14:52:40 +0300 Subject: [core] GeometryCollection must not be implicitly copied --- src/mbgl/layout/pattern_layout.hpp | 2 +- src/mbgl/layout/symbol_feature.hpp | 2 +- src/mbgl/tile/geometry_tile_data.cpp | 23 +++++++++--------- src/mbgl/tile/geometry_tile_data.hpp | 7 ++++++ src/mbgl/tile/geometry_tile_worker.cpp | 2 +- test/gl/bucket.test.cpp | 2 +- test/util/merge_lines.test.cpp | 44 +++++++++++++++++----------------- 7 files changed, 44 insertions(+), 38 deletions(-) diff --git a/src/mbgl/layout/pattern_layout.hpp b/src/mbgl/layout/pattern_layout.hpp index 95f8b181fc..858d515347 100644 --- a/src/mbgl/layout/pattern_layout.hpp +++ b/src/mbgl/layout/pattern_layout.hpp @@ -106,7 +106,7 @@ public: const auto i = patternFeature.i; std::unique_ptr feature = std::move(patternFeature.feature); const PatternLayerMap& patterns = patternFeature.patterns; - GeometryCollection geometries = feature->getGeometries(); + const GeometryCollection& geometries = feature->getGeometries(); bucket->addFeature(*feature, geometries, patternPositions, patterns); featureIndex->insert(geometries, i, sourceLayerID, bucketLeaderID); diff --git a/src/mbgl/layout/symbol_feature.hpp b/src/mbgl/layout/symbol_feature.hpp index 2c952fc12a..72e88fb55e 100644 --- a/src/mbgl/layout/symbol_feature.hpp +++ b/src/mbgl/layout/symbol_feature.hpp @@ -13,7 +13,7 @@ class SymbolFeature : public GeometryTileFeature { public: SymbolFeature(std::unique_ptr feature_) : feature(std::move(feature_)), - geometry(feature->getGeometries()) // we need a mutable copy of the geometry for mergeLines() + geometry(feature->getGeometries().clone()) // we need a mutable copy of the geometry for mergeLines() {} FeatureType getType() const override { return feature->getType(); } diff --git a/src/mbgl/tile/geometry_tile_data.cpp b/src/mbgl/tile/geometry_tile_data.cpp index c7aea35944..db9f37ad4b 100644 --- a/src/mbgl/tile/geometry_tile_data.cpp +++ b/src/mbgl/tile/geometry_tile_data.cpp @@ -57,32 +57,31 @@ std::vector classifyRings(const GeometryCollection& rings) { std::size_t len = rings.size(); if (len <= 1) { - polygons.push_back(rings); + polygons.emplace_back(rings.clone()); return polygons; } GeometryCollection polygon; int8_t ccw = 0; - for (std::size_t i = 0; i < len; i++) { - double area = signedArea(rings[i]); - - if (area == 0) - continue; + for (const auto& ring : rings) { + double area = signedArea(ring); + if (area == 0) continue; - if (ccw == 0) + if (ccw == 0) { ccw = (area < 0 ? -1 : 1); + } if (ccw == (area < 0 ? -1 : 1) && !polygon.empty()) { - polygons.push_back(polygon); - polygon.clear(); + polygons.emplace_back(std::move(polygon)); } - polygon.push_back(rings[i]); + polygon.emplace_back(ring); } - if (!polygon.empty()) - polygons.push_back(polygon); + if (!polygon.empty()) { + polygons.emplace_back(std::move(polygon)); + } return polygons; } diff --git a/src/mbgl/tile/geometry_tile_data.hpp b/src/mbgl/tile/geometry_tile_data.hpp index a0d41608a3..203afcace7 100644 --- a/src/mbgl/tile/geometry_tile_data.hpp +++ b/src/mbgl/tile/geometry_tile_data.hpp @@ -35,6 +35,13 @@ public: GeometryCollection(Args&&... args) : std::vector(std::forward(args)...) {} GeometryCollection(std::initializer_list args) : std::vector(std::move(args)) {} + GeometryCollection(GeometryCollection&&) = default; + GeometryCollection& operator=(GeometryCollection&&) = default; + + GeometryCollection clone() const { return GeometryCollection(*this); } + +private: + GeometryCollection(const GeometryCollection&) = default; }; class GeometryTileFeature { diff --git a/src/mbgl/tile/geometry_tile_worker.cpp b/src/mbgl/tile/geometry_tile_worker.cpp index a69825d346..f4d57e5bfc 100644 --- a/src/mbgl/tile/geometry_tile_worker.cpp +++ b/src/mbgl/tile/geometry_tile_worker.cpp @@ -382,7 +382,7 @@ void GeometryTileWorker::parse() { if (!filter(expression::EvaluationContext { static_cast(this->id.overscaledZ), feature.get() })) continue; - GeometryCollection geometries = feature->getGeometries(); + const GeometryCollection& geometries = feature->getGeometries(); bucket->addFeature(*feature, geometries, {}, PatternLayerMap ()); featureIndex->insert(geometries, i, sourceLayerID, leaderImpl.id); } diff --git a/test/gl/bucket.test.cpp b/test/gl/bucket.test.cpp index 9c9847cf4d..f97fd53919 100644 --- a/test/gl/bucket.test.cpp +++ b/test/gl/bucket.test.cpp @@ -131,7 +131,7 @@ TEST(Buckets, SymbolBucket) { // SymbolBucket::addFeature() is a no-op. GeometryCollection point { { { 0, 0 } } }; - bucket.addFeature(StubGeometryTileFeature { {}, FeatureType::Point, point, properties }, point, {}, PatternLayerMap()); + bucket.addFeature(StubGeometryTileFeature { {}, FeatureType::Point, std::move(point), properties }, point, {}, PatternLayerMap()); ASSERT_FALSE(bucket.hasData()); ASSERT_FALSE(bucket.needsUpload()); diff --git a/test/util/merge_lines.test.cpp b/test/util/merge_lines.test.cpp index 1c8c7aae8f..5dc846ad6e 100644 --- a/test/util/merge_lines.test.cpp +++ b/test/util/merge_lines.test.cpp @@ -42,14 +42,14 @@ TEST(MergeLines, SameText) { input1.push_back(SymbolFeatureStub({}, FeatureType::LineString, {{{6, 0}, {7, 0}, {8, 0}}}, properties, aaa, {}, 0)); input1.push_back(SymbolFeatureStub({}, FeatureType::LineString, {{{5, 0}, {6, 0}}}, properties, aaa, {}, 0)); - const std::vector expected1 = { - { {}, FeatureType::LineString, {{{0, 0}, {1, 0}, {2, 0}, {3, 0}, {4, 0}}}, properties }, - { {}, FeatureType::LineString, {{{4, 0}, {5, 0}, {6, 0}}}, properties }, - { {}, FeatureType::LineString, {{{5, 0}, {6, 0}, {7, 0}, {8, 0}, {9, 0}}}, properties }, - { {}, FeatureType::LineString, { emptyLine }, properties }, - { {}, FeatureType::LineString, { emptyLine }, properties }, - { {}, FeatureType::LineString, { emptyLine }, properties } - }; + std::vector expected1; + expected1.emplace_back(StubGeometryTileFeature({}, FeatureType::LineString, {{{0, 0}, {1, 0}, {2, 0}, {3, 0}, {4, 0}}}, properties)); + expected1.emplace_back(StubGeometryTileFeature({}, FeatureType::LineString, {{{4, 0}, {5, 0}, {6, 0}}}, properties)); + expected1.emplace_back(StubGeometryTileFeature({}, FeatureType::LineString, {{{5, 0}, {6, 0}, {7, 0}, {8, 0}, {9, 0}}}, properties)); + expected1.emplace_back(StubGeometryTileFeature({}, FeatureType::LineString, { emptyLine }, properties)); + expected1.emplace_back(StubGeometryTileFeature({}, FeatureType::LineString, { emptyLine }, properties)); + expected1.emplace_back(StubGeometryTileFeature({}, FeatureType::LineString, { emptyLine }, properties)); + mbgl::util::mergeLines(input1); @@ -65,11 +65,11 @@ TEST(MergeLines, BothEnds) { input2.push_back(SymbolFeatureStub { {}, FeatureType::LineString, {{{4, 0}, {5, 0}, {6, 0}}}, properties, aaa, {}, 0 }); input2.push_back(SymbolFeatureStub { {}, FeatureType::LineString, {{{2, 0}, {3, 0}, {4, 0}}}, properties, aaa, {}, 0 }); - const std::vector expected2 = { - { {}, FeatureType::LineString, {{{0, 0}, {1, 0}, {2, 0}, {3, 0}, {4, 0}, {5, 0}, {6, 0}}}, properties }, - { {}, FeatureType::LineString, { emptyLine }, properties }, - { {}, FeatureType::LineString, { emptyLine }, properties } - }; + std::vector expected2; + expected2.emplace_back(StubGeometryTileFeature({}, FeatureType::LineString, {{{0, 0}, {1, 0}, {2, 0}, {3, 0}, {4, 0}, {5, 0}, {6, 0}}}, properties)); + expected2.emplace_back(StubGeometryTileFeature({}, FeatureType::LineString, { emptyLine }, properties)); + expected2.emplace_back(StubGeometryTileFeature({}, FeatureType::LineString, { emptyLine }, properties)); + mbgl::util::mergeLines(input2); @@ -85,11 +85,11 @@ TEST(MergeLines, CircularLines) { input3.push_back(SymbolFeatureStub { {}, FeatureType::LineString, {{{2, 0}, {3, 0}, {4, 0}}}, properties, aaa, {}, 0 }); input3.push_back(SymbolFeatureStub { {}, FeatureType::LineString, {{{4, 0}, {0, 0}}}, properties, aaa, {}, 0 }); - const std::vector expected3 = { - { {}, FeatureType::LineString, {{{0, 0}, {1, 0}, {2, 0}, {3, 0}, {4, 0}, {0, 0}}}, properties }, - { {}, FeatureType::LineString, { emptyLine }, properties }, - { {}, FeatureType::LineString, { emptyLine }, properties } - }; + std::vector expected3; + expected3.emplace_back(StubGeometryTileFeature({}, FeatureType::LineString, {{{0, 0}, {1, 0}, {2, 0}, {3, 0}, {4, 0}, {0, 0}}}, properties)); + expected3.emplace_back(StubGeometryTileFeature({}, FeatureType::LineString, { emptyLine }, properties)); + expected3.emplace_back(StubGeometryTileFeature({}, FeatureType::LineString, { emptyLine }, properties)); + mbgl::util::mergeLines(input3); @@ -102,20 +102,20 @@ TEST(MergeLines, EmptyOuterGeometry) { std::vector input; input.push_back(SymbolFeatureStub { {}, FeatureType::LineString, {}, properties, aaa, {}, 0 }); - const std::vector expected = { { {}, FeatureType::LineString, {}, properties } }; + const StubGeometryTileFeature expected{ {}, FeatureType::LineString, {}, properties }; mbgl::util::mergeLines(input); - EXPECT_EQ(input[0].geometry, expected[0].getGeometries()); + EXPECT_EQ(input[0].geometry, expected.getGeometries()); } TEST(MergeLines, EmptyInnerGeometry) { std::vector input; input.push_back(SymbolFeatureStub { {}, FeatureType::LineString, {}, properties, aaa, {}, 0 }); - const std::vector expected = { { {}, FeatureType::LineString, {}, properties } }; + const StubGeometryTileFeature expected{ {}, FeatureType::LineString, {}, properties }; mbgl::util::mergeLines(input); - EXPECT_EQ(input[0].geometry, expected[0].getGeometries()); + EXPECT_EQ(input[0].geometry, expected.getGeometries()); } -- cgit v1.2.1