[core] Don't use a separate SpriteAtlas for annotation images

CMakeLists.txt

@@ -56,6 +56,7 @@ mason_use(pixelmatch VERSION 0.10.0 HEADER_ONLY)
 mason_use(geojson VERSION 0.4.0 HEADER_ONLY)
 mason_use(polylabel VERSION 1.0.2 HEADER_ONLY)
 mason_use(wagyu VERSION 0.4.1 HEADER_ONLY)
+mason_use(shelf-pack VERSION 2.0.1 HEADER_ONLY)
 
 add_definitions(-DRAPIDJSON_HAS_STDSTRING=1)
 

cmake/core-files.cmake

@@ -288,8 +288,6 @@ set(MBGL_CORE_FILES
     # sprite
     src/mbgl/sprite/sprite_atlas.cpp
     src/mbgl/sprite/sprite_atlas.hpp
-    src/mbgl/sprite/sprite_image_collection.cpp
-    src/mbgl/sprite/sprite_image_collection.hpp
     src/mbgl/sprite/sprite_loader.cpp
     src/mbgl/sprite/sprite_loader.hpp
     src/mbgl/sprite/sprite_loader_observer.hpp

cmake/core.cmake

@@ -26,6 +26,7 @@ target_add_mason_package(mbgl-core PRIVATE earcut)
 target_add_mason_package(mbgl-core PRIVATE protozero)
 target_add_mason_package(mbgl-core PRIVATE polylabel)
 target_add_mason_package(mbgl-core PRIVATE wagyu)
+target_add_mason_package(mbgl-core PRIVATE shelf-pack)
 
 mbgl_platform_core()
 

cmake/test-files.cmake

@@ -46,7 +46,6 @@ set(MBGL_TEST_FILES
 
     # sprite
     test/sprite/sprite_atlas.test.cpp
-    test/sprite/sprite_image_collection.test.cpp
     test/sprite/sprite_loader.test.cpp
     test/sprite/sprite_parser.test.cpp
 

cmake/test.cmake

@@ -35,6 +35,7 @@ target_add_mason_package(mbgl-test PRIVATE pixelmatch)
 target_add_mason_package(mbgl-test PRIVATE boost)
 target_add_mason_package(mbgl-test PRIVATE geojson)
 target_add_mason_package(mbgl-test PRIVATE geojsonvt)
+target_add_mason_package(mbgl-test PRIVATE shelf-pack)
 
 mbgl_platform_test()
 

include/mbgl/style/image.hpp

@@ -11,6 +11,7 @@ namespace style {
 class Image {
 public:
     Image(std::string id, PremultipliedImage&&, float pixelRatio, bool sdf = false);
+    Image(const Image&);
 
     std::string getID() const;
 

src/mbgl/annotation/annotation_manager.cpp

@@ -4,9 +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/sprite/sprite_image_collection.hpp>
 #include <mbgl/style/style.hpp>
-#include <mbgl/style/image_impl.hpp>
 #include <mbgl/style/layers/symbol_layer.hpp>
 #include <mbgl/style/layers/symbol_layer_impl.hpp>
 #include <mbgl/storage/file_source.hpp>
@@ -20,13 +18,7 @@ using namespace style;
 const std::string AnnotationManager::SourceID = "com.mapbox.annotations";
 const std::string AnnotationManager::PointLayerID = "com.mapbox.annotations.points";
 
-AnnotationManager::AnnotationManager(float pixelRatio)
-        : spriteAtlas({ 1024, 1024 }, pixelRatio){
-    // This is a special atlas, holding only images added via addIcon, so we always treat it as
-    // loaded.
-    spriteAtlas.markAsLoaded();
-}
-
+AnnotationManager::AnnotationManager() = default;
 AnnotationManager::~AnnotationManager() = default;
 
 AnnotationID AnnotationManager::addAnnotation(const Annotation& annotation, const uint8_t maxZoom) {
@@ -156,7 +148,7 @@ void AnnotationManager::updateStyle(Style& style) {
         std::unique_ptr<SymbolLayer> layer = std::make_unique<SymbolLayer>(PointLayerID, SourceID);
 
         layer->setSourceLayer(PointLayerID);
-        layer->setIconImage({"{sprite}"});
+        layer->setIconImage({SourceID + ".{sprite}"});
         layer->setIconAllowOverlap(true);
         layer->setIconIgnorePlacement(true);
 
@@ -167,13 +159,30 @@ void AnnotationManager::updateStyle(Style& style) {
         shape.second->updateStyle(style);
     }
 
+    for (const auto& image : images) {
+        // Call addImage even for images we may have previously added, because we must support
+        // addAnnotationImage being used to update an existing image. Creating a new image is
+        // relatively cheap, as it copies only the Immutable reference. (We can't keep track
+        // 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);
+        }
+    }
+
     obsoleteShapeAnnotationLayers.clear();
+    obsoleteImages.clear();
 }
 
 void AnnotationManager::updateData() {
@@ -192,20 +201,23 @@ void AnnotationManager::removeTile(AnnotationTile& tile) {
 }
 
 void AnnotationManager::addImage(std::unique_ptr<style::Image> image) {
-    addSpriteImage(spriteImages, std::move(image), [&](style::Image& added) {
-        spriteAtlas.addImage(added.impl);
-    });
+    // To ensure that annotation images do not collide with images from the style,
+    // create a new image with the input ID prefixed by "com.mapbox.annotations".
+    std::string id = SourceID + "." + image->getID();
+    images.erase(id);
+    images.emplace(id,
+        style::Image(id, image->getImage().clone(), image->getPixelRatio(), image->isSdf()));
+    obsoleteImages.erase(id);
 }
 
 void AnnotationManager::removeImage(const std::string& id) {
-    removeSpriteImage(spriteImages, id, [&] () {
-        spriteAtlas.removeImage(id);
-    });
+    images.erase(id);
+    obsoleteImages.insert(id);
 }
 
 double AnnotationManager::getTopOffsetPixelsForImage(const std::string& id) {
-    const style::Image::Impl* impl = spriteAtlas.getImage(id);
-    return impl ? -(impl->image.size.height / impl->pixelRatio) / 2 : 0;
+    auto it = images.find(id);
+    return it == images.end() ? -(it->second.getImage().size.height / it->second.getPixelRatio()) / 2 : 0;
 }
 
 } // namespace mbgl

src/mbgl/annotation/annotation_manager.hpp

@@ -2,7 +2,7 @@
 
 #include <mbgl/annotation/annotation.hpp>
 #include <mbgl/annotation/symbol_annotation_impl.hpp>
-#include <mbgl/sprite/sprite_atlas.hpp>
+#include <mbgl/style/image.hpp>
 #include <mbgl/map/update.hpp>
 #include <mbgl/util/noncopyable.hpp>
 
@@ -21,12 +21,11 @@ class ShapeAnnotationImpl;
 
 namespace style {
 class Style;
-class Image;
 } // namespace style
 
 class AnnotationManager : private util::noncopyable {
 public:
-    AnnotationManager(float pixelRatio);
+    AnnotationManager();
     ~AnnotationManager();
 
     AnnotationID addAnnotation(const Annotation&, const uint8_t maxZoom);
@@ -36,7 +35,6 @@ class AnnotationManager : private util::noncopyable {
     void addImage(std::unique_ptr<style::Image>);
     void removeImage(const std::string&);
     double getTopOffsetPixelsForImage(const std::string&);
-    SpriteAtlas& getSpriteAtlas() { return spriteAtlas; }
 
     void updateStyle(style::Style&);
     void updateData();
@@ -67,16 +65,16 @@ class AnnotationManager : private util::noncopyable {
     // <https://github.com/mapbox/mapbox-gl-native/issues/5691>
     using SymbolAnnotationMap = std::map<AnnotationID, std::shared_ptr<SymbolAnnotationImpl>>;
     using ShapeAnnotationMap = std::map<AnnotationID, std::unique_ptr<ShapeAnnotationImpl>>;
+    using ImageMap = std::unordered_map<std::string, style::Image>;
 
     SymbolAnnotationTree symbolTree;
     SymbolAnnotationMap symbolAnnotations;
     ShapeAnnotationMap shapeAnnotations;
+    ImageMap images;
     std::unordered_set<std::string> obsoleteShapeAnnotationLayers;
+    std::unordered_set<std::string> obsoleteImages;
     std::unordered_set<AnnotationTile*> tiles;
 
-    std::unordered_map<std::string, std::unique_ptr<style::Image>> spriteImages;
-    SpriteAtlas spriteAtlas;
-
     friend class AnnotationTile;
 };
 

src/mbgl/annotation/annotation_tile.cpp

@@ -13,7 +13,7 @@ AnnotationTile::AnnotationTile(const OverscaledTileID& overscaledTileID,
                                const TileParameters& parameters)
     : GeometryTile(overscaledTileID, AnnotationManager::SourceID, parameters,
                    *parameters.style.glyphAtlas,
-                   parameters.annotationManager.spriteAtlas),
+                   *parameters.style.spriteAtlas),
       annotationManager(parameters.annotationManager) {
     annotationManager.addTile(*this);
 }

src/mbgl/map/map.cpp

@@ -90,7 +90,7 @@ class Map::Impl : public style::Observer {
 
     Update updateFlags = Update::Nothing;
 
-    std::unique_ptr<AnnotationManager> annotationManager;
+    AnnotationManager annotationManager;
     std::unique_ptr<Painter> painter;
     std::unique_ptr<Style> style;
 
@@ -153,7 +153,6 @@ Map::Impl::Impl(Map& map_,
       contextMode(contextMode_),
       pixelRatio(pixelRatio_),
       programCacheDir(std::move(programCacheDir_)),
-      annotationManager(std::make_unique<AnnotationManager>(pixelRatio)),
       asyncInvalidate([this] {
           if (mode == MapMode::Continuous) {
               backend.invalidate();
@@ -171,7 +170,6 @@ Map::~Map() {
     // Explicit resets currently necessary because these abandon resources that need to be
     // cleaned up by context.reset();
     impl->style.reset();
-    impl->annotationManager.reset();
     impl->painter.reset();
 }
 
@@ -235,11 +233,11 @@ void Map::Impl::render(View& view) {
     transform.updateTransitions(timePoint);
 
     if (style->loaded && updateFlags & Update::AnnotationStyle) {
-        annotationManager->updateStyle(*style);
+        annotationManager.updateStyle(*style);
     }
 
     if (updateFlags & Update::AnnotationData) {
-        annotationManager->updateData();
+        annotationManager.updateData();
     }
 
     style->update({
@@ -251,7 +249,7 @@ void Map::Impl::render(View& view) {
         transform.getState(),
         scheduler,
         fileSource,
-        *annotationManager
+        annotationManager
     });
 
     updateFlags = Update::Nothing;
@@ -278,8 +276,7 @@ void Map::Impl::render(View& view) {
 
         painter->render(*style,
                         frameData,
-                        view,
-                        annotationManager->getSpriteAtlas());
+                        view);
 
         painter->cleanup();
 
@@ -312,8 +309,7 @@ void Map::Impl::render(View& view) {
 
         painter->render(*style,
                         frameData,
-                        view,
-                        annotationManager->getSpriteAtlas());
+                        view);
 
         auto request = std::move(stillImageRequest);
         request->callback(nullptr);
@@ -789,29 +785,31 @@ LatLng Map::latLngForPixel(const ScreenCoordinate& pixel) const {
 #pragma mark - Annotations
 
 void Map::addAnnotationImage(std::unique_ptr<style::Image> image) {
-    impl->annotationManager->addImage(std::move(image));
+    impl->annotationManager.addImage(std::move(image));
+    impl->onUpdate(Update::AnnotationStyle);
 }
 
 void Map::removeAnnotationImage(const std::string& id) {
-    impl->annotationManager->removeImage(id);
+    impl->annotationManager.removeImage(id);
+    impl->onUpdate(Update::AnnotationStyle);
 }
 
 double Map::getTopOffsetPixelsForAnnotationImage(const std::string& id) {
-    return impl->annotationManager->getTopOffsetPixelsForImage(id);
+    return impl->annotationManager.getTopOffsetPixelsForImage(id);
 }
 
 AnnotationID Map::addAnnotation(const Annotation& annotation) {
-    auto result = impl->annotationManager->addAnnotation(annotation, getMaxZoom());
+    auto result = impl->annotationManager.addAnnotation(annotation, getMaxZoom());
     impl->onUpdate(Update::AnnotationStyle | Update::AnnotationData);
     return result;
 }
 
 void Map::updateAnnotation(AnnotationID id, const Annotation& annotation) {
-    impl->onUpdate(impl->annotationManager->updateAnnotation(id, annotation, getMaxZoom()));
+    impl->onUpdate(impl->annotationManager.updateAnnotation(id, annotation, getMaxZoom()));
 }
 
 void Map::removeAnnotation(AnnotationID annotation) {
-    impl->annotationManager->removeAnnotation(annotation);
+    impl->annotationManager.removeAnnotation(annotation);
     impl->onUpdate(Update::AnnotationStyle | Update::AnnotationData);
 }
 

src/mbgl/renderer/buckets/symbol_bucket.hpp

@@ -70,8 +70,6 @@ class SymbolBucket : public Bucket {
         optional<gl::VertexBuffer<CollisionBoxVertex>> vertexBuffer;
         optional<gl::IndexBuffer<gl::Lines>> indexBuffer;
     } collisionBox;
-
-    SpriteAtlas* spriteAtlas = nullptr;
 };
 
 } // namespace mbgl

src/mbgl/renderer/painter.cpp

@@ -125,7 +125,7 @@ void Painter::cleanup() {
     context.performCleanup();
 }
 
-void Painter::render(const Style& style, const FrameData& frame_, View& view, SpriteAtlas& annotationSpriteAtlas) {
+void Painter::render(const Style& style, const FrameData& frame_, View& view) {
     frame = frame_;
     if (frame.contextMode == GLContextMode::Shared) {
         context.setDirtyState();
@@ -176,7 +176,6 @@ void Painter::render(const Style& style, const FrameData& frame_, View& view, Sp
         lineAtlas->upload(context, 0);
         glyphAtlas->upload(context, 0);
         frameHistory.upload(context, 0);
-        annotationSpriteAtlas.upload(context, 0);
 
         for (const auto& item : order) {
             for (const auto& tileRef : item.tiles) {

src/mbgl/renderer/painter.hpp

@@ -79,8 +79,7 @@ class Painter : private util::noncopyable {
 
     void render(const style::Style&,
                 const FrameData&,
-                View&,
-                SpriteAtlas& annotationSpriteAtlas);
+                View&);
 
     void cleanup();
 

src/mbgl/renderer/painters/painter_symbol.cpp

@@ -67,12 +67,11 @@ void Painter::renderSymbol(PaintParameters& parameters,
         auto values = layer.iconPropertyValues(layout);
         auto paintPropertyValues = layer.iconPaintProperties();
 
-        SpriteAtlas& atlas = *bucket.spriteAtlas;
         const bool iconScaled = layout.get<IconSize>().constantOr(1.0) != 1.0 || bucket.iconsNeedLinear;
         const bool iconTransformed = values.rotationAlignment == AlignmentType::Map || state.getPitch() != 0;
-        atlas.bind(bucket.sdfIcons || state.isChanging() || iconScaled || iconTransformed, context, 0);
+        spriteAtlas->bind(bucket.sdfIcons || state.isChanging() || iconScaled || iconTransformed, context, 0);
 
-        const Size texsize = atlas.getPixelSize();
+        const Size texsize = spriteAtlas->getPixelSize();
 
         if (bucket.sdfIcons) {
             if (values.hasHalo) {