Cloned ShaderMaterial won't upload its uniform textures

[0b5vr]

Describe the bug

ShaderMaterial.clone() fails? to clone texture uniforms to cloned one.
The intention of "?" above is I don't know I should treat this as a bug or an intended behavior.
The behavior is counter-intuitive anyway imo.

Inside the code

Here is what ShaderMaterial.clone() will do:

three.js/src/materials/ShaderMaterial.js

Line 86 in d9af995

this.uniforms = cloneUniforms( source.uniforms );

and here is how the cloneUniorms clone textures:

three.js/src/renderers/shaders/UniformsUtils.js

Lines 17 to 22 in d9af995

	if ( property && ( property.isColor ||
	property.isMatrix3 || property.isMatrix4 ||
	property.isVector2 || property.isVector3 || property.isVector4 ||
	property.isTexture || property.isQuaternion ) ) {
	dst[ u ][ p ] = property.clone();

This will leave .version of cloned textures to be 0.
Constructor of texture sets version to 0, and clone does not change the value.
And this leads to not upload assigned textures to GPU.

Reproducing the issue

I've made a working example that triggers the issue.

https://glitch.com/edit/#!/grand-plume-windflower

Workaround

There is a workaround code commented out in the Glitch example above.
Manually copying uniform textures to the cloned one will resolve the issue.
Setting .needsUpdate of cloned textures will also resolve this.

Expected behavior

I have to ask first whether the current behavior is intended or not. If this is not intended, I would like to make it ( reference copy || bump .version ) these uniform textures when ShaderMaterial.clone() is called.

Detailed context which probably doesn't matter for you

I'm doing the development of three-vrm.
I was trying to bump three.js version from r126 to r133.
three-vrm 1.0 uses extended ShaderMaterial (MToonMaterial namely) and a GLTFLoader plugin which loads the material.
(my current working branch is #1.0 if you're interested)

In GLTFLoader, there is a code which clones materials for corresponding material variants:

three.js/examples/jsm/loaders/GLTFLoader.js

Line 2980 in d9af995

cachedMaterial = material.clone();

Recently, one of variant conditions are changed from useVertexTangents to useDerivativeTangents
and this triggers the issue for the first time for us.
PR: #22584

Since this issue has a workaround it can be resolved by our side but it takes a whole business day to resolve this 😭
Understandable difficulty since we're using ShaderMaterial in an unusual way and there is no other users who combines ShaderMaterial with GLTFLoader plugins...

Platform

• Device: *
• OS: *
• Browser: *
• Three.js version: r133, at least. I don't see anyone changing these codes recently

[Mugen87]

The current behavior is actually intended.

I agree the implementation can be confusing but the problem is that the clone() method does not know whether a texture can be considered as "complete" or not. Hence, it can't set needsUpdate to true.

A class like TextureLoader does know the right moment in its onLoad() callback. In your case, you have to make a decision on app level.

[0b5vr]

the problem is that the clone() method does not know whether a texture can be considered as "complete" or not. Hence, it can't set needsUpdate to true.

100% agreed.

Referencing implementation of other builtin materials such as MeshStandardMaterial, it reference copies these textures when copy() is called.

three.js/src/materials/MeshStandardMaterial.js

Line 123 in d9af995

this.map = source.map;

What is the reason cloneUniforms actually clones textures instead of referencing them?
I thought it's natural to just reference copy instead.
I assume there is a reason though......

Edit: Also thanks for the quick response as usual.

[Mugen87]

What is the reason cloneUniforms actually clones textures instead of referencing them?

Um, that is actually a good point. I'm not sure why not just an assign happens like with built-in materials. It seems more consistent to me if built-in and custom materials are treated in the same way.

However, it could be difficult to change this now since there might be users who rely on the current behavior. E.g. they use different texture repeat/offset settings with the same image source (in context of spritesheets).

[wizgrav]

@Mugen87 can't you just add some flags to clone() to adjust the behaviour?

[0b5vr]

However, it could be difficult to change this now since there might be users who rely on the current behavior

That is also 100% understandable, yes...

Having a logic that detects textures with .version = 0 in somewhere near renderer.render and show a warning under certain condition might help us......?

[Mugen87]

Instead of setting needsUpdate to true in Texture.clone(), I wonder if we can solve the issue by honoring the version property in Texture.copy().

In this way textures wouldn't be marked too early for a texture upload. However, if a texture's image is already "ready", the renderer should detect it since the version of the cloned texture would be > 0.

@0b5vr Do you mind testing this approach in your project by adding this.version = source.version; in Texture.copy()?

[Mugen87]

Um, it's probably a bad idea. Existing texture objects can break when a higher version number gets replaced with a smaller one.

[mrdoob]

@Mugen87

Um, it's probably a bad idea. Existing texture objects can break when a higher version number gets replaced with a smaller one.

Hmm, how so?

[Mugen87]

Sorry, I have described the issue not correctly. It's about this line:

three.js/src/renderers/webgl/WebGLTextures.js

Line 367 in e1ead8c

if ( texture.version > 0 && textureProperties.__version !== texture.version ) {

Let's say you have two textures A and B. A has version 1 and B has version 0. The user loads texture B via TextureLoader and then copies B to A in the onLoad()callback. If Texture.copy() honors the version, A will still have a version of 1 and thus no upload will be executed.

[Mugen87]

Closed via #23637.

I don't think there will be a solution for the sync pattern mentioned in #23637 (comment). However, cloning textures in the callback function or by using async/await should now work.