[core] Simplify and fix sprite atlas coordinate calculations

* Always return image metrics exclusive of padding
* Work with integer coordinates whenever possible
* Eliminate redundant SpriteAtlasElement members
* Fix asymmetric re-padding in getIconQuad when pixelRatio != 1
* Add explanatory comments

2 files changed, 64 insertions, 42 deletions

diff --git a/src/mbgl/sprite/sprite_atlas.cpp b/src/mbgl/sprite/sprite_atlas.cpp
index bcf35050ab..57a5488105 100644
--- a/src/mbgl/sprite/sprite_atlas.cpp
+++ b/src/mbgl/sprite/sprite_atlas.cpp
@@ -14,28 +14,27 @@
 
 namespace mbgl {
 
-static constexpr uint32_t padding = 1;
-
-SpriteAtlasElement::SpriteAtlasElement(Rect<uint16_t> rect_,
-                                       const style::Image::Impl& image,
-                                       float pixelRatio)
-    : pos(std::move(rect_)),
-      sdf(image.sdf),
-      relativePixelRatio(image.pixelRatio / pixelRatio),
-      size{{image.image.size.width / image.pixelRatio,
-            image.image.size.height / image.pixelRatio}} {
-
-    const float w = image.image.size.width / pixelRatio;
-    const float h = image.image.size.height / pixelRatio;
-
-    tl = {{ float(pos.x + padding),     float(pos.y + padding)     }};
-    br = {{ float(pos.x + padding + w), float(pos.y + padding + h) }};
-}
-
-SpriteAtlas::SpriteAtlas(Size size_, float pixelRatio_)
-    : size(std::move(size_)),
-      pixelRatio(pixelRatio_),
-      bin(size.width, size.height),
+// When copied into the atlas texture, image data is padded by one pixel on each side. Icon
+// images are padded with fully transparent pixels, while pattern images are padded with a
+// copy of the image data wrapped from the opposite side. In both cases, this ensures the
+// correct behavior of GL_LINEAR texture sampling mode.
+static constexpr uint16_t padding = 1;
+
+SpriteAtlasElement::SpriteAtlasElement(Rect<uint16_t> rect, const style::Image::Impl& image)
+    : sdf(image.sdf),
+      pixelRatio(image.pixelRatio),
+      textureRect(
+        rect.x + padding,
+        rect.y + padding,
+        rect.w - padding * 2,
+        rect.h - padding * 2
+      ) {
+}
+
+SpriteAtlas::SpriteAtlas(Size size, float pixelRatio)
+    : pixelSize(std::ceil(size.width * pixelRatio),
+                std::ceil(size.height * pixelRatio)),
+      bin(pixelSize.width, pixelSize.height),
       dirty(true) {
 }
 
@@ -141,13 +140,12 @@ optional<SpriteAtlasElement> SpriteAtlas::getImage(const std::string& id,
         assert(entry.image.get());
         return SpriteAtlasElement {
             *(entry.*entryRect),
-            *entry.image,
-            pixelRatio
+            *entry.image
         };
     }
 
-    const uint16_t width = std::ceil(entry.image->image.size.width / pixelRatio) + 2 * padding;
-    const uint16_t height = std::ceil(entry.image->image.size.height / pixelRatio) + 2 * padding;
+    const uint16_t width = entry.image->image.size.width + padding * 2;
+    const uint16_t height = entry.image->image.size.height + padding * 2;
 
     Rect<uint16_t> rect = bin.allocate(width, height);
     if (rect.w == 0) {
@@ -162,23 +160,25 @@ optional<SpriteAtlasElement> SpriteAtlas::getImage(const std::string& id,
 
     return SpriteAtlasElement {
         rect,
-        *entry.image,
-        pixelRatio
+        *entry.image
     };
 }
 
+Size SpriteAtlas::getPixelSize() const {
+    return pixelSize;
+}
+
 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 = PremultipliedImage(getPixelSize());
         image.fill(0);
     }
 
     const PremultipliedImage& src = entry.image->image;
     const Rect<uint16_t>& rect = *(entry.*entryRect);
 
-    const uint32_t x = (rect.x + padding) * pixelRatio;
-    const uint32_t y = (rect.y + padding) * pixelRatio;
+    const uint32_t x = rect.x + padding;
+    const uint32_t y = rect.y + padding;
     const uint32_t w = src.size.width;
     const uint32_t h = src.size.height;
 
diff --git a/src/mbgl/sprite/sprite_atlas.hpp b/src/mbgl/sprite/sprite_atlas.hpp
index 52749d389b..3629ed6eb1 100644
--- a/src/mbgl/sprite/sprite_atlas.hpp
+++ b/src/mbgl/sprite/sprite_atlas.hpp
@@ -20,15 +20,32 @@ class Context;
 
 class SpriteAtlasElement {
 public:
-    SpriteAtlasElement(Rect<uint16_t>, const style::Image::Impl&, float pixelRatio);
+    SpriteAtlasElement(Rect<uint16_t>, const style::Image::Impl&);
 
-    Rect<uint16_t> pos;
     bool sdf;
+    float pixelRatio;
+    Rect<uint16_t> textureRect;
+
+    std::array<uint16_t, 2> tl() const {
+        return {{
+            textureRect.x,
+            textureRect.y
+        }};
+    }
+
+    std::array<uint16_t, 2> br() const {
+        return {{
+            static_cast<uint16_t>(textureRect.x + textureRect.w),
+            static_cast<uint16_t>(textureRect.y + textureRect.h)
+        }};
+    }
 
-    float relativePixelRatio;
-    std::array<float, 2> size;
-    std::array<float, 2> tl;
-    std::array<float, 2> br;
+    std::array<float, 2> displaySize() const {
+        return {{
+            textureRect.w / pixelRatio,
+            textureRect.h / pixelRatio,
+        }};
+    }
 };
 
 using IconMap = std::unordered_map<std::string, SpriteAtlasElement>;
@@ -64,7 +81,14 @@ public:
     void getIcons(IconRequestor& requestor);
     void removeRequestor(IconRequestor& requestor);
 
+    // Ensure that the atlas contains the named image suitable for rendering as an icon, and
+    // return its metrics. The image will be padded on each side with a one pixel wide transparent
+    // strip, but the returned metrics are exclusive of this padding.
     optional<SpriteAtlasElement> getIcon(const std::string& name);
+
+    // Ensure that the atlas contains the named image suitable for rendering as an pattern, and
+    // return its metrics. The image will be padded on each side with a one pixel wide copy of
+    // pixels from the opposite side, but the returned metrics are exclusive of this padding.
     optional<SpriteAtlasElement> getPattern(const std::string& name);
 
     // Binds the atlas texture to the GPU, and uploads data if it is out of date.
@@ -74,8 +98,7 @@ public:
     // the texture is only bound when the data is out of date (=dirty).
     void upload(gl::Context&, gl::TextureUnit unit);
 
-    Size getSize() const { return size; }
-    float getPixelRatio() const { return pixelRatio; }
+    Size getPixelSize() const;
 
     // Only for use in tests.
     const PremultipliedImage& getAtlasImage() const {
@@ -83,8 +106,7 @@ public:
     }
 
 private:
-    const Size size;
-    const float pixelRatio;
+    const Size pixelSize;
     bool loaded = false;
 
     struct Entry {