Merge pull request #1532 from mapbox/1465-text_collision

Calculate text collision correctly

11 files changed, 82 insertions, 40 deletions

diff --git a/src/mbgl/map/live_tile_data.cpp b/src/mbgl/map/live_tile_data.cpp
index 096e899d8f..59f7b4de3d 100644
--- a/src/mbgl/map/live_tile_data.cpp
+++ b/src/mbgl/map/live_tile_data.cpp
@@ -21,7 +21,7 @@ LiveTileData::LiveTileData(const TileID& id_,
                                      spriteAtlas_, sprite_, source_),
       annotationManager(annotationManager_) {
     // live features are always ready
-    state = State::loaded;
+    setState(State::loaded);
 }
 
 LiveTileData::~LiveTileData() {
@@ -31,7 +31,7 @@ LiveTileData::~LiveTileData() {
 }
 
 void LiveTileData::parse() {
-    if (state != State::loaded) {
+    if (getState() != State::loaded) {
         return;
     }
 
@@ -46,12 +46,12 @@ void LiveTileData::parse() {
             parser.parse();
         } catch (const std::exception& ex) {
             Log::Error(Event::ParseTile, "Live-parsing [%d/%d/%d] failed: %s", id.z, id.x, id.y, ex.what());
-            state = State::obsolete;
+            setState(State::obsolete);
             return;
         }
     }
 
-    if (state != State::obsolete) {
-        state = State::parsed;
+    if (getState() != State::obsolete) {
+        setState(State::parsed);
     }
 }
diff --git a/src/mbgl/map/raster_tile_data.cpp b/src/mbgl/map/raster_tile_data.cpp
index 20c07b573d..0f8efe3b48 100644
--- a/src/mbgl/map/raster_tile_data.cpp
+++ b/src/mbgl/map/raster_tile_data.cpp
@@ -15,14 +15,14 @@ RasterTileData::~RasterTileData() {
 }
 
 void RasterTileData::parse() {
-    if (state != State::loaded) {
+    if (getState() != State::loaded) {
         return;
     }
 
     if (bucket.setImage(data)) {
-        state = State::parsed;
+        setState(State::parsed);
     } else {
-        state = State::invalid;
+        setState(State::invalid);
     }
 }
 
diff --git a/src/mbgl/map/source.cpp b/src/mbgl/map/source.cpp
index 8b0bf905ff..e663ef3d58 100644
--- a/src/mbgl/map/source.cpp
+++ b/src/mbgl/map/source.cpp
@@ -127,7 +127,7 @@ bool Source::isLoaded() const {
     }
 
     for (const auto& tile : tiles) {
-        if (tile.second->data->state != TileData::State::parsed) {
+        if (tile.second->data->getState() != TileData::State::parsed) {
             return false;
         }
     }
@@ -193,11 +193,11 @@ void Source::finishRender(Painter &painter) {
     }
 }
 
-std::forward_list<Tile *> Source::getLoadedTiles() const {
-    std::forward_list<Tile *> ptrs;
+std::forward_list<Tile*> Source::getLoadedTiles() const {
+    std::forward_list<Tile*> ptrs;
     auto it = ptrs.before_begin();
-    for (const auto &pair : tiles) {
-        if (pair.second->data->ready()) {
+    for (const auto& pair : tiles) {
+        if (pair.second->data->isReady()) {
             it = ptrs.insert_after(it, pair.second.get());
         }
     }
@@ -211,16 +211,16 @@ const std::vector<Tile*>& Source::getTiles() const {
 TileData::State Source::hasTile(const TileID& id) {
     auto it = tiles.find(id);
     if (it != tiles.end()) {
-        Tile &tile = *it->second;
+        Tile& tile = *it->second;
         if (tile.id == id && tile.data) {
-            return tile.data->state;
+            return tile.data->getState();
         }
     }
 
     return TileData::State::invalid;
 }
 
-void Source::handlePartialTile(const TileID &id, Worker &worker) {
+void Source::handlePartialTile(const TileID& id, Worker& worker) {
     const TileID normalized_id = id.normalized();
 
     auto it = tile_data.find(normalized_id);
@@ -265,7 +265,7 @@ TileData::State Source::addTile(MapData& data,
         new_tile.data = it->second.lock();
     }
 
-    if (new_tile.data && new_tile.data->state == TileData::State::obsolete) {
+    if (new_tile.data && new_tile.data->getState() == TileData::State::obsolete) {
         // Do not consider the tile if it's already obsolete.
         new_tile.data.reset();
     }
@@ -296,7 +296,7 @@ TileData::State Source::addTile(MapData& data,
         tile_data.emplace(new_tile.data->id, new_tile.data);
     }
 
-    return new_tile.data->state;
+    return new_tile.data->getState();
 }
 
 double Source::getZoom(const TransformState& state) const {
@@ -453,7 +453,7 @@ void Source::update(MapData& data,
         bool obsolete = std::find(retain.begin(), retain.end(), tile.id) == retain.end();
         if (!obsolete) {
             retain_data.insert(tile.data->id);
-        } else if (type != SourceType::Raster && tile.data->state == TileData::State::parsed) {
+        } else if (type != SourceType::Raster && tile.data->getState() == TileData::State::parsed) {
             // Partially parsed tiles are never added to the cache because otherwise
             // they never get updated if the go out from the viewport and the pending
             // resources arrive.
diff --git a/src/mbgl/map/tile_cache.cpp b/src/mbgl/map/tile_cache.cpp
index 25aed6d15e..37acf628a0 100644
--- a/src/mbgl/map/tile_cache.cpp
+++ b/src/mbgl/map/tile_cache.cpp
@@ -46,7 +46,7 @@ std::shared_ptr<TileData> TileCache::get(uint64_t key) {
         data = it->second;
         tiles.erase(it);
         orderedKeys.remove(key);
-        assert(data->ready());
+        assert(data->isReady());
     }
 
     return data;
diff --git a/src/mbgl/map/tile_data.cpp b/src/mbgl/map/tile_data.cpp
index 51ae37fd1c..aa66ceb80b 100644
--- a/src/mbgl/map/tile_data.cpp
+++ b/src/mbgl/map/tile_data.cpp
@@ -12,9 +12,9 @@ using namespace mbgl;
 TileData::TileData(const TileID& id_, const SourceInfo& source_)
     : id(id_),
       name(id),
-      state(State::initial),
       source(source_),
       env(Environment::Get()),
+      state(State::initial),
       debugBucket(debugFontBuffer) {
     // Initialize tile debug coordinates
     debugFontBuffer.addText(name.c_str(), 50, 200, 5);
@@ -28,6 +28,12 @@ const std::string TileData::toString() const {
     return std::string { "[tile " } + name + "]";
 }
 
+void TileData::setState(const State& state_) {
+    assert(!isImmutable());
+
+    state = state_;
+}
+
 void TileData::request(Worker& worker, float pixelRatio, std::function<void()> callback) {
     std::string url = source.tileURL(id, pixelRatio);
     state = State::loading;
diff --git a/src/mbgl/map/tile_data.hpp b/src/mbgl/map/tile_data.hpp
index 57f7c30ff3..7f16e7c413 100644
--- a/src/mbgl/map/tile_data.hpp
+++ b/src/mbgl/map/tile_data.hpp
@@ -49,10 +49,24 @@ public:
     void cancel();
     const std::string toString() const;
 
-    inline bool ready() const {
+    inline bool isReady() const {
         return isReadyState(state);
     }
 
+    // Returns true if the TileData is in a final state and we cannot
+    // make changes to it anymore.
+    inline bool isImmutable() const {
+        return state == State::parsed || state == State::obsolete;
+    }
+
+    // We let subclasses override setState() so they
+    // can intercept the state change and react accordingly.
+    virtual void setState(const State& state);
+
+    inline State getState() const {
+        return state;
+    }
+
     void endParsing();
 
     // Override this in the child class.
@@ -61,7 +75,6 @@ public:
 
     const TileID id;
     const std::string name;
-    std::atomic<State> state;
     std::atomic_flag parsing = ATOMIC_FLAG_INIT;
 
 protected:
@@ -81,6 +94,10 @@ protected:
 
     std::unique_ptr<WorkRequest> workRequest;
 
+private:
+    std::atomic<State> state;
+
+protected:
     // Contains the tile ID string for painting debug information.
     DebugFontBuffer debugFontBuffer;
 
diff --git a/src/mbgl/map/tile_parser.cpp b/src/mbgl/map/tile_parser.cpp
index 65b445a6cf..388950491c 100644
--- a/src/mbgl/map/tile_parser.cpp
+++ b/src/mbgl/map/tile_parser.cpp
@@ -8,7 +8,6 @@
 #include <mbgl/renderer/line_bucket.hpp>
 #include <mbgl/renderer/symbol_bucket.hpp>
 #include <mbgl/util/constants.hpp>
-#include <mbgl/text/collision.hpp>
 #include <mbgl/util/std.hpp>
 #include <mbgl/style/style.hpp>
 
@@ -35,13 +34,13 @@ TileParser::TileParser(const GeometryTile& geometryTile_,
       glyphStore(glyphStore_),
       spriteAtlas(spriteAtlas_),
       sprite(sprite_),
-      collision(util::make_unique<Collision>(tile.id.z, 4096, tile.source.tile_size, tile.depth)),
       partialParse(false) {
     assert(sprite);
-    assert(collision);
 }
 
-bool TileParser::obsolete() const { return tile.state == TileData::State::obsolete; }
+bool TileParser::obsolete() const {
+    return tile.getState() == TileData::State::obsolete;
+}
 
 void TileParser::parse() {
     for (const auto& layer_desc : style.layers) {
@@ -119,6 +118,10 @@ 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) {
@@ -194,7 +197,7 @@ std::unique_ptr<Bucket> TileParser::createSymbolBucket(const GeometryTileLayer&
         return nullptr;
     }
 
-    auto bucket = util::make_unique<SymbolBucket>(*collision);
+    auto bucket = util::make_unique<SymbolBucket>(*tile.getCollision());
 
     const float z = tile.id.z;
     auto& layout = bucket->layout;
diff --git a/src/mbgl/map/tile_parser.hpp b/src/mbgl/map/tile_parser.hpp
index 18184e27a3..67150bcd4d 100644
--- a/src/mbgl/map/tile_parser.hpp
+++ b/src/mbgl/map/tile_parser.hpp
@@ -29,7 +29,6 @@ class StyleLayoutRaster;
 class StyleLayoutLine;
 class StyleLayoutSymbol;
 class VectorTileData;
-class Collision;
 
 class TileParser : private util::noncopyable {
 public:
@@ -69,7 +68,6 @@ private:
     SpriteAtlas& spriteAtlas;
     util::ptr<Sprite> sprite;
 
-    std::unique_ptr<Collision> collision;
     bool partialParse;
 };
 
diff --git a/src/mbgl/map/vector_tile_data.cpp b/src/mbgl/map/vector_tile_data.cpp
index 7560168ed7..263ad52b9d 100644
--- a/src/mbgl/map/vector_tile_data.cpp
+++ b/src/mbgl/map/vector_tile_data.cpp
@@ -6,6 +6,7 @@
 #include <mbgl/map/source.hpp>
 #include <mbgl/geometry/glyph_atlas.hpp>
 #include <mbgl/platform/log.hpp>
+#include <mbgl/text/collision.hpp>
 #include <mbgl/util/pbf.hpp>
 
 using namespace mbgl;
@@ -19,12 +20,13 @@ VectorTileData::VectorTileData(const TileID& id_,
                                util::ptr<Sprite> sprite_,
                                const SourceInfo& source_)
     : TileData(id_, source_),
+      depth(id_.z >= source_.max_zoom ? mapMaxZoom - id_.z : 1),
       glyphAtlas(glyphAtlas_),
       glyphStore(glyphStore_),
       spriteAtlas(spriteAtlas_),
       sprite(sprite_),
       style(style_),
-      depth(id.z >= source.max_zoom ? mapMaxZoom - id.z : 1) {
+      collision(util::make_unique<Collision>(id_.z, 4096, source_.tile_size, depth)) {
 }
 
 VectorTileData::~VectorTileData() {
@@ -35,7 +37,7 @@ VectorTileData::~VectorTileData() {
 }
 
 void VectorTileData::parse() {
-    if (state != State::loaded && state != State::partial) {
+    if (getState() != State::loaded && getState() != State::partial) {
         return;
     }
 
@@ -48,24 +50,24 @@ void VectorTileData::parse() {
         TileParser parser(*vt, *this, style, glyphAtlas, glyphStore, spriteAtlas, sprite);
         parser.parse();
 
-        if (state == State::obsolete) {
+        if (getState() == State::obsolete) {
             return;
         }
 
         if (parser.isPartialParse()) {
-            state = State::partial;
+            setState(State::partial);
         } else {
-            state = State::parsed;
+            setState(State::parsed);
         }
     } catch (const std::exception& ex) {
         Log::Error(Event::ParseTile, "Parsing [%d/%d/%d] failed: %s", id.z, id.x, id.y, ex.what());
-        state = State::obsolete;
+        setState(State::obsolete);
         return;
     }
 }
 
 Bucket* VectorTileData::getBucket(StyleLayer const& layer) {
-    if (!ready() || !layer.bucket) {
+    if (!isReady() || !layer.bucket) {
         return nullptr;
     }
 
@@ -91,3 +93,11 @@ void VectorTileData::setBucket(StyleLayer const& layer, std::unique_ptr<Bucket>
 
     buckets[layer.bucket->name] = std::move(bucket);
 }
+
+void VectorTileData::setState(const State& state_) {
+    TileData::setState(state_);
+
+    if (isImmutable()) {
+        collision.reset();
+    }
+}
diff --git a/src/mbgl/map/vector_tile_data.hpp b/src/mbgl/map/vector_tile_data.hpp
index 146bc93229..5fe351ca0c 100644
--- a/src/mbgl/map/vector_tile_data.hpp
+++ b/src/mbgl/map/vector_tile_data.hpp
@@ -16,6 +16,7 @@
 namespace mbgl {
 
 class Bucket;
+class Collision;
 class Painter;
 class SourceInfo;
 class StyleLayer;
@@ -45,6 +46,14 @@ public:
 
     void setBucket(StyleLayer const &layer_desc, std::unique_ptr<Bucket> bucket);
 
+    void setState(const State& state) override;
+
+    inline Collision* getCollision() const {
+        return collision.get();
+    }
+
+    const float depth;
+
 protected:
     // Holds the actual geometries in this tile.
     FillVertexBuffer fillVertexBuffer;
@@ -69,8 +78,7 @@ private:
     std::unordered_map<std::string, std::unique_ptr<Bucket>> buckets;
     mutable std::mutex bucketsMutex;
 
-public:
-    const float depth;
+    std::unique_ptr<Collision> collision;
 };
 
 }
diff --git a/src/mbgl/renderer/painter.cpp b/src/mbgl/renderer/painter.cpp
index e2a3df6253..56d814c03a 100644
--- a/src/mbgl/renderer/painter.cpp
+++ b/src/mbgl/renderer/painter.cpp
@@ -381,7 +381,7 @@ std::vector<RenderItem> Painter::determineRenderOrder(const Style& style) {
         const auto& tiles = layer.bucket->source->getTiles();
         for (auto tile : tiles) {
             assert(tile);
-            if (!tile->data && !tile->data->ready()) {
+            if (!tile->data && !tile->data->isReady()) {
                 continue;
             }