[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.
mapbox · Jan 10, 2017 · 73a182a · 73a182a
1 parent d827073
commit 73a182a
Show file tree

Hide file tree

Showing 13 changed files with 123 additions and 26 deletions.
diff --git 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
@@ -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
@@ -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<std::string>& layerIDs);
 
 private:
     void addFeature(

diff --git 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
@@ -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 @@ class SymbolLayout {
 
     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
@@ -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
@@ -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
@@ -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
@@ -52,7 +52,7 @@ class SymbolLayer::Impl : public Layer::Impl {
 
     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
@@ -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
@@ -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
@@ -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
@@ -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();
+    }
+}