[core] Fix multiple line vertical text shapingupstream/zmiao-fix-mixed-fonts-disalignment

6 files changed, 144 insertions, 120 deletions

diff --git a/src/mbgl/text/glyph.hpp b/src/mbgl/text/glyph.hpp
index 4ba93985c7..09b90009e8 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 773e2abe9d..accae0cf9c 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 49b43c413a..b2406fc4d3 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 42b29b9f8d..02d53598b3 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 7aaeb4870d..7a1651c7f4 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 ce6f4f48ee..b7f4589f67 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);
     }
 }