WebGLPrograms: Improve performance of getParameters(). #19673

Mugen87 · 2020-06-16T09:20:04Z

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 · 2020-06-16T09:47:24Z

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 · 2020-06-16T12:48:34Z

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

Mugen87 · 2020-06-16T13:05:06Z

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 · 2020-06-16T14:58:51Z

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 · 2020-06-16T17:40:49Z

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 · 2020-06-16T17:52:44Z

src/renderers/WebGLRenderer.js

@@ -1536,6 +1536,10 @@ function WebGLRenderer( parameters ) {

 		if ( programChange ) {

+			parameters.uniforms = programCache.getUniforms( material, parameters );
+
+			material.onBeforeCompile( parameters, _this );


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

Mugen87 · 2020-06-16T18:05:44Z

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 · 2020-06-16T21:21:23Z

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

Merged! 👍

mrdoob · 2020-06-16T21:31:17Z

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

Mugen87 · 2020-06-16T21:39:39Z

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 · 2020-06-17T09:34:19Z

@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 · 2020-06-20T02:13:17Z

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 · 2020-06-20T17:48:06Z

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 · 2020-06-22T15:23:31Z

Related NodeMaterial issue: #18347

sunag · 2020-06-24T06:37:19Z

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

Mugen87 · 2020-06-24T08:44:22Z

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 · 2020-06-24T08:49:11Z

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

Mugen87 · 2020-06-24T08:55:48Z

Sounds good! This should also fix #18347.

al3xpisani · 2020-06-25T16:38:08Z

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

Mugen87 · 2020-06-25T16:40:30Z

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

al3xpisani · 2020-06-25T16:47:21Z

@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 · 2020-06-25T17:00:02Z

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/[email protected]/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 · 2020-06-25T17:02:52Z

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/[email protected]/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 · 2020-07-13T17:19:28Z

Thanks!

Mugen87 · 2020-07-13T17:35:16Z

Yay! 🎉

Mugen87 mentioned this pull request Jun 16, 2020

WebGLLights state and its hashing produce unintended material reinitialization #14121

Closed

Mugen87 force-pushed the dev51 branch from 50c47bc to f50f42b Compare June 16, 2020 09:33

WebGLPrograms: Improve performance of getParameters().

924a6d3

Mugen87 force-pushed the dev51 branch from f50f42b to 924a6d3 Compare June 16, 2020 09:40

Mugen87 closed this Jun 16, 2020

WebGLPrograms: Refactor call of onBeforeCompile().

3c934fc

Mugen87 reopened this Jun 16, 2020

Mugen87 closed this Jun 16, 2020

Mugen87 reopened this Jun 16, 2020

Mugen87 commented Jun 16, 2020

View reviewed changes

mrdoob added this to the r118 milestone Jun 16, 2020

carstenschwede mentioned this pull request Jun 16, 2020

Unnecessary .initMaterial() calls if CubeCamera && [texture.encoding != output.Encoding || light-based-material] #19648

Closed

9 tasks

Merge remote-tracking branch 'upstream/dev' into dev51

477c924

Merge remote-tracking branch 'upstream/dev' into dev51

941beb6

mrdoob modified the milestones: r118, r119 Jun 21, 2020

Mugen87 mentioned this pull request Jul 12, 2020

NodeMaterial: getHash and customProgramCacheKey #19833

Merged

mrdoob merged commit 2f70cd4 into mrdoob:dev Jul 13, 2020

Mugen87 mentioned this pull request Jul 24, 2020

Node material compiling performance issue #18511

Closed

4 tasks

Mugen87 mentioned this pull request Sep 27, 2020

WebGLRenderer: Avoid creating per-camera render states. #20422

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WebGLPrograms: Improve performance of getParameters(). #19673

WebGLPrograms: Improve performance of getParameters(). #19673

Mugen87 commented Jun 16, 2020

Mugen87 commented Jun 16, 2020 •

edited

Loading

carstenschwede commented Jun 16, 2020

Mugen87 commented Jun 16, 2020

carstenschwede commented Jun 16, 2020 •

edited

Loading

Mugen87 commented Jun 16, 2020 •

edited

Loading

Mugen87 Jun 16, 2020 •

edited

Loading

Mugen87 commented Jun 16, 2020 •

edited

Loading

mrdoob commented Jun 16, 2020

mrdoob commented Jun 16, 2020

Mugen87 commented Jun 16, 2020 •

edited

Loading

Mugen87 commented Jun 17, 2020 •

edited

Loading

carstenschwede commented Jun 20, 2020

Mugen87 commented Jun 20, 2020

Mugen87 commented Jun 22, 2020

sunag commented Jun 24, 2020

Mugen87 commented Jun 24, 2020 •

edited

Loading

sunag commented Jun 24, 2020

Mugen87 commented Jun 24, 2020

al3xpisani commented Jun 25, 2020 •

edited

Loading

Mugen87 commented Jun 25, 2020

al3xpisani commented Jun 25, 2020 •

edited

Loading

Mugen87 commented Jun 25, 2020 •

edited

Loading

al3xpisani commented Jun 25, 2020

mrdoob commented Jul 13, 2020

Mugen87 commented Jul 13, 2020

WebGLPrograms: Improve performance of getParameters(). #19673

WebGLPrograms: Improve performance of getParameters(). #19673

Conversation

Mugen87 commented Jun 16, 2020

Mugen87 commented Jun 16, 2020 • edited Loading

carstenschwede commented Jun 16, 2020

Mugen87 commented Jun 16, 2020

carstenschwede commented Jun 16, 2020 • edited Loading

Mugen87 commented Jun 16, 2020 • edited Loading

Mugen87 Jun 16, 2020 • edited Loading

Choose a reason for hiding this comment

Mugen87 commented Jun 16, 2020 • edited Loading

mrdoob commented Jun 16, 2020

mrdoob commented Jun 16, 2020

Mugen87 commented Jun 16, 2020 • edited Loading

Mugen87 commented Jun 17, 2020 • edited Loading

carstenschwede commented Jun 20, 2020

Mugen87 commented Jun 20, 2020

Mugen87 commented Jun 22, 2020

sunag commented Jun 24, 2020

Mugen87 commented Jun 24, 2020 • edited Loading

sunag commented Jun 24, 2020

Mugen87 commented Jun 24, 2020

al3xpisani commented Jun 25, 2020 • edited Loading

Mugen87 commented Jun 25, 2020

al3xpisani commented Jun 25, 2020 • edited Loading

Mugen87 commented Jun 25, 2020 • edited Loading

al3xpisani commented Jun 25, 2020

mrdoob commented Jul 13, 2020

Mugen87 commented Jul 13, 2020

Mugen87 commented Jun 16, 2020 •

edited

Loading

carstenschwede commented Jun 16, 2020 •

edited

Loading

Mugen87 commented Jun 16, 2020 •

edited

Loading

Mugen87 Jun 16, 2020 •

edited

Loading

Mugen87 commented Jun 16, 2020 •

edited

Loading

Mugen87 commented Jun 16, 2020 •

edited

Loading

Mugen87 commented Jun 17, 2020 •

edited

Loading

Mugen87 commented Jun 24, 2020 •

edited

Loading

al3xpisani commented Jun 25, 2020 •

edited

Loading

al3xpisani commented Jun 25, 2020 •

edited

Loading

Mugen87 commented Jun 25, 2020 •

edited

Loading