[core] Eliminate SpritePatternMode

7 files changed, 52 insertions, 65 deletions

diff --git a/src/mbgl/layout/symbol_layout.cpp b/src/mbgl/layout/symbol_layout.cpp
index 11c26fa661..4ff9bd9452 100644
--- a/src/mbgl/layout/symbol_layout.cpp
+++ b/src/mbgl/layout/symbol_layout.cpp
@@ -255,7 +255,7 @@ void SymbolLayout::prepare(uintptr_t tileUID,
 
         // if feature has icon, get sprite atlas position
         if (feature.icon) {
-            auto image = spriteAtlas.getImage(*feature.icon, SpritePatternMode::Single);
+            auto image = spriteAtlas.getIcon(*feature.icon);
             if (image) {
                 shapedIcon = shapeIcon(*image, feature);
                 assert((*image).spriteImage);
diff --git a/src/mbgl/renderer/painter_background.cpp b/src/mbgl/renderer/painter_background.cpp
index 6ce3b59358..4ac414335b 100644
--- a/src/mbgl/renderer/painter_background.cpp
+++ b/src/mbgl/renderer/painter_background.cpp
@@ -24,10 +24,8 @@ void Painter::renderBackground(PaintParameters& parameters, const BackgroundLaye
     const FillProgram::PaintPropertyBinders paintAttibuteData(properties, 0);
 
     if (!background.get<BackgroundPattern>().to.empty()) {
-        optional<SpriteAtlasElement> imagePosA = spriteAtlas->getImage(
-            background.get<BackgroundPattern>().from, SpritePatternMode::Repeating);
-        optional<SpriteAtlasElement> imagePosB = spriteAtlas->getImage(
-            background.get<BackgroundPattern>().to, SpritePatternMode::Repeating);
+        optional<SpriteAtlasElement> imagePosA = spriteAtlas->getPattern(background.get<BackgroundPattern>().from);
+        optional<SpriteAtlasElement> imagePosB = spriteAtlas->getPattern(background.get<BackgroundPattern>().to);
 
         if (!imagePosA || !imagePosB)
             return;
diff --git a/src/mbgl/renderer/painter_fill.cpp b/src/mbgl/renderer/painter_fill.cpp
index bdc99d42f0..4276bd06ed 100644
--- a/src/mbgl/renderer/painter_fill.cpp
+++ b/src/mbgl/renderer/painter_fill.cpp
@@ -24,10 +24,8 @@ void Painter::renderFill(PaintParameters& parameters,
             return;
         }
 
-        optional<SpriteAtlasElement> imagePosA = spriteAtlas->getImage(
-            properties.get<FillPattern>().from, SpritePatternMode::Repeating);
-        optional<SpriteAtlasElement> imagePosB = spriteAtlas->getImage(
-            properties.get<FillPattern>().to, SpritePatternMode::Repeating);
+        optional<SpriteAtlasElement> imagePosA = spriteAtlas->getPattern(properties.get<FillPattern>().from);
+        optional<SpriteAtlasElement> imagePosB = spriteAtlas->getPattern(properties.get<FillPattern>().to);
 
         if (!imagePosA || !imagePosB) {
             return;
diff --git a/src/mbgl/renderer/painter_line.cpp b/src/mbgl/renderer/painter_line.cpp
index 545f01ce88..4e19f841b1 100644
--- a/src/mbgl/renderer/painter_line.cpp
+++ b/src/mbgl/renderer/painter_line.cpp
@@ -61,10 +61,8 @@ void Painter::renderLine(PaintParameters& parameters,
                  lineAtlas->getSize().width));
 
     } else if (!properties.get<LinePattern>().from.empty()) {
-        optional<SpriteAtlasElement> posA = spriteAtlas->getImage(
-            properties.get<LinePattern>().from, SpritePatternMode::Repeating);
-        optional<SpriteAtlasElement> posB = spriteAtlas->getImage(
-            properties.get<LinePattern>().to, SpritePatternMode::Repeating);
+        optional<SpriteAtlasElement> posA = spriteAtlas->getPattern(properties.get<LinePattern>().from);
+        optional<SpriteAtlasElement> posB = spriteAtlas->getPattern(properties.get<LinePattern>().to);
 
         if (!posA || !posB)
             return;
diff --git a/src/mbgl/sprite/sprite_atlas.cpp b/src/mbgl/sprite/sprite_atlas.cpp
index 81a8013e86..2bf8a3095a 100644
--- a/src/mbgl/sprite/sprite_atlas.cpp
+++ b/src/mbgl/sprite/sprite_atlas.cpp
@@ -153,6 +153,11 @@ void SpriteAtlas::removeSprite(const std::string& name) {
 
 void SpriteAtlas::_setSprite(const std::string& name,
                              const std::shared_ptr<const SpriteImage>& sprite) {
+    if (!sprite->image.valid()) {
+        Log::Warning(Event::Sprite, "invalid sprite image '%s'", name.c_str());
+        return;
+    }
+
     auto it = entries.find(name);
     if (it == entries.end()) {
         entries.emplace(name, Entry { sprite, {}, {} });
@@ -170,11 +175,11 @@ void SpriteAtlas::_setSprite(const std::string& name,
     entry.spriteImage = sprite;
 
     if (entry.iconRect) {
-        copy(entry.spriteImage->image, *entry.iconRect, SpritePatternMode::Single);
+        copy(entry, &Entry::iconRect);
     }
 
     if (entry.patternRect) {
-        copy(entry.spriteImage->image, *entry.patternRect, SpritePatternMode::Repeating);
+        copy(entry, &Entry::patternRect);
     }
 }
 
@@ -191,8 +196,16 @@ std::shared_ptr<const SpriteImage> SpriteAtlas::getSprite(const std::string& nam
     }
 }
 
+optional<SpriteAtlasElement> SpriteAtlas::getIcon(const std::string& name) {
+    return getImage(name, &Entry::iconRect);
+}
+
+optional<SpriteAtlasElement> SpriteAtlas::getPattern(const std::string& name) {
+    return getImage(name, &Entry::patternRect);
+}
+
 optional<SpriteAtlasElement> SpriteAtlas::getImage(const std::string& name,
-                                                   const SpritePatternMode mode) {
+                                                   optional<Rect<uint16_t>> Entry::*entryRect) {
     std::lock_guard<std::mutex> lock(mutex);
 
     auto it = entries.find(name);
@@ -205,18 +218,9 @@ optional<SpriteAtlasElement> SpriteAtlas::getImage(const std::string& name,
 
     Entry& entry = it->second;
 
-    if (mode == SpritePatternMode::Single && entry.iconRect) {
+    if (entry.*entryRect) {
         return SpriteAtlasElement {
-            *entry.iconRect,
-            entry.spriteImage,
-            size,
-            pixelRatio
-        };
-    }
-
-    if (mode == SpritePatternMode::Repeating && entry.patternRect) {
-        return SpriteAtlasElement {
-            *entry.patternRect,
+            *(entry.*entryRect),
             entry.spriteImage,
             size,
             pixelRatio
@@ -241,13 +245,8 @@ optional<SpriteAtlasElement> SpriteAtlas::getImage(const std::string& name,
         return {};
     }
 
-    copy(entry.spriteImage->image, rect, mode);
-
-    if (mode == SpritePatternMode::Single) {
-        entry.iconRect = rect;
-    } else {
-        entry.patternRect = rect;
-    }
+    entry.*entryRect = rect;
+    copy(entry, entryRect);
 
     return SpriteAtlasElement {
         rect,
@@ -257,26 +256,25 @@ optional<SpriteAtlasElement> SpriteAtlas::getImage(const std::string& name,
     };
 }
 
-void SpriteAtlas::copy(const PremultipliedImage& src, Rect<uint16_t> pos, const SpritePatternMode mode) {
+void SpriteAtlas::copy(const Entry& entry, optional<Rect<uint16_t>> Entry::*entryRect) {
     if (!image.valid()) {
         image = PremultipliedImage({ static_cast<uint32_t>(std::ceil(size.width * pixelRatio)),
                                      static_cast<uint32_t>(std::ceil(size.height * pixelRatio)) });
         image.fill(0);
     }
 
-    if (!src.valid()) {
-        return;
-    }
+    const PremultipliedImage& src = entry.spriteImage->image;
+    const Rect<uint16_t>& rect = *(entry.*entryRect);
 
     const uint32_t padding = 1;
-    const uint32_t x = (pos.x + padding) * pixelRatio;
-    const uint32_t y = (pos.y + padding) * pixelRatio;
+    const uint32_t x = (rect.x + padding) * pixelRatio;
+    const uint32_t y = (rect.y + padding) * pixelRatio;
     const uint32_t w = src.size.width;
     const uint32_t h = src.size.height;
 
     PremultipliedImage::copy(src, image, { 0, 0 }, { x, y }, { w, h });
 
-    if (mode == SpritePatternMode::Repeating) {
+    if (entryRect == &Entry::patternRect) {
         // Add 1 pixel wrapped padding on each side of the image.
         PremultipliedImage::copy(src, image, { 0, h - 1 }, { x, y - 1 }, { w, 1 }); // T
         PremultipliedImage::copy(src, image, { 0,     0 }, { x, y + h }, { w, 1 }); // B
diff --git a/src/mbgl/sprite/sprite_atlas.hpp b/src/mbgl/sprite/sprite_atlas.hpp
index 10ef2ddaa9..c7b266376b 100644
--- a/src/mbgl/sprite/sprite_atlas.hpp
+++ b/src/mbgl/sprite/sprite_atlas.hpp
@@ -36,11 +36,6 @@ public:
     std::array<float, 2> br;
 };
 
-enum class SpritePatternMode : bool {
-    Single = false,
-    Repeating = true,
-};
-
 class SpriteAtlas : public util::noncopyable {
 public:
     using Sprites = std::map<std::string, std::shared_ptr<const SpriteImage>>;
@@ -58,21 +53,13 @@ public:
 
     void setObserver(SpriteAtlasObserver*);
 
-    // Adds/replaces a Sprite image.
     void setSprite(const std::string&, std::shared_ptr<const SpriteImage>);
-
-    // Adds/replaces mutliple Sprite images.
-    void setSprites(const Sprites& sprites);
-
-    // Removes a Sprite.
     void removeSprite(const std::string&);
 
-    // Obtains a Sprite image.
     std::shared_ptr<const SpriteImage> getSprite(const std::string&);
 
-    // If the sprite is loaded, copies the requsted image from it into the atlas and returns
-    // the resulting icon measurements. If not, returns an empty optional.
-    optional<SpriteAtlasElement> getImage(const std::string& name, SpritePatternMode mode);
+    optional<SpriteAtlasElement> getIcon(const std::string& name);
+    optional<SpriteAtlasElement> getPattern(const std::string& name);
 
     // Binds the atlas texture to the GPU, and uploads data if it is out of date.
     void bind(bool linear, gl::Context&, gl::TextureUnit unit);
@@ -85,6 +72,7 @@ public:
     float getPixelRatio() const { return pixelRatio; }
 
     // Only for use in tests.
+    void setSprites(const Sprites& sprites);
     const PremultipliedImage& getAtlasImage() const {
         return image;
     }
@@ -93,7 +81,6 @@ private:
     void _setSprite(const std::string&, const std::shared_ptr<const SpriteImage>& = nullptr);
     void emitSpriteLoadedIfComplete();
 
-    void copy(const PremultipliedImage&, Rect<uint16_t>, SpritePatternMode);
 
     const Size size;
     const float pixelRatio;
@@ -107,10 +94,18 @@ private:
 
     struct Entry {
         std::shared_ptr<const SpriteImage> spriteImage;
+
+        // One sprite image might be used as both an icon image and a pattern image. If so,
+        // it must have two distinct entries in the texture. The one for the icon image has
+        // a single pixel transparent border, and the one for the pattern image has a single
+        // pixel border wrapped from the opposite side.
         optional<Rect<uint16_t>> iconRect;
         optional<Rect<uint16_t>> patternRect;
     };
 
+    optional<SpriteAtlasElement> getImage(const std::string& name, optional<Rect<uint16_t>> Entry::*rect);
+    void copy(const Entry&, optional<Rect<uint16_t>> Entry::*rect);
+
     std::mutex mutex;
     std::unordered_map<std::string, Entry> entries;
     BinPack<uint16_t> bin;
diff --git a/test/sprite/sprite_atlas.test.cpp b/test/sprite/sprite_atlas.test.cpp
index da8a781585..2c425a95d2 100644
--- a/test/sprite/sprite_atlas.test.cpp
+++ b/test/sprite/sprite_atlas.test.cpp
@@ -28,7 +28,7 @@ TEST(SpriteAtlas, Basic) {
     EXPECT_EQ(63u, atlas.getSize().width);
     EXPECT_EQ(112u, atlas.getSize().height);
 
-    auto metro = *atlas.getImage("metro", SpritePatternMode::Single);
+    auto metro = *atlas.getIcon("metro");
     EXPECT_EQ(0, metro.pos.x);
     EXPECT_EQ(0, metro.pos.y);
     EXPECT_EQ(20, metro.pos.w);
@@ -42,7 +42,7 @@ TEST(SpriteAtlas, Basic) {
     EXPECT_EQ(63u, atlas.getAtlasImage().size.width);
     EXPECT_EQ(112u, atlas.getAtlasImage().size.height);
 
-    auto pos = *atlas.getImage("metro", SpritePatternMode::Single);
+    auto pos = *atlas.getIcon("metro");
     EXPECT_DOUBLE_EQ(18, pos.size[0]);
     EXPECT_DOUBLE_EQ(18, pos.size[1]);
     EXPECT_DOUBLE_EQ(1.0f / 63, pos.tl[0]);
@@ -50,7 +50,7 @@ TEST(SpriteAtlas, Basic) {
     EXPECT_DOUBLE_EQ(19.0f / 63, pos.br[0]);
     EXPECT_DOUBLE_EQ(19.0f / 112, pos.br[1]);
 
-    auto missing = atlas.getImage("doesnotexist", SpritePatternMode::Single);
+    auto missing = atlas.getIcon("doesnotexist");
     EXPECT_FALSE(missing);
 
     EXPECT_EQ(1u, log.count({
@@ -61,7 +61,7 @@ TEST(SpriteAtlas, Basic) {
                   }));
 
     // Different wrapping mode produces different image.
-    auto metro2 = *atlas.getImage("metro", SpritePatternMode::Repeating);
+    auto metro2 = *atlas.getPattern("metro");
     EXPECT_EQ(20, metro2.pos.x);
     EXPECT_EQ(0, metro2.pos.y);
     EXPECT_EQ(20, metro2.pos.w);
@@ -81,7 +81,7 @@ TEST(SpriteAtlas, Size) {
     EXPECT_EQ(63u, atlas.getSize().width);
     EXPECT_EQ(112u, atlas.getSize().height);
 
-    auto metro = *atlas.getImage("metro", SpritePatternMode::Single);
+    auto metro = *atlas.getIcon("metro");
     EXPECT_EQ(0, metro.pos.x);
     EXPECT_EQ(0, metro.pos.y);
     EXPECT_EQ(16, metro.pos.w);
@@ -107,7 +107,7 @@ TEST(SpriteAtlas, Updates) {
     EXPECT_EQ(32u, atlas.getSize().height);
 
     atlas.setSprite("one", std::make_shared<SpriteImage>(PremultipliedImage({ 16, 12 }), 1));
-    auto one = *atlas.getImage("one", SpritePatternMode::Single);
+    auto one = *atlas.getIcon("one");
     EXPECT_EQ(0, one.pos.x);
     EXPECT_EQ(0, one.pos.y);
     EXPECT_EQ(20, one.pos.w);
@@ -189,12 +189,12 @@ TEST(SpriteAtlas, RemoveReleasesBinPackRect) {
     const auto big = std::make_shared<SpriteImage>(PremultipliedImage({ 32, 32 }), 1);
 
     atlas.setSprite("big", big);
-    EXPECT_TRUE(atlas.getImage("big", SpritePatternMode::Single));
+    EXPECT_TRUE(atlas.getIcon("big"));
 
     atlas.removeSprite("big");
 
     atlas.setSprite("big", big);
-    EXPECT_TRUE(atlas.getImage("big", SpritePatternMode::Single));
+    EXPECT_TRUE(atlas.getIcon("big"));
 
     EXPECT_EQ(big, atlas.getSprite("big"));
     EXPECT_TRUE(log.empty());