From e44db93f1cb3276dcdc7de8400ca96beda1b1d30 Mon Sep 17 00:00:00 2001
From: Ansis Brammanis <brammanis@gmail.com>
Date: Wed, 13 Jan 2016 21:39:42 -0800
Subject: [core] place symbol layers in correct order

Previously, the placement of symbols depended on the order in which the
glyphs they need were loaded. The placement order should be based on the
style.

To fix this, symbol layers are placed in `placementPending` after they
are parsed. After all layers are parsed, symbol layers (stored in
`placementPending`) are placed and returned.
---
 src/mbgl/layer/symbol_layer.cpp            |  3 +-
 src/mbgl/map/tile_worker.cpp               | 46 +++++++++++++++++++++---------
 src/mbgl/map/tile_worker.hpp               |  8 ++++--
 src/mbgl/map/vector_tile_data.cpp          |  6 +++-
 src/mbgl/renderer/symbol_bucket.cpp        | 11 +------
 src/mbgl/renderer/symbol_bucket.hpp        |  4 +--
 src/mbgl/style/style_bucket_parameters.hpp |  3 --
 src/mbgl/util/worker.cpp                   |  6 ++--
 src/mbgl/util/worker.hpp                   |  1 +
 9 files changed, 51 insertions(+), 37 deletions(-)

(limited to 'src')

diff --git a/src/mbgl/layer/symbol_layer.cpp b/src/mbgl/layer/symbol_layer.cpp
index e05b27bffc..77254d6f2b 100644
--- a/src/mbgl/layer/symbol_layer.cpp
+++ b/src/mbgl/layer/symbol_layer.cpp
@@ -177,8 +177,7 @@ std::unique_ptr<Bucket> SymbolLayer::createBucket(StyleBucketParameters& paramet
         bucket->addFeatures(parameters.tileUID,
                             *spriteAtlas,
                             parameters.glyphAtlas,
-                            parameters.glyphStore,
-                            parameters.collisionTile);
+                            parameters.glyphStore);
     }
 
     return std::move(bucket);
diff --git a/src/mbgl/map/tile_worker.cpp b/src/mbgl/map/tile_worker.cpp
index f483082cfc..a7f733eed9 100644
--- a/src/mbgl/map/tile_worker.cpp
+++ b/src/mbgl/map/tile_worker.cpp
@@ -41,14 +41,12 @@ TileParseResult TileWorker::parseAllLayers(std::vector<std::unique_ptr<StyleLaye
                                            PlacementConfig config) {
     // We're doing a fresh parse of the tile, because the underlying data has changed.
     pending.clear();
+    placementPending.clear();
     partialParse = false;
 
     // Store the layers for use in redoPlacement.
     layers = std::move(layers_);
 
-    // Reset the collision tile so we have a clean slate; we're placing all features anyway.
-    collisionTile = std::make_unique<CollisionTile>(config);
-
     // We're storing a set of bucket names we've parsed to avoid parsing a bucket twice that is
     // referenced from more than one layer
     std::set<std::string> parsed;
@@ -62,10 +60,15 @@ TileParseResult TileWorker::parseAllLayers(std::vector<std::unique_ptr<StyleLaye
     }
 
     result.state = pending.empty() ? TileData::State::parsed : TileData::State::partial;
+
+    if (result.state == TileData::State::parsed) {
+        placeLayers(config);
+    }
+
     return std::move(result);
 }
 
