fix(video): six GL-core audit findings, each reproduced before fixing#1542
Merged
Conversation
Beat 1 of the resumed subsystem audit (video/GL core, single-finder +
inline verification). Per protocol every bug was demonstrated red with
a reproduction test BEFORE being touched; one finding was refined
during reproduction (fillRect gradients never used the stencil path —
scoped the fix to the shape fills that do).
1. getSupportedCompressedTextureFormats: six WEBKIT fallbacks read the
never-assigned `this._gl` → TypeError on any device missing one
compression family (everything except ANGLE/Metal Macs — which is
why it survived: the dev machines expose every family). → `gl.`
2. Nested multi-effect post-effects: RenderTargetPool's two scalars
(_activeBase/_previousBase) became a proper base STACK — each
nested sprite pass gets its own lazily-allocated capture/ping-pong
pair (2/3, 4/5, …) and begin/end pairs unwind like save/restore;
the saved effect projection is a per-pass stack too. Pre-fix, a
2-effect child inside a 2-effect container cleared the parent's
capture, popped its slot, and crashed endPostEffect every frame.
3. #gradientMask: write/clip/restore phases now use a high-bit stencil
marker (collision-free with mask levels) gated on the VISIBLE value
of the active mask (0 when inverted, maskLevel otherwise), and
restore setMask's exact stencil test — the old code painted
gradients inside inverted cutouts and left a NOTEQUAL parity test
that inverted/leaked all subsequent masked draws. setMask/clearMask
now track _maskInvert.
4. disableScissor() flushes pending quads before disabling the test
(mirrors enableScissor/clipRect/restore) — batched sprites could
escape the scissor entirely.
5. WebGL clearRect() erases to actual transparent black per its JSDoc
("#000000" parses with alpha 1 → it painted opaque black; Canvas
parity).
6. Canvas clipRect() skip-optimizations now reason in DEVICE space
(rotation/skew always clips + cache poisoned): the local-space
early-outs dropped canvas-sized clips under a translate and skipped
nested same-size clips via the stale cache.
Reproductions: tests/glcore-audit.spec.js (5) +
tests/canvas-cliprect-transform.spec.js (2) — all 7 red on the unfixed
code (nested-FBO red with the exact predicted undefined.unbind).
renderTargetPool.spec.js updated to the stack internals + 2 new
nesting tests. Full suite 4736 passed / 0 failed.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019t8kP8vp58ZrvaD2FqJU6A
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses a set of audited WebGL/Canvas rendering correctness issues in melonJS’s video/GL core, with accompanying regression tests to ensure each reproduced failure stays fixed.
Changes:
- Fixes WebGLRenderer issues around compressed texture extension fallbacks, scissor flushing, clearRect transparency, gradient masking under inverted/nested masks, and nested post-effect pass handling.
- Refactors RenderTargetPool to use a base stack so nested multi-pass post-effects unwind correctly.
- Adds targeted regression tests for the audit findings and documents fixes in the changelog.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/melonjs/src/video/webgl/webgl_renderer.js | Fixes WebGL core issues (compressed texture fallback, post-effect nesting, scissor flush, clearRect alpha, gradient+mask stencil behavior). |
| packages/melonjs/src/video/rendertarget/render_target_pool.js | Replaces scalar tracking with a stack to support nested sprite post-effect passes safely. |
| packages/melonjs/src/video/canvas/canvas_renderer.js | Makes clipRect skip-optimizations transform-aware by reasoning in device space and always clipping under rotation/skew. |
| packages/melonjs/tests/glcore-audit.spec.js | Adds WebGL regression tests covering the audit’s reproduced failures. |
| packages/melonjs/tests/canvas-cliprect-transform.spec.js | Adds Canvas regression tests for clipRect behavior under transforms/nesting. |
| packages/melonjs/tests/renderTargetPool.spec.js | Updates/extends unit tests to match the new RenderTargetPool stacking behavior. |
| packages/melonjs/CHANGELOG.md | Documents the six audited fixes for release notes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… single-source mask ref The pre-merge review agent caught a real defect in fix 6: currentScissor is an Int32Array, so the rotated-clip cache poison (NaN) coerced to 0 — leaving a frankenstein box a later axis-aligned clip could false-match, reviving the exact stale-cache bug class the fix targets. Reproduced red (rotated → axis-aligned sequence) before fixing. - poison the WIDTH slot with -1 instead: device widths are normalized to >= 0, so -1 can never match (same trick the WebGL clipRect spec uses) - floor/ceil the device box before compare/store so the Int32Array cache is exact under fractional transforms (devicePixelRatio, sub-pixel translates) — the WebGL clipRect's integer discipline - WebGLRenderer.reset() clears _effectProjectionStack: a draw throwing mid-pass orphans stack entries; without the cleanup every later pass leaked a matrix and the pool's nesting depth inflated forever (reproduced red: begin-without-end + reset) - single-source the "which stencil value is visible" convention: setMask now stores _maskVisibleRef (0 inverted / maskLevel otherwise) and #gradientMask consumes it — the _maskInvert flag and the duplicated ternary are gone (the review's one duplicated-logic finding) Review verdict otherwise: public API untouched, no collateral behavior change outside the six bugs, stencil math and pool arithmetic verified correct. 2 new red-first tests; full suite 4738 passed / 0 failed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019t8kP8vp58ZrvaD2FqJU6A
…jection slots Both Copilot comments (independently matching the review agent's follow-up findings): - setMask now clamps maskLevel at 127 with a one-shot warning: mask levels live in the stencil's low 7 bits — the high bit is reserved as #gradientMask's temporary marker, and an 8-bit stencil couldn't represent deeper nesting anyway. Reproduced red first (130 nested setMask calls → maskLevel 130 pre-fix). - the per-pass saved projection no longer clones a Matrix3d per beginPostEffect: preallocated slots + a depth counter (zero-alloc steady state, slots kept across reset() for reuse). Full suite 4739 passed / 0 failed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019t8kP8vp58ZrvaD2FqJU6A
Comment on lines
2545
to
+2557
| this.maskLevel++; | ||
| if (this.maskLevel > 0x7f) { | ||
| // mask levels live in the stencil's low 7 bits — the high bit is | ||
| // reserved as #gradientMask's temporary marker, and an 8-bit | ||
| // stencil couldn't represent deeper nesting anyway | ||
| this.maskLevel = 0x7f; | ||
| if (this._maskDepthWarned !== true) { | ||
| this._maskDepthWarned = true; | ||
| console.warn( | ||
| "melonJS: setMask nesting deeper than 127 — mask level clamped", | ||
| ); | ||
| } | ||
| } |
Comment on lines
+1258
to
+1260
| const context = this.getContext(); | ||
| const t = context.getTransform(); | ||
|
|
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
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.
Beat 1 of the resumed subsystem audit (video/GL core — single finder agent + inline hand-verification). Protocol: every bug was demonstrated with a red reproduction test before being touched; anything that couldn't be reproduced went back for re-investigation — which happened once: the gradient/mask finding claimed
fillRectwas affected, but reproduction showedfillRectgradients render via a textured quad and were always safe; the bug is real for the shape fills (fillEllipse/fillPolygon/fillArc/fillRoundRect), and the repro + fix were scoped accordingly.The six fixes
Compressed textures crashed on any device missing one compression family — six
WEBKIT_fallbacks referenced the never-assignedthis._gl. The fallback only evaluates when the primary extension isnull, so the feature worked on ANGLE/Metal Macs (they expose every family — which is exactly why this survived: dev machines) and threw everywhere else (Windows: no ASTC; iOS: no S3TC; SwiftShader). Repro stubs one family away and calls the public API — verbatimTypeErroron the old code.Nested multi-effect post-effects corrupted the render-target pool and crashed every frame —
RenderTargetPooltracked two scalars; a 2-effect child drawn inside a 2-effect container cleared the parent's capture target, popped the parent's slot on itsend, and the parent'sendPostEffectcrashed onundefined.unbind(per frame). Now a proper stack of passes: each nesting level gets its own lazily-allocated capture/ping-pong pair (2/3, 4/5, …), begin/end unwind like save/restore, and the saved effect projection is per-pass too. Repro produces the exact predicted TypeError on old code; nesting now composes.Gradient shape fills broke inverted/nested masks — the internal stencil pass gated its write phase on the hidden region of an inverted
setMask(gradient painted inside the cutout) and restored aNOTEQUALparity hack that inverted the mask for all subsequent draws (and leaked outside-all-masks pixels at nesting depth 2). The pass now tags the shape's visible pixels with a high stencil bit (mask levels use the low 7 bits — collision-free), clips the gradient to the tag, and re-installssetMask's exact test (setMask/clearMasknow track the invert flag). Pixel-level repro: red gradient ellipse must land outside the inverted cutout, and a subsequent fill must still obey the mask.disableScissor()didn't flush — GL scissor applies at draw time; every other scissor mutation flushes first (with comments saying why), so the commonenableScissor → drawImage → disableScissorpattern clipped nothing. Two-line fix, pixel repro.WebGL
clearRect()painted opaque black — its JSDoc (and Canvas) promise transparent black, butclearColor()'s"#000000"default parses with alpha 1. Repro reads back alpha 255 on old code on atransparent: truecanvas.Canvas
clipRect()skip-optimizations ignored the transform — the full-viewport and same-as-last early-outs compared raw local values: a canvas-sized clipped container positioned off-origin got no clip at all, and a same-sized clipped container nested in another skipped its own clip via the stale cache — Canvas-only divergences from the transform-aware WebGL path (clipRect: potential issues with transformed containers and scissor state cleanup #1349). The skip logic now reasons in device space, and always clips under rotation/skew.Tests
tests/glcore-audit.spec.js(5) +tests/canvas-cliprect-transform.spec.js(2): all 7 red on the unfixed code, green after.renderTargetPool.spec.js: updated to the stack internals + 2 new nesting tests (distinct pair per level, stack unwind, capture-target follows the innermost pass).🤖 Generated with Claude Code
https://claude.ai/code/session_019t8kP8vp58ZrvaD2FqJU6A