[core] mutate style on annotation mutations immediately

9 files changed, 77 insertions, 75 deletions

diff --git a/src/mbgl/annotation/annotation_manager.cpp b/src/mbgl/annotation/annotation_manager.cpp
index a69dba1bf2..ec5a360a90 100644
--- a/src/mbgl/annotation/annotation_manager.cpp
+++ b/src/mbgl/annotation/annotation_manager.cpp
@@ -4,6 +4,7 @@
 #include <mbgl/annotation/symbol_annotation_impl.hpp>
 #include <mbgl/annotation/line_annotation_impl.hpp>
 #include <mbgl/annotation/fill_annotation_impl.hpp>
+#include <mbgl/style/style.hpp>
 #include <mbgl/style/style_impl.hpp>
 #include <mbgl/style/layers/symbol_layer.hpp>
 #include <mbgl/style/layers/symbol_layer_impl.hpp>
@@ -18,9 +19,20 @@ using namespace style;
 const std::string AnnotationManager::SourceID = "com.mapbox.annotations";
 const std::string AnnotationManager::PointLayerID = "com.mapbox.annotations.points";
 
-AnnotationManager::AnnotationManager() = default;
+AnnotationManager::AnnotationManager(Style& style_)
+        : style(style_) {
+};
+
 AnnotationManager::~AnnotationManager() = default;
 
