[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.

10 files changed, 56 insertions, 93 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);
diff --git a/src/mbgl/tile/geometry_tile.cpp b/src/mbgl/tile/geometry_tile.cpp
index 4e67144082..36ac4155d7 100644
--- a/src/mbgl/tile/geometry_tile.cpp
+++ b/src/mbgl/tile/geometry_tile.cpp
@@ -133,8 +133,8 @@ void GeometryTile::onError(std::exception_ptr err) {
     observer->onTileError(*this, err);
 }
     
-void GeometryTile::onGlyphsAvailable(GlyphPositionMap glyphPositions, GlyphRangeSet loadedRanges) {
-    worker.invoke(&GeometryTileWorker::onGlyphsAvailable, std::move(glyphPositions), std::move(loadedRanges));
+void GeometryTile::onGlyphsAvailable(GlyphPositionMap glyphPositions) {
+    worker.invoke(&GeometryTileWorker::onGlyphsAvailable, std::move(glyphPositions));
 }
 
 void GeometryTile::getGlyphs(GlyphDependencies glyphDependencies) {
diff --git a/src/mbgl/tile/geometry_tile.hpp b/src/mbgl/tile/geometry_tile.hpp
index f057e21507..0531917acc 100644
--- a/src/mbgl/tile/geometry_tile.hpp
+++ b/src/mbgl/tile/geometry_tile.hpp
@@ -40,7 +40,7 @@ public:
     void setPlacementConfig(const PlacementConfig&) override;
     void redoLayout() override;
     
-    void onGlyphsAvailable(GlyphPositionMap, GlyphRangeSet) override;
+    void onGlyphsAvailable(GlyphPositionMap) override;
     void onIconsAvailable(SpriteAtlas*, IconMap) override;
     
     void getGlyphs(GlyphDependencies);
diff --git a/src/mbgl/tile/geometry_tile_worker.cpp b/src/mbgl/tile/geometry_tile_worker.cpp
index d1d4c9e9b8..d9b720d226 100644
--- a/src/mbgl/tile/geometry_tile_worker.cpp
+++ b/src/mbgl/tile/geometry_tile_worker.cpp
@@ -196,32 +196,23 @@ void GeometryTileWorker::coalesce() {
     self.invoke(&GeometryTileWorker::coalesced);
 }
 
+void GeometryTileWorker::onGlyphsAvailable(GlyphPositionMap newGlyphPositions) {
+    for (auto& newFontGlyphs : newGlyphPositions) {
+        const FontStack& fontStack = newFontGlyphs.first;
+        GlyphPositions& newPositions = newFontGlyphs.second;
 
-void GeometryTileWorker::onGlyphsAvailable(GlyphPositionMap newGlyphPositions, GlyphRangeSet loadedRanges) {
-    GlyphDependencies loadedGlyphs;
-    for (auto& pendingFontGlyphs : pendingGlyphDependencies) {
-        auto newFontGlyphs = newGlyphPositions.find(pendingFontGlyphs.first);
-        for (auto glyphID : pendingFontGlyphs.second) {
-            if (newFontGlyphs != newGlyphPositions.end()) {
-                auto newFontGlyph = newFontGlyphs->second.find(glyphID);
-                if (newFontGlyph != newFontGlyphs->second.end()) {
-                    glyphPositions[pendingFontGlyphs.first].emplace(glyphID, newFontGlyph->second);
-                }
-            }
-            if (loadedRanges.find(getGlyphRange(glyphID)) != loadedRanges.end()) {
-                // Erase the glyph from our pending font set as long as its range is loaded
-                // If the glyph itself is missing, that means we can't get a glyph for
-                // this fontstack, and we go ahead and render with missing glyphs
-                loadedGlyphs[pendingFontGlyphs.first].insert(glyphID);
+        GlyphPositions& positions = glyphPositions[fontStack];
+        GlyphIDs& pendingGlyphIDs = pendingGlyphDependencies[fontStack];
+
+        for (auto& newPosition : newPositions) {
+            const GlyphID& glyphID = newPosition.first;
+            optional<Glyph>& glyph = newPosition.second;
+
+            if (pendingGlyphIDs.erase(glyphID)) {
+                positions.emplace(glyphID, std::move(glyph));
             }
         }
     }
-    
-    for (auto& loadedFont : loadedGlyphs) {
-        for (auto loadedGlyph : loadedFont.second) {
-            pendingGlyphDependencies[loadedFont.first].erase(loadedGlyph);
-        }
-    }
     symbolDependenciesChanged();
 }
 
diff --git a/src/mbgl/tile/geometry_tile_worker.hpp b/src/mbgl/tile/geometry_tile_worker.hpp
index 39f4411e23..5ce4260fdf 100644
--- a/src/mbgl/tile/geometry_tile_worker.hpp
+++ b/src/mbgl/tile/geometry_tile_worker.hpp
@@ -35,7 +35,7 @@ public:
     void setData(std::unique_ptr<const GeometryTileData>, uint64_t correlationID);
     void setPlacementConfig(PlacementConfig, uint64_t correlationID);
     
-    void onGlyphsAvailable(GlyphPositionMap glyphs, GlyphRangeSet loadedRanges);
+    void onGlyphsAvailable(GlyphPositionMap glyphs);
     void onIconsAvailable(IconAtlasMap icons);
 
 private:
diff --git a/test/text/glyph_atlas.test.cpp b/test/text/glyph_atlas.test.cpp
index e57a7944a4..5aff1ee441 100644
--- a/test/text/glyph_atlas.test.cpp
+++ b/test/text/glyph_atlas.test.cpp
@@ -12,11 +12,11 @@ using namespace mbgl;
 
 class StubGlyphRequestor : public GlyphRequestor {
 public:
-    void onGlyphsAvailable(GlyphPositionMap positions, GlyphRangeSet ranges) override {
-        if (glyphsAvailable) glyphsAvailable(std::move(positions), std::move(ranges));
+    void onGlyphsAvailable(GlyphPositionMap positions) override {
+        if (glyphsAvailable) glyphsAvailable(std::move(positions));
     }
 
-    std::function<void (GlyphPositionMap, GlyphRangeSet)> glyphsAvailable;
+    std::function<void (GlyphPositionMap)> glyphsAvailable;
 };
 
 class GlyphAtlasTest {
@@ -63,15 +63,14 @@ TEST(GlyphAtlas, LoadingSuccess) {
         ASSERT_EQ(range, GlyphRange(0, 255));
     };
 
-    test.requestor.glyphsAvailable = [&] (GlyphPositionMap positions, GlyphRangeSet ranges) {
+    test.requestor.glyphsAvailable = [&] (GlyphPositionMap positions) {
         const auto& testPositions = positions.at({{"Test Stack"}});
 
-        ASSERT_EQ(testPositions.size(), 2u);
+        ASSERT_EQ(testPositions.size(), 3u);
         ASSERT_EQ(testPositions.count(u'a'), 1u);
         ASSERT_EQ(testPositions.count(u'å'), 1u);
-
-        ASSERT_EQ(ranges.size(), 1u);
-        ASSERT_EQ((ranges.count({0, 255})), 1u);
+        ASSERT_EQ(testPositions.count(u' '), 1u);
+        ASSERT_TRUE(bool(testPositions.at(u' ')));
 
         test.end();
     };
@@ -79,7 +78,7 @@ TEST(GlyphAtlas, LoadingSuccess) {
     test.run(
         "test/fixtures/resources/glyphs.pbf",
         GlyphDependencies {
-            {{{"Test Stack"}}, {u'a', u'å'}}
+            {{{"Test Stack"}}, {u'a', u'å', u' '}}
         });
 }
 
@@ -104,7 +103,7 @@ TEST(GlyphAtlas, LoadingFail) {
         test.end();
     };
 
-    test.requestor.glyphsAvailable = [&] (GlyphPositionMap, GlyphRangeSet) {
+    test.requestor.glyphsAvailable = [&] (GlyphPositionMap) {
         FAIL();
         test.end();
     };
@@ -135,7 +134,7 @@ TEST(GlyphAtlas, LoadingCorrupted) {
         test.end();
     };
 
-    test.requestor.glyphsAvailable = [&] (GlyphPositionMap, GlyphRangeSet) {
+    test.requestor.glyphsAvailable = [&] (GlyphPositionMap) {
         FAIL();
         test.end();
     };
@@ -186,15 +185,12 @@ TEST(GlyphAtlas, LoadingInvalid) {
         ASSERT_EQ(range, GlyphRange(0, 255));
     };
 
-    test.requestor.glyphsAvailable = [&] (GlyphPositionMap positions, GlyphRangeSet ranges) {
+    test.requestor.glyphsAvailable = [&] (GlyphPositionMap positions) {
         const auto& testPositions = positions.at({{"Test Stack"}});
 
-        ASSERT_EQ(testPositions.size(), 1u);
-        ASSERT_EQ(testPositions.count(u'A'), 0u);
-        ASSERT_EQ(testPositions.count(u'E'), 1u);
-
-        ASSERT_EQ(ranges.size(), 1u);
-        ASSERT_EQ((ranges.count({0, 255})), 1u);
+        ASSERT_EQ(testPositions.size(), 2u);
+        ASSERT_FALSE(bool(testPositions.at(u'A')));
+        ASSERT_TRUE(bool(testPositions.at(u'E')));
 
         test.end();
     };