WebGPU: changing texture uniforms requires a recompile of the material

[Mirei3D]

Description

WebGPU renderer merges multiple texture uniform nodes into one uniform if they use the same texture at compile time. If one of these uniforms is updated at a later point, this requires a recompile of the material, so it seems rather slow.

There is an additional problem, that forcing a recompile is not trivial if the uniform values aren't exposed are properties on the material (see #34392).

Reproduction steps

1. open the live example
2. the sphere should become yellow after a second but stays red instead
3. uncomment the // this.forceRecompile = 1; line
4. now it should work as expected

Code

// code goes here

Live example

https://jsfiddle.net/6fochL70/

Screenshots

No response

Version

r185

Device

No response

Browser

No response

OS

No response

[Mugen87]

WebGPU renderer merges multiple texture uniform nodes into one uniform if they use the same texture at compile time

Correct. This is actually an important optimization in order to reduce uniforms and bindings. I understand your code example feels like a "blind spot" in the system though. However fixing it is difficult and I think we must decide first if we want in general support the "two nodes sharing a texture at build time, diverging later" use case. I personally would say no because the related tracking we would have to add would potentially slow down the node cache key computation.

I suggest you accommodate your code at the dedup logic and initialize your nodes with empty texture objects. In this case, you don't even need a needsUpdate/recompile: https://jsfiddle.net/o6qz4jtb/1/

[gkjohnson]

However fixing it is difficult and I think we must decide first if we want in general support the "two nodes sharing a texture at build time, diverging later" use case.

I think the answer is that "optimizations" should not be undermining user intent in the node graph design in the first place. Users are specifying different uniform inputs with the goal of using as separate inputs with the given shape because they intend to use them that way. An "optimization" should not change the behavior on the input surface of the material. Issue #33145 reports a similar problem where TSL is silently changing the shape of a uniform input when a storage buffer is 1 in length, leading to this same rebuild issue. This is like merging all uniforms that have their value set to "1.0" when it's inevitable that users have exposed those uniforms separately for a reason.

It's great that optimizations are being done but the priority should be on retaining the users intent of use. Right now the biggest performance sink for TSL is the node graph rebuild so I don't think we should be adding silent, unconfigurable features like this that can break user code and cause unnecessary rebuilds. If we really want this optimization to be available then it should be guarded behind per-material flag like "mergeUniformsOnBuild" that defaults to false - but it should be determined if this is really a big enough win to justify first.

It should go without saying that this kind of auto-merging will also prevent materials from sharing shaders if the textures are configured in a certain way (possibly somewhat related to #33821).

I suggest you accommodate your code at the dedup logic and initialize your nodes with empty texture objects.

This feels like an incredibly hacky and not-very-understandable workaround for something that shouldn't be a problem.

[Mugen87]

I agree a reconsideration makes sense. Although the original code feels a bit like an edge case to me (we did not encounter it in our repo so far), we should handle this more gracefully. The "solution" I have shared in #34393 (comment) is a workaround for now.

To clarify, I believe the current system must be the default behavior since we don't know how users write their TSL. They might use the texture() TSL function every time when they want to do a sample and it would be a severe bug if we create for each texture node a new uniform.

So we should investigate first how the node system can detect the above use case (two nodes sharing a texture at build time, diverging later) and then trigger a rebuild/compile. The compile will be necessary since the uniforms structurally change.

Consequently, even with a detection/recompilation logic, developers who want best performance should not setup texture nodes with shared texture and later replace the reference. They should start with dedicated nodes right from the beginning if they want to avoid a shader recompilation.

[Mirei3D]

I suggest you accommodate your code at the dedup logic and initialize your nodes with empty texture objects. In this case, you don't even need a needsUpdate/recompile: https://jsfiddle.net/o6qz4jtb/1/

First of all, thanks for the swift response here and on the other issue! And yeah, I am already using this workaround in one shader, where I know both uniforms always end up with different values anyways.

I do worry what this means for authoring custom node materials in general. I'm porting our engine to WebGPU and there are quite a few custom materials and render passes that need to be rewritten. So I want to be sure, that the patterns we use will be the best ones from the beginning.

I agree a reconsideration makes sense. Although the original code feels a bit like an edge case to me (we did not encounter it in our repo so far)

Our materials have texture slots, which can be updated in our editor or - via code - in the final deployment. I anticipate that binding one texture to two slots will be somewhat rare and that binding a texture twice instead of once occasionally probably is negligible performance-wise. But avoiding a node tree rebuild every time a texture changes would be nice.

I do wonder though, if we implement customProgramCacheKey(), must the cache key change, when a texture uniform changes, that was previously merged with another uniform?

This is actually an important optimization in order to reduce uniforms and bindings. I understand your code example feels like a "blind spot" in the system though. However fixing it is difficult and I think we must decide first if we want in general support the "two nodes sharing a texture at build time, diverging later" use case. I personally would say no because the related tracking we would have to add would potentially slow down the node cache key computation.

I guess that this optimisation is necessary in part, because of how TextureNode was designed. TextureNode.sample() creates a new TextureNode every time it's called. It does seem like TextureNode already tracks a reference node though, so I wonder if it would be possible to create a UniqueTextureUniform subclass, which overrides getUniformHash() to return the id of the reference node. I'll give this a shot and report back. I don't feel very familiar with how the node system works under the hood yet, so do let me know if this would have any bad consequences.

[Mugen87]

I do wonder though, if we implement customProgramCacheKey(), must the cache key change, when a texture uniform changes, that was previously merged with another uniform?

I would say no. It's the responsibility of TextureNode to detect the exchange somehow. In #34392, part of the node graph depends on a custom state and that's where customProgramCacheKey() is relevant and must be implemented.

[Mirei3D]

I went ahead and subclassed TextureNode. It does seem to work, see: https://jsfiddle.net/8a629hmw/. In the WebGPU Inspector I can also see that this creates only a single render pipeline :)

Would there be any interest in supporting the use case in this way (as a subclass or maybe via a flag on TextureNode)? Would it cause any problems I'm not seeing right now?