diff options
author | John Firebaugh <john.firebaugh@gmail.com> | 2017-01-09 13:05:13 -0800 |
---|---|---|
committer | John Firebaugh <john.firebaugh@gmail.com> | 2017-01-10 10:15:22 -0800 |
commit | e7778509c546f4d1e0dd73e2d1deceb146c90de2 (patch) | |
tree | d8101c341d45a6a011cb37101f486814be7a6259 | |
parent | ea4c0b77c39926c770b0003097509e36dc26621d (diff) | |
download | qtlocation-mapboxgl-e7778509c546f4d1e0dd73e2d1deceb146c90de2.tar.gz |
[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.
-rw-r--r-- | cmake/test-files.cmake | 1 | ||||
-rw-r--r-- | src/mbgl/geometry/feature_index.cpp | 4 | ||||
-rw-r--r-- | src/mbgl/geometry/feature_index.hpp | 2 | ||||
-rw-r--r-- | src/mbgl/layout/symbol_layout.cpp | 6 | ||||
-rw-r--r-- | src/mbgl/layout/symbol_layout.hpp | 4 | ||||
-rw-r--r-- | src/mbgl/style/group_by_layout.cpp | 11 | ||||
-rw-r--r-- | src/mbgl/style/group_by_layout.hpp | 2 | ||||
-rw-r--r-- | src/mbgl/style/layers/symbol_layer_impl.cpp | 2 | ||||
-rw-r--r-- | src/mbgl/style/layers/symbol_layer_impl.hpp | 2 | ||||
-rw-r--r-- | src/mbgl/tile/geometry_tile_worker.cpp | 13 | ||||
-rw-r--r-- | test/src/mbgl/test/stub_tile_observer.hpp | 22 | ||||
-rw-r--r-- | test/style/group_by_layout.test.cpp | 8 | ||||
-rw-r--r-- | test/tile/geojson_tile.test.cpp | 72 |
13 files changed, 123 insertions, 26 deletions
diff --git a/cmake/test-files.cmake b/cmake/test-files.cmake index e05b956b1e..cd40d177cb 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 a04d9e0606..5019d888ca 100644 --- a/src/mbgl/geometry/feature_index.cpp +++ b/src/mbgl/geometry/feature_index.cpp @@ -151,8 +151,8 @@ optional<GeometryCoordinates> 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<std::string>& layerIDs) { + bucketLayerIDs[bucketName] = layerIDs; } } // namespace mbgl diff --git a/src/mbgl/geometry/feature_index.hpp b/src/mbgl/geometry/feature_index.hpp index 662e78aa2c..ca813f4b6b 100644 --- a/src/mbgl/geometry/feature_index.hpp +++ b/src/mbgl/geometry/feature_index.hpp @@ -52,7 +52,7 @@ public: 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<std::string>& layerIDs); private: void addFeature( diff --git a/src/mbgl/layout/symbol_layout.cpp b/src/mbgl/layout/symbol_layout.cpp index 85e0b19609..eaa0332995 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<std::unique_ptr<Layer>> layers_, +SymbolLayout::SymbolLayout(std::vector<std::string> layerIDs_, std::string sourceLayerName_, uint32_t overscaling_, float zoom_, @@ -37,7 +37,7 @@ SymbolLayout::SymbolLayout(std::vector<std::unique_ptr<Layer>> 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<SymbolPlacement>(); 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 63c0a8859d..c89b791ccc 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<std::unique_ptr<style::Layer>>, + SymbolLayout(std::vector<std::string> layerIDs_, std::string sourceLayerName_, uint32_t overscaling, float zoom, @@ -56,7 +56,7 @@ public: State state = Pending; - const std::vector<std::unique_ptr<style::Layer>> layers; + const std::vector<std::string> 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 52d33827ef..b15fe1ca0d 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<std::vector<std::unique_ptr<Layer>>> groupByLayout(std::vector<std::unique_ptr<Layer>> layers) { - std::unordered_map<std::string, std::vector<std::unique_ptr<Layer>>> map; +std::vector<std::vector<const Layer*>> groupByLayout(const std::vector<std::unique_ptr<Layer>>& layers) { + std::unordered_map<std::string, std::vector<const Layer*>> 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<std::vector<std::unique_ptr<Layer>>> result; + std::vector<std::vector<const Layer*>> 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 dd7b5d118a..3a89ac95f3 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<std::vector<std::unique_ptr<Layer>>> groupByLayout(std::vector<std::unique_ptr<Layer>>); +std::vector<std::vector<const Layer*>> groupByLayout(const std::vector<std::unique_ptr<Layer>>&); } // 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 8fb85513cf..f058598f5f 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<Bucket> SymbolLayer::Impl::createBucket(BucketParameters&, const std::unique_ptr<SymbolLayout> SymbolLayer::Impl::createLayout(BucketParameters& parameters, const GeometryTileLayer& layer, - std::vector<std::unique_ptr<Layer>> group) const { + std::vector<std::string> 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 0d9a7c07e3..db18da07e2 100644 --- a/src/mbgl/style/layers/symbol_layer_impl.hpp +++ b/src/mbgl/style/layers/symbol_layer_impl.hpp @@ -52,7 +52,7 @@ public: std::unique_ptr<Bucket> createBucket(BucketParameters&, const GeometryTileLayer&) const override; std::unique_ptr<SymbolLayout> createLayout(BucketParameters&, const GeometryTileLayer&, - std::vector<std::unique_ptr<Layer>>) const; + std::vector<std::string>) 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 169339f13f..2a86b7feda 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<FeatureIndex>(); BucketParameters parameters { id, obsolete, *featureIndex, mode }; - std::vector<std::vector<std::unique_ptr<Layer>>> groups = groupByLayout(std::move(*layers)); + std::vector<std::vector<const Layer*>> groups = groupByLayout(*layers); for (auto& group : groups) { if (obsolete) { return; @@ -231,13 +231,16 @@ void GeometryTileWorker::redoLayout() { continue; } + std::vector<std::string> 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<SymbolLayer>()) { symbolLayoutMap.emplace(leader.getID(), - leader.as<SymbolLayer>()->impl->createLayout(parameters, *geometryLayer, std::move(group))); + leader.as<SymbolLayer>()->impl->createLayout(parameters, *geometryLayer, layerIDs)); } else { std::shared_ptr<Bucket> bucket = leader.baseImpl->createBucket(parameters, *geometryLayer); if (!bucket->hasData()) { @@ -321,8 +324,8 @@ void GeometryTileWorker::attemptPlacement() { } std::shared_ptr<Bucket> 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 0000000000..43ae4d8360 --- /dev/null +++ b/test/src/mbgl/test/stub_tile_observer.hpp @@ -0,0 +1,22 @@ +#pragma once + +#include <mbgl/tile/tile_observer.hpp> + +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<void (Tile&)> tileChanged; + std::function<void (Tile&, std::exception_ptr)> tileError; +}; diff --git a/test/style/group_by_layout.test.cpp b/test/style/group_by_layout.test.cpp index d74916cdc9..600ba6a0f1 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<std::unique_ptr<Layer>> layers; layers.push_back(std::make_unique<LineLayer>("a", "source")); layers.push_back(std::make_unique<LineLayer>("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<std::unique_ptr<Layer>> layers; layers.push_back(std::make_unique<BackgroundLayer>("background")); layers.push_back(std::make_unique<CircleLayer>("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<LineLayer>("a", "source")); layers.push_back(std::make_unique<LineLayer>("b", "source")); layers[0]->as<LineLayer>()->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<LineLayer>("a", "source")); layers.push_back(std::make_unique<LineLayer>("b", "source")); layers[0]->as<LineLayer>()->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 0000000000..920e946a2b --- /dev/null +++ b/test/tile/geojson_tile.test.cpp @@ -0,0 +1,72 @@ +#include <mbgl/test/util.hpp> +#include <mbgl/test/fake_file_source.hpp> +#include <mbgl/test/stub_tile_observer.hpp> +#include <mbgl/tile/geojson_tile.hpp> +#include <mbgl/tile/tile_loader_impl.hpp> + +#include <mbgl/util/default_thread_pool.hpp> +#include <mbgl/util/run_loop.hpp> +#include <mbgl/map/transform.hpp> +#include <mbgl/style/style.hpp> +#include <mbgl/style/update_parameters.hpp> +#include <mbgl/style/layers/circle_layer.hpp> +#include <mbgl/annotation/annotation_manager.hpp> + +#include <memory> + +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<CircleLayer>("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<int16_t> features; + features.push_back(mapbox::geometry::feature<int16_t> { + mapbox::geometry::point<int16_t>(0, 0) + }); + + tile.updateData(features); + while (!tile.isComplete()) { + test.loop.runOnce(); + } + + tile.updateData(features); + while (!tile.isComplete()) { + test.loop.runOnce(); + } +} |