feat(texture): generalize Texture2d + setTexture(assets) + renderer.toFrameTexture() (closes #1544)#1543
Open
obiot wants to merge 9 commits into
Open
feat(texture): generalize Texture2d + setTexture(assets) + renderer.toFrameTexture() (closes #1544)#1543obiot wants to merge 9 commits into
obiot wants to merge 9 commits into
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
obiot
changed the title
…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
Copilot review on the toFrameTexture addition:
- Per-frame reallocation: copyTexImage2D ran every capture, re-specifying the
texture storage each frame. Now copyTexImage2D allocates on the FIRST capture
(or on a size change, which nulls the slot), then copyTexSubImage2D refreshes
in place — no per-frame realloc. Verified the steady-state copyTexSubImage2D
path still samples on ANGLE Metal (fish animate through the live capture).
- GPUResidentTexture wasn't actually opaque: the optional `never` brand let any
object satisfy it, defeating Texture2dSource's type safety. Rebranded with a
`unique symbol` (a real nominal brand) — only engine-provided values match.
- CanvasRenderer.toFrameTexture with `target` returned a DIFFERENT instance on a
size change; now it resizes the same capture in place, matching the WebGL
object-identity contract. Canvas JSDoc documents `target: null` (mint).
- Doc/comment drift from the blit→copyTexImage2D switch: WebGL JSDoc
(copyTexSubImage2D mention, "Color only (RGBA)" → RGB/opaque), texture2d.ts
("a future toFrameTexture" → implemented), and the spec header all corrected.
- Aquarium example passes the NoiseTexture2d asset straight to setTexture
(19.9 API) instead of noise.getTexture().
Full suite 4753 green, lint 0, build 8/8.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019t8kP8vp58ZrvaD2FqJU6A
Comment on lines
+134
to
+138
| const canvas = globalThis.document | ||
| ? globalThis.document.createElement("canvas") | ||
| : new OffscreenCanvas(w, h); | ||
| canvas.width = w; | ||
| canvas.height = h; |
Comment on lines
+152
to
+154
| const ctx = frame.canvas.getContext("2d"); | ||
| ctx.clearRect(0, 0, w, h); | ||
| ctx.drawImage(src, x, y, w, h, 0, 0, w, h); |
Comment on lines
+895
to
+899
| let frame = shared | ||
| ? this._frameTexture | ||
| : options.target === null | ||
| ? undefined | ||
| : options.target; |
Comment on lines
+304
to
+306
| // register the debug plugin and open it by default (toggle with the S key) | ||
| plugin.register(DebugPanelPlugin, "debugPanel"); | ||
| (plugin.get("debugPanel") as { show?: () => void })?.show?.(); |
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.