Skip to content

NodeMaterialObeserver: Fix detection of replaced attribute/index.#32933

Merged
Mugen87 merged 1 commit intomrdoob:devfrom
Mugen87:dev1
Feb 2, 2026
Merged

NodeMaterialObeserver: Fix detection of replaced attribute/index.#32933
Mugen87 merged 1 commit intomrdoob:devfrom
Mugen87:dev1

Conversation

@Mugen87
Copy link
Copy Markdown
Collaborator

@Mugen87 Mugen87 commented Feb 2, 2026

Fixed #32926.

Description

Ensures NodeMaterialObeserver can detect replaced geometry attributes and indices.

@Mugen87 Mugen87 added this to the r183 milestone Feb 2, 2026
@github-actions
Copy link
Copy Markdown

github-actions bot commented Feb 2, 2026

📦 Bundle size

Full ESM build, minified and gzipped.

Before After Diff
WebGL 359.21
85.29
359.21
85.29
+0 B
+0 B
WebGPU 618.3
172.12
618.4
172.15
+98 B
+24 B
WebGPU Nodes 616.88
171.87
616.98
171.9
+98 B
+23 B

🌳 Bundle size after tree-shaking

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

Before After Diff
WebGL 490.85
119.64
490.85
119.64
+0 B
+0 B
WebGPU 692.12
187.28
692.22
187.3
+98 B
+22 B
WebGPU Nodes 641.5
174.69
641.6
174.71
+98 B
+22 B

@Mugen87 Mugen87 merged commit 70f24f6 into mrdoob:dev Feb 2, 2026
10 checks passed
@Mugen87
Copy link
Copy Markdown
Collaborator Author

Mugen87 commented Feb 2, 2026

@sunag Unrelated to this PR but since the other PR modified below code section:

if ( this.renderId !== renderId ) {
this.renderId = renderId;
return true;
}

This bit is actually a bandage and should be removed from NodeMaterialObserver. It is currently required though to make sure "frame" and "render" scoped bindings are at least updated once. This is a design issue that should be resolved.

For that, I believe we have to build the node materials after the render lists are ready but before _renderObjectDirect() is called. Then the "frame" and "render" bindings (e.g. the camera related uniforms) are updated just once. _renderObjectDirect() should then only update the object related bindings.

The node material build is currently triggered in needsRefresh() which is too late.

Besides, "frame" and "render" bindings are unnecessarily updated too often. UniformsGroup can detect that no update is required beause of the internal value cache but the checks itself are not necessary in the first place.

To sum up, the renderer should do this:

  • Prepare render lists.
  • Build all node materials in the lists by calling NodeManager.getForRender() for all render objects.
  • Update frame and render scoped bindings once if necessary.
  • Iterate through render lists, call _renderObjectDirect(), check for refresh and update object scoped bindings if necessary.

@arodic
Copy link
Copy Markdown
Contributor

arodic commented Feb 2, 2026

This seems like a pretty proper fix to me. Thanks! Any feedback on #32689/#32690?

@Mugen87
Copy link
Copy Markdown
Collaborator Author

Mugen87 commented Feb 2, 2026

I'm trying to have a look at this when the bind group cache is fixed. This is a performance issue that affects all WebGPURenderer applications right now (see #32913 and #32934).

Besides, I'm not sure what design plans @sunag has with the viewport nodes and CanvasTarget. Maybe he wants to solve #32689 differently so I would prefer to wait on his feedback.

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.

NodeMaterialObserver: setAttribute()/setIndex() crashes on second mesh when materials share same properties

2 participants