feat(texture): generalize Texture2d + setTexture(assets) + renderer.toFrameTexture() (closes #1544)#1543
Merged
Merged
Conversation
… assets directly
Two pieces of 19.9 polish that must land before the release freezes the
new texture surface — groundwork for the 19.10 renderer.toFrameTexture()
screen-capture feature designed with the maintainer:
- Texture2d contract generalized while still unreleased: the class was
documented as "owns a drawable image source" with
getTexture(): HTMLCanvasElement | HTMLImageElement, but its own docs
already anticipated GPU-backed sources (the WebGPU note). The contract
now deliberately admits both: a new exported Texture2dSource type
(drawables | TextureResource), so GPU-resident subclasses — frame
captures, future WebGPU surfaces — fit without a 19.10 compat debate.
TextureResource itself stays internal (type-only reference).
- ShaderEffect.setTexture accepts a Texture2d asset directly and
resolves it via getTexture() internally — retiring the documented
`noise.getTexture()` dance: setTexture("uNoise", noise) now works.
Raw drawables unchanged. JSDoc + class example updated.
Red-first tests: a minimal Texture2d subclass and a NoiseTexture2d
passed directly to setTexture, sampled end-to-end via readPixels —
both red pre-change (the raw asset object reached createTexture2D).
Full suite 4741 passed / 0 failed; tsc build clean.
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 expands the engine’s Texture2d API surface to support both CPU-backed drawables and future GPU-resident texture resources, and updates ShaderEffect.setTexture to accept Texture2d assets directly (resolving via getTexture() internally). It adds tests to validate the new setTexture behavior and updates the 19.9 changelog entries accordingly.
Changes:
- Generalize
Texture2d#getTexture()to return a newTexture2dSourceunion type that can include rendererTextureResources. - Update
ShaderEffect.setTextureto accept aTexture2dasset instance directly (no manual.getTexture()needed). - Add Vitest coverage for passing both a minimal
Texture2dsubclass and aNoiseTexture2ddirectly intosetTexture, and update changelog wording.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/melonjs/tests/shadereffect-settexture.spec.js | Adds end-to-end WebGL tests proving setTexture accepts Texture2d assets directly. |
| packages/melonjs/src/video/webgl/shadereffect.js | Extends setTexture to resolve Texture2d assets via getTexture() and updates JSDoc/example. |
| packages/melonjs/src/video/texture/texture2d.ts | Introduces Texture2dSource and broadens Texture2d#getTexture() return type/documentation. |
| packages/melonjs/CHANGELOG.md | Updates 19.9 bullets to reflect the generalized Texture2d contract and setTexture usage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The unwrap runs before the enabled check, so an inert Canvas-mode stub receiving a Texture2d asset must still no-op safely — one more cell in the acceptance matrix for the direct-asset form. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019t8kP8vp58ZrvaD2FqJU6A
Found by re-examining the GL-core audit's rejected-findings list after the maintainer asked what pending review items link to this PR: the finder had flagged (and under-graded) a real inconsistency in the exact method this PR widens. _onContextLost auto-sets enabled = false; setUniform was gated on enabled — so a uniform set during the loss window was silently dropped ONE LAYER ABOVE the GLShader suspend-cache replay built precisely for that window (19.6), reverting to its pre-loss value after restore. The same gate ate setTexture bindings (permanently — no per-frame refresh to self-heal) and also dropped values while the USER had merely disabled an effect, no context loss required. - setUniform: forwards whenever a shader exists — GLShader already handles live vs suspended (cache deferral) correctly - setTexture: stores its entry regardless of enabled; the GL work is lazy in _prepareTextures anyway (loss handler nulls tex/unit, so the replacement cleanup is naturally safe mid-window). Canvas stubs keep the inert no-op via the no-shader guard - setTime: gate changed from enabled to genuinely-suspended (uniforms is null mid-loss; a per-frame call self-heals next frame) Red-first: user-disabled setUniform + mid-loss setUniform/setTexture survival across restore — both red pre-fix. Full suite green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019t8kP8vp58ZrvaD2FqJU6A
The ungating (previous commit) forwarded setUniform/setTexture/setTime whenever a shader existed — but a DESTROYED ShaderEffect still holds its _shader reference (partial-state immunity, tested in webgl_pipeline_adversarial), and GLShader.setUniform on a destroyed program throws "undefined (name) uniform". Re-add the destroyed guard so destroy-then-set stays an inert no-op, matching the pre-existing immunity contract, while a merely-disabled or context-lost effect keeps the new behaviour. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019t8kP8vp58ZrvaD2FqJU6A
… docs Copilot review on #1543: - Texture2dSource named the internal TextureResource in an EXPORTED type, making it a de-facto public API commitment. Replaced with a new opaque GPUResidentTexture marker interface — the union no longer references any internal class, and TextureResource stays internal (its only consumers are the tmxlayer + material batcher, unchanged). - setTexture released a replaced sampler's reserved unit and re-reserved it lazily on the next enabled draw. Now that setTexture stores bindings while disabled, that window is unbounded — the cache allocator could hand the high unit to a regular texture, reintroducing the collision reserveUnit() prevents. Keep the reservation across a replace; release only on context loss / destroy. New regression test pins it (unit held + allocator still skips it while disabled, new image uploads into the same unit on re-enable). - CHANGELOG Texture2d bullet contradicted itself ("owns a drawable source" / "backing canvas/image" vs "admits GPU-resident"). Reworded to "texture source" / "backing source". - shader-canvas test comment claimed the Texture2d unwrap runs before an enabled check; in Canvas mode setTexture returns early on the no-shader guard, so getTexture() is never called. Comment corrected. setUniform/setTexture resolving the asset while a WEBGL effect is merely disabled is intentional (the audit drop-fix): the value/binding must survive to apply on re-enable. The Canvas no-op contract is preserved by the no-shader early return. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019t8kP8vp58ZrvaD2FqJU6A
…loses #1544) Folds the #1544 ticket into this PR (no reason to defer it to 19.10 — the Texture2d contract + setTexture(Texture2d) groundwork this PR already lands is only half the feature; toFrameTexture is the half users actually asked for, to replace a readPixels screen-capture fork hack). Capture the current framebuffer into a Texture2d entirely on the GPU: - WebGL2: blitFramebuffer into an RGBA texture (converts an alpha-less default backbuffer — melonJS uses alpha:transparent, usually false — into an opaque RGBA capture; sidesteps the copyTex format constraints) - WebGL1: copyTexImage2D(RGBA) (ES2 fills the missing alpha with 1) - Canvas: offscreen-canvas self-copy, for toDataURL/toBlob/toImageBitmap family parity Returns a Texture2d (unexported FrameTexture / CanvasFrameTexture). setTexture gains a live-bind branch: a GPU-resident capture is bound by its live handle each draw (never uploaded as a static copy), so re-capturing into the shared slot refreshes what a shader samples with no re-bind. options.target reuses (or, with null, mints) a caller-owned capture; options.region copies a sub-region. Shared slot is dropped + reallocated on context loss (self-heal via gl.isTexture). Tests (tests/toframetexture.spec.js): capture content via FBO readback, shared-slot reuse, target mint/refresh, region, context-loss self-heal, Canvas variant, and the setTexture live-bind sampling path. NOTE: the headless software rasterizer in CI captures + reads back a blit-destination texture fine but cannot SAMPLE one, so capture content is verified by FBO readback and the live-bind sampling path is verified with a normally-uploaded texture; the end-to-end shader sampling of a real capture is exercised by the Aquarium example on a real GPU. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019t8kP8vp58ZrvaD2FqJU6A
…x capture to copyTexImage2D Replaces the Water Refraction (swimming-pool) example with an Aquarium that exercises the real toFrameTexture use case: fish swim across a planted tank, then a full-screen water surface (drawn last) captures the live frame in its draw() via renderer.toFrameTexture(), binds it to a custom ShaderEffect as uScene, and refracts it through a scrolling NoiseTexture2d flow map — the fish shimmer as if seen through moving water. The screen-texture pattern (Godot hint_screen_texture / Unity _CameraOpaqueTexture). Building the example on a real GPU (ANGLE Metal) surfaced that the earlier blitFramebuffer capture path produced a texture that some drivers refuse to SAMPLE in the same frame (the capture + FBO-readback were correct, but a shader sampling it read black — on Metal too, not just the CI software rasterizer). Switched to copyTexImage2D — the standard framebuffer→texture path (Three.js copyFramebufferToTexture) — with an RGB internalformat: a valid subset of both an alpha-less default backbuffer (melonJS uses alpha:transparent, usually false) and an RGBA camera FBO, so the copy never trips INVALID_OPERATION on a format mismatch, samples as an opaque backdrop (alpha=1), and samples reliably on every driver tested. Removes the blit scratch-FBO + WebGL1 split + FrameTexture._fbo. Verified end-to-end on ANGLE Metal (fish visibly refracted + animating); all 9 toFrameTexture specs green in CI. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019t8kP8vp58ZrvaD2FqJU6A
…al assets + debug panel) Reworks the Aquarium example to use the actual assets the contributor provided (seabed background, fish swim sheet, water texture — TexturePacker atlas shipped as aquarium.webp) instead of procedural stand-ins, and adapts their water effect to the 19.9 API: the original captured the screen with gl.readPixels + texImage2D on a fork; the WaterSurface now calls renderer.toFrameTexture() in draw() to grab the frame on the GPU and binds it to the refraction ShaderEffect as uScene. - Regions cropped from the atlas webp (seabed + water pre-scaled to the viewport so a full-screen sprite's framewidth matches its image → exact cover + clean UVs; fish sheet kept at 128² as a 2×2 of 64px frames). - Fixed-resolution `scale:"auto"` (not flex, which grew the viewport after onResetEvent read it and left the scene in a corner). - Shader samples the capture in the quad's own UV space (Y-flipped for the Y-up framebuffer), not gl_FragCoord/a resolution uniform, so it's correct under any canvas scaling. - @melonjs/debug-plugin registered and shown by default (an HTML overlay, so toFrameTexture doesn't capture it). Asset credit added to LICENSE.md. Verified end-to-end on ANGLE Metal: seabed fills the viewport, fish swim and refract through the captured frame, debug panel visible. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019t8kP8vp58ZrvaD2FqJU6A
The lit quad batcher parks normal maps in the TOP half of the texture-unit range; toFrameTexture binds its copy to a scratch unit at the top of the quad batcher's range — the same units. That scratch bind goes DIRECTLY through the batcher (not the shared texture cache), so other batchers' unit caches don't learn it changed — a later lit draw could assume its normal map is still resident, skip re-binding, and sample the captured frame AS a normal map. Aligns with how Three.js/Pixi handle off-path binds (invalidate the cache), finer-grained than their full resetState()/reset() since toFrameTexture runs every frame: - new MaterialBatcher.invalidateUnit(unit); LitQuadBatcher overrides to also drop the paired normal-map slot. - toFrameTexture invalidates the scratch unit across every batcher after copy. - also fixes a PRE-EXISTING latent gap surfaced here: the base cache-reset handler (unit-pool-wrap path) cleared colors but not the lit normal-map cache. Refactored into an overridable _onTextureCacheReset; LitQuadBatcher clears normals too. Adversarial tests (3, red without the fix). New Heat Haze example: normal-mapped tiles under a moving Light2d distorted by a toFrameTexture heat-haze — the lit-scene + capture scenario the fix protects. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019t8kP8vp58ZrvaD2FqJU6A
…ture scratch Addresses the code review's HIGH finding on the previous commit: the fix invalidated only toFrameTexture's single scratch unit, but ShaderEffect _prepareTextures binds its extra samplers counting DOWN from the top (maxBatchTextures-1, -2, ...), each a direct bind that bypasses the shared texture cache and each overlapping a lit normal-map unit. Only the top one was covered — the shipped Heat Haze example actually clobbers the SECOND unit (uScene) which was left un-invalidated. - new WebGLRenderer.invalidateTextureUnit(unit, except): the shared primitive, loops batchers (skipping the binder whose cache is already correct). - toFrameTexture and _prepareTextures both call it, so EVERY directly-bound reserved unit is invalidated across the other batchers — a complete fix, not just the scratch unit. Also from the review (lower severity): - _onTextureCacheReset guards boundNormalMaps?.fill against a reset firing before init allocates the arrays (latent; hardens against init reordering). - Heat Haze example: build each distinct NoiseTexture2d ONCE and reuse across grid cells (was one per sprite), and destroy() them on teardown. - JSDoc precision (samplers land on top AND lower units). Tests: new GPU-independent coverage — invalidateTextureUnit(unit, except) clears all-but-excluded; a spy test proving _prepareTextures invalidates EVERY reserved sampler unit (verified RED with the _prepareTextures call removed: "expected [] to include 15"). Full suite 4758 green, lint 0, build 8/8. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019t8kP8vp58ZrvaD2FqJU6A
… README
Copilot review + the missing docs entry:
- README: replaced the removed "Water Refraction" example link with the new
Aquarium + Heat Haze examples (both showcase renderer.toFrameTexture()).
- CanvasRenderer.toFrameTexture now validates `target` is a CanvasFrameTexture
(mirrors the WebGL guard) instead of throwing a confusing frame.canvas error.
- Both backends clamp the region ORIGIN (x/y) into the framebuffer/canvas
before sizing, and default a missing width/height to the remaining extent —
an out-of-range x/y could otherwise push x+w past the edge and trip
copyTex(Sub)Image2D INVALID_VALUE. Fixed the stale `{x,y,w,h}` comment.
- WebGLRenderer.toFrameTexture rejects a `target` from a DIFFERENT renderer
(frame._renderer !== this) — would delete textures on the wrong GL context.
- ShaderEffect.setTexture live-source detection now also requires a `glTexture`
field; a Texture2d with isGPUResident=true but no handle falls back to the
static getTexture() unwrap instead of silently never binding.
- Texture2d docs: clarify a GPU-resident capture (FrameTexture) is a
custom-shader sampler (setTexture), NOT a drawable assignable to Sprite#image.
Base renderer JSDoc notes the region origin differs by backend (WebGL
bottom-left / Canvas top-left).
Tests: bad target throws (WebGL + Canvas); an out-of-bounds region clamps to a
valid capture with no GL error. Full suite 4761 green, lint 0, build 8/8.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019t8kP8vp58ZrvaD2FqJU6A
Audited every test added in this PR for "would it catch a regression" and
strengthened the weak ones:
- region capture: the old test painted a UNIFORM scene, so it couldn't tell a
correct offset from a full-frame capture. Now paints framebuffer halves
(red/blue) at KNOWN pixel coords via scissored clears — bypassing the bare
harness's centred projection — and asserts a LEFT-half region reads red and a
RIGHT-half region reads blue (also catches an x-mirror). Verified it goes RED
when the x offset is ignored.
- Rect-style region ({pos} + width/height) now covered (the region.pos branch).
- new guards get adversarial coverage: a foreign-renderer target is rejected;
a Texture2d claiming isGPUResident but with no glTexture falls back to the
static path (entry.live === false) instead of silently never binding;
invalidateUnit on an out-of-range unit is a no-op that doesn't extend the
normal-map array; an out-of-bounds region clamps to a valid capture.
- Canvas: correct sub-region offset + out-of-bounds clamp + refresh-in-place on
resize (same object identity).
Full suite 4767 green, lint 0, build 8/8.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019t8kP8vp58ZrvaD2FqJU6A
Drops the Rect (`.pos`) special case from the region resolver in both renderers — there's no reason to accept a Rect when a Bounds already covers it. `region` is now a Bounds (or any object exposing numeric x/y/width/height); a Rect caller passes `rect.getBounds()`. Simpler resolver, one documented shape. JSDoc (WebGL / Canvas / base Renderer) updated accordingly. Test now exercises a real Bounds AND the Rect.getBounds() migration path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019t8kP8vp58ZrvaD2FqJU6A
Splits the combined test: one for a plain Bounds, and a dedicated Rect migration-path test that proves the Rect's ACTUAL position and size drive the capture — getBounds() reflects x/y/width/height (non-square, to catch a swap); a right-half Rect reads blue, a left-half Rect reads red, each sized to the Rect. Verified RED when the region x offset is dropped. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019t8kP8vp58ZrvaD2FqJU6A
…lidate guard, aquarium leak Copilot review: - CHANGELOG: the setTexture + shader-preloading bullets still pointed at the removed "Water Refraction" example — updated to Aquarium / Heat Haze (matches the README). - test comment: the capture texture is RGB (opaque), not RGBA — corrected. - _prepareTextures: only invalidateTextureUnit when the sampler actually bound; a skipped live bind (capture not ready) clobbers nothing, so invalidating other batchers' caches was needless extra rebinds. - Aquarium example: destroy() its NoiseTexture2d on teardown (was leaking the baked canvas across stage reloads, like the Heat Haze fix). Full suite green, lint 0, build 8/8. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019t8kP8vp58ZrvaD2FqJU6A
obiot
added a commit
that referenced
this pull request
Jul 9, 2026
…with #1543) #1543 widened Texture2d.getTexture() to the new Texture2dSource union (GPU-resident backings incl. ImageBitmap); the event signature added in this branch predated that and was too narrow to typecheck. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019t8kP8vp58ZrvaD2FqJU6A
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.
19.9 polish that must land before the release freezes the new texture surface — groundwork for the
renderer.toFrameTexture()screen-capture feature (19.10) designed in review with the maintainer.1.
Texture2dcontract generalized (while still unreleased)The class doc said "owns a drawable image source" with
getTexture(): HTMLCanvasElement | HTMLImageElement— but its own docs already anticipated GPU-backed sources ("a future WebGPU implementation would back getTexture with a GPUTexture-wrapping surface"). The contract now deliberately admits both:Texture2dSourcetype: drawables (canvas/image/OffscreenCanvas/ImageBitmap) or a rendererTextureResource(GPU-resident, uploads/binds itself through the texture cache — never read back to the CPU)TextureResourceitself stays internal (type-only reference — one public texture taxonomy, not two)Doing this now costs nothing; doing it after 19.9 ships would start the 19.10 capture feature with a compat debate.
2.
ShaderEffect.setTextureaccepts aTexture2dasset directlywater.setTexture("uNoise", noise)now works — the effect resolves the asset viagetTexture()internally, retiring the documented.getTexture()dance. Raw drawables unchanged; JSDoc and the class example updated.Tests
Red-first: a minimal
Texture2dsubclass and a realNoiseTexture2dpassed directly tosetTexture, sampled end-to-end viareadPixels— both red pre-change (the raw asset object fell through tocreateTexture2D). Full suite 4741 / 0; tsc build clean (the JSDoc-typedresource.jsbridges cleanly into the TS type).CHANGELOG: folded into the existing 19.9
Texture2dandsetTexturebullets (unreleased-feature convention).🤖 Generated with Claude Code
https://claude.ai/code/session_019t8kP8vp58ZrvaD2FqJU6A
Added mid-review:
renderer.toFrameTexture()(closes #1544)On reflection there was no reason to defer the actual capture method to 19.10 — the Texture2d contract +
setTexture(Texture2d)groundwork here is only half the feature;toFrameTexture()is the half users asked for (to replace areadPixelsscreen-capture fork hack). Folded the #1544 ticket in:renderer.toFrameTexture(options?)→ aTexture2dcapturing the bound framebuffer viacopyTexImage2D(RGB, opaque; noreadPixelsstall).options.targetreuses/mints a caller-owned capture;options.regioncopies a sub-region. Camera2d/Camera3d agnostic; shared slot self-heals on context loss.setTexturelive-bind branch: a GPU-resident capture binds by live handle each draw, so re-capturing into the shared slot refreshes what a shader samples with no re-bind.tests/toframetexture.spec.js(9 tests): capture content (FBO readback), shared-slot reuse, target mint/refresh, region, context-loss self-heal, Canvas variant, live-bind sampling.Closes #1544.