+void AnnotationManager::setStyle(Style& style_) {
+    style = style_;
+}
+
+void AnnotationManager::onStyleLoaded() {
+    updateStyle();
+}
+
 AnnotationID AnnotationManager::addAnnotation(const Annotation& annotation, const uint8_t maxZoom) {
     std::lock_guard<std::mutex> lock(mutex);
     AnnotationID id = nextID++;
@@ -51,13 +63,13 @@ void AnnotationManager::add(const AnnotationID& id, const SymbolAnnotation& anno
 void AnnotationManager::add(const AnnotationID& id, const LineAnnotation& annotation, const uint8_t maxZoom) {
     ShapeAnnotationImpl& impl = *shapeAnnotations.emplace(id,
         std::make_unique<LineAnnotationImpl>(id, annotation, maxZoom)).first->second;
-    obsoleteShapeAnnotationLayers.erase(impl.layerID);
+    impl.updateStyle(*style.get().impl);
 }
 
 void AnnotationManager::add(const AnnotationID& id, const FillAnnotation& annotation, const uint8_t maxZoom) {
     ShapeAnnotationImpl& impl = *shapeAnnotations.emplace(id,
         std::make_unique<FillAnnotationImpl>(id, annotation, maxZoom)).first->second;
-    obsoleteShapeAnnotationLayers.erase(impl.layerID);
+    impl.updateStyle(*style.get().impl);
 }
 
 Update AnnotationManager::update(const AnnotationID& id, const SymbolAnnotation& annotation, const uint8_t maxZoom) {
@@ -71,16 +83,11 @@ Update AnnotationManager::update(const AnnotationID& id, const SymbolAnnotation&
 
     const SymbolAnnotation& existing = it->second->annotation;
 
-    if (existing.geometry != annotation.geometry) {
+    if (existing.geometry != annotation.geometry || existing.icon != annotation.icon) {
         result |= Update::AnnotationData;
-    }
-
-    if (existing.icon != annotation.icon) {
-        result |= Update::AnnotationData | Update::AnnotationStyle;
-    }
 
-    if (result != Update::Nothing) {
-        removeAndAdd(id, annotation, maxZoom);
+        remove(id);
+        add(id, annotation, maxZoom);
     }
 
     return result;
@@ -93,8 +100,9 @@ Update AnnotationManager::update(const AnnotationID& id, const LineAnnotation& a
         return Update::Nothing;
     }
 
-    removeAndAdd(id, annotation, maxZoom);
-    return Update::AnnotationData | Update::AnnotationStyle;
+    shapeAnnotations.erase(it);
+    add(id, annotation, maxZoom);
+    return Update::AnnotationData;
 }
 
 Update AnnotationManager::update(const AnnotationID& id, const FillAnnotation& annotation, const uint8_t maxZoom) {
@@ -104,15 +112,9 @@ Update AnnotationManager::update(const AnnotationID& id, const FillAnnotation& a
         return Update::Nothing;
     }
 
-    removeAndAdd(id, annotation, maxZoom);
-    return Update::AnnotationData | Update::AnnotationStyle;
-}
-
-void AnnotationManager::removeAndAdd(const AnnotationID& id, const Annotation& annotation, const uint8_t maxZoom) {
-    remove(id);
-    Annotation::visit(annotation, [&] (const auto& annotation_) {
-        this->add(id, annotation_, maxZoom);
-    });
+    shapeAnnotations.erase(it);
+    add(id, annotation, maxZoom);
+    return Update::AnnotationData;
 }
 
 void AnnotationManager::remove(const AnnotationID& id) {
@@ -120,8 +122,9 @@ void AnnotationManager::remove(const AnnotationID& id) {
         symbolTree.remove(symbolAnnotations.at(id));
         symbolAnnotations.erase(id);
     } else if (shapeAnnotations.find(id) != shapeAnnotations.end()) {
-        obsoleteShapeAnnotationLayers.insert(shapeAnnotations.at(id)->layerID);
-        shapeAnnotations.erase(id);
+        auto it = shapeAnnotations.find(id);
+        *style.get().impl->removeLayer(it->second->layerID);
+        shapeAnnotations.erase(it);
     } else {
         assert(false); // Should never happen
     }
@@ -149,11 +152,11 @@ std::unique_ptr<AnnotationTileData> AnnotationManager::getTileData(const Canonic
     return tileData;
 }
 
-void AnnotationManager::updateStyle(Style::Impl& style) {
+void AnnotationManager::updateStyle() {
     // Create annotation source, point layer, and point bucket. We do everything via Style::Impl
     // because we don't want annotation mutations to trigger Style::Impl::styleMutated to be set.
-    if (!style.getSource(SourceID)) {
-        style.addSource(std::make_unique<AnnotationSource>());
+    if (!style.get().impl->getSource(SourceID)) {
+        style.get().impl->addSource(std::make_unique<AnnotationSource>());
 
         std::unique_ptr<SymbolLayer> layer = std::make_unique<SymbolLayer>(PointLayerID, SourceID);
 
@@ -162,13 +165,13 @@ void AnnotationManager::updateStyle(Style::Impl& style) {
         layer->setIconAllowOverlap(true);
         layer->setIconIgnorePlacement(true);
 
-        style.addLayer(std::move(layer));
+        style.get().impl->addLayer(std::move(layer));
     }
 
     std::lock_guard<std::mutex> lock(mutex);
 
     for (const auto& shape : shapeAnnotations) {
-        shape.second->updateStyle(style);
+        shape.second->updateStyle(*style.get().impl);
     }
 
     for (const auto& image : images) {
@@ -178,23 +181,8 @@ void AnnotationManager::updateStyle(Style::Impl& style) {
         // of which images need to be added because we don't know if the style is the same
         // instance as in the last updateStyle call. If it's a new style, we need to add all
         // images.)
-        style.addImage(std::make_unique<style::Image>(image.second));
-    }
-
-    for (const auto& layer : obsoleteShapeAnnotationLayers) {
-        if (style.getLayer(layer)) {
-            style.removeLayer(layer);
-        }
-    }
-
-    for (const auto& image : obsoleteImages) {
-        if (style.getImage(image)) {
-            style.removeImage(image);
-        }
+        style.get().impl->addImage(std::make_unique<style::Image>(image.second));
     }
-
-    obsoleteShapeAnnotationLayers.clear();
-    obsoleteImages.clear();
 }
 
 void AnnotationManager::updateData() {
@@ -225,16 +213,17 @@ void AnnotationManager::addImage(std::unique_ptr<style::Image> image) {
     std::lock_guard<std::mutex> lock(mutex);
     const std::string id = prefixedImageID(image->getID());
     images.erase(id);
-    images.emplace(id,
-        style::Image(id, image->getImage().clone(), image->getPixelRatio(), image->isSdf()));
-    obsoleteImages.erase(id);
+    auto inserted = images.emplace(id, style::Image(id, image->getImage().clone(),
+                                                    image->getPixelRatio(), image->isSdf()));
+
+    style.get().impl->addImage(std::make_unique<style::Image>(inserted.first->second));
 }
 
 void AnnotationManager::removeImage(const std::string& id_) {
     std::lock_guard<std::mutex> lock(mutex);
     const std::string id = prefixedImageID(id_);
     images.erase(id);
-    obsoleteImages.insert(id);
+    style.get().impl->removeImage(id);
 }
 
 double AnnotationManager::getTopOffsetPixelsForImage(const std::string& id_) {
diff --git a/src/mbgl/annotation/annotation_manager.hpp b/src/mbgl/annotation/annotation_manager.hpp
index 6906791db7..dee823bc0f 100644
--- a/src/mbgl/annotation/annotation_manager.hpp
+++ b/src/mbgl/annotation/annotation_manager.hpp
@@ -4,7 +4,6 @@
 #include <mbgl/annotation/symbol_annotation_impl.hpp>
 #include <mbgl/style/image.hpp>
 #include <mbgl/map/update.hpp>
-#include <mbgl/style/style.hpp>
 #include <mbgl/util/noncopyable.hpp>
 
 #include <mutex>
@@ -21,9 +20,13 @@ class AnnotationTileData;
 class SymbolAnnotationImpl;
 class ShapeAnnotationImpl;
 
+namespace style {
+class Style;
+} // namespace style
+
 class AnnotationManager : private util::noncopyable {
 public:
-    AnnotationManager();
+    AnnotationManager(style::Style&);
     ~AnnotationManager();
 
     AnnotationID addAnnotation(const Annotation&, const uint8_t maxZoom);
@@ -34,7 +37,9 @@ public:
     void removeImage(const std::string&);
     double getTopOffsetPixelsForImage(const std::string&);
 
-    void updateStyle(style::Style::Impl&);
+    void setStyle(style::Style&);
+    void onStyleLoaded();
+
     void updateData();
 
     void addTile(AnnotationTile&);
@@ -52,12 +57,14 @@ private:
     Update update(const AnnotationID&, const LineAnnotation&, const uint8_t);
     Update update(const AnnotationID&, const FillAnnotation&, const uint8_t);
 
-    void removeAndAdd(const AnnotationID&, const Annotation&, const uint8_t);
-
     void remove(const AnnotationID&);
 
+    void updateStyle();
+
     std::unique_ptr<AnnotationTileData> getTileData(const CanonicalTileID&);
 
+    std::reference_wrapper<style::Style> style;
+
     std::mutex mutex;
 
     AnnotationID nextID = 0;
@@ -73,8 +80,7 @@ private:
     SymbolAnnotationMap symbolAnnotations;
     ShapeAnnotationMap shapeAnnotations;
     ImageMap images;
-    std::unordered_set<std::string> obsoleteShapeAnnotationLayers;
-    std::unordered_set<std::string> obsoleteImages;
+
     std::unordered_set<AnnotationTile*> tiles;
 
     friend class AnnotationTile;
diff --git a/src/mbgl/map/map.cpp b/src/mbgl/map/map.cpp
index 26795f7814..e644f91c4f 100644
--- a/src/mbgl/map/map.cpp
+++ b/src/mbgl/map/map.cpp
@@ -87,11 +87,12 @@ public:
 
     Update updateFlags = Update::Nothing;
 
-    AnnotationManager annotationManager;
     std::unique_ptr<Painter> painter;
     std::unique_ptr<Style> style;
     std::unique_ptr<RenderStyle> renderStyle;
 
+    AnnotationManager annotationManager;
+
     bool cameraMutated = false;
 
     uint8_t prefetchZoomDelta = util::DEFAULT_PREFETCH_ZOOM_DELTA;
@@ -147,6 +148,8 @@ Map::Impl::Impl(Map& map_,
       contextMode(contextMode_),
       pixelRatio(pixelRatio_),
       programCacheDir(std::move(programCacheDir_)),
+      style(std::make_unique<Style>(scheduler, fileSource, pixelRatio)),
+      annotationManager(*style),
       asyncInvalidate([this] {
           if (mode == MapMode::Continuous) {
               backend.invalidate();
@@ -154,7 +157,6 @@ Map::Impl::Impl(Map& map_,
               renderStill();
           }
       }) {
-    style = std::make_unique<Style>(scheduler, fileSource, pixelRatio);
     style->impl->setObserver(this);
 }
 
@@ -217,10 +219,6 @@ void Map::Impl::render(View& view) {
 
     transform.updateTransitions(timePoint);
 
-    if (style->impl->loaded && updateFlags & Update::AnnotationStyle) {
-        annotationManager.updateStyle(*style->impl);
-    }
-
     if (updateFlags & Update::AnnotationData) {
         annotationManager.updateData();
     }
@@ -327,8 +325,10 @@ const style::Style& Map::getStyle() const {
 }
 
 void Map::setStyle(std::unique_ptr<Style> style) {
+    assert(style);
     impl->onStyleLoading();
     impl->style = std::move(style);
+    impl->annotationManager.setStyle(*impl->style);
 }
 
 #pragma mark - Transitions
@@ -708,12 +708,10 @@ LatLng Map::latLngForPixel(const ScreenCoordinate& pixel) const {
 
 void Map::addAnnotationImage(std::unique_ptr<style::Image> image) {
     impl->annotationManager.addImage(std::move(image));
-    impl->onUpdate(Update::AnnotationStyle);
 }
 
 void Map::removeAnnotationImage(const std::string& id) {
     impl->annotationManager.removeImage(id);
-    impl->onUpdate(Update::AnnotationStyle);
 }
 
 double Map::getTopOffsetPixelsForAnnotationImage(const std::string& id) {
@@ -722,7 +720,7 @@ double Map::getTopOffsetPixelsForAnnotationImage(const std::string& id) {
 
 AnnotationID Map::addAnnotation(const Annotation& annotation) {
     auto result = impl->annotationManager.addAnnotation(annotation, getMaxZoom());
-    impl->onUpdate(Update::AnnotationStyle | Update::AnnotationData);
+    impl->onUpdate(Update::AnnotationData);
     return result;
 }
 
@@ -732,7 +730,7 @@ void Map::updateAnnotation(AnnotationID id, const Annotation& annotation) {
 
 void Map::removeAnnotation(AnnotationID annotation) {
     impl->annotationManager.removeAnnotation(annotation);
-    impl->onUpdate(Update::AnnotationStyle | Update::AnnotationData);
+    impl->onUpdate(Update::AnnotationData);
 }
 
 #pragma mark - Feature query api
@@ -874,7 +872,7 @@ void Map::Impl::onStyleLoaded() {
         map.setPitch(style->getDefaultPitch());
     }
 
-    onUpdate(Update::AnnotationStyle);
+    annotationManager.onStyleLoaded();
     observer.onDidFinishLoadingStyle();
 }
 
diff --git a/src/mbgl/map/update.hpp b/src/mbgl/map/update.hpp
index ab8b10c651..057720a5c9 100644
--- a/src/mbgl/map/update.hpp
+++ b/src/mbgl/map/update.hpp
@@ -8,7 +8,6 @@ namespace mbgl {
 enum class Update {
     Nothing                   = 0,
     Repaint                   = 1 << 0,
-    AnnotationStyle           = 1 << 6,
     AnnotationData            = 1 << 7
 };
 
diff --git a/test/style/source.test.cpp b/test/style/source.test.cpp
index 8b9cda1db3..919260ffe9 100644
--- a/test/style/source.test.cpp
+++ b/test/style/source.test.cpp
@@ -3,6 +3,7 @@
 #include <mbgl/test/stub_style_observer.hpp>
 #include <mbgl/test/stub_render_source_observer.hpp>
 
+#include <mbgl/style/style.hpp>
 #include <mbgl/style/source_impl.hpp>
 #include <mbgl/style/sources/raster_source.hpp>
 #include <mbgl/style/sources/vector_source.hpp>
@@ -47,7 +48,8 @@ public:
     Transform transform;
     TransformState transformState;
     ThreadPool threadPool { 1 };
-    AnnotationManager annotationManager;
+    Style style { loop, fileSource, 1 };
+    AnnotationManager annotationManager { style };
     ImageManager imageManager;
     GlyphManager glyphManager { fileSource };
 
diff --git a/test/tile/annotation_tile.test.cpp b/test/tile/annotation_tile.test.cpp
index 813b813220..739307b78a 100644
--- a/test/tile/annotation_tile.test.cpp
+++ b/test/tile/annotation_tile.test.cpp
@@ -16,6 +16,7 @@
 #include <mbgl/text/glyph_manager.hpp>
 #include <mbgl/map/backend_scope.hpp>
 #include <mbgl/gl/headless_backend.hpp>
+#include <mbgl/style/style.hpp>
 
 #include <memory>
 
@@ -27,10 +28,11 @@ public:
     TransformState transformState;
     util::RunLoop loop;
     ThreadPool threadPool { 1 };
-    AnnotationManager annotationManager;
+    style::Style style { loop, fileSource, 1 };
+    AnnotationManager annotationManager { style };
     HeadlessBackend backend { test::sharedDisplay() };
     BackendScope scope { backend };
-    RenderStyle style { threadPool, fileSource };
+    RenderStyle renderStyle { threadPool, fileSource };
     ImageManager imageManager;
     GlyphManager glyphManager { fileSource };
 
@@ -92,7 +94,7 @@ TEST(AnnotationTile, Issue8289) {
     TransformState transformState;
     RenderedQueryOptions options;
 
-    tile.queryRenderedFeatures(result, queryGeometry, transformState, test.style, options);
+    tile.queryRenderedFeatures(result, queryGeometry, transformState, test.renderStyle, options);
 
     EXPECT_TRUE(result.empty());
 }
diff --git a/test/tile/geojson_tile.test.cpp b/test/tile/geojson_tile.test.cpp
index 52f7a20f00..31fb8c1fd0 100644
--- a/test/tile/geojson_tile.test.cpp
+++ b/test/tile/geojson_tile.test.cpp
@@ -8,6 +8,7 @@
 #include <mbgl/util/run_loop.hpp>
 #include <mbgl/map/transform.hpp>
 #include <mbgl/renderer/tile_parameters.hpp>
+#include <mbgl/style/style.hpp>
 #include <mbgl/style/layers/circle_layer.hpp>
 #include <mbgl/annotation/annotation_manager.hpp>
 #include <mbgl/renderer/image_manager.hpp>
@@ -24,7 +25,8 @@ public:
     TransformState transformState;
     util::RunLoop loop;
     ThreadPool threadPool { 1 };
-    AnnotationManager annotationManager;
+    style::Style style { loop, fileSource, 1 };
+    AnnotationManager annotationManager { style };
     ImageManager imageManager;
     GlyphManager glyphManager { fileSource };
     Tileset tileset { { "https://example.com" }, { 0, 22 }, "none" };
diff --git a/test/tile/raster_tile.test.cpp b/test/tile/raster_tile.test.cpp
index 4680e7f485..e96e09bacf 100644
--- a/test/tile/raster_tile.test.cpp
+++ b/test/tile/raster_tile.test.cpp
@@ -3,6 +3,7 @@
 #include <mbgl/tile/raster_tile.hpp>
 #include <mbgl/tile/tile_loader_impl.hpp>
 
+#include <mbgl/style/style.hpp>
 #include <mbgl/util/default_thread_pool.hpp>
 #include <mbgl/util/run_loop.hpp>
 #include <mbgl/map/transform.hpp>
@@ -20,7 +21,8 @@ public:
     TransformState transformState;
     util::RunLoop loop;
     ThreadPool threadPool { 1 };
-    AnnotationManager annotationManager;
+    style::Style style { loop, fileSource, 1 };
+    AnnotationManager annotationManager { style };
     ImageManager imageManager;
     GlyphManager glyphManager { fileSource };
     Tileset tileset { { "https://example.com" }, { 0, 22 }, "none" };
diff --git a/test/tile/vector_tile.test.cpp b/test/tile/vector_tile.test.cpp
index d7ad3c999d..b0fee28edf 100644
--- a/test/tile/vector_tile.test.cpp
+++ b/test/tile/vector_tile.test.cpp
@@ -7,6 +7,7 @@
 #include <mbgl/util/run_loop.hpp>
 #include <mbgl/map/transform.hpp>
 #include <mbgl/map/query.hpp>
+#include <mbgl/style/style.hpp>
 #include <mbgl/style/layers/symbol_layer.hpp>
 #include <mbgl/renderer/tile_parameters.hpp>
 #include <mbgl/renderer/buckets/symbol_bucket.hpp>
@@ -26,7 +27,8 @@ public:
     TransformState transformState;
     util::RunLoop loop;
     ThreadPool threadPool { 1 };
-    AnnotationManager annotationManager;
+    style::Style style { loop, fileSource, 1 };
+    AnnotationManager annotationManager { style };
     ImageManager imageManager;
     GlyphManager glyphManager { fileSource };
     Tileset tileset { { "https://example.com" }, { 0, 22 }, "none" };