WebGLRenderer: Add support for changing the assigned render target depth texture #28584

gkjohnson · 2024-06-08T00:43:44Z

Related issue: #19447, #15490, other depth peeling discussions

Description

This PR adjusts the setRenderTarget function to rebind the depth attachment so that it can be re assigned, unbound, and shared between multiple different render targets. If you set the render target and then set .depthTexture, though, it will not take effect until setRenderTarget is called again.

This allows a lot more flexibility in the reuse of render targets and depth textures while avoiding texture read / write feedback loops. For example it allows for implementing depth peeling without having to manually copy data between textures to avoid this kind of feedback.

There's a demo here and here demonstrating this PRs use for depth peeling:

Before	After

cc @donmccurdy I know you've expressed interest in this kind of OIT previously.

github-actions · 2024-06-08T00:45:39Z

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize `dev`	Filesize PR	Diff
682.2 kB (169 kB)	683.3 kB (169.2 kB)	+1.11 kB

🌳 Bundle size after tree-shaking

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

Filesize `dev`	Filesize PR	Diff
459.4 kB (110.9 kB)	460.6 kB (111.2 kB)	+1.11 kB

edankwan · 2024-06-11T10:08:58Z

This needs more love.

Mugen87 · 2024-06-11T12:04:00Z

src/renderers/WebGLRenderer.js

