Calibrate sixel cell size to agg's actual rendering (#84)#86
Merged
Conversation
tuirec advertised a sixel cell resolution from a static formula (~0.6*fontSize, rounded to int). That was wrong two ways: the 0.6 ratio does not match whatever monospace font agg resolves on the host, and agg's per-column advance is fractional (e.g. 8.425px at font-size 14) while terminal cell-size reports (CSI 16t) must be integers. An app that sizes a sixel raster as cells*reportedCell therefore rendered a few percent undersized — at font-size 14, ~5% (visibly short of borders). Fix: when a GIF will be rendered, measure agg's real per-column advance by rendering two probe casts that differ only in column count (the slope cancels agg's fixed padding and is independent of the resolved font), then align the grid so the integer report matches what agg renders: - Width: pick the font-size nearest the requested one whose cell rounds closest to an integer (kept as-is when already within ~0.15px; font-size is integer-only in agg, so this is the only width lever). - Height: report round(fontSize*lineHeight) and pass agg a line-height of cellH/fontSize so agg's row height equals the report exactly. The chosen font-size/line-height are used for the render too, so the cast reports and the GIF agree. Falls back to the previous formula when no GIF is produced (agg unavailable) or calibration fails. Verified end-to-end: UICatalog's About-box fire now fills the dialog interior edge-to-edge (font-size 14 auto-aligns to 15; cell 9x20; raster 540px vs ~542px interior = 99.6% vs the previous ~96%). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 18db4e016a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Two P2s from Codex review on #86: - record.Run gated calibration on `config.Output != "" && GIF.AggPath != ""`, but a GIF renders whenever Output is set (AggPath is normalized to "agg" downstream). Zero-value-config API callers therefore rendered a GIF but skipped calibration, falling back to the 0.6*fontSize formula. Gate on a new willRenderGIF(config) that mirrors the render condition only. - MeasureColumnWidthPx reused the caller's full config, including Select. The probe cast only has events at 0.1s/0.2s, so a Select like "1.." trims it to empty and calibration silently fails back to the formula. Strip Select via probeConfig() (geometry is timeline-independent); font/spacing options that do affect column width are preserved. Added unit tests for both seams (TestWillRenderGIF, TestProbeConfig...). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fixes Windows integration failure (TestRunRecordsKeystrokesAndRenders: "recording hit max duration") introduced by enabling calibration in the zero-value path. The recording's MaxDuration context is created at the top of Run, and calibration ran after it — so the agg probe-render wall-clock time was charged against the recording budget. With MaxDuration=1s and two agg renders on Windows, the recording timed out immediately. Move geometry computation + calibration to before the MaxDuration context is created, and run the probes under their own timeout off `parent`. The recording now gets its full budget; probe time is pre-recording setup. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #84. tuirec advertised the sixel cell resolution (
CSI 16t, plus theCSI 14t/18treports) from a static formula —~0.6 × fontSize, rounded to an integer. That is wrong in two compounding ways:0.6ratio does not match whatever monospace font agg actually resolves on the host (measured0.6018for DejaVu Sans Mono here; JetBrains Mono is0.6, but it is not always the one agg picks).cells × reportedCell(the correct, standard approach — it fills exactly on a real terminal) therefore renders undersized. At the default font-size 14 that is ~5%, visibly short of a bordered region.Fix
When a GIF will be rendered, measure agg''s real per-column advance instead of guessing it, then align the grid so the integer report matches what agg renders:
pkg/gif/measure.go): render two tiny probe casts that differ only in column count;(wideWidth − narrowWidth) / Δcolsisolates the per-column advance, cancelling agg''s fixed horizontal padding and working for whatever font agg resolves.pkg/record/calibrate.go): agg''s--font-sizeis integer-only, so the cell can only be made integer by font-size choice. Keep the requested size when its cell already rounds within ~0.15 px; otherwise pick the nearest size that rounds cleanly (e.g. 14 → 15: cell9.04, error0.04).round(fontSize × lineHeight)and pass agglineHeight = cellH / fontSize, so agg''s row height equals the report exactly.The chosen font-size/line-height are threaded into the render, so the cast''s sixel reports and the GIF agree. Falls back to the previous formula when no GIF is produced (agg unavailable) or calibration fails, so cast-only recordings are unaffected. When the font-size is nudged it is logged:
adjusted agg font-size to 15 to align the sixel cell grid (#84).Trade-off
Aligning the width grid can nudge the rendered font-size by ±1 (only when the requested size is poorly aligned). Faithful sixel geometry seemed more valuable than honoring an exact pixel font-size, and the change is ≤1px and logged. Happy to gate this behind a flag or apply it only to the default size if you prefer.
Verification
go test ./pkg/record/ ./pkg/gif/— pass (added unit tests for the alignment math);go vet,gofmtclean.8×17, raster480px, ~96% of the 60-col interior (visible right-edge gap). After:adjusted agg font-size to 15, cell9×20, raster540px, ~99.6% — the fire now fills the dialog interior edge-to-edge.Pairs with the doc fix in #85 (which tells agents to measure sixel size rather than eyeball it).
🤖 Generated with Claude Code