Files
Zach Nelson 075ad7d021 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>
2026-04-14 14:35:50 -05:00

179 lines
5.6 KiB
C++

#include "EpdFont.h"
#include <Utf8.h>
#include <algorithm>
void EpdFont::getTextBounds(const char* string, const int startX, const int startY, int* minX, int* minY, int* maxX,
int* maxY) const {
*minX = startX;
*minY = startY;
*maxX = startX;
*maxY = startY;
if (*string == '\0') {
return;
}
int lastBaseX = startX;
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
uint32_t cp;
uint32_t prevCp = 0;
while ((cp = utf8NextCodepoint(reinterpret_cast<const uint8_t**>(&string)))) {
const bool isCombining = utf8IsCombiningMark(cp);
if (!isCombining) {
cp = applyLigatures(cp, string);
}
const EpdGlyph* glyph = getGlyph(cp);
if (!glyph) {
// Keep cursor movement stable when a base glyph is missing, but don't attach subsequent
// combining marks to stale base metrics.
if (!isCombining) {
lastBaseX += fp4::toPixel(prevAdvanceFP); // flush pending advance before resetting
prevCp = 0;
prevAdvanceFP = 0;
lastBaseLeft = 0;
lastBaseWidth = 0;
lastBaseTop = 0;
}
continue;
}
const int raiseBy = isCombining ? combiningMark::raiseAboveBase(glyph->top, glyph->height, lastBaseTop) : 0;
if (!isCombining && prevCp != 0) {
const auto kernFP = getKerning(prevCp, cp); // 4.4 fixed-point kern
lastBaseX += fp4::toPixel(prevAdvanceFP + kernFP);
}
const int glyphBaseX =
isCombining ? combiningMark::centerOver(lastBaseX, lastBaseLeft, lastBaseWidth, glyph->left, glyph->width)
: lastBaseX;
const int glyphBaseY = startY - raiseBy;
*minX = std::min(*minX, glyphBaseX + glyph->left);
*maxX = std::max(*maxX, glyphBaseX + glyph->left + glyph->width);
*minY = std::min(*minY, glyphBaseY + glyph->top - glyph->height);
*maxY = std::max(*maxY, glyphBaseY + glyph->top);
if (!isCombining) {
lastBaseLeft = glyph->left;
lastBaseWidth = glyph->width;
lastBaseTop = glyph->top;
prevAdvanceFP = glyph->advanceX; // 12.4 fixed-point
prevCp = cp;
}
}
}
void EpdFont::getTextDimensions(const char* string, int* w, int* h) const {
int minX = 0, minY = 0, maxX = 0, maxY = 0;
getTextBounds(string, 0, 0, &minX, &minY, &maxX, &maxY);
*w = maxX - minX;
*h = maxY - minY;
}
static uint8_t lookupKernClass(const EpdKernClassEntry* entries, const uint16_t count, const uint32_t cp) {
if (!entries || count == 0 || cp > 0xFFFF) {
return 0;
}
const auto target = static_cast<uint16_t>(cp);
const auto* end = entries + count;
// lower_bound: exact-key lookup. Finds the first entry with codepoint >= target,
// then the equality check confirms an exact match exists.
const auto it = std::lower_bound(
entries, end, target, [](const EpdKernClassEntry& entry, uint16_t value) { return entry.codepoint < value; });
if (it != end && it->codepoint == target) {
return it->classId;
}
return 0;
}
int8_t EpdFont::getKerning(const uint32_t leftCp, const uint32_t rightCp) const {
if (!data->kernMatrix) {
return 0;
}
const uint8_t lc = lookupKernClass(data->kernLeftClasses, data->kernLeftEntryCount, leftCp);
if (lc == 0) return 0;
const uint8_t rc = lookupKernClass(data->kernRightClasses, data->kernRightEntryCount, rightCp);
if (rc == 0) return 0;
return data->kernMatrix[(lc - 1) * data->kernRightClassCount + (rc - 1)];
}
uint32_t EpdFont::getLigature(const uint32_t leftCp, const uint32_t rightCp) const {
const auto* pairs = data->ligaturePairs;
const auto count = data->ligaturePairCount;
if (!pairs || count == 0 || leftCp > 0xFFFF || rightCp > 0xFFFF) {
return 0;
}
const uint32_t key = (leftCp << 16) | rightCp;
const auto* end = pairs + count;
// lower_bound: exact-key lookup. Finds the first entry with pair >= key,
// then the equality check confirms an exact match exists.
const auto it =
std::lower_bound(pairs, end, key, [](const EpdLigaturePair& pair, uint32_t value) { return pair.pair < value; });
if (it != end && it->pair == key) {
return it->ligatureCp;
}
return 0;
}
uint32_t EpdFont::applyLigatures(uint32_t cp, const char*& text) const {
if (!data->ligaturePairs || data->ligaturePairCount == 0) {
return cp;
}
while (true) {
const auto saved = reinterpret_cast<const uint8_t*>(text);
const uint32_t nextCp = utf8NextCodepoint(reinterpret_cast<const uint8_t**>(&text));
if (nextCp == 0) break;
const uint32_t lig = getLigature(cp, nextCp);
if (lig == 0) {
text = reinterpret_cast<const char*>(saved);
break;
}
cp = lig;
}
return cp;
}
const EpdGlyph* EpdFont::getGlyph(const uint32_t cp) const {
const int count = data->intervalCount;
if (count == 0) return nullptr;
const EpdUnicodeInterval* intervals = data->intervals;
const auto* end = intervals + count;
// upper_bound: range lookup. Finds the first interval with first > cp, so the
// interval just before it is the last one with first <= cp. That's the only
// candidate that could contain cp. Then we verify cp <= candidate.last.
const auto it = std::upper_bound(
intervals, end, cp, [](uint32_t value, const EpdUnicodeInterval& interval) { return value < interval.first; });
if (it != intervals) {
const auto& interval = *(it - 1);
if (cp <= interval.last) {
return &data->glyph[interval.offset + (cp - interval.first)];
}
}
if (cp != REPLACEMENT_GLYPH) {
return getGlyph(REPLACEMENT_GLYPH);
}
return nullptr;
}