Repository navigation
fix(cli): check skips WebGL draws on its box-only seeks so Three.js scenes finish - #5825
Conversation
Edit accuracy: accurate 271 (base branch 271), smooth 180 of thoseThe gate passes. Quarantined, measured but not gated (0) Unstable (1)
|
terencecho
left a comment
terencecho
left a comment
There was a problem hiding this comment.
Reviewed assigned HEAD daf15a46602ed032379bf008ddab894ce6ef99e5 against effective/API base 49671b73dd417f7fe0f3c3f500290df18b447cd0.
No concrete introduced blocker found. I read all changed base files before all changed head files and traced the real dense-geometry caller, overlap collector, runtime transport/adapters, completion boundary, and fresh-page finding crops. Both independent source-review passes reached the same conclusion. In the inspected check flow, pixel sampling precedes the dense geometry pass; the latter reads DOM/text geometry, while the gate restores normal draw forwarding in finally.
Nonblocking test-contract caveat: the new browser fixture compares raw times, whereas the real runtime normally quantizes them. A 1 ms step-off is not always a distinct frame (including at zero), so that fixture does not prove a general next-pixel-seek repaint guarantee. A real-runtime zero/off-grid test would strengthen this contract. I did not establish a production same-page geometry-to-pixel schedule exposing that limitation, so I am not treating it as a served regression.
Source-only review: tests, browser/GL rendering, performance claims, and CI were not executed or independently verified.
— Review by tai (pr-review)
Verdict: APPROVE
Reasoning: The inspected production opt-in is geometry-only and runs after pixel collection, with the normal seek cascade and completion handling retained. No reachable incorrect served result was established; the broader recovery-test limitation is scoped above.
What changes
hyperframes checkno longer runs out of time on compositions with several Three.js or WebGL canvases. It used to stop with "Check did not finish within its deadline." and report nothing.Why it happened
Check's text-overlap pass seeks the timeline about 80 times and reads only element boxes. But every seek runs the composition's own update code, and that redraws each WebGL scene. On software WebGL (no GPU, or
--no-browser-gpu), four 960x540 Three.js scenes cost about 0.9 s per seek, plus about 0.3 s more for the box read that waits behind those draws. That pass alone took about 95 s of check's fixed 120 s budget.The fix
The box-only seek (
DENSE_GEOMETRY_SEEK_OPTIONS) now skips WebGL draw calls (clear,draw*,blitFramebuffer) while the seek runs. It does this through a small gate on the WebGL prototypes, installed by the first such seek. Draws outside that seek are untouched.A skipped draw leaves the canvas stale, and seeking again to the same time redraws nothing: GSAP and the
hf-seekdispatch both treat a repeat time as a no-op. So the next pixel-reading seek after a skipped draw first steps 1 ms off the target with draws still skipped, then lands on it, which repaints the scene. Every pixel-reading seek (layout grid, contrast, finding crops, snapshot) draws exactly as before.Evidence
Fixture: one 1920x1080, 10 s composition with four Three.js canvases (120 torus knots each, shadows, GSAP-driven), software WebGL, same Mac, back to back:
check --no-browser-gpuOn Linux (8-core x86 box, Chrome 152 headless shell, software WebGL), 3 runs each, alternating: 0.8.169 took 51.5 / 50.8 / 51.2 s and this change took 14.0 / 14.9 / 13.9 s. Both passed there; how close a project gets to the 120 s deadline depends on the machine and how heavy the scenes are, and this pass's share of the time is what the change removes.
Per call, before vs after: a box-only seek went from about 900 ms to about 2.5 ms, and the box read after it from about 290 ms to about 1 ms. Findings are unchanged (0 layout issues, 5/5 contrast checks pass).
Tests
captureCompositionFrame.webglDraws.browser.test.tsuses real Chromium with SwiftShader WebGL and a player that, like GSAP, does nothing on a repeat-time seek. It checks:It fails on main (
expected 100 to be +0: the box-only seek drew). With only the step-off removed it fails on the stale canvas (expected +0 to be 100). It passed 3 runs in a row. The existing seek tests pin the page call's arguments, so they now include the new flag.