[core] Combine two GlyphID-keyed maps in GlyphAtlas::Entry

2 files changed, 39 insertions, 54 deletions

diff --git a/src/mbgl/text/glyph_atlas.cpp b/src/mbgl/text/glyph_atlas.cpp
index 04b48f11c9..4feaab01f9 100644
--- a/src/mbgl/text/glyph_atlas.cpp
+++ b/src/mbgl/text/glyph_atlas.cpp
@@ -96,10 +96,14 @@ void GlyphAtlas::processResponse(const Response& res, const FontStack& fontStack
         }
 
         for (auto& glyph : glyphs) {
-            auto it = entry.sdfs.find(glyph.id);
-            if (it == entry.sdfs.end()) {
+            auto it = entry.glyphs.find(glyph.id);
+            if (it == entry.glyphs.end()) {
                 // Glyph doesn't exist yet.
-                entry.sdfs.emplace(glyph.id, std::move(glyph));
+                entry.glyphs.emplace(glyph.id, GlyphValue {
+                    std::move(glyph.bitmap),
+                    std::move(glyph.metrics),
+                    {}, {}
+                });
             } else if (it->second.metrics == glyph.metrics) {
                 if (it->second.bitmap != glyph.bitmap) {
                     // The actual bitmap was updated; this is unsupported.
@@ -146,17 +150,19 @@ void GlyphAtlas::addGlyphs(GlyphRequestor& requestor, const GlyphDependencies& g
         Entry& entry = entries[fontStack];
 
         for (const auto& glyphID : glyphIDs) {
+            // Make a glyph position entry even if we didn't get an SDF for the glyph. During layout,
+            // an empty optional is treated as "loaded but nothing to show", wheras no entry in the
+            // positions map means "not loaded yet".
             optional<Glyph>& glyph = positions[glyphID];
 
-            auto it = entry.sdfs.find(glyphID);
-            if (it == entry.sdfs.end())
+            auto it = entry.glyphs.find(glyphID);
+            if (it == entry.glyphs.end())
                 continue;
 
-            addGlyph(requestor, fontStack, it->second);
+            it->second.ids.insert(&requestor);
 
-            auto valueIt = entry.glyphValues.find(glyphID);
             glyph = Glyph {
-                valueIt == entry.glyphValues.end() ? Rect<uint16_t>() : valueIt->second.rect,
+                addGlyph(it->second),
                 it->second.metrics
             };
         }
@@ -165,31 +171,21 @@ void GlyphAtlas::addGlyphs(GlyphRequestor& requestor, const GlyphDependencies& g
     requestor.onGlyphsAvailable(glyphPositions);
 }
 
-void GlyphAtlas::addGlyph(GlyphRequestor& requestor,
-                          const FontStack& fontStack,
-                          const SDFGlyph& glyph)
-{
-    std::map<uint32_t, GlyphValue>& face = entries[fontStack].glyphValues;
-    auto it = face.find(glyph.id);
-
+Rect<uint16_t> GlyphAtlas::addGlyph(GlyphValue& value) {
     // The glyph is already in this texture.
-    if (it != face.end()) {
-        GlyphValue& value = it->second;
-        value.ids.insert(&requestor);
-        return;
+    if (value.rect) {
+        return *value.rect;
     }
 
-    // Guard against glyphs that are too large, or that we don't need to place into the atlas since
-    // they don't have any pixels.
-    if (glyph.metrics.width == 0 || glyph.metrics.width >= 256 ||
-        glyph.metrics.height == 0 || glyph.metrics.height >= 256) {
-        return;
+    // We don't need to add glyphs without a bitmap (e.g. whitespace).
+    if (!value.bitmap.valid()) {
+        return {};
     }
 
     // Add a 1px border around every image.
     const uint32_t padding = 1;
-    uint16_t width = glyph.bitmap.size.width + 2 * padding;
-    uint16_t height = glyph.bitmap.size.height + 2 * padding;
+    uint16_t width = value.bitmap.size.width + 2 * padding;
+    uint16_t height = value.bitmap.size.height + 2 * padding;
 
     // Increase to next number divisible by 4, but at least 1.
     // This is so we can scale down the texture coordinates and pack them
@@ -200,23 +196,21 @@ void GlyphAtlas::addGlyph(GlyphRequestor& requestor,
     Rect<uint16_t> rect = bin.allocate(width, height);
     if (rect.w == 0) {
         Log::Error(Event::OpenGL, "glyph bitmap overflow");
-        return;
+        return {};
     }
 
-    face.emplace(glyph.id, GlyphValue { rect, &requestor });
-
-    AlphaImage::copy(glyph.bitmap, image, { 0, 0 }, { rect.x + padding, rect.y + padding }, glyph.bitmap.size);
-
+    AlphaImage::copy(value.bitmap, image, { 0, 0 }, { rect.x + padding, rect.y + padding }, value.bitmap.size);
+    value.rect = rect;
     dirty = true;
+
+    return rect;
 }
 
-void GlyphAtlas::removeGlyphValues(GlyphRequestor& requestor, std::map<uint32_t, GlyphValue>& face) {
-    for (auto it = face.begin(); it != face.end(); /* we advance in the body */) {
+void GlyphAtlas::removeGlyphValues(GlyphRequestor& requestor, std::map<GlyphID, GlyphValue>& values) {
+    for (auto it = values.begin(); it != values.end(); it++) {
         GlyphValue& value = it->second;
-        value.ids.erase(&requestor);
-
-        if (value.ids.empty()) {
-            const Rect<uint16_t>& rect = value.rect;
+        if (value.ids.erase(&requestor) && value.ids.empty() && value.rect) {
+            const Rect<uint16_t>& rect = *value.rect;
 
             // Clear out the bitmap.
             uint8_t *target = image.data.get();
@@ -228,14 +222,7 @@ void GlyphAtlas::removeGlyphValues(GlyphRequestor& requestor, std::map<uint32_t,
             }
 
             bin.release(rect);
-
-            // Make sure to post-increment the iterator: This will return the
-            // current iterator, but will go to the next position before we
-            // erase the element from the map. That way, the iterator stays
-            // valid.
-            face.erase(it++);
-        } else {
-            ++it;
+            value.rect = {};
         }
     }
 }
@@ -248,7 +235,7 @@ void GlyphAtlas::removePendingRanges(mbgl::GlyphRequestor& requestor, std::map<G
 
 void GlyphAtlas::removeGlyphs(GlyphRequestor& requestor) {
     for (auto& entry : entries) {
-        removeGlyphValues(requestor, entry.second.glyphValues);
+        removeGlyphValues(requestor, entry.second.glyphs);
         removePendingRanges(requestor, entry.second.ranges);
     }
 }
diff --git a/src/mbgl/text/glyph_atlas.hpp b/src/mbgl/text/glyph_atlas.hpp
index 74fb22661c..f4c4601530 100644
--- a/src/mbgl/text/glyph_atlas.hpp
+++ b/src/mbgl/text/glyph_atlas.hpp
@@ -3,7 +3,6 @@
 #include <mbgl/text/glyph.hpp>
 #include <mbgl/text/glyph_atlas_observer.hpp>
 #include <mbgl/text/glyph_range.hpp>
-#include <mbgl/text/glyph_pbf.hpp>
 #include <mbgl/geometry/binpack.hpp>
 #include <mbgl/util/noncopyable.hpp>
 #include <mbgl/util/optional.hpp>
@@ -71,9 +70,9 @@ private:
     std::string glyphURL;
 
     struct GlyphValue {
-        GlyphValue(Rect<uint16_t> rect_, GlyphRequestor* id)
-            : rect(std::move(rect_)), ids({ id }) {}
-        Rect<uint16_t> rect;
+        AlphaImage bitmap;
+        GlyphMetrics metrics;
+        optional<Rect<uint16_t>> rect;
         std::unordered_set<GlyphRequestor*> ids;
     };
 
@@ -85,8 +84,7 @@ private:
 
     struct Entry {
         std::map<GlyphRange, GlyphRequest> ranges;
-        std::map<uint32_t, SDFGlyph> sdfs;
-        std::map<uint32_t, GlyphValue> glyphValues;
+        std::map<GlyphID, GlyphValue> glyphs;
     };
 
     std::unordered_map<FontStack, Entry, FontStackHash> entries;
@@ -95,9 +93,9 @@ private:
     void processResponse(const Response&, const FontStack&, const GlyphRange&);
 
     void addGlyphs(GlyphRequestor&, const GlyphDependencies&);
-    void addGlyph(GlyphRequestor&, const FontStack&, const SDFGlyph&);
+    Rect<uint16_t> addGlyph(GlyphValue&);
 
-    void removeGlyphValues(GlyphRequestor&, std::map<uint32_t, GlyphValue>&);
+    void removeGlyphValues(GlyphRequestor&, std::map<GlyphID, GlyphValue>&);
     void removePendingRanges(GlyphRequestor&, std::map<GlyphRange, GlyphRequest>&);
 
     GlyphAtlasObserver* observer = nullptr;