Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WebGLRender & NodeMaterials program caching errors #26225

Closed
wants to merge 11 commits into from

Conversation

aardgoose
Copy link
Contributor

** Program caching issue with Node materials and the webgl renderer **

With certain NodeMaterials eg MeshBasicNodeMaterial and MeshPhongNodeMaterial, program caching can fail with various breakages: example test case added in the PR: examples\webgl_nodes_test.html based on webgl_nodes_materials_physical_clearcoat.html.

The root cause is that program parameters from programCache.getParameters() are relied on to generate a unique key value, however the parameters and key are derived in turn from the material's shaders.

In the NodeMaterials which derive from ShaderMaterial, all node materials have the default ShaderMaterial shaders at this point, and the real shaders are only built later via onBuild(). Depending on the material's other parameters it is possible to get duplicate cache keys for different NodeMaterials, and hence the wrong programs being used for some materials.

I have have removed the inherited shaders in NodeMaterial and moved the material.onBuild() call into getParameters() to ensure accurate customShaderID values are generated.

I am not sure if this is the best solution, but it fixes the problem.

@sunag

@github-actions
Copy link

github-actions bot commented Jun 9, 2023

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
640 kB (158.8 kB) 640.4 kB (158.9 kB) +317 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Filesize dev Filesize PR Diff
433.4 kB (105 kB) 433.7 kB (105.1 kB) +317 B

Comment on lines +31 to +32
import { color, float, vec2, texture, normalMap, uv,
MeshPhongNodeMaterial, MeshBasicNodeMaterial } from 'three/nodes';

Check notice

Code scanning / CodeQL

Unused variable, import, function or class

Unused imports color, float, normalMap, texture, uv, vec2.
@@ -44,6 +44,9 @@ class NodeMaterial extends ShaderMaterial {

this.positionNode = null;

this.vertexShader = null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these properties are already created by ShaderMaterial?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They have the text for a minimal shader applied.

this.vertexShader = default_vertex;
this.fragmentShader = default_fragment;

setting these to null ensures the new 'shader missing' code in getParameters() is triggered and hence material.onBuild()

@sunag
Copy link
Collaborator

sunag commented Jun 9, 2023

I think that another fix could be add a this.type in customProgramCacheKey().
it's common WebGLRenderer users use differents materials just with differents maps and color. Preserve the cacheKey can help for it.

image

return getCacheKey( this );

customProgramCacheKey() {

	return this.type + getCacheKey( this );

}

@aardgoose
Copy link
Contributor Author

I considered that, downsides it relies on authors coding it consistently and may miss changes of shader text and possible sharing of programs between different materials. Admittedly those may be corner cases, and there may be other consumers of the onBuild() mechanism.

@sunag
Copy link
Collaborator

sunag commented Jun 10, 2023

Closing in favor of #26227
Thanks!

@sunag sunag closed this Jun 10, 2023
@aardgoose aardgoose deleted the node-cache-issue branch July 5, 2023 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants