From a05789f1b8f8e6466827ecef5f46c1c0a75d6aa6 Mon Sep 17 00:00:00 2001
From: zmiao <zmiao.jamie@gmail.com>
Date: Thu, 19 Sep 2019 13:27:02 +0300
Subject: [PATCH 01/12] [core] Add descender, ascender in glyphMetrics. Fix
 mixed fonts dis-allignment

---
 src/mbgl/text/glyph.hpp     |  2 ++
 src/mbgl/text/glyph_pbf.cpp | 53 ++++++++++++++++++++++++++++---------
 src/mbgl/text/shaping.cpp   | 22 ++++++++-------
 3 files changed, 55 insertions(+), 22 deletions(-)

diff --git a/src/mbgl/text/glyph.hpp b/src/mbgl/text/glyph.hpp
index ba9c521f77b..8977f4992af 100644
--- a/src/mbgl/text/glyph.hpp
+++ b/src/mbgl/text/glyph.hpp
@@ -30,6 +30,8 @@ struct GlyphMetrics {
     int32_t left = 0;
     int32_t top = 0;
     uint32_t advance = 0;
+    double ascender = 0.0;
+    double descender = 0.0;
 };
 
 inline bool operator==(const GlyphMetrics& lhs, const GlyphMetrics& rhs) {
diff --git a/src/mbgl/text/glyph_pbf.cpp b/src/mbgl/text/glyph_pbf.cpp
index cfaf803f75e..d7b176b0423 100644
--- a/src/mbgl/text/glyph_pbf.cpp
+++ b/src/mbgl/text/glyph_pbf.cpp
@@ -11,10 +11,8 @@ std::vector<Glyph> parseGlyphPBF(const GlyphRange& glyphRange, const std::string
     protozero::pbf_reader glyphs_pbf(data);
 
     while (glyphs_pbf.next(1)) {
-        auto fontstack_pbf = glyphs_pbf.get_message();
-        while (fontstack_pbf.next(3)) {
+        auto readGlyphMetrics = [glyphRange, &result](protozero::pbf_reader& fontstack_pbf) {
             auto glyph_pbf = fontstack_pbf.get_message();
-
             Glyph glyph;
             protozero::data_view glyphData;
 
@@ -64,27 +62,58 @@ std::vector<Glyph> parseGlyphPBF(const GlyphRange& glyphRange, const std::string
                 glyph.metrics.width >= 256 || glyph.metrics.height >= 256 ||
                 glyph.metrics.left < -128 || glyph.metrics.left >= 128 ||
                 glyph.metrics.top < -128 || glyph.metrics.top >= 128 ||
-                glyph.metrics.advance >= 256 ||
-                glyph.id < glyphRange.first || glyph.id > glyphRange.second) {
-                continue;
+                glyph.metrics.advance >= 256 || glyph.id < glyphRange.first ||
+                glyph.id > glyphRange.second) {
+                return;
             }
 
             // If the area of width/height is non-zero, we need to adjust the expected size
             // with the implicit border size, otherwise we expect there to be no bitmap at all.
             if (glyph.metrics.width && glyph.metrics.height) {
-                const Size size {
-                    glyph.metrics.width + 2 * Glyph::borderSize,
-                    glyph.metrics.height + 2 * Glyph::borderSize
-                };
+                const Size size{ glyph.metrics.width + 2 * Glyph::borderSize,
+                                 glyph.metrics.height + 2 * Glyph::borderSize };
 
                 if (size.area() != glyphData.size()) {
-                    continue;
+                    return;
                 }
 
-                glyph.bitmap = AlphaImage(size, reinterpret_cast<const uint8_t*>(glyphData.data()), glyphData.size());
+                glyph.bitmap = AlphaImage(size, reinterpret_cast<const uint8_t*>(glyphData.data()),
+                                          glyphData.size());
             }
 
             result.push_back(std::move(glyph));
+        };
+
+        double ascender{ 0.0 }, descender{ 0.0 };
+        uint16_t count{ 0 };
+        auto fontstack_pbf = glyphs_pbf.get_message();
+        while (fontstack_pbf.next()) {
+            switch (fontstack_pbf.tag()) {
+            case 3: {
+                readGlyphMetrics(fontstack_pbf);
+                ++count;
+                break;
+            }
+            case 4: {
+                ascender = fontstack_pbf.get_double();
+                break;
+            }
+            case 5: {
+                descender = fontstack_pbf.get_double();
+                break;
+            }
+            default: {
+                fontstack_pbf.skip();
+                break;
+            }
+            }
+        }
+        if (ascender != 0.0 || descender != 0.0) {
+            assert(count <= result.size());
+            for (uint16_t i = result.size() - count; i <= result.size() - 1; ++i) {
+                result[i].metrics.ascender = ascender;
+                result[i].metrics.descender = descender;
+            }
         }
     }
 
diff --git a/src/mbgl/text/shaping.cpp b/src/mbgl/text/shaping.cpp
index d6d9a3d34e0..d62401c97e7 100644
--- a/src/mbgl/text/shaping.cpp
+++ b/src/mbgl/text/shaping.cpp
@@ -326,8 +326,7 @@ void shapeLines(Shaping& shaping,
                 const GlyphMap& glyphMap,
                 bool allowVerticalPlacement) {
     float x = 0;
-    float y = Shaping::yOffset;
-    
+    float y = 0;
     float maxLineLength = 0;
 
 
@@ -359,21 +358,25 @@ void shapeLines(Shaping& shaping,
             if (it == glyphs->second.end() || !it->second) {
                 continue;
             }
-            
-            // We don't know the baseline, but since we're laying out
-            // at 24 points, we can calculate how much it will move when
-            // we scale up or down.
-            const double baselineOffset = (lineMaxScale - section.scale) * util::ONE_EM;
-            
+
             const Glyph& glyph = **it->second;
 
+            // Each glyph's baseline is starting from its acsender, which is the vertical distance
+            // from the horizontal baseline to the highest ‘character’ coordinate in a font face.
+            // 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 = -glyph.metrics.ascender * section.scale +
+                                          (lineMaxScale - section.scale) * util::ONE_EM;
+
             if (writingMode == WritingModeType::Horizontal ||
                 // Don't verticalize glyphs that have no upright orientation if vertical placement is disabled.
                 (!allowVerticalPlacement && !util::i18n::hasUprightVerticalOrientation(codePoint)) ||
                 // If vertical placement is ebabled, don't verticalize glyphs that
                 // are from complex text layout script, or whitespaces.
                 (allowVerticalPlacement && (util::i18n::isWhitespace(codePoint) || util::i18n::isCharInComplexShapingScript(codePoint)))) {
-                shaping.positionedGlyphs.emplace_back(codePoint, x, y + baselineOffset, false, section.fontStackHash, section.scale, sectionIndex);
+                shaping.positionedGlyphs.emplace_back(codePoint, x, y + baselineOffset, false,
+                                                      section.fontStackHash, section.scale,
+                                                      sectionIndex);
                 x += glyph.metrics.advance * section.scale + spacing;
             } else {
                 shaping.positionedGlyphs.emplace_back(codePoint, x, y + baselineOffset, true, section.fontStackHash, section.scale, sectionIndex);
@@ -399,7 +402,6 @@ void shapeLines(Shaping& shaping,
     align(shaping, justify, anchorAlign.horizontalAlign, anchorAlign.verticalAlign, maxLineLength,
           lineHeight, lines.size());
     const float height = y - Shaping::yOffset;
-
     // Calculate the bounding box
     shaping.top += -anchorAlign.verticalAlign * height;
     shaping.bottom = shaping.top + height;

From f669a7d26bcd90959204a4d990d52aaccacad917 Mon Sep 17 00:00:00 2001
From: zmiao <zmiao.jamie@gmail.com>
Date: Thu, 19 Sep 2019 14:14:54 +0300
Subject: [PATCH 02/12] [core] Fix clang-format error

---
 src/mbgl/text/glyph_pbf.cpp | 115 ++++++++++++++++++------------------
 src/mbgl/text/shaping.cpp   |   9 ++-
 2 files changed, 60 insertions(+), 64 deletions(-)

diff --git a/src/mbgl/text/glyph_pbf.cpp b/src/mbgl/text/glyph_pbf.cpp
index d7b176b0423..b4abfdeb392 100644
--- a/src/mbgl/text/glyph_pbf.cpp
+++ b/src/mbgl/text/glyph_pbf.cpp
@@ -21,36 +21,36 @@ std::vector<Glyph> parseGlyphPBF(const GlyphRange& glyphRange, const std::string
 
             while (glyph_pbf.next()) {
                 switch (glyph_pbf.tag()) {
-                case 1: // id
-                    glyph.id = glyph_pbf.get_uint32();
-                    hasID = true;
-                    break;
-                case 2: // bitmap
-                    glyphData = glyph_pbf.get_view();
-                    break;
-                case 3: // width
-                    glyph.metrics.width = glyph_pbf.get_uint32();
-                    hasWidth = true;
-                    break;
-                case 4: // height
-                    glyph.metrics.height = glyph_pbf.get_uint32();
-                    hasHeight = true;
-                    break;
-                case 5: // left
-                    glyph.metrics.left = glyph_pbf.get_sint32();
-                    hasLeft = true;
-                    break;
-                case 6: // top
-                    glyph.metrics.top = glyph_pbf.get_sint32();
-                    hasTop = true;
-                    break;
-                case 7: // advance
-                    glyph.metrics.advance = glyph_pbf.get_uint32();
-                    hasAdvance = true;
-                    break;
-                default:
-                    glyph_pbf.skip();
-                    break;
+                    case 1: // id
+                        glyph.id = glyph_pbf.get_uint32();
+                        hasID = true;
+                        break;
+                    case 2: // bitmap
+                        glyphData = glyph_pbf.get_view();
+                        break;
+                    case 3: // width
+                        glyph.metrics.width = glyph_pbf.get_uint32();
+                        hasWidth = true;
+                        break;
+                    case 4: // height
+                        glyph.metrics.height = glyph_pbf.get_uint32();
+                        hasHeight = true;
+                        break;
+                    case 5: // left
+                        glyph.metrics.left = glyph_pbf.get_sint32();
+                        hasLeft = true;
+                        break;
+                    case 6: // top
+                        glyph.metrics.top = glyph_pbf.get_sint32();
+                        hasTop = true;
+                        break;
+                    case 7: // advance
+                        glyph.metrics.advance = glyph_pbf.get_uint32();
+                        hasAdvance = true;
+                        break;
+                    default:
+                        glyph_pbf.skip();
+                        break;
                 }
             }
 
@@ -58,54 +58,51 @@ std::vector<Glyph> parseGlyphPBF(const GlyphRange& glyphRange, const std::string
             // needs to satisfy a few metrics conditions that ensure that the glyph isn't bogus.
             // All other glyphs are malformed.  We're also discarding all glyphs that are outside
             // the expected glyph range.
-            if (!hasID || !hasWidth || !hasHeight || !hasLeft || !hasTop || !hasAdvance ||
-                glyph.metrics.width >= 256 || glyph.metrics.height >= 256 ||
-                glyph.metrics.left < -128 || glyph.metrics.left >= 128 ||
-                glyph.metrics.top < -128 || glyph.metrics.top >= 128 ||
-                glyph.metrics.advance >= 256 || glyph.id < glyphRange.first ||
-                glyph.id > glyphRange.second) {
+            if (!hasID || !hasWidth || !hasHeight || !hasLeft || !hasTop || !hasAdvance || glyph.metrics.width >= 256 ||
+                glyph.metrics.height >= 256 || glyph.metrics.left < -128 || glyph.metrics.left >= 128 ||
+                glyph.metrics.top < -128 || glyph.metrics.top >= 128 || glyph.metrics.advance >= 256 ||
+                glyph.id < glyphRange.first || glyph.id > glyphRange.second) {
                 return;
             }
 
             // If the area of width/height is non-zero, we need to adjust the expected size
             // with the implicit border size, otherwise we expect there to be no bitmap at all.
             if (glyph.metrics.width && glyph.metrics.height) {
-                const Size size{ glyph.metrics.width + 2 * Glyph::borderSize,
-                                 glyph.metrics.height + 2 * Glyph::borderSize };
+                const Size size{glyph.metrics.width + 2 * Glyph::borderSize,
+                                glyph.metrics.height + 2 * Glyph::borderSize};
 
                 if (size.area() != glyphData.size()) {
                     return;
                 }
 
-                glyph.bitmap = AlphaImage(size, reinterpret_cast<const uint8_t*>(glyphData.data()),
-                                          glyphData.size());
+                glyph.bitmap = AlphaImage(size, reinterpret_cast<const uint8_t*>(glyphData.data()), glyphData.size());
             }
 
             result.push_back(std::move(glyph));
         };
 
-        double ascender{ 0.0 }, descender{ 0.0 };
-        uint16_t count{ 0 };
+        double ascender{0.0}, descender{0.0};
+        uint16_t count{0};
         auto fontstack_pbf = glyphs_pbf.get_message();
         while (fontstack_pbf.next()) {
             switch (fontstack_pbf.tag()) {
-            case 3: {
-                readGlyphMetrics(fontstack_pbf);
-                ++count;
-                break;
-            }
-            case 4: {
-                ascender = fontstack_pbf.get_double();
-                break;
-            }
-            case 5: {
-                descender = fontstack_pbf.get_double();
-                break;
-            }
-            default: {
-                fontstack_pbf.skip();
-                break;
-            }
+                case 3: {
+                    readGlyphMetrics(fontstack_pbf);
+                    ++count;
+                    break;
+                }
+                case 4: {
+                    ascender = fontstack_pbf.get_double();
+                    break;
+                }
+                case 5: {
+                    descender = fontstack_pbf.get_double();
+                    break;
+                }
+                default: {
+                    fontstack_pbf.skip();
+                    break;
+                }
             }
         }
         if (ascender != 0.0 || descender != 0.0) {
diff --git a/src/mbgl/text/shaping.cpp b/src/mbgl/text/shaping.cpp
index d62401c97e7..81639adefd4 100644
--- a/src/mbgl/text/shaping.cpp
+++ b/src/mbgl/text/shaping.cpp
@@ -365,8 +365,8 @@ void shapeLines(Shaping& shaping,
             // from the horizontal baseline to the highest ‘character’ coordinate in a font face.
             // 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 = -glyph.metrics.ascender * section.scale +
-                                          (lineMaxScale - section.scale) * util::ONE_EM;
+            const double baselineOffset =
+                -glyph.metrics.ascender * section.scale + (lineMaxScale - section.scale) * util::ONE_EM;
 
             if (writingMode == WritingModeType::Horizontal ||
                 // Don't verticalize glyphs that have no upright orientation if vertical placement is disabled.
@@ -374,9 +374,8 @@ void shapeLines(Shaping& shaping,
                 // If vertical placement is ebabled, don't verticalize glyphs that
                 // are from complex text layout script, or whitespaces.
                 (allowVerticalPlacement && (util::i18n::isWhitespace(codePoint) || util::i18n::isCharInComplexShapingScript(codePoint)))) {
-                shaping.positionedGlyphs.emplace_back(codePoint, x, y + baselineOffset, false,
-                                                      section.fontStackHash, section.scale,
-                                                      sectionIndex);
+                shaping.positionedGlyphs.emplace_back(
+                    codePoint, x, y + baselineOffset, false, section.fontStackHash, section.scale, sectionIndex);
                 x += glyph.metrics.advance * section.scale + spacing;
             } else {
                 shaping.positionedGlyphs.emplace_back(codePoint, x, y + baselineOffset, true, section.fontStackHash, section.scale, sectionIndex);

From 54c310f95dec0ed2d972a62a65ef283fce7683a1 Mon Sep 17 00:00:00 2001
From: zmiao <zmiao.jamie@gmail.com>
Date: Fri, 20 Sep 2019 11:17:20 +0300
Subject: [PATCH 03/12] [core] Keep yOffset in case ascender is not available,
 preventing breaking existed render tests

---
 src/mbgl/text/shaping.cpp | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/src/mbgl/text/shaping.cpp b/src/mbgl/text/shaping.cpp
index 81639adefd4..d1f70e51735 100644
--- a/src/mbgl/text/shaping.cpp
+++ b/src/mbgl/text/shaping.cpp
@@ -329,7 +329,6 @@ void shapeLines(Shaping& shaping,
     float y = 0;
     float maxLineLength = 0;
 
-
     const float justify = textJustify == style::TextJustifyType::Right ? 1 :
         textJustify == style::TextJustifyType::Left ? 0 :
         0.5;
@@ -365,15 +364,17 @@ void shapeLines(Shaping& shaping,
             // from the horizontal baseline to the highest ‘character’ coordinate in a font face.
             // 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 =
-                -glyph.metrics.ascender * section.scale + (lineMaxScale - section.scale) * util::ONE_EM;
+            const bool hasBaseline = glyph.metrics.ascender != 0 && glyph.metrics.descender != 0;
+            const double baselineOffset = (hasBaseline ? (-glyph.metrics.ascender * section.scale) : shaping.yOffset) +
+                                          (lineMaxScale - section.scale) * util::ONE_EM;
 
             if (writingMode == WritingModeType::Horizontal ||
                 // Don't verticalize glyphs that have no upright orientation if vertical placement is disabled.
                 (!allowVerticalPlacement && !util::i18n::hasUprightVerticalOrientation(codePoint)) ||
                 // If vertical placement is ebabled, don't verticalize glyphs that
                 // are from complex text layout script, or whitespaces.
-                (allowVerticalPlacement && (util::i18n::isWhitespace(codePoint) || util::i18n::isCharInComplexShapingScript(codePoint)))) {
+                (allowVerticalPlacement &&
+                 (util::i18n::isWhitespace(codePoint) || util::i18n::isCharInComplexShapingScript(codePoint)))) {
                 shaping.positionedGlyphs.emplace_back(
                     codePoint, x, y + baselineOffset, false, section.fontStackHash, section.scale, sectionIndex);
                 x += glyph.metrics.advance * section.scale + spacing;
@@ -400,7 +401,7 @@ void shapeLines(Shaping& shaping,
 
     align(shaping, justify, anchorAlign.horizontalAlign, anchorAlign.verticalAlign, maxLineLength,
           lineHeight, lines.size());
-    const float height = y - Shaping::yOffset;
+    const float height = y;
     // Calculate the bounding box
     shaping.top += -anchorAlign.verticalAlign * height;
     shaping.bottom = shaping.top + height;

From 97c6ed20ee00df22dd737498107d656ec3ef7a67 Mon Sep 17 00:00:00 2001
From: zmiao <zmiao.jamie@gmail.com>
Date: Tue, 24 Sep 2019 18:56:48 +0300
Subject: [PATCH 04/12] [core] Change font's baseline value to the midline of
 the font fase.

---
 src/mbgl/text/glyph.hpp   |  1 +
 src/mbgl/text/quads.cpp   | 15 +++++++------
 src/mbgl/text/shaping.cpp | 44 ++++++++++++++++++++++++++++++++-------
 3 files changed, 46 insertions(+), 14 deletions(-)

diff --git a/src/mbgl/text/glyph.hpp b/src/mbgl/text/glyph.hpp
index 8977f4992af..c56247f2059 100644
--- a/src/mbgl/text/glyph.hpp
+++ b/src/mbgl/text/glyph.hpp
@@ -94,6 +94,7 @@ class Shaping {
     explicit operator bool() const { return !positionedGlyphs.empty(); }
     // The y offset *should* be part of the font metadata.
     static constexpr int32_t yOffset = -17;
+    bool hasBaseline{false};
 };
 
 enum class WritingModeType : uint8_t {
diff --git a/src/mbgl/text/quads.cpp b/src/mbgl/text/quads.cpp
index 281c5d99de8..c703f331bce 100644
--- a/src/mbgl/text/quads.cpp
+++ b/src/mbgl/text/quads.cpp
@@ -80,7 +80,7 @@ SymbolQuads getGlyphQuads(const Shaping& shapedText,
         const GlyphPosition& glyph = positionsIt->second;
         const Rect<uint16_t>& rect = glyph.rect;
 
-        // The rects have an addditional buffer that is not included in their size;
+        // The rects have an additional buffer that is not included in their size;
         const float glyphPadding = 1.0f;
         const float rectBuffer = 3.0f + glyphPadding;
 
@@ -115,8 +115,9 @@ SymbolQuads getGlyphQuads(const Shaping& shapedText,
 
         if (rotateVerticalGlyph) {
             // Vertical-supporting glyphs are laid out in 24x24 point boxes (1 square em)
-            // In horizontal orientation, the y values for glyphs are below the midline
-            // and we use a "yOffset" of -17 to pull them up to the middle.
+            // In horizontal orientation, the y values for glyphs are below the midline.
+            // If the glyph's baseline is applicable, we take the value of the baseline offset.
+            // Otherwise, we use a "yOffset" of -17 to pull them up to the middle.
             // By rotating counter-clockwise around the point at the center of the left
             // edge of a 24x24 layout box centered below the midline, we align the center
             // of the glyphs with the horizontal midline, so the yOffset is no longer
@@ -124,14 +125,16 @@ SymbolQuads getGlyphQuads(const Shaping& shapedText,
             // The y coordinate includes baseline yOffset, therefore, needs to be accounted
             // for when glyph is rotated and translated.
 
-            const Point<float> center{ -halfAdvance, halfAdvance - Shaping::yOffset };
+            float yShift =
+                shapedText.hasBaseline ? (-glyph.metrics.ascender + glyph.metrics.descender) / 2 : Shaping::yOffset;
+            const Point<float> center{-halfAdvance, halfAdvance - yShift};
             const float verticalRotation = -M_PI_2;
 
             // xHalfWidhtOffsetcorrection is a difference between full-width and half-width
             // advance, should be 0 for full-width glyphs and will pull up half-width glyphs.
             const float xHalfWidhtOffsetcorrection = util::ONE_EM / 2 - halfAdvance;
-            const Point<float> xOffsetCorrection{ 5.0f - Shaping::yOffset - xHalfWidhtOffsetcorrection, 0.0f };
-            
+            const Point<float> xOffsetCorrection{5.0f - yShift - xHalfWidhtOffsetcorrection, 0.0f};
+
             tl = util::rotate(tl - center, verticalRotation) + center + xOffsetCorrection + verticalizedLabelOffset;
             tr = util::rotate(tr - center, verticalRotation) + center + xOffsetCorrection + verticalizedLabelOffset;
             bl = util::rotate(bl - center, verticalRotation) + center + xOffsetCorrection + verticalizedLabelOffset;
diff --git a/src/mbgl/text/shaping.cpp b/src/mbgl/text/shaping.cpp
index d1f70e51735..efcf17873e4 100644
--- a/src/mbgl/text/shaping.cpp
+++ b/src/mbgl/text/shaping.cpp
@@ -328,11 +328,34 @@ void shapeLines(Shaping& shaping,
     float x = 0;
     float y = 0;
     float maxLineLength = 0;
+    bool hasBaseline{false};
+
+    for (std::size_t i = 0; i < lines.size(); ++i) {
+        TaggedString& line = lines[i];
+        line.trim();
+        for (std::size_t j = 0; j < line.length(); ++j) {
+            const std::size_t sectionIndex = line.getSectionIndex(j);
+            const SectionOptions& section = line.sectionAt(sectionIndex);
+            char16_t codePoint = line.getCharCodeAt(i);
+            auto glyphs = glyphMap.find(section.fontStackHash);
+            if (glyphs == glyphMap.end()) {
+                continue;
+            }
+            auto it = glyphs->second.find(codePoint);
+            if (it == glyphs->second.end() || !it->second) {
+                continue;
+            }
+            const Glyph& glyph = **it->second;
+            hasBaseline = glyph.metrics.ascender != 0 && glyph.metrics.descender != 0;
+            if (!hasBaseline) break;
+        }
+        if (!hasBaseline) break;
+    }
 
     const float justify = textJustify == style::TextJustifyType::Right ? 1 :
         textJustify == style::TextJustifyType::Left ? 0 :
         0.5;
-    
+
     for (TaggedString& line : lines) {
         // Collapse whitespace so it doesn't throw off justification
         line.trim();
@@ -360,13 +383,17 @@ void shapeLines(Shaping& shaping,
 
             const Glyph& glyph = **it->second;
 
-            // Each glyph's baseline is starting from its acsender, which is the vertical distance
-            // from the horizontal baseline to the highest ‘character’ coordinate in a font face.
-            // Since we're laying out at 24 points, we need also calculate how much it will move
-            // when we scale up or down.
-            const bool hasBaseline = glyph.metrics.ascender != 0 && glyph.metrics.descender != 0;
-            const double baselineOffset = (hasBaseline ? (-glyph.metrics.ascender * section.scale) : shaping.yOffset) +
-                                          (lineMaxScale - section.scale) * util::ONE_EM;
+            // In order to make different fonts aligned, they must share a general baseline that starts from the midline
+            // of each font face.  Baseline offset is the vertical distance from font face's baseline to its top most
+            // position, which is the half size of the sum (ascender + descender). Since glyph's position is counted
+            // from the top left corner, the negative shift is needed. So different fonts shares the same baseline but
+            // 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) +
+                (lineMaxScale - section.scale) * util::ONE_EM;
 
             if (writingMode == WritingModeType::Horizontal ||
                 // Don't verticalize glyphs that have no upright orientation if vertical placement is disabled.
@@ -407,6 +434,7 @@ void shapeLines(Shaping& shaping,
     shaping.bottom = shaping.top + height;
     shaping.left += -anchorAlign.horizontalAlign * maxLineLength;
     shaping.right = shaping.left + maxLineLength;
+    shaping.hasBaseline = hasBaseline;
 }
 
 const Shaping getShaping(const TaggedString& formattedString,

From 358c79b31dabac987376406ca29dc42cda160d5e Mon Sep 17 00:00:00 2001
From: zmiao <miao.zhao@mapbox.com>
Date: Thu, 26 Sep 2019 22:28:04 +0300
Subject: [PATCH 05/12] [core] Add hasBaseline flag for fontstack, change
 ascender/descendr type to int32

---
 src/mbgl/text/glyph.hpp                | 22 +++----
 src/mbgl/text/glyph_atlas.cpp          |  2 +-
 src/mbgl/text/glyph_manager.cpp        |  7 ++-
 src/mbgl/text/glyph_manager.hpp        |  1 +
 src/mbgl/text/glyph_pbf.cpp            | 33 ++++++-----
 src/mbgl/text/glyph_pbf.hpp            |  3 +-
 src/mbgl/text/shaping.cpp              | 49 +++++++---------
 src/mbgl/tile/geometry_tile_worker.cpp |  9 ++-
 test/text/glyph_manager.test.cpp       | 20 ++++---
 test/text/glyph_pbf.test.cpp           |  8 ++-
 test/text/shaping.test.cpp             | 81 ++++++++++++++++++++++++--
 11 files changed, 154 insertions(+), 81 deletions(-)

diff --git a/src/mbgl/text/glyph.hpp b/src/mbgl/text/glyph.hpp
index c56247f2059..73e9ec97152 100644
--- a/src/mbgl/text/glyph.hpp
+++ b/src/mbgl/text/glyph.hpp
@@ -25,21 +25,18 @@ using GlyphIDs = std::set<GlyphID>;
 GlyphRange getGlyphRange(GlyphID glyph);
 
 struct GlyphMetrics {
-    uint32_t width = 0;
-    uint32_t height = 0;
+    uint32_t width = 0U;
+    uint32_t height = 0U;
     int32_t left = 0;
     int32_t top = 0;
-    uint32_t advance = 0;
-    double ascender = 0.0;
-    double descender = 0.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;
+    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;
 }
 
 class Glyph {
@@ -57,7 +54,10 @@ class Glyph {
     GlyphMetrics metrics;
 };
 
-using Glyphs = std::map<GlyphID, optional<Immutable<Glyph>>>;
+struct Glyphs {
+    std::map<GlyphID, optional<Immutable<Glyph>>> glyphs;
+    bool hasBaseline;
+};
 using GlyphMap = std::map<FontStackHash, Glyphs>;
 
 class PositionedGlyph {
diff --git a/src/mbgl/text/glyph_atlas.cpp b/src/mbgl/text/glyph_atlas.cpp
index da65aea8a9d..601582cc49e 100644
--- a/src/mbgl/text/glyph_atlas.cpp
+++ b/src/mbgl/text/glyph_atlas.cpp
@@ -17,7 +17,7 @@ GlyphAtlas makeGlyphAtlas(const GlyphMap& glyphs) {
         FontStackHash fontStack = glyphMapEntry.first;
         GlyphPositionMap& positions = result.positions[fontStack];
 
-        for (const auto& entry : glyphMapEntry.second) {
+        for (const auto& entry : glyphMapEntry.second.glyphs) {
             if (entry.second && (*entry.second)->bitmap.valid()) {
                 const Glyph& glyph = **entry.second;
 
diff --git a/src/mbgl/text/glyph_manager.cpp b/src/mbgl/text/glyph_manager.cpp
index 35ea1031d53..8b37b652abe 100644
--- a/src/mbgl/text/glyph_manager.cpp
+++ b/src/mbgl/text/glyph_manager.cpp
@@ -91,7 +91,7 @@ void GlyphManager::processResponse(const Response& res, const FontStack& fontSta
         std::vector<Glyph> glyphs;
 
         try {
-            glyphs = parseGlyphPBF(range, *res.data);
+            std::tie(glyphs, entry.hasBaseline) = parseGlyphPBF(range, *res.data);
         } catch (...) {
             observer->onGlyphsError(fontStack, range, std::current_exception());
             return;
@@ -134,13 +134,14 @@ void GlyphManager::notify(GlyphRequestor& requestor, const GlyphDependencies& gl
 
         Glyphs& glyphs = response[FontStackHasher()(fontStack)];
         Entry& entry = entries[fontStack];
+        glyphs.hasBaseline = entry.hasBaseline;
 
         for (const auto& glyphID : glyphIDs) {
             auto it = entry.glyphs.find(glyphID);
             if (it != entry.glyphs.end()) {
-                glyphs.emplace(*it);
+                glyphs.glyphs.emplace(*it);
             } else {
-                glyphs.emplace(glyphID, std::experimental::nullopt);
+                glyphs.glyphs.emplace(glyphID, std::experimental::nullopt);
             }
         }
     }
diff --git a/src/mbgl/text/glyph_manager.hpp b/src/mbgl/text/glyph_manager.hpp
index 8603a320d28..1955fb752c2 100644
--- a/src/mbgl/text/glyph_manager.hpp
+++ b/src/mbgl/text/glyph_manager.hpp
@@ -61,6 +61,7 @@ class GlyphManager {
     struct Entry {
         std::map<GlyphRange, GlyphRequest> ranges;
         std::map<GlyphID, Immutable<Glyph>> glyphs;
+        bool hasBaseline;
     };
 
     std::unordered_map<FontStack, Entry, FontStackHasher> entries;
diff --git a/src/mbgl/text/glyph_pbf.cpp b/src/mbgl/text/glyph_pbf.cpp
index b4abfdeb392..4658bc69da9 100644
--- a/src/mbgl/text/glyph_pbf.cpp
+++ b/src/mbgl/text/glyph_pbf.cpp
@@ -4,14 +4,14 @@
 
 namespace mbgl {
 
-std::vector<Glyph> parseGlyphPBF(const GlyphRange& glyphRange, const std::string& data) {
-    std::vector<Glyph> result;
-    result.reserve(256);
+std::pair<std::vector<Glyph>, bool> parseGlyphPBF(const GlyphRange& glyphRange, const std::string& data) {
+    std::vector<Glyph> glyphs;
+    glyphs.reserve(256);
 
     protozero::pbf_reader glyphs_pbf(data);
-
+    bool hasBaseline{true};
     while (glyphs_pbf.next(1)) {
-        auto readGlyphMetrics = [glyphRange, &result](protozero::pbf_reader& fontstack_pbf) {
+        auto readGlyphMetrics = [glyphRange, &glyphs](protozero::pbf_reader& fontstack_pbf) {
             auto glyph_pbf = fontstack_pbf.get_message();
             Glyph glyph;
             protozero::data_view glyphData;
@@ -78,10 +78,10 @@ std::vector<Glyph> parseGlyphPBF(const GlyphRange& glyphRange, const std::string
                 glyph.bitmap = AlphaImage(size, reinterpret_cast<const uint8_t*>(glyphData.data()), glyphData.size());
             }
 
-            result.push_back(std::move(glyph));
+            glyphs.push_back(std::move(glyph));
         };
 
-        double ascender{0.0}, descender{0.0};
+        int32_t ascender{0}, descender{0};
         uint16_t count{0};
         auto fontstack_pbf = glyphs_pbf.get_message();
         while (fontstack_pbf.next()) {
@@ -92,11 +92,11 @@ std::vector<Glyph> parseGlyphPBF(const GlyphRange& glyphRange, const std::string
                     break;
                 }
                 case 4: {
-                    ascender = fontstack_pbf.get_double();
+                    ascender = fontstack_pbf.get_sint32();
                     break;
                 }
                 case 5: {
-                    descender = fontstack_pbf.get_double();
+                    descender = fontstack_pbf.get_sint32();
                     break;
                 }
                 default: {
@@ -105,16 +105,19 @@ std::vector<Glyph> parseGlyphPBF(const GlyphRange& glyphRange, const std::string
                 }
             }
         }
-        if (ascender != 0.0 || descender != 0.0) {
-            assert(count <= result.size());
-            for (uint16_t i = result.size() - count; i <= result.size() - 1; ++i) {
-                result[i].metrics.ascender = ascender;
-                result[i].metrics.descender = descender;
+        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 result;
+    return std::make_pair(std::move(glyphs), hasBaseline);
 }
 
 } // namespace mbgl
diff --git a/src/mbgl/text/glyph_pbf.hpp b/src/mbgl/text/glyph_pbf.hpp
index 28a28b41143..6adbb9137ff 100644
--- a/src/mbgl/text/glyph_pbf.hpp
+++ b/src/mbgl/text/glyph_pbf.hpp
@@ -4,10 +4,11 @@
 #include <mbgl/text/glyph_range.hpp>
 
 #include <string>
+#include <utility>
 #include <vector>
 
 namespace mbgl {
 
-std::vector<Glyph> parseGlyphPBF(const GlyphRange&, const std::string& data);
+std::pair<std::vector<Glyph>, bool> parseGlyphPBF(const GlyphRange&, const std::string& data);
 
 } // namespace mbgl
diff --git a/src/mbgl/text/shaping.cpp b/src/mbgl/text/shaping.cpp
index efcf17873e4..ff0f7dee15b 100644
--- a/src/mbgl/text/shaping.cpp
+++ b/src/mbgl/text/shaping.cpp
@@ -144,8 +144,8 @@ void justifyLine(std::vector<PositionedGlyph>& positionedGlyphs,
     if (glyphs == glyphMap.end()) {
         return;
     }
-    auto it = glyphs->second.find(glyph.glyph);
-    if (it != glyphs->second.end() && it->second) {
+    auto it = glyphs->second.glyphs.find(glyph.glyph);
+    if (it != glyphs->second.glyphs.end() && it->second) {
         const float lastAdvance = (*it->second)->metrics.advance * glyph.scale;
         const float lineIndent = float(glyph.x + lastAdvance) * justify;
         
@@ -168,11 +168,11 @@ float determineAverageLineWidth(const TaggedString& logicalInput,
         if (glyphs == glyphMap.end()) {
             continue;
         }
-        auto it = glyphs->second.find(codePoint);
-        if (it == glyphs->second.end() || !it->second) {
+        auto it = glyphs->second.glyphs.find(codePoint);
+        if (it == glyphs->second.glyphs.end() || !it->second) {
             continue;
         }
-        
+
         totalWidth += (*it->second)->metrics.advance * section.scale + spacing;
     }
     
@@ -294,11 +294,11 @@ std::set<std::size_t> determineLineBreaks(const TaggedString& logicalInput,
         if (glyphs == glyphMap.end()) {
             continue;
         }
-        auto it = glyphs->second.find(codePoint);
-        if (it != glyphs->second.end() && it->second && !util::i18n::isWhitespace(codePoint)) {
+        auto it = glyphs->second.glyphs.find(codePoint);
+        if (it != glyphs->second.glyphs.end() && it->second && !util::i18n::isWhitespace(codePoint)) {
             currentX += (*it->second)->metrics.advance * section.scale + spacing;
         }
-        
+
         // Ideographic characters, spaces, and word-breaking punctuation that often appear without
         // surrounding spaces.
         if (i < logicalInput.length() - 1) {
@@ -325,28 +325,19 @@ void shapeLines(Shaping& shaping,
                 const WritingModeType writingMode,
                 const GlyphMap& glyphMap,
                 bool allowVerticalPlacement) {
-    float x = 0;
-    float y = 0;
-    float maxLineLength = 0;
+    float x = 0.0f;
+    float y = 0.0f;
+    float maxLineLength = 0.0f;
     bool hasBaseline{false};
 
-    for (std::size_t i = 0; i < lines.size(); ++i) {
-        TaggedString& line = lines[i];
-        line.trim();
-        for (std::size_t j = 0; j < line.length(); ++j) {
-            const std::size_t sectionIndex = line.getSectionIndex(j);
-            const SectionOptions& section = line.sectionAt(sectionIndex);
-            char16_t codePoint = line.getCharCodeAt(i);
+    for (const auto& line : lines) {
+        const auto& sections = line.getSections();
+        for (const auto& section : sections) {
             auto glyphs = glyphMap.find(section.fontStackHash);
             if (glyphs == glyphMap.end()) {
                 continue;
             }
-            auto it = glyphs->second.find(codePoint);
-            if (it == glyphs->second.end() || !it->second) {
-                continue;
-            }
-            const Glyph& glyph = **it->second;
-            hasBaseline = glyph.metrics.ascender != 0 && glyph.metrics.descender != 0;
+            hasBaseline = glyphs->second.hasBaseline;
             if (!hasBaseline) break;
         }
         if (!hasBaseline) break;
@@ -366,7 +357,7 @@ void shapeLines(Shaping& shaping,
             y += lineHeight; // Still need a line feed after empty line
             continue;
         }
-        
+
         std::size_t lineStartIndex = shaping.positionedGlyphs.size();
         for (std::size_t i = 0; i < line.length(); i++) {
             const std::size_t sectionIndex = line.getSectionIndex(i);
@@ -376,8 +367,8 @@ void shapeLines(Shaping& shaping,
             if (glyphs == glyphMap.end()) {
                 continue;
             }
-            auto it = glyphs->second.find(codePoint);
-            if (it == glyphs->second.end() || !it->second) {
+            auto it = glyphs->second.glyphs.find(codePoint);
+            if (it == glyphs->second.glyphs.end() || !it->second) {
                 continue;
             }
 
@@ -386,7 +377,7 @@ void shapeLines(Shaping& shaping,
             // In order to make different fonts aligned, they must share a general baseline that starts from the midline
             // of each font face.  Baseline offset is the vertical distance from font face's baseline to its top most
             // position, which is the half size of the sum (ascender + descender). Since glyph's position is counted
-            // from the top left corner, the negative shift is needed. So different fonts shares the same baseline but
+            // from the top left corner, the negative shift is needed. So different fonts share the same baseline but
             // 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.
@@ -410,7 +401,7 @@ void shapeLines(Shaping& shaping,
                 x += util::ONE_EM * section.scale + spacing;
             }
         }
-        
+
         // Only justify if we placed at least one glyph
         if (shaping.positionedGlyphs.size() != lineStartIndex) {
             float lineLength = x - spacing; // Don't count trailing spacing
diff --git a/src/mbgl/tile/geometry_tile_worker.cpp b/src/mbgl/tile/geometry_tile_worker.cpp
index 7815e62b75e..5ac168de9df 100644
--- a/src/mbgl/tile/geometry_tile_worker.cpp
+++ b/src/mbgl/tile/geometry_tile_worker.cpp
@@ -259,18 +259,20 @@ void GeometryTileWorker::onGlyphsAvailable(GlyphMap newGlyphMap) {
         Glyphs& newGlyphs = newFontGlyphs.second;
 
         Glyphs& glyphs = glyphMap[fontStack];
+        glyphs.hasBaseline =
+            glyphs.glyphs.empty() ? newGlyphs.hasBaseline : glyphs.hasBaseline && newGlyphs.hasBaseline;
         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
             // There should not be many fontstacks to look through
             if (FontStackHasher()(pendingGlyphDependency.first) == fontStack) {
                 GlyphIDs& pendingGlyphIDs = pendingGlyphDependency.second;
-                for (auto& newGlyph : newGlyphs) {
+                for (auto& newGlyph : newGlyphs.glyphs) {
                     const GlyphID& glyphID = newGlyph.first;
                     optional<Immutable<Glyph>>& glyph = newGlyph.second;
 
                     if (pendingGlyphIDs.erase(glyphID)) {
-                        glyphs.emplace(glyphID, std::move(glyph));
+                        glyphs.glyphs.emplace(glyphID, std::move(glyph));
                     }
                 }
             }
@@ -294,7 +296,8 @@ void GeometryTileWorker::requestNewGlyphs(const GlyphDependencies& glyphDependen
     for (auto& fontDependencies : glyphDependencies) {
         auto fontGlyphs = glyphMap.find(FontStackHasher()(fontDependencies.first));
         for (auto glyphID : fontDependencies.second) {
-            if (fontGlyphs == glyphMap.end() || fontGlyphs->second.find(glyphID) == fontGlyphs->second.end()) {
+            if (fontGlyphs == glyphMap.end() ||
+                fontGlyphs->second.glyphs.find(glyphID) == fontGlyphs->second.glyphs.end()) {
                 pendingGlyphDependencies[fontDependencies.first].insert(glyphID);
             }
         }
diff --git a/test/text/glyph_manager.test.cpp b/test/text/glyph_manager.test.cpp
index 1e9aef38ede..cdb98702cfe 100644
--- a/test/text/glyph_manager.test.cpp
+++ b/test/text/glyph_manager.test.cpp
@@ -32,6 +32,8 @@ class StubLocalGlyphRasterizer : public LocalGlyphRasterizer {
         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);
 
@@ -106,8 +108,8 @@ TEST(GlyphManager, LoadingSuccess) {
         ASSERT_EQ(range, GlyphRange(0, 255));
     };
 
-    test.requestor.glyphsAvailable = [&] (GlyphMap glyphs) {
-        const auto& testPositions = glyphs.at(FontStackHasher()({{"Test Stack"}}));
+    test.requestor.glyphsAvailable = [&](GlyphMap glyphs) {
+        const auto& testPositions = glyphs.at(FontStackHasher()({{"Test Stack"}})).glyphs;
 
         ASSERT_EQ(testPositions.size(), 3u);
         ASSERT_EQ(testPositions.count(u'a'), 1u);
@@ -223,8 +225,8 @@ TEST(GlyphManager, LoadLocalCJKGlyph) {
     
     test.requestor.glyphsAvailable = [&] (GlyphMap glyphs) {
         EXPECT_EQ(glyphResponses, 0); // Local generation should prevent requesting any glyphs
-        
-        const auto& testPositions = glyphs.at(FontStackHasher()({{"Test Stack"}}));
+
+        const auto& testPositions = glyphs.at(FontStackHasher()({{"Test Stack"}})).glyphs;
 
         ASSERT_EQ(testPositions.size(), 1u);
         ASSERT_EQ(testPositions.count(u'中'), 1u);
@@ -264,9 +266,9 @@ TEST(GlyphManager, LoadLocalCJKGlyphAfterLoadingRangeFromURL) {
         return response;
 
     };
-    
-    test.requestor.glyphsAvailable = [&] (GlyphMap glyphs) {
-        const auto& testPositions = glyphs.at(FontStackHasher()({{"Test Stack"}}));
+
+    test.requestor.glyphsAvailable = [&](GlyphMap glyphs) {
+        const auto& testPositions = glyphs.at(FontStackHasher()({{"Test Stack"}})).glyphs;
 
         if (firstGlyphResponse == true) {
             firstGlyphResponse = false;
@@ -325,8 +327,8 @@ TEST(GlyphManager, LoadingInvalid) {
         ASSERT_EQ(range, GlyphRange(0, 255));
     };
 
-    test.requestor.glyphsAvailable = [&] (GlyphMap glyphs) {
-        const auto& testPositions = glyphs.at(FontStackHasher()({{"Test Stack"}}));
+    test.requestor.glyphsAvailable = [&](GlyphMap glyphs) {
+        const auto& testPositions = glyphs.at(FontStackHasher()({{"Test Stack"}})).glyphs;
 
         ASSERT_EQ(testPositions.size(), 2u);
         ASSERT_FALSE(bool(testPositions.at(u'A')));
diff --git a/test/text/glyph_pbf.test.cpp b/test/text/glyph_pbf.test.cpp
index c222ec1dd9a..1f87403982f 100644
--- a/test/text/glyph_pbf.test.cpp
+++ b/test/text/glyph_pbf.test.cpp
@@ -8,9 +8,9 @@ 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"));
-    EXPECT_TRUE(sdfs.size() == 1);
-
-    auto& sdf = sdfs[0];
+    ASSERT_EQ(1, sdfs.first.size());
+    EXPECT_FALSE(sdfs.second);
+    const auto& sdf = sdfs.first[0];
     EXPECT_EQ(69u, sdf.id);
     AlphaImage expected({7, 7});
     expected.fill('x');
@@ -20,4 +20,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);
 }
diff --git a/test/text/shaping.test.cpp b/test/text/shaping.test.cpp
index b22cd7da369..e034ce1503e 100644
--- a/test/text/shaping.test.cpp
+++ b/test/text/shaping.test.cpp
@@ -17,22 +17,25 @@ 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)));
     const std::vector<std::string> fontStack{{"font-stack"}};
     const SectionOptions sectionOptions(1.0f, fontStack);
-    GlyphMap glyphs = {
-        { FontStackHasher()(fontStack), {{u'中', std::move(immutableGlyph)}} }
-    };
+    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) {
+    const auto testGetShaping = [&](const TaggedString& string, unsigned maxWidthInChars) {
         return getShaping(string,
                           maxWidthInChars * ONE_EM,
-                          ONE_EM,       // lineHeight
+                          ONE_EM, // lineHeight
                           style::SymbolAnchorType::Center,
                           style::TextJustifyType::Center,
-                          0,            // spacing
+                          0,              // spacing
                           {{0.0f, 0.0f}}, // translate
                           WritingModeType::Horizontal,
                           bidi,
@@ -80,6 +83,11 @@ TEST(Shaping, ZWSP) {
         ASSERT_EQ(shaping.left, -21);
         ASSERT_EQ(shaping.right, 21);
         ASSERT_EQ(shaping.writingMode, WritingModeType::Horizontal);
+        ASSERT_EQ(2, shaping.positionedGlyphs.size());
+        EXPECT_FLOAT_EQ(-21, shaping.positionedGlyphs[0].x);
+        EXPECT_FLOAT_EQ(-17, shaping.positionedGlyphs[0].y);
+        EXPECT_FLOAT_EQ(0, shaping.positionedGlyphs[1].x);
+        EXPECT_FLOAT_EQ(-17, shaping.positionedGlyphs[1].y);
     }
 
     // 5 'new' lines.
@@ -94,3 +102,64 @@ TEST(Shaping, ZWSP) {
         ASSERT_EQ(shaping.writingMode, WritingModeType::Horizontal);
     }
 }
+
+TEST(Shaping, FontWithBaseline) {
+    Glyph glyph1;
+    glyph1.id = u'阳';
+    glyph1.metrics.width = 18;
+    glyph1.metrics.height = 19;
+    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'光';
+    glyph2.metrics.width = 18;
+    glyph2.metrics.height = 18;
+    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)}};
+
+    const auto testGetShaping = [&](const TaggedString& string, unsigned maxWidthInChars) {
+        return getShaping(string,
+                          maxWidthInChars * ONE_EM,
+                          ONE_EM, // lineHeight
+                          style::SymbolAnchorType::Center,
+                          style::TextJustifyType::Center,
+                          0,              // spacing
+                          {{0.0f, 0.0f}}, // translate
+                          WritingModeType::Horizontal,
+                          bidi,
+                          glyphs,
+                          /*allowVerticalPlacement*/ false);
+    };
+
+    {
+        TaggedString string(u"阳光\u200b", sectionOptions);
+        auto shaping = testGetShaping(string, 5);
+        ASSERT_EQ(shaping.lineCount, 1);
+        ASSERT_EQ(shaping.top, -12);
+        ASSERT_EQ(shaping.bottom, 12);
+        ASSERT_EQ(shaping.left, -21);
+        ASSERT_EQ(shaping.right, 21);
+        ASSERT_EQ(shaping.writingMode, WritingModeType::Horizontal);
+        ASSERT_EQ(2, shaping.positionedGlyphs.size());
+        EXPECT_FLOAT_EQ(-21, shaping.positionedGlyphs[0].x);
+        EXPECT_FLOAT_EQ(-16, shaping.positionedGlyphs[0].y);
+        EXPECT_FLOAT_EQ(0, shaping.positionedGlyphs[1].x);
+        EXPECT_FLOAT_EQ(-15, shaping.positionedGlyphs[1].y);
+    }
+}

From 02314284769a8bd082d2bd61df68fcfe740cd847 Mon Sep 17 00:00:00 2001
From: zmiao <miao.zhao@mapbox.com>
Date: Fri, 27 Sep 2019 17:04:13 +0300
Subject: [PATCH 06/12] [core] Move de/ascender to be font-level member instead
 of glyph level

---
 src/mbgl/text/glyph.hpp                |  9 +++---
 src/mbgl/text/glyph_atlas.cpp          | 21 ++++++-------
 src/mbgl/text/glyph_atlas.hpp          |  9 ++++--
 src/mbgl/text/glyph_manager.cpp        | 13 +++++---
 src/mbgl/text/glyph_manager.hpp        |  3 +-
 src/mbgl/text/glyph_pbf.cpp            | 39 ++++++++++++++----------
 src/mbgl/text/glyph_pbf.hpp            |  4 +--
 src/mbgl/text/quads.cpp                | 15 +++++----
 src/mbgl/text/shaping.cpp              | 18 +++++++----
 src/mbgl/text/tagged_string.hpp        |  8 ++---
 src/mbgl/tile/geometry_tile_worker.cpp |  5 +--
 test/text/glyph_manager.test.cpp       |  2 --
 test/text/glyph_pbf.test.cpp           | 14 +++++----
 test/text/shaping.test.cpp             | 42 +++++++++++++++++---------
 14 files changed, 119 insertions(+), 83 deletions(-)

diff --git a/src/mbgl/text/glyph.hpp b/src/mbgl/text/glyph.hpp
index 73e9ec97152..4ba93985c78 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 @@ class Glyph {
 };
 
 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 601582cc49e..61dfb775a35 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 9dd063ef698..fa9140c3c2f 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 8b37b652abe..2a1132e758a 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 1955fb752c2..b4c8570926e 100644
--- a/src/mbgl/text/glyph_manager.hpp
+++ b/src/mbgl/text/glyph_manager.hpp
@@ -61,7 +61,8 @@ class GlyphManager {
     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 4658bc69da9..eec9bd2f493 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 6adbb9137ff..5e938a63016 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 c703f331bce..c6bc3c2bdc2 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 ff0f7dee15b..a0c0c9d9767 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 698e539a455..966ef0bc610 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 5ac168de9df..5bff71bdc6b 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 cdb98702cfe..90551a5f1fd 100644
--- a/test/text/glyph_manager.test.cpp
+++ b/test/text/glyph_manager.test.cpp
@@ -32,8 +32,6 @@ class StubLocalGlyphRasterizer : public LocalGlyphRasterizer {
         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 1f87403982f..462f073b22c 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 e034ce1503e..142fcfbffba 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);

From c7eae66721a91f2a9a681e443741d7ff81c07f58 Mon Sep 17 00:00:00 2001
From: zmiao <miao.zhao@mapbox.com>
Date: Tue, 1 Oct 2019 10:45:45 +0300
Subject: [PATCH 07/12] [core] Fix clang tidy error

---
 src/mbgl/text/glyph_manager.cpp | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/mbgl/text/glyph_manager.cpp b/src/mbgl/text/glyph_manager.cpp
index 2a1132e758a..773e2abe9d5 100644
--- a/src/mbgl/text/glyph_manager.cpp
+++ b/src/mbgl/text/glyph_manager.cpp
@@ -105,8 +105,8 @@ void GlyphManager::processResponse(const Response& res, const FontStack& fontSta
             }
         }
         if (ascender != 0 || descender != 0) {
-            entry.ascender = std::move(ascender);
-            entry.descender = std::move(descender);
+            entry.ascender = ascender;
+            entry.descender = descender;
         }
     }
 

From a8842884e27ff0567b5264ac3f96a5452d6bf69d Mon Sep 17 00:00:00 2001
From: zmiao <miao.zhao@mapbox.com>
Date: Tue, 1 Oct 2019 11:58:12 +0300
Subject: [PATCH 08/12] [core] minor fix

---
 src/mbgl/text/glyph_pbf.cpp |  2 +-
 src/mbgl/text/quads.cpp     | 11 +++++++----
 src/mbgl/text/shaping.cpp   |  3 +++
 3 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/src/mbgl/text/glyph_pbf.cpp b/src/mbgl/text/glyph_pbf.cpp
index eec9bd2f493..8ed48d08c13 100644
--- a/src/mbgl/text/glyph_pbf.cpp
+++ b/src/mbgl/text/glyph_pbf.cpp
@@ -8,7 +8,7 @@ std::tuple<std::vector<Glyph>, int32_t, int32_t> parseGlyphPBF(const GlyphRange&
     std::vector<Glyph> glyphs;
     glyphs.reserve(256);
     int32_t ascender{0}, descender{0};
-    bool ascenderSet{false}, descenderSet{0};
+    bool ascenderSet{false}, descenderSet{false};
 
     protozero::pbf_reader glyphs_pbf(data);
 
diff --git a/src/mbgl/text/quads.cpp b/src/mbgl/text/quads.cpp
index c6bc3c2bdc2..3e3beb29a31 100644
--- a/src/mbgl/text/quads.cpp
+++ b/src/mbgl/text/quads.cpp
@@ -125,11 +125,14 @@ 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 = 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;
+            if (shapedText.hasBaseline) {
+                assert(fontPositions->second.ascender && fontPositions->second.descender);
             }
+            const float yShift =
+                (shapedText.hasBaseline
+                     ? ((-(fontPositions->second.ascender.value()) + fontPositions->second.descender.value()) / 2.0 *
+                        positionedGlyph.scale)
+                     : Shaping::yOffset);
             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 a0c0c9d9767..d4a30a3446d 100644
--- a/src/mbgl/text/shaping.cpp
+++ b/src/mbgl/text/shaping.cpp
@@ -381,6 +381,9 @@ 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.
+            if (hasBaseline) {
+                assert(glyphs->second.ascender && glyphs->second.descender);
+            }
             const float baselineOffset =
                 (hasBaseline
                      ? ((-(glyphs->second.ascender.value()) + glyphs->second.descender.value()) / 2.0 * section.scale)

From 2bea4c50094dc236a34b082773a8f9424db0fc35 Mon Sep 17 00:00:00 2001
From: zmiao <miao.zhao@mapbox.com>
Date: Tue, 1 Oct 2019 18:37:02 +0300
Subject: [PATCH 09/12] [core] Make text's baseline locating on the general
 virtual baseline.

---
 src/mbgl/text/quads.cpp   | 11 ++------
 src/mbgl/text/shaping.cpp | 56 +++++++++++++++++++++++++--------------
 2 files changed, 38 insertions(+), 29 deletions(-)

diff --git a/src/mbgl/text/quads.cpp b/src/mbgl/text/quads.cpp
index 3e3beb29a31..49b43c413ab 100644
--- a/src/mbgl/text/quads.cpp
+++ b/src/mbgl/text/quads.cpp
@@ -117,7 +117,7 @@ SymbolQuads getGlyphQuads(const Shaping& shapedText,
         if (rotateVerticalGlyph) {
             // Vertical-supporting glyphs are laid out in 24x24 point boxes (1 square em)
             // In horizontal orientation, the y values for glyphs are below the midline.
-            // If the glyph's baseline is applicable, we take the value of the baseline offset.
+            // If the glyph's baseline is applicable, we take the y value of the glyph.
             // Otherwise, we use a "yOffset" of -17 to pull them up to the middle.
             // By rotating counter-clockwise around the point at the center of the left
             // edge of a 24x24 layout box centered below the midline, we align the center
@@ -125,14 +125,7 @@ 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.
-            if (shapedText.hasBaseline) {
-                assert(fontPositions->second.ascender && fontPositions->second.descender);
-            }
-            const float yShift =
-                (shapedText.hasBaseline
-                     ? ((-(fontPositions->second.ascender.value()) + fontPositions->second.descender.value()) / 2.0 *
-                        positionedGlyph.scale)
-                     : Shaping::yOffset);
+            const float yShift = (shapedText.hasBaseline ? positionedGlyph.y : Shaping::yOffset);
             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 d4a30a3446d..337dd613dd0 100644
--- a/src/mbgl/text/shaping.cpp
+++ b/src/mbgl/text/shaping.cpp
@@ -134,11 +134,12 @@ void justifyLine(std::vector<PositionedGlyph>& positionedGlyphs,
                  const GlyphMap& glyphMap,
                  std::size_t start,
                  std::size_t end,
-                 float justify) {
-    if (!justify) {
+                 float justify,
+                 float baselineOffset) {
+    if (!justify && !baselineOffset) {
         return;
     }
-    
+
     PositionedGlyph& glyph = positionedGlyphs[end];
     auto glyphs = glyphMap.find(glyph.font);
     if (glyphs == glyphMap.end()) {
@@ -151,6 +152,7 @@ void justifyLine(std::vector<PositionedGlyph>& positionedGlyphs,
         
         for (std::size_t j = start; j <= end; j++) {
             positionedGlyphs[j].x -= lineIndent;
+            positionedGlyphs[j].y += baselineOffset;
         }
     }
 }
@@ -358,6 +360,7 @@ void shapeLines(Shaping& shaping,
             continue;
         }
 
+        float biggestHeight{0}, baselineOffset{0};
         std::size_t lineStartIndex = shaping.positionedGlyphs.size();
         for (std::size_t i = 0; i < line.length(); i++) {
             const std::size_t sectionIndex = line.getSectionIndex(i);
@@ -374,21 +377,29 @@ void shapeLines(Shaping& shaping,
 
             const Glyph& glyph = **it->second;
 
-            // In order to make different fonts aligned, they must share a general baseline that starts from the midline
-            // of each font face.  Baseline offset is the vertical distance from font face's baseline to its top most
-            // position, which is the half size of the sum (ascender + descender). Since glyph's position is counted
-            // from the top left corner, the negative shift is needed. So different fonts share the same baseline but
-            // 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.
+            double ascender{0}, descender{0}, glyphOffset{0};
+            // In order to make different fonts aligned, they must share a general baseline that aligns with every
+            // font's real baseline. Glyph's position is counted from the top left corner, where is the ascender line
+            // starts. Since ascender is above the baseline, the glyphOffset is the negative shift. In order to make all
+            // the glyphs aligned with shaping box, for each line, we lock the heighest glyph (with scale) locating
+            // at the middle of the line, which will lead to a baseline shift. Then adjust the whole line with the
+            // baseline offset we calculated from the shift.
             if (hasBaseline) {
                 assert(glyphs->second.ascender && glyphs->second.descender);
+                ascender = std::abs(glyphs->second.ascender.value());
+                descender = std::abs(glyphs->second.descender.value());
+                auto value = (ascender + descender) * section.scale;
+                if (biggestHeight < value) {
+                    biggestHeight = value;
+                    baselineOffset = (ascender - descender) / 2 * section.scale;
+                }
+                glyphOffset = -ascender * section.scale;
+            } else {
+                // 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.
+                glyphOffset = Shaping::yOffset + (lineMaxScale - section.scale) * util::ONE_EM;
             }
-            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 ||
                 // Don't verticalize glyphs that have no upright orientation if vertical placement is disabled.
@@ -398,10 +409,11 @@ void shapeLines(Shaping& shaping,
                 (allowVerticalPlacement &&
                  (util::i18n::isWhitespace(codePoint) || util::i18n::isCharInComplexShapingScript(codePoint)))) {
                 shaping.positionedGlyphs.emplace_back(
-                    codePoint, x, y + baselineOffset, false, section.fontStackHash, section.scale, sectionIndex);
+                    codePoint, x, y + glyphOffset, false, section.fontStackHash, section.scale, sectionIndex);
                 x += glyph.metrics.advance * section.scale + spacing;
             } else {
-                shaping.positionedGlyphs.emplace_back(codePoint, x, y + baselineOffset, true, section.fontStackHash, section.scale, sectionIndex);
+                shaping.positionedGlyphs.emplace_back(
+                    codePoint, x, y + glyphOffset, true, section.fontStackHash, section.scale, sectionIndex);
                 x += util::ONE_EM * section.scale + spacing;
             }
         }
@@ -410,9 +422,13 @@ void shapeLines(Shaping& shaping,
         if (shaping.positionedGlyphs.size() != lineStartIndex) {
             float lineLength = x - spacing; // Don't count trailing spacing
             maxLineLength = util::max(lineLength, maxLineLength);
-            
-            justifyLine(shaping.positionedGlyphs, glyphMap, lineStartIndex,
-                        shaping.positionedGlyphs.size() - 1, justify);
+
+            justifyLine(shaping.positionedGlyphs,
+                        glyphMap,
+                        lineStartIndex,
+                        shaping.positionedGlyphs.size() - 1,
+                        justify,
+                        baselineOffset);
         }
         
         x = 0;

From ddbd3929030e46f00c30157617e04a4bcb53a2af Mon Sep 17 00:00:00 2001
From: zmiao <miao.zhao@mapbox.com>
Date: Wed, 2 Oct 2019 11:46:42 +0300
Subject: [PATCH 10/12] [core] nit fix

---
 src/mbgl/text/glyph_pbf.cpp |  6 ++----
 src/mbgl/text/shaping.cpp   | 21 +++++++++++----------
 2 files changed, 13 insertions(+), 14 deletions(-)

diff --git a/src/mbgl/text/glyph_pbf.cpp b/src/mbgl/text/glyph_pbf.cpp
index 8ed48d08c13..dd9fe1638f2 100644
--- a/src/mbgl/text/glyph_pbf.cpp
+++ b/src/mbgl/text/glyph_pbf.cpp
@@ -83,18 +83,16 @@ std::tuple<std::vector<Glyph>, int32_t, int32_t> parseGlyphPBF(const GlyphRange&
             glyphs.push_back(std::move(glyph));
         };
 
-        uint16_t count{0};
         auto fontstack_pbf = glyphs_pbf.get_message();
         while (fontstack_pbf.next()) {
             switch (fontstack_pbf.tag()) {
                 case 3: {
                     readGlyphMetrics(fontstack_pbf);
-                    ++count;
                     break;
                 }
                 case 4: {
                     // ascender value for one fontstack shall keep the same, if different values appear, set ascender to
-                    // be 0.
+                    // be 0/invalid.
                     const auto value = fontstack_pbf.get_sint32();
                     if (!ascenderSet) {
                         ascender = value;
@@ -106,7 +104,7 @@ std::tuple<std::vector<Glyph>, int32_t, int32_t> parseGlyphPBF(const GlyphRange&
                 }
                 case 5: {
                     // descender value for one fontstack shall keep the same, if different values appear, set descender
-                    // to be 0.
+                    // to be 0/invalid.
                     const auto value = fontstack_pbf.get_sint32();
                     if (!descenderSet) {
                         descender = value;
diff --git a/src/mbgl/text/shaping.cpp b/src/mbgl/text/shaping.cpp
index 337dd613dd0..42b29b9f8d5 100644
--- a/src/mbgl/text/shaping.cpp
+++ b/src/mbgl/text/shaping.cpp
@@ -360,7 +360,7 @@ void shapeLines(Shaping& shaping,
             continue;
         }
 
-        float biggestHeight{0}, baselineOffset{0};
+        float biggestHeight{0.0f}, baselineOffset{0.0f};
         std::size_t lineStartIndex = shaping.positionedGlyphs.size();
         for (std::size_t i = 0; i < line.length(); i++) {
             const std::size_t sectionIndex = line.getSectionIndex(i);
@@ -377,13 +377,14 @@ void shapeLines(Shaping& shaping,
 
             const Glyph& glyph = **it->second;
 
-            double ascender{0}, descender{0}, glyphOffset{0};
+            float ascender{0.0f}, descender{0.0f}, glyphOffset{0.0f};
             // In order to make different fonts aligned, they must share a general baseline that aligns with every
-            // font's real baseline. Glyph's position is counted from the top left corner, where is the ascender line
-            // starts. Since ascender is above the baseline, the glyphOffset is the negative shift. In order to make all
-            // the glyphs aligned with shaping box, for each line, we lock the heighest glyph (with scale) locating
-            // at the middle of the line, which will lead to a baseline shift. Then adjust the whole line with the
-            // baseline offset we calculated from the shift.
+            // font's real baseline. Glyph's offset is counted from the top left corner, where is the ascender line
+            // starts. First of all, every glyph's baseline lies on the middle line of each shaping line. Since ascender
+            // is above the baseline, the glyphOffset is the negative shift. Then, in order to make glyphs fit in the
+            // shaping box, for each line, we shift the glyph with biggest height(with scale) to make its middle line
+            // lie on the middle line of the line, which will lead to a baseline shift. Then adjust the whole line with
+            // the baseline offset we calculated from the shift.
             if (hasBaseline) {
                 assert(glyphs->second.ascender && glyphs->second.descender);
                 ascender = std::abs(glyphs->second.ascender.value());
@@ -395,9 +396,9 @@ void shapeLines(Shaping& shaping,
                 }
                 glyphOffset = -ascender * section.scale;
             } else {
-                // 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.
+                // 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.
                 glyphOffset = Shaping::yOffset + (lineMaxScale - section.scale) * util::ONE_EM;
             }
 

From 1b97cc9e227f7b9729839b033c1223f33fe1a1e8 Mon Sep 17 00:00:00 2001
From: zmiao <miao.zhao@mapbox.com>
Date: Thu, 3 Oct 2019 11:57:09 +0300
Subject: [PATCH 11/12] [core] Add more shaping unit test cases

---
 test/text/shaping.test.cpp | 212 ++++++++++++++++++++++++++++++++-----
 1 file changed, 183 insertions(+), 29 deletions(-)

diff --git a/test/text/shaping.test.cpp b/test/text/shaping.test.cpp
index 142fcfbffba..ce6f4f48eeb 100644
--- a/test/text/shaping.test.cpp
+++ b/test/text/shaping.test.cpp
@@ -50,11 +50,12 @@ TEST(Shaping, ZWSP) {
         TaggedString string(u"中中\u200b中中\u200b中中\u200b中中中中中中\u200b中中", sectionOptions);
         auto shaping = testGetShaping(string, 5);
         ASSERT_EQ(shaping.lineCount, 3);
-        ASSERT_EQ(shaping.top, -36);
-        ASSERT_EQ(shaping.bottom, 36);
-        ASSERT_EQ(shaping.left, -63);
-        ASSERT_EQ(shaping.right, 63);
+        ASSERT_FLOAT_EQ(-36.0f, shaping.top);
+        ASSERT_FLOAT_EQ(36.0f, shaping.bottom);
+        ASSERT_FLOAT_EQ(-63.0f, shaping.left);
+        ASSERT_FLOAT_EQ(63.0f, shaping.right);
         ASSERT_EQ(shaping.writingMode, WritingModeType::Horizontal);
+        ASSERT_FALSE(shaping.hasBaseline);
     }
 
     // 2 lines
@@ -64,11 +65,12 @@ TEST(Shaping, ZWSP) {
         TaggedString string(u"中中\u200b中", sectionOptions);
         auto shaping = testGetShaping(string, 1);
         ASSERT_EQ(shaping.lineCount, 2);
-        ASSERT_EQ(shaping.top, -24);
-        ASSERT_EQ(shaping.bottom, 24);
-        ASSERT_EQ(shaping.left, -21);
-        ASSERT_EQ(shaping.right, 21);
+        ASSERT_FLOAT_EQ(-24.0f, shaping.top);
+        ASSERT_FLOAT_EQ(24.0f, shaping.bottom);
+        ASSERT_FLOAT_EQ(-21.0f, shaping.left);
+        ASSERT_FLOAT_EQ(21.0f, shaping.right);
         ASSERT_EQ(shaping.writingMode, WritingModeType::Horizontal);
+        ASSERT_FALSE(shaping.hasBaseline);
     }
 
     // 1 line
@@ -77,16 +79,17 @@ TEST(Shaping, ZWSP) {
         TaggedString string(u"中中\u200b", sectionOptions);
         auto shaping = testGetShaping(string, 2);
         ASSERT_EQ(shaping.lineCount, 1);
-        ASSERT_EQ(shaping.top, -12);
-        ASSERT_EQ(shaping.bottom, 12);
-        ASSERT_EQ(shaping.left, -21);
-        ASSERT_EQ(shaping.right, 21);
+        ASSERT_FLOAT_EQ(-12.0f, shaping.top);
+        ASSERT_FLOAT_EQ(12.0f, shaping.bottom);
+        ASSERT_FLOAT_EQ(-21.0f, shaping.left);
+        ASSERT_FLOAT_EQ(21.0f, shaping.right);
         ASSERT_EQ(shaping.writingMode, WritingModeType::Horizontal);
         ASSERT_EQ(2, shaping.positionedGlyphs.size());
-        EXPECT_FLOAT_EQ(-21, shaping.positionedGlyphs[0].x);
-        EXPECT_FLOAT_EQ(-17, shaping.positionedGlyphs[0].y);
-        EXPECT_FLOAT_EQ(0, shaping.positionedGlyphs[1].x);
-        EXPECT_FLOAT_EQ(-17, shaping.positionedGlyphs[1].y);
+        ASSERT_FLOAT_EQ(-21.0f, shaping.positionedGlyphs[0].x);
+        ASSERT_FLOAT_EQ(-17.0f, shaping.positionedGlyphs[0].y);
+        ASSERT_FLOAT_EQ(0.0f, shaping.positionedGlyphs[1].x);
+        ASSERT_FLOAT_EQ(-17.0f, shaping.positionedGlyphs[1].y);
+        ASSERT_FALSE(shaping.hasBaseline);
     }
 
     // 5 'new' lines.
@@ -94,15 +97,16 @@ TEST(Shaping, ZWSP) {
         TaggedString string(u"\u200b\u200b\u200b\u200b\u200b", sectionOptions);
         auto shaping = testGetShaping(string, 1);
         ASSERT_EQ(shaping.lineCount, 5);
-        ASSERT_EQ(shaping.top, -60);
-        ASSERT_EQ(shaping.bottom, 60);
-        ASSERT_EQ(shaping.left, 0);
-        ASSERT_EQ(shaping.right, 0);
+        ASSERT_FLOAT_EQ(-60.0f, shaping.top);
+        ASSERT_FLOAT_EQ(60.0f, shaping.bottom);
+        ASSERT_FLOAT_EQ(-0.0f, shaping.left);
+        ASSERT_FLOAT_EQ(0.0f, shaping.right);
         ASSERT_EQ(shaping.writingMode, WritingModeType::Horizontal);
+        ASSERT_FALSE(shaping.hasBaseline);
     }
 }
 
-TEST(Shaping, FontWithBaseline) {
+TEST(Shaping, MixedFontsBothWithBaselines) {
     Glyph glyph1;
     glyph1.id = u'阳';
     glyph1.metrics.width = 18;
@@ -163,15 +167,165 @@ TEST(Shaping, FontWithBaseline) {
 
         auto shaping = testGetShaping(string, 5);
         ASSERT_EQ(shaping.lineCount, 1);
-        ASSERT_EQ(shaping.top, -12);
-        ASSERT_EQ(shaping.bottom, 12);
-        ASSERT_EQ(shaping.left, -21);
-        ASSERT_EQ(shaping.right, 21);
+        ASSERT_FLOAT_EQ(-12.0f, shaping.top);
+        ASSERT_FLOAT_EQ(12.0f, shaping.bottom);
+        ASSERT_FLOAT_EQ(-21.0f, shaping.left);
+        ASSERT_FLOAT_EQ(21.0f, shaping.right);
         ASSERT_EQ(shaping.writingMode, WritingModeType::Horizontal);
         ASSERT_EQ(2, shaping.positionedGlyphs.size());
-        EXPECT_FLOAT_EQ(-21, shaping.positionedGlyphs[0].x);
-        EXPECT_FLOAT_EQ(-16, shaping.positionedGlyphs[0].y);
-        EXPECT_FLOAT_EQ(0, shaping.positionedGlyphs[1].x);
-        EXPECT_FLOAT_EQ(-15, shaping.positionedGlyphs[1].y);
+        ASSERT_FLOAT_EQ(-21.0f, shaping.positionedGlyphs[0].x);
+        ASSERT_FLOAT_EQ(-16.0f, shaping.positionedGlyphs[0].y);
+        ASSERT_FLOAT_EQ(0.0f, shaping.positionedGlyphs[1].x);
+        ASSERT_FLOAT_EQ(-15.0f, shaping.positionedGlyphs[1].y);
+        ASSERT_TRUE(shaping.hasBaseline);
+    }
+}
+
+TEST(Shaping, MixedFontsOneWithBaselineOneWithout) {
+    Glyph glyph1;
+    glyph1.id = u'阳';
+    glyph1.metrics.width = 18;
+    glyph1.metrics.height = 19;
+    glyph1.metrics.left = 2;
+    glyph1.metrics.top = -8;
+    glyph1.metrics.advance = 21;
+
+    Glyph glyph2;
+    glyph2.id = u'光';
+    glyph2.metrics.width = 18;
+    glyph2.metrics.height = 18;
+    glyph2.metrics.left = 2;
+    glyph2.metrics.top = -8;
+    glyph2.metrics.advance = 21;
+
+    BiDi bidi;
+    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))));
+
+    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,
+                          maxWidthInChars * ONE_EM,
+                          ONE_EM, // lineHeight
+                          style::SymbolAnchorType::Center,
+                          style::TextJustifyType::Center,
+                          0,              // spacing
+                          {{0.0f, 0.0f}}, // translate
+                          WritingModeType::Horizontal,
+                          bidi,
+                          glyphs,
+                          /*allowVerticalPlacement*/ false);
+    };
+
+    {
+        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_FLOAT_EQ(-12.0f, shaping.top);
+        ASSERT_FLOAT_EQ(12.0f, shaping.bottom);
+        ASSERT_FLOAT_EQ(-21.0f, shaping.left);
+        ASSERT_FLOAT_EQ(21.0f, shaping.right);
+        ASSERT_EQ(shaping.writingMode, WritingModeType::Horizontal);
+        ASSERT_EQ(2, shaping.positionedGlyphs.size());
+        ASSERT_FLOAT_EQ(-21.0f, shaping.positionedGlyphs[0].x);
+        ASSERT_FLOAT_EQ(-17.0f, shaping.positionedGlyphs[0].y);
+        ASSERT_FLOAT_EQ(0.0f, shaping.positionedGlyphs[1].x);
+        ASSERT_FLOAT_EQ(-17.0f, shaping.positionedGlyphs[1].y);
+        ASSERT_FALSE(shaping.hasBaseline);
+    }
+}
+
+TEST(Shaping, MixedFontsWithBaselineWithFontScale) {
+    Glyph glyph1;
+    glyph1.id = u'阳';
+    glyph1.metrics.width = 18;
+    glyph1.metrics.height = 19;
+    glyph1.metrics.left = 2;
+    glyph1.metrics.top = -8;
+    glyph1.metrics.advance = 21;
+
+    Glyph glyph2;
+    glyph2.id = u'光';
+    glyph2.metrics.width = 18;
+    glyph2.metrics.height = 18;
+    glyph2.metrics.left = 2;
+    glyph2.metrics.top = -8;
+    glyph2.metrics.advance = 21;
+
+    BiDi bidi;
+    std::vector<SectionOptions> sectionOptions;
+    const std::vector<std::string> fontStack1{{"font-stack1"}};
+    sectionOptions.emplace_back(1.0f, fontStack1);
+    sectionOptions.back().scale = 1.5;
+    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,
+                          maxWidthInChars * ONE_EM,
+                          ONE_EM, // lineHeight
+                          style::SymbolAnchorType::Center,
+                          style::TextJustifyType::Center,
+                          0,              // spacing
+                          {{0.0f, 0.0f}}, // translate
+                          WritingModeType::Horizontal,
+                          bidi,
+                          glyphs,
+                          /*allowVerticalPlacement*/ false);
+    };
+
+    {
+        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_FLOAT_EQ(-18.0f, shaping.top);
+        ASSERT_FLOAT_EQ(18.0f, shaping.bottom);
+        ASSERT_FLOAT_EQ(-26.25f, shaping.left);
+        ASSERT_FLOAT_EQ(26.25f, shaping.right);
+        ASSERT_EQ(shaping.writingMode, WritingModeType::Horizontal);
+        ASSERT_EQ(2, shaping.positionedGlyphs.size());
+        ASSERT_FLOAT_EQ(-26.25f, shaping.positionedGlyphs[0].x);
+        ASSERT_FLOAT_EQ(-24.0f, shaping.positionedGlyphs[0].y);
+        ASSERT_FLOAT_EQ(5.25f, shaping.positionedGlyphs[1].x);
+        ASSERT_FLOAT_EQ(-10.0f, shaping.positionedGlyphs[1].y);
+        ASSERT_TRUE(shaping.hasBaseline);
     }
 }

From da7a67bae8bbd09d0785897539092fffd02c8fa6 Mon Sep 17 00:00:00 2001
From: zmiao <miao.zhao@mapbox.com>
Date: Fri, 4 Oct 2019 22:58:53 +0300
Subject: [PATCH 12/12] [core] Fix multiple line vertical text shaping

---
 src/mbgl/text/glyph.hpp         |   2 +-
 src/mbgl/text/glyph_manager.cpp |   2 +-
 src/mbgl/text/quads.cpp         | 173 ++++++++++++++++----------------
 src/mbgl/text/shaping.cpp       |  30 +++---
 test/text/quads.test.cpp        |   2 +-
 test/text/shaping.test.cpp      |  55 ++++++----
 6 files changed, 144 insertions(+), 120 deletions(-)

diff --git a/src/mbgl/text/glyph.hpp b/src/mbgl/text/glyph.hpp
index 4ba93985c78..09b90009e83 100644
--- a/src/mbgl/text/glyph.hpp
+++ b/src/mbgl/text/glyph.hpp
@@ -83,7 +83,7 @@ class Shaping {
     Shaping() = default;
     explicit Shaping(float x, float y, WritingModeType writingMode_, std::size_t lineCount_)
         : top(y), bottom(y), left(x), right(x), writingMode(writingMode_), lineCount(lineCount_) {}
-    std::vector<PositionedGlyph> positionedGlyphs;
+    std::unordered_map<uint32_t, std::vector<PositionedGlyph>> positionedGlyphs;
     float top = 0;
     float bottom = 0;
     float left = 0;
diff --git a/src/mbgl/text/glyph_manager.cpp b/src/mbgl/text/glyph_manager.cpp
index 773e2abe9d5..accae0cf9c8 100644
--- a/src/mbgl/text/glyph_manager.cpp
+++ b/src/mbgl/text/glyph_manager.cpp
@@ -146,7 +146,7 @@ void GlyphManager::notify(GlyphRequestor& requestor, const GlyphDependencies& gl
             if (it != entry.glyphs.end()) {
                 glyphs.glyphs.emplace(*it);
             } else {
-                glyphs.glyphs.emplace(glyphID, std::experimental::nullopt);
+                glyphs.glyphs.emplace(glyphID, nullopt);
             }
         }
     }
diff --git a/src/mbgl/text/quads.cpp b/src/mbgl/text/quads.cpp
index 49b43c413ab..b2406fc4d33 100644
--- a/src/mbgl/text/quads.cpp
+++ b/src/mbgl/text/quads.cpp
@@ -68,91 +68,96 @@ SymbolQuads getGlyphQuads(const Shaping& shapedText,
 
     SymbolQuads quads;
 
-    for (const PositionedGlyph &positionedGlyph: shapedText.positionedGlyphs) {
-        auto fontPositions = positions.find(positionedGlyph.font);
-        if (fontPositions == positions.end())
-            continue;
-
-        auto positionsIt = fontPositions->second.glyphPositionMap.find(positionedGlyph.glyph);
-        if (positionsIt == fontPositions->second.glyphPositionMap.end()) {
-            continue;
+    if (shapedText.lineCount == 0) return quads;
+    const float lineHeight = (std::fabs(shapedText.bottom) + std::fabs(shapedText.top)) / shapedText.lineCount;
+    for (const auto& positionedGlyphs : shapedText.positionedGlyphs) {
+        const float currentHeight = lineHeight * positionedGlyphs.first;
+        for (const PositionedGlyph& positionedGlyph : positionedGlyphs.second) {
+            auto fontPositions = positions.find(positionedGlyph.font);
+            if (fontPositions == positions.end()) continue;
+
+            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;
+
+            // The rects have an additional buffer that is not included in their size;
+            const float glyphPadding = 1.0f;
+            const float rectBuffer = 3.0f + glyphPadding;
+
+            const float halfAdvance = glyph.metrics.advance * positionedGlyph.scale / 2.0;
+
+            const Point<float> glyphOffset =
+                alongLine ? Point<float>{positionedGlyph.x + halfAdvance, positionedGlyph.y} : Point<float>{0.0f, 0.0f};
+
+            Point<float> builtInOffset = alongLine ? Point<float>{0.0f, 0.0f}
+                                                   : Point<float>{positionedGlyph.x + halfAdvance + textOffset[0],
+                                                                  positionedGlyph.y + textOffset[1]};
+
+            Point<float> verticalizedLabelOffset = {0.0f, 0.0f};
+            const bool rotateVerticalGlyph = (alongLine || allowVerticalPlacement) && positionedGlyph.vertical;
+            if (rotateVerticalGlyph) {
+                // Vertical POI labels, that are rotated 90deg CW and whose glyphs must preserve upright orientation
+                // need to be rotated 90deg CCW. After quad is rotated, it is translated to the original built-in
+                // offset.
+                verticalizedLabelOffset = builtInOffset;
+                builtInOffset = {0.0f, 0.0f};
+            }
+
+            const float x1 = (glyph.metrics.left - rectBuffer) * positionedGlyph.scale - halfAdvance + builtInOffset.x;
+            const float y1 = (-glyph.metrics.top - rectBuffer) * positionedGlyph.scale + builtInOffset.y;
+            const float x2 = x1 + rect.w * positionedGlyph.scale;
+            const float y2 = y1 + rect.h * positionedGlyph.scale;
+
+            Point<float> tl{x1, y1};
+            Point<float> tr{x2, y1};
+            Point<float> bl{x1, y2};
+            Point<float> br{x2, y2};
+
+            if (rotateVerticalGlyph) {
+                // Vertical-supporting glyphs are laid out in 24x24 point boxes (1 square em)
+                // In horizontal orientation, the y values for glyphs are below the midline.
+                // If the glyph's baseline is applicable, we take the relative y offset of the
+                //  glyph, which needs to erase out current line height that added to the glyphs.
+                // Otherwise, we use a "yOffset" of -17 to pull them up to the middle.
+                // By rotating counter-clockwise around the point at the center of the left
+                // edge of a 24x24 layout box centered below the midline, we align the center
+                // of the glyphs with the horizontal midline, so the yOffset is no longer
+                // 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.
+                const float yShift = (shapedText.hasBaseline ? (positionedGlyph.y - currentHeight) : Shaping::yOffset);
+                const Point<float> center{-halfAdvance, halfAdvance - yShift};
+                const float verticalRotation = -M_PI_2;
+
+                // xHalfWidhtOffsetcorrection is a difference between full-width and half-width
+                // advance, should be 0 for full-width glyphs and will pull up half-width glyphs.
+                const float xHalfWidhtOffsetcorrection = util::ONE_EM / 2 - halfAdvance;
+                const Point<float> xOffsetCorrection{5.0f - yShift - xHalfWidhtOffsetcorrection, 0.0f};
+
+                tl = util::rotate(tl - center, verticalRotation) + center + xOffsetCorrection + verticalizedLabelOffset;
+                tr = util::rotate(tr - center, verticalRotation) + center + xOffsetCorrection + verticalizedLabelOffset;
+                bl = util::rotate(bl - center, verticalRotation) + center + xOffsetCorrection + verticalizedLabelOffset;
+                br = util::rotate(br - center, verticalRotation) + center + xOffsetCorrection + verticalizedLabelOffset;
+            }
+
+            if (textRotate) {
+                // Compute the transformation matrix.
+                float angle_sin = std::sin(textRotate);
+                float angle_cos = std::cos(textRotate);
+                std::array<float, 4> matrix = {{angle_cos, -angle_sin, angle_sin, angle_cos}};
+
+                tl = util::matrixMultiply(matrix, tl);
+                tr = util::matrixMultiply(matrix, tr);
+                bl = util::matrixMultiply(matrix, bl);
+                br = util::matrixMultiply(matrix, br);
+            }
+
+            quads.emplace_back(tl, tr, bl, br, rect, shapedText.writingMode, glyphOffset, positionedGlyph.sectionIndex);
         }
-
-        const GlyphPosition& glyph = positionsIt->second;
-        const Rect<uint16_t>& rect = glyph.rect;
-
-        // The rects have an additional buffer that is not included in their size;
-        const float glyphPadding = 1.0f;
-        const float rectBuffer = 3.0f + glyphPadding;
-
-        const float halfAdvance = glyph.metrics.advance * positionedGlyph.scale / 2.0;
-
-        const Point<float> glyphOffset = alongLine ?
-            Point<float>{ positionedGlyph.x + halfAdvance, positionedGlyph.y } :
-            Point<float>{ 0.0f, 0.0f };
-
-        Point<float> builtInOffset = alongLine ?
-            Point<float>{ 0.0f, 0.0f } :
-            Point<float>{ positionedGlyph.x + halfAdvance + textOffset[0], positionedGlyph.y + textOffset[1] };
-
-        Point<float> verticalizedLabelOffset = { 0.0f, 0.0f };
-        const bool rotateVerticalGlyph = (alongLine || allowVerticalPlacement) && positionedGlyph.vertical;
-        if (rotateVerticalGlyph) {
-            // Vertical POI labels, that are rotated 90deg CW and whose glyphs must preserve upright orientation
-            // need to be rotated 90deg CCW. After quad is rotated, it is translated to the original built-in offset.
-            verticalizedLabelOffset = builtInOffset;
-            builtInOffset = { 0.0f, 0.0f };
-        }
-
-        const float x1 = (glyph.metrics.left - rectBuffer) * positionedGlyph.scale - halfAdvance + builtInOffset.x;
-        const float y1 = (-glyph.metrics.top - rectBuffer) * positionedGlyph.scale + builtInOffset.y;
-        const float x2 = x1 + rect.w * positionedGlyph.scale;
-        const float y2 = y1 + rect.h * positionedGlyph.scale;
-
-        Point<float> tl{x1, y1};
-        Point<float> tr{x2, y1};
-        Point<float> bl{x1, y2};
-        Point<float> br{x2, y2};
-
-        if (rotateVerticalGlyph) {
-            // Vertical-supporting glyphs are laid out in 24x24 point boxes (1 square em)
-            // In horizontal orientation, the y values for glyphs are below the midline.
-            // If the glyph's baseline is applicable, we take the y value of the glyph.
-            // Otherwise, we use a "yOffset" of -17 to pull them up to the middle.
-            // By rotating counter-clockwise around the point at the center of the left
-            // edge of a 24x24 layout box centered below the midline, we align the center
-            // of the glyphs with the horizontal midline, so the yOffset is no longer
-            // 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.
-            const float yShift = (shapedText.hasBaseline ? positionedGlyph.y : Shaping::yOffset);
-            const Point<float> center{-halfAdvance, halfAdvance - yShift};
-            const float verticalRotation = -M_PI_2;
-
-            // xHalfWidhtOffsetcorrection is a difference between full-width and half-width
-            // advance, should be 0 for full-width glyphs and will pull up half-width glyphs.
-            const float xHalfWidhtOffsetcorrection = util::ONE_EM / 2 - halfAdvance;
-            const Point<float> xOffsetCorrection{5.0f - yShift - xHalfWidhtOffsetcorrection, 0.0f};
-
-            tl = util::rotate(tl - center, verticalRotation) + center + xOffsetCorrection + verticalizedLabelOffset;
-            tr = util::rotate(tr - center, verticalRotation) + center + xOffsetCorrection + verticalizedLabelOffset;
-            bl = util::rotate(bl - center, verticalRotation) + center + xOffsetCorrection + verticalizedLabelOffset;
-            br = util::rotate(br - center, verticalRotation) + center + xOffsetCorrection + verticalizedLabelOffset;
-        }
-
-        if (textRotate) {
-            // Compute the transformation matrix.
-            float angle_sin = std::sin(textRotate);
-            float angle_cos = std::cos(textRotate);
-            std::array<float, 4> matrix = {{angle_cos, -angle_sin, angle_sin, angle_cos}};
-
-            tl = util::matrixMultiply(matrix, tl);
-            tr = util::matrixMultiply(matrix, tr);
-            bl = util::matrixMultiply(matrix, bl);
-            br = util::matrixMultiply(matrix, br);
-        }
-
-        quads.emplace_back(tl, tr, bl, br, rect, shapedText.writingMode, glyphOffset, positionedGlyph.sectionIndex);
     }
 
     return quads;
diff --git a/src/mbgl/text/shaping.cpp b/src/mbgl/text/shaping.cpp
index 42b29b9f8d5..02d53598b3b 100644
--- a/src/mbgl/text/shaping.cpp
+++ b/src/mbgl/text/shaping.cpp
@@ -122,10 +122,12 @@ void align(Shaping& shaping,
            const std::size_t lineCount) {
     const float shiftX = (justify - horizontalAlign) * maxLineLength;
     const float shiftY = (-verticalAlign * lineCount + 0.5) * lineHeight;
-    
-    for (auto& glyph : shaping.positionedGlyphs) {
-        glyph.x += shiftX;
-        glyph.y += shiftY;
+
+    for (auto& glyphs : shaping.positionedGlyphs) {
+        for (auto& glyph : glyphs.second) {
+            glyph.x += shiftX;
+            glyph.y += shiftY;
+        }
     }
 }
 
@@ -149,7 +151,7 @@ void justifyLine(std::vector<PositionedGlyph>& positionedGlyphs,
     if (it != glyphs->second.glyphs.end() && it->second) {
         const float lastAdvance = (*it->second)->metrics.advance * glyph.scale;
         const float lineIndent = float(glyph.x + lastAdvance) * justify;
-        
+
         for (std::size_t j = start; j <= end; j++) {
             positionedGlyphs[j].x -= lineIndent;
             positionedGlyphs[j].y += baselineOffset;
@@ -349,6 +351,7 @@ void shapeLines(Shaping& shaping,
         textJustify == style::TextJustifyType::Left ? 0 :
         0.5;
 
+    uint32_t lineIndex{0};
     for (TaggedString& line : lines) {
         // Collapse whitespace so it doesn't throw off justification
         line.trim();
@@ -357,11 +360,11 @@ void shapeLines(Shaping& shaping,
         
         if (line.empty()) {
             y += lineHeight; // Still need a line feed after empty line
+            ++lineIndex;
             continue;
         }
 
         float biggestHeight{0.0f}, baselineOffset{0.0f};
-        std::size_t lineStartIndex = shaping.positionedGlyphs.size();
         for (std::size_t i = 0; i < line.length(); i++) {
             const std::size_t sectionIndex = line.getSectionIndex(i);
             const SectionOptions& section = line.sectionAt(sectionIndex);
@@ -409,31 +412,32 @@ void shapeLines(Shaping& shaping,
                 // are from complex text layout script, or whitespaces.
                 (allowVerticalPlacement &&
                  (util::i18n::isWhitespace(codePoint) || util::i18n::isCharInComplexShapingScript(codePoint)))) {
-                shaping.positionedGlyphs.emplace_back(
+                shaping.positionedGlyphs[lineIndex].emplace_back(
                     codePoint, x, y + glyphOffset, false, section.fontStackHash, section.scale, sectionIndex);
                 x += glyph.metrics.advance * section.scale + spacing;
             } else {
-                shaping.positionedGlyphs.emplace_back(
+                shaping.positionedGlyphs[lineIndex].emplace_back(
                     codePoint, x, y + glyphOffset, true, section.fontStackHash, section.scale, sectionIndex);
                 x += util::ONE_EM * section.scale + spacing;
             }
         }
 
         // Only justify if we placed at least one glyph
-        if (shaping.positionedGlyphs.size() != lineStartIndex) {
+        if (!shaping.positionedGlyphs[lineIndex].empty()) {
             float lineLength = x - spacing; // Don't count trailing spacing
             maxLineLength = util::max(lineLength, maxLineLength);
 
-            justifyLine(shaping.positionedGlyphs,
+            justifyLine(shaping.positionedGlyphs[lineIndex],
                         glyphMap,
-                        lineStartIndex,
-                        shaping.positionedGlyphs.size() - 1,
+                        0,
+                        shaping.positionedGlyphs[lineIndex].size() - 1,
                         justify,
                         baselineOffset);
         }
-        
+
         x = 0;
         y += lineHeight * lineMaxScale;
+        ++lineIndex;
     }
 
     auto anchorAlign = AnchorAlignment::getAnchorAlignment(textAnchor);
diff --git a/test/text/quads.test.cpp b/test/text/quads.test.cpp
index 7aaeb4870df..7a1651c7f49 100644
--- a/test/text/quads.test.cpp
+++ b/test/text/quads.test.cpp
@@ -47,7 +47,7 @@ TEST(getIconQuads, style) {
     shapedText.bottom = 30.0f;
     shapedText.left = -60.0f;
     shapedText.right = 20.0f;
-    shapedText.positionedGlyphs.emplace_back(PositionedGlyph(32, 0.0f, 0.0f, false, 0, 1.0));
+    shapedText.positionedGlyphs[0].emplace_back(PositionedGlyph(32, 0.0f, 0.0f, false, 0, 1.0));
 
     // none
     {
diff --git a/test/text/shaping.test.cpp b/test/text/shaping.test.cpp
index ce6f4f48eeb..b7f4589f67c 100644
--- a/test/text/shaping.test.cpp
+++ b/test/text/shaping.test.cpp
@@ -55,6 +55,7 @@ TEST(Shaping, ZWSP) {
         ASSERT_FLOAT_EQ(-63.0f, shaping.left);
         ASSERT_FLOAT_EQ(63.0f, shaping.right);
         ASSERT_EQ(shaping.writingMode, WritingModeType::Horizontal);
+        ASSERT_EQ(shaping.lineCount, shaping.positionedGlyphs.size());
         ASSERT_FALSE(shaping.hasBaseline);
     }
 
@@ -70,6 +71,7 @@ TEST(Shaping, ZWSP) {
         ASSERT_FLOAT_EQ(-21.0f, shaping.left);
         ASSERT_FLOAT_EQ(21.0f, shaping.right);
         ASSERT_EQ(shaping.writingMode, WritingModeType::Horizontal);
+        ASSERT_EQ(shaping.lineCount, shaping.positionedGlyphs.size());
         ASSERT_FALSE(shaping.hasBaseline);
     }
 
@@ -84,11 +86,14 @@ TEST(Shaping, ZWSP) {
         ASSERT_FLOAT_EQ(-21.0f, shaping.left);
         ASSERT_FLOAT_EQ(21.0f, shaping.right);
         ASSERT_EQ(shaping.writingMode, WritingModeType::Horizontal);
-        ASSERT_EQ(2, shaping.positionedGlyphs.size());
-        ASSERT_FLOAT_EQ(-21.0f, shaping.positionedGlyphs[0].x);
-        ASSERT_FLOAT_EQ(-17.0f, shaping.positionedGlyphs[0].y);
-        ASSERT_FLOAT_EQ(0.0f, shaping.positionedGlyphs[1].x);
-        ASSERT_FLOAT_EQ(-17.0f, shaping.positionedGlyphs[1].y);
+        ASSERT_EQ(shaping.lineCount, shaping.positionedGlyphs.size());
+        ASSERT_EQ(1, shaping.positionedGlyphs.count(0));
+        ASSERT_EQ(2, shaping.positionedGlyphs[0].size());
+        const auto& glyphs = shaping.positionedGlyphs[0];
+        ASSERT_FLOAT_EQ(-21.0f, glyphs[0].x);
+        ASSERT_FLOAT_EQ(-17.0f, glyphs[0].y);
+        ASSERT_FLOAT_EQ(0.0f, glyphs[1].x);
+        ASSERT_FLOAT_EQ(-17.0f, glyphs[1].y);
         ASSERT_FALSE(shaping.hasBaseline);
     }
 
@@ -102,6 +107,7 @@ TEST(Shaping, ZWSP) {
         ASSERT_FLOAT_EQ(-0.0f, shaping.left);
         ASSERT_FLOAT_EQ(0.0f, shaping.right);
         ASSERT_EQ(shaping.writingMode, WritingModeType::Horizontal);
+        ASSERT_EQ(shaping.lineCount, shaping.positionedGlyphs.size());
         ASSERT_FALSE(shaping.hasBaseline);
     }
 }
@@ -172,11 +178,14 @@ TEST(Shaping, MixedFontsBothWithBaselines) {
         ASSERT_FLOAT_EQ(-21.0f, shaping.left);
         ASSERT_FLOAT_EQ(21.0f, shaping.right);
         ASSERT_EQ(shaping.writingMode, WritingModeType::Horizontal);
-        ASSERT_EQ(2, shaping.positionedGlyphs.size());
-        ASSERT_FLOAT_EQ(-21.0f, shaping.positionedGlyphs[0].x);
-        ASSERT_FLOAT_EQ(-16.0f, shaping.positionedGlyphs[0].y);
-        ASSERT_FLOAT_EQ(0.0f, shaping.positionedGlyphs[1].x);
-        ASSERT_FLOAT_EQ(-15.0f, shaping.positionedGlyphs[1].y);
+        ASSERT_EQ(shaping.lineCount, shaping.positionedGlyphs.size());
+        ASSERT_EQ(1, shaping.positionedGlyphs.count(0));
+        ASSERT_EQ(2, shaping.positionedGlyphs[0].size());
+        const auto& glyphs = shaping.positionedGlyphs[0];
+        ASSERT_FLOAT_EQ(-21.0f, glyphs[0].x);
+        ASSERT_FLOAT_EQ(-16.0f, glyphs[0].y);
+        ASSERT_FLOAT_EQ(0.0f, glyphs[1].x);
+        ASSERT_FLOAT_EQ(-15.0f, glyphs[1].y);
         ASSERT_TRUE(shaping.hasBaseline);
     }
 }
@@ -245,11 +254,14 @@ TEST(Shaping, MixedFontsOneWithBaselineOneWithout) {
         ASSERT_FLOAT_EQ(-21.0f, shaping.left);
         ASSERT_FLOAT_EQ(21.0f, shaping.right);
         ASSERT_EQ(shaping.writingMode, WritingModeType::Horizontal);
-        ASSERT_EQ(2, shaping.positionedGlyphs.size());
-        ASSERT_FLOAT_EQ(-21.0f, shaping.positionedGlyphs[0].x);
-        ASSERT_FLOAT_EQ(-17.0f, shaping.positionedGlyphs[0].y);
-        ASSERT_FLOAT_EQ(0.0f, shaping.positionedGlyphs[1].x);
-        ASSERT_FLOAT_EQ(-17.0f, shaping.positionedGlyphs[1].y);
+        ASSERT_EQ(shaping.lineCount, shaping.positionedGlyphs.size());
+        ASSERT_EQ(1, shaping.positionedGlyphs.count(0));
+        ASSERT_EQ(2, shaping.positionedGlyphs[0].size());
+        const auto& glyphs = shaping.positionedGlyphs[0];
+        ASSERT_FLOAT_EQ(-21.0f, glyphs[0].x);
+        ASSERT_FLOAT_EQ(-17.0f, glyphs[0].y);
+        ASSERT_FLOAT_EQ(0.0f, glyphs[1].x);
+        ASSERT_FLOAT_EQ(-17.0f, glyphs[1].y);
         ASSERT_FALSE(shaping.hasBaseline);
     }
 }
@@ -321,11 +333,14 @@ TEST(Shaping, MixedFontsWithBaselineWithFontScale) {
         ASSERT_FLOAT_EQ(-26.25f, shaping.left);
         ASSERT_FLOAT_EQ(26.25f, shaping.right);
         ASSERT_EQ(shaping.writingMode, WritingModeType::Horizontal);
-        ASSERT_EQ(2, shaping.positionedGlyphs.size());
-        ASSERT_FLOAT_EQ(-26.25f, shaping.positionedGlyphs[0].x);
-        ASSERT_FLOAT_EQ(-24.0f, shaping.positionedGlyphs[0].y);
-        ASSERT_FLOAT_EQ(5.25f, shaping.positionedGlyphs[1].x);
-        ASSERT_FLOAT_EQ(-10.0f, shaping.positionedGlyphs[1].y);
+        ASSERT_EQ(shaping.lineCount, shaping.positionedGlyphs.size());
+        ASSERT_EQ(1, shaping.positionedGlyphs.count(0));
+        ASSERT_EQ(2, shaping.positionedGlyphs[0].size());
+        const auto& glyphs = shaping.positionedGlyphs[0];
+        ASSERT_FLOAT_EQ(-26.25f, glyphs[0].x);
+        ASSERT_FLOAT_EQ(-24.0f, glyphs[0].y);
+        ASSERT_FLOAT_EQ(5.25f, glyphs[1].x);
+        ASSERT_FLOAT_EQ(-10.0f, glyphs[1].y);
         ASSERT_TRUE(shaping.hasBaseline);
     }
 }