Skip to content

Conversation

@querielo
Copy link
Contributor

Related issue: #32527

Description

This PR addresses an issue where WebGPURenderer (and the WebGL fallback) would generate a unique shader for every skinned mesh instance, preventing shader reuse.

Problem

Previously, ReferenceNode did not have a name, causing the Node Builders (GLSLNodeBuilder and WGSLNodeBuilder) to generate uniform buffer names based on the node's unique id (e.g., NodeBuffer_1179). This resulted in different shader source code for every skinned mesh, forcing the renderer to recompile shaders unnecessarily.

Solution

  • Updated ReferenceNode to generate a deterministic name based on its properties (e.g., ref_...).
  • Updated GLSLNodeBuilder and WGSLNodeBuilder to prioritize using node.name when generating uniform names, falling back to id only if no name is present.

This ensures that skinned meshes with the same configuration generate identical shader code, allowing the renderer to cache and reuse the programs effectively.

Known Limitations

There are still two scenarios where meshes sharing the same material will generate different shaders:

  1. Morph Targets: If two meshes have a different amount of morph targets.
  2. Bones: If two meshes have a different amount of bones.

Copilot AI review requested due to automatic review settings December 22, 2025 14:58
@github-actions
Copy link

github-actions bot commented Dec 22, 2025

📦 Bundle size

Full ESM build, minified and gzipped.

Before After Diff
WebGL 355.34
84.47
355.34
84.47
+0 B
+0 B
WebGPU 618.69
171.84
618.8
171.89
+115 B
+50 B
WebGPU Nodes 617.29
171.6
617.41
171.65
+115 B
+51 B

🌳 Bundle size after tree-shaking

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

Before After Diff
WebGL 487.5
119.33
487.5
119.33
+0 B
+0 B
WebGPU 690.95
187.49
691.06
187.54
+115 B
+44 B
WebGPU Nodes 640.79
174.72
640.9
174.77
+115 B
+44 B

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR resolves shader caching issues in WebGPURenderer and its WebGL fallback by ensuring skinned meshes with identical configurations generate identical shader code. Previously, each skinned mesh instance created a unique shader due to non-deterministic uniform buffer naming.

Key changes:

  • Modified ReferenceNode to generate deterministic names based on properties instead of unique IDs
  • Updated shader builders to prioritize node names over IDs when generating uniform buffer names

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
src/nodes/accessors/ReferenceNode.js Sets deterministic name for ReferenceNode based on properties
src/renderers/webgpu/nodes/WGSLNodeBuilder.js Updates uniform naming logic to use node.name when available
src/renderers/webgl-fallback/nodes/GLSLNodeBuilder.js Updates buffer naming to use node.name when available

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

* @default null
*/
this.name = null;
this.name = 'ref_' + this.properties.join( '_' );
Copy link

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

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

If this.properties is an array containing objects or other non-primitive values, join('_') will produce non-deterministic names (e.g., '[object Object]'). Consider sanitizing the properties or using a stable serialization method to ensure deterministic naming.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@querielo querielo Dec 22, 2025

Choose a reason for hiding this comment

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

		/**
		 * The property name might have dots so nested properties can be referred.
		 * The hierarchy of the names is stored inside this array.
		 *
		 * @type {Array<string>}
		 */
		this.properties = property.split( '.' );

this.properties has to be Array<string>

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.

1 participant