fix: Use font metrics for combining mark positioning (#1310)

## Summary

**What is the goal of this PR?** (e.g., Implements the new feature for
file uploading.)

Combining diacritical marks (U+0300–U+036F) were positioned using a
heuristic that centered them at the midpoint of the base glyph's
**advance width**. This worked acceptably for Bookerly but produced
visibly off-center marks for Noto Sans due to a fundamental difference
in how the two fonts design their combining mark metrics.

Builds on the work of #1037.

## The problem

The two built-in body fonts encode combining mark `left` offsets with
very different conventions:

| Mark | Bookerly `left` | Noto Sans `left` |
|---|---|---|
| U+0301 (acute) | -2 | -10 |
| U+0300 (grave) | -5 | -15 |
| U+0302 (circumflex) | -5 | -5 |
| U+0323 (dot below) | -2 | -11 |

Noto Sans uses large negative `left` values because its marks are
designed for placement at the post-advance cursor position, with `left`
pulling the bitmap back over the base glyph. Bookerly uses small offsets
because its marks sit closer to the glyph origin. The old `advance/2`
centering split the difference poorly — it happened to land close to
correct for Bookerly but placed Noto Sans marks roughly 6px left of
center on a typical lowercase letter.

There was also a bug in the vertical gap heuristic. It unconditionally
computed a `raiseBy` value to prevent above-baseline marks from
colliding with tall base glyphs, but it applied the same logic to
**below-baseline** marks like cedilla (U+0327), dot below (U+0323), and
ogonek (U+0328). For those marks, the math produced a large positive
raise (e.g., 24px for dot-below on 'a'), launching them above the
x-height instead of keeping them below the baseline.

## The fix

**Horizontal positioning**: Instead of centering at `advance/2`, align
the mark bitmap's visual midpoint directly over the base glyph bitmap's
visual midpoint. This uses the base glyph's actual `left` and `width`
rather than its advance width, producing correct results regardless of
how the font encodes its mark offsets.

**Vertical positioning**: The raise heuristic now checks `markTop -
markHeight > 0` and skips below-baseline marks entirely, leaving them at
their font-designed position.

**Consolidation**: The shared math is extracted into two `constexpr`
helpers (`combiningMark::centerOver` and
`combiningMark::raiseAboveBase`) in `EpdFontData.h`, eliminating the
previously triplicated inline calculations across `drawText`,
`drawTextRotated90CW`, and `getTextBounds`. The `MIN_COMBINING_GAP_PX`
constant is also centralized as `combiningMark::MIN_GAP_PX`.

| Before | After |
| -- | -- |
| <img
src="https://github.com/user-attachments/files/25752257/before-noto.bmp"
width="250" /> | <img
src="https://github.com/user-attachments/files/25752258/after-noto.bmp"
width="250" /> |
| <img
src="https://github.com/user-attachments/files/25752259/before-bookerly.bmp"
width="250" /> | <img
src="https://github.com/user-attachments/files/25752260/after-bookerly.bmp"
width="250" /> |

---

### AI Usage

While CrossPoint doesn't have restrictions on AI tools in contributing,
please be transparent about their usage as it
helps set the right context for reviewers.

Did you use AI tools to help write this code? _**YES to analyze
differences between Noto Sans and Bookerly font metrics**_

---------

Co-authored-by: Uri Tauber <142022451+Uri-Tauber@users.noreply.github.com>
This commit is contained in:
Zach Nelson
2026-04-14 14:35:50 -05:00
committed by GitHub
co-authored by Uri Tauber
parent 243ae8b408
commit 075ad7d021
3 changed files with 69 additions and 43 deletions
+19 -28
View File
@@ -216,7 +216,8 @@ void GfxRenderer::drawText(const int fontId, const int x, const int y, const cha
const EpdFontFamily::Style style) const {
const int yPos = y + getFontAscenderSize(fontId);
int lastBaseX = x;
int lastBaseAdvanceFP = 0; // 12.4 fixed-point
int lastBaseLeft = 0;
int lastBaseWidth = 0;
int lastBaseTop = 0;
int32_t prevAdvanceFP = 0; // 12.4 fixed-point: prev glyph's advance + next kern for snap
@@ -236,24 +237,17 @@ void GfxRenderer::drawText(const int fontId, const int x, const int y, const cha
return;
}
const auto& font = fontIt->second;
constexpr int MIN_COMBINING_GAP_PX = 1;
uint32_t cp;
uint32_t prevCp = 0;
while ((cp = utf8NextCodepoint(reinterpret_cast<const uint8_t**>(&text)))) {
if (utf8IsCombiningMark(cp)) {
const EpdGlyph* combiningGlyph = font.getGlyph(cp, style);
int raiseBy = 0;
if (combiningGlyph) {
const int currentGap = combiningGlyph->top - combiningGlyph->height - lastBaseTop;
if (currentGap < MIN_COMBINING_GAP_PX) {
raiseBy = MIN_COMBINING_GAP_PX - currentGap;
}
}
const int combiningX = lastBaseX + fp4::toPixel(lastBaseAdvanceFP / 2);
const int combiningY = yPos - raiseBy;
renderCharImpl<TextRotation::None>(*this, renderMode, font, cp, combiningX, combiningY, black, style);
if (!combiningGlyph) continue;
const int raiseBy = combiningMark::raiseAboveBase(combiningGlyph->top, combiningGlyph->height, lastBaseTop);
const int combiningX = combiningMark::centerOver(lastBaseX, lastBaseLeft, lastBaseWidth, combiningGlyph->left,
combiningGlyph->width);
renderCharImpl<TextRotation::None>(*this, renderMode, font, cp, combiningX, yPos - raiseBy, black, style);
continue;
}
@@ -269,9 +263,10 @@ void GfxRenderer::drawText(const int fontId, const int x, const int y, const cha
const EpdGlyph* glyph = font.getGlyph(cp, style);
lastBaseAdvanceFP = glyph ? glyph->advanceX : 0;
lastBaseLeft = glyph ? glyph->left : 0;
lastBaseWidth = glyph ? glyph->width : 0;
lastBaseTop = glyph ? glyph->top : 0;
prevAdvanceFP = lastBaseAdvanceFP;
prevAdvanceFP = glyph ? glyph->advanceX : 0; // 12.4 fixed-point
renderCharImpl<TextRotation::None>(*this, renderMode, font, cp, lastBaseX, yPos, black, style);
prevCp = cp;
@@ -1079,26 +1074,21 @@ void GfxRenderer::drawTextRotated90CW(const int fontId, const int x, const int y
const auto& font = fontIt->second;
int lastBaseY = y;
int lastBaseAdvanceFP = 0; // 12.4 fixed-point
int lastBaseLeft = 0;
int lastBaseWidth = 0;
int lastBaseTop = 0;
int32_t prevAdvanceFP = 0; // 12.4 fixed-point: prev glyph's advance + next kern for snap
constexpr int MIN_COMBINING_GAP_PX = 1;
uint32_t cp;
uint32_t prevCp = 0;
while ((cp = utf8NextCodepoint(reinterpret_cast<const uint8_t**>(&text)))) {
if (utf8IsCombiningMark(cp)) {
const EpdGlyph* combiningGlyph = font.getGlyph(cp, style);
int raiseBy = 0;
if (combiningGlyph) {
const int currentGap = combiningGlyph->top - combiningGlyph->height - lastBaseTop;
if (currentGap < MIN_COMBINING_GAP_PX) {
raiseBy = MIN_COMBINING_GAP_PX - currentGap;
}
}
if (!combiningGlyph) continue;
const int raiseBy = combiningMark::raiseAboveBase(combiningGlyph->top, combiningGlyph->height, lastBaseTop);
const int combiningX = x - raiseBy;
const int combiningY = lastBaseY - fp4::toPixel(lastBaseAdvanceFP / 2);
const int combiningY = combiningMark::centerOverRotated90CW(lastBaseY, lastBaseLeft, lastBaseWidth,
combiningGlyph->left, combiningGlyph->width);
renderCharImpl<TextRotation::Rotated90CW>(*this, renderMode, font, cp, combiningX, combiningY, black, style);
continue;
}
@@ -1114,9 +1104,10 @@ void GfxRenderer::drawTextRotated90CW(const int fontId, const int x, const int y
const EpdGlyph* glyph = font.getGlyph(cp, style);
lastBaseAdvanceFP = glyph ? glyph->advanceX : 0; // 12.4 fixed-point
lastBaseLeft = glyph ? glyph->left : 0;
lastBaseWidth = glyph ? glyph->width : 0;
lastBaseTop = glyph ? glyph->top : 0;
prevAdvanceFP = lastBaseAdvanceFP;
prevAdvanceFP = glyph ? glyph->advanceX : 0; // 12.4 fixed-point
renderCharImpl<TextRotation::Rotated90CW>(*this, renderMode, font, cp, x, lastBaseY, black, style);
prevCp = cp;