From 88d95e36fdca05dfaddfab9f39eb6cb687722140 Mon Sep 17 00:00:00 2001 From: Olivier Biot Date: Fri, 3 Jul 2026 13:09:50 +0800 Subject: [PATCH 1/2] =?UTF-8?q?fix(mesh):=20per-mesh=20textureRepeat=20?= =?UTF-8?q?=E2=80=94=20stop=20mutating=20the=20shared=20per-image=20Textur?= =?UTF-8?q?eAtlas?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mesh applied its textureRepeat setting by writing to this.texture.repeat, but that TextureAtlas is shared per source image (renderer.cache.get), so the wrap mode leaked image-globally: two meshes (or a mesh and a sprite/pattern) using one image with different wrap needs were last-writer-wins, and the loser silently sampled with the wrong wrap. The wrap now lives on the mesh (mesh.textureRepeat) and is threaded through MeshBatcher.addMesh → uploadTexture → TextureCache.getUnit at draw time — sampler state per use, the separation modern GPU APIs (sampler objects / GPUSampler) enforce. The unit cache's (source, repeat) keying (#1448) already gives each wrap its own GL texture, so per-mesh wraps coexist on a single image. Also fixed alongside: TextureCache.delete(image) now sweeps the texture units of ALL the source's wrap-mode variants (new freeAllUnits helper) — the granular freeTextureUnit only freed the unit matching the atlas's current repeat field, pinning the rest until a full cache reset. freeTextureUnit itself stays granular so freeing one live consumer never evicts another's unit. New tests/mesh-texture-repeat.spec.js (4 tests) written failing-first against the pre-fix code: constructor mutation, per-mesh wrap at draw time (unit map + GL sampler params), pixel-level repeat-vs-clamp readback on one shared image, and the delete-time unit sweep. Closes #1503 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_019t8kP8vp58ZrvaD2FqJU6A --- packages/melonjs/CHANGELOG.md | 1 + packages/melonjs/src/renderable/mesh.js | 49 ++-- packages/melonjs/src/video/texture/cache.js | 69 ++++- .../video/webgl/batchers/material_batcher.js | 12 +- .../src/video/webgl/batchers/mesh_batcher.js | 15 +- packages/melonjs/tests/gltf_model.spec.js | 9 +- .../melonjs/tests/mesh-texture-repeat.spec.js | 250 ++++++++++++++++++ packages/melonjs/tests/mesh.spec.js | 14 +- 8 files changed, 375 insertions(+), 44 deletions(-) create mode 100644 packages/melonjs/tests/mesh-texture-repeat.spec.js diff --git a/packages/melonjs/CHANGELOG.md b/packages/melonjs/CHANGELOG.md index 9627297c4..c90d4c936 100644 --- a/packages/melonjs/CHANGELOG.md +++ b/packages/melonjs/CHANGELOG.md @@ -19,6 +19,7 @@ - **Animated `NineSliceSprite` collapsed to its frame size** (#1115) — a `NineSliceSprite` driven by an animation (or any per-frame texture change) had its user-specified "expanded" `width`/`height` overwritten by each frame's source dimensions, shrinking the 9-slice panel to a single frame after the first animation step. Static nine-slices were unaffected because the frame is applied only once (during construction, before the expanded size is set). `NineSliceSprite` now applies a new frame by swapping only the source sub-texture, leaving the expanded size and bounds intact. First-ever test coverage for `NineSliceSprite` (the class previously had none). - **SVG `A`/`H`/`V` commands opening a new sub-path drew from the wrong origin** — the arc (`A`), horizontal-line (`H`) and vertical-line (`V`) parsers used the previous sub-path's last point as the current pen position instead of the point set by the preceding `M`. A circular hole authored with arcs (e.g. a donut) therefore degenerated into a spiral with a radial slit. They now track the pen position correctly. Latent since these commands were added; surfaced by the new multi-sub-path holes support (#1253). - **2D sprites could silently vanish when using a large `pos.z` sort key** (regression since 19.7) — since the depth pipeline landed, a sprite's `depth`/`pos.z` is written into the vertex `z` and participates in clip-space. Under a 2D orthographic camera `pos.z` is only a painter's sort key (the world is CPU-sorted on it), so a large value — a `baseZ + pos.y` Y-sort, or `Container.autoDepth` on a big world — pushed the vertex past the camera far plane (`Camera2d.far`, 1e6) and the GPU clip-culled the sprite (it disappeared, or rendered incorrectly on some drivers). `Renderer.setDepth` now feeds depth into the vertex stream only under a perspective projection (`Camera3d`, which genuinely uses z for parallax/depth); orthographic cameras keep depth out of clip-space, so a sort key can never clip a sprite. `Camera3d` / mesh depth is unaffected. +- **`Mesh.textureRepeat` leaked its wrap mode to every consumer of the same image** (#1503) — the setting was applied by mutating the per-image `TextureAtlas` shared by everything drawing that image, so two meshes (or a mesh and a sprite/pattern) pointing at one image with different wrap needs were last-writer-wins: the loser silently sampled with the wrong wrap. The wrap now lives on the mesh and is threaded to the batcher at draw time — sampler state per use, the same separation modern GPU APIs enforce — and the texture-unit cache's `(source, repeat)` keying gives each wrap its own GL texture, so per-mesh wraps coexist on one image. Also fixed alongside: unloading an image now frees the texture units of **all** its wrap-mode variants — previously only the unit matching the atlas's current `repeat` field was freed, pinning the rest until a full cache reset. ## [19.8.0] (melonJS 2) - _2026-06-26_ diff --git a/packages/melonjs/src/renderable/mesh.js b/packages/melonjs/src/renderable/mesh.js index 7b71d23e0..35976c83d 100644 --- a/packages/melonjs/src/renderable/mesh.js +++ b/packages/melonjs/src/renderable/mesh.js @@ -150,7 +150,7 @@ export default class Mesh extends Renderable { * @param {boolean} [settings.normalize=true] - fit the source geometry into a `[-0.5, 0.5]` unit cube before scaling, so `width`/`height` behave like a Sprite. Set `false` to keep the geometry's real-world coordinates — required when several meshes share one coordinate space (e.g. nodes of an imported glTF scene) so their relative scale and layout are preserved. * @param {number} [settings.scale] - world-space scale (pixels per source unit) for the Camera3d path; defaults to `width`. Set this when `width`/`height` describe the renderable's world bounds (frustum culling) rather than the geometry scale — see {@link Mesh#meshScale}. * @param {boolean} [settings.rightHanded=false] - treat the source as right-handed (Y-up, e.g. glTF) under the `Camera3d` world path. The default Y-up→Y-down bridge negates Y only (a reflection, which mirrors the scene left/right); `true` negates Y **and** Z (a rotation) so chirality is preserved and the result matches the authoring tool. See {@link Mesh#rightHanded}. - * @param {string} [settings.textureRepeat] - texture wrap mode (`"repeat"` / `"repeat-x"` / `"repeat-y"` / `"no-repeat"`) applied to the resolved texture. Use `"repeat"` when the geometry's UVs fall outside the `[0, 1]` range and rely on the texture tiling (e.g. glTF assets, whose default sampler wrap is REPEAT) — otherwise the texture clamps to its edge texels and looks flat. Ignored for the white-pixel fallback. Note: REPEAT on a non-power-of-two texture requires WebGL 2. + * @param {string} [settings.textureRepeat] - texture wrap mode (`"repeat"` / `"repeat-x"` / `"repeat-y"` / `"no-repeat"`) this mesh samples its texture with (per-mesh — it does not modify the shared texture, so other meshes/sprites using the same image are unaffected). Use `"repeat"` when the geometry's UVs fall outside the `[0, 1]` range and rely on the texture tiling (e.g. glTF assets, whose default sampler wrap is REPEAT) — otherwise the texture clamps to its edge texels and looks flat. Ignored for the white-pixel fallback. Note: REPEAT on a non-power-of-two texture requires WebGL 2. * @param {string} [settings.textureFilter] - texture magnification filter (`"nearest"` for crisp pixel-art upscaling, `"linear"` for smooth) applied to the resolved texture. Omit to keep the renderer's global `antiAlias` default. WebGL only (ignored by the Canvas renderer). * @param {number} [settings.alphaCutoff=0] - alpha cutout threshold. Fragments whose final alpha is below this value are discarded (hard-edged cutout — foliage, fences, decals — with no blending or sorting). `0` disables the cutout. Set automatically by the glTF loader from a material's `alphaMode: "MASK"`. WebGL mesh path only. * @param {number[]|Float32Array} [settings.emissive] - emissive (self-illumination) color `[r, g, b]` (0..1, may exceed 1 for HDR glow) added on top of the lit/unlit color so the surface glows regardless of scene lights (neon, lava, screens). Omit / all-zero for no emission. Set automatically by the glTF loader (`emissiveFactor`) and OBJ loader (MTL `Ke`). WebGL mesh path only. @@ -509,28 +509,39 @@ export default class Mesh extends Renderable { settings.frameheight, ); - // Optional texture wrap mode. Some assets author UVs outside the - // `[0, 1]` range and rely on the sampler repeating the texture (this is - // the glTF default sampler behavior); the mesh would otherwise clamp to - // the edge texels and look flat / untextured. Applied only to a real - // texture — never the shared white-pixel fallback, which is global and - // must stay `"no-repeat"`. One of `"repeat"` / `"repeat-x"` / - // `"repeat-y"` / `"no-repeat"`. - // - // NOTE: `cache.get(image)` returns a TextureAtlas shared per source - // image, so this sets the wrap mode IMAGE-GLOBALLY — every consumer of - // the same image object samples with this wrap. Harmless for glTF (each - // asset decodes its own image objects), but don't point two meshes that - // need different wrap modes at the same image. (Tracked in #1503, to be - // fixed with the #1410 TextureCache refactor.) - if (hasRealTexture && typeof settings.textureRepeat === "string") { - this.texture.repeat = settings.textureRepeat; - } + /** + * Per-mesh texture wrap mode (`"repeat"` / `"repeat-x"` / `"repeat-y"` + * / `"no-repeat"`), or `undefined` to sample with the texture's own + * wrap. Some assets author UVs outside the `[0, 1]` range and rely on + * the sampler repeating the texture (this is the glTF default sampler + * behavior); the mesh would otherwise clamp to the edge texels and + * look flat / untextured. + * + * Kept on the mesh and threaded to the batcher at draw time — sampler + * state per use, like a GL sampler object — so it never mutates the + * per-image `TextureAtlas` shared with every other consumer of the + * same image (#1503). Two meshes (or a mesh and a sprite) can point + * at one image with different wrap modes; the texture cache keys GL + * units by `(source, repeat)` so each wrap gets its own GL texture. + * Applied only to a real texture — never the shared white-pixel + * fallback, which is global and must stay `"no-repeat"`. + * @type {string|undefined} + */ + this.textureRepeat = + hasRealTexture && typeof settings.textureRepeat === "string" + ? settings.textureRepeat + : undefined; // Optional texture magnification filter (`"nearest"` for crisp pixel-art // upscaling, `"linear"` for smooth). When omitted the texture keeps the // renderer's global `antiAlias` default. WebGL only — the Canvas renderer - // ignores it. Same image-global caveat as `textureRepeat` above (#1503). + // ignores it. + // + // NOTE: unlike `textureRepeat` above, this still mutates the shared + // per-image atlas — the texture-unit cache doesn't discriminate by + // filter, so a true per-mesh filter needs the unit-key change planned + // with the #1410 TextureCache refactor. Two consumers of one image + // wanting different filters is last-writer-wins until then. if ( hasRealTexture && typeof settings.textureFilter === "string" && diff --git a/packages/melonjs/src/video/texture/cache.js b/packages/melonjs/src/video/texture/cache.js index c23302c3c..bac443208 100644 --- a/packages/melonjs/src/video/texture/cache.js +++ b/packages/melonjs/src/video/texture/cache.js @@ -135,10 +135,13 @@ class TextureCache { /** * @ignore * - * Hot-path note: `getUnit` / `peekUnit` / `freeTextureUnit` are - * called per-texture per-draw, so the `(source, repeat)` lookup is - * inlined here rather than going through a helper that allocates a - * `{source, repeat}` object on every call. + * Frees the single `(source, repeat)` unit matching the texture's + * current `repeat` field — deliberately granular, so freeing one live + * consumer (e.g. one of two patterns with different repeats over the + * same image) never evicts the other's unit. When a SOURCE goes away + * entirely, use {@link freeAllUnits} instead — a source can hold units + * under several repeat modes (per-mesh `textureRepeat` overrides, + * #1503) that this granular free would leave pinned. */ freeTextureUnit(texture) { const source = texture.sources.get(texture.activeAtlas); @@ -157,30 +160,63 @@ class TextureCache { /** * @ignore + * + * Free every texture unit allocated for the texture's source, across + * ALL repeat modes. A single source can hold several `(source, repeat)` + * units — patterns with different repeats over one image, or meshes + * sampling one image with per-mesh `textureRepeat` overrides (#1503) — + * and the source going away invalidates all of them at once. */ - getUnit(texture) { + freeAllUnits(texture) { const source = texture.sources.get(texture.activeAtlas); - const repeat = normalizeRepeat(texture.repeat); + const perRepeat = this.units.get(source); + if (typeof perRepeat !== "undefined") { + for (const unit of perRepeat.values()) { + this.usedUnits.delete(unit); + } + this.units.delete(source); + } + } + + /** + * @ignore + * @param {string} [repeat] - overrides the texture's own `repeat` for the + * unit lookup — sampler state per use (a mesh's `textureRepeat`), so one + * source can be sampled with several wrap modes without mutating the + * shared atlas (#1503). Omit to use `texture.repeat`. + * + * Hot-path note: `getUnit` / `peekUnit` are called per-texture per-draw, + * so the `(source, repeat)` lookup is inlined here rather than going + * through a helper that allocates a `{source, repeat}` object per call. + */ + getUnit(texture, repeat) { + const source = texture.sources.get(texture.activeAtlas); + const wrap = normalizeRepeat( + typeof repeat === "string" ? repeat : texture.repeat, + ); let perRepeat = this.units.get(source); if (perRepeat === undefined) { perRepeat = new Map(); this.units.set(source, perRepeat); } - if (!perRepeat.has(repeat)) { - perRepeat.set(repeat, this.allocateTextureUnit()); + if (!perRepeat.has(wrap)) { + perRepeat.set(wrap, this.allocateTextureUnit()); } - return perRepeat.get(repeat); + return perRepeat.get(wrap); } /** * @ignore * return the texture unit for the given texture, or -1 if not allocated + * @param {string} [repeat] - same per-use wrap override as {@link getUnit} */ - peekUnit(texture) { + peekUnit(texture, repeat) { const source = texture.sources.get(texture.activeAtlas); - const repeat = normalizeRepeat(texture.repeat); + const wrap = normalizeRepeat( + typeof repeat === "string" ? repeat : texture.repeat, + ); const perRepeat = this.units.get(source); - return perRepeat?.has(repeat) ? perRepeat.get(repeat) : -1; + return perRepeat?.has(wrap) ? perRepeat.get(wrap) : -1; } /** @@ -254,9 +290,14 @@ class TextureCache { // multiple atlases can coexist for one image — freeing only // `cache.get(image)[0]` would leak the remaining repeats' // texture units after `this.cache.delete(image)` wipes the - // entire multimap bucket. + // entire multimap bucket. Deleting an image means the SOURCE + // is going away, so sweep ALL repeat modes per atlas + // (`freeAllUnits`) — one atlas can hold several per-repeat + // units via per-mesh `textureRepeat` overrides (#1503), which + // the granular `freeTextureUnit` (keyed on the atlas's current + // `repeat` field) would leave pinned in `units`/`usedUnits`. for (const texture of this.cache.get(image)) { - this.freeTextureUnit(texture); + this.freeAllUnits(texture); } this.cache.delete(image); } diff --git a/packages/melonjs/src/video/webgl/batchers/material_batcher.js b/packages/melonjs/src/video/webgl/batchers/material_batcher.js index dd6fd6f6a..5d45c61ee 100644 --- a/packages/melonjs/src/video/webgl/batchers/material_batcher.js +++ b/packages/melonjs/src/video/webgl/batchers/material_batcher.js @@ -357,9 +357,15 @@ export class MaterialBatcher extends Batcher { * @param {number} [h] - same as `w`. * @param {boolean} [force=false] * @param {boolean} [flush=true] + * @param {string} [repeat] - per-use wrap-mode override (a mesh's + * `textureRepeat`, #1503) — sampled with this wrap without mutating + * the shared atlas's `repeat`. The texture-unit cache keys by + * `(source, repeat)`, so each wrap gets its own unit + GL texture. + * Omit to use `texture.repeat`. */ - uploadTexture(texture, w, h, force = false, flush = true) { - const unit = this.renderer.cache.getUnit(texture); + uploadTexture(texture, w, h, force = false, flush = true, repeat) { + const wrap = typeof repeat === "string" ? repeat : texture.repeat; + const unit = this.renderer.cache.getUnit(texture, wrap); const texture2D = this.boundTextures[unit]; if (typeof texture2D === "undefined" || force) { @@ -392,7 +398,7 @@ export class MaterialBatcher extends Batcher { unit, source, filter, - texture.repeat, + wrap, texW, texH, texture.premultipliedAlpha, diff --git a/packages/melonjs/src/video/webgl/batchers/mesh_batcher.js b/packages/melonjs/src/video/webgl/batchers/mesh_batcher.js index f74189a2a..29cebcc59 100644 --- a/packages/melonjs/src/video/webgl/batchers/mesh_batcher.js +++ b/packages/melonjs/src/video/webgl/batchers/mesh_batcher.js @@ -314,8 +314,19 @@ export default class MeshBatcher extends MaterialBatcher { const indices = mesh.indices; const vertexColors = mesh.vertexColors; - // upload and activate the texture - const unit = this.uploadTexture(mesh.texture); + // upload and activate the texture. The mesh's own `textureRepeat` + // (when set) is threaded through as a per-use wrap override — sampler + // state per mesh, never a mutation of the shared per-image atlas + // (#1503). The unit cache keys by `(source, repeat)`, so meshes with + // different wraps over one image coexist on distinct GL textures. + const unit = this.uploadTexture( + mesh.texture, + undefined, + undefined, + false, + true, + mesh.textureRepeat, + ); if (unit !== this.currentSamplerUnit) { this.currentShader.setUniform("uSampler", unit); this.currentSamplerUnit = unit; diff --git a/packages/melonjs/tests/gltf_model.spec.js b/packages/melonjs/tests/gltf_model.spec.js index 52e88fde2..ab13e4cf6 100644 --- a/packages/melonjs/tests/gltf_model.spec.js +++ b/packages/melonjs/tests/gltf_model.spec.js @@ -143,9 +143,12 @@ describe("GLTFModel", () => { it("forwards the glTF texture wrap mode onto each part mesh", () => { const mesh = childOf(makeModel()); - // PRIM() carries textureRepeat:"repeat" + a real texture → the atlas - // must end up REPEAT-wrapped (tiling UVs sample correctly vs clamping) - expect(mesh.texture.repeat).toBe("repeat"); + // PRIM() carries textureRepeat:"repeat" + a real texture → the part + // mesh must sample REPEAT-wrapped (tiling UVs vs clamping). The wrap + // is per-mesh (threaded to the batcher at draw time) — the shared + // per-image atlas stays untouched (#1503). + expect(mesh.textureRepeat).toBe("repeat"); + expect(mesh.texture.repeat).toBe("no-repeat"); }); it("poses to the bind/rest pose on construction (parent at origin → child at its local x)", () => { diff --git a/packages/melonjs/tests/mesh-texture-repeat.spec.js b/packages/melonjs/tests/mesh-texture-repeat.spec.js new file mode 100644 index 000000000..e4c43212d --- /dev/null +++ b/packages/melonjs/tests/mesh-texture-repeat.spec.js @@ -0,0 +1,250 @@ +import { afterAll, beforeAll, describe, expect, it } from "vitest"; +import { boot, Matrix3d, Mesh, video, WebGLRenderer } from "../src/index.js"; + +/** + * Tests for issue #1503 — `Mesh`'s `textureRepeat` setting used to apply the + * wrap mode by MUTATING `this.texture.repeat`, and `this.texture` is the + * TextureAtlas shared per source image (`renderer.cache.get(image)`). So the + * wrap mode leaked image-globally: two meshes pointing at the same image but + * wanting different wrap modes were last-writer-wins, and the loser's wrap + * silently flipped. + * + * The fix keeps the wrap on the Mesh (`mesh.textureRepeat`) and threads it + * through `MeshBatcher.addMesh` → `uploadTexture` → `TextureCache.getUnit` + * at draw time — the sampler-state-per-use model — leaving the shared atlas + * untouched. The `(source, repeat)` unit keying (#1448) already gives each + * wrap mode its own GL texture, so per-mesh wraps coexist on one image. + * + * Also covered: `TextureCache.freeTextureUnit` used to free only the unit + * matching the atlas's CURRENT `repeat` field, leaking any other per-repeat + * units for the same source until a full cache reset (pre-existing, but the + * per-mesh override makes multi-repeat sources a first-class case). + * + * Written failing-first against the pre-fix code: every test below is RED + * with the constructor mutation in place and GREEN with the fix. + */ +describe("Mesh textureRepeat vs shared TextureAtlas (issue #1503)", () => { + let renderer; + + beforeAll(async () => { + await boot(); + try { + video.init(128, 128, { + parent: "screen", + renderer: video.WEBGL, + // Chromium headless uses a software GL backend that trips the + // "major performance caveat" flag — opt out so the tests run + // on SwiftShader instead of silently falling back to Canvas. + failIfMajorPerformanceCaveat: false, + }); + } catch { + // Genuine WebGL absence — tests skip below. + } + if (video.renderer instanceof WebGLRenderer) { + renderer = video.renderer; + } + }); + + afterAll(() => { + // Restore the video subsystem to AUTO so this spec doesn't leak a + // forced-WebGL renderer into other test files sharing the `video` + // global (same cleanup pattern as webgl_mesh_depth.spec.js). + try { + video.init(128, 128, { + parent: "screen", + renderer: video.AUTO, + }); + } catch { + // ignore + } + }); + + const requireWebGL = (ctx) => { + if (renderer === undefined) { + ctx.skip("WebGL renderer not available in this environment"); + } + }; + + // a fresh 4x4 solid-white canvas per test — each test needs its own + // source object so unit-cache state can't bleed between tests + const makeSolidImage = () => { + const canvas = document.createElement("canvas"); + canvas.width = 4; + canvas.height = 4; + const ctx = canvas.getContext("2d"); + ctx.fillStyle = "#ffffff"; + ctx.fillRect(0, 0, 4, 4); + return canvas; + }; + + // 4x4 canvas, left half red / right half blue — under `"repeat"` a UV of + // u=1.25 wraps to 0.25 (red half); under `"no-repeat"` it clamps to the + // right edge (blue). Rows are uniform so v-precision doesn't matter. + const makeSplitImage = () => { + const canvas = document.createElement("canvas"); + canvas.width = 4; + canvas.height = 4; + const ctx = canvas.getContext("2d"); + ctx.fillStyle = "#ff0000"; + ctx.fillRect(0, 0, 2, 4); + ctx.fillStyle = "#0000ff"; + ctx.fillRect(2, 0, 2, 4); + return canvas; + }; + + // minimal quad geometry for real Mesh construction (unit quad — the + // constructor normalizes and scales it to `width`, we never draw these + // for pixel assertions) + const quadSettings = (image, textureRepeat) => { + return { + vertices: [-1, -1, 0, 1, -1, 0, 1, 1, 0, -1, 1, 0], + uvs: [0, 0, 2, 0, 2, 1, 0, 1], + indices: [0, 1, 2, 0, 2, 3], + texture: image, + width: 32, + textureRepeat, + }; + }; + + it("constructing a Mesh with textureRepeat leaves the shared per-image atlas untouched", (ctx) => { + requireWebGL(ctx); + const image = makeSolidImage(); + const shared = renderer.cache.get(image); + expect(shared.repeat).toBe("no-repeat"); + + const mesh = new Mesh(0, 0, quadSettings(image, "repeat")); + + // the wrap belongs to the mesh, not to the image-global atlas + expect(mesh.textureRepeat).toBe("repeat"); + expect(shared.repeat).toBe("no-repeat"); + // the mesh still resolved the shared atlas (no private copy churn) + expect(mesh.texture).toBe(shared); + }); + + it("two meshes sharing one image each draw with their own wrap mode", (ctx) => { + requireWebGL(ctx); + const gl = renderer.gl; + const image = makeSolidImage(); + const meshRepeat = new Mesh(0, 0, quadSettings(image, "repeat")); + const meshClamp = new Mesh(0, 0, quadSettings(image, "no-repeat")); + + // capture the TEXTURE_WRAP_S values actually sent to the GPU + const wrapS = []; + const origTexParameteri = gl.texParameteri.bind(gl); + gl.texParameteri = (target, pname, value) => { + if (pname === gl.TEXTURE_WRAP_S) { + wrapS.push(value); + } + return origTexParameteri(target, pname, value); + }; + try { + renderer.drawMesh(meshRepeat); + renderer.drawMesh(meshClamp); + } finally { + gl.texParameteri = origTexParameteri; + } + + // each wrap mode got its own (source, repeat) texture unit … + const perRepeat = renderer.cache.units.get(image); + expect(perRepeat).toBeDefined(); + expect(perRepeat.has("repeat")).toBe(true); + expect(perRepeat.has("no-repeat")).toBe(true); + expect(perRepeat.get("repeat")).not.toBe(perRepeat.get("no-repeat")); + // … and both wrap modes reached the GL sampler state (pre-fix, the + // shared-field mutation meant only the LAST constructed mesh's wrap + // was ever uploaded) + expect(wrapS).toContain(gl.REPEAT); + expect(wrapS).toContain(gl.CLAMP_TO_EDGE); + }); + + it("pixel readback: repeat tiles and no-repeat clamps on the same source image", (ctx) => { + requireWebGL(ctx); + const gl = renderer.gl; + const image = makeSplitImage(); + const atlas = renderer.cache.get(image); + + // duck-typed meshes (the shape drawMesh/addMesh consume — same + // pattern as webgl_mesh_depth.spec.js) sharing ONE atlas object, + // differing only in the per-mesh wrap. u spans 0..2 so the right + // half of each quad samples u > 1 — where wrap vs clamp diverge. + const makeQuad = (cx, textureRepeat) => { + const half = 24; + const cy = 64; + return { + vertices: new Float32Array([ + cx - half, + cy - half, + 0, + cx + half, + cy - half, + 0, + cx + half, + cy + half, + 0, + cx - half, + cy + half, + 0, + ]), + uvs: new Float32Array([0, 0, 2, 0, 2, 1, 0, 1]), + indices: new Uint16Array([0, 1, 2, 0, 2, 3]), + texture: atlas, + textureRepeat, + cullBackFaces: false, + alphaCutoff: 0, + }; + }; + + const proj = new Matrix3d(); + proj.ortho(0, 128, 128, 0, -1000, 1000); + renderer.setProjection(proj); + renderer.currentTint.setColor(255, 255, 255, 1); + renderer.backgroundColor.setColor(0, 0, 0, 255); + renderer.clear(); + + // left quad tiles, right quad clamps — same image, same frame + renderer.drawMesh(makeQuad(32, "repeat")); + renderer.drawMesh(makeQuad(96, "no-repeat")); + gl.finish(); + + // sample each quad at u = 1.25 (30px past its left edge), mid-height. + // repeat → u wraps to 0.25 → red half; no-repeat → clamps to the + // right edge → blue half. + const readPixel = (x, y) => { + const px = new Uint8Array(4); + gl.readPixels(x, 128 - y, 1, 1, gl.RGBA, gl.UNSIGNED_BYTE, px); + return px; + }; + const tiled = readPixel(32 - 24 + 30, 64); + const clamped = readPixel(96 - 24 + 30, 64); + + expect(tiled[0]).toBeGreaterThan(200); // red: wrapped back into the red half + expect(tiled[2]).toBeLessThan(50); + expect(clamped[2]).toBeGreaterThan(200); // blue: clamped to the right edge + expect(clamped[0]).toBeLessThan(50); + }); + + it("cache.delete(image) frees every per-repeat texture unit for the source", (ctx) => { + requireWebGL(ctx); + const image = makeSolidImage(); + const atlas = renderer.cache.get(image); + + // upload the same source under two wrap modes (this is what the + // per-mesh override produces; pre-fix it was also reachable by + // mutating `atlas.repeat` between uploads) + renderer.setBatcher("quad"); + atlas.repeat = "repeat"; + renderer.currentBatcher.uploadTexture(atlas); + atlas.repeat = "no-repeat"; + renderer.currentBatcher.uploadTexture(atlas); + expect(renderer.cache.units.get(image).size).toBe(2); + + const usedBefore = renderer.cache.usedUnits.size; + renderer.cache.delete(image); + + // pre-fix, freeTextureUnit only released the unit matching the + // atlas's CURRENT repeat field — the "repeat" unit stayed pinned + // in units/usedUnits until a full cache reset + expect(renderer.cache.units.has(image)).toBe(false); + expect(renderer.cache.usedUnits.size).toBe(usedBefore - 2); + }); +}); diff --git a/packages/melonjs/tests/mesh.spec.js b/packages/melonjs/tests/mesh.spec.js index a6436ee80..6b1e522fc 100644 --- a/packages/melonjs/tests/mesh.spec.js +++ b/packages/melonjs/tests/mesh.spec.js @@ -1302,7 +1302,7 @@ describe("Mesh × Camera3d world-space path", () => { // ── textureRepeat setting (tiling UVs, e.g. glTF default wrap) ────────── - it("textureRepeat applies the wrap mode to a real texture", () => { + it("textureRepeat is kept per-mesh and never mutates the shared atlas (#1503)", () => { const m = new Mesh(0, 0, { vertices: new Float32Array(9), uvs: new Float32Array(6), @@ -1312,10 +1312,14 @@ describe("Mesh × Camera3d world-space path", () => { normalize: false, textureRepeat: "repeat", }); - expect(m.texture.repeat).toBe("repeat"); + // the wrap lives on the mesh (threaded to the batcher at draw + // time), NOT on the per-image TextureAtlas shared with every + // other consumer of the same image + expect(m.textureRepeat).toBe("repeat"); + expect(m.texture.repeat).toBe("no-repeat"); }); - it("texture defaults to 'no-repeat' when textureRepeat is omitted", () => { + it("textureRepeat is undefined when omitted (mesh samples with the texture's own wrap)", () => { const m = new Mesh(0, 0, { vertices: new Float32Array(9), uvs: new Float32Array(6), @@ -1324,6 +1328,7 @@ describe("Mesh × Camera3d world-space path", () => { width: 10, normalize: false, }); + expect(m.textureRepeat).toBeUndefined(); expect(m.texture.repeat).toBe("no-repeat"); }); @@ -1339,5 +1344,8 @@ describe("Mesh × Camera3d world-space path", () => { textureRepeat: "repeat", }); expect(m.texture.repeat).not.toBe("repeat"); + // the per-mesh override is dropped too — a REPEAT sample of the + // shared 1×1 white pixel would just burn a texture unit for nothing + expect(m.textureRepeat).toBeUndefined(); }); }); From 3d43c0cc6dab5c1490d2a1d0a8d448e81331e99e Mon Sep 17 00:00:00 2001 From: Olivier Biot Date: Sat, 4 Jul 2026 07:42:30 +0800 Subject: [PATCH 2/2] fix(review): deleteTexture2D sweeps every per-repeat unit's GL binding MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot caught a real hazard the per-use override introduced: deleteTexture2D located the GL texture to delete via peekUnit(atlas) — one unit, keyed on the atlas's current repeat field — while a single atlas can now own several (source, repeat) units. cache.delete(image) freed them all, but the extra variants' WebGLTextures stayed in boundTextures; a later allocation reusing such a unit would treat the stale texture as already-uploaded and bind the wrong texture. deleteTexture2D now iterates cache.peekAllUnits(atlas) (new @ignore helper returning every unit for the texture's source) and deletes+unbinds each. Regression test added (revert-checked: fails against the single-peek code with a stale WebGLTexture on the freed unit). Also reworked the delete-leak test to set up its two units via the per-use uploadTexture override instead of mutating atlas.repeat — the anti-pattern this PR removes (second review comment). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_019t8kP8vp58ZrvaD2FqJU6A --- packages/melonjs/src/video/texture/cache.js | 15 ++++++ .../video/webgl/batchers/material_batcher.js | 19 +++---- .../melonjs/tests/mesh-texture-repeat.spec.js | 53 +++++++++++++++---- 3 files changed, 68 insertions(+), 19 deletions(-) diff --git a/packages/melonjs/src/video/texture/cache.js b/packages/melonjs/src/video/texture/cache.js index bac443208..3a5ffae3b 100644 --- a/packages/melonjs/src/video/texture/cache.js +++ b/packages/melonjs/src/video/texture/cache.js @@ -205,6 +205,21 @@ class TextureCache { return perRepeat.get(wrap); } + /** + * @ignore + * return every texture unit allocated for the given texture's source, + * across ALL repeat modes — the unload-time counterpart of the per-use + * `getUnit(texture, repeat)` override (#1503): a single source can hold + * one unit per wrap mode it was sampled with, and per-repeat `peekUnit` + * lookups would only ever find the one matching the texture's current + * `repeat` field. Not a hot path (allocates the result array). + */ + peekAllUnits(texture) { + const source = texture.sources.get(texture.activeAtlas); + const perRepeat = this.units.get(source); + return typeof perRepeat !== "undefined" ? [...perRepeat.values()] : []; + } + /** * @ignore * return the texture unit for the given texture, or -1 if not allocated diff --git a/packages/melonjs/src/video/webgl/batchers/material_batcher.js b/packages/melonjs/src/video/webgl/batchers/material_batcher.js index 5d45c61ee..460b7febf 100644 --- a/packages/melonjs/src/video/webgl/batchers/material_batcher.js +++ b/packages/melonjs/src/video/webgl/batchers/material_batcher.js @@ -265,19 +265,20 @@ export class MaterialBatcher extends Batcher { deleteTexture2D(texture) { if (typeof texture.getTexture === "function") { // Iterate every atlas registered under this image — post-#1448, - // the multimap can hold multiple atlases per image (one per - // repeat mode), each bound to its own GL texture in - // `boundTextures`. Without this loop the OTHER repeats' GL - // textures would be orphaned: `cache.delete(image)` frees - // every (source, repeat) unit, but only THIS texture's - // `boundTextures[unit]` would be deleted+unbound, leaving - // stale binds on the freed units until something overwrites. + // the multimap can hold multiple atlases per image — and, per + // atlas, EVERY (source, repeat) unit: a single atlas can own + // several per-repeat units via the per-use wrap override + // (meshes' `textureRepeat`, #1503), which a per-repeat + // `peekUnit` lookup (keyed on the atlas's current `repeat` + // field) would miss. `cache.delete(image)` below frees all of + // those units; any GL texture left behind in + // `boundTextures[unit]` would make a later allocation of the + // same unit look "already uploaded" and bind a stale texture. const image = texture.getTexture(); const cache = this.renderer.cache; if (cache.has(image)) { for (const atlas of cache.cache.get(image)) { - const unit = cache.peekUnit(atlas); - if (unit !== -1) { + for (const unit of cache.peekAllUnits(atlas)) { const texture2D = this.boundTextures[unit]; if (typeof texture2D !== "undefined") { this.gl.deleteTexture(texture2D); diff --git a/packages/melonjs/tests/mesh-texture-repeat.spec.js b/packages/melonjs/tests/mesh-texture-repeat.spec.js index e4c43212d..6252ce663 100644 --- a/packages/melonjs/tests/mesh-texture-repeat.spec.js +++ b/packages/melonjs/tests/mesh-texture-repeat.spec.js @@ -228,23 +228,56 @@ describe("Mesh textureRepeat vs shared TextureAtlas (issue #1503)", () => { const image = makeSolidImage(); const atlas = renderer.cache.get(image); - // upload the same source under two wrap modes (this is what the - // per-mesh override produces; pre-fix it was also reachable by - // mutating `atlas.repeat` between uploads) + // upload the same source under two wrap modes via the per-use + // override — what the per-mesh `textureRepeat` produces at draw + // time; the shared atlas is never touched renderer.setBatcher("quad"); - atlas.repeat = "repeat"; - renderer.currentBatcher.uploadTexture(atlas); - atlas.repeat = "no-repeat"; - renderer.currentBatcher.uploadTexture(atlas); + renderer.currentBatcher.uploadTexture( + atlas, + undefined, + undefined, + false, + true, + "repeat", + ); + renderer.currentBatcher.uploadTexture(atlas); // atlas's own wrap: "no-repeat" expect(renderer.cache.units.get(image).size).toBe(2); const usedBefore = renderer.cache.usedUnits.size; renderer.cache.delete(image); - // pre-fix, freeTextureUnit only released the unit matching the - // atlas's CURRENT repeat field — the "repeat" unit stayed pinned - // in units/usedUnits until a full cache reset + // pre-fix, delete's per-atlas freeTextureUnit only released the + // unit matching each atlas's CURRENT repeat field — the "repeat" + // unit stayed pinned in units/usedUnits until a full cache reset expect(renderer.cache.units.has(image)).toBe(false); expect(renderer.cache.usedUnits.size).toBe(usedBefore - 2); }); + + it("deleteTexture2D purges the GL binding of every per-repeat unit (no stale rebinds)", (ctx) => { + requireWebGL(ctx); + const image = makeSolidImage(); + const atlas = renderer.cache.get(image); + renderer.setBatcher("quad"); + const batcher = renderer.currentBatcher; + + batcher.uploadTexture(atlas, undefined, undefined, false, true, "repeat"); + batcher.uploadTexture(atlas); // atlas's own wrap: "no-repeat" + const units = [...renderer.cache.units.get(image).values()]; + expect(units.length).toBe(2); + for (const unit of units) { + expect(batcher.getTexture2D(unit)).toBeDefined(); + } + + batcher.deleteTexture2D(atlas); + + // the cache freed the units AND the batcher dropped both GL + // bindings. A stale `boundTextures[unit]` entry on a freed unit + // would make the next texture allocated on that unit look + // "already uploaded" and silently bind the wrong texture (the + // hazard flagged in the #1537 review). + expect(renderer.cache.units.has(image)).toBe(false); + for (const unit of units) { + expect(batcher.getTexture2D(unit)).toBeUndefined(); + } + }); });