[core] Move de/ascender to be font-level member instead of glyph level

14 files changed, 119 insertions, 83 deletions

diff --git a/src/mbgl/text/glyph.hpp b/src/mbgl/text/glyph.hpp
index 73e9ec9715..4ba93985c7 100644
--- a/src/mbgl/text/glyph.hpp
+++ b/src/mbgl/text/glyph.hpp
@@ -30,13 +30,11 @@ struct GlyphMetrics {
     int32_t left = 0;
     int32_t top = 0;
     uint32_t advance = 0U;
-    int32_t ascender = 0;
-    int32_t descender = 0;
 };
 
 inline bool operator==(const GlyphMetrics& lhs, const GlyphMetrics& rhs) {
     return lhs.width == rhs.width && lhs.height == rhs.height && lhs.left == rhs.left && lhs.top == rhs.top &&
-           lhs.advance == rhs.advance && lhs.ascender == rhs.ascender && lhs.descender == rhs.descender;
+           lhs.advance == rhs.advance;
 }
 
 class Glyph {
@@ -55,8 +53,9 @@ public:
 };
 
 struct Glyphs {
-    std::map<GlyphID, optional<Immutable<Glyph>>> glyphs;
-    bool hasBaseline;
+    std::map<GlyphID, optional<Immutable<Glyph>>> glyphs{};
+    optional<int32_t> ascender{nullopt};
+    optional<int32_t> descender{nullopt};
 };
 using GlyphMap = std::map<FontStackHash, Glyphs>;
 
diff --git a/src/mbgl/text/glyph_atlas.cpp b/src/mbgl/text/glyph_atlas.cpp
index 601582cc49..61dfb775a3 100644
--- a/src/mbgl/text/glyph_atlas.cpp
+++ b/src/mbgl/text/glyph_atlas.cpp
@@ -15,8 +15,9 @@ GlyphAtlas makeGlyphAtlas(const GlyphMap& glyphs) {
 
     for (const auto& glyphMapEntry : glyphs) {
         FontStackHash fontStack = glyphMapEntry.first;
-        GlyphPositionMap& positions = result.positions[fontStack];
-
+        GlyphPositionData& positions = result.positions[fontStack];
+        positions.ascender = glyphMapEntry.second.ascender;
+        positions.descender = glyphMapEntry.second.descender;
         for (const auto& entry : glyphMapEntry.second.glyphs) {
             if (entry.second && (*entry.second)->bitmap.valid()) {
                 const Glyph& glyph = **entry.second;
@@ -39,16 +40,12 @@ GlyphAtlas makeGlyphAtlas(const GlyphMap& glyphs) {
                                  },
                                  glyph.bitmap.size);
 
-                positions.emplace(glyph.id,
-                                  GlyphPosition {
-                                     Rect<uint16_t> {
-                                         static_cast<uint16_t>(bin.x),
-                                         static_cast<uint16_t>(bin.y),
-                                         static_cast<uint16_t>(bin.w),
-                                         static_cast<uint16_t>(bin.h)
-                                     },
-                                     glyph.metrics
-                                  });
+                positions.glyphPositionMap.emplace(glyph.id,
+                                                   GlyphPosition{Rect<uint16_t>{static_cast<uint16_t>(bin.x),
+                                                                                static_cast<uint16_t>(bin.y),
+                                                                                static_cast<uint16_t>(bin.w),
+                                                                                static_cast<uint16_t>(bin.h)},
+                                                                 glyph.metrics});
             }
         }
     }
diff --git a/src/mbgl/text/glyph_atlas.hpp b/src/mbgl/text/glyph_atlas.hpp
index 9dd063ef69..fa9140c3c2 100644
--- a/src/mbgl/text/glyph_atlas.hpp
+++ b/src/mbgl/text/glyph_atlas.hpp
@@ -11,8 +11,13 @@ struct GlyphPosition {
     GlyphMetrics metrics;
 };
 
