diff --git a/src/mbgl/text/glyph.hpp b/src/mbgl/text/glyph.hpp index 10bdeeea4..9cf39de84 100644 --- a/src/mbgl/text/glyph.hpp +++ b/src/mbgl/text/glyph.hpp @@ -4,6 +4,7 @@ #include #include #include +#include #include #include @@ -19,17 +20,11 @@ typedef std::set 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 rect_, GlyphMetrics metrics_) - : rect(std::move(rect_)), metrics(std::move(metrics_)) {} - - explicit operator bool() const { - return metrics || rect.hasArea(); - } - - const Rect rect; - const GlyphMetrics metrics; + Rect rect; + GlyphMetrics metrics; }; -typedef std::map GlyphPositions; +typedef std::map> GlyphPositions; +typedef std::map GlyphPositionMap; class PositionedGlyph { public: @@ -111,7 +99,6 @@ constexpr WritingModeType operator~(WritingModeType value) { typedef std::map GlyphDependencies; typedef std::map GlyphRangeDependencies; -typedef std::map GlyphPositionMap; } // end namespace mbgl diff --git a/src/mbgl/text/glyph_atlas.cpp b/src/mbgl/text/glyph_atlas.cpp index 2b09bc576..04b48f11c 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 = 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(0,0,0,0). - auto glyphRect = entry.glyphValues.find(glyphID); - const Rect rect = glyphRect == entry.glyphValues.end() - ? Rect(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() : 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 ef1ddc85e..74fb22661 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 bf7791581..e1a969983 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 &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& 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 5fae03b4c..78aa142c6 100644 --- a/src/mbgl/text/shaping.cpp +++ b/src/mbgl/text/shaping.cpp @@ -56,8 +56,8 @@ void justifyLine(std::vector& 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 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 4e6714408..36ac4155d 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 f057e2150..0531917ac 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 d1d4c9e9b..d9b720d22 100644 --- a/src/mbgl/tile/geometry_tile_worker.cpp +++ b/src/mbgl/tile/geometry_tile_worker.cpp @@ -196,30 +196,21 @@ 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); - } + GlyphPositions& positions = glyphPositions[fontStack]; + GlyphIDs& pendingGlyphIDs = pendingGlyphDependencies[fontStack]; + + for (auto& newPosition : newPositions) { + const GlyphID& glyphID = newPosition.first; + optional& glyph = newPosition.second; + + if (pendingGlyphIDs.erase(glyphID)) { + positions.emplace(glyphID, std::move(glyph)); } - 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); - } - } - } - - 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 39f4411e2..5ce4260fd 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, 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 e57a7944a..5aff1ee44 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 glyphsAvailable; + std::function 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(); };