Repository navigation
[Fix] Let gallery picks and auto-advance actually re-render - #117
Conversation
The gallery advanced its counter and rewrote the URL, but the shader never changed: restartMatrixWithNewConfig() looked for a renderer 'updateConfig' export that did not exist, so it was a no-op with only a reassuring log line behind it. Its fallback was removed in 2025 on the assumption that renderers are reactive to config changes - they are not, config is baked into each pass as it is built. - js/webgl, js/webgpu, js/three-rain, js/p5-rain: track the live session and export updateConfig(), which tears down the frame loop, listeners and GPU resources and rebuilds the pipeline on the same canvas. regl frees its resources on destroy() without losing the WebGL context, so the canvas can be handed straight to a new instance. - js/main.js: serialize restarts (the gallery can advance mid-switch) and log an error instead of failing silently. - js/webgpu/main.js: the fullscreen/resize handlers referenced canvas, config and pipeline from outside their scope - a ReferenceError on the first resize. - js/webgl/bloomPass.js: the no-bloom path handed the palette pass an uninitialised 1x1 texture, so it sampled recycled GPU memory and washed the whole screen flat grey. Zero it once. - js/gallery.js: a throw while displaying an item left the switch timer unset and froze the slideshow permanently; always reschedule. - tests/matrix-gallery.spec.js: asserts a pick and an auto-advance each rebuild the renderer and flip the frame from the green classic palette to the red nightmare one. Both fail without this change.
🎬 PR Preview Deployed!Your changes are now available for testing: Preview URL: https://ap0ught.github.io/matrix/pr-117/ Test Links:The preview will be updated automatically when you push new commits. Preview deployed from commit 5659fdf |
![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: 5659fdf50b
ℹ️ 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".
| if (canvas.width !== canvasWidth || canvas.height !== canvasHeight) { | ||
| canvas.width = canvasWidth; | ||
| canvas.height = canvasHeight; | ||
| outputs = pipeline.build(canvasSize); |
There was a problem hiding this comment.
When a WebGPU screensaver switch rebuilds the renderer with the same resolution, the canvas already has the requested dimensions, so this condition remains false and the newly created pipeline never receives pipeline.build(). Its passes then run with undefined textures and bind groups (for example, rainPass calls output.createView()), leaving the first same-size auto-switch broken. Track build state independently of the persistent canvas dimensions or build the pipeline unconditionally once after creation.
Useful? React with 👍 / 👎.
| if (typeof currentMatrixRenderer.updateConfig === "function") { | ||
| await currentMatrixRenderer.updateConfig(newConfig); |
There was a problem hiding this comment.
For automatic screensaver changes, newConfig.renderer can differ from the active module: getAvailableModes() includes mathcode_alphabet_three and mathcode_p5, whose configs select the Three.js and p5 renderers. Calling the existing module's updateConfig() means a WebGL session renders those presets through WebGL, while a session started in p5 or Three.js never returns to WebGL for ordinary presets. The restart path needs to dispose the current module and import/start the renderer selected by newConfig.renderer when that value changes.
Useful? React with 👍 / 👎.
| document.removeEventListener("webkitfullscreenchange", recalcOnFullscreenChange); | ||
| document.removeEventListener("mozfullscreenchange", recalcOnFullscreenChange); | ||
| document.removeEventListener("MSFullscreenChange", recalcOnFullscreenChange); | ||
| cleanupFullscreen(); |
There was a problem hiding this comment.
When a gallery or screensaver switch occurs while the canvas is fullscreen, this cleanup invokes the fullscreen utility's releaseWakeLock(). The replacement renderer merely installs new listeners; because no new fullscreenchange event occurs, it does not reacquire the lock, allowing the display to sleep during the remainder of the fullscreen session. Renderer replacement should retain the active fullscreen wake lock or explicitly reacquire it after setup.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Startup races, unrecoverable rebuild failures, and camera-resource leaks remain.
Review effort: Balanced
Findings: 1
Open (5)
What changed in this PR
Fixes gallery selections and auto-advance by rebuilding active renderers while improving teardown reliability.
Changes:
- Adds restart support across all renderers.
- Serializes configuration changes and hardens gallery scheduling.
- Initializes disabled bloom textures and adds gallery rendering tests.
| File | Description |
|---|---|
js/main.js |
Serializes renderer restarts. |
js/gallery.js |
Reschedules gallery progression after failures. |
js/webgl/main.js |
Adds WebGL teardown and rebuilding. |
js/webgl/bloomPass.js |
Clears disabled bloom output. |
js/webgpu/main.js |
Adds WebGPU rebuilding and scoped handlers. |
js/three-rain/main.js |
Adds Three.js session lifecycle. |
js/p5-rain/main.js |
Adds p5 session lifecycle. |
tests/matrix-gallery.spec.js |
Tests selection and automatic advancement. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| .catch((error) => { | ||
| console.error("[Matrix] Failed to switch renderer config:", error); | ||
| }); |
| /* | ||
| * Whatever happens while displaying an item, the slideshow keeps moving: a thrown | ||
| * callback (renderer restart, missing screenshot) must not leave the timer unset and | ||
| * freeze the gallery on one entry forever. |
| * previous switch is still building its pipeline, and two overlapping teardown/init pairs | ||
| * would leave a half-torn-down renderer attached to the canvas. | ||
| */ | ||
| let restartChain = Promise.resolve(); |
| session.dispose(); | ||
| return startWebGLMatrix(session.canvas, config); |
| session.dispose(); | ||
| return startWebGPUMatrix(session.canvas, config); |
Summary
Gallery mode advances its counter and rewrites the URL, but the shader never changes — picking an item does nothing and the auto-cycle is invisible. Two bugs, plus a latent one the first one was hiding.
1.
restartMatrixWithNewConfig()was a no-op (both symptoms)It looked for a renderer
updateConfig()export that no renderer had, so every mode switch ended at aconsole.log("Matrix restarted with: …"). The fallback that used to cover that was deleted in 2025 on the assumption that renderers "are reactive to config changes" — they are not: each pass readsconfigas it is built, so a new config needs a new pipeline.Every renderer now tracks its live session and exports
updateConfig(), which tears down the frame loop, listeners and GPU resources and rebuilds on the same canvas:js/webgl/main.js—regl.destroy()frees its resources without losing the WebGL context, so the canvas can go straight to a new regl instance (verified before relying on it). Per-pipeline size bookkeeping moved inside the start function, otherwise a fresh pipeline would never resize its passes when the viewport matches the old one.js/webgpu/main.js— same, plus its fullscreen/resize handlers referencedcanvas,config,pipelineanddevicefrom outside their scope, so they threw aReferenceErroron the first resize or fullscreen change.js/three-rain/main.js,js/p5-rain/main.js— same, disposing the three.js renderer / removing the p5 sketch so rebuilds don't stack two draw loops or canvases.js/main.js— restarts are serialized (the gallery can advance mid-switch), and a renderer that can't restart now logs an error instead of failing silently.2. No-bloom modes washed the screen flat grey
bloomPassreturns a no-op pass withmakePassFBO(regl)whenbloomSize/bloomStrengthare unset — which is the case for the defaultclassicversion, since those keys only exist in some version blocks. That 1x1 texture has uninitialised storage, so once other GL allocations have recycled that memory the palette pass adds garbage to every pixel and the screen goes uniformly grey. Zero it once at creation.3. A throw could freeze the gallery forever
showNextItem()armed the next switch after displaying an item, so any exception in between left the timer unset and the slideshow stuck on one entry. It now always reschedules.Testing
tests/matrix-gallery.spec.js(new): asserts a playlist pick and an auto-advance each rebuild the renderer and flip the rendered frame from the green classic palette to the red nightmare one.Math.randomis stubbed so the playlist order is deterministic; the frame is read withreadPixelson the canvas' own GL context, because a page screenshot of a WebGL canvas can come back mid-grey while it animates. Both tests fail on the old code and pass on this one.npm test— 19 passed, 2 skipped (WebGPU unavailable here)npm run test:regression— 224 passedManual: gallery picks and auto-advance verified in Chrome;
renderer=threeandrenderer=p5each survive three rebuilds without errors or stacked canvasesnpx prettier --check --use-tabs --print-width 160onjs/**andtests/**Tested on Chrome (Linux/AMD)
Tested classic variant
Tested 3D variant
Tested on Firefox
Tested on mobile
Performance validated
Related Issues
Fixes #116