fix(mesh): per-mesh textureRepeat — stop mutating the shared per-image TextureAtlas#1537
Merged
Conversation
…e TextureAtlas 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019t8kP8vp58ZrvaD2FqJU6A
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes #1503 by making Mesh.textureRepeat a per-mesh sampler override (threaded through MeshBatcher.addMesh → uploadTexture → TextureCache.getUnit) instead of mutating the shared per-image TextureAtlas.repeat, preventing wrap-mode leakage across consumers sharing the same image.
Changes:
- Introduce per-use wrap override plumbing:
Mesh.textureRepeat→MaterialBatcher.uploadTexture(..., repeat)→TextureCache.getUnit(texture, repeat). - Add
TextureCache.freeAllUnitsand updatecache.delete(image)to free units across all repeat variants for a source. - Add/adjust tests to validate shared-atlas immutability, per-mesh wrap correctness (including GL state + pixel readback), and cache cleanup.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/melonjs/src/renderable/mesh.js | Stores wrap override on the mesh (textureRepeat) instead of mutating the shared atlas. |
| packages/melonjs/src/video/webgl/batchers/mesh_batcher.js | Passes mesh.textureRepeat through to uploadTexture for per-draw sampler state. |
| packages/melonjs/src/video/webgl/batchers/material_batcher.js | Extends uploadTexture to accept a per-use repeat override and keys unit lookup by (source, repeat). |
| packages/melonjs/src/video/texture/cache.js | Adds per-use repeat override to getUnit/peekUnit and introduces freeAllUnits used by delete(image). |
| packages/melonjs/tests/mesh-texture-repeat.spec.js | New regression tests covering shared-atlas immutability, per-mesh wrap behavior, pixel readback, and cache cleanup. |
| packages/melonjs/tests/mesh.spec.js | Updates expectations to assert wrap is per-mesh and atlas repeat remains unchanged. |
| packages/melonjs/tests/gltf_model.spec.js | Updates expectations to assert glTF wrap is forwarded onto mesh.textureRepeat (not atlas mutation). |
| packages/melonjs/CHANGELOG.md | Documents the fix and the improved unit cleanup behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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 <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.
Closes #1503.
The bug
Meshapplied itstextureRepeatsetting by mutatingthis.texture.repeat— andthis.textureis theTextureAtlasshared per source image (renderer.cache.get(image)). The wrap mode therefore leaked image-globally: two meshes (or a mesh and a sprite/pattern) pointing at the same image with different wrap needs were last-writer-wins, and the loser silently sampled with the wrong wrap. Preventative today (glTF, the only in-treetextureRepeatproducer, decodes private per-asset images), but unguarded shared-state mutation waiting to bite.The fix (option A from the ticket)
The wrap now lives on the mesh (
mesh.textureRepeat) and is threaded throughMeshBatcher.addMesh→uploadTexture→TextureCache.getUnitat draw time — sampler state per use, the same texture/sampler separation modern GPU APIs enforce (GL sampler objects,GPUSampler; Godot 4 made the same per-use move fortexture_repeat). The heavy lifting already existed: the unit cache keys GL textures by(source, repeat)since #1448, so per-mesh wraps coexist on one image with distinct GL textures. The shared atlas is never written.Also fixed alongside:
TextureCache.delete(image)now sweeps the units of all the source's wrap-mode variants (newfreeAllUnitshelper). The granularfreeTextureUnitonly freed the unit matching the atlas's currentrepeatfield, pinning the other variants inunits/usedUnitsuntil a full cache reset.freeTextureUnititself deliberately stays granular — freeing one live consumer (e.g. one of two patterns over the same image) must not evict the other's unit, andtests/texture.spec.jsguards exactly that.Out of scope, left for #1410:
textureFilterhas the same shared-atlas mutation, but a true per-mesh filter needs the unit-cache key to discriminate by filter — noted in a code comment.Tests — written failing-first
tests/mesh-texture-repeat.spec.js(4 tests) was committed red against the pre-fix code (all 4 failed) and is green with the fix:MeshwithtextureRepeatleaves the shared atlas untouched(source, repeat)units and bothTEXTURE_WRAP_Svalues reach the GL sampler — pre-fix, only the last constructor's wrap was ever uploaded)"repeat"mesh tiles (wrapped UV lands red) while the"no-repeat"mesh clamps (edge texel blue)cache.delete(image)frees every per-repeat unit (pre-fix leak)Two existing tests that asserted the old mutation behavior (
mesh.spec.js,gltf_model.spec.js) were updated to assert the per-mesh model, and the white-pixel adversarial test now also checks the override is dropped for the fallback.Full suite: 4569 passed / 15 skipped / 0 failed. eslint + biome + build clean.
🤖 Generated with Claude Code
https://claude.ai/code/session_019t8kP8vp58ZrvaD2FqJU6A