fix: Accumulate glyph positions in fixed-point for smoother text spacing#1662
fix: Accumulate glyph positions in fixed-point for smoother text spacing#1662ingpaschke wants to merge 2 commits intocrosspoint-reader:masterfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📜 Recent review details🧰 Additional context used🧠 Learnings (14)📓 Common learnings📚 Learning: 2026-03-17T15:27:17.468ZApplied to files:
📚 Learning: 2026-04-08T15:24:06.672ZApplied to files:
📚 Learning: 2026-02-19T12:17:05.421ZApplied to files:
📚 Learning: 2026-02-26T06:00:13.512ZApplied to files:
📚 Learning: 2026-04-12T02:49:05.293ZApplied to files:
📚 Learning: 2026-03-21T15:30:11.892ZApplied to files:
📚 Learning: 2026-04-16T06:13:27.052ZApplied to files:
📚 Learning: 2026-04-13T22:45:24.939ZApplied to files:
📚 Learning: 2026-04-12T03:00:48.309ZApplied to files:
📚 Learning: 2026-02-27T22:49:59.600ZApplied to files:
📚 Learning: 2026-03-02T10:14:16.036ZApplied to files:
📚 Learning: 2026-03-28T11:06:29.611ZApplied to files:
📚 Learning: 2026-04-12T12:28:33.205ZApplied to files:
🔇 Additional comments (6)
📝 WalkthroughWalkthroughRefactors text layout to use a single 12.4 fixed-point cursor accumulator ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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. ✨ Finishing Touches⚔️ Resolve merge conflicts
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 |
ingpaschke
changed the title
Previously each glyph advance was snapped to the nearest integer pixel before adding to the cursor, discarding fractional precision at every step. This caused visible spacing unevenness — letter pairs alternating between N and N+1 pixel gaps in a pattern unrelated to the font design. Now the cursor accumulates in 12.4 fixed-point and only snaps to pixels at render time. The same fix is applied to drawText, drawTextRotated90CW, and getTextBounds to keep rendering and measurement consistent. Zero performance cost — same number of operations, same types. The binary is actually 12 bytes smaller since the toPixel() call was removed from the advance step. Tested on X4 hardware with Bookerly 14pt justified text. Before/after screenshots confirm 6,030 pixels shifted across a single page, with visibly more even character spacing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ingpaschke
force-pushed
the
fix/font-spacing-precision
branch
from
cf14d43 to
2a660c3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/GfxRenderer/GfxRenderer.cpp (1)
261-277: KeepgetTextAdvanceX()on the same rounding model.These paths now snap only the cumulative cursor, but
GfxRenderer::getTextAdvanceX()at Line 1023 still sums rounded segments and says it “matches drawText.” That is no longer true, so any caller that uses the helper to place a follow-on run can drift by a pixel relative to whatdrawText()now renders.Suggested follow-up
int GfxRenderer::getTextAdvanceX(const int fontId, const char* text, EpdFontFamily::Style style) const { const auto fontIt = fontMap.find(fontId); if (fontIt == fontMap.end()) { LOG_ERR("GFX", "Font %d not found", fontId); return 0; } uint32_t cp; uint32_t prevCp = 0; - int widthPx = 0; - int32_t prevAdvanceFP = 0; // 12.4 fixed-point: prev glyph's advance + next kern for snap + int32_t cursorFP = 0; // accumulate advance in 12.4 fixed-point const auto& font = fontIt->second; while ((cp = utf8NextCodepoint(reinterpret_cast<const uint8_t**>(&text)))) { if (utf8IsCombiningMark(cp)) { continue; } cp = font.applyLigatures(cp, text, style); - // Differential rounding: snap (previous advance + current kern) together, - // matching drawText so measurement and rendering agree exactly. if (prevCp != 0) { - const auto kernFP = font.getKerning(prevCp, cp, style); // 4.4 fixed-point kern - widthPx += fp4::toPixel(prevAdvanceFP + kernFP); // snap 12.4 fixed-point to nearest pixel + cursorFP += font.getKerning(prevCp, cp, style); } const EpdGlyph* glyph = font.getGlyph(cp, style); - prevAdvanceFP = glyph ? glyph->advanceX : 0; + cursorFP += glyph ? glyph->advanceX : 0; prevCp = cp; } - widthPx += fp4::toPixel(prevAdvanceFP); // final glyph's advance - return widthPx; + return fp4::toPixel(cursorFP); }Also applies to: 1108-1124
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/GfxRenderer/GfxRenderer.cpp` around lines 261 - 277, getTextAdvanceX() currently sums rounded segment advances and will diverge from drawText() which keeps a fractional accumulator (cursorFP) and only snaps to pixels for rendering; update getTextAdvanceX() to mirror drawText() by accumulating advances and kerning in the same fixed-point representation (use the same units as cursorFP / lastBaseAdvanceFP and include font.getKerning(...) values) and only convert to pixels once using fp4::toPixel at the end so placement matches drawText(); apply the same change to the other helper path mentioned (the code region corresponding to lines 1108-1124) so both helpers use the identical fixed-point accumulation and final snapping strategy as drawText()/renderCharImpl.
🤖 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/GfxRenderer/GfxRenderer.cpp`:
- Around line 253-255: The combining-mark positioning is using cursorFP (which
is already after the base glyph) and adding lastBaseAdvanceFP/2, so flip the
signs: change the computation of combiningX/combiningY to use cursorFP -
lastBaseAdvanceFP/2 for the horizontal branch
(renderCharImpl<TextRotation::None>) and the opposite sign in the rotated branch
(use + lastBaseAdvanceFP/2 where it previously used -), and then apply the
mirrored change to the corresponding math in EpdFont (the code that computes
combining mark bounds) so bounds stay aligned with the new centering logic;
update every matching occurrence (including the other occurrence noted in the
review) so combining marks are centered over the base glyph.
---
Nitpick comments:
In `@lib/GfxRenderer/GfxRenderer.cpp`:
- Around line 261-277: getTextAdvanceX() currently sums rounded segment advances
and will diverge from drawText() which keeps a fractional accumulator (cursorFP)
and only snaps to pixels for rendering; update getTextAdvanceX() to mirror
drawText() by accumulating advances and kerning in the same fixed-point
representation (use the same units as cursorFP / lastBaseAdvanceFP and include
font.getKerning(...) values) and only convert to pixels once using fp4::toPixel
at the end so placement matches drawText(); apply the same change to the other
helper path mentioned (the code region corresponding to lines 1108-1124) so both
helpers use the identical fixed-point accumulation and final snapping strategy
as drawText()/renderCharImpl.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 97043913-71f2-42ee-a4ca-a5afcb5432fc
📒 Files selected for processing (2)
lib/EpdFont/EpdFont.cpplib/GfxRenderer/GfxRenderer.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/EpdFont/EpdFont.cpp
📜 Review details
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: znelson
Repo: crosspoint-reader/crosspoint-reader PR: 1413
File: lib/EpdFont/EpdFont.cpp:34-36
Timestamp: 2026-03-17T15:27:17.468Z
Learning: In crosspoint-reader/crosspoint-reader, `EpdFont::getGlyph()` falls back to `getGlyph(REPLACEMENT_GLYPH)` (U+FFFD) before returning `nullptr`. All fonts in this project are guaranteed to include a U+FFFD replacement glyph, so the `!glyph` null branch in `EpdFont::getTextBounds()` (and similar rendering paths) is unreachable in practice. Do not flag stale-state issues in that branch (e.g., leftover `lastBaseAdvanceFP`/`lastBaseTop` after a null glyph) as bugs in future reviews.
Learnt from: znelson
Repo: crosspoint-reader/crosspoint-reader PR: 1168
File: lib/EpdFont/builtinFonts/opendyslexic_10_bold.h:2553-2553
Timestamp: 2026-02-26T06:00:13.512Z
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: 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.
📚 Learning: 2026-03-17T15:27:17.468Z
Learnt from: znelson
Repo: crosspoint-reader/crosspoint-reader PR: 1413
File: lib/EpdFont/EpdFont.cpp:34-36
Timestamp: 2026-03-17T15:27:17.468Z
Learning: In crosspoint-reader/crosspoint-reader, `EpdFont::getGlyph()` falls back to `getGlyph(REPLACEMENT_GLYPH)` (U+FFFD) before returning `nullptr`. All fonts in this project are guaranteed to include a U+FFFD replacement glyph, so the `!glyph` null branch in `EpdFont::getTextBounds()` (and similar rendering paths) is unreachable in practice. Do not flag stale-state issues in that branch (e.g., leftover `lastBaseAdvanceFP`/`lastBaseTop` after a null glyph) as bugs in future reviews.
Applied to files:
lib/GfxRenderer/GfxRenderer.cpp
📚 Learning: 2026-04-08T15:24:06.651Z
Learnt from: zgredex
Repo: crosspoint-reader/crosspoint-reader PR: 0
File: :0-0
Timestamp: 2026-04-08T15:24:06.651Z
Learning: In crosspoint-reader/crosspoint-reader (src/activities/reader/XtcReaderActivity.cpp, src/activities/util/BmpViewerActivity.cpp, src/activities/reader/EpubReaderActivity.cpp, and other callsites), renderGrayscale's callback type is `void (*renderFn)(const GfxRenderer&, const void*)`. Lambdas passed as this callback correctly use `const GfxRenderer& r` as the first parameter. Where the renderer must be mutated inside the callback (e.g., drawBitmap, drawButtonHints), `const_cast<GfxRenderer&>(r)` is used. No `cppcheck-suppress constParameterReference` is needed. Do not flag `const GfxRenderer& r` in these lambdas or suggest removing the `const_cast` in future reviews — this is the established pattern.
Applied to files:
lib/GfxRenderer/GfxRenderer.cpp
📚 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/GfxRenderer/GfxRenderer.cpp
📚 Learning: 2026-02-26T06:00:13.512Z
Learnt from: znelson
Repo: crosspoint-reader/crosspoint-reader PR: 1168
File: lib/EpdFont/builtinFonts/opendyslexic_10_bold.h:2553-2553
Timestamp: 2026-02-26T06:00:13.512Z
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
📚 Learning: 2026-02-27T22:49:59.600Z
Learnt from: ngxson
Repo: crosspoint-reader/crosspoint-reader PR: 1218
File: src/activities/ActivityManager.cpp:254-265
Timestamp: 2026-02-27T22:49:59.600Z
Learning: In this codebase, assertions are always enabled (no NDEBUG). Use assert() to crash on programmer errors and surface logic bugs during development and in production builds. Do not rely on asserts for runtime error handling; they should enforce invariants that must always hold. Keep asserts side-effect free and inexpensive, and avoid relying on them for user-visible failures. Include <cassert> where appropriate and document the invariant being tested.
Applied to files:
lib/GfxRenderer/GfxRenderer.cpp
📚 Learning: 2026-03-02T10:14:16.036Z
Learnt from: Uri-Tauber
Repo: crosspoint-reader/crosspoint-reader PR: 1245
File: lib/Epub/Epub/Section.cpp:277-308
Timestamp: 2026-03-02T10:14:16.036Z
Learning: Guideline: Strengthen serialization::readString to defend against unbounded growth when reading from disk data. Implement and enforce a maximum allowed length (e.g., a configured or reasonable constant) and validate the incoming length before resizing or allocating. Audit all call sites (e.g., BookMetadataCache, TextBlock, KOReaderCredentialStore, Section cache readers) to ensure they do not rely on unbounded len-based resizing. If the readString API must remain, add internal safeguards (bounds checks, length validation, and error handling) so per-call-site validations are not required. Ensure Section cache files remain versioned (SECTION_FILE_VERSION) and parameter mismatches invalidate caches, but do not rely on unsafe allocations; prefer safe, bounded reads with explicit errors on invalid data.
Applied to files:
lib/GfxRenderer/GfxRenderer.cpp
📚 Learning: 2026-03-28T11:06:29.611Z
Learnt from: pablohc
Repo: crosspoint-reader/crosspoint-reader PR: 1488
File: src/activities/home/HomeActivity.cpp:92-95
Timestamp: 2026-03-28T11:06:29.611Z
Learning: When reviewing crosspoint-reader code, avoid flagging a missing `renderer.displayBuffer()` call immediately after `GUI.drawPopup()` / `BaseTheme::drawPopup()`: `BaseTheme::drawPopup()` already calls `renderer.displayBuffer()` before returning, so the popup is guaranteed to be flushed to the e-ink panel before subsequent blocking work begins. Conversely, do not require a `renderer.displayBuffer()` call after `fillPopupProgress()`; it intentionally does not flush, so intermediate progress-bar updates may not appear unless the update granularity warrants an explicit flush elsewhere.
Applied to files:
lib/GfxRenderer/GfxRenderer.cpp
📚 Learning: 2026-04-12T12:28:33.205Z
Learnt from: Uri-Tauber
Repo: crosspoint-reader/crosspoint-reader PR: 1629
File: src/activities/home/HomeActivity.cpp:119-120
Timestamp: 2026-04-12T12:28:33.205Z
Learning: When reviewing code in this repository (C/C++ sources), set review comment severity according to this policy:
- Use **Major** (🟠) only for defects with realistic risk of crash, out-of-memory (OOM), invalid pointer dereference, data corruption, or other severe issues that are unlikely to be caught in casual/manual device testing.
- Use **Minor** or informational for UX gaps, logic edge-cases, style issues, or missing feature completeness that the author can verify (or has verified) through normal device use.
- Do **not** escalate severity to Major based on behavioral/UX observations alone; assume the author has already tested the feature on their own device and only treat issues as Major if they match the high-risk defect categories above.
Applied to files:
lib/GfxRenderer/GfxRenderer.cpp
| const int combiningX = fp4::toPixel(cursorFP + lastBaseAdvanceFP / 2); | ||
| const int combiningY = yPos - raiseBy; | ||
| renderCharImpl<TextRotation::None>(*this, renderMode, font, cp, combiningX, combiningY, black, style); |
There was a problem hiding this comment.
Combining marks are centered from the wrong side of the pen.
By the time these branches run, cursorFP already points after the previous base glyph. Using + lastBaseAdvanceFP / 2 in horizontal text and - lastBaseAdvanceFP / 2 in the rotated path shifts the mark by a full advance instead of centering it over the base. Please flip the signs here, and mirror the same change in the matching lib/EpdFont/EpdFont.cpp math so bounds stay aligned.
Suggested fix
- const int combiningX = fp4::toPixel(cursorFP + lastBaseAdvanceFP / 2);
+ const int combiningX = fp4::toPixel(cursorFP - lastBaseAdvanceFP / 2);
...
- const int combiningY = fp4::toPixel(cursorFP - lastBaseAdvanceFP / 2);
+ const int combiningY = fp4::toPixel(cursorFP + lastBaseAdvanceFP / 2);Also applies to: 1100-1102
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/GfxRenderer/GfxRenderer.cpp` around lines 253 - 255, The combining-mark
positioning is using cursorFP (which is already after the base glyph) and adding
lastBaseAdvanceFP/2, so flip the signs: change the computation of
combiningX/combiningY to use cursorFP - lastBaseAdvanceFP/2 for the horizontal
branch (renderCharImpl<TextRotation::None>) and the opposite sign in the rotated
branch (use + lastBaseAdvanceFP/2 where it previously used -), and then apply
the mirrored change to the corresponding math in EpdFont (the code that computes
combining mark bounds) so bounds stay aligned with the new centering logic;
update every matching occurrence (including the other occurrence noted in the
review) so combining marks are centered over the base glyph.
|
Hey @ingpaschke, I appreciate having another set of eyes on the subtle spacing tweaks to get great typographic layout! I see the PR notes only call out #1168, not the tweaks for differential rounding in #1413. Can you help me compare this to 1398aeb? I was trying to fix the case where I'd see common pairs of characters, e.g. "oo", with slightly different spacing across a page. My analysis was that it was happening because each character snaps to a position based on accumulated fixed-point error, and so spacing between specific glyph pairs can vary. For example, say the spacing for "oo" is 8.875 px in fixed-point. Depending on the cumulative error, sometimes that can snap to 8px, other times to 9px. On a relatively low density display like this, and with small font, the pixel snapping difference starts to become obvious, and felt off to me. The "typographic consensus" is largely shaped by systems that have sub-pixel rendering, anti-aliasing, and high DPI displays, luxuries the X4 doesn't have. My math is that a 1px difference on "oo", for example, works out to a 100% variance in layout: either 1px or 2px space. Maybe the "non-smooth" cases you observe primarily show up with justified line layout, because the right edge becomes slightly ragged due to the accumulated error of the current differential rounding algorithm? I admit I'm primarily testing with left-aligned text which intentionally leaves a ragged-right. If #1413 gets us "even" rendering for words, in the sense that pairs of characters get consistent spacing (by using fixed-point differential rounding from the x-advance and kerning space), but leaves us with line-length accumulated error (with ragged edges when justified as a remaining pain point), perhaps we should distribute accumulated line error into inter-word spacing for justified layout? I'd love to hear from @jonasdiemer too, with his observations regarding #1182 after 1398aeb (which is currently in main but not in 1.2.0 release). |
|
Hi @znelson, thanks for the detailed analysis! You're right that I should have referenced #1413 — the differential rounding was the foundation this builds on. To answer your question about "oo" consistency: yes, with the accumulator approach, identical pairs can get different pixel spacing depending on accumulated fractional position. I understand why differential rounding was designed to prevent this. I like the accumulator approach of the PR better (at least with justified text). But I agree that's probably personal preference. It would be easy to make this a config option. The fundamental problem is that we calculate sub-pixel positions, but with our pre-rendered pixel font that's antialiased for whole pixels, we can only render at whole pixel positions. I've spent the last two days experimenting extensively with sub-pixel positioning approaches to find the best trade-off for this hardware. Here's what I tested:
With the dual-position approach identical pairs always render with the same variant (since the threshold is deterministic) AND each variant is properly hinted for its grid position. No blending, no softening. I think there's a path where the accumulator fix (this PR) lands first as a standalone simple option for those who prefer it, and dual-position glyphs come as a follow-up PR once the storage budget is worked out (possibly combined with #589's compression improvements). Screenshot of the dual position glyph rendering: 
|
|
@ingpaschke Thanks for the details! Let me give this a few days on device to see if anything stands out 🙂 |
getTextAdvanceX used per-step differential rounding while drawText uses a fixed-point accumulator. This mismatch caused the layout engine to compute word widths slightly differently from how they actually render, leading to justification gaps being computed from incorrect measurements. Now both use the same accumulator approach: accumulate advances and kerning in 12.4 fixed-point, snap to pixels once at the end. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
After using this for a few days, I think it's a regression from what's in main.
Yes, that's true. And we need to work within those constraints. The fundamental problem with this PR is that the kerning choice for any given pair of letters depends not only on the letters themselves, but also where those letters happen to fall in the accumulated X position across the line. This reverts exactly what #1413 was fixing. Glyphs must snap to a pixel, yes. But if we pick the kerning snap including accumulated fixed-point X position error across the line, we get inconsistencies in common kerning pairs. Here are a number of examples to compare:
@ingpaschke Can you please provide specific examples of what looks smoother in rendering with this PR versus master? If it's about the right-edge alignment with justified text, maybe we'd be better with something like this patch: |
|
Based on your extensive analysis, I have to agree that this PR on its own is really a regression. That's why I went further and added the dual glyph rendering and tracking, which I still think produced superior results — not just consistent within pairs, but consistent across the whole line. The eye sees a uniform rhythm. You can't really see the difference in screenshots from the device, because they don't show the greyscale antialising, so here are two photos. Stock firmware: 
With FP4 accumulation, dual glyph and tracking: 
But I understand that these changes are a larger restructuring than fits CrossPoint's current constraints. The dual-glyph approach naturally fits an SD-card-delivered model once user-font support lands — happy to contribute in that direction when the time comes. |
|
@ingpaschke Would you be willing to join the group discussing font changes in #1693? We definitely need people passionate about fonts and font rendering to drive the next wave of improvements. |
|
Closing in favor of other font discussions. |
2 participants
Summary
drawText(),drawTextRotated90CW(), andgetTextBounds()now accumulate the cursor position in 12.4 fixed-point and only snap to integer pixels at render timeBackground
PR #1168 by @znelson introduced 12.4 fixed-point advances and differential rounding, which was a significant improvement. However, as @jonasdiemer noted in #1182:
He attributed the remaining unevenness to screen resolution limits, but it was actually residual rounding error from the differential rounding approach, which still discarded the fractional remainder at each step.
How it works
The old code rounded and added each step:
cursor (int) += round(advance + kern) // fraction lost every characterThe new code accumulates without rounding:
This ensures each glyph is placed at the rounded version of its true cumulative position, rather than a chain of individually rounded steps. The fractional error never compounds.
Impact
toPixel()call from the advance step)getTextBounds) updated to match, so line breaks stay consistentTested on X4 hardware.
Side-by-side with highlights (left = old/red, right = new/blue):
Fixes #1182
AI Usage
Did you use AI tools to help write this code? YES
The fix was developed collaboratively with Claude Code (Opus 4.6). The approach (fixed-point accumulator vs per-step rounding) is a well-known technique in text rendering — Claude identified the rounding pattern in the existing code and implemented the fix.