Fixes framebuffer caching with multiple references of different sizes #32690
Conversation
arodic
force-pushed
the
fixes/sharedDepthTexture
branch
2 times, most recently
from
65169bc to
17ae2b6
Compare
📦 Bundle sizeFull ESM build, minified and gzipped.
🌳 Bundle size after tree-shakingMinimal build including a renderer, camera, empty scene, and dependencies.
|
arodic
force-pushed
the
fixes/sharedDepthTexture
branch
from
17ae2b6 to
0f44cd5
Compare
arodic
changed the title
arodic
force-pushed
the
fixes/sharedDepthTexture
branch
2 times, most recently
from
538f416 to
a5ceff2
Compare
|
|
||
| if ( cacheTextures === undefined ) { | ||
|
|
||
| cacheTextures = new WeakMap(); |
There was a problem hiding this comment.
Looking at this WeakMap() I just realized that ViewportTextureNode doesn't really have a way to free up GPU resources. WeakMap afaik does not free up texture memory. This is not a new issue, previous version also had a WeakMap and lacked disposal mechanism just something to keep an eye on.
arodic
force-pushed
the
fixes/sharedDepthTexture
branch
2 times, most recently
from
23d839d to
c7dd472
Compare
arodic
force-pushed
the
fixes/sharedDepthTexture
branch
2 times, most recently
from
363d659 to
c2b2888
Compare
arodic
force-pushed
the
fixes/sharedDepthTexture
branch
from
c2b2888 to
2806820
Compare
|
I cleaned up all the slop. This should be minimal and pretty clean implementation fixing the bug. Ready for review! |
arodic
changed the title
| const canvasTarget = renderer.getCanvasTarget(); | ||
|
|
||
| this.value = this.getTextureForReference( renderTarget ); | ||
| const reference = canvasTarget ? canvasTarget : renderTarget; |
There was a problem hiding this comment.
This line makes me think that there is underlying issue with RenderTarget/CanvasTarget API design.
@Mugen87 Can renderer have both canvasTarget and renderTarget set? If so, which case should have priority here?
There was a problem hiding this comment.
renderer.getCanvasTarget() will never be null; the default canvas itself is defined in a CanvasTarget. In that sense, I think we should prioritize RenderTarget renderTarget ? renderTarget : canvasTarget, so when we have a RenderTarget, it is used as output.
2 participants
#Change summary
The Root Cause of #32689
ViewportDepthTextureNode: Uses a singleton _sharedDepthbuffer that is shared across all RenderTargets/CanvasTargets. This causes brute-force destruction and re-creation of depth buffer per reference on every rendered frame if references are not exactly the same size. This affects both CanvasTarget and RenderTargetViewporTextureNodeDoes not consider CanvasTarget as a reference. It simply does not do caching for CanvasTargetsRenderer: Uses a single _frameBufferTarget that resizes for each canvas (if not exactly the same size), triggeringdepthTexture.needsUpdate = true and causing texture recreation
The Fix for #32689
ViewportDepthTextureNoderemoved getTextureForReference() override to allow texture caching. Now it caches depth textures per referenceViewportTextureNodeAdded check for CanvasTarget. Previous implementation considered RenderTargets only.RendererChanged _frameBufferTarget to _frameBufferTargets (Map<CanvasTarget, RenderTarget>). Each canvas target now gets its own framebuffer target instead of sharing oneThis allows multi-canvas rendering with different sizes and tone mapping enabled without constant texture buffer recreation.