Skip to content
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

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

Merged
merged 7 commits into from
Aug 5, 2024

Conversation

gkjohnson
Copy link
Collaborator

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
image image
image image
image image

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

@gkjohnson gkjohnson requested a review from Mugen87 June 8, 2024 00:43
@gkjohnson 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
Copy link

github-actions bot commented Jun 8, 2024

📦 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

@gkjohnson gkjohnson added this to the r166 milestone Jun 9, 2024
@edankwan
Copy link

This needs more love.

@@ -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 ) {
Copy link
Collaborator

@Mugen87 Mugen87 Jun 11, 2024

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

@@ -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 ) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

@gkjohnson gkjohnson Jun 22, 2024

Choose a reason for hiding this comment

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

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.

@@ -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 ) ) {
Copy link
Collaborator

@Mugen87 Mugen87 Jun 20, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@gkjohnson gkjohnson Jun 22, 2024

Choose a reason for hiding this comment

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

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
Copy link
Collaborator

Mugen87 commented Jun 20, 2024

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
Copy link
Collaborator Author

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.

@mrdoob mrdoob modified the milestones: r166, r167 Jun 28, 2024
@mrdoob mrdoob modified the milestones: r167, r168 Jul 25, 2024
@gkjohnson
Copy link
Collaborator Author

@Mugen87 is this okay to merge?

@gkjohnson gkjohnson merged commit 609ba9c into mrdoob:dev Aug 5, 2024
12 checks passed
@gkjohnson gkjohnson deleted the reassignable-depth branch August 5, 2024 10:04
@pailhead
Copy link
Contributor

pailhead commented Aug 18, 2024

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
Copy link
Contributor

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
Copy link
Collaborator Author

gkjohnson commented Aug 19, 2024

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.

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.

5 participants