Skip to content

Texture.replaceImage: take the image of another texture, already loaded - #19099

Open
enriqxxx wants to merge 14 commits into
BabylonJS:masterfrom
enriqxxx:texture-replace-image
Open

enriqxxx wants to merge 14 commits into
BabylonJS:masterfrom
enriqxxx:texture-replace-image

Conversation

@enriqxxx

@enriqxxx enriqxxx commented Oct 31, 2026 •

Copy link
Copy Markdown
Contributor

Forum: 64429 #5 and #6.

A streamed world swaps the images of many textures at once when it changes their levels. Texture.updateURL releases the old image right away, so the meshes that use it are not drawn until the new one has loaded, and for each texture it marks dirty every material of the scene that has it. This PR adds Texture.replaceImage(source): the texture takes the image of another texture, already loaded, without loading it again.

What it does.

  • The texture takes a reference to the source's internal texture and releases its own. The two then share the source's GPU texture, which stays alive until both are disposed, so the source can be disposed right after.
  • It keeps its own settings for the materials (UV transform, coordinatesIndex, alpha and wrapping) and adopts the source's loading description (url or buffer, mipmaps, sampling mode, format, sRGB buffer, loader options), so serializing it and loading it again gives the image it shows. The source's UV transform, which already holds the _invertVScale correction of its loading, is not copied.
  • Texture.Parse loads from rootUrl + name. A name that followed the url the image was loaded from follows the source's url, relative to the same root (going up from it for a source outside it, when both urls are relative to the page), or becomes the source's full url for an absolute or data url. The texture keeps that root for its next replacements and gives it to its clones. A name of its own is kept.
  • The materials that use the texture (for the scene's environment texture, every material of the scene, as the environmentTexture setter does: a PBR material without a reflection texture reads it without having it), frozen ones included, compute their defines again when something they turn into defines changes: the gamma space, the RGBD encoding, the linear specular LOD or the irradiance texture. What they only write into uniforms or bind (the size, the LOD generation, the LOD textures) reaches the frozen ones only, which markDirty(true) makes write it again. The others write it at their next bind, so replacing an image with the same image at another size, as streaming does, does not even ask them whether they use the texture. The effective gamma is the getter's (the internal texture's gamma state with !_useSRGBBuffer), read without setting it. The source's image keeps the source's gamma, also when its gamma state is not set yet and the two wrappers already share it. When the gamma space changes, the materials' image processing is marked dirty as well as their textures, so a StandardMaterial computes again the space of its reflection and refraction textures (IS_REFLECTION_LINEAR, IS_REFRACTION_LINEAR).
  • In a node material, TextureBlock and TriPlanarBlock write both color space conversions under their defines, whatever the texture's gamma space when the graph is built. So a material converts a texture whose image becomes linear. In WGSL, which has no overloads, NodeMaterialBuildState._toGammaSpace picks the function for the output's type: toGammaSpaceVec3 for a 3-component output, and for a float as a vec3f. When the vertex shader reads an output of the block, its helper functions are emitted in the vertex stage too, and the block names its pair of conversion defines once per build, so both stages test the same defines.
  • With snapshot rendering, in both modes, the snapshot is recorded again when the image or the defines change, since its bundles hold the bind groups of the previous image.
  • An IblCdfGenerator whose source's image is replaced generates its maps again, at the new size when it changed: replaceImage notifies an internal observable of the texture (_onImageReplaced), which the generator subscribes to in its iblSource setter and releases when the source changes and on dispose.
  • It returns false, changing neither texture, when the image cannot be taken: either texture is not a plain Texture (a subclass manages its image: render target, procedural, raw, dynamic and video textures, also a video texture showing its poster or a procedural texture its fallback), or the image of either texture is not a 2D image loaded from a url or a buffer and ready (render target, procedural, raw, dynamic and video textures are refused), the two belong to different engines, the source has neither a url nor a buffer, or their effective orientations differ (invertY, and whether loading inverted the V scale). Replacing a texture with itself returns true.

Playground (on a build with this PR): https://playground.babylonjs.com/#2IS5NN#0. 600 boxes, every two sharing an unlit, frozen material, take the images of 300 textures loaded ahead, and each frame the canvas is read back at the center of each box. On this branch, all 600 boxes show their new images on the first frame after the calls, on WebGL2 and on WebGPU with compatibilityMode on and off and with snapshot rendering in STANDARD and FAST modes; no frame shows an old image or a missing box, and no material is marked dirty (no hasTexture call). Recording a STANDARD snapshot again is needed too: without it, the first frame still showed the old images and the next ones showed none of the boxes.

Unit tests.

  • babylon.texture.replaceImage.test.ts, 23, all failing on master, where the method does not exist:
    • it takes the source's image and loading description, sampling mode included, and keeps its own settings for the materials;
    • serialize and parse, 6: the name follows the url with an empty root url and relative to a root url (a parsed scene); a name of its own is kept; it goes up from the root for a source outside it, and takes a source's absolute url; the root is kept across replacements, back into the root too; a clone gets the root;
    • it refuses what it cannot take or replace, leaving both textures unchanged: another orientation, a V scale inverted by loading, an image still loading, no loading description, a cube image, a raw image, a render target as the source or as the texture, a dynamic image replaced, and another engine;
    • a texture replacing itself;
    • an image two wrappers already share, and one that has no gamma state yet, which takes the source's gamma;
    • a source of the same engine that has no scene;
    • the source disposed afterwards;
    • a size-only change marks no material that is not frozen and does not ask them whether they use the texture, and a change of defines marks them;
    • a node material converts its texture to gamma space once its gamma space image is replaced with a linear one;
    • a real PBR material and a frozen one without a reflection texture are reached when the scene's environment texture changes its gamma, then its size;
    • a frozen material computes again what it reads from the image, only when it changes;
    • the source's gamma kept when its image has none set yet, and the materials marked dirty only when the effective gamma changes;
    • a snapshot recorded again, in the STANDARD mode as in the FAST one;
    • from the last review: a texture the vertex shader reads, converted to gamma and to linear space once its image is replaced, with the helpers in the vertex shader; a standard material's reflection and refraction space once their gamma space images are replaced with linear ones; the IBL CDF maps of an environment generated again from its new image, at its new size.
  • textureBlock.test.ts and triPlanarBlock.test.ts, 4 more each: both conversions written under their defines for a gamma space texture, for an rgb and an r output, in GLSL and in WGSL.
  • babylon.texture.replaceImage.wrappers.test.ts (jsdom), 2: a video texture showing its poster and a procedural texture showing its fallback are refused, as the source and as the texture.

Our streamed world. On the bench of our 8c644 world (272 tiles, 1,082 draws, snapshot rendering on, materials not frozen), replaceImage swaps 16 textures every 60 frames: 368 replacements per run, none refused. In 8 runs in ABBA order, f1fb914 and 0bbb2c6 render the same image: 0 pixels differ between the two heads and between runs of the same head, except 2 pixels of the browser's first run, on the 0bbb2c6 arm. No console error in either.

All the core unit tests on f1fb914 (295 files): 4,873 passed, 1 expected fail (already marked on master).

Prepared by Mema, the AI of our studio 53ON (Claude in the 53ON harness, with MemoriaBios, our long-term memory), and reviewed by me.

🤖 Generated with Claude Code

enriqxxx and others added 2 commits October 30, 2026 20:08
A texture can take the image of another one, already loaded, without
loading it again: replaceImage(source). It keeps its own settings for the
materials (UV transform, coordinatesIndex, alpha, wrapping) and takes the
source's loading description (url or buffer, mipmaps, format, sRGB buffer,
loader options), so serializing it and loading it again gives the image it
shows; the two textures share the source's GPU texture. It returns false,
changing neither texture, for anything but ordinary loaded 2D image
textures of the same engine with the same effective orientation.

- The source's image keeps the source's gamma, also when its gamma state
  is not set yet. The materials are marked dirty only when the effective
  gamma or RGBD encoding changes; otherwise they bind the new image at
  their next draw.
- With snapshot rendering, the snapshot is recorded again.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ARD snapshots again

- The images it takes and replaces must be 2D images loaded from a url
  or a buffer, and ready. It checked the class name, but render target,
  procedural and raw textures do not override getClassName, so a render
  target could take an image.
- A STANDARD snapshot also replays the bundles it recorded: it binds the
  new image to the materials but draws nothing new, and once the previous
  image is released the boxes of a Playground showed nothing from the
  second frame on. The snapshot is recorded again in both modes.
- The sampling mode is the image's: a later load of the texture uses the
  one it showed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@deltakosh deltakosh self-assigned this Oct 31, 2026

@deltakosh deltakosh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @enriqxxx,

Nice API for streaming. The guards (2D url/buffer images only, same engine, matching orientation), taking the reference before releasing the own image, and the snapshot reset look good. One issue:

  • Serialization round-trip (texture.pure.ts:661): the doc says serializing the texture and loading it again gives the image it shows, but only url is updated while name keeps the old image. Texture.Parse loads from rootUrl + parsedTexture.name by default (parsedTexture.url is only used for data URLs or with Texture.UseSerializedUrlIfAny), so replacing old.png with new.png and round-tripping the scene reloads old.png. Please update name when it follows the old url (or otherwise make the round-trip hold), or adjust the doc, and add a serialize/parse test.

Non-blocking: a frozen material whose UBO holds image-dependent values (for example a PBR 2D reflection texture's width in vReflectionMicrosurfaceInfos) keeps the old value when the replacement has a different size, since no material is marked dirty in that case. Worth a line in the doc.

Thanks!

@Popov72

Popov72 commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@bjsplat

bjsplat commented Nov 2, 2026

Copy link
Copy Markdown
Collaborator

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@bjsplat

bjsplat commented Nov 2, 2026

Copy link
Copy Markdown
Collaborator

Snapshot stored with reference name:
refs/pull/19099/merge

Test environment:
https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/19099/merge/index.html

To test a playground add it to the URL, for example:

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/19099/merge/index.html#WGZLGJ#4600

Links to test your changes to core in the published versions of the Babylon tools (does not contain changes you made to the tools themselves):

https://playground.babylonjs.com/?snapshot=refs/pull/19099/merge
https://sandbox.babylonjs.com/?snapshot=refs/pull/19099/merge
https://gui.babylonjs.com/?snapshot=refs/pull/19099/merge
https://nme.babylonjs.com/?snapshot=refs/pull/19099/merge

To test the snapshot in the playground with a playground ID add it after the snapshot query string:

https://playground.babylonjs.com/?snapshot=refs/pull/19099/merge#BCU1XR#0

If you made changes to the sandbox or playground in this PR, additional comments will be generated soon containing links to the dev versions of those tools.

@bjsplat

bjsplat commented Nov 2, 2026

Copy link
Copy Markdown
Collaborator

@bjsplat

bjsplat commented Nov 2, 2026

Copy link
Copy Markdown
Collaborator

🟢 Memory Leak Test Results

4 passed, 0 leaked out of 4 scenarios

🟢 All memory leak tests passed — no leaks detected.

Passed Scenarios (4)
Scenario Package
Core Playground #2FDQT5#1508 @babylonjs/core
Core Playground #T90MQ4#14 @babylonjs/core
Core Playground #8EDB5N#2 @babylonjs/core
Core Playground #LL5BIQ#636 @babylonjs/core

@bjsplat

bjsplat commented Nov 2, 2026

Copy link
Copy Markdown
Collaborator

@bjsplat

bjsplat commented Nov 2, 2026

Copy link
Copy Markdown
Collaborator

⚡ Performance Test Results

🟢 All performance tests passed — no regressions detected.

@bjsplat

bjsplat commented Nov 2, 2026

Copy link
Copy Markdown
Collaborator

enriqxxx and others added 4 commits November 2, 2026 13:00
…alize/parse round-trip loads the image shown

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…he root the image was loaded from, also in a parsed scene

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ge, frozen ones included; engines compared by image; snapshot reset only on change

- The materials that use the texture are marked with markDirty(true), which also reaches frozen materials, whenever something they read from the image besides its texels changes (gamma, RGBD, size, LOD generation, irradiance texture, type, format), so a frozen PBR material rewrites a 2D reflection texture's width and LOD in its uniform buffer. An image read the same way marks nothing.
- The engines are compared on the two validated images, so a source created with an engine and no scene is taken.
- A snapshot is recorded again only when the image or what the materials read from it changes.
- A name that followed the url goes up from the root for a source outside it, when both urls are relative to the page.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…als; defines invalidate every material

What the materials turn into defines (gamma, RGBD, linear specular LOD, irradiance texture) invalidates every material that has the texture: TextureDirtyFlag, or markDirty(true) for a frozen one. What they only write into uniforms or bind (size, LOD generation, LOD textures) reaches only the frozen materials, since the others write it at every bind: replacing an image with the same image at another resolution, as texture streaming does, marks no material that is not frozen. textureType and textureFormat are no longer compared: no material reads them. The snapshot is recorded again when the image or the defines change.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@enriqxxx

enriqxxx commented Nov 2, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @Popov72, all four addressed in aef6f69 and f00ad6f (on top of 7456174 and 1069819, which answered the round-trip point of the previous review and were not pushed yet when you reviewed 893768e).

1. Round trip. replaceImage now updates name when it followed the url the image was loaded from (7456174, 1069819). The root is read from the url the own image was actually loaded from (the internal texture's url): after Texture.Parse, the texture's url and name hold the serialized values, not rootUrl + name. The name becomes the source's url relative to that root, or the source's full url when it is absolute or a data url (which Texture.Parse loads from the name alone). A source outside the root now round-trips too, without any opt-in: when both urls are relative to the page, the name goes up from the root (scenes/ + ../other/new.png).
Two cases keep the name. A name of its own (say "ground") is kept on purpose: Texture.Parse loads from it exactly as it does for a renamed texture that was never replaced, and replaceImage should not rename the user's texture. A root that cannot be related to a page-relative source without the page url (a root-relative or ../ root) is the other case; the doc says so.
Tests: each one serializes the texture and parses it again without the internal texture's id, as a file saved in another session would be, and asserts the url the parsed texture actually loads (its internal texture's url, which is what the loader fetches). For the source outside the root, both urls are resolved against a page url and compared. keeps a name of its own... passes on every head, since it describes what Texture.Parse already does for a renamed texture.

2. Image-dependent material state. replaceImage now reads what the materials take from the image besides its texels, before and after the swap, in two groups:

  • what they turn into defines: gammaSpace, isRGBD, linearSpecularLOD (LINEARSPECULARREFLECTION, SS_LINEARSPECULARREFRACTION) and irradianceTexture (USEIRRADIANCEMAP);
  • what they only write into uniforms or bind: getSize(), lodGenerationScale, lodGenerationOffset (vReflectionMicrosurfaceInfos, vRefractionMicrosurfaceInfos) and the three LOD textures.

textureType and textureFormat are not in either group: no material reads them. sphericalPolynomial only applies to cube images, which replaceImage refuses.
When the defines group changes, every material that has the texture is invalidated. A material that is not frozen is marked with MATERIAL_TextureDirtyFlag. A frozen one gets markDirty(true), because it skips the dirty flags: that resets _wasPreviouslyReady, so it runs isReadyForSubMesh and its defines again, and it sets _forceRebindOnNextCall, so bindForSubMesh writes its uniform buffer even when it is frozen and synchronized (the reflection width and LOD in BindIBLParameters).
When only the uniform group changes, only the frozen materials get markDirty(true). The others write these uniforms at every bind, so marking them would add work for nothing. That is the ordinary path you asked to keep cheap, and it is also the common one: texture streaming replaces an image with the same image at another resolution, so the size changes on almost every replacement. In our streaming world, marking the materials dirty on each streamed swap cost about 1 ms per frame and a hitch every 500 ms; without it, the same route gave +3.9% fps and −17% on the p95 frame time. I also measured this head against 893768e on a streamed world in WebGPU: 272 tiles, 951 meshes, 287 materials and 345 images, with snapshot rendering on. Every 60 frames, 16 textures are replaced, and the same calls are repeated on the next frame (368 replacements per run of 600 frames). Over 3 batches of 12 runs in ABBA order, the median CPU time per frame moved by +0.5, −0.6 and 0.0 ms from one batch to the next, so there is no consistent difference. The frame times are the same (p50 8.3 ms, p95 12.5 ms), the snapshot is recorded again 268 times per run instead of 368, and the last frames of both are identical (0 px).
Tests: marks no material that is not frozen when only the size changes, as streaming does, and marks it when its defines change, and makes a frozen material that uses it compute again what it reads from the image, only when it changes. The second draws a frozen material once: a same-size replacement leaves its draw wrapper ready, and a 1024 replacement of a 256 image makes it not ready and forces the rebind. The gamma test checks the frozen case through markDirty. These are NullEngine tests at the draw-wrapper level: NullEngine has no uniform buffers, so they cannot see the stale uniform itself. Happy to add a visualization test with a frozen PBR material if you want it in this PR.

3. Engine ownership. The engines are compared on the two validated images (image.getEngine() !== own.getEngine()), so the source no longer needs a scene. Only the destination's scene is used, to reach its materials.
Test: takes a source of the same engine that has no scene (a new Texture(url, engine) source). A source of another engine is still refused.

4. Snapshot reset (non-blocking). The snapshot is recorded again only when the image or the defines change.
Test: keeps an image two wrappers already share now also checks that the snapshot is not reset. records a snapshot again, in the STANDARD mode as in the FAST one still checks the reset for a new image.

The branch's tests: 14 passed. Every test that these two commits add or change fails without them (5 of 14 fail on 1069819). The one exception is the streaming test, which guards the cheap path: it passes on 1069819 and fails on aef6f69, which marked every material on a size change. All core unit tests (294 files): 4,854 passed, 1 expected fail (already marked on master). tsc of core: 0 errors; prettier: clean; eslint: no errors. Visualization tests run locally on this head and on 893768e: WebGPU 867 of 907 passed and WebGL2 876 of 891 passed. Every failure there also fails on 893768e on this machine, except one test that is flaky here (Gaussian Splatting LOD COMPOUND, which passed 3 out of 3 when rerun).

Prepared by Mema, the AI of our studio 53ON (Claude in the 53ON harness, with MemoriaBios, our long-term memory), and reviewed by me.

@deltakosh deltakosh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @enriqxxx,

Thanks, the single replacement now round-trips correctly. Consecutive replacements outside the root still lose it (texture.pure.ts:670-699):

With a texture named x.png loaded from scenes/x.png, replaceImage with a source at other/a.png sets name = "../other/a.png". A second replaceImage with other/b.png (or back to scenes/c.png) recomputes nameRoot from own.url = other/a.png, which does not end with ../other/a.png, so nameRoot is null and the name is left at ../other/a.png. Serializing and parsing then loads a.png instead of the image shown.

Please keep the root the name is relative to across replacements (e.g. store it on the first swap), and add a round-trip test with two successive swaps.

Thanks!

…placements, and in clones

A name that went up from the root (../other/a.png) no longer tells the root, so the next replacement left it unchanged and a round
trip loaded the previous image. The root is kept from the replacement that set the name, while the name is still the one it gave,
and a clone takes it with the name it copies.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Popov72

Popov72 commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command.

@enriqxxx

enriqxxx commented Nov 2, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @deltakosh, fixed in 680f2d7 (on top of f00ad6f).

Consecutive replacements outside the root. The texture now keeps the root its name is relative to. The replacement that sets the name stores the root, and the next replacement reuses it while the name is still the one it gave. Otherwise the root is read again from the url the own image was loaded from, as before. So a name of its own given between two replacements is still kept, and a parsed texture still finds its root from rootUrl + name. One helper computes the name from a root and a url. It is used both to check the kept root and to compute the new name, so the two cannot disagree.

With your example, a texture named x.png loaded from scenes/x.png takes three names in turn:

  • ../other/a.png with a source at other/a.png;
  • ../other/b.png with a source at other/b.png;
  • c.png back in the root, with a source at scenes/c.png.

Each round-trip loads the image shown.

Clones. The same loss would hit a clone. clone() copies the name, and the clone loads from the image's url (other/a.png), so it could not tell the root either, and its next replacement would keep the previous name. clone() now gives the clone the root together with the name.

Tests:

  • keeps the root across replacements: a name that went up from it follows the next source, back into the root too is your example. After the second replacement it serializes and parses the texture, and compares the url the parsed texture loads with the source's, both resolved against a page url. Then it goes back into the root.
  • gives a clone the root its name is relative to.
  • keeps a name of its own... now also renames the texture between two replacements outside the root. It checks that the kept root never overrides a name of its own.

The first two fail on f00ad6f; the third passes on both heads.

The branch's tests: 16 passed. All core unit tests (294 files): 4,856 passed, 1 expected fail (already marked on master). tsc of core: 0 errors (a full check, without the incremental cache); prettier: clean; eslint: no errors. The change only touches the name and the clone, not rendering, so I did not run the visualization tests again.

Prepared by Mema, the AI of our studio 53ON (Claude in the 53ON harness, with MemoriaBios, our long-term memory), and reviewed by me.

@deltakosh deltakosh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @enriqxxx,

Keeping _nameRoot (and checking it still produces the current name) fixes consecutive outside-root replacements: the second swap now resolves ../other/b.png from scenes/, and clones inherit the root. Tests cover both. Approved.

Thanks!

@Popov72

Popov72 commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command.

enriqxxx and others added 2 commits November 3, 2026 09:05
…becomes linear, its conversion written whatever the image at build

TextureBlock and TriPlanarBlock wrote the conversion of a texture to gamma space only when the texture was linear when the graph was
built, though its define follows the texture's image. After Texture.replaceImage gave a gamma space texture a linear image, the
define asked for a conversion the shader did not have, and the colors were wrong. Both blocks now write both conversions under their
defines. In WGSL, TextureBlock converts a 3-component output with toGammaSpaceVec3, as TriPlanarBlock does, since WGSL has no
overload of toGammaSpace for it. The test builds a node material with a gamma space texture, replaces its image with a linear one,
and checks that the effect it then draws with converts the texture.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ting a shared image's gamma, and only frozen materials asked on uniform changes

What the materials read from the image before the swap was read through the gammaSpace getter, which sets the gamma of an image that
has none yet: when both wrappers already shared the image, it took this texture's gamma instead of the source's, and both then read
gamma space. It is now read as the getter reads it, without setting it, so the image takes the source's gamma. The previous state
is kept in named scalars, compared one by one after the swap, instead of four arrays and two closures for each replacement, with the
same getSize() result and the same decisions. When only what the materials write into uniforms or bind changes, a material that is
not frozen is skipped before hasTexture(): it writes them at its next bind anyway.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@enriqxxx

enriqxxx commented Nov 4, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @Popov72 and @deltakosh, both points are addressed in 0bbb2c6.

The scene's environment texture. A PBR material without a reflection texture reads scene.environmentTexture (_getReflectionTexture() falls back to it), but its hasTexture() only checks its own slots, so the invalidation skipped it. When the replaced texture is the scene's environment texture, the replacement now reaches every material of the scene, as the environmentTexture setter does. Frozen ones are forced too (markDirty(true)), and for a size-only change only the frozen ones are reached, as for any texture. Test: reaches the materials that read it as the scene's environment texture, frozen ones included. It uses a real PBRMaterial and a frozen one without a reflection texture, without stubbing hasTexture: the environment's image is replaced with another gamma, then with another size. It fails on f42ae7d.

The wrappers, not only their images. Both textures must now be plain Textures, checked before anything changes. A subclass manages its own image: a video texture showing its poster, or a procedural texture showing its fallback, has a ready image loaded from a url, which the image checks alone accepted. Tests, in a jsdom suite:

  • refuses a video texture showing its poster, as the source and as the texture, with a real VideoTexture;
  • refuses a procedural texture showing its fallback texture, as the source and as the texture, with a real ProceduralTexture whose effect fails to compile.

Both fail on f42ae7d, where the replacement was accepted.

As suggested for the node material post-processes and procedural textures, the doc now says they follow a change of gamma space once they are built again, as with the gammaSpace setter.

All the core unit tests on 0bbb2c6 (295 files): 4,869 passed, 1 expected fail (already marked on master). tsc of core reports 0 errors in a full check, prettier is clean, and eslint reports no new warning.

Prepared by Mema, the AI of our studio 53ON (Claude in the 53ON harness, with MemoriaBios, our long-term memory), and reviewed by me.

@deltakosh deltakosh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @enriqxxx,

The three points of my last review are resolved (environment-texture consumers, unsupported wrappers, node post-process note). Remaining items, all at once:

  1. Vertex-stage conversion has no helpers - textureBlock.pure.ts:620-633 now always writes both toGammaSpace/toLinearSpace calls under their defines, so a later replaceImage can switch them on without rebuilding the graph. But helperFunctions is emitted only in the fragment path (_buildBlock, line 717); the vertex path (_injectVertexCode -> _writeOutput(..., true)) never emits it. A TextureBlock read in the vertex shader with convertToGammaSpace = true whose gamma-space image is replaced by a linear one (or the reverse with convertToLinearSpace) then fails to compile, unless another block happens to bring the include. Please emit the helpers in the vertex state too and cover the swap.
  2. StandardMaterial reflection/refraction space stays stale - texture.pure.ts:761 marks non-frozen consumers with MATERIAL_TextureDirtyFlag only, but StandardMaterial recomputes IS_REFLECTION_LINEAR / IS_REFRACTION_LINEAR only under _areImageProcessingDirty (standardMaterial.pure.ts:950-956). Replacing a gamma reflection/refraction image with a linear one keeps the old conversion. Please also mark image-processing dirty when gammaSpace changes, with a StandardMaterial regression.
  3. IBL CDF maps keep the previous environment - IblCdfGenerator builds its procedural textures with refreshRate = 0 and its iblSource setter returns early on the same texture (iblCdfGenerator.ts:63-64), and replaceImage does not notify onLoadObservable. Replacing the environment image therefore leaves IBL shadows (and anything using the CDF / dominant direction) on the old light distribution, and on the old map size when dimensions differ. Please have the generator regenerate when its source image is replaced (or document the limitation explicitly).
  4. packages/dev/core/test/unit/Materials/babylon.texture.replaceImage.wrappers.test.ts:72 uses engine.createEffect.bind(engine); Function.bind is prohibited under packages/dev/** (.github/instructions/prohibited-apis.instructions.md). Please capture the method and call it with .call(engine, ...) from the arrow.

Thanks!

@Popov72

Popov72 commented Nov 4, 2026

Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@bjsplat

bjsplat commented Nov 4, 2026

Copy link
Copy Markdown
Collaborator

🟢 Memory Leak Test Results

4 passed, 0 leaked out of 4 scenarios

🟢 All memory leak tests passed — no leaks detected.

Passed Scenarios (4)
Scenario Package
Core Playground #2FDQT5#1508 @babylonjs/core
Core Playground #T90MQ4#14 @babylonjs/core
Core Playground #8EDB5N#2 @babylonjs/core
Core Playground #LL5BIQ#636 @babylonjs/core

@bjsplat

bjsplat commented Nov 4, 2026

Copy link
Copy Markdown
Collaborator

@bjsplat

bjsplat commented Nov 4, 2026

Copy link
Copy Markdown
Collaborator

@bjsplat

bjsplat commented Nov 4, 2026

Copy link
Copy Markdown
Collaborator

⚡ Performance Test Results

🟢 All performance tests passed — no regressions detected.

@bjsplat

bjsplat commented Nov 4, 2026

Copy link
Copy Markdown
Collaborator

…ial reflection/refraction space, IBL CDF maps follow the new image

- TextureBlock emits the helper functions in the vertex shader too, and keeps one pair of conversion defines per build for both
  shaders: the fragment build named new ones, so the conversions of a texture the vertex shader reads were under defines nothing set.
- replaceImage also marks the image processing defines dirty when the gamma space changes: StandardMaterial computes
  IS_REFLECTION_LINEAR / IS_REFRACTION_LINEAR there.
- Texture notifies an internal observable when replaceImage gives it another image; IblCdfGenerator regenerates its maps from it.
- The wrappers test no longer uses Function.bind.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@deltakosh

Copy link
Copy Markdown
Contributor

Up @enriqxxx

@enriqxxx

enriqxxx commented Nov 5, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @deltakosh and @Popov72. The four points are addressed in f1fb914.

1. Helpers in the vertex stage. _injectVertexCode now emits helperFunctions in the vertex state when an output of the block is read there. Covering the swap found a second issue, on master too: the conversion defines of a TextureBlock read in the vertex shader never turned on. The fragment pass named the block's defines again (ISLINEAR1 was set while the vertex shader tested ISLINEAR0). A block now names its pair of conversion defines once per build. Tests: converts a texture the vertex shader reads to gamma space once its image is replaced, with the helpers in the vertex shader, and the same to linear space.

2. StandardMaterial reflection and refraction space. When gammaSpace changes, replaceImage now marks the image processing dirty as well as the textures, so IS_REFLECTION_LINEAR and IS_REFRACTION_LINEAR are computed again. Test: gives a standard material the space of its reflection and refraction once their gamma space images are replaced with linear ones.

3. IBL CDF maps. replaceImage now notifies an internal observable of the texture (_onImageReplaced). IblCdfGenerator subscribes to it in its iblSource setter: on a replacement it resets the dominant direction and generates its maps again, at the new size when it changed. The subscription is released when the source changes and on dispose. The doc of replaceImage says so. Test: makes the IBL CDF maps of an environment generate again from its new image, at its new size.

4. Function.bind. The wrappers test now calls createEffect.call(engine, ...) from the arrow.

Each new or changed test fails with the source of 0bbb2c6 (6 of them), and all 25 tests of the two replaceImage suites pass on f1fb914. On f1fb914, the core unit suite passes (295 files, 4,873 tests) and core type-checks with no errors. Prettier is clean, and eslint reports no new warning in the files of the PR. On the bench of our 8c644 world (272 tiles), with replaceImage on 16 textures every 60 frames (368 replacements per run, none refused), f1fb914 renders the same image as 0bbb2c6 in 8 runs in ABBA order: 0 pixels differ, except 2 pixels of the browser's first run, on the 0bbb2c6 arm, and no console error in either.

Prepared by Ramon and Mema, the AI of our studio 53ON (🖐️, 👀, 🧮🔨🪛, 🧠 human memory simulation).

@deltakosh deltakosh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @enriqxxx. All four points from 0bbb2c6 are addressed at f1fb914:

  • The vertex-stage TextureBlock conversion now emits helperFunctions.
  • replaceImage marks the image-processing dirty flag when the gamma space changes, so StandardMaterial recomputes IS_REFLECTION_LINEAR / IS_REFRACTION_LINEAR.
  • The IBL CDF generator regenerates its maps from _onImageReplaced.
  • The .bind test is fixed.

LGTM.

@deltakosh
deltakosh enabled auto-merge (squash) November 5, 2026 05:05
@Popov72

Popov72 commented Nov 5, 2026

Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@bjsplat

bjsplat commented Nov 5, 2026

Copy link
Copy Markdown
Collaborator

Reviewer - this PR has made changes to one or more package.json files.

@bjsplat

bjsplat commented Nov 5, 2026

Copy link
Copy Markdown
Collaborator

@bjsplat

bjsplat commented Nov 5, 2026

Copy link
Copy Markdown
Collaborator

🟢 Memory Leak Test Results

4 passed, 0 leaked out of 4 scenarios

🟢 All memory leak tests passed — no leaks detected.

Passed Scenarios (4)
Scenario Package
Core Playground #2FDQT5#1508 @babylonjs/core
Core Playground #T90MQ4#14 @babylonjs/core
Core Playground #8EDB5N#2 @babylonjs/core
Core Playground #LL5BIQ#636 @babylonjs/core

@bjsplat

bjsplat commented Nov 5, 2026

Copy link
Copy Markdown
Collaborator

@bjsplat

bjsplat commented Nov 5, 2026

Copy link
Copy Markdown
Collaborator

@deltakosh

Copy link
Copy Markdown
Contributor

Up @enriqxxx

enriqxxx and others added 2 commits November 6, 2026 07:41
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…conversion

The two tests built a RegExp from the expected text escaping only ".",
"(" and ")", which CodeQL reports: the backslash stayed unescaped.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
auto-merge was automatically disabled November 6, 2026 17:39

Head branch was pushed to by a user without write access

@enriqxxx

enriqxxx commented Nov 6, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @deltakosh. Master is merged (8d2c342), and the diff of the PR is unchanged. The two CodeQL alerts were real, in textureBlock.test.ts:36 and triPlanarBlock.test.ts:54: those tests built a RegExp from the expected text escaping only ., ( and ). 5764c81 escapes every metacharacter, the backslash included.

On 5764c81, the core unit suite passes (316 files, 5,163 tests, plus the expected fail already marked on master), and core type-checks with no errors.

Prepared by Ramon and Mema, the AI of our studio 53ON (🖐️, 👀, 🧮🔨🪛, 🧠 human memory simulation).

@deltakosh
deltakosh enabled auto-merge (squash) November 6, 2026 17:53

@deltakosh deltakosh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed f1fb914...5764c81.

  • Master merge 8d2c342: master changed none of this PR's files since the previous base, so the merge is clean on the PR's side.
  • 5764c81: the TextureBlock/TriPlanarBlock tests now escape every regex metacharacter of the expected conversion ([.*+?^${}()|[\]\\]), not just ., ( and ). This addresses the CodeQL incomplete-escaping report.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants