From 2f9bfb99d172402b3600d65d0ddf6f240b087449 Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Fri, 6 Jan 2017 13:24:33 -0800 Subject: [PATCH 1/3] [core] Keep symbol and non-symbol buckets segregated Discard prior symbol buckets only when new symbol buckets became available, in order to eliminate flickering when tiles are refreshed. --- src/mbgl/tile/geometry_tile.cpp | 8 +++--- src/mbgl/tile/geometry_tile.hpp | 7 +++--- test/tile/vector_tile.test.cpp | 43 +++++++++++++++++++++++++++++++++ 3 files changed, 51 insertions(+), 7 deletions(-) diff --git a/src/mbgl/tile/geometry_tile.cpp b/src/mbgl/tile/geometry_tile.cpp index 2a0047fecfc..4c5a61672d8 100644 --- a/src/mbgl/tile/geometry_tile.cpp +++ b/src/mbgl/tile/geometry_tile.cpp @@ -6,6 +6,7 @@ #include #include #include +#include #include #include #include @@ -105,7 +106,7 @@ void GeometryTile::redoLayout() { void GeometryTile::onLayout(LayoutResult result) { availableData = DataAvailability::Some; - buckets = std::move(result.buckets); + nonSymbolBuckets = std::move(result.nonSymbolBuckets); featureIndex = std::move(result.featureIndex); data = std::move(result.tileData); observer->onTileChanged(*this); @@ -115,9 +116,7 @@ void GeometryTile::onPlacement(PlacementResult result) { if (result.correlationID == correlationID) { availableData = DataAvailability::All; } - for (auto& bucket : result.buckets) { - buckets[bucket.first] = std::move(bucket.second); - } + symbolBuckets = std::move(result.symbolBuckets); featureIndex->setCollisionTile(std::move(result.collisionTile)); observer->onTileChanged(*this); } @@ -128,6 +127,7 @@ void GeometryTile::onError(std::exception_ptr err) { } Bucket* GeometryTile::getBucket(const Layer& layer) { + const auto& buckets = layer.is() ? symbolBuckets : nonSymbolBuckets; const auto it = buckets.find(layer.baseImpl->id); if (it == buckets.end()) { return nullptr; diff --git a/src/mbgl/tile/geometry_tile.hpp b/src/mbgl/tile/geometry_tile.hpp index 993f7b018ea..5e7e501e89c 100644 --- a/src/mbgl/tile/geometry_tile.hpp +++ b/src/mbgl/tile/geometry_tile.hpp @@ -50,7 +50,7 @@ class GeometryTile : public Tile { class LayoutResult { public: - std::unordered_map> buckets; + std::unordered_map> nonSymbolBuckets; std::unique_ptr featureIndex; std::unique_ptr tileData; uint64_t correlationID; @@ -59,7 +59,7 @@ class GeometryTile : public Tile { class PlacementResult { public: - std::unordered_map> buckets; + std::unordered_map> symbolBuckets; std::unique_ptr collisionTile; uint64_t correlationID; }; @@ -80,7 +80,8 @@ class GeometryTile : public Tile { uint64_t correlationID = 0; optional requestedConfig; - std::unordered_map> buckets; + std::unordered_map> nonSymbolBuckets; + std::unordered_map> symbolBuckets; std::unique_ptr featureIndex; std::unique_ptr data; }; diff --git a/test/tile/vector_tile.test.cpp b/test/tile/vector_tile.test.cpp index 210422feec3..9732f23b320 100644 --- a/test/tile/vector_tile.test.cpp +++ b/test/tile/vector_tile.test.cpp @@ -8,8 +8,14 @@ #include #include #include +#include +#include +#include +#include #include +#include + using namespace mbgl; class VectorTileTest { @@ -47,3 +53,40 @@ TEST(VectorTile, onError) { tile.onError(std::make_exception_ptr(std::runtime_error("test"))); EXPECT_TRUE(tile.isRenderable()); } + +TEST(VectorTile, Issue7615) { + VectorTileTest test; + VectorTile tile(OverscaledTileID(0, 0, 0), "source", test.updateParameters, test.tileset); + + style::SymbolLayer symbolLayer("symbol", "source"); + auto symbolBucket = std::make_shared( + MapMode::Continuous, style::SymbolLayoutProperties::Evaluated(), false, false); + + // First onLayout is required so that a non-null FeatureIndex is available. + tile.onLayout(GeometryTile::LayoutResult { + {}, + std::make_unique(), + nullptr, + 0 + }); + + // Simulate placement of a symbol layer. + tile.onPlacement(GeometryTile::PlacementResult { + {{ + symbolLayer.getID(), + symbolBucket + }}, + nullptr, + 0 + }); + + // Second onLayout should not cause the existing symbol bucket to be discarded. + tile.onLayout(GeometryTile::LayoutResult { + {}, + nullptr, + nullptr, + 0 + }); + + EXPECT_EQ(symbolBucket.get(), tile.getBucket(symbolLayer)); +} From fa2a465902721883d0f9c48ac715a6c179ca0b31 Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Fri, 6 Jan 2017 13:26:58 -0800 Subject: [PATCH 2/3] [core] Replace FeatureIndex::collisionTile with a method parameter This reduces state and simplifies the test added in the prior commit. --- src/mbgl/geometry/feature_index.cpp | 7 ++----- src/mbgl/geometry/feature_index.hpp | 6 ++---- src/mbgl/text/collision_tile.cpp | 2 +- src/mbgl/text/collision_tile.hpp | 2 +- src/mbgl/tile/geometry_tile.cpp | 5 +++-- src/mbgl/tile/geometry_tile.hpp | 4 +++- test/tile/vector_tile.test.cpp | 10 +--------- 7 files changed, 13 insertions(+), 23 deletions(-) diff --git a/src/mbgl/geometry/feature_index.cpp b/src/mbgl/geometry/feature_index.cpp index b37bdb5ecc0..a04d9e06068 100644 --- a/src/mbgl/geometry/feature_index.cpp +++ b/src/mbgl/geometry/feature_index.cpp @@ -59,7 +59,8 @@ void FeatureIndex::query( const optional>& filterLayerIDs, const GeometryTileData& geometryTileData, const CanonicalTileID& tileID, - const style::Style& style) const { + const style::Style& style, + const CollisionTile* collisionTile) const { mapbox::geometry::box box = mapbox::geometry::envelope(queryGeometry); @@ -154,8 +155,4 @@ void FeatureIndex::addBucketLayerName(const std::string& bucketName, const std:: bucketLayerIDs[bucketName].push_back(layerID); } -void FeatureIndex::setCollisionTile(std::unique_ptr collisionTile_) { - collisionTile = std::move(collisionTile_); -} - } // namespace mbgl diff --git a/src/mbgl/geometry/feature_index.hpp b/src/mbgl/geometry/feature_index.hpp index 021770c78d3..662e78aa2c8 100644 --- a/src/mbgl/geometry/feature_index.hpp +++ b/src/mbgl/geometry/feature_index.hpp @@ -42,7 +42,8 @@ class FeatureIndex { const optional>& layerIDs, const GeometryTileData&, const CanonicalTileID&, - const style::Style&) const; + const style::Style&, + const CollisionTile*) const; static optional translateQueryGeometry( const GeometryCoordinates& queryGeometry, @@ -53,8 +54,6 @@ class FeatureIndex { void addBucketLayerName(const std::string& bucketName, const std::string& layerName); - void setCollisionTile(std::unique_ptr); - private: void addFeature( std::unordered_map>& result, @@ -67,7 +66,6 @@ class FeatureIndex { const float bearing, const float pixelsToTileUnits) const; - std::unique_ptr collisionTile; GridIndex grid; unsigned int sortIndex = 0; diff --git a/src/mbgl/text/collision_tile.cpp b/src/mbgl/text/collision_tile.cpp index 419ab31a79b..368750c89f3 100644 --- a/src/mbgl/text/collision_tile.cpp +++ b/src/mbgl/text/collision_tile.cpp @@ -168,7 +168,7 @@ Box CollisionTile::getTreeBox(const Point& anchor, const CollisionBox& bo }; } -std::vector CollisionTile::queryRenderedSymbols(const GeometryCoordinates& queryGeometry, float scale) { +std::vector CollisionTile::queryRenderedSymbols(const GeometryCoordinates& queryGeometry, float scale) const { std::vector result; if (queryGeometry.empty() || (tree.empty() && ignoredTree.empty())) { return result; diff --git a/src/mbgl/text/collision_tile.hpp b/src/mbgl/text/collision_tile.hpp index 186cd19d28b..ea4324edaf8 100644 --- a/src/mbgl/text/collision_tile.hpp +++ b/src/mbgl/text/collision_tile.hpp @@ -42,7 +42,7 @@ class CollisionTile { float placeFeature(const CollisionFeature&, bool allowOverlap, bool avoidEdges); void insertFeature(CollisionFeature&, float minPlacementScale, bool ignorePlacement); - std::vector queryRenderedSymbols(const GeometryCoordinates&, float scale); + std::vector queryRenderedSymbols(const GeometryCoordinates&, float scale) const; const PlacementConfig config; diff --git a/src/mbgl/tile/geometry_tile.cpp b/src/mbgl/tile/geometry_tile.cpp index 4c5a61672d8..9aeb35c8218 100644 --- a/src/mbgl/tile/geometry_tile.cpp +++ b/src/mbgl/tile/geometry_tile.cpp @@ -117,7 +117,7 @@ void GeometryTile::onPlacement(PlacementResult result) { availableData = DataAvailability::All; } symbolBuckets = std::move(result.symbolBuckets); - featureIndex->setCollisionTile(std::move(result.collisionTile)); + collisionTile = std::move(result.collisionTile); observer->onTileChanged(*this); } @@ -153,7 +153,8 @@ void GeometryTile::queryRenderedFeatures( layerIDs, *data, id.canonical, - style); + style, + collisionTile.get()); } } // namespace mbgl diff --git a/src/mbgl/tile/geometry_tile.hpp b/src/mbgl/tile/geometry_tile.hpp index 5e7e501e89c..c61a5103113 100644 --- a/src/mbgl/tile/geometry_tile.hpp +++ b/src/mbgl/tile/geometry_tile.hpp @@ -81,9 +81,11 @@ class GeometryTile : public Tile { optional requestedConfig; std::unordered_map> nonSymbolBuckets; - std::unordered_map> symbolBuckets; std::unique_ptr featureIndex; std::unique_ptr data; + + std::unordered_map> symbolBuckets; + std::unique_ptr collisionTile; }; } // namespace mbgl diff --git a/test/tile/vector_tile.test.cpp b/test/tile/vector_tile.test.cpp index 9732f23b320..e34629bdbaf 100644 --- a/test/tile/vector_tile.test.cpp +++ b/test/tile/vector_tile.test.cpp @@ -62,14 +62,6 @@ TEST(VectorTile, Issue7615) { auto symbolBucket = std::make_shared( MapMode::Continuous, style::SymbolLayoutProperties::Evaluated(), false, false); - // First onLayout is required so that a non-null FeatureIndex is available. - tile.onLayout(GeometryTile::LayoutResult { - {}, - std::make_unique(), - nullptr, - 0 - }); - // Simulate placement of a symbol layer. tile.onPlacement(GeometryTile::PlacementResult { {{ @@ -80,7 +72,7 @@ TEST(VectorTile, Issue7615) { 0 }); - // Second onLayout should not cause the existing symbol bucket to be discarded. + // Subsequent onLayout should not cause the existing symbol bucket to be discarded. tile.onLayout(GeometryTile::LayoutResult { {}, nullptr, From d8eed43f3443891031b9c07a4878566996dc1617 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Konstantin=20K=C3=A4fer?= Date: Mon, 9 Jan 2017 18:17:06 +0100 Subject: [PATCH 3/3] [core] clear out GeometryTileWorker::layers during redoLayout This avoids a corner case where redoLayout was called with a new set of data, but the layer objects were moved out during the previous call to redoLayout. When setting new data, we call setData() first, then setLayout(). The setData() call doesn't immediately process the tiles, since it will still be lacking layer data. --- include/mbgl/util/optional.hpp | 3 +++ src/mbgl/tile/geometry_tile_worker.cpp | 4 +++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/include/mbgl/util/optional.hpp b/include/mbgl/util/optional.hpp index a9374a1b536..85c6dd2bd6f 100644 --- a/include/mbgl/util/optional.hpp +++ b/include/mbgl/util/optional.hpp @@ -7,4 +7,7 @@ namespace mbgl { template using optional = std::experimental::optional; +using nullopt_t = std::experimental::nullopt_t; +constexpr auto nullopt = std::experimental::nullopt; + } // namespace mbgl diff --git a/src/mbgl/tile/geometry_tile_worker.cpp b/src/mbgl/tile/geometry_tile_worker.cpp index 169339f13fc..fe7dac1be38 100644 --- a/src/mbgl/tile/geometry_tile_worker.cpp +++ b/src/mbgl/tile/geometry_tile_worker.cpp @@ -215,6 +215,8 @@ void GeometryTileWorker::redoLayout() { BucketParameters parameters { id, obsolete, *featureIndex, mode }; std::vector>> groups = groupByLayout(std::move(*layers)); + layers = nullopt; + for (auto& group : groups) { if (obsolete) { return; @@ -280,7 +282,7 @@ bool GeometryTileWorker::hasPendingSymbolDependencies() const { } void GeometryTileWorker::attemptPlacement() { - if (!data || !layers || !placementConfig) { + if (!placementConfig) { return; }