@@ -2268,6 +2268,22 @@ class WebGLRenderer {
 					// Color and depth texture must be rebound in order for the swapchain to update.
 					textures.rebindTextures( renderTarget, properties.get( renderTarget.texture ).__webglTexture, properties.get( renderTarget.depthTexture ).__webglTexture );

+				} else if ( renderTarget.depthBuffer || renderTarget.depthTexture ) {


I've tested the PR with webgl_rtt and it does not look right (or at least optimal) to me that this code path is taken in normal setRenderTarget() calls. The additional bindings executed in setupDepthRenderbuffer() are a waste for all scenarios with a static depth buffer.

Isn't it possible to make the depth texture exchange more explicit? E.g. use a flag that is set to true when depth texture reference is changed?

Good call - I just added a check in setupDepthRenderbuffer to avoid rebinding the depth buffer if it has already been bound. Let me know what you think.

cc @Mugen87 - just wanted to check on this again. The PR has now been changed so that the depth buffer is only updated and rebound if it has changed.

Mugen87 · 2024-06-20T11:05:03Z

src/renderers/webgl/WebGLTextures.js

@@ -1763,7 +1798,7 @@ function WebGLTextures( _gl, extensions, state, properties, capabilities, utils,

 				_gl.bindRenderbuffer( _gl.RENDERBUFFER, null );

-				if ( renderTarget.depthBuffer ) {
+				if ( renderTarget.depthBuffer && renderTargetProperties.__webglDepthRenderbuffer === undefined ) {


I don't understand this bit.

setupRenderTarget() is only called when __webglFramebuffer is undefined (so no framebuffer creation happened so far). When is __webglFramebuffer, undefined but not __webglDepthRenderbuffer?

Just removed this. I added it when checking whether the depth buffer needed to be initialized in other functions but you're right it's not needed here.

Mugen87 · 2024-06-20T11:07:03Z

src/renderers/webgl/WebGLTextures.js

@@ -1594,6 +1594,15 @@ function WebGLTextures( _gl, extensions, state, properties, capabilities, utils,
 		const renderTargetProperties = properties.get( renderTarget );
 		const isCube = ( renderTarget.isWebGLCubeRenderTarget === true );

+		// check if the depth texture is already bound to the frame buffer and that it's been initialized
+		if ( renderTargetProperties.__boundDepthTexture === renderTarget.depthTexture && properties.has( renderTarget.depthTexture ) ) {


Can we move this check in setRenderTarget() and do it before checking the dimensions of the depth texture? The dimensions check should be only relevant when exchanging depth textures and not in the default case.

I realized this is a bit more complicated when I was thinking through it again. Hopefully the current change makes sense. We need account for the case the where a depth texture is used and already bound on a render target, then disposed - meaning the depth texture is implicitly unbound so we need to make sure the three.js state reflects this. I tested and pushed the current change to the depth peeling demo page.

Mugen87 · 2024-06-20T11:29:26Z

I guess you are planning to implement a depth peeling demo with a different PR, right? (that would be awesome, btw^^)

We should have at least one example that uses this new code path.

gkjohnson · 2024-06-22T09:55:28Z

I guess you are planning to implement a depth peeling demo with a different PR, right? (that would be awesome, btw^^)

I can do this when I have a chance - I'll base it on the demo above. I'll have to leave it to someone else to make a nicer, reusable component, though.

gkjohnson · 2024-08-05T00:19:29Z

@Mugen87 is this okay to merge?

pailhead · 2024-08-18T19:55:20Z

Hi, I’m on a phone atm so can’t look at the code, but since

#15490

is mentioned, is there any chance to inquire what was the issue with that one? For what it’s worth a depth peel example was proposed as well:

#15312

it was never rejected, but after a few years with no feedback I closed it.

Does this solve the stencil sharing as well?

pailhead · 2024-08-19T04:39:19Z

For all intents and purposes this (DepthTexture) works exactly the same as the render buffer, before, it needed an extension, now it doesn’t? #15490 is basically solved now?

gkjohnson · 2024-08-19T08:48:20Z

it was never rejected, but after a few years with no feedback I closed it.

I think I'd given quite a bit of feedback on that PR 😅 The solution in this PR is the one I had advocated for in #15490 since it aligns with three.js' existing API designs - though without an additional DepthBuffer class since an extension is no longer needed for readable depth textures.

Does this solve the stencil sharing as well?

Yes. Stencils use the same depth buffer attachment.

#15490 is basically solved now

It should be, yes. The demo in the depth peeling test example I made used a three.js build from this PR to ensure everything wsa working.

Add support for swapping depth textures

7b420b1

gkjohnson requested a review from Mugen87 June 8, 2024 00:43

gkjohnson changed the title ~~WebGLRenderer: Add support for changing the assigned depth texture~~ WebGLRenderer: Add support for changing the assigned render target depth texture Jun 8, 2024

gkjohnson added 4 commits June 8, 2024 09:55

Don't swap textures if cube target

bf626e4

Don't recreate render buffers

9daaabe

Only attach the buffers if they have been created already

ecc85ea

remove unused argument

e3aabfd

gkjohnson added this to the r166 milestone Jun 9, 2024

Mugen87 reviewed Jun 11, 2024

View reviewed changes

Don't rebind the buffers if we can

929f112

Mugen87 reviewed Jun 20, 2024

View reviewed changes

Handle cases where the depth texture is disposed and unbound

3175f66

mrdoob modified the milestones: r166, r167 Jun 28, 2024

mrdoob modified the milestones: r167, r168 Jul 25, 2024

Mugen87 approved these changes Aug 5, 2024

View reviewed changes

gkjohnson merged commit 609ba9c into mrdoob:dev Aug 5, 2024
12 checks passed

gkjohnson deleted the reassignable-depth branch August 5, 2024 10:04

gkjohnson mentioned this pull request Aug 19, 2024

WebGLRenderTarget: DepthTexture cannot be changed after initial use #19447

Closed

Methuselah96 mentioned this pull request Aug 23, 2024

WebGLProperties.has three-types/three-ts-types#1171

Merged

Methuselah96 mentioned this pull request Aug 23, 2024

r168 three-types/three-ts-types#1147

Closed

64 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WebGLRenderer: Add support for changing the assigned render target depth texture #28584

WebGLRenderer: Add support for changing the assigned render target depth texture #28584

gkjohnson commented Jun 8, 2024

github-actions bot commented Jun 8, 2024 •

edited

Loading

edankwan commented Jun 11, 2024

Mugen87 Jun 11, 2024 •

edited

Loading

gkjohnson Jun 11, 2024

gkjohnson Jun 19, 2024

Mugen87 Jun 20, 2024

gkjohnson Jun 22, 2024 •

edited

Loading

Mugen87 Jun 20, 2024 •

edited

Loading

gkjohnson Jun 22, 2024 •

edited

Loading

Mugen87 commented Jun 20, 2024 •

edited

Loading

gkjohnson commented Jun 22, 2024

gkjohnson commented Aug 5, 2024

pailhead commented Aug 18, 2024 •

edited

Loading

pailhead commented Aug 19, 2024

gkjohnson commented Aug 19, 2024 •

edited

Loading

WebGLRenderer: Add support for changing the assigned render target depth texture #28584

WebGLRenderer: Add support for changing the assigned render target depth texture #28584

Conversation

gkjohnson commented Jun 8, 2024

github-actions bot commented Jun 8, 2024 • edited Loading

📦 Bundle size

🌳 Bundle size after tree-shaking

edankwan commented Jun 11, 2024

Mugen87 Jun 11, 2024 • edited Loading

Choose a reason for hiding this comment

gkjohnson Jun 11, 2024

Choose a reason for hiding this comment

gkjohnson Jun 19, 2024

Choose a reason for hiding this comment

Mugen87 Jun 20, 2024

Choose a reason for hiding this comment

gkjohnson Jun 22, 2024 • edited Loading

Choose a reason for hiding this comment

Mugen87 Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

gkjohnson Jun 22, 2024 • edited Loading

Choose a reason for hiding this comment

Mugen87 commented Jun 20, 2024 • edited Loading

gkjohnson commented Jun 22, 2024

gkjohnson commented Aug 5, 2024

pailhead commented Aug 18, 2024 • edited Loading

pailhead commented Aug 19, 2024

gkjohnson commented Aug 19, 2024 • edited Loading

github-actions bot commented Jun 8, 2024 •

edited

Loading

Mugen87 Jun 11, 2024 •

edited

Loading

gkjohnson Jun 22, 2024 •

edited

Loading

Mugen87 Jun 20, 2024 •

edited

Loading

gkjohnson Jun 22, 2024 •

edited

Loading

Mugen87 commented Jun 20, 2024 •

edited

Loading

pailhead commented Aug 18, 2024 •

edited

Loading

gkjohnson commented Aug 19, 2024 •

edited

Loading