-
-
Notifications
You must be signed in to change notification settings - Fork 36k
TSL: viewportTexture()
cache FramebufferTexture
according to RenderTarget
#31343
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
Conversation
📦 Bundle sizeFull ESM build, minified and gzipped.
🌳 Bundle size after tree-shakingMinimal build including a renderer, camera, empty scene, and dependencies.
|
|
||
if ( this._textures.has( reference ) === false ) { | ||
|
||
const framebufferTexture = defaultFramebuffer.clone(); |
There was a problem hiding this comment.
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 );
There was a problem hiding this comment.
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
Fixes #31294 (comment)
Description
Cache
FramebufferTexture
according toRenderTarget
, preventing the same framebuffer texture from being used in different rendering contexts.