From 73a182a00dc248cd03e0251acdbbaa0b0ce5d93f Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Mon, 9 Jan 2017 13:05:13 -0800 Subject: [PATCH] [core] Fix flickering caused by regression in #7586 It should be safe to invoke GeometryTileWorker::setData multiple times without invoking GeometryTileWorker::setLayers. Therefore GeometryTileWorker::redoLayout() must not consume the layers. --- cmake/test-files.cmake | 1 + src/mbgl/geometry/feature_index.cpp | 4 +- src/mbgl/geometry/feature_index.hpp | 2 +- src/mbgl/layout/symbol_layout.cpp | 6 +- src/mbgl/layout/symbol_layout.hpp | 4 +- src/mbgl/style/group_by_layout.cpp | 11 ++-- src/mbgl/style/group_by_layout.hpp | 2 +- src/mbgl/style/layers/symbol_layer_impl.cpp | 2 +- src/mbgl/style/layers/symbol_layer_impl.hpp | 2 +- src/mbgl/tile/geometry_tile_worker.cpp | 13 ++-- test/src/mbgl/test/stub_tile_observer.hpp | 22 +++++++ test/style/group_by_layout.test.cpp | 8 +-- test/tile/geojson_tile.test.cpp | 72 +++++++++++++++++++++ 13 files changed, 123 insertions(+), 26 deletions(-) create mode 100644 test/src/mbgl/test/stub_tile_observer.hpp create mode 100644 test/tile/geojson_tile.test.cpp diff --git a/cmake/test-files.cmake b/cmake/test-files.cmake index e05b956b1ec..cd40d177cb8 100644 --- a/cmake/test-files.cmake +++ b/cmake/test-files.cmake @@ -90,6 +90,7 @@ set(MBGL_TEST_FILES test/text/quads.test.cpp # tile + test/tile/geojson_tile.test.cpp test/tile/geometry_tile_data.test.cpp test/tile/raster_tile.test.cpp test/tile/tile_coordinate.test.cpp diff --git a/src/mbgl/geometry/feature_index.cpp b/src/mbgl/geometry/feature_index.cpp index a04d9e06068..5019d888ca1 100644 --- a/src/mbgl/geometry/feature_index.cpp +++ b/src/mbgl/geometry/feature_index.cpp @@ -151,8 +151,8 @@ optional FeatureIndex::translateQueryGeometry( return translated; } -void FeatureIndex::addBucketLayerName(const std::string& bucketName, const std::string& layerID) { - bucketLayerIDs[bucketName].push_back(layerID); +void FeatureIndex::setBucketLayerIDs(const std::string& bucketName, const std::vector& layerIDs) { + bucketLayerIDs[bucketName] = layerIDs; } } // namespace mbgl diff --git a/src/mbgl/geometry/feature_index.hpp b/src/mbgl/geometry/feature_index.hpp index 662e78aa2c8..ca813f4b6b8 100644 --- a/src/mbgl/geometry/feature_index.hpp +++ b/src/mbgl/geometry/feature_index.hpp @@ -52,7 +52,7 @@ class FeatureIndex { const float bearing, const float pixelsToTileUnits); - void addBucketLayerName(const std::string& bucketName, const std::string& layerName); + void setBucketLayerIDs(const std::string& bucketName, const std::vector& layerIDs); private: void addFeature( diff --git a/src/mbgl/layout/symbol_layout.cpp b/src/mbgl/layout/symbol_layout.cpp index 85e0b196096..eaa03329952 100644 --- a/src/mbgl/layout/symbol_layout.cpp +++ b/src/mbgl/layout/symbol_layout.cpp @@ -27,7 +27,7 @@ namespace mbgl { using namespace style; -SymbolLayout::SymbolLayout(std::vector> layers_, +SymbolLayout::SymbolLayout(std::vector layerIDs_, std::string sourceLayerName_, uint32_t overscaling_, float zoom_, @@ -37,7 +37,7 @@ SymbolLayout::SymbolLayout(std::vector> layers_, style::SymbolLayoutProperties::Evaluated layout_, float textMaxSize_, SpriteAtlas& spriteAtlas_) - : layers(std::move(layers_)), + : layerIDs(std::move(layerIDs_)), sourceLayerName(std::move(sourceLayerName_)), overscaling(overscaling_), zoom(zoom_), @@ -254,7 +254,7 @@ void SymbolLayout::addFeature(const SymbolFeature& feature, ? SymbolPlacementType::Point : layout.get(); const float textRepeatDistance = symbolSpacing / 2; - IndexedSubfeature indexedFeature = {feature.index, sourceLayerName, layers.at(0)->getID(), symbolInstances.size()}; + IndexedSubfeature indexedFeature = {feature.index, sourceLayerName, layerIDs.at(0), symbolInstances.size()}; auto addSymbolInstance = [&] (const GeometryCoordinates& line, Anchor& anchor) { // https://github.com/mapbox/vector-tile-spec/tree/master/2.1#41-layers diff --git a/src/mbgl/layout/symbol_layout.hpp b/src/mbgl/layout/symbol_layout.hpp index 63c0a8859d9..c89b791ccc4 100644 --- a/src/mbgl/layout/symbol_layout.hpp +++ b/src/mbgl/layout/symbol_layout.hpp @@ -28,7 +28,7 @@ struct Anchor; class SymbolLayout { public: - SymbolLayout(std::vector>, + SymbolLayout(std::vector layerIDs_, std::string sourceLayerName_, uint32_t overscaling, float zoom, @@ -56,7 +56,7 @@ class SymbolLayout { State state = Pending; - const std::vector> layers; + const std::vector layerIDs; const std::string sourceLayerName; private: diff --git a/src/mbgl/style/group_by_layout.cpp b/src/mbgl/style/group_by_layout.cpp index 52d33827ef9..b15fe1ca0d1 100644 --- a/src/mbgl/style/group_by_layout.cpp +++ b/src/mbgl/style/group_by_layout.cpp @@ -32,16 +32,15 @@ std::string layoutKey(const Layer& layer) { return s.GetString(); } -std::vector>> groupByLayout(std::vector> layers) { - std::unordered_map>> map; +std::vector> groupByLayout(const std::vector>& layers) { + std::unordered_map> map; for (auto& layer : layers) { - auto& vector = map[layoutKey(*layer)]; - vector.push_back(std::move(layer)); + map[layoutKey(*layer)].push_back(layer.get()); } - std::vector>> result; + std::vector> result; for (auto& pair : map) { - result.push_back(std::move(pair.second)); + result.push_back(pair.second); } return result; diff --git a/src/mbgl/style/group_by_layout.hpp b/src/mbgl/style/group_by_layout.hpp index dd7b5d118a2..3a89ac95f3c 100644 --- a/src/mbgl/style/group_by_layout.hpp +++ b/src/mbgl/style/group_by_layout.hpp @@ -8,7 +8,7 @@ namespace style { class Layer; -std::vector>> groupByLayout(std::vector>); +std::vector> groupByLayout(const std::vector>&); } // namespace style } // namespace mbgl diff --git a/src/mbgl/style/layers/symbol_layer_impl.cpp b/src/mbgl/style/layers/symbol_layer_impl.cpp index 8fb85513cf0..f058598f5f3 100644 --- a/src/mbgl/style/layers/symbol_layer_impl.cpp +++ b/src/mbgl/style/layers/symbol_layer_impl.cpp @@ -31,7 +31,7 @@ std::unique_ptr SymbolLayer::Impl::createBucket(BucketParameters&, const std::unique_ptr SymbolLayer::Impl::createLayout(BucketParameters& parameters, const GeometryTileLayer& layer, - std::vector> group) const { + std::vector group) const { PropertyEvaluationParameters p(parameters.tileID.overscaledZ); SymbolLayoutProperties::Evaluated evaluated = layout.evaluate(p); diff --git a/src/mbgl/style/layers/symbol_layer_impl.hpp b/src/mbgl/style/layers/symbol_layer_impl.hpp index 0d9a7c07e39..db18da07e2a 100644 --- a/src/mbgl/style/layers/symbol_layer_impl.hpp +++ b/src/mbgl/style/layers/symbol_layer_impl.hpp @@ -52,7 +52,7 @@ class SymbolLayer::Impl : public Layer::Impl { std::unique_ptr createBucket(BucketParameters&, const GeometryTileLayer&) const override; std::unique_ptr createLayout(BucketParameters&, const GeometryTileLayer&, - std::vector>) const; + std::vector) const; SymbolPropertyValues iconPropertyValues(const SymbolLayoutProperties::Evaluated&) const; SymbolPropertyValues textPropertyValues(const SymbolLayoutProperties::Evaluated&) const; diff --git a/src/mbgl/tile/geometry_tile_worker.cpp b/src/mbgl/tile/geometry_tile_worker.cpp index 169339f13fc..2a86b7fedaf 100644 --- a/src/mbgl/tile/geometry_tile_worker.cpp +++ b/src/mbgl/tile/geometry_tile_worker.cpp @@ -214,7 +214,7 @@ void GeometryTileWorker::redoLayout() { auto featureIndex = std::make_unique(); BucketParameters parameters { id, obsolete, *featureIndex, mode }; - std::vector>> groups = groupByLayout(std::move(*layers)); + std::vector> groups = groupByLayout(*layers); for (auto& group : groups) { if (obsolete) { return; @@ -231,13 +231,16 @@ void GeometryTileWorker::redoLayout() { continue; } + std::vector layerIDs; for (const auto& layer : group) { - featureIndex->addBucketLayerName(leader.getID(), layer->getID()); + layerIDs.push_back(layer->getID()); } + featureIndex->setBucketLayerIDs(leader.getID(), layerIDs); + if (leader.is()) { symbolLayoutMap.emplace(leader.getID(), - leader.as()->impl->createLayout(parameters, *geometryLayer, std::move(group))); + leader.as()->impl->createLayout(parameters, *geometryLayer, layerIDs)); } else { std::shared_ptr bucket = leader.baseImpl->createBucket(parameters, *geometryLayer); if (!bucket->hasData()) { @@ -321,8 +324,8 @@ void GeometryTileWorker::attemptPlacement() { } std::shared_ptr bucket = symbolLayout->place(*collisionTile); - for (const auto& layer : symbolLayout->layers) { - buckets.emplace(layer->getID(), bucket); + for (const auto& layerID : symbolLayout->layerIDs) { + buckets.emplace(layerID, bucket); } } diff --git a/test/src/mbgl/test/stub_tile_observer.hpp b/test/src/mbgl/test/stub_tile_observer.hpp new file mode 100644 index 00000000000..43ae4d8360e --- /dev/null +++ b/test/src/mbgl/test/stub_tile_observer.hpp @@ -0,0 +1,22 @@ +#pragma once + +#include + +using namespace mbgl; + +/** + * An implementation of TileObserver that forwards all methods to dynamically-settable lambas. + */ +class StubTileObserver : public TileObserver { +public: + void onTileChanged(Tile& tile) override { + if (tileChanged) tileChanged(tile); + } + + void onTileError(Tile& tile, std::exception_ptr error) override { + if (tileError) tileError(tile, error); + } + + std::function tileChanged; + std::function tileError; +}; diff --git a/test/style/group_by_layout.test.cpp b/test/style/group_by_layout.test.cpp index d74916cdc9a..600ba6a0f12 100644 --- a/test/style/group_by_layout.test.cpp +++ b/test/style/group_by_layout.test.cpp @@ -12,7 +12,7 @@ TEST(GroupByLayout, Related) { std::vector> layers; layers.push_back(std::make_unique("a", "source")); layers.push_back(std::make_unique("b", "source")); - auto result = groupByLayout(std::move(layers)); + auto result = groupByLayout(layers); ASSERT_EQ(1u, result.size()); ASSERT_EQ(2u, result[0].size()); } @@ -21,7 +21,7 @@ TEST(GroupByLayout, UnrelatedType) { std::vector> layers; layers.push_back(std::make_unique("background")); layers.push_back(std::make_unique("circle", "source")); - auto result = groupByLayout(std::move(layers)); + auto result = groupByLayout(layers); ASSERT_EQ(2u, result.size()); } @@ -30,7 +30,7 @@ TEST(GroupByLayout, UnrelatedFilter) { layers.push_back(std::make_unique("a", "source")); layers.push_back(std::make_unique("b", "source")); layers[0]->as()->setFilter(EqualsFilter()); - auto result = groupByLayout(std::move(layers)); + auto result = groupByLayout(layers); ASSERT_EQ(2u, result.size()); } @@ -39,6 +39,6 @@ TEST(GroupByLayout, UnrelatedLayout) { layers.push_back(std::make_unique("a", "source")); layers.push_back(std::make_unique("b", "source")); layers[0]->as()->setLineCap(LineCapType::Square); - auto result = groupByLayout(std::move(layers)); + auto result = groupByLayout(layers); ASSERT_EQ(2u, result.size()); } diff --git a/test/tile/geojson_tile.test.cpp b/test/tile/geojson_tile.test.cpp new file mode 100644 index 00000000000..920e946a2b3 --- /dev/null +++ b/test/tile/geojson_tile.test.cpp @@ -0,0 +1,72 @@ +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include +#include +#include + +#include + +using namespace mbgl; +using namespace mbgl::style; + +class GeoJSONTileTest { +public: + FakeFileSource fileSource; + TransformState transformState; + util::RunLoop loop; + ThreadPool threadPool { 1 }; + AnnotationManager annotationManager { 1.0 }; + style::Style style { fileSource, 1.0 }; + Tileset tileset { { "https://example.com" }, { 0, 22 }, "none" }; + + style::UpdateParameters updateParameters { + 1.0, + MapDebugOptions(), + transformState, + threadPool, + fileSource, + MapMode::Continuous, + annotationManager, + style + }; +}; + +TEST(GeoJSONTile, Issue7648) { + GeoJSONTileTest test; + GeoJSONTile tile(OverscaledTileID(0, 0, 0), "source", test.updateParameters); + + test.style.addLayer(std::make_unique("circle", "source")); + + StubTileObserver observer; + observer.tileChanged = [&] (const Tile&) { + // Once present, the bucket should never "disappear", which would cause + // flickering. + ASSERT_NE(nullptr, tile.getBucket(*test.style.getLayer("circle"))); + }; + tile.setObserver(&observer); + + tile.setPlacementConfig({}); + + mapbox::geometry::feature_collection features; + features.push_back(mapbox::geometry::feature { + mapbox::geometry::point(0, 0) + }); + + tile.updateData(features); + while (!tile.isComplete()) { + test.loop.runOnce(); + } + + tile.updateData(features); + while (!tile.isComplete()) { + test.loop.runOnce(); + } +}