fix: Use fixed-point fractional x-advance and kerning for better text layout#1168
fix: Use fixed-point fractional x-advance and kerning for better text layout#1168znelson merged 8 commits intocrosspoint-reader:masterfrom
Conversation
📝 WalkthroughWalkthroughSwitches horizontal text layout to 12.4 fixed‑point: font conversion, font data, EpdFont core, and GfxRenderer now store/compute advances and kerning in fixed‑point (12.4 advances, 4.4 kerning). Public renderChar wrapper removed. Builtin fonts regenerated to use the new fixed‑point metrics. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Noticed that too |
znelson
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/EpdFont/scripts/fontconvert.py (1)
389-395:⚠️ Potential issue | 🟡 MinorStale docstring: return values are now 4.4 fixed-point, not pixel integers.
The function docstring still says
pixel_adjuston line 393, but the return values are now 4.4 fixed-point (as correctly noted in the inline comment on line 440). Consider updating the docstring to avoid confusion for future maintainers.📝 Suggested docstring fix
def extract_kerning_fonttools(font_path, codepoints, ppem): """Extract kerning pairs from a font file using fonttools. - Returns dict of {(leftCp, rightCp): pixel_adjust} for the given - codepoints. Values are scaled from font design units to integer - pixels at ppem. + Returns dict of {(leftCp, rightCp): fp4_adjust} for the given + codepoints. Values are scaled from font design units to 4.4 + fixed-point pixels at ppem. """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/EpdFont/scripts/fontconvert.py` around lines 389 - 395, The docstring for extract_kerning_fonttools is stale: it claims the return dict maps to "pixel_adjust" integer values but the implementation returns values in 4.4 fixed-point (as noted in the inline comment near the kerning computation). Update the docstring to describe the return type as dict of {(leftCp, rightCp): fixed_point_adjust} and explicitly state the values are 4.4 fixed-point (i.e., 4 integer bits + 4 fractional bits) rather than integer pixel adjustments so future maintainers understand the scaling.
🧹 Nitpick comments (1)
lib/GfxRenderer/GfxRenderer.cpp (1)
201-219: Use style-aware ascender and move early-return before font work.
drawTextcurrently computesyPosfromgetFontAscenderSize(fontId)(regular style) before the null/empty guard and before style lookup. This can cause unnecessary lookup/log noise and style baseline mismatch.♻️ Proposed refactor
void GfxRenderer::drawText(const int fontId, const int x, const int y, const char* text, const bool black, const EpdFontFamily::Style style) const { - const int yPos = y + getFontAscenderSize(fontId); - int32_t xPosFP = fp4::fromPixel(x); // 12.4 fixed-point accumulator + // cannot draw a NULL / empty string + if (text == nullptr || *text == '\0') { + return; + } + + int32_t xPosFP = fp4::fromPixel(x); // 12.4 fixed-point accumulator int lastBaseX = x; int lastBaseAdvanceFP = 0; // 12.4 fixed-point int lastBaseTop = 0; - - // cannot draw a NULL / empty string - if (text == nullptr || *text == '\0') { - return; - } const auto fontIt = fontMap.find(fontId); if (fontIt == fontMap.end()) { LOG_ERR("GFX", "Font %d not found", fontId); return; } const auto& font = fontIt->second; + const int yPos = y + font.getData(style)->ascender;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/GfxRenderer/GfxRenderer.cpp` around lines 201 - 219, Move the null/empty string guard and avoid computing style-independent values before validating the font: in GfxRenderer::drawText, perform the "if (text == nullptr || *text == '\0')" early and return immediately, then locate the font via fontMap and determine the style-aware ascender (use the style-aware API instead of calling getFontAscenderSize(fontId) too early) and only then compute yPos; also shift the creation of xPosFP / lastBaseX / lastBaseAdvanceFP initialization to after the font/style lookup so you don't do unnecessary work or produce spurious LOG_ERR noise when fontId is invalid (refer to drawText, fontMap, fontIt, font, and getFontAscenderSize).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/EpdFont/builtinFonts/opendyslexic_10_bold.h`:
- Line 2553: The advance width for glyph U+205F in the builtin font metrics
array is wrong (entry currently shows { 0, 0, 1553, 0, 0, 0, 1963 } in
opendyslexic_10_bold.h) and must be reduced to match neighboring space glyphs;
update the third field (advance) for U+205F to a value consistent with
U+2000–U+200A (e.g., scale down by ~10× into the same fp4 range such as ~155 fp4
or another value matching the font’s space metrics), leaving the other tuple
fields unchanged.
In `@lib/EpdFont/EpdFont.cpp`:
- Line 51: The calculation for glyphBaseX rounds lastBaseAdvanceFP before
halving, causing 1px jitter for combining marks; instead divide the fixed-point
advance by 2 first and then convert to pixels so fractional precision is
preserved. Update the isCombining branch that computes glyphBaseX (the
expression using lastBaseX, lastBaseAdvanceFP, fp4::toPixel, cursorXPixels) to
compute the half-advance in fixed-point (e.g., lastBaseAdvanceFP / 2) and pass
that to fp4::toPixel (or otherwise convert after halving) so centering uses the
true half-advance rather than a rounded half of pixels. Ensure other symbols
(isCombining, lastBaseX, lastBaseAdvanceFP, cursorXPixels, glyphBaseX) remain
unchanged.
---
Outside diff comments:
In `@lib/EpdFont/scripts/fontconvert.py`:
- Around line 389-395: The docstring for extract_kerning_fonttools is stale: it
claims the return dict maps to "pixel_adjust" integer values but the
implementation returns values in 4.4 fixed-point (as noted in the inline comment
near the kerning computation). Update the docstring to describe the return type
as dict of {(leftCp, rightCp): fixed_point_adjust} and explicitly state the
values are 4.4 fixed-point (i.e., 4 integer bits + 4 fractional bits) rather
than integer pixel adjustments so future maintainers understand the scaling.
---
Nitpick comments:
In `@lib/GfxRenderer/GfxRenderer.cpp`:
- Around line 201-219: Move the null/empty string guard and avoid computing
style-independent values before validating the font: in GfxRenderer::drawText,
perform the "if (text == nullptr || *text == '\0')" early and return
immediately, then locate the font via fontMap and determine the style-aware
ascender (use the style-aware API instead of calling getFontAscenderSize(fontId)
too early) and only then compute yPos; also shift the creation of xPosFP /
lastBaseX / lastBaseAdvanceFP initialization to after the font/style lookup so
you don't do unnecessary work or produce spurious LOG_ERR noise when fontId is
invalid (refer to drawText, fontMap, fontIt, font, and getFontAscenderSize).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (59)
lib/EpdFont/EpdFont.cpplib/EpdFont/EpdFont.hlib/EpdFont/EpdFontData.hlib/EpdFont/builtinFonts/bookerly_12_bold.hlib/EpdFont/builtinFonts/bookerly_12_bolditalic.hlib/EpdFont/builtinFonts/bookerly_12_italic.hlib/EpdFont/builtinFonts/bookerly_12_regular.hlib/EpdFont/builtinFonts/bookerly_14_bold.hlib/EpdFont/builtinFonts/bookerly_14_bolditalic.hlib/EpdFont/builtinFonts/bookerly_14_italic.hlib/EpdFont/builtinFonts/bookerly_14_regular.hlib/EpdFont/builtinFonts/bookerly_16_bold.hlib/EpdFont/builtinFonts/bookerly_16_bolditalic.hlib/EpdFont/builtinFonts/bookerly_16_italic.hlib/EpdFont/builtinFonts/bookerly_16_regular.hlib/EpdFont/builtinFonts/bookerly_18_bold.hlib/EpdFont/builtinFonts/bookerly_18_bolditalic.hlib/EpdFont/builtinFonts/bookerly_18_italic.hlib/EpdFont/builtinFonts/bookerly_18_regular.hlib/EpdFont/builtinFonts/notosans_12_bold.hlib/EpdFont/builtinFonts/notosans_12_bolditalic.hlib/EpdFont/builtinFonts/notosans_12_italic.hlib/EpdFont/builtinFonts/notosans_12_regular.hlib/EpdFont/builtinFonts/notosans_14_bold.hlib/EpdFont/builtinFonts/notosans_14_bolditalic.hlib/EpdFont/builtinFonts/notosans_14_italic.hlib/EpdFont/builtinFonts/notosans_14_regular.hlib/EpdFont/builtinFonts/notosans_16_bold.hlib/EpdFont/builtinFonts/notosans_16_bolditalic.hlib/EpdFont/builtinFonts/notosans_16_italic.hlib/EpdFont/builtinFonts/notosans_16_regular.hlib/EpdFont/builtinFonts/notosans_18_bold.hlib/EpdFont/builtinFonts/notosans_18_bolditalic.hlib/EpdFont/builtinFonts/notosans_18_italic.hlib/EpdFont/builtinFonts/notosans_18_regular.hlib/EpdFont/builtinFonts/notosans_8_regular.hlib/EpdFont/builtinFonts/opendyslexic_10_bold.hlib/EpdFont/builtinFonts/opendyslexic_10_bolditalic.hlib/EpdFont/builtinFonts/opendyslexic_10_italic.hlib/EpdFont/builtinFonts/opendyslexic_10_regular.hlib/EpdFont/builtinFonts/opendyslexic_12_bold.hlib/EpdFont/builtinFonts/opendyslexic_12_bolditalic.hlib/EpdFont/builtinFonts/opendyslexic_12_italic.hlib/EpdFont/builtinFonts/opendyslexic_12_regular.hlib/EpdFont/builtinFonts/opendyslexic_14_bold.hlib/EpdFont/builtinFonts/opendyslexic_14_bolditalic.hlib/EpdFont/builtinFonts/opendyslexic_14_italic.hlib/EpdFont/builtinFonts/opendyslexic_14_regular.hlib/EpdFont/builtinFonts/opendyslexic_8_bold.hlib/EpdFont/builtinFonts/opendyslexic_8_bolditalic.hlib/EpdFont/builtinFonts/opendyslexic_8_italic.hlib/EpdFont/builtinFonts/opendyslexic_8_regular.hlib/EpdFont/builtinFonts/ubuntu_10_bold.hlib/EpdFont/builtinFonts/ubuntu_10_regular.hlib/EpdFont/builtinFonts/ubuntu_12_bold.hlib/EpdFont/builtinFonts/ubuntu_12_regular.hlib/EpdFont/scripts/fontconvert.pylib/GfxRenderer/GfxRenderer.cpplib/GfxRenderer/GfxRenderer.h
💤 Files with no reviewable changes (1)
- lib/GfxRenderer/GfxRenderer.h
👮 Files not reviewed due to content moderation or server errors (2)
- lib/EpdFont/builtinFonts/opendyslexic_14_regular.h
- lib/EpdFont/builtinFonts/opendyslexic_14_bold.h
📜 Review details
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-02-19T12:17:05.421Z
Learnt from: Uri-Tauber
Repo: crosspoint-reader/crosspoint-reader PR: 988
File: lib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpp:649-661
Timestamp: 2026-02-19T12:17:05.421Z
Learning: In ChapterHtmlSlimParser.cpp, when computing footnote word indices in endElement() for footnote links, the wordIndex must be cumulative across the 750-word mid-paragraph flush boundary. The correct calculation is: `wordIndex = wordsExtractedInBlock + currentTextBlock->size()`, not just `currentTextBlock->size()`. This ensures footnotes attach to the page containing their actual anchor word, even after layoutAndExtractLines(false) has extracted and removed earlier words from the block.
Applied to files:
lib/EpdFont/EpdFont.cpp
📚 Learning: 2026-02-23T19:55:03.054Z
Learnt from: martinbrook
Repo: crosspoint-reader/crosspoint-reader PR: 1136
File: scripts/patch_jpegdec.py:20-23
Timestamp: 2026-02-23T19:55:03.054Z
Learning: In the crosspoint-reader project, enable Python linters (e.g., Ruff or Flake8) in CI for all Python scripts. Run linting as part of the CI workflow and fix reported issues to maintain code quality. Do not skip linting for any Python files.
Applied to files:
lib/EpdFont/scripts/fontconvert.py
🧬 Code graph analysis (1)
lib/GfxRenderer/GfxRenderer.cpp (5)
lib/EpdFont/EpdFontData.h (2)
fromPixel(22-22)toPixel(25-25)lib/EpdFont/EpdFont.cpp (2)
getKerning(100-109)getKerning(100-100)lib/EpdFont/EpdFontFamily.cpp (2)
getKerning(31-33)getKerning(31-31)lib/Utf8/Utf8.cpp (2)
utf8NextCodepoint(11-31)utf8NextCodepoint(11-11)lib/Utf8/Utf8.h (1)
utf8IsCombiningMark(14-19)
🔇 Additional comments (19)
lib/EpdFont/EpdFontData.h (2)
17-29: Well-structured fp4 namespace with clear documentation.The fixed-point helpers are clean, constexpr, and the documentation linking 12.4 advances with 4.4 kerns sharing 4 fractional bits is helpful.
One minor note:
toPixeluses(fp + HALF) >> FRAC_BITSwhich performs arithmetic right-shift on negative values. This is implementation-defined in C++ prior to C++20. On ESP32 (and virtually all modern targets) this is arithmetic shift, so it works correctly — but worth being aware of if portability to unusual platforms ever matters.
36-45:advanceXwidened touint16_t— appropriate for 12.4 fixed-point.This gives a max advance of ~4096 pixels, more than sufficient. The struct field ordering preserves natural alignment (uint16_t after two uint8_t fields fills the same 4-byte word), so there's no size regression.
lib/EpdFont/scripts/fontconvert.py (3)
154-156: Conversion from FreeType 16.16 to 12.4 is correct.
(val + (1 << 11)) >> 12correctly rounds to nearest when shifting right by 12 bits. The result is unsigned in practice sincelinearHoriAdvanceis always non-negative for glyph advances.
158-165:fp4_from_design_unitsis clean and correctly clamped.The clamping to
[-128, 127]ensures the result fits inint8_tfor the kern matrix. Good.
297-308:linearHoriAdvanceis available and correctly used in freetype-py-2.5.1.The project pins freetype-py==2.5.1 in requirements.txt, which exposes
linearHoriAdvanceas an attribute ofGlyphSlot. The implementation at line 302 correctly accesses this field to retrieve the unhinted 16.16 fixed-point advance, which is the right approach for sub-pixel-accurate layout accumulation.lib/EpdFont/builtinFonts/opendyslexic_8_bold.h (2)
1351-1446: Regenerated glyph metrics look consistent with 12.4 fixed-point encoding.Spot-checking a few values:
- Space (U+0020):
advanceX = 232→ 232/16 = 14.5 pixels ✓- 'A':
advanceX = 260→ 16.25 pixels ✓- 'a':
advanceX = 228→ 14.25 pixels ✓The fractional precision is the core improvement of this PR.
2398-2400: Kern matrix value 43 confirmed for punctuation-space pair.The kern matrix structure maps correctly:
- Left class 1: punctuation/symbols (U+203A–U+208E and others)
- Right class 1: space (U+0020)
- Value: 43 = 2.6875 pixels (43/16) — arithmetic is correct
The conversion math and class mappings are accurate. However, verification against the source OpenDyslexic font's actual kern data is required to confirm this represents the intended kerning behavior.
lib/EpdFont/builtinFonts/opendyslexic_10_bold.h (2)
1747-2490: Bulk glyph metric regeneration looks consistent with fixed-point data.The updated advances/offsets and regrouped bitmap spans look coherent for the 12.4 migration in this generated font table.
2794-2794: Kerning matrix value update looks valid for fp4 storage.
53fitsint8_tand is aligned with the fixed-point kerning migration.lib/EpdFont/builtinFonts/opendyslexic_12_regular.h (3)
1-7: Generator provenance is preserved and clear.Keeping the source command embedded here is great for reproducibility and auditing generated font artifacts.
2016-2899: Regenerated glyph table is structurally consistent for fixed-point metrics.The updated glyph entries keep a valid, consistent shape and align with the PR’s fixed-point advance migration.
3063-3063: Kerning matrix scaling update looks correct.Using
64here matches the expected fixed-point kerning representation introduced by this PR.lib/EpdFont/builtinFonts/opendyslexic_12_bold.h (1)
2365-3250: Regenerated fixed-point font data looks internally consistent.The glyph metric table and kerning/class metadata are coherent with the fixed-point transition.
Also applies to: 3412-3433
lib/EpdFont/EpdFont.h (1)
15-16: Kerning return-unit documentation update is correct.The comment now clearly documents the 4.4 fixed-point contract for kerning values.
lib/EpdFont/EpdFont.cpp (1)
18-20: 12.4 cursor accumulation and kerning application are wired correctly.The fixed-point cursor flow and carry-forward logic are consistent with the new metric format.
Also applies to: 47-50, 59-64
lib/EpdFont/builtinFonts/opendyslexic_10_regular.h (2)
1669-2424: Generated glyph metric migration looks consistent.The glyph descriptor updates are structurally consistent and align with the fixed-point metric regeneration in this PR.
2715-2717: Kerning matrix update is coherent with fractional kerning encoding.The updated matrix entry is type-safe for
int8_tand fits the PR’s fixed-point kerning direction.lib/EpdFont/builtinFonts/opendyslexic_8_regular.h (1)
1248-2319: Generated font data update looks consistent with the fixed-point migration.This is a data-only regeneration, and the updated glyph/kerning payload remains structurally aligned with the font data schema.
lib/GfxRenderer/GfxRenderer.cpp (1)
242-254: Fixed-point accumulation + deferred pixel snap is implemented well here.The fp4 accumulator usage for kerning/advance in both draw and measurement paths is consistent and avoids per-glyph drift from repeated integer truncation.
Also applies to: 972-987, 1033-1072
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/EpdFont/scripts/convert-builtin-fonts.sh (1)
12-20: Optional: Quote variables for shell robustness.While the current variable values are controlled and won't contain spaces or special characters, quoting variables is a shell scripting best practice that prevents subtle bugs if values ever change.
🔧 Suggested improvement
for size in ${BOOKERLY_FONT_SIZES[@]}; do for style in ${READER_FONT_STYLES[@]}; do font_name="bookerly_${size}_$(echo $style | tr '[:upper:]' '[:lower:]')" font_path="../builtinFonts/source/Bookerly/Bookerly-${style}.ttf" output_path="../builtinFonts/${font_name}.h" - python fontconvert.py $font_name $size $font_path --2bit --compress > $output_path + python fontconvert.py "$font_name" "$size" "$font_path" --2bit --compress > "$output_path" echo "Generated $output_path" done doneThe same pattern could be applied to the other loops (lines 22-30, 32-40, 45-53, 55) for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/EpdFont/scripts/convert-builtin-fonts.sh` around lines 12 - 20, The loop uses unquoted shell variables which can break if values contain spaces or special chars; update the Bookerly generation loop to quote variable expansions (e.g., "${BOOKERLY_FONT_SIZES[@]}", "${READER_FONT_STYLES[@]}", "$font_name", "$size", "$font_path", "$output_path") and quote arguments to the python invocation (fontconvert.py "$font_name" "$size" "$font_path" --2bit --compress > "$output_path"); apply the same quoting pattern to the other font-generation loops and any other variable uses in this script to ensure robustness.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/EpdFont/scripts/convert-builtin-fonts.sh`:
- Around line 12-20: The loop uses unquoted shell variables which can break if
values contain spaces or special chars; update the Bookerly generation loop to
quote variable expansions (e.g., "${BOOKERLY_FONT_SIZES[@]}",
"${READER_FONT_STYLES[@]}", "$font_name", "$size", "$font_path", "$output_path")
and quote arguments to the python invocation (fontconvert.py "$font_name"
"$size" "$font_path" --2bit --compress > "$output_path"); apply the same quoting
pattern to the other font-generation loops and any other variable uses in this
script to ensure robustness.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
lib/EpdFont/builtinFonts/bookerly_12_bold.hlib/EpdFont/builtinFonts/bookerly_12_bolditalic.hlib/EpdFont/builtinFonts/bookerly_12_italic.hlib/EpdFont/builtinFonts/bookerly_12_regular.hlib/EpdFont/builtinFonts/bookerly_14_bold.hlib/EpdFont/builtinFonts/bookerly_14_bolditalic.hlib/EpdFont/builtinFonts/bookerly_14_italic.hlib/EpdFont/builtinFonts/bookerly_14_regular.hlib/EpdFont/builtinFonts/bookerly_16_bold.hlib/EpdFont/builtinFonts/bookerly_16_bolditalic.hlib/EpdFont/builtinFonts/bookerly_16_italic.hlib/EpdFont/builtinFonts/bookerly_16_regular.hlib/EpdFont/builtinFonts/bookerly_18_bold.hlib/EpdFont/builtinFonts/bookerly_18_bolditalic.hlib/EpdFont/builtinFonts/bookerly_18_italic.hlib/EpdFont/builtinFonts/bookerly_18_regular.hlib/EpdFont/scripts/convert-builtin-fonts.sh
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: cppcheck
- GitHub Check: build
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: znelson
Repo: crosspoint-reader/crosspoint-reader PR: 1168
File: lib/EpdFont/builtinFonts/opendyslexic_10_bold.h:2553-2553
Timestamp: 2026-02-26T05:59:54.187Z
Learning: fontconvert.py derives glyph advance from FreeType's linearHoriAdvance (16.16, unhinted). For OpenDyslexic, U+205F (MEDIUM MATHEMATICAL SPACE) returns ~97 px advance across sizes, which is an upstream font issue, not a code bug.
Learnt from: znelson
Repo: crosspoint-reader/crosspoint-reader PR: 1168
File: lib/EpdFont/builtinFonts/opendyslexic_10_bold.h:2553-2553
Timestamp: 2026-02-26T05:59:54.187Z
Learning: If we ever need to mitigate broken space widths from upstream fonts, the correct place is fontconvert.py (e.g., optional clamping or substituting normative em-fractions for specific Unicode spaces like U+205F), not the generated headers.
Learnt from: znelson
Repo: crosspoint-reader/crosspoint-reader PR: 1168
File: lib/EpdFont/builtinFonts/opendyslexic_10_bold.h:2553-2553
Timestamp: 2026-02-26T05:59:54.187Z
Learning: Generated font headers (lib/EpdFont/builtinFonts/*.h) are produced by convert-builtin-fonts.sh and must not be edited by hand; any changes will be overwritten.
📚 Learning: 2026-02-26T05:59:54.187Z
Learnt from: znelson
Repo: crosspoint-reader/crosspoint-reader PR: 1168
File: lib/EpdFont/builtinFonts/opendyslexic_10_bold.h:2553-2553
Timestamp: 2026-02-26T05:59:54.187Z
Learning: Generated font headers (lib/EpdFont/builtinFonts/*.h) are produced by convert-builtin-fonts.sh and must not be edited by hand; any changes will be overwritten.
Applied to files:
lib/EpdFont/scripts/convert-builtin-fonts.sh
📚 Learning: 2026-02-26T05:59:54.187Z
Learnt from: znelson
Repo: crosspoint-reader/crosspoint-reader PR: 1168
File: lib/EpdFont/builtinFonts/opendyslexic_10_bold.h:2553-2553
Timestamp: 2026-02-26T05:59:54.187Z
Learning: If we ever need to mitigate broken space widths from upstream fonts, the correct place is fontconvert.py (e.g., optional clamping or substituting normative em-fractions for specific Unicode spaces like U+205F), not the generated headers.
Applied to files:
lib/EpdFont/scripts/convert-builtin-fonts.sh
🔇 Additional comments (1)
lib/EpdFont/scripts/convert-builtin-fonts.sh (1)
17-17: Confirm intent: removal of--force-autohintfor Bookerly.The commit message mentions "a/b test: is Bookerly better without --force-autohint?" — please confirm this removal is the intended final state after testing, not a leftover from experimentation. This now aligns Bookerly with the other fonts (NotoSans, OpenDyslexic, Ubuntu) which also don't use
--force-autohint.If this is intentional, consider adding a brief comment in the script or PR description documenting why Bookerly no longer uses forced autohinting (e.g., better spacing metrics with the new fixed-point approach).
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/GfxRenderer/GfxRenderer.cpp (1)
242-243: Minor: Consider adding a brief comment on fixed-point format compatibility.The code adds 4.4 kerning to a 12.4 accumulator, which works correctly because both share 4 fractional bits. A brief inline note could help future maintainers understand this isn't a mistake:
// 4.4 kern adds directly to 12.4 accumulator (same fractional precision) xPosFP += kernFP;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/GfxRenderer/GfxRenderer.cpp` around lines 242 - 243, Add a one-line inline clarifying comment explaining that kernFP (from font.getKerning) is in 4.4 fixed-point and is safe to add directly to xPosFP (a 12.4 accumulator) because they share 4 fractional bits; update the code near the computation of kernFP and the line "xPosFP += kernFP;" referencing kernFP, xPosFP, font.getKerning(prevCp, cp, style) so future maintainers know the fixed-point formats are compatible.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/GfxRenderer/GfxRenderer.cpp`:
- Around line 242-243: Add a one-line inline clarifying comment explaining that
kernFP (from font.getKerning) is in 4.4 fixed-point and is safe to add directly
to xPosFP (a 12.4 accumulator) because they share 4 fractional bits; update the
code near the computation of kernFP and the line "xPosFP += kernFP;" referencing
kernFP, xPosFP, font.getKerning(prevCp, cp, style) so future maintainers know
the fixed-point formats are compatible.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/GfxRenderer/GfxRenderer.cpp
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: cppcheck
- GitHub Check: build
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: znelson
Repo: crosspoint-reader/crosspoint-reader PR: 1168
File: lib/EpdFont/builtinFonts/opendyslexic_10_bold.h:2553-2553
Timestamp: 2026-02-26T05:59:54.187Z
Learning: fontconvert.py derives glyph advance from FreeType's linearHoriAdvance (16.16, unhinted). For OpenDyslexic, U+205F (MEDIUM MATHEMATICAL SPACE) returns ~97 px advance across sizes, which is an upstream font issue, not a code bug.
Learnt from: znelson
Repo: crosspoint-reader/crosspoint-reader PR: 1168
File: lib/EpdFont/builtinFonts/opendyslexic_10_bold.h:2553-2553
Timestamp: 2026-02-26T05:59:54.187Z
Learning: If we ever need to mitigate broken space widths from upstream fonts, the correct place is fontconvert.py (e.g., optional clamping or substituting normative em-fractions for specific Unicode spaces like U+205F), not the generated headers.
Learnt from: znelson
Repo: crosspoint-reader/crosspoint-reader PR: 1168
File: lib/EpdFont/builtinFonts/opendyslexic_10_bold.h:2553-2553
Timestamp: 2026-02-26T05:59:54.187Z
Learning: Generated font headers (lib/EpdFont/builtinFonts/*.h) are produced by convert-builtin-fonts.sh and must not be edited by hand; any changes will be overwritten.
📚 Learning: 2026-02-26T05:59:54.187Z
Learnt from: znelson
Repo: crosspoint-reader/crosspoint-reader PR: 1168
File: lib/EpdFont/builtinFonts/opendyslexic_10_bold.h:2553-2553
Timestamp: 2026-02-26T05:59:54.187Z
Learning: fontconvert.py derives glyph advance from FreeType's linearHoriAdvance (16.16, unhinted). For OpenDyslexic, U+205F (MEDIUM MATHEMATICAL SPACE) returns ~97 px advance across sizes, which is an upstream font issue, not a code bug.
Applied to files:
lib/GfxRenderer/GfxRenderer.cpp
🔇 Additional comments (5)
lib/GfxRenderer/GfxRenderer.cpp (5)
67-69: LGTM!The refactor to by-value cursor coordinates correctly delegates fixed-point accumulation to the callers while keeping this function focused on pixel-level rendering.
203-256: Solid fixed-point accumulation pattern for eliminating spacing drift.The implementation correctly:
- Accumulates kerning and advances in 12.4 fixed-point
- Snaps to nearest pixel only at render time (line 245, 251)
- Halves combining mark advance in fixed-point before rounding (line 235), preserving precision
This addresses the root cause of issue
#1182where cumulative rounding errors caused uneven spacing.
943-961: LGTM!Public APIs correctly convert internal fixed-point values to pixels for external consumers, maintaining the abstraction boundary.
972-986: LGTM!Text width measurement uses the same fixed-point accumulation pattern as rendering, ensuring consistency between measured and rendered text widths.
1033-1074: LGTM!Rotated text rendering correctly mirrors the horizontal implementation with appropriate directional adjustments (subtraction instead of addition for Y-axis progression).
There was a problem hiding this comment.
Pull request overview
This PR implements fixed-point arithmetic (12.4 for glyph advances, 4.4 for kerning) to fix uneven character spacing (issue #1182). The solution replaces integer-only glyph positioning with sub-pixel precision that accumulates fractional remainders across the entire text layout, only rounding to whole pixels at render time. This eliminates the compounding rounding errors that caused visible spacing inconsistencies in words like "drew".
Changes:
- Introduced
fp4namespace with fixed-point conversion helpers (fromPixel, toPixel, toFloat) - Changed EpdGlyph::advanceX from uint8_t to uint16_t to store 12.4 fixed-point values
- Updated font conversion script to use linearHoriAdvance (unhinted) instead of grid-fitted advance.x, and changed kerning from floor() to round()
- Modified text rendering to use int32_t accumulators for x-position tracking in both normal and rotated text
- Regenerated all built-in font data with new fixed-point values
- Removed --force-autohint flag from Bookerly font generation
Reviewed changes
Copilot reviewed 15 out of 60 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| lib/EpdFont/EpdFontData.h | Added fp4 namespace with fixed-point helpers; changed EpdGlyph::advanceX to uint16_t; documented fixed-point conventions |
| lib/EpdFont/EpdFont.h | Updated getKerning() documentation to note 4.4 fixed-point return value |
| lib/EpdFont/EpdFont.cpp | Modified getTextBounds() to use fixed-point accumulator for cursor position |
| lib/EpdFont/scripts/fontconvert.py | Added fp4 conversion functions; switched to linearHoriAdvance; changed kerning from floor() to round() |
| lib/EpdFont/scripts/convert-builtin-fonts.sh | Removed --force-autohint flag from Bookerly font generation |
| lib/EpdFont/builtinFonts/opendyslexic_8_regular.h | Regenerated with new fixed-point advance and kerning values |
| lib/GfxRenderer/GfxRenderer.h | Removed renderChar() method declaration |
| lib/GfxRenderer/GfxRenderer.cpp | Replaced integer cursor with fixed-point accumulator in drawText() and drawTextRotated90CW(); removed renderChar() wrapper; updated all measurement functions to convert from fixed-point at API boundaries |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Haven’t reviewed the code, but kerning has definitely improved. |
|
I would like to merge this as a likely improvement upon the current mainline behavior, and gather feedback in #1182. If kerning and ligatures are not behaving well, I propose that we add a compile time flag to make This change disables the |
lukestein
left a comment
lukestein
left a comment
There was a problem hiding this comment.
I've been reading with this for a few days and looks great to my eyes
…actors) Merges upstream commits: - fix: fixed-point x-advance and kerning for text layout (crosspoint-reader#1168) - chore: add firmware size history script (crosspoint-reader#1235) - refactor: binary search for font lookups (crosspoint-reader#1202) - perf: remove unused ConfirmationActivity member (crosspoint-reader#1234) Resolved conflicts in ubuntu_10/12 bold/regular font headers by accepting upstream regenerated values (local had no changes to these files).
Was the point of this PR to fix the kerning or the consistency of the font weight? It seems you've changed both, and now, while the kerning might be more consistent (to my eye, I can barely notice this tbh) now the glyph strokes are much more inconsistent, like the BEFORE images in #1098. |
|
@adriancaruana I appreciate the feedback -- I responded to your comment in #1182. Let's keep the discussion going over there. |
Summary
What is the goal of this PR?
Hopefully fixes #1182.
Note: I think letterforms got a "heavier" appearance after #1098, which makes this more noticeable. The current version of this PR reverts the change to add
--force-autohintfor Bookerly, which to me seems to bring the font back to a more aesthetic and consistent weight.Problem
Character spacing was uneven in certain words. The word "drew" in Bookerly was the clearest example: a visible gap between
dandr, whileeandwappeared tightly condensed. The root cause was twofold:Integer-only glyph advances.
advanceXwas stored as auint8_tof whole pixels, sourced from FreeType's hintedadvance.x(which grid-fits to integers). A glyph whose true advance is 15.56px was stored as 16px -- an error of +0.44px per character that compounds across a line.Floor-rounded kerning. Kern adjustments were converted with
math.floor(), which systematically over-tightened negative kerns. A kern of -0.3px became -1px -- a 0.7px over-correction that visibly closed gaps.Combined, these produced the classic symptom: some pairs too wide, others too tight, with the imbalance varying per word.
Solution: fixed-point accumulation with 1/16-pixel resolution, for sub-pixel precision during text layout
All font metrics now use a "fixed-point 4" format -- 4 fractional bits giving 1/16-pixel (0.0625px) resolution. This is implemented with plain integer arithmetic (shifts and adds), requiring no floating-point on the ESP32.
How it works:
A value like 15.56px is stored as the integer
249:Two storage widths share the same 4 fractional bits:
advanceXuint16_tkernMatrixint8_tBecause both have 4 fractional bits, they add directly into a single
int32_taccumulator during layout. The accumulator is only snapped to the nearest whole pixel at the moment each glyph is rendered:Fractional remainders carry forward indefinitely. Rounding errors stay below +/- 0.5px and never compound.
Concrete example: "drew" in Bookerly
Before (integer advances, floor-rounded kerning):
The d-to-r gap was visibly wider than the tightly packed
rew.After (12.4 advances, 4.4 kerning, fractional accumulation):
Spacing is now
0, 1, 0pixels -- nearly uniform. Verified on-device: all 5 copies of "drew" in the test EPUB produce identical spacing, confirming zero accumulator drift.Changes
Font conversion (
fontconvert.py)linearHoriAdvance(FreeType 16.16, unhinted) instead ofadvance.x(26.6, grid-fitted to integers) for glyph advancesround()instead offloor()fp4_from_ft16_16()andfp4_from_design_units()helper functionsFont data structures (
EpdFontData.h)EpdGlyph::advanceX:uint8_ttouint16_t(no memory cost due to existing struct padding)fp4namespace withconstexprhelpers:fromPixel(),toPixel(),toFloat()Font API (
EpdFont.h/cpp,EpdFontFamily.h/cpp)getKerning()return type:int8_ttoint(to avoid truncation of the 4.4 value)Rendering (
GfxRenderer.cpp)drawText(): replace integer cursor withint32_tfixed-point accumulatordrawTextRotated90CW(): same accumulator treatment for vertical layoutgetTextAdvanceX(),getSpaceWidth(),getSpaceKernAdjust(),getKerning(): convert from fixed-point to pixel at API boundaryRegenerated all built-in font headers with new 12.4 advances and 4.4 kern values.
Memory impact
Zero additional RAM. The
advanceXfield grew fromuint8_ttouint16_t, but theEpdGlyphstruct already had 1 byte of padding at that position, so the struct size is unchanged. The fixed-point accumulator is a singleint32_ton the stack.Test plan
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, Claude Opus 4.6 helped figure out a non-floating point approach for sub-pixel error accumulation