-TileParseResult TileWorker::parsePendingLayers() {
+TileParseResult TileWorker::parsePendingLayers(const PlacementConfig config) {
     // Try parsing the remaining layers that we couldn't parse in the first step due to missing
     // dependencies.
     for (auto it = pending.begin(); it != pending.end();) {
@@ -77,9 +80,8 @@ TileParseResult TileWorker::parsePendingLayers() {
             bucket->addFeatures(reinterpret_cast<uintptr_t>(this),
                                 *layer.spriteAtlas,
                                 glyphAtlas,
-                                glyphStore,
-                                *collisionTile);
-            insertBucket(layer.bucketName(), std::move(it->second));
+                                glyphStore);
+            placementPending.emplace(layer.bucketName(), std::move(it->second));
             pending.erase(it++);
             continue;
         }
@@ -89,20 +91,33 @@ TileParseResult TileWorker::parsePendingLayers() {
     }
 
     result.state = pending.empty() ? TileData::State::parsed : TileData::State::partial;
+
+    if (result.state == TileData::State::parsed) {
+        placeLayers(config);
+    }
+
     return std::move(result);
 }
 
+void TileWorker::placeLayers(const PlacementConfig config) {
+    redoPlacement(&placementPending, config);
+    for (auto &p : placementPending) {
+        p.second->swapRenderData();
+        insertBucket(p.first, std::move(p.second));
+    }
+    placementPending.clear();
+}
+
 void TileWorker::redoPlacement(
     const std::unordered_map<std::string, std::unique_ptr<Bucket>>* buckets,
     PlacementConfig config) {
 
-    // Reset the collision tile so we have a clean slate; we're placing all features anyway.
-    collisionTile = std::make_unique<CollisionTile>(config);
+    CollisionTile collisionTile(config);
 
     for (auto i = layers.rbegin(); i != layers.rend(); i++) {
         const auto it = buckets->find((*i)->id);
         if (it != buckets->end()) {
-            it->second->placeFeatures(*collisionTile);
+            it->second->placeFeatures(collisionTile);
         }
     }
 }
@@ -142,14 +157,17 @@ void TileWorker::parseLayer(const StyleLayer* layer, const GeometryTile& geometr
                                      spriteStore,
                                      glyphAtlas,
                                      glyphStore,
-                                     *collisionTile,
                                      mode);
 
     std::unique_ptr<Bucket> bucket = layer->createBucket(parameters);
 
-    if (layer->is<SymbolLayer>() && partialParse) {
-        // We cannot parse this bucket yet. Instead, we're saving it for later.
-        pending.emplace_back(layer->as<SymbolLayer>(), std::move(bucket));
+    if (layer->is<SymbolLayer>()) {
+        if (partialParse) {
+            // We cannot parse this bucket yet. Instead, we're saving it for later.
+            pending.emplace_back(layer->as<SymbolLayer>(), std::move(bucket));
+        } else {
+            placementPending.emplace(layer->bucketName(), std::move(bucket));
+        }
     } else {
         insertBucket(layer->bucketName(), std::move(bucket));
     }
diff --git a/src/mbgl/map/tile_worker.hpp b/src/mbgl/map/tile_worker.hpp
index 74894a9eb8..a5e892e2b6 100644
--- a/src/mbgl/map/tile_worker.hpp
+++ b/src/mbgl/map/tile_worker.hpp
@@ -53,7 +53,7 @@ public:
                                    const GeometryTile&,
                                    PlacementConfig);
 
-    TileParseResult parsePendingLayers();
+    TileParseResult parsePendingLayers(PlacementConfig);
 
     void redoPlacement(const std::unordered_map<std::string, std::unique_ptr<Bucket>>*,
                        PlacementConfig);
@@ -61,6 +61,7 @@ public:
 private:
     void parseLayer(const StyleLayer*, const GeometryTile&);
     void insertBucket(const std::string& name, std::unique_ptr<Bucket>);
+    void placeLayers(PlacementConfig);
 
     const TileID id;
     const std::string sourceID;
@@ -74,12 +75,15 @@ private:
     bool partialParse = false;
 
     std::vector<std::unique_ptr<StyleLayer>> layers;
-    std::unique_ptr<CollisionTile> collisionTile;
 
     // Contains buckets that we couldn't parse so far due to missing resources.
     // They will be attempted on subsequent parses.
     std::list<std::pair<const SymbolLayer*, std::unique_ptr<Bucket>>> pending;
 
+    // Contains buckets that have been parsed, but still need placement.
+    // They will be placed when all buckets have been parsed.
+    std::unordered_map<std::string, std::unique_ptr<Bucket>> placementPending;
+
     // Temporary holder
     TileParseResultBuckets result;
 };
diff --git a/src/mbgl/map/vector_tile_data.cpp b/src/mbgl/map/vector_tile_data.cpp
index 048cadc987..f44dc75982 100644
--- a/src/mbgl/map/vector_tile_data.cpp
+++ b/src/mbgl/map/vector_tile_data.cpp
@@ -102,7 +102,7 @@ bool VectorTileData::parsePending(std::function<void()> callback) {
     }
 
     workRequest.reset();
-    workRequest = worker.parsePendingGeometryTileLayers(tileWorker, [this, callback] (TileParseResult result) {
+    workRequest = worker.parsePendingGeometryTileLayers(tileWorker, targetConfig, [this, callback, config = targetConfig] (TileParseResult result) {
         workRequest.reset();
         if (state == State::obsolete) {
             return;
@@ -118,6 +118,10 @@ bool VectorTileData::parsePending(std::function<void()> callback) {
                 buckets[bucket.first] = std::move(bucket.second);
             }
 
+            // Persist the configuration we just placed so that we can later check whether we need to
+            // place again in case the configuration has changed.
+            placedConfig = config;
+
             // The target configuration could have changed since we started placement. In this case,
             // we're starting another placement run.
             if (placedConfig != targetConfig) {
diff --git a/src/mbgl/renderer/symbol_bucket.cpp b/src/mbgl/renderer/symbol_bucket.cpp
index 67539637bc..a34cbcd028 100644
--- a/src/mbgl/renderer/symbol_bucket.cpp
+++ b/src/mbgl/renderer/symbol_bucket.cpp
@@ -173,8 +173,7 @@ bool SymbolBucket::needsDependencies(GlyphStore& glyphStore, SpriteStore& sprite
 void SymbolBucket::addFeatures(uintptr_t tileUID,
                                SpriteAtlas& spriteAtlas,
                                GlyphAtlas& glyphAtlas,
-                               GlyphStore& glyphStore,
-                               CollisionTile& collisionTile) {
+                               GlyphStore& glyphStore) {
     float horizontalAlign = 0.5;
     float verticalAlign = 0.5;
 
@@ -266,8 +265,6 @@ void SymbolBucket::addFeatures(uintptr_t tileUID,
     }
 
     features.clear();
-
-    placeFeatures(collisionTile, true);
 }
 
 
@@ -358,10 +355,6 @@ bool SymbolBucket::anchorIsTooClose(const std::u32string &text, const float repe
 }
 
 void SymbolBucket::placeFeatures(CollisionTile& collisionTile) {
-    placeFeatures(collisionTile, false);
-}
-
-void SymbolBucket::placeFeatures(CollisionTile& collisionTile, bool swapImmediately) {
 
     renderDataInProgress = std::make_unique<SymbolRenderData>();
 
@@ -448,8 +441,6 @@ void SymbolBucket::placeFeatures(CollisionTile& collisionTile, bool swapImmediat
     if (collisionTile.config.debug) {
         addToDebugBuffers(collisionTile);
     }
-
-    if (swapImmediately) swapRenderData();
 }
 
 template <typename Buffer, typename GroupType>
diff --git a/src/mbgl/renderer/symbol_bucket.hpp b/src/mbgl/renderer/symbol_bucket.hpp
index 4f52f2300b..68a622bb8a 100644
--- a/src/mbgl/renderer/symbol_bucket.hpp
+++ b/src/mbgl/renderer/symbol_bucket.hpp
@@ -79,8 +79,7 @@ public:
     void addFeatures(uintptr_t tileUID,
                      SpriteAtlas&,
                      GlyphAtlas&,
-                     GlyphStore&,
-                     CollisionTile&);
+                     GlyphStore&);
 
     void drawGlyphs(SDFShader& shader);
     void drawIcons(SDFShader& shader);
@@ -101,7 +100,6 @@ private:
     
     void addToDebugBuffers(CollisionTile &collisionTile);
 
-    void placeFeatures(CollisionTile& collisionTile, bool swapImmediately);
     void swapRenderData() override;
 
     // Adds placed items to the buffer.
diff --git a/src/mbgl/style/style_bucket_parameters.hpp b/src/mbgl/style/style_bucket_parameters.hpp
index 732ebade1d..3329cac032 100644
--- a/src/mbgl/style/style_bucket_parameters.hpp
+++ b/src/mbgl/style/style_bucket_parameters.hpp
@@ -27,7 +27,6 @@ public:
                           SpriteStore& spriteStore_,
                           GlyphAtlas& glyphAtlas_,
                           GlyphStore& glyphStore_,
-                          CollisionTile& collisionTile_,
                           const MapMode mode_)
         : tileID(tileID_),
           layer(layer_),
@@ -37,7 +36,6 @@ public:
           spriteStore(spriteStore_),
           glyphAtlas(glyphAtlas_),
           glyphStore(glyphStore_),
-          collisionTile(collisionTile_),
           mode(mode_) {}
 
     bool cancelled() const {
@@ -54,7 +52,6 @@ public:
     SpriteStore& spriteStore;
     GlyphAtlas& glyphAtlas;
     GlyphStore& glyphStore;
-    CollisionTile& collisionTile;
     const MapMode mode;
 };
 
diff --git a/src/mbgl/util/worker.cpp b/src/mbgl/util/worker.cpp
index 49d1c2bc5b..d1e54fa375 100644
--- a/src/mbgl/util/worker.cpp
+++ b/src/mbgl/util/worker.cpp
@@ -39,9 +39,10 @@ public:
     }
 
     void parsePendingGeometryTileLayers(TileWorker* worker,
+                                        PlacementConfig config,
                                         std::function<void(TileParseResult)> callback) {
         try {
-            callback(worker->parsePendingLayers());
+            callback(worker->parsePendingLayers(config));
         } catch (...) {
             callback(std::current_exception());
         }
@@ -87,10 +88,11 @@ Worker::parseGeometryTile(TileWorker& worker,
 
 std::unique_ptr<WorkRequest>
 Worker::parsePendingGeometryTileLayers(TileWorker& worker,
+                                       PlacementConfig config,
                                        std::function<void(TileParseResult)> callback) {
     current = (current + 1) % threads.size();
     return threads[current]->invokeWithCallback(&Worker::Impl::parsePendingGeometryTileLayers,
-                                                callback, &worker);
+                                                callback, &worker, config);
 }
 
 std::unique_ptr<WorkRequest>
diff --git a/src/mbgl/util/worker.hpp b/src/mbgl/util/worker.hpp
index d4660d9e7c..d72438bc75 100644
--- a/src/mbgl/util/worker.hpp
+++ b/src/mbgl/util/worker.hpp
@@ -46,6 +46,7 @@ public:
                               std::function<void(TileParseResult)> callback);
 
     Request parsePendingGeometryTileLayers(TileWorker&,
+                                           PlacementConfig config,
                                            std::function<void(TileParseResult)> callback);
 
     Request redoPlacement(TileWorker&,
-- 
cgit v1.2.1