[core] Replace inline SpriteAtlas updates with diffing

7 files changed, 77 insertions, 32 deletions

diff --git a/src/mbgl/renderer/style_diff.cpp b/src/mbgl/renderer/style_diff.cpp
index 12000069ea..e2be95432f 100644
--- a/src/mbgl/renderer/style_diff.cpp
+++ b/src/mbgl/renderer/style_diff.cpp
@@ -6,15 +6,10 @@
 
 namespace mbgl {
 
-template <class T>
-StyleDifference<T> diff(const std::vector<T>& a, const std::vector<T>& b) {
+template <class T, class Eq>
+StyleDifference<T> diff(const std::vector<T>& a, const std::vector<T>& b, const Eq& eq) {
     std::vector<T> lcs;
 
-    auto eq = [] (const T& lhs, const T& rhs) {
-        return std::tie(lhs->id, lhs->type)
-            == std::tie(rhs->id, rhs->type);
-    };
-
     longest_common_subsequence(a.begin(), a.end(), b.begin(), b.end(), std::back_inserter(lcs), eq);
 
     auto aIt = a.begin();
@@ -43,14 +38,27 @@ StyleDifference<T> diff(const std::vector<T>& a, const std::vector<T>& b) {
     return result;
 }
 
+ImageDifference diffImages(const std::vector<ImmutableImage>& a,
+                           const std::vector<ImmutableImage>& b) {
+    return diff(a, b, [] (const ImmutableImage& lhs, const ImmutableImage& rhs) {
+        return lhs->id == rhs->id;
+    });
+}
+
 SourceDifference diffSources(const std::vector<ImmutableSource>& a,
                              const std::vector<ImmutableSource>& b) {
-    return diff(a, b);
+    return diff(a, b, [] (const ImmutableSource& lhs, const ImmutableSource& rhs) {
+        return std::tie(lhs->id, lhs->type)
+            == std::tie(rhs->id, rhs->type);
+    });
 }
 
 LayerDifference diffLayers(const std::vector<ImmutableLayer>& a,
                            const std::vector<ImmutableLayer>& b) {
-    return diff(a, b);
+    return diff(a, b, [] (const ImmutableLayer& lhs, const ImmutableLayer& rhs) {
+        return std::tie(lhs->id, lhs->type)
+            == std::tie(rhs->id, rhs->type);
+    });
 }
 
 } // namespace mbgl
diff --git a/src/mbgl/renderer/style_diff.hpp b/src/mbgl/renderer/style_diff.hpp
index dc2ef95c6c..0ebb1fbfd6 100644
--- a/src/mbgl/renderer/style_diff.hpp
+++ b/src/mbgl/renderer/style_diff.hpp
@@ -1,3 +1,4 @@
+#include <mbgl/style/image_impl.hpp>
 #include <mbgl/style/source_impl.hpp>
 #include <mbgl/style/layer_impl.hpp>
 #include <mbgl/util/immutable.hpp>
@@ -15,6 +16,12 @@ public:
     std::unordered_map<std::string, T> changed;
 };
 
+using ImmutableImage = Immutable<style::Image::Impl>;
+using ImageDifference = StyleDifference<ImmutableImage>;
+
+ImageDifference diffImages(const std::vector<ImmutableImage>&,
+                           const std::vector<ImmutableImage>&);
+
 using ImmutableSource = Immutable<style::Source::Impl>;
 using SourceDifference = StyleDifference<ImmutableSource>;
 
diff --git a/src/mbgl/sprite/sprite_atlas.cpp b/src/mbgl/sprite/sprite_atlas.cpp
index 97bf9edcf9..69f2b0ce71 100644
--- a/src/mbgl/sprite/sprite_atlas.cpp
+++ b/src/mbgl/sprite/sprite_atlas.cpp
@@ -52,17 +52,16 @@ void SpriteAtlas::onSpriteLoaded() {
 }
 
 void SpriteAtlas::addImage(Immutable<style::Image::Impl> image_) {
+    assert(entries.find(image_->id) == entries.end());
+    entries.emplace(image_->id, Entry { image_ });
     icons.clear();
+}
 
-    auto it = entries.find(image_->id);
-    if (it == entries.end()) {
-        entries.emplace(image_->id, Entry { image_, {}, {} });
-        return;
-    }
+void SpriteAtlas::updateImage(Immutable<style::Image::Impl> image_) {
+    assert(entries.find(image_->id) != entries.end());
+    Entry& entry = entries.at(image_->id);
 
-    Entry& entry = it->second;
-
-    // There is already a sprite with that name in our store.
+    // Style::addImage should prevent changing size.
     assert(entry.image->image.size == image_->image.size);
 
     entry.image = std::move(image_);
@@ -74,15 +73,13 @@ void SpriteAtlas::addImage(Immutable<style::Image::Impl> image_) {
     if (entry.patternBin) {
         copy(entry, &Entry::patternBin);
     }
-}
 
-void SpriteAtlas::removeImage(const std::string& id) {
     icons.clear();
+}
 
-    auto it = entries.find(id);
-    assert(it != entries.end());
-
-    Entry& entry = it->second;
+void SpriteAtlas::removeImage(const std::string& id) {
+    assert(entries.find(id) != entries.end());
+    Entry& entry = entries.at(id);
 
     if (entry.iconBin) {
         shelfPack.unref(*entry.iconBin);
@@ -92,7 +89,8 @@ void SpriteAtlas::removeImage(const std::string& id) {
         shelfPack.unref(*entry.patternBin);
     }
 
-    entries.erase(it);
+    entries.erase(id);
+    icons.clear();
 }
 
 const style::Image::Impl* SpriteAtlas::getImage(const std::string& id) const {
diff --git a/src/mbgl/sprite/sprite_atlas.hpp b/src/mbgl/sprite/sprite_atlas.hpp
index 807c871731..1dbef86f7e 100644
--- a/src/mbgl/sprite/sprite_atlas.hpp
+++ b/src/mbgl/sprite/sprite_atlas.hpp
@@ -73,7 +73,9 @@ public:
     void dumpDebugLogs() const;
 
     const style::Image::Impl* getImage(const std::string&) const;
+
     void addImage(Immutable<style::Image::Impl>);
+    void updateImage(Immutable<style::Image::Impl>);
     void removeImage(const std::string&);
 
     void getIcons(IconRequestor&, IconDependencies);
diff --git a/src/mbgl/style/style.cpp b/src/mbgl/style/style.cpp
index 0a212b977f..bc5d77b4bd 100644
--- a/src/mbgl/style/style.cpp
+++ b/src/mbgl/style/style.cpp
@@ -361,6 +361,35 @@ void Style::update(const UpdateParameters& parameters) {
     }
 
 
+    std::vector<Immutable<Image::Impl>> newImageImpls;
+    newImageImpls.reserve(images.size());
+    for (const auto& entry : images) {
+        newImageImpls.push_back(entry.second->impl);
+    }
+
+    const ImageDifference imageDiff = diffImages(imageImpls, newImageImpls);
+    imageImpls = std::move(newImageImpls);
+
+    // Remove removed images from sprite atlas.
+    for (const auto& entry : imageDiff.removed) {
+        spriteAtlas->removeImage(entry.first);
+    }
+
+    // Add added images to sprite atlas.
+    for (const auto& entry : imageDiff.added) {
+        spriteAtlas->addImage(entry.second);
+    }
+
+    // Update changed images.
+    for (const auto& entry : imageDiff.changed) {
+        spriteAtlas->updateImage(entry.second);
+    }
+
+    if (spriteLoaded && !spriteAtlas->isLoaded()) {
+        spriteAtlas->onSpriteLoaded();
+    }
+
+
     std::vector<Immutable<Source::Impl>> newSourceImpls;
     newSourceImpls.reserve(sources.size());
     for (const auto& source : sources) {
@@ -509,6 +538,10 @@ bool Style::isLoaded() const {
         return false;
     }
 
+    if (!spriteLoaded) {
+        return false;
+    }
+
     for (const auto& source: sources) {
         if (!source->loaded) {
             return false;
@@ -521,10 +554,6 @@ bool Style::isLoaded() const {
         }
     }
 
-    if (!spriteAtlas->isLoaded()) {
-        return false;
-    }
-
     return true;
 }
 
@@ -535,13 +564,11 @@ void Style::addImage(std::unique_ptr<style::Image> image) {
         Log::Warning(Event::Sprite, "Can't change sprite dimensions for '%s'", id.c_str());
         return;
     }
-    spriteAtlas->addImage(image->impl);
     images[id] = std::move(image);
 }
 
 void Style::removeImage(const std::string& id) {
     images.erase(id);
-    spriteAtlas->removeImage(id);
 }
 
 const style::Image* Style::getImage(const std::string& id) const {
@@ -764,7 +791,7 @@ void Style::onSpriteLoaded(std::vector<std::unique_ptr<Image>>&& images_) {
     for (auto& image : images_) {
         addImage(std::move(image));
     }
-    spriteAtlas->onSpriteLoaded();
+    spriteLoaded = true;
     observer->onUpdate(Update::Repaint); // For *-pattern properties.
 }
 
diff --git a/src/mbgl/style/style.hpp b/src/mbgl/style/style.hpp
index 5d3c2899fd..2d0b9eddbb 100644
--- a/src/mbgl/style/style.hpp
+++ b/src/mbgl/style/style.hpp
@@ -6,6 +6,7 @@
 #include <mbgl/style/layer_observer.hpp>
 #include <mbgl/style/light_observer.hpp>
 #include <mbgl/style/update_batch.hpp>
+#include <mbgl/style/image.hpp>
 #include <mbgl/renderer/render_source.hpp>
 #include <mbgl/renderer/render_source_observer.hpp>
 #include <mbgl/renderer/render_layer.hpp>
@@ -138,6 +139,7 @@ private:
     double defaultBearing = 0;
     double defaultPitch = 0;
 
+    std::vector<Immutable<Image::Impl>> imageImpls;
     std::vector<Immutable<Source::Impl>> sourceImpls;
     std::vector<Immutable<Layer::Impl>> layerImpls;
 
@@ -179,6 +181,7 @@ private:
 
     UpdateBatch updateBatch;
     ZoomHistory zoomHistory;
+    bool spriteLoaded = false;
 
 public:
     bool loaded = false;
diff --git a/test/sprite/sprite_atlas.test.cpp b/test/sprite/sprite_atlas.test.cpp
index 4291fe9902..3226f19c1b 100644
--- a/test/sprite/sprite_atlas.test.cpp
+++ b/test/sprite/sprite_atlas.test.cpp
@@ -98,7 +98,7 @@ TEST(SpriteAtlas, Updates) {
     for (size_t i = 0; i < image2.bytes(); i++) {
         image2.data.get()[i] = 255;
     }
-    atlas.addImage(makeMutable<style::Image::Impl>("one", std::move(image2), 1));
+    atlas.updateImage(makeMutable<style::Image::Impl>("one", std::move(image2), 1));
 
     test::checkImage("test/fixtures/sprite_atlas/updates_after", atlas.getAtlasImage());
 }