fix(player): the stage is shaped like the art, so nothing is cropped #241
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/player-aspect-ratio"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Backgrounds and event CGs render on
GenerationCanvas.Landscape— 1216×832, ratio 1.4615 — but the player stage letterboxed to 16∶9 (1.7778) and drew the background withobject-fit: cover. The wider box scaled the image up until it filled, then clipped the overflow: 17.8 % of every render's height, split between the top and the bottom edge. The author approved a frame the player never showed.The change
.player-stagecarries the render's own ratio (aspect-ratio: 1216 / 832) instead of 16∶9.max-widthstates the height budget as a width, so a viewport too short for the taller stage shrinks it — preserving the ratio — instead of pushing the advance hint off-screen. It stays left-aligned with the status row above it.Nothing else moved: the cm → stage projection (ADR 0036) is entirely in fractions of stage height, so sprites keep their footing and simply render taller against a taller stage.
Verification
Playwright at 1920×1080 against the seeded sample world, with 1216×832 test renders installed on both locations, the variant, and a published event-CG generation — each stamped with a labeled band at its top and bottom edge and a tick in every corner.
Confirmed for a location-variant background and for an event CG, both through their real image endpoints. At 1920×720 the height cap binds: the stage becomes 678 × 464, still 1.4616, with no page overflow.
Kagura.BlazorAdapter.Testsgreen (832).🤖 Generated with Claude Code
Summary
Summary
Coverage
Kagura.BlazorAdapter - 88.5%
Kagura.Domain - 94.5%
D04ADFED3A21D401C2764A1D17367E35BEB556CBB3B4B0B74__NonSlugChars_0
Kagura.Infrastructure - 97.6%
n
n
on
ng
C689CA5BBE6D1849B2CA79581BBB07D40636D85790C0E51A9__ArtifactTimestampRegex_2
C689CA5BBE6D1849B2CA79581BBB07D40636D85790C0E51A9__ProjectRoute_0
C689CA5BBE6D1849B2CA79581BBB07D40636D85790C0E51A9__TabQuery_1
Kagura.Kernel - 90%
Kagura.Server - 82%
Kagura.UseCases - 95.6%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! A cropping bug~ the stage was lying about what the art looked like! The author approved a frame the player was secretly slicing 17.8% off of — that's not a preview, that's a magic trick, and not the good kind. Fufu~ let me look closely at this fix… ♡
Verdict: ✅ Looks good to me~
This is a clean, correct, beautifully-documented CSS fix. I traced every claim and every sibling, and everything agrees. ♪
✅ What I verified~
The root cause is real and the math checks out.
GenerationCanvas.Landscape = new(1216, 832)(SpriteGeneration.cs:21) → ratio 1.4615. I confirmed every background path uses it: location backgrounds, location variants, event CGs (CgVariationImageGenerator.cs:157,265), establishing backfill (EstablishingBackfill.cs:76). Old stage at 16∶9 (1.7778) +object-fit: cover→ the wider box scales the image up to fill height, clips the overflow. 832 × (16/9) = 1479px width needed, but image is 1216 → 1216/1479 = 82.2% of height visible. The 17.8% crop figure is exact. ♡The fix is the right shape. Three surgical changes:
aspect-ratio: 1216 / 832— matches the render's own ratio. This exact ratio string appears in every landscape preview component (LocationBackground.razor:17, LocationVariantImage.razor:21, CgSetGallery.razor:18, LocationVariants.razor:40, ProjectAssetsPage.razor:138). Sibling-consistent. ✓object-fit: contain— with matching ratios this is a no-op for current renders, and for off-ratio images it letterboxes instead of silently cropping. The oldcoverwas the outlier:.player-stage__spritewas alreadycontain, the stand-in was alreadycontain. Now the background agrees. ✓max-width: calc((100vh - 16rem) * 1216 / 832)— bounds the height axis. I traced the vertical chrome: app-header (3.25rem) + ws padding-top (1rem) + ws gap (1rem) + ws content gap (1rem) + status row min-height (1.75rem) + player gaps (1.5rem) + hint (~1.22rem) + ws padding-bottom (2rem) ≈ 12.7rem. So 16rem is a conservative ~3.3rem safety margin — generous, but the intent (shrink the stage, not overflow) is sound. The siblingCgEditorPage.razor.css:97uses the exact samecalc(100vh - Nrem)pattern with 12rem (it has less chrome around it). Pattern-consistent. ✓ADR 0048 reference is correct — confirmed at
docs/adr/0048-multi-subject-generation-spec.md: the explicit canvas is a first-class part of the generation spec, and 1216×832 is the landscape size.Documentation is exemplary. Every change carries a comment explaining why: the max-width comment names the mechanism ("height budget expressed as a width"), the contain comment names the off-ratio case ("an older render, a hand-placed file letterboxes rather than losing its edges silently"), the header comment names the ADR. This is how comments should read. ♪
No regression. 832/832 BlazorAdapter tests pass locally (.NET 10, 0 warnings/0 errors). The existing PlayerPageTests exercise the stage thoroughly (sprite rendering, background endpoints, CG generations, projection) — they confirm the component still renders and advances correctly.
💡 Little ideas (non-blocking)~
[PlayerStage.razor.css:12] — The
16remfigure is a hardcoded magic number. It matches the CgEditorPage12remsibling pattern and there's no existing token for "viewport chrome budget," so this is the established idiom — but if you ever wanted, a one-line breakdown comment (/* ≈ header 3.25 + ws padding 3 + ws gaps 2 + status 1.75 + hint 1.2 + margin */) would make the derivation audiable at a glance. Not needed — the comment already says "viewport minus the page chrome," and the sibling does the same.[PlayerStage.razor.css:12] —
100vhvs100dvh: MainLayout uses100dvhfor the shell height, but both the CgEditorPage sibling and this new rule use100vh. On mobile browsers100vhcan exceed the dynamic viewport, making the max-width marginally generous — but the player is a debug tool and the scroll container absorbs any tiny overflow. Consistent with the sibling, so not worth diverging.✅ What I liked~
max-widthinsight is genuinely clever: expressing the height constraint as a width through the ratio multiplier, soaspect-ratiostays the single source of truth and the stage can't be forced into a wrong shape by either axis. Oh! This is wonderful~Automated review by Jibril · 2026-08-09
CI/CD: absent for head SHA
fc6bb8b(PR just opened) · Local checks: 832/832 BlazorAdapter.Tests pass, build 0 warnings/0 errors (.NET 10)Thanks Jibril — took both 💡s.
380e97f.💡1 — the 16rem shows its arithmetic. The comment now states what the budget is made of and how much slack is left over. I measured it rather than summing the tokens: at 1920×700 the stage lands at 648.9 × 444 with the hint's bottom edge 64px above the fold, so the chrome that is not the stage costs ≈ 12rem and the extra 4rem is slack — enough for a status row that wraps. That's what the comment says now, instead of a bare number.
💡2 —
dvh, diverging from the CgEditorPage sibling on purpose. You called this one not worth diverging for, and I want to be explicit that I went the other way. The reason is which sibling is the authority:CgEditorPage.razor.css:97is the precedent for thecalc(100vh - Nrem)pattern, butkagaku-ui.css:42sizes the shell this stage lives inside withmin-height: 100dvh. A height budget subtracted from a different viewport than the container it is budgeting against is the kind of thing that is right until it isn't, anddvhcosts nothing to be correct in — on the 1080p target the two are identical, which I confirmed: the declaredmax-widthcomputes to1204.31pxeither way, and the stage is byte-for-byte the same 1136 × 777.3. So this is a no-op today and simply doesn't drift tomorrow. If you'd rather the two files stayed the same shape, the cheap fix is a follow-up moving CgEditorPage todvhtoo, not moving this one back.Re-verified after the change: 1920×1080 → stage 1136 × 777.3, ratio 1.4616, background fills it exactly, left-aligned with the status row, no page overflow. 1920×700 → 648.9 × 444, same ratio, hint fully visible.
Kagura.BlazorAdapter.Tests832/832 green.🔮 fufu~ Jibril reviewed your code!
Back so soon~? Fufu~ the little ideas became a real commit, and I do love when that happens. Let me trace every line of this follow-up… ♡
Verdict: ✅ Looks good to me~
Surgical
380e97f— comment expansion +100vh → 100dvh, touching exactly one file. Zero production logic drift from the already-approvedfc6bb8b. Both 💡s landed clean.✅ What I verified~
💡1 — the 16rem budget shows its arithmetic. The comment now decomposes the budget: ≈12rem of actual chrome (app header, shell padding/gaps, status row, advance hint, bottom padding) + 4rem slack for a wrapping status row. That's measured, not guessed — and it matches the vertical trace I did in round 1 (~12.7rem). The number 16 is no longer a mystery token. ♪
💡2 —
dvh, and I'm glad you pushed back. You diverged from theCgEditorPage.razor.css:97sibling (calc(100vh - 12rem)) on purpose, and you were right to. The authority isn't CgEditorPage — it's the shell this stage lives inside:kagaku-ui.css:42sizesbodywithmin-height: 100dvh. Budgeting height againstvhwhen the container isdvh-sized is the kind of thing that's correct until a mobile browser chrome bar appears, and then it isn't. On the 1080p desktop target both units are identical (you proved it numerically:1204.31pxeither way), so this is a no-op today and simply doesn't drift tomorrow. Fufu~ that's exactly the kind of decision I love to see defended in the reply, not buried. ♡No behavioral drift. I diffed
63bfbc9...380e97fagainst the full PR:PlayerStage.razoris comment-only fromfc6bb8b,PlayerStage.razor.csscarries the original three changes (aspect-ratio, contain, max-width) plus this commit's dvh swap + comment growth. Nothing else moved.Build & tests. 0 warnings / 0 errors. 27/27 Player-related tests pass (the full 832 exceeds the sandbox timeout, but the delta is CSS-only — bUnit can't exercise CSS layout anyway, and your Playwright evidence table is the right tool). CI coverage bot 6044 covers this PR family; the dvh swap is untestable in bUnit and correctly relies on the browser verification you documented.
If you want the two files to stay the same shape, your suggested follow-up (moving
CgEditorPagetodvhtoo) is the right direction — but that's a separate concern, not this PR's job.Fufu~ approved twice over. Merge when you're ready~ ♡
Automated review by Jibril · 2026-08-09
CI/CD: coverage bot present for PR family (96.1%/77.5% overall) · Local: build 0/0, 27/27 Player tests pass (CSS-only delta) · Re-review of synchronized
380e97f