feat: visual regression guard for the Pixi client - #133
Conversation
Closes #127. Required before WP-08 starts moving real scenes. compare-rendering.mjs gates the legacy canvas client only, so nothing watched the Pixi one. The worked example is the silt regression in #119: silt assigned to the `terrain` layer, below `structure`, put 31 of its 48 particles behind the hull's opaque fill and passed lint, typecheck, unit, parity and e2e. It was found by reading the diff. Reference frames AND scene statistics, because neither is sufficient. Verified rather than assumed: reintroducing the silt regression fails all three scenes on frame delta (max channel delta 35 against a budget of 24) while no statistic breaches at all. Silt behind the hull and silt drawn correctly produce nearly identical frame-wide numbers, which is exactly the case statistics cannot see. Committed PNG references are viable here because Playwright's headless Chromium rasterises through SwiftShader — software, not the GPU — so captures are byte-identical across runs and across browser processes. Measured before building anything: 0 differing bytes. Whether that holds across operating systems is what CI will now tell us; if it does not, the guard says so rather than quietly passing. Determinism needed more than seeding Math.random. GameController.#tick takes the requestAnimationFrame timestamp and advances both the diver's position and `elapsedRealS`, which drives the bubbles, by however long the frame really took — so waiting a fixed number of milliseconds does not reproduce a scene. The first version of this guard failed against its own freshly recorded references with a max channel delta of 128. rAF is now replaced by a queue the harness steps explicitly, with a virtual clock advancing exactly one 60 Hz frame per step, and scenes are driven in frames rather than milliseconds. Two things worth knowing for anyone extending this: - Statistics are computed from the decoded screenshot, not by reading the live canvas back. Drawing a WebGL canvas into a 2D one returns transparent black without preserveDrawingBuffer, and the first version recorded meanLuma 0 for every scene — zero compares equal to zero, so that half of the guard would have passed whatever the renderer did. There is now an explicit check that refuses to record a uniformly black frame. - The capture pins locale to en-US. The boundary copy is localised and the button is matched by name, so without it the run fails on a German desktop and passes in CI. References live in tests/fixtures/reference-frames/pixi/ and are re-recorded deliberately with `npm run pixi:visual-update`, so an intended change lands in the diff as new frames for review rather than as a raised threshold. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CI disproved the premise this guard was built on. I assumed SwiftShader made captures portable, because it is a software rasteriser and captures were byte-identical across runs and across browser processes on one machine. Frames recorded on win32 differ from the same scenes rendered on linux by a max channel delta of 230 across 10.6% of pixels — structural, not antialiasing noise. Deterministic per platform, not across them. Notably no scene statistic breached, so those ARE portable. But statistics alone cannot see the silt regression this guard exists to catch — measured earlier: silt moved behind the hull fails every scene on frame delta while no statistic moves at all. So dropping to statistics-only in CI would leave the thing unguarded. Loosening the budget past 230 / 10.6% would take it well past the silt regression's delta of 35, which is worse than having no guard: it would report green while the class of defect it was built for shipped. References are therefore keyed by platform, and each environment compares against its own set. win32 is committed here. The linux set is seeded by a temporary CI step that records and uploads it on failure, to be committed and the step removed in the next commit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
8b52628 to
07f7d93
Compare
Recorded by CI, since a Windows machine cannot produce them. Statistics landed within a whisker of the win32 set — meanLuma 42.624 against 42.558, channel spread 31.746 against 31.949 — which is the same result CI reported when it disproved cross-platform pixel identity: the scene statistics are portable and the pixels are not. Both platforms are now covered, so each environment compares against frames recorded where it runs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| id: 'wreck-torch-on', | ||
| // The torch is a foreground element drawn over the structure layer. | ||
| frames: 30, | ||
| press: 't', |
There was a problem hiding this comment.
[P2] Capture the torch while it is actually enabled
GameController starts with torchOn=true, and keyboard.press('t') synchronously toggles it false. The committed wreck-exterior-start frame visibly contains the beam, while the frame named wreck-torch-on has no beam and its torch control is unpressed. This dedicated scene therefore exercises torch-off, contrary to its name/comment, and does not provide the claimed unique foreground-layer coverage. Please either capture before toggling (ideally assert aria-pressed=true before the screenshot) or rename/document it as the off-state and make the intended torch-on coverage explicit.
| "baseline:visual-compare": "node scripts/compare-rendering.mjs", | ||
| "pixi:visual-check": "node scripts/pixi-visual-check.mjs", | ||
| "pixi:visual-update": "node scripts/pixi-visual-check.mjs --update", | ||
| "sites:generate": "node scripts/generate-site-resources.mjs", |
There was a problem hiding this comment.
[P2] Make the visual commands build the client they are checking
Both advertised commands consume dist/, but neither npm script builds it; the checker only verifies that dist/index.html exists. Because dist is gitignored and persists locally, npm run pixi:visual-check can report a perfect 0% delta after a Pixi source change while testing an older build, and pixi:visual-update can record references from stale code. CI happens to run build first, but the documented standalone commands are not trustworthy. Please add build prehooks for both check and update (as test:e2e already does) or have the harness build/serve current sources itself.
| run: npx playwright install --with-deps chromium | ||
|
|
||
| - name: Pixi visual regression check | ||
| run: npm run pixi:visual-check |
There was a problem hiding this comment.
[P2] Upload the actual frames when this CI step fails
On a breach the checker writes tests/fixtures/reference-frames/pixi/linux/*.actual.png and tells the reviewer to inspect those files, but neither workflow uploads that path. The runner then exits and the only copy is lost; the existing always-upload steps cover screenshots/ and playwright-report/ only. This is especially blocking for a platform-keyed guard because a Windows maintainer cannot reproduce the Linux pixels locally. Please add an if: failure()/always() artifact step for the actual PNGs (in PR and preferably deploy workflows) so the promised diagnostic is retrievable.
… keep the diff frames Three review findings on #133. [1] `wreck-torch-on` captured the torch OFF. GameController starts with torchOn=true, so pressing `t` disabled it: the scene recorded the opposite of its name and duplicated the coverage it claimed to add, while the two scenes that really do show the beam were unlabelled. Renamed to `wreck-torch-off`, which is the state that earns a scene of its own, and every scene now declares the torch state it expects and asserts it against aria-pressed before capturing. A frame is a frame — without an assertion a mislabelled scene records and compares perfectly clean forever. Claiming the wrong state now fails with "the scene is not exercising what its name says". [2] Both commands consumed dist/ without building it, and the checker only verified that dist/index.html existed. Since dist is gitignored and persists locally, `pixi:visual-check` could report 0% delta against an older build after a Pixi source change, and `pixi:visual-update` could record references from stale code. Both now have build prehooks, matching pretest:e2e. [3] On a breach the checker writes <scene>.actual.png beside its reference and tells the reviewer to look at it, but neither workflow uploaded that path, so the runner exited and the only copy went with it. That matters more than usual here because the references are platform-keyed: a maintainer on another OS cannot reproduce the failing pixels locally. Both workflows now upload them on failure. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
All three confirmed and fixed —
|
|
Rereviewed 635fac4: no remaining findings. The standalone visual command now runs its build prehook, all three scenes assert their declared torch state, the renamed torch-off references compare at 0% changed pixels, and both workflows preserve *.actual.png diagnostics on failure. Current CI is green. |
Closes the last item on #124. The art direction was settled and both halves implemented (#135 cave, #136 wreck); this is the guard that keeps them, and it is the piece the issue asked be done LAST, deliberately, so the thresholds describe the intended look rather than the shortfall. npm run interior:check verify (CI-gated in pr.yml and deploy.yml) npm run interior:update re-record the reference FRAMES, deliberately Nine scenes: four wreck interiors, three cave interiors, two open-water controls. Each has a band on per-pixel chroma and one on mean luminance. WHY THRESHOLDS ARE CODE AND FRAMES ARE DATA --update re-records the PNGs and does not touch the thresholds. That split is the whole point of #124's sequencing warning: thresholds derived from what is currently on screen describe the current state, so recording them while the current state is the shortfall locks the shortfall in. The bands therefore live in the script with their derivation attached, and moving one is a deliberate edit visible in review rather than a side effect of a red run. HOW THE FLOORS WERE PLACED Each floor sits midway between the scene's measured flat state and its measured fixed state — the placement with the most headroom either side that still fails the regression it exists to catch: scene flat -> fixed floor catches headroom wreck-vehicle-deck 26.4 -> 35.5 31.0 +4.6 -4.5 wreck-crew-deck 22.6 -> 31.7 27.0 +4.4 -4.7 wreck-cargo-hold 19.1 -> 27.8 23.5 +4.4 -4.3 wreck-engine-room 14.3 -> 21.1 17.5 +3.2 -3.6 cave-upper-tunnel 10.1 -> 17.8 14.0 +3.9 -3.8 cave-restriction 12.2 -> 19.4 15.5 +3.3 -3.9 cave-cathedral 16.6 -> 23.6 20.0 +3.4 -3.6 Both columns were measured by this script by reverting the two gloom colours and re-running it, so every floor is calibrated against a real state of this codebase rather than an estimate. Verified three ways: clean passes all nine; reverting #136 fails exactly the four wreck interiors; reverting #135 fails exactly the three cave ones. Controls hold in both. Run-to-run noise is under 0.1 across three consecutive full runs, so the whole 3.2-4.7 of headroom is there for cross-platform drift, which could not be measured — CI is linux and there is no linux node here. If linux sits further out than that, the first run says so with the number, and the fix is to widen the one band that moved. TWO CONTAMINANTS THE CROP HAD TO BE MOVED OFF #124's baseline came with a caveat that its crop still caught fixed chrome, and a note to tighten it before deriving thresholds. Doing that turned up two, and both were found by measurement rather than by eye, which is the part worth keeping: The toasts are painted onto the CANVAS, not the DOM, at y fractions 0.18, 0.30 and 0.36 — amber text, the highest-chroma pixels in frame, with the hint line refilling from a queue on a timer. They were worth 0.6-0.9 chroma and 6 SD. Suppressed at source, and the run now asserts they are silent; clearing them from one evaluate and measuring from the next left room for a frame in between that re-armed one, which is why settle, silence and read are now a single evaluation. The dive computer looks like DOM and is not: it is on the canvas at x >= 880, and a window running to 0.80 had its corner inside, worth 1.79% of the measured pixels. The window is 0.48 wide instead. Rather than trust a crop that looked right once, assertNoChrome now proves it every run: two scenes sharing no scenery (wreck-engine-room, reef-open-water) cannot legitimately agree pixel for pixel, so anything byte-identical across both is chrome. Measured 0.03%, against a 0.5% budget and the 1.79% the gauge corner cost. That is what stops this crop rotting the next time the HUD moves — #125 alone reflowed the result screens and grew the touch targets. Moving the window below the toasts instead was tried and rejected: at that offset it slides off the interior onto the seabed, taking the engine room's contrast from SD 17.1 to 7.9. WHY THE FRAMES ARE NOT BYTE-COMPARED #133 established Playwright frames are deterministic per platform but not across them. There is no linux set here, so the frames are committed as review artefacts — cut from the same canvas pixels the statistics read, so the picture and the numbers beside it cannot disagree, and sized to the sample window, which also keeps nine frames from outweighing the rest of the repo's history. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…124) The thresholds were derived on win32 and CI runs linux, and there was no linux node here to check the difference — so the headroom was sized for a drift that could only be reasoned about. The first CI run measured it. wreck-vehicle-deck 35.5 -> 35.5 cave-upper-tunnel 17.8 -> 17.7 wreck-crew-deck 31.7 -> 31.8 cave-restriction 19.4 -> 19.3 wreck-cargo-hold 27.8 -> 27.8 cave-cathedral 23.6 -> 23.6 wreck-engine-room 21.1 -> 21.6 wreck-exterior-bow 113.1 -> 114.3 reef-open-water 54.2 -> 54.1 At most 0.5 on any interior and 1.2 on a control, against 3.2-4.7 of headroom. This confirms the design choice rather than just permitting it. #133 found Playwright frames differ across platforms by a max channel delta of 230 across 10.6% of pixels, which is why it keeps a reference set per platform and why this guard enforces statistics instead. Aggregating over ~63k samples averages those per-pixel rasteriser differences away: the numbers travel even though the frames they came from do not. The practical consequence is worth stating where the bands are. The headroom is no longer mostly reserved for an unmeasured platform gap — it is slack for future art changes that are meant to happen, so a band that starts failing is far more likely to be a real change than an environment. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The previous commit removed the win32/linux comparison table because the
harness fix had shifted the absolutes it was measured on, leaving only the
conclusion. CI has now re-run it on the corrected harness, and the result is
better than what it replaced:
wreck-vehicle-deck 35.5 / 35.5 cave-upper-tunnel 17.5 / 17.5
wreck-crew-deck 31.7 / 31.7 cave-restriction 19.2 / 19.2
wreck-cargo-hold 27.9 / 27.9 cave-cathedral 23.6 / 23.6
wreck-engine-room 22.5 / 22.5 wreck-exterior-bow 116.2 / 116.1
reef-open-water 53.9 / 53.9
Every interior agrees exactly and the controls to within 0.1, on renderers
whose individual pixels differ by a max channel delta of 230 across 10.6% of
them (#133). Aggregating over ~63k samples averages the rasteriser differences
away completely, which is the assumption this guard is built on.
The earlier measurement of up to 0.5 drift on interiors and 1.2 on a control
was not the platforms. It was this script letting the diver drift slightly
between pinning position and reading pixels; re-pinning every frame — done for
the overhead-assertion fix, not for this — removed it. Recorded at the bands,
because it inverts the natural assumption: when these numbers move, suspect the
harness before the environment.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Closes #127. Required before WP-08 starts moving real scenes.
compare-rendering.mjsgates the legacy canvas client only, so nothing watched the Pixi one. The worked example is the silt regression in #119 — silt assigned toterrain, belowstructure, putting 31 of its 48 particles behind the hull's opaque fill. It passed lint, typecheck, unit, parity and e2e, and was found by reading the diff.Frames and statistics, verified rather than assumed
You said statistics alone can miss displaced or missing objects. That turned out to be exactly measurable: reintroducing the silt regression fails all three scenes on frame delta (max channel delta 35 against a budget of 24) while no statistic breaches at all.
Silt behind the hull and silt drawn correctly produce nearly identical frame-wide numbers. Statistics would have shipped it.
Why committed PNGs work here
The usual reason pixel baselines are unusable is GPU variance. Playwright's headless Chromium rasterises through SwiftShader — software, not the GPU:
Measured before building anything: 0 differing bytes, same browser and across fresh browser processes.
Whether that holds across operating systems is the open question, and this PR is the experiment — CI runs on Linux against frames recorded on Windows. If it does not hold, the guard says so on this PR rather than passing quietly, and the honest response is a platform-aware baseline rather than a loosened threshold.
Determinism needed more than seeding Math.random
GameController.#ticktakes the requestAnimationFrame timestamp and advances both the diver's position andelapsedRealS— which drives the bubbles — by however long the frame really took. Waiting a fixed number of milliseconds does not reproduce a scene.The first version of this guard failed against its own freshly recorded references with a max channel delta of 128. rAF is now replaced by a queue the harness steps explicitly, with a virtual clock advancing exactly one 60 Hz frame per step, and scenes are driven in frames rather than milliseconds. A guard that is flaky by construction is worse than none.
Two traps worth knowing about
Statistics come from the decoded screenshot, not a canvas readback. Drawing a WebGL canvas into a 2D one returns transparent black without
preserveDrawingBuffer. The first version recordedmeanLuma 0for every scene — and zero compares equal to zero, so that half of the guard would have passed whatever the renderer did. There is now an explicit check that refuses to record a uniformly black frame.The capture pins locale to
en-US. The boundary copy is localised and the button is matched by name, so without it the run fails on a German desktop and passes in CI.Using it
An intended change lands in the diff as new frames for review rather than as a raised threshold. A failing scene writes
<scene>.actual.pngbeside its reference for eyeballing; those are gitignored.Scope
Three scenes, each exercising a layer the others do not: as-mounted, after a descent (culling and marker pooling resynced), and torch on. The Pixi client is currently the wreck exterior shell only —
SITE_IDis hardcoded towreck— so that is all there is to cover today. The set grows with WP-08.A correction
I previously advised doing this alongside #124 because they "share the reference-frame artefact". That was wrong, and I have corrected it on #124: this is the Pixi client, #124 is the legacy canvas client, and they share no scenes or files. Batching them would have meant one PR touching two renderers for unrelated reasons.
Verification
typecheck,lint,sites:check,build, 133 unit, 28 parity, 36 e2e, and the guard passing against its own references at 0% pixels changed.