-using GlyphPositionMap = std::map<GlyphID, GlyphPosition>;
-using GlyphPositions = std::map<FontStackHash, GlyphPositionMap>;
+struct GlyphPositionData {
+    std::map<GlyphID, GlyphPosition> glyphPositionMap{};
+    optional<int32_t> ascender{nullopt};
+    optional<int32_t> descender{nullopt};
+};
+
+using GlyphPositions = std::map<FontStackHash, GlyphPositionData>;
 
 class GlyphAtlas {
 public:
diff --git a/src/mbgl/text/glyph_manager.cpp b/src/mbgl/text/glyph_manager.cpp
index 8b37b652ab..2a1132e758 100644
--- a/src/mbgl/text/glyph_manager.cpp
+++ b/src/mbgl/text/glyph_manager.cpp
@@ -89,9 +89,9 @@ void GlyphManager::processResponse(const Response& res, const FontStack& fontSta
 
     if (!res.noContent) {
         std::vector<Glyph> glyphs;
-
+        int32_t ascender{0}, descender{0};
         try {
-            std::tie(glyphs, entry.hasBaseline) = parseGlyphPBF(range, *res.data);
+            std::tie(glyphs, ascender, descender) = parseGlyphPBF(range, *res.data);
         } catch (...) {
             observer->onGlyphsError(fontStack, range, std::current_exception());
             return;
@@ -104,6 +104,10 @@ void GlyphManager::processResponse(const Response& res, const FontStack& fontSta
                 entry.glyphs.emplace(id, makeMutable<Glyph>(std::move(glyph)));
             }
         }
+        if (ascender != 0 || descender != 0) {
+            entry.ascender = std::move(ascender);
+            entry.descender = std::move(descender);
+        }
     }
 
     request.parsed = true;
@@ -134,7 +138,8 @@ void GlyphManager::notify(GlyphRequestor& requestor, const GlyphDependencies& gl
 
         Glyphs& glyphs = response[FontStackHasher()(fontStack)];
         Entry& entry = entries[fontStack];
-        glyphs.hasBaseline = entry.hasBaseline;
+        glyphs.ascender = entry.ascender;
+        glyphs.descender = entry.descender;
 
         for (const auto& glyphID : glyphIDs) {
             auto it = entry.glyphs.find(glyphID);
@@ -146,7 +151,7 @@ void GlyphManager::notify(GlyphRequestor& requestor, const GlyphDependencies& gl
         }
     }
 
-    requestor.onGlyphsAvailable(response);
+    requestor.onGlyphsAvailable(std::move(response));
 }
 
 void GlyphManager::removeRequestor(GlyphRequestor& requestor) {
diff --git a/src/mbgl/text/glyph_manager.hpp b/src/mbgl/text/glyph_manager.hpp
index 1955fb752c..b4c8570926 100644
--- a/src/mbgl/text/glyph_manager.hpp
+++ b/src/mbgl/text/glyph_manager.hpp
@@ -61,7 +61,8 @@ private:
     struct Entry {
         std::map<GlyphRange, GlyphRequest> ranges;
         std::map<GlyphID, Immutable<Glyph>> glyphs;
-        bool hasBaseline;
+        optional<int32_t> ascender;
+        optional<int32_t> descender;
     };
 
     std::unordered_map<FontStack, Entry, FontStackHasher> entries;
diff --git a/src/mbgl/text/glyph_pbf.cpp b/src/mbgl/text/glyph_pbf.cpp
index 4658bc69da..eec9bd2f49 100644
--- a/src/mbgl/text/glyph_pbf.cpp
+++ b/src/mbgl/text/glyph_pbf.cpp
@@ -4,12 +4,14 @@
 
 namespace mbgl {
 
-std::pair<std::vector<Glyph>, bool> parseGlyphPBF(const GlyphRange& glyphRange, const std::string& data) {
+std::tuple<std::vector<Glyph>, int32_t, int32_t> parseGlyphPBF(const GlyphRange& glyphRange, const std::string& data) {
     std::vector<Glyph> glyphs;
     glyphs.reserve(256);
+    int32_t ascender{0}, descender{0};
+    bool ascenderSet{false}, descenderSet{0};
 
     protozero::pbf_reader glyphs_pbf(data);
-    bool hasBaseline{true};
+
     while (glyphs_pbf.next(1)) {
         auto readGlyphMetrics = [glyphRange, &glyphs](protozero::pbf_reader& fontstack_pbf) {
             auto glyph_pbf = fontstack_pbf.get_message();
@@ -81,7 +83,6 @@ std::pair<std::vector<Glyph>, bool> parseGlyphPBF(const GlyphRange& glyphRange,
             glyphs.push_back(std::move(glyph));
         };
 
-        int32_t ascender{0}, descender{0};
         uint16_t count{0};
         auto fontstack_pbf = glyphs_pbf.get_message();
         while (fontstack_pbf.next()) {
@@ -92,11 +93,27 @@ std::pair<std::vector<Glyph>, bool> parseGlyphPBF(const GlyphRange& glyphRange,
                     break;
                 }
                 case 4: {
-                    ascender = fontstack_pbf.get_sint32();
+                    // ascender value for one fontstack shall keep the same, if different values appear, set ascender to
+                    // be 0.
+                    const auto value = fontstack_pbf.get_sint32();
+                    if (!ascenderSet) {
+                        ascender = value;
+                        ascenderSet = true;
+                    } else if (ascender != value) {
+                        ascender = 0;
+                    }
                     break;
                 }
                 case 5: {
-                    descender = fontstack_pbf.get_sint32();
+                    // descender value for one fontstack shall keep the same, if different values appear, set descender
+                    // to be 0.
+                    const auto value = fontstack_pbf.get_sint32();
+                    if (!descenderSet) {
+                        descender = value;
+                        descenderSet = true;
+                    } else if (descender != value) {
+                        descender = 0;
+                    }
                     break;
                 }
                 default: {
@@ -105,19 +122,9 @@ std::pair<std::vector<Glyph>, bool> parseGlyphPBF(const GlyphRange& glyphRange,
                 }
             }
         }
-        if (hasBaseline && (ascender != 0.0 || descender != 0.0)) {
-            assert(count <= glyphs.size());
-            const auto lastIndex = glyphs.size() - 1;
-            for (uint16_t i = glyphs.size() - count; i <= lastIndex; ++i) {
-                glyphs[i].metrics.ascender = ascender;
-                glyphs[i].metrics.descender = descender;
-            }
-        } else {
-            hasBaseline = false;
-        }
     }
 
-    return std::make_pair(std::move(glyphs), hasBaseline);
+    return std::make_tuple(std::move(glyphs), ascender, descender);
 }
 
 } // namespace mbgl
diff --git a/src/mbgl/text/glyph_pbf.hpp b/src/mbgl/text/glyph_pbf.hpp
index 6adbb9137f..5e938a6301 100644
--- a/src/mbgl/text/glyph_pbf.hpp
+++ b/src/mbgl/text/glyph_pbf.hpp
@@ -4,11 +4,11 @@
 #include <mbgl/text/glyph_range.hpp>
 
 #include <string>
-#include <utility>
+#include <tuple>
 #include <vector>
 
 namespace mbgl {
 
-std::pair<std::vector<Glyph>, bool> parseGlyphPBF(const GlyphRange&, const std::string& data);
+std::tuple<std::vector<Glyph>, int32_t, int32_t> parseGlyphPBF(const GlyphRange&, const std::string& data);
 
 } // namespace mbgl
diff --git a/src/mbgl/text/quads.cpp b/src/mbgl/text/quads.cpp
index c703f331bc..c6bc3c2bdc 100644
--- a/src/mbgl/text/quads.cpp
+++ b/src/mbgl/text/quads.cpp
@@ -72,10 +72,11 @@ SymbolQuads getGlyphQuads(const Shaping& shapedText,
         auto fontPositions = positions.find(positionedGlyph.font);
         if (fontPositions == positions.end())
             continue;
-        
-        auto positionsIt = fontPositions->second.find(positionedGlyph.glyph);
-        if (positionsIt == fontPositions->second.end())
+
+        auto positionsIt = fontPositions->second.glyphPositionMap.find(positionedGlyph.glyph);
+        if (positionsIt == fontPositions->second.glyphPositionMap.end()) {
             continue;
+        }
 
         const GlyphPosition& glyph = positionsIt->second;
         const Rect<uint16_t>& rect = glyph.rect;
@@ -124,9 +125,11 @@ SymbolQuads getGlyphQuads(const Shaping& shapedText,
             // necessary, but we also pull the glyph to the left along the x axis.
             // The y coordinate includes baseline yOffset, therefore, needs to be accounted
             // for when glyph is rotated and translated.
-
-            float yShift =
-                shapedText.hasBaseline ? (-glyph.metrics.ascender + glyph.metrics.descender) / 2 : Shaping::yOffset;
+            float yShift = Shaping::yOffset;
+            if (shapedText.hasBaseline && fontPositions->second.ascender.has_value() &&
+                fontPositions->second.descender.has_value()) {
+                yShift = (-fontPositions->second.ascender.value() + fontPositions->second.descender.value()) / 2;
+            }
             const Point<float> center{-halfAdvance, halfAdvance - yShift};
             const float verticalRotation = -M_PI_2;
 
diff --git a/src/mbgl/text/shaping.cpp b/src/mbgl/text/shaping.cpp
index ff0f7dee15..a0c0c9d976 100644
--- a/src/mbgl/text/shaping.cpp
+++ b/src/mbgl/text/shaping.cpp
@@ -337,7 +337,7 @@ void shapeLines(Shaping& shaping,
             if (glyphs == glyphMap.end()) {
                 continue;
             }
-            hasBaseline = glyphs->second.hasBaseline;
+            hasBaseline = (glyphs->second.ascender.has_value() && glyphs->second.descender.has_value());
             if (!hasBaseline) break;
         }
         if (!hasBaseline) break;
@@ -381,9 +381,10 @@ void shapeLines(Shaping& shaping,
             // with different offset shift. If font's baseline is not applicable, fall back to use a default baseline
             // offset, see shaping.yOffset. Since we're laying out at 24 points, we need also calculate how much it will
             // move when we scale up or down.
-            const double baselineOffset =
-                (hasBaseline ? ((-glyph.metrics.ascender + glyph.metrics.descender) / 2 * section.scale)
-                             : shaping.yOffset) +
+            const float baselineOffset =
+                (hasBaseline
+                     ? ((-(glyphs->second.ascender.value()) + glyphs->second.descender.value()) / 2.0 * section.scale)
+                     : shaping.yOffset) +
                 (lineMaxScale - section.scale) * util::ONE_EM;
 
             if (writingMode == WritingModeType::Horizontal ||
@@ -417,8 +418,13 @@ void shapeLines(Shaping& shaping,
 
     auto anchorAlign = AnchorAlignment::getAnchorAlignment(textAnchor);
 
-    align(shaping, justify, anchorAlign.horizontalAlign, anchorAlign.verticalAlign, maxLineLength,
-          lineHeight, lines.size());
+    align(shaping,
+          justify,
+          anchorAlign.horizontalAlign,
+          anchorAlign.verticalAlign,
+          maxLineLength,
+          lineHeight,
+          lines.size());
     const float height = y;
     // Calculate the bounding box
     shaping.top += -anchorAlign.verticalAlign * height;
diff --git a/src/mbgl/text/tagged_string.hpp b/src/mbgl/text/tagged_string.hpp
index 698e539a45..966ef0bc61 100644
--- a/src/mbgl/text/tagged_string.hpp
+++ b/src/mbgl/text/tagged_string.hpp
@@ -38,12 +38,12 @@ struct SectionOptions {
 struct TaggedString {
     TaggedString() = default;
 
-    TaggedString(std::u16string text_, SectionOptions options)
-        : styledText(std::move(text_),
-                     std::vector<uint8_t>(text_.size(), 0)) {
+    TaggedString(std::u16string text_, SectionOptions options) {
+        styledText.second = std::vector<uint8_t>(text_.size(), 0);
+        styledText.first = std::move(text_);
         sections.push_back(std::move(options));
     }
-    
+
     TaggedString(StyledText styledText_, std::vector<SectionOptions> sections_)
         : styledText(std::move(styledText_))
         , sections(std::move(sections_)) {
diff --git a/src/mbgl/tile/geometry_tile_worker.cpp b/src/mbgl/tile/geometry_tile_worker.cpp
index 5ac168de9d..5bff71bdc6 100644
--- a/src/mbgl/tile/geometry_tile_worker.cpp
+++ b/src/mbgl/tile/geometry_tile_worker.cpp
@@ -259,8 +259,9 @@ void GeometryTileWorker::onGlyphsAvailable(GlyphMap newGlyphMap) {
         Glyphs& newGlyphs = newFontGlyphs.second;
 
         Glyphs& glyphs = glyphMap[fontStack];
-        glyphs.hasBaseline =
-            glyphs.glyphs.empty() ? newGlyphs.hasBaseline : glyphs.hasBaseline && newGlyphs.hasBaseline;
+        glyphs.ascender = newGlyphs.ascender;
+        glyphs.descender = newGlyphs.descender;
+
         for (auto& pendingGlyphDependency : pendingGlyphDependencies) {
             // Linear lookup here to handle reverse of FontStackHash -> FontStack,
             // since dependencies need the full font stack name to make a request
diff --git a/test/text/glyph_manager.test.cpp b/test/text/glyph_manager.test.cpp
index cdb98702cf..90551a5f1f 100644
--- a/test/text/glyph_manager.test.cpp
+++ b/test/text/glyph_manager.test.cpp
@@ -32,8 +32,6 @@ public:
         stub.metrics.left = 0;
         stub.metrics.top = -8;
         stub.metrics.advance = 24;
-        stub.metrics.ascender = 0;
-        stub.metrics.descender = 0;
 
         stub.bitmap = AlphaImage(Size(30, 30), stubBitmap, stubBitmapLength);
 
diff --git a/test/text/glyph_pbf.test.cpp b/test/text/glyph_pbf.test.cpp
index 1f87403982..462f073b22 100644
--- a/test/text/glyph_pbf.test.cpp
+++ b/test/text/glyph_pbf.test.cpp
@@ -7,10 +7,12 @@ using namespace mbgl;
 
 TEST(GlyphPBF, Parsing) {
     // The fake glyphs contain a number of invalid glyphs, which should be skipped by the parser.
-    auto sdfs = parseGlyphPBF(GlyphRange { 0, 255 }, util::read_file("test/fixtures/resources/fake_glyphs-0-255.pbf"));
-    ASSERT_EQ(1, sdfs.first.size());
-    EXPECT_FALSE(sdfs.second);
-    const auto& sdf = sdfs.first[0];
+    std::vector<Glyph> sdfs;
+    int32_t ascender, descender;
+    std::tie(sdfs, ascender, descender) =
+        parseGlyphPBF(GlyphRange{0, 255}, util::read_file("test/fixtures/resources/fake_glyphs-0-255.pbf"));
+    ASSERT_EQ(1, sdfs.size());
+    const auto& sdf = sdfs[0];
     EXPECT_EQ(69u, sdf.id);
     AlphaImage expected({7, 7});
     expected.fill('x');
@@ -20,6 +22,6 @@ TEST(GlyphPBF, Parsing) {
     EXPECT_EQ(20, sdf.metrics.left);
     EXPECT_EQ(2, sdf.metrics.top);
     EXPECT_EQ(8u, sdf.metrics.advance);
-    EXPECT_EQ(0, sdf.metrics.ascender);
-    EXPECT_EQ(0, sdf.metrics.descender);
+    EXPECT_EQ(0, ascender);
+    EXPECT_EQ(0, descender);
 }
diff --git a/test/text/shaping.test.cpp b/test/text/shaping.test.cpp
index e034ce1503..142fcfbffb 100644
--- a/test/text/shaping.test.cpp
+++ b/test/text/shaping.test.cpp
@@ -6,6 +6,8 @@
 #include <mbgl/text/shaping.hpp>
 #include <mbgl/util/constants.hpp>
 
+#include <vector>
+
 using namespace mbgl;
 using namespace util;
 
@@ -17,8 +19,6 @@ TEST(Shaping, ZWSP) {
     glyph.metrics.left = 2;
     glyph.metrics.top = -8;
     glyph.metrics.advance = 21;
-    glyph.metrics.ascender = 0;
-    glyph.metrics.descender = 0;
 
     BiDi bidi;
     auto immutableGlyph = Immutable<Glyph>(makeMutable<Glyph>(std::move(glyph)));
@@ -26,7 +26,6 @@ TEST(Shaping, ZWSP) {
     const SectionOptions sectionOptions(1.0f, fontStack);
     Glyphs glyphData;
     glyphData.glyphs.emplace(u'中', std::move(immutableGlyph));
-    glyphData.hasBaseline = false;
     GlyphMap glyphs = {{FontStackHasher()(fontStack), std::move(glyphData)}};
 
     const auto testGetShaping = [&](const TaggedString& string, unsigned maxWidthInChars) {
@@ -111,8 +110,6 @@ TEST(Shaping, FontWithBaseline) {
     glyph1.metrics.left = 2;
     glyph1.metrics.top = -8;
     glyph1.metrics.advance = 21;
-    glyph1.metrics.ascender = 26;
-    glyph1.metrics.descender = -6;
 
     Glyph glyph2;
     glyph2.id = u'光';
@@ -121,17 +118,26 @@ TEST(Shaping, FontWithBaseline) {
     glyph2.metrics.left = 2;
     glyph2.metrics.top = -8;
     glyph2.metrics.advance = 21;
-    glyph2.metrics.ascender = 25;
-    glyph2.metrics.descender = -5;
 
     BiDi bidi;
-    const std::vector<std::string> fontStack{{"font-stack"}};
-    const SectionOptions sectionOptions(1.0f, fontStack);
-    Glyphs glyphData;
-    glyphData.glyphs.emplace(u'阳', Immutable<Glyph>(makeMutable<Glyph>(std::move(glyph1))));
-    glyphData.glyphs.emplace(u'光', Immutable<Glyph>(makeMutable<Glyph>(std::move(glyph2))));
-    glyphData.hasBaseline = true;
-    GlyphMap glyphs = {{FontStackHasher()(fontStack), std::move(glyphData)}};
+    std::vector<SectionOptions> sectionOptions;
+    const std::vector<std::string> fontStack1{{"font-stack1"}};
+    sectionOptions.emplace_back(1.0f, fontStack1);
+    Glyphs glyphData1;
+    glyphData1.glyphs.emplace(u'阳', Immutable<Glyph>(makeMutable<Glyph>(std::move(glyph1))));
+    glyphData1.ascender = 26;
+    glyphData1.descender = -6;
+
+    const std::vector<std::string> fontStack2{{"font-stack2"}};
+    sectionOptions.emplace_back(1.0f, fontStack2);
+    Glyphs glyphData2;
+    glyphData2.glyphs.emplace(u'光', Immutable<Glyph>(makeMutable<Glyph>(std::move(glyph2))));
+    glyphData2.ascender = 25;
+    glyphData2.descender = -5;
+
+    GlyphMap glyphs;
+    glyphs.emplace(FontStackHasher()(fontStack1), std::move(glyphData1));
+    glyphs.emplace(FontStackHasher()(fontStack2), std::move(glyphData2));
 
     const auto testGetShaping = [&](const TaggedString& string, unsigned maxWidthInChars) {
         return getShaping(string,
@@ -148,7 +154,13 @@ TEST(Shaping, FontWithBaseline) {
     };
 
     {
-        TaggedString string(u"阳光\u200b", sectionOptions);
+        std::u16string text{u"阳光\u200b"};
+        StyledText styledText;
+        styledText.second = std::vector<uint8_t>{0, 1, 0};
+        styledText.first = std::move(text);
+
+        TaggedString string{styledText, sectionOptions};
+
         auto shaping = testGetShaping(string, 5);
         ASSERT_EQ(shaping.lineCount, 1);
         ASSERT_EQ(shaping.top, -12);