[core] Replace GlyphRangeSet in onGlyphsAvailable with optionals in the map

GlyphRangeSet isn't keyed by FontStack, so using it to indicate that a particular range was loaded could have produced false positives.

5 files changed, 25 insertions, 49 deletions

diff --git a/src/mbgl/text/glyph.hpp b/src/mbgl/text/glyph.hpp
index 10bdeeea4a..9cf39de840 100644
--- a/src/mbgl/text/glyph.hpp
+++ b/src/mbgl/text/glyph.hpp
@@ -4,6 +4,7 @@
 #include <mbgl/util/font_stack.hpp>
 #include <mbgl/util/rect.hpp>
 #include <mbgl/util/traits.hpp>
+#include <mbgl/util/optional.hpp>
 
 #include <cstdint>
 #include <vector>
@@ -19,17 +20,11 @@ typedef std::set<GlyphID> GlyphIDs;
 GlyphRange getGlyphRange(GlyphID glyph);
 
 struct GlyphMetrics {
-    explicit operator bool() const {
-        return !(width == 0 && height == 0 && advance == 0);
-    }
-
-    // Glyph metrics.
     uint32_t width = 0;
     uint32_t height = 0;
     int32_t left = 0;
     int32_t top = 0;
     uint32_t advance = 0;
-
 };
 
 inline bool operator==(const GlyphMetrics& lhs, const GlyphMetrics& rhs) {
@@ -41,19 +36,12 @@ inline bool operator==(const GlyphMetrics& lhs, const GlyphMetrics& rhs) {
 }
 
 struct Glyph {
-    explicit Glyph() : rect(0, 0, 0, 0), metrics() {}
-    explicit Glyph(Rect<uint16_t> rect_, GlyphMetrics metrics_)
-        : rect(std::move(rect_)), metrics(std::move(metrics_)) {}
-
-    explicit operator bool() const {
-        return metrics || rect.hasArea();
-    }
-
-    const Rect<uint16_t> rect;
-    const GlyphMetrics metrics;
+    Rect<uint16_t> rect;
+    GlyphMetrics metrics;
 };
 
-typedef std::map<GlyphID, Glyph> GlyphPositions;
+typedef std::map<GlyphID, optional<Glyph>> GlyphPositions;
+typedef std::map<FontStack, GlyphPositions> GlyphPositionMap;
 
 class PositionedGlyph {
 public:
@@ -111,7 +99,6 @@ constexpr WritingModeType operator~(WritingModeType value) {
 
 typedef std::map<FontStack,GlyphIDs> GlyphDependencies;
 typedef std::map<FontStack,GlyphRangeSet> GlyphRangeDependencies;
-typedef std::map<FontStack,GlyphPositions> GlyphPositionMap;
 
 
 } // end namespace mbgl
diff --git a/src/mbgl/text/glyph_atlas.cpp b/src/mbgl/text/glyph_atlas.cpp
index 2b09bc5763..04b48f11c9 100644
--- a/src/mbgl/text/glyph_atlas.cpp
+++ b/src/mbgl/text/glyph_atlas.cpp
@@ -137,7 +137,6 @@ void GlyphAtlas::setObserver(GlyphAtlasObserver* observer_) {
 
 void GlyphAtlas::addGlyphs(GlyphRequestor& requestor, const GlyphDependencies& glyphDependencies) {
     GlyphPositionMap glyphPositions;
-    GlyphRangeSet loadedRanges;
 
     for (const auto& dependency : glyphDependencies) {
         const FontStack& fontStack = dependency.first;
@@ -147,27 +146,23 @@ void GlyphAtlas::addGlyphs(GlyphRequestor& requestor, const GlyphDependencies& g
         Entry& entry = entries[fontStack];
 
         for (const auto& glyphID : glyphIDs) {
-            loadedRanges.insert(getGlyphRange(glyphID));
+            optional<Glyph>& glyph = positions[glyphID];
+
             auto it = entry.sdfs.find(glyphID);
             if (it == entry.sdfs.end())
                 continue;
 
             addGlyph(requestor, fontStack, it->second);
 
-            // It's possible to have an SDF without a valid position (if the SDF was malformed).
-            // We indicate this case with Rect<uint16_t>(0,0,0,0).
-            auto glyphRect = entry.glyphValues.find(glyphID);
-            const Rect<uint16_t> rect = glyphRect == entry.glyphValues.end()
-                ? Rect<uint16_t>(0,0,0,0)
-                : glyphRect->second.rect;
-
-            positions.emplace(std::piecewise_construct,
-                              std::forward_as_tuple(glyphID),
-                              std::forward_as_tuple(rect, it->second.metrics));
+            auto valueIt = entry.glyphValues.find(glyphID);
+            glyph = Glyph {
+                valueIt == entry.glyphValues.end() ? Rect<uint16_t>() : valueIt->second.rect,
+                it->second.metrics
+            };
         }
     }
 
-    requestor.onGlyphsAvailable(glyphPositions, loadedRanges);
+    requestor.onGlyphsAvailable(glyphPositions);
 }
 
 void GlyphAtlas::addGlyph(GlyphRequestor& requestor,
diff --git a/src/mbgl/text/glyph_atlas.hpp b/src/mbgl/text/glyph_atlas.hpp
index ef1ddc85ed..74fb22661c 100644
--- a/src/mbgl/text/glyph_atlas.hpp
+++ b/src/mbgl/text/glyph_atlas.hpp
@@ -31,7 +31,7 @@ class Context;
 
 class GlyphRequestor {
 public:
-    virtual void onGlyphsAvailable(GlyphPositionMap, GlyphRangeSet) = 0;
+    virtual void onGlyphsAvailable(GlyphPositionMap) = 0;
 };
     
 class GlyphAtlas : public util::noncopyable {
diff --git a/src/mbgl/text/quads.cpp b/src/mbgl/text/quads.cpp
index bf7791581a..e1a9699835 100644
--- a/src/mbgl/text/quads.cpp
+++ b/src/mbgl/text/quads.cpp
@@ -303,17 +303,11 @@ SymbolQuads getGlyphQuads(Anchor& anchor,
 
     for (const PositionedGlyph &positionedGlyph: shapedText.positionedGlyphs) {
         auto face_it = face.find(positionedGlyph.glyph);
-        if (face_it == face.end())
-            continue;
-        const Glyph &glyph = face_it->second;
-        const Rect<uint16_t> &rect = glyph.rect;
-
-        if (!glyph)
-            continue;
-
-        if (!rect.hasArea())
+        if (face_it == face.end() || !face_it->second || !(*face_it->second).rect.hasArea())
             continue;
 
+        const Glyph& glyph = *face_it->second;
+        const Rect<uint16_t>& rect = glyph.rect;
         const float centerX = (positionedGlyph.x + glyph.metrics.advance / 2.0f) * boxScale;
 
         GlyphInstances glyphInstances;
diff --git a/src/mbgl/text/shaping.cpp b/src/mbgl/text/shaping.cpp
index 5fae03b4c7..78aa142c61 100644
--- a/src/mbgl/text/shaping.cpp
+++ b/src/mbgl/text/shaping.cpp
@@ -56,8 +56,8 @@ void justifyLine(std::vector<PositionedGlyph>& positionedGlyphs,
     
     PositionedGlyph& glyph = positionedGlyphs[end];
     auto it = glyphs.find(glyph.glyph);
-    if (it != glyphs.end()) {
-        const uint32_t lastAdvance = it->second.metrics.advance;
+    if (it != glyphs.end() && it->second) {
+        const uint32_t lastAdvance = it->second->metrics.advance;
         const float lineIndent = float(glyph.x + lastAdvance) * justify;
         
         for (std::size_t j = start; j <= end; j++) {
@@ -74,8 +74,8 @@ float determineAverageLineWidth(const std::u16string& logicalInput,
     
     for (char16_t chr : logicalInput) {
         auto it = glyphs.find(chr);
-        if (it != glyphs.end()) {
-            totalWidth += it->second.metrics.advance + spacing;
+        if (it != glyphs.end() && it->second) {
+            totalWidth += it->second->metrics.advance + spacing;
         }
     }
     
@@ -185,8 +185,8 @@ std::set<std::size_t> determineLineBreaks(const std::u16string& logicalInput,
     for (std::size_t i = 0; i < logicalInput.size(); i++) {
         const char16_t codePoint = logicalInput[i];
         auto it = glyphs.find(codePoint);
-        if (it != glyphs.end() && !boost::algorithm::is_any_of(u" \t\n\v\f\r")(codePoint)) {
-            currentX += it->second.metrics.advance + spacing;
+        if (it != glyphs.end() && it->second && !boost::algorithm::is_any_of(u" \t\n\v\f\r")(codePoint)) {
+            currentX += it->second->metrics.advance + spacing;
         }
         
         // Ideographic characters, spaces, and word-breaking punctuation that often appear without
@@ -234,11 +234,11 @@ void shapeLines(Shaping& shaping,
         std::size_t lineStartIndex = shaping.positionedGlyphs.size();
         for (char16_t chr : line) {
             auto it = glyphs.find(chr);
-            if (it == glyphs.end()) {
+            if (it == glyphs.end() || !it->second) {
                 continue;
             }
             
-            const Glyph& glyph = it->second;
+            const Glyph& glyph = *it->second;
             
             if (writingMode == WritingModeType::Horizontal || !util::i18n::hasUprightVerticalOrientation(chr)) {
                 shaping.positionedGlyphs.emplace_back(chr, x, y, 0);