Merge pull request #1539 from mapbox/1539-figure_out_deps_first

Figure out Glyph PBF dependencies in the first parsing step

4 files changed, 37 insertions, 56 deletions

diff --git a/src/mbgl/map/tile_parser.cpp b/src/mbgl/map/tile_parser.cpp
index 388950491c..a932453136 100644
--- a/src/mbgl/map/tile_parser.cpp
+++ b/src/mbgl/map/tile_parser.cpp
@@ -118,18 +118,7 @@ std::unique_ptr<Bucket> TileParser::createBucket(const StyleBucket &bucketDesc)
         } else if (bucketDesc.type == StyleLayerType::Line) {
             return createLineBucket(*layer, bucketDesc);
         } else if (bucketDesc.type == StyleLayerType::Symbol) {
-            if (partialParse) {
-                return nullptr;
-            }
-
-            bool needsResources = false;
-            auto symbolBucket = createSymbolBucket(*layer, bucketDesc, needsResources);
-            if (needsResources) {
-                partialParse = true;
-                return nullptr;
-            } else {
-                return std::move(symbolBucket);
-            }
+            return createSymbolBucket(*layer, bucketDesc);
         } else if (bucketDesc.type == StyleLayerType::Raster) {
             return nullptr;
         } else {
@@ -190,13 +179,7 @@ std::unique_ptr<Bucket> TileParser::createLineBucket(const GeometryTileLayer& la
 }
 
 std::unique_ptr<Bucket> TileParser::createSymbolBucket(const GeometryTileLayer& layer,
-                                                       const StyleBucket& bucket_desc,
-                                                       bool& needsResources) {
-    if (!sprite->isLoaded()) {
-        needsResources = true;
-        return nullptr;
-    }
-
+                                                       const StyleBucket& bucket_desc) {
     auto bucket = util::make_unique<SymbolBucket>(*tile.getCollision());
 
     const float z = tile.id.z;
@@ -240,14 +223,21 @@ std::unique_ptr<Bucket> TileParser::createSymbolBucket(const GeometryTileLayer&
     applyLayoutProperty(PropertyKey::TextOffset, bucket_desc.layout, layout.text.offset, z);
     applyLayoutProperty(PropertyKey::TextAllowOverlap, bucket_desc.layout, layout.text.allow_overlap, z);
 
-    bucket->addFeatures(
-        layer, bucket_desc.filter, reinterpret_cast<uintptr_t>(&tile), spriteAtlas, *sprite, glyphAtlas, glyphStore);
+    if (bucket->needsDependencies(layer, bucket_desc.filter, glyphStore, *sprite)) {
+        partialParse = true;
+    }
 
-    if (bucket->needsGlyphs()) {
-        needsResources = true;
+    // We do not proceed if the parser is in a "partial" state because
+    // the layer ordering needs to be respected when calculating text
+    // collisions. Although, at this point, we requested all the resources
+    // needed by this tile.
+    if (partialParse) {
         return nullptr;
     }
 
+    bucket->addFeatures(
+        reinterpret_cast<uintptr_t>(&tile), spriteAtlas, *sprite, glyphAtlas, glyphStore);
+
     return bucket->hasData() ? std::move(bucket) : nullptr;
 }
 
diff --git a/src/mbgl/map/tile_parser.hpp b/src/mbgl/map/tile_parser.hpp
index 67150bcd4d..88e5687804 100644
--- a/src/mbgl/map/tile_parser.hpp
+++ b/src/mbgl/map/tile_parser.hpp
@@ -53,7 +53,7 @@ private:
     std::unique_ptr<Bucket> createBucket(const StyleBucket&);
     std::unique_ptr<Bucket> createFillBucket(const GeometryTileLayer&, const StyleBucket&);
     std::unique_ptr<Bucket> createLineBucket(const GeometryTileLayer&, const StyleBucket&);
-    std::unique_ptr<Bucket> createSymbolBucket(const GeometryTileLayer&, const StyleBucket&, bool& needsResources);
+    std::unique_ptr<Bucket> createSymbolBucket(const GeometryTileLayer&, const StyleBucket&);
 
     template <class Bucket>
     void addBucketGeometries(Bucket&, const GeometryTileLayer&, const FilterExpression&);
diff --git a/src/mbgl/renderer/symbol_bucket.cpp b/src/mbgl/renderer/symbol_bucket.cpp
index 03de3d8a23..4d957a1e50 100644
--- a/src/mbgl/renderer/symbol_bucket.cpp
+++ b/src/mbgl/renderer/symbol_bucket.cpp
@@ -29,7 +29,7 @@
 namespace mbgl {
 
 SymbolBucket::SymbolBucket(Collision &collision_)
-    : collision(collision_), needsGlyphs_(false) {
+    : collision(collision_) {
 }
 
 SymbolBucket::~SymbolBucket() {
@@ -62,16 +62,15 @@ bool SymbolBucket::hasTextData() const { return !text.groups.empty(); }
 
 bool SymbolBucket::hasIconData() const { return !icon.groups.empty(); }
 
-std::vector<SymbolFeature> SymbolBucket::processFeatures(const GeometryTileLayer& layer,
-                                                         const FilterExpression& filter,
-                                                         GlyphStore &glyphStore) {
+bool SymbolBucket::needsDependencies(const GeometryTileLayer& layer,
+                                     const FilterExpression& filter,
+                                     GlyphStore& glyphStore,
+                                     Sprite& sprite) {
     const bool has_text = !layout.text.field.empty() && !layout.text.font.empty();
     const bool has_icon = !layout.icon.image.empty();
 
-    std::vector<SymbolFeature> features;
-
     if (!has_text && !has_icon) {
-        return features;
+        return false;
     }
 
     // Determine and load glyph ranges
@@ -135,28 +134,21 @@ std::vector<SymbolFeature> SymbolBucket::processFeatures(const GeometryTileLayer
     }
 
     if (glyphStore.requestGlyphRangesIfNeeded(layout.text.font, ranges)) {
-        needsGlyphs_ = true;
-        return {};
+        return true;
+    }
+
+    if (!sprite.isLoaded()) {
+        return true;
     }
 
-    return features;
+    return false;
 }
 
-void SymbolBucket::addFeatures(const GeometryTileLayer& layer,
-                               const FilterExpression& filter,
-                               uintptr_t tileUID,
+void SymbolBucket::addFeatures(uintptr_t tileUID,
                                SpriteAtlas& spriteAtlas,
                                Sprite& sprite,
                                GlyphAtlas& glyphAtlas,
                                GlyphStore& glyphStore) {
-    const std::vector<SymbolFeature> features = processFeatures(layer, filter, glyphStore);
-
-    // Stop if we still need glyphs because the
-    // bucket will be discarded.
-    if (needsGlyphs()) {
-        return;
-    }
-
     float horizontalAlign = 0.5;
     float verticalAlign = 0.5;
 
@@ -244,6 +236,8 @@ void SymbolBucket::addFeatures(const GeometryTileLayer& layer,
             }
         }
     }
+
+    features.clear();
 }
 
 bool byScale(const Anchor &a, const Anchor &b) { return a.scale < b.scale; }
diff --git a/src/mbgl/renderer/symbol_bucket.hpp b/src/mbgl/renderer/symbol_bucket.hpp
index 9c67d66a52..d0b133bc77 100644
--- a/src/mbgl/renderer/symbol_bucket.hpp
+++ b/src/mbgl/renderer/symbol_bucket.hpp
@@ -61,25 +61,22 @@ public:
     bool hasTextData() const;
     bool hasIconData() const;
 
-    void addFeatures(const GeometryTileLayer&,
-                     const FilterExpression&,
-                     uintptr_t tileUID,
+    void addFeatures(uintptr_t tileUID,
                      SpriteAtlas&,
                      Sprite&,
                      GlyphAtlas&,
                      GlyphStore&);
 
-    inline bool needsGlyphs() const { return needsGlyphs_; }
-
     void drawGlyphs(SDFShader& shader);
     void drawIcons(SDFShader& shader);
     void drawIcons(IconShader& shader);
 
-private:
-    std::vector<SymbolFeature> processFeatures(const GeometryTileLayer&,
-                                               const FilterExpression&,
-                                               GlyphStore&);
+    bool needsDependencies(const GeometryTileLayer&,
+                           const FilterExpression&,
+                           GlyphStore&,
+                           Sprite&);
 
+private:
     void addFeature(const std::vector<Coordinate> &line, const Shaping &shaping, const GlyphPositions &face, const Rect<uint16_t> &image);
 
     // Adds placed items to the buffer.
@@ -92,6 +89,7 @@ public:
 
 private:
     Collision &collision;
+    std::vector<SymbolFeature> features;
 
     struct TextBuffer {
         TextVertexBuffer vertices;
@@ -104,9 +102,8 @@ private:
         TriangleElementsBuffer triangles;
         std::vector<std::unique_ptr<IconElementGroup>> groups;
     } icon;
-
-    bool needsGlyphs_;
 };
+
 }
 
 #endif