Three.js: THREE.UniformsUtils.merge shoudnt clone the texture

[Makio64]

Merging should use the existing texture, not cloning it ( same for others parameters ), its counter intuitive and memory expensive. I find myself in the case I wanted to just add some fog uniforms & other common to my rawShader and it take me time to figure out my texture was cloning and thats why my shader wasnt working and my texture share between all my instance not updated..

If the user want to clone the texture then he should use UniformsUtils.clone(myUniforms) and then merging it, trying to hide this step can lead to some serious bugs.

[mrdoob]

Hmm, the problem is not the Texture but the Image.

[Zob1]

I ran into this issue with a Color uniform.

[mrdoob]

@Zob1 do you mind elaborating?

[Makio64]

@mrdoob Hmm, what do you mean the problem is the Image.?

Maybe i wasnt clear, for exemple :

a ={ texture: { type:'t', value:mytexture }  }
b ={ opacity: { type:'f', value:0 }  }
uniforms = THREE.UniformsUtils.merge( [a,b] )

Expectation :

uniforms = {
   texture : { type:'t', value:mytexture }
   opacity : {  type:'f', value:0 }
}

Currently :

uniforms = {
   texture :  { type:'t', value:mytextureCloned } // mytextureCloned !== mytexture
   opacity :  { type:'f', value:0 }
}

same for other object ( including color ) as @Zob1 said.

And the problem is after if the original color/texture/vector/etc.. is changed, it will note change into the merged uniforms.

It come from here :
https://github.com/mrdoob/three.js/blob/master/src/renderers/shaders/UniformsUtils.js#L39

I feel this "if" should be removed cause cloning looks like out of range of a merge utils function

Cheers!

[WestLangley]

I believe the intention is for you to merge the uniforms data structures and assign values afterward.

var a = { texture: { type: 't', value: null } };
var b = { opacity: { type: 'f', value: 0 } };
var uniforms = THREE.UniformsUtils.merge( [ a, b ] );

uniforms.texture.value = myTexture;

This is how it is done in the examples.

If you assign the texture a value prior to merging, the texture will be cloned and the needsUpdate flag will be set to false. Obviously, that is not what you want.

[Makio64]

Yeah, solving the problem is easy but when I use a merge function on 2 objects who have reference values, I expect reference values to be merge too, not replaced by reference to a clone. I really don't understand the point of the current behavior of this function.

by default it shoudnt clone and if i want to clone then:

var a = { texture: { type: 't', value: myTexture } };
var b = { opacity: { type: 'f', value: 0 } };
var uniforms = THREE.UniformsUtils.merge( [ a, b ] );
uniforms.texture.value = uniforms.texture.value.clone();

[WestLangley]

Yeah, solving the problem is easy

Good. So you now have a workaround. : - )

I really don't understand the point of the current behavior of this function.

It is so uniforms will be unique, and the uniforms for one material (color, for example) will not be shared with another.

var uniforms1 = THREE.UniformsUtils.merge( [ a, b ] );
var uniforms2 = THREE.UniformsUtils.merge( [ a, b ] );

[Makio64]

if you want this behavior the user should do this :

var uniforms1 = THREE.UniformsUtils.merge( [ a, b ] );
var uniforms2 = THREE.UniformsUtils.clone( uniforms1 );
uniforms1.texture === uniforms2.texture // false

var uniforms1 = THREE.UniformsUtils.merge( [ a, b ] );
var uniforms2 = THREE.UniformsUtils.merge( [ a, b ] );
uniforms1.texture === uniforms2.texture // true

[mrdoob]

One can't instantiate JavaScipt Numbers. So Unifoms that use a Number as value will get "cloned" anyway. This behaviour of cloning then follows this limitation.

[pailhead]

possibly related?
https://github.com/mrdoob/three.js/issues/7367

It is so uniforms will be unique, and the uniforms for one material (color, for example) will not be shared with another.

But some, like cameraPosition , projectionMatrix (i assume lights, i dont use that one) are shared in a sense. These are though, hardcorded and directly uploaded via webgl calls from the camera, not from the material? Still, conceptually they are all of type uniform?

One can't instantiate JavaScipt Numbers. So Unifoms that use a Number as value will get "cloned" anyway. This behaviour of cloning then follows this limitation.

I've been trying to track down the source of this decision. Is it a good idea though? Calling clone on a Mesh copies a lot of properties by value, yet geometry is referenced. Texture does something similar, while copying most of parameters it does a shallow copy of the image.

I've even encountered an issue when cloning a mesh that has a material containing a yet to be loaded compressed texture. When imageutils encounters it, it does a shallow copy on the .image, but attempts to splice .mipmaps, which are undefined. If i could just inform the system to not bother creating copies that are going to be discarded anyway, it would solve a world of problems.

I really believe that this behavior is counter intuitive.

[sjlynch]

Changing
var tmp = this.clone( uniforms[ u ] );
to
var tmp = uniforms[ u ];

in the library fixes the issue for me. So I created a personal patch file with that code which replaces the merge function in threejs.

I'm still not sure why it isn't like that by default, since this works fine with my shaders. I'm not seeing any issues. @mrdoob cloning a number is fine because a number is a number. Cloning an object is different since you lose references that way. So I guess I don't understand your comment.

[zEh-]

I debugged the hell out of my ShaderMaterial's fragment shader.
Everytime I tried to use fog I ended up with black fragments due to missing image references.

Would be nice to have this solved. Until then I merge all required UniformsLibs and then append my custom ones, because UniformsUtils.merge can't handle them.

[zEh-]

May be of interest. This works: (Don't forget the potentially required polyfill and yeah, this isn't recursive but could replace UniformsUtils.merge altogether.)

const uniforms = THREE.UniformsUtils.merge([
    THREE.UniformsLib['common'],
    THREE.UniformsLib['fog']
]);
Object.assign(uniforms, {
    bumpFactor: {value: bumpFactor},
    // ...
});

[mrdoob]

@Makio64

Sounds like one can use Object.assign (which is already polyfilled in the library)

var a = { texture: { value: mytexture } }
var b = { opacity: { value: 0 } }
var uniforms = Object.assign( a, b )

Knowing this, we could start deprecating UniformsUtils.merge() in favour of Obect.assign(). I think this is better than changing the current behaviour of UniformsUtils.merge().

[Makio64]

@mrdoob Totally agree on this change, it will be more intuitive and clean 👍

[mrdoob]

UniformUtils is now gone! ✌️

[Makio64]

Just run into this stuff again and discover the change get reverse :'(

Everytime I go in advanced case and some old code use this function I get trap, maybe its just me, but I feel its an un-natural approach.

A solution can be a rename more explicit about what the function is actually doing
merge(uniforms) => cloneAndMerge(uniforms)