From 32a5c1c3581919d3ad7ed2aab2149022bfa37829 Mon Sep 17 00:00:00 2001 From: Zach Nelson Date: Tue, 10 Mar 2026 22:55:23 -0500 Subject: [PATCH] fix: Fix inter-word spacing rounding error in text layout (#1311) ## Summary **What is the goal of this PR?** ### Problem Inter-word gap widths were computed as two separately-snapped integers: ```cpp gap = getSpaceWidth(fontId, style); // fp4::toPixel(spaceAdvance) gap += getSpaceKernAdjust(fontId, leftCp, rightCp, style); // fp4::toPixel(kern1 + kern2) ``` Because `fp4::toPixel(a) + fp4::toPixel(b)` can differ from `fp4::toPixel(a + b)` by +/-1 pixel when the fractional parts straddle a rounding boundary, each inter-word space could be one pixel wider or narrower than the correct value. This affected line-break width decisions and word-position accumulation across the whole paragraph layout pipeline. ### Fix Replaces `getSpaceKernAdjust()` with `getSpaceAdvance(fontId, leftCp, rightCp, style)`, which combines the space glyph advance and both flanking kern values (`kern(leftCp, ' ')` + `kern(' ', rightCp)`) into a single fixed-point sum before the snap: ```cpp return fp4::toPixel(spaceAdvanceFP + kern(leftCp, ' ') + kern(' ', rightCp)); ``` This is the same single-snap pattern already used by `getTextAdvanceX` for word widths. ### Changes - **`GfxRenderer`**: Replaces `getSpaceKernAdjust()` with `getSpaceAdvance()`. `getSpaceWidth()` is retained for the single-space-word case in `measureWordWidth` where no adjacent-word kern context is available. - **`ParsedText`**: All four call sites (`computeLineBreaks`, `computeHyphenatedLineBreaks`, and both loops in `extractLine`) updated to use `getSpaceAdvance()`. The now-redundant `spaceWidth` pre-computation and parameter are removed from all three internal layout functions. --- ### 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 for correctness**_ --- lib/Epub/Epub/ParsedText.cpp | 44 ++++++++++++++------------------- lib/Epub/Epub/ParsedText.h | 7 +++--- lib/GfxRenderer/GfxRenderer.cpp | 13 +++++++--- lib/GfxRenderer/GfxRenderer.h | 7 +++--- 4 files changed, 34 insertions(+), 37 deletions(-) diff --git a/lib/Epub/Epub/ParsedText.cpp b/lib/Epub/Epub/ParsedText.cpp index 3c26fb29..043020d7 100644 --- a/lib/Epub/Epub/ParsedText.cpp +++ b/lib/Epub/Epub/ParsedText.cpp @@ -101,20 +101,19 @@ void ParsedText::layoutAndExtractLines(const GfxRenderer& renderer, const int fo applyParagraphIndent(); const int pageWidth = viewportWidth; - const int spaceWidth = renderer.getSpaceWidth(fontId, EpdFontFamily::REGULAR); auto wordWidths = calculateWordWidths(renderer, fontId); std::vector lineBreakIndices; if (hyphenationEnabled) { // Use greedy layout that can split words mid-loop when a hyphenated prefix fits. - lineBreakIndices = computeHyphenatedLineBreaks(renderer, fontId, pageWidth, spaceWidth, wordWidths, wordContinues); + lineBreakIndices = computeHyphenatedLineBreaks(renderer, fontId, pageWidth, wordWidths, wordContinues); } else { - lineBreakIndices = computeLineBreaks(renderer, fontId, pageWidth, spaceWidth, wordWidths, wordContinues); + lineBreakIndices = computeLineBreaks(renderer, fontId, pageWidth, wordWidths, wordContinues); } const size_t lineCount = includeLastLine ? lineBreakIndices.size() : lineBreakIndices.size() - 1; for (size_t i = 0; i < lineCount; ++i) { - extractLine(i, pageWidth, spaceWidth, wordWidths, wordContinues, lineBreakIndices, processLine, renderer, fontId); + extractLine(i, pageWidth, wordWidths, wordContinues, lineBreakIndices, processLine, renderer, fontId); } // Remove consumed words so size() reflects only remaining words @@ -138,8 +137,7 @@ std::vector ParsedText::calculateWordWidths(const GfxRenderer& rendere } std::vector ParsedText::computeLineBreaks(const GfxRenderer& renderer, const int fontId, const int pageWidth, - const int spaceWidth, std::vector& wordWidths, - std::vector& continuesVec) { + std::vector& wordWidths, std::vector& continuesVec) { if (words.empty()) { return {}; } @@ -187,9 +185,8 @@ std::vector ParsedText::computeLineBreaks(const GfxRenderer& renderer, c // Add space before word j, unless it's the first word on the line or a continuation int gap = 0; if (j > static_cast(i) && !continuesVec[j]) { - gap = spaceWidth; - gap += renderer.getSpaceKernAdjust(fontId, lastCodepoint(words[j - 1]), firstCodepoint(words[j]), - wordStyles[j - 1]); + gap = + renderer.getSpaceAdvance(fontId, lastCodepoint(words[j - 1]), firstCodepoint(words[j]), wordStyles[j - 1]); } else if (j > static_cast(i) && continuesVec[j]) { // Cross-boundary kerning for continuation words (e.g. nonbreaking spaces, attached punctuation) gap = renderer.getKerning(fontId, lastCodepoint(words[j - 1]), firstCodepoint(words[j]), wordStyles[j - 1]); @@ -275,8 +272,7 @@ void ParsedText::applyParagraphIndent() { // Builds break indices while opportunistically splitting the word that would overflow the current line. std::vector ParsedText::computeHyphenatedLineBreaks(const GfxRenderer& renderer, const int fontId, - const int pageWidth, const int spaceWidth, - std::vector& wordWidths, + const int pageWidth, std::vector& wordWidths, std::vector& continuesVec) { // Calculate first line indent (only for left/justified text). // Positive text-indent (paragraph indent) is suppressed when extraParagraphSpacing is on. @@ -304,9 +300,8 @@ std::vector ParsedText::computeHyphenatedLineBreaks(const GfxRenderer& r const bool isFirstWord = currentIndex == lineStart; int spacing = 0; if (!isFirstWord && !continuesVec[currentIndex]) { - spacing = spaceWidth; - spacing += renderer.getSpaceKernAdjust(fontId, lastCodepoint(words[currentIndex - 1]), - firstCodepoint(words[currentIndex]), wordStyles[currentIndex - 1]); + spacing = renderer.getSpaceAdvance(fontId, lastCodepoint(words[currentIndex - 1]), + firstCodepoint(words[currentIndex]), wordStyles[currentIndex - 1]); } else if (!isFirstWord && continuesVec[currentIndex]) { // Cross-boundary kerning for continuation words (e.g. nonbreaking spaces, attached punctuation) spacing = renderer.getKerning(fontId, lastCodepoint(words[currentIndex - 1]), @@ -440,9 +435,8 @@ bool ParsedText::hyphenateWordAtIndex(const size_t wordIndex, const int availabl return true; } -void ParsedText::extractLine(const size_t breakIndex, const int pageWidth, const int spaceWidth, - const std::vector& wordWidths, const std::vector& continuesVec, - const std::vector& lineBreakIndices, +void ParsedText::extractLine(const size_t breakIndex, const int pageWidth, const std::vector& wordWidths, + const std::vector& continuesVec, const std::vector& lineBreakIndices, const std::function)>& processLine, const GfxRenderer& renderer, const int fontId) { const size_t lineBreak = lineBreakIndices[breakIndex]; @@ -471,11 +465,9 @@ void ParsedText::extractLine(const size_t breakIndex, const int pageWidth, const // Count gaps: each word after the first creates a gap, unless it's a continuation if (wordIdx > 0 && !continuesVec[lastBreakAt + wordIdx]) { actualGapCount++; - int naturalGap = spaceWidth; - naturalGap += renderer.getSpaceKernAdjust(fontId, lastCodepoint(words[lastBreakAt + wordIdx - 1]), - firstCodepoint(words[lastBreakAt + wordIdx]), - wordStyles[lastBreakAt + wordIdx - 1]); - totalNaturalGaps += naturalGap; + totalNaturalGaps += + renderer.getSpaceAdvance(fontId, lastCodepoint(words[lastBreakAt + wordIdx - 1]), + firstCodepoint(words[lastBreakAt + wordIdx]), wordStyles[lastBreakAt + wordIdx - 1]); } else if (wordIdx > 0 && continuesVec[lastBreakAt + wordIdx]) { // Cross-boundary kerning for continuation words (e.g. nonbreaking spaces, attached punctuation) totalNaturalGaps += @@ -520,11 +512,11 @@ void ParsedText::extractLine(const size_t breakIndex, const int pageWidth, const firstCodepoint(words[lastBreakAt + wordIdx + 1]), wordStyles[lastBreakAt + wordIdx]); xpos += advance; } else { - int gap = spaceWidth; + int gap = 0; if (wordIdx + 1 < lineWordCount) { - gap += renderer.getSpaceKernAdjust(fontId, lastCodepoint(words[lastBreakAt + wordIdx]), - firstCodepoint(words[lastBreakAt + wordIdx + 1]), - wordStyles[lastBreakAt + wordIdx]); + gap = renderer.getSpaceAdvance(fontId, lastCodepoint(words[lastBreakAt + wordIdx]), + firstCodepoint(words[lastBreakAt + wordIdx + 1]), + wordStyles[lastBreakAt + wordIdx]); } if (blockStyle.alignment == CssTextAlign::Justify && !isLastLine) { gap += justifyExtra; diff --git a/lib/Epub/Epub/ParsedText.h b/lib/Epub/Epub/ParsedText.h index 222dd5e3..9d43400e 100644 --- a/lib/Epub/Epub/ParsedText.h +++ b/lib/Epub/Epub/ParsedText.h @@ -21,14 +21,13 @@ class ParsedText { bool hyphenationEnabled; void applyParagraphIndent(); - std::vector computeLineBreaks(const GfxRenderer& renderer, int fontId, int pageWidth, int spaceWidth, + std::vector computeLineBreaks(const GfxRenderer& renderer, int fontId, int pageWidth, std::vector& wordWidths, std::vector& continuesVec); std::vector computeHyphenatedLineBreaks(const GfxRenderer& renderer, int fontId, int pageWidth, - int spaceWidth, std::vector& wordWidths, - std::vector& continuesVec); + std::vector& wordWidths, std::vector& continuesVec); bool hyphenateWordAtIndex(size_t wordIndex, int availableWidth, const GfxRenderer& renderer, int fontId, std::vector& wordWidths, bool allowFallbackBreaks); - void extractLine(size_t breakIndex, int pageWidth, int spaceWidth, const std::vector& wordWidths, + void extractLine(size_t breakIndex, int pageWidth, const std::vector& wordWidths, const std::vector& continuesVec, const std::vector& lineBreakIndices, const std::function)>& processLine, const GfxRenderer& renderer, int fontId); diff --git a/lib/GfxRenderer/GfxRenderer.cpp b/lib/GfxRenderer/GfxRenderer.cpp index afd064c6..d8ccc991 100644 --- a/lib/GfxRenderer/GfxRenderer.cpp +++ b/lib/GfxRenderer/GfxRenderer.cpp @@ -943,13 +943,18 @@ int GfxRenderer::getSpaceWidth(const int fontId, const EpdFontFamily::Style styl return spaceGlyph ? fp4::toPixel(spaceGlyph->advanceX) : 0; // snap 12.4 fixed-point to nearest pixel } -int GfxRenderer::getSpaceKernAdjust(const int fontId, const uint32_t leftCp, const uint32_t rightCp, - const EpdFontFamily::Style style) const { +int GfxRenderer::getSpaceAdvance(const int fontId, const uint32_t leftCp, const uint32_t rightCp, + const EpdFontFamily::Style style) const { const auto fontIt = fontMap.find(fontId); if (fontIt == fontMap.end()) return 0; const auto& font = fontIt->second; - const int kernFP = font.getKerning(leftCp, ' ', style) + font.getKerning(' ', rightCp, style); // 4.4 fixed-point - return fp4::toPixel(kernFP); // snap 4.4 fixed-point to nearest pixel + const EpdGlyph* spaceGlyph = font.getGlyph(' ', style); + const int32_t spaceAdvanceFP = spaceGlyph ? static_cast(spaceGlyph->advanceX) : 0; + // Combine space advance + flanking kern into one fixed-point sum before snapping. + // Snapping the combined value avoids the +/-1 px error from snapping each component separately. + const int32_t kernFP = static_cast(font.getKerning(leftCp, ' ', style)) + + static_cast(font.getKerning(' ', rightCp, style)); + return fp4::toPixel(spaceAdvanceFP + kernFP); } int GfxRenderer::getKerning(const int fontId, const uint32_t leftCp, const uint32_t rightCp, diff --git a/lib/GfxRenderer/GfxRenderer.h b/lib/GfxRenderer/GfxRenderer.h index 53a7065d..50544e13 100644 --- a/lib/GfxRenderer/GfxRenderer.h +++ b/lib/GfxRenderer/GfxRenderer.h @@ -110,9 +110,10 @@ class GfxRenderer { void drawText(int fontId, int x, int y, const char* text, bool black = true, EpdFontFamily::Style style = EpdFontFamily::REGULAR) const; int getSpaceWidth(int fontId, EpdFontFamily::Style style = EpdFontFamily::REGULAR) const; - /// Returns the kerning adjustment for a space between two codepoints: - /// kern(leftCp, ' ') + kern(' ', rightCp). Returns 0 if kerning is unavailable. - int getSpaceKernAdjust(int fontId, uint32_t leftCp, uint32_t rightCp, EpdFontFamily::Style style) const; + /// Returns the total inter-word advance: fp4::toPixel(spaceAdvance + kern(leftCp,' ') + kern(' ',rightCp)). + /// Using a single snap avoids the +/-1 px rounding error that arises when space advance and kern are + /// snapped separately and then added as integers. + int getSpaceAdvance(int fontId, uint32_t leftCp, uint32_t rightCp, EpdFontFamily::Style style) const; /// Returns the kerning adjustment between two adjacent codepoints. int getKerning(int fontId, uint32_t leftCp, uint32_t rightCp, EpdFontFamily::Style style) const; int getTextAdvanceX(int fontId, const char* text, EpdFontFamily::Style style) const;