WebGLPrograms: Improve performance of getParameters().

[Mugen87]

I've realized today that cloning uniforms in getParameters() should not be necessary. This potential expensive task can be done only when shader program actually needs a compilation. Besides, uniforms are not necessary to compute the program cache key.

@carstenschwede Can you please check if this PR mitigates your performance issue?

[Mugen87]

Too bad, this approach does not work. There is an unfortunate dependency to Material.onBeforeCompile(). This callback is executed with the shader object where users have the possibility to modify existing uniforms or add new uniforms. Meaning at the point where Material.onBeforeCompile() is called, the final uniforms have to be ready. But since Material.onBeforeCompile() can alter the shader code, we can't call it after getParamters(), only before.

[carstenschwede]

Thanks for the effort! Unfortunately I still see light state changes in a scene rendering only a CubeCamera.

[Mugen87]

Thanks for the effort! Unfortunately I still see light state changes in a scene rendering only a CubeCamera.

Yes, but the overhead related to getUniforms() should be gone.

[carstenschwede]

Sorry, you are right of course:

Before:

With this PR:

I have to check with my larger scene, but I assumes this would fix my performance issues. But you say it would lead to incorrect results if I would change material properties in .onBeforeCompile()? E.g. it would break https://threejs.org/examples/?q=env#webgl_materials_envmaps_parallax?

Thanks again for your efforts, much appreciated!

[Mugen87]

Sorry, but I see no way to fix this without refactoring NodeMaterial. Even with the previous commit, node examples like webgl_sprites_nodes break. See #14442 and related issues/PRs for more information .

@mrdoob In the meanwhile, it seems to me NodeMaterial somewhat misused Material.onBeforeCompile()for its build system. Hence, it was necessary to refactor the usage of Material.onBeforeCompile() in the past so that node material works.

And now, even the name onBeforeCompile() is wrong since the callback is executed not before each shader compilation but in each invocation of getParameters().

This will definitely be a problem if the engine wants to have multiple programs per material, see #15047. I also think that the code of this PR is the right one since it saves a certain amount of overhead in getParameters() and calls Material.onBeforeCompile() at the intended spot. Examples like webgl_sprites_nodes should be fixed on NodeMaterial level.

[Mugen87]

Need to pass in parameters so user code actually modifies the correct vertexShader and fragmentShader used for shader compilation.

[Mugen87]

While we are at it: #17567 should be merged to solve a conceptual issue with onBeforeCompile().

If the user implements onBeforeCompile() with dynamic code (e.g. the output depends on if/else or switch statements), the following statement fails since toString() always returns the same result:

three.js/src/renderers/webgl/WebGLPrograms.js

Line 339 in 681ba71

array.push( parameters.onBeforeCompile.toString() );

This leads so identical program cache keys and thus prevents different shaders.

Merging #17567 might provide an API for NodeMaterial in order to distinct shader programs.

[mrdoob]

Merging #17567 might provide an API for NodeMaterial in order to distinct shader programs.

Merged! 👍

[mrdoob]

I've not studied this PR yet but, just so you know, I've updated webgl_materials_modified to reflect #17567:

[Mugen87]

I've updated webgl_materials_modified to reflect #17567:

Cool! 👍

Sidenote: This whole topic is quite complicated and I had to invest a lot of time today in order to recall all the details^^. It might be necessary to target r119 if NodeMaterial shouldn't break with this change.

[Mugen87]

@sunag It seems NodeMaterial needs the ability to quickly compute a hash that represents the current node configuration/setup. This hash value could be returned in the new callback Material.customProgramCacheKey() in order to exactly identify the shader program for a node material.

Meaning instead of having this code in NodeMaterial:

three.js/examples/jsm/nodes/materials/NodeMaterial.js

Lines 47 to 53 in 5dbfa90

	// it fix the programCache and share the code with others materials
	this.onBeforeCompile.toString = function () {
	return scope.needsCompile;
	};

it should be:

this.customProgramCacheKey = function () {

    return this.computeHash();
 
};

Otherwise it's necessary that NodeMaterial does not call its build() method in onBeforeCompile(). The renderer could add some code right before getParameters() in initMaterial() to check for NodeMaterial and then execute some logic. Something like:

if ( material.isNodeMaterial === true && material.requiresBuild() === true ) {

    material.build( { renderer: _this } );

}

However, requiresBuild() would also need the ability to tell the renderer if the node material requires a rebuild based on some internal version or key. Hence, I would prefer the usage of customProgramCacheKey().

[carstenschwede]

Is there currently an example that fails using this PR? I had some issues using GLTFLoader() but those seem to be fixed in the current state of this PR.

[Mugen87]

Is there currently an example that fails using this PR?

Yes, it's webgl_sprites_nodes. This example exposes a problem in NodeMaterial described above.

[Mugen87]

Related NodeMaterial issue: #18347

[sunag]

@Mugen87 Interesting... Material.customProgramCacheKey seems very good. This PR break only NodeMaterial example?

[Mugen87]

Yes. But it seems only webgl_sprites_nodes is affected. This example actually exposes the problem that the renderer can't identify the correct program for NodeMaterial.

If you can manage to compute a key or hash that represents the configuration of a NodeMaterial, the issue should be solved by just returning this value in customProgramCacheKey(). Ideally NodeMaterial can determine this key fast since customProgramCacheKey() will be called more often than NodeMaterial.build().

[sunag]

It seems a good opportunity to create a Node.getHash().

[Mugen87]

Sounds good! This should also fix #18347.

[al3xpisani]

hi @Mugen87 , what needs to be done to fix this issue pls ? Im facing this error on my project

TypeError: d.customProgramCacheKey is not a functionthree.min.js:103:324
getParameters https://threejs.org/build/three.min.js:103
p https://threejs.org/build/three.min.js:199
k https://threejs.org/build/three.min.js:204
renderBufferDirect https://threejs.org/build/three.min.js:219
onBeforeRender http://192.168.15.3:8080/js/Lensflare.js:226
q https://threejs.org/build/three.min.js:198
m https://threejs.org/build/three.min.js:198
render https://threejs.org/build/three.min.js:225
render http://192.168.15.3:8080/js/app.js:630
init http://192.168.15.3:8080/js/app.js:172
Kd https://threejs.org/build/three.min.js:222
a https://threejs.org/build/three.min.js:28

[Mugen87]

You need to share some more details about this issue. Do you mind demonstrating it with a live example? https://jsfiddle.net/hyok6tvj/

[al3xpisani]

@Mugen87 > You need to share some more details about this issue. Do you mind demonstrating it with a live example? https://jsfiddle.net/hyok6tvj/

sure. (https://codesandbox.io/s/elated-bas-jm9ze)

its on /js/Light/Lights.js

If I comment out line 93 ( scene.add(light2); ) it works .

[Mugen87]

Sorry, but I'm afraid your app setup is incorrect. You include three.js (via https://threejs.org/build/three.js) AND js/three.module.js which is invalid. You should always have just a single version of three.js in your build. Besides, it's best to focus on a ES6 modules build and avoid the usage of global scripts if possible.

Another thing which is unrelated to your issue: You use the latest three.js version but then include OrbitControls from r85(via https://unpkg.com/three@0.85.0/examples/js/controls/OrbitControls.js). This is also not valid. All library files have to be from the same release.

Please use the forum if you need help to clean up these inconsistencies.

[al3xpisani]

Sorry, but I'm afraid your app setup is incorrect. You include three.js (via https://threejs.org/build/three.js) AND js/three.module.js which is invalid. You should always have just a single version of three.js in your build. Besides, it's best to focus on a ES6 modules build and avoid the usage of global scripts if possible.

Another thing which is unrelated to your issue: You use the latest three.js version but then include OrbitControls from r85(via https://unpkg.com/three@0.85.0/examples/js/controls/OrbitControls.js). This is also not valid. All library files have to be from the same release.

Please use the forum if you need help to clean up these inconsistencies.

Sure. I see and thank you for helping me. Im just starting at this threejs and js world. will get updates :)

[mrdoob]

Thanks!

[Mugen87]

Yay! 🎉