Skip to content

Conversation

sunag
Copy link
Collaborator

@sunag sunag commented Jun 30, 2025

Fixes #31294 (comment)

Description

Cache FramebufferTexture according to RenderTarget, preventing the same framebuffer texture from being used in different rendering contexts.

image

@sunag sunag added this to the r179 milestone Jun 30, 2025
Copy link

github-actions bot commented Jun 30, 2025

📦 Bundle size

Full ESM build, minified and gzipped.

Before After Diff
WebGL 337.69
78.76
337.69
78.76
+0 B
+0 B
WebGPU 557.35
154.34
557.75
154.43
+402 B
+99 B
WebGPU Nodes 556.27
154.12
556.67
154.22
+402 B
+99 B

🌳 Bundle size after tree-shaking

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

Before After Diff
WebGL 468.91
113.45
468.91
113.45
+0 B
+0 B
WebGPU 632.98
171.34
633.38
171.45
+401 B
+111 B
WebGPU Nodes 588.11
160.69
588.51
160.8
+401 B
+108 B

@sunag sunag marked this pull request as ready for review July 2, 2025 22:52
@sunag sunag merged commit 287727d into mrdoob:dev Jul 2, 2025
12 checks passed
@sunag sunag deleted the dev-framebuffer-node branch July 2, 2025 22:52

if ( this._textures.has( reference ) === false ) {

const framebufferTexture = defaultFramebuffer.clone();
Copy link
Collaborator

@Mugen87 Mugen87 Aug 6, 2025

Choose a reason for hiding this comment

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

The root cause for the performance regression (#31565) is this clone.

Viewport texture nodes might be cloned implicitly in TSL e.g. when using sample(). Because each instance of ViewportTextureNode has its own texture cache, we end up with cloned framebuffer textures for each cloned viewport texture node. That was not the case with the previous implementation since the framebuffer texture was unique and shared across all cloned viewport texture nodes.

Because the texture value is the reference for the update map in NodeFrame, we end up with many redundant updateBefore() calls which all call copyFramebufferToTexture(). These calls slow down the renderer significantly.

An easy fix is to share the texture cache in cloned instance. Simply adding this line in clone() fixes the regression:

viewportTextureNode._textures = this._textures;

However, this does not fix the use case when you manually create multiple instance of ViewportTextureNode and share the same framebuffer texture:

const tex1 = viewportTexture( uvNode, levelNode, framebufferTexture ); 
const tex2 = viewportTexture( uvNode, levelNode, framebufferTexture ); 
const tex3 = viewportTexture( uvNode, levelNode, framebufferTexture ); 

Copy link
Collaborator Author

@sunag sunag Aug 6, 2025

Choose a reason for hiding this comment

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

Hmm... Thank you for finding the root cause. I think I made a mistake when I put the _textures property as a class property, it should be outside.

I'll check it out more calmly and create a PR

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.

2 participants