fix(captions): place captions by anchor and margin, not by an invisible band (rc.3) - #484
Merged
Merged
Conversation
…ts block Adds one optional field to the text payload, `verticalAlign`, threaded through the scene into all three rasterizers. Absent means centred — so annotations, which never emit it, render byte for byte as before. Centring is what made caption placement incoherent: a centred block moves BOTH its edges when it grows, so a caption drifted vertically whenever the text wrapped to another line, and no setting could hold it still. An anchored block keeps its anchored edge exactly where it was put, at any line count. The Linux test asserts precisely that, which is the assertion the old geometry could not express. Option<String> and not an enum, for the same reason as `space`: serde rejects an unknown unit variant, so a future value would cost the whole scene on an older binary rather than one misplaced caption. Windows needed the layout box inset vertically by the plate margin and the draw origin offset to match, or a bottom anchor puts the glyphs flush against the box and the plate's lower margin gets clipped. That arithmetic cancels exactly for the centred case; it is now a pure function with a test pinning it to where it was, because it was the one calculation on the Windows path no test covered. Nothing emits the field yet. (cherry picked from commit c1fb929)
…le band Replaces the whole placement model. Every control now names the edge it measures from, and there is exactly one per axis: anchorV + insetY bottom | top, and a distance from that edge anchorH + insetX left | center | right, and a distance from that edge Deleted: verticalPosition, offsetY, offsetX, width, textAlign — and the machinery that existed only to compensate for the old geometry (the fixed 22% band, the ink-height estimate, the overhang, the reachable-offset range, the preset-vs-slider epsilon). The old model drew every caption inside an invisible fixed-height box and let each rasterizer centre the ink in it, while the only thing on screen — the background plate — hugs the text. So `width` changed nothing visible until the text happened to wrap; the horizontal offset moved a band the text floated inside; text-align fought that offset for the same outcome; wrapping grew a centred block from both edges, which moved the caption vertically when nothing vertical had been touched; and the vertical offset had to be signed and clamped against an estimate, which is where "-7.3%" came from. All five are the same decision, so this replaces the decision rather than the controls. `width` becomes a derived column instead of a control (BBC's line-length table: 68% landscape, 90% vertical). How much text is on screen is already a legible question elsewhere — min/max words per line. The default inset follows the output aspect, because 5% on a 9:16 export is under the platform's own chrome. Migration reproduces the PIXELS, not the fields: the old band's geometry is known, so the drawn block's edges are recoverable, and the nearer one becomes the anchor. A migrated project does not move on screen. Line breaks do change for a project with a non-default width, since that WAS the wrap column. Tests assert the invariant as a property — the anchored edge lands at 100−insetY (or insetY) for every font size, background state and inset — rather than pinning numbers a future change would just have to update. (cherry picked from commit b4810d1)
The Linux anchor test measured the bottom-most inked row and called a 184→199 move a drift. It is not: "Hx" has no descenders and "replier" does, so the ink reaches further down inside an identically-placed line box. What this code pins is the line box — and therefore the plate, which is what the compositor draws and what the viewer sees. Anchoring the box rather than the last glyph pixel is the behaviour every text renderer has. So the assertion moves to `atlas.plate`, which is deterministic and is the actual contract. The ink is still checked, but only for the thing that is true of it: that it stays inside the plate carrying it, with the same `pad_y` tolerance `the_plate_hugs_the_text_instead_of_filling_the_box` already uses for that relationship — a glyph may overshoot its own line box slightly, which is precisely the assumption the first version got wrong. Found by CI: only text_windows.rs compiles on the machine this was written on. (cherry picked from commit b061746)
…igrate insetX Two findings from CodeRabbit's review of #482, both real. **The plate lost its margin on the anchored side.** Pinning the TEXT block flush against the box edge left the plate laying all of its padding on the opposite side and none on the anchored one: at the default bottom anchor the background hugged the glyphs' baseline exactly while breathing twice as much above them. What should touch the box edge is the plate — that is what the viewer sees, and what the anchor invariant is stated in terms of. Reserving `pad_y` on the anchored side puts the plate's edge in the same place and makes its padding symmetric again. Conditioned on a plate actually being drawn, on all three backends. Windows was already inset but did it unconditionally, which would have placed its glyphs `pad_y` away from Linux and macOS whenever the background was off; it is now conditional too, so the three agree in both states. **The migration snapped left/right captions to the frame edge.** It returned `insetX: 0` for every document, so a migrated caption whose band sat at 5% moved to 0%. The vertical half already reconstructed the drawn edge exactly; the horizontal half now does the same from the values it had already computed — which is what "reproduce the pixels, not the fields" was supposed to mean. (cherry picked from commit 18f4d09)
rustc rejects `///` on a parameter; it has to be a plain `//`. Only text_macos.rs compiles on macOS, so the machine this was written on could not see it — and rustfmt parses the file happily, so a local syntax gate would not have caught it either. CI did. (cherry picked from commit 2cf1b22)
`patchCaptionSettings` read the document without an aspect, so the write that MATERIALISES the defaults into a project that never had caption settings always wrote the landscape ones. A 9:16 export got `insetY: 5` frozen in, and the stored value then won for good — putting the caption under the platform's own chrome, which is the exact failure the aspect-derived default exists to prevent. `useCaptions` now resolves the aspect once (through `resolveAspectRatioValue`, the same resolver the preview and the scene description use, so all three agree on what a legacy "native" selection means) and hands it to the read and to every write: `set`, `setLive` and the language reset in `deleteTranslation`. Found by CodeRabbit on #482. (cherry picked from commit 096a551)
The anchor redesign's doc rewrite rode along with the guide-overlay commit, which is add-then-revert within the PR and was therefore skipped here. This carries the surviving half: the settings table, and the sections that described the fixed band, the overhang and the preset machinery — none of which exists any more. Applied as one commit rather than cherry-picked because this branch's copy of the file never received #471's docs commit (docs are excluded from the cherry-pick lane), so its lineage differs from main's and the patch does not apply.
… padding 1.5% from the anchored edge, 10% from the horizontal one. Picked by eye against the editor's default padding rather than from a broadcast spec: the footage sits inset inside the frame, so what reads as "just off the edge" is a much smaller number than the 5% BBC states for a full-bleed broadcast frame. Landscape only — those two values were eyeballed on a 16:9 export. Vertical keeps its 12.5%, which answers a different question: TikTok, Reels and Shorts draw their own chrome over the bottom eighth of a 9:16 video, so 1.5% there would put the caption behind a UI. Nobody has looked at that case, so it stays on the conservative value. (cherry picked from commit 7dcd9f0)
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
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.
Cherry-pick of #482 onto
release/v1.10.0, for rc.3.Important
Merge #482 to
mainfirst. This branch carries the same commits, but they are not onmainyet — merging here first would put the release branch ahead ofmain, which inverts the cherry-pick contract intechnical-documentation/engineering/release-and-secrets.md.Why this is a fix, not a feature
rc.2 shipped the caption placement UI from #471, and it was rejected on sight: the
widthslider changed nothing visible, the horizontal position appeared to do nothing, it collided with text-align, changingwidthmoved the caption vertically, and the readout could say-7.3%.All five are one decision: every caption was drawn inside an invisible fixed-height box (22% of frame) with the ink centred in it, while the only visible thing — the background plate — hugs the text. Every control moved that invisible box; none controlled the visible block. This replaces the decision rather than the controls.
What changes
One control per axis, each naming the edge it measures from:
Invariant, asserted as a property test: bottom anchor → the drawn block's bottom edge is at
100 − insetY; top anchor → its top edge is atinsetY. For every font size, background state, word count, wrap outcome and resolution.Deleted:
verticalPosition,offsetY,offsetX,width,textAlign, and the machinery that only existed to compensate for the old geometry.widthbecomes a derived column (BBC line-length table).Native contract: one optional field,
verticalAlign, modelled exactly onspacefrom #396 —Option<String>,#[serde(default)], key omitted when absent, so annotations render byte for byte as before. Export stays iso-preview: one render path feeds both, and captions stay fully native.Perf: the per-cue box goes 22% → 20% of frame height, so the texture gets marginally smaller. Same texture count, same draws, same shader.
Migration
Reproduces the pixels, not the fields: the old band's geometry is known, so the drawn block's edges are recoverable and the nearer one becomes the anchor. A migrated project does not move on screen.
One deliberate exception: line breaks change for a project saved with a non-default
width, because that WAS the wrap column. Unavoidable, and worth a changelog line.Differences from #482, and why
git diffoversrc/,crates/andelectron/: empty.Test plan
npm run test— 181 files / 2160 tests on this branchnpx tsc --noEmit+npx tsc -p tsconfig.test.json --noEmit,npm run i18n:check(13 locales),npm run docs:checkgit diffvs fix(captions): place captions by anchor and margin, not by an invisible band #482 oversrc/,crates/,electron/— empty, so the shipped code is exactly what CI validated there (all three Rust backends green on fix(captions): place captions by anchor and margin, not by an invisible band #482)Known limits
🤖 Generated with Claude Code