From 6c417c9052754fd67a64d3fa9dffdef157968520 Mon Sep 17 00:00:00 2001 From: Sergey Fedorov Date: Sat, 25 Jul 2026 19:08:32 +0000 Subject: [PATCH 10/13] fix(vendor): step the text pen by cluster, not shaping-font advance (ghost math glyphs) A grid cell holding a covered base plus a codepoint the primary font lacks is one cluster, and a cluster is indivisible, so font fallback re-shapes the whole cell with a substitute font. That substitute is usually proportional, and HarfBuzz returns its advances for a normal side-by-side run -- e.g. ~2 cells for a two-codepoint cell, ~3 for a base plus two combining marks. renderTextGroup stepped the pen by those per-glyph advances (an exact 2x survives the snap-to-whole-cells too), so the cell's later glyphs, and everything after, were drawn a cell or two too far right: a "ghost" glyph beside the real one, whose position shifted with the surrounding SGR runs (they change which cells share a shaping group). This was the math-symbol ghost blob (e.g. after the summation sign) and the dropped/misplaced combining marks in mathematical text. Carry the input cluster on text::glyph_position (open_shaper fills it from info[i].cluster; it survives the fallback splice), and step the pen by the cluster: every glyph of one cluster shares the cell's column, a combining mark stacks via its own HarfBuzz offset, and the column advances only when the cluster does. A shaper that does not populate clusters (DirectWrite; never built by this port) is detected -- a multi-glyph run whose back cluster is no greater than its front -- and keeps the old advance-based stepping, so Windows behaviour is unchanged. Regression tests: open_shaper.fallback.carries_the_cluster_through_the_splice, and a new section in TextRenderer.fallback_run_stays_on_the_cell_grid. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01JgBWxn9uoMJzT5vQNqviAA (cherry picked from commit ae54e6875350e5233a313541d0cf55739b3f5602) --- src/text_shaper/OpenShaper.cpp | 1 + src/text_shaper/OpenShaper_test.cpp | 42 +++++++++++++++ src/text_shaper/Shaper.hpp | 10 ++++ src/vtrasterizer/TextRenderer.cpp | 72 ++++++++++++++++++-------- src/vtrasterizer/TextRenderer_test.cpp | 17 ++++++ 5 files changed, 119 insertions(+), 23 deletions(-) diff --git a/src/text_shaper/OpenShaper.cpp b/src/text_shaper/OpenShaper.cpp index d05528ec..536ab7c7 100644 --- a/src/text_shaper/OpenShaper.cpp +++ b/src/text_shaper/OpenShaper.cpp @@ -620,6 +620,7 @@ namespace GlyphPosition gpos {}; gpos.glyph = GlyphKey { .size = fontInfo.size, .font = font, .index = GlyphIndex { info[i].codepoint } }; + gpos.cluster = info[i].cluster; #ifdef GLYPH_KEY_DEBUG { auto const cluster = info[i].cluster; diff --git a/src/text_shaper/OpenShaper_test.cpp b/src/text_shaper/OpenShaper_test.cpp index 4bbf5e14..5a4fcf7f 100644 --- a/src/text_shaper/OpenShaper_test.cpp +++ b/src/text_shaper/OpenShaper_test.cpp @@ -359,6 +359,48 @@ TEST_CASE("OpenShaper.fallback.a_combining_mark_travels_with_its_base", "[OpenSh } } +TEST_CASE("OpenShaper.fallback.carries_the_cluster_through_the_splice", "[OpenShaper][fallback]") +{ + // The cluster is the glyph's cell, and the renderer steps the pen by it instead of by the shaping + // font's advance -- so it must survive font fallback. A covered base and a codepoint the primary + // lacks share one cell (cluster 0); a following letter is a second cell (cluster 1). The base+missing + // pair is re-shaped whole by a proportional fallback whose advances overshoot the terminal grid, which + // is exactly the ghost-glyph case: the pen must not trust those advances, only the cluster. + auto env = FallbackEnv { { + { .name = "primary", .monospace = Monospaced, .glyphs = { { U'a', 8 }, { U'b', 8 } } }, + { .name = "fallback", + .monospace = Proportional, + .glyphs = { { U'a', 20 }, { Snowman, 20 }, { U'b', 20 } } }, + } }; + + auto const primary = env.key("primary"); + auto const fallback = env.key("fallback"); + + // Cell 0: 'a' + Snowman (Snowman missing from the primary). Cell 1: 'b'. + auto clusters = vector { 0, 0, 1 }; + auto result = ShapeResult {}; + env.shaper().shape(primary, + u32string { U'a', Snowman, U'b' }, + gsl::span { clusters }, + unicode::Script::Latin, + unicode::PresentationStyle::Text, + result); + REQUIRE(result.size() == 3); + + // 'a' and Snowman are one cell together in the fallback; 'b' is the next cell in the primary. + CHECK(result[0].cluster == 0); + CHECK(result[1].cluster == 0); + CHECK(result[2].cluster == 1); + + CHECK(result[0].glyph.font == fallback); + CHECK(result[1].glyph.font == fallback); + CHECK(result[2].glyph.font == primary); + + // The fallback really does report an over-a-cell advance for the base; the renderer must ignore it and + // step by the cluster, which is what this test pins the cluster values down for. + CHECK(result[0].advance.x > 8); +} + TEST_CASE("OpenShaper.fallback.respects_the_fallback_limit", "[OpenShaper][fallback]") { auto env = FallbackEnv { { diff --git a/src/text_shaper/Shaper.hpp b/src/text_shaper/Shaper.hpp index 1bbd4c15..434a2ad7 100644 --- a/src/text_shaper/Shaper.hpp +++ b/src/text_shaper/Shaper.hpp @@ -74,6 +74,16 @@ struct GlyphPosition crispy::Point advance; unicode::PresentationStyle presentation {}; + + /// The input cluster this glyph was shaped from, i.e. which grid cell it belongs to. + /// + /// Carried all the way through font fallback so the renderer can step the pen by whole cells -- + /// zero for a combining mark sharing its base's cluster, one per cell for ordinary text -- instead + /// of accumulating the shaping font's advances. A fallback font asked to shape a whole cell-cluster + /// (a covered base plus a codepoint the primary lacked) reports advances for a proportional run, + /// which overshoot the terminal grid and draw the next glyph a cell too far; the cluster is the only + /// datum that pins each glyph back to its one cell. @see TextRenderer::renderTextGroup. + unsigned cluster {}; }; using ShapeResult = std::vector; diff --git a/src/vtrasterizer/TextRenderer.cpp b/src/vtrasterizer/TextRenderer.cpp index 25e4ea71..f0ea3638 100644 --- a/src/vtrasterizer/TextRenderer.cpp +++ b/src/vtrasterizer/TextRenderer.cpp @@ -659,22 +659,65 @@ void TextRenderer::renderTextGroup(std::u32string_view codepoints, return; } + // The pen steps by the glyph's CLUSTER, not by the shaping font's advance. A cluster is a grid + // cell, so every glyph of one cluster shares a single cell-aligned pen base -- a combining mark + // draws over its base via its own HarfBuzz offset, and the cell steps forward only when the + // cluster does. The shaping font's advance is deliberately ignored for pen stepping: a fallback + // font asked to shape a whole cell-cluster (a covered base plus a codepoint the primary lacked) + // reports a proportional run's advances -- e.g. ~2 cells for a two-codepoint cell -- which + // overshoot the grid and drew the next cell's glyph a cell too far, a "ghost" glyph beside the + // real one whose position shifted with the surrounding SGR segmentation. Clusters count cells + // appended, monotone non-decreasing within a group, so `cluster - baseCluster` is the cell offset + // of every glyph from the group's origin; a ligature that merged N cells leaves an N-wide gap to + // the next cluster, which is exactly its width. (TextClusterGrouper's east-asian-width fixme is + // orthogonal: clusters count cells, and a wide cell's second column is a separate space cell that + // ends the group, so no glyph is ever stepped across it here.) + // + // A shaper that leaves the cluster field unpopulated opts out: every glyph then reports the group's + // base cluster, which a multi-glyph run reveals as a back cluster no greater than the front. Such a + // run keeps the legacy advance-based stepping so it is not collapsed onto one cell -- the guard is a + // capability check, not a heuristic on the text. + auto const baseCluster = glyphPositions.empty() ? 0u : glyphPositions.front().cluster; + auto const clustersPopulated = + glyphPositions.size() < 2 || glyphPositions.back().cluster > baseCluster; + + // Legacy pen for a shaper that did not fill in clusters (see the capability check above): it steps by + // the shaping font's advance, snapped DOWN to whole cells. A bold/fallback face reports an advance a + // little over one cell (~1.3), and nearest-rounding would push some to two, producing ragged + // "Ke rnel" spacing; a real N-cell glyph reports an exact N-cell multiple, so flooring keeps it at N. + // A non-zero advance always steps at least one cell. Unused while clusters are populated. + auto legacyPen = pen; + auto const stepLegacyPen = [&](text::GlyphPosition const& gp) noexcept { + legacyPen.x += (gp.advance.x == 0 ? 0 : std::max(1, gp.advance.x / std::max(1, cellWidth))) + * cellWidth * advanceScale; + }; + + // Where this glyph's cell begins: its cluster offset from the group origin when the shaper gave us + // clusters, otherwise wherever the accumulated advances have reached. + auto const cellPenFor = [&](text::GlyphPosition const& gp) noexcept { + if (!clustersPopulated) + return legacyPen; + auto cellPen = pen; + cellPen.x += static_cast(gp.cluster - baseCluster) * cellWidth * advanceScale; + return cellPen; + }; + for (auto const& glyphPosition: glyphPositions) { + auto const cellPen = cellPenFor(glyphPosition); + if (!clustersPopulated) + stepLegacyPen(glyphPosition); + if (auto const* attributes = ensureRasterizedIfDirectMapped(glyphPosition.glyph)) { auto const attributesCopy = adjustTileAttributesForLineFlags(lineFlags, *attributes); - auto pen1 = applyGlyphPositionToPen(pen, attributesCopy, glyphPosition); + auto pen1 = applyGlyphPositionToPen(cellPen, attributesCopy, glyphPosition); pen1 = adjustPenForLineFlags(lineFlags, _gridMetrics, unbox(attributes->bitmapSize.height), attributes->metadata.y.value, pen1); renderRasterizedGlyph(pen1, color, attributesCopy); - - // Direct mapping only ever covers printable US-ASCII of the primary font, which occupies - // exactly one cell. The advance is known, so the font is not consulted for it. - pen.x += cellWidth * advanceScale; continue; } @@ -685,7 +728,7 @@ void TextRenderer::renderTextGroup(std::u32string_view codepoints, if (attributes) { auto const attributesCopy = adjustTileAttributesForLineFlags(lineFlags, *attributes); - auto pen1 = applyGlyphPositionToPen(pen, attributesCopy, glyphPosition); + auto pen1 = applyGlyphPositionToPen(cellPen, attributesCopy, glyphPosition); pen1 = adjustPenForLineFlags(lineFlags, _gridMetrics, unbox(attributes->bitmapSize.height), @@ -711,23 +754,6 @@ void TextRenderer::renderTextGroup(std::u32string_view codepoints, sliceKey += unbox(textureAtlas().tileSize().width); } } - - // TODO: The font's advance is a stand-in for the datum the pipeline actually has and then drops: - // the glyph's cluster, i.e. which cell it belongs to. Carrying the cluster on GlyphPosition would - // let the pen step by the exact cell delta -- zero for a combining mark, N for a ligature spanning - // N cells -- with no rounding at all. That awaits TextClusterGrouper's east-asian-width fixme, - // since clusters presently count cells appended rather than columns occupied. - // - // Until then, snap the advance DOWN to whole cells rather than to the nearest: a bold/fallback - // face whose glyphs are not perfectly cell-matched reports an advance a little over one cell - // (e.g. ~1.3), and nearest-rounding pushes some of those to two cells, producing ragged - // "Ke rnel"/"Di splay" spacing. A real N-cell glyph (wide CJK, ligature) reports an advance at - // an exact N-cell multiple, so flooring keeps it at N; only the sub-multiple overshoot of a - // mismatched face is trimmed. A non-zero advance always steps at least one cell. - auto const advCells = glyphPosition.advance.x == 0 - ? 0 - : std::max(1, glyphPosition.advance.x / std::max(1, cellWidth)); - pen.x += advCells * cellWidth * advanceScale; } } diff --git a/src/vtrasterizer/TextRenderer_test.cpp b/src/vtrasterizer/TextRenderer_test.cpp index 5aaadcba..99db79e6 100644 --- a/src/vtrasterizer/TextRenderer_test.cpp +++ b/src/vtrasterizer/TextRenderer_test.cpp @@ -1846,6 +1846,23 @@ TEST_CASE("TextRenderer.fallback_run_stays_on_the_cell_grid", "[renderer][fallba CHECK(positions[0] == 0); CHECK(positions[1] == 1 * CellWidth); } + + SECTION("a covered base sharing its cell with a missing codepoint does not ghost") + { + // The ghost-glyph case. One cell holds 'A' (which the primary covers) and the ornament (which it + // does not); because a cluster is indivisible, the whole cell is re-shaped by the proportional + // fallback, and HarfBuzz reports the pair's combined width (A@7 + ornament@12 = 19px, ~2 cells) + // against a one-cell cluster. Stepping the pen by that advance carried the ornament off 'A' and + // onto the next cell, drawing a phantom glyph over 'B'. Stepping by the cluster instead keeps both + // glyphs of the cell on the cell's own column, so 'B' stands alone in the next one. + auto const positions = + renderedGlyphPositions(renderer, renderTarget, { std::u32string { U'A', Ornament }, U"B" }); + + REQUIRE(positions.size() == 3); + CHECK(positions[0] == 0); // 'A' on cell 0 + CHECK(positions[1] == 0); // the ornament shares cell 0, drawn over 'A', not on cell 1 + CHECK(positions[2] == 1 * CellWidth); // 'B' stands alone on cell 1, with no ghost over it + } } TEST_CASE("TextRenderer.a_scaled_block_draws_one_cell_sized_tile_per_band", "[renderer][textsizing]")