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.

Sign up for GitHub

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
+90 −4
Merged

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7b420b1
Add support for swapping depth textures
gkjohnson Jun 7, 2024
bf626e4
Don't swap textures if cube target
gkjohnson Jun 8, 2024
9daaabe
Don't recreate render buffers
gkjohnson Jun 8, 2024
ecc85ea
Only attach the buffers if they have been created already
gkjohnson Jun 8, 2024
e3aabfd
remove unused argument
gkjohnson Jun 8, 2024
929f112
Don't rebind the buffers if we can
gkjohnson Jun 11, 2024
3175f66
Handle cases where the depth texture is disposed and unbound
gkjohnson Jun 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions src/renderers/WebGLRenderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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.


// Swap the depth buffer to the currently attached one
const depthTexture = renderTarget.depthTexture;
if (
depthTexture &&
properties.has( depthTexture ) &&
( renderTarget.width !== depthTexture.image.width || renderTarget.height !== depthTexture.image.height )
) {

throw new Error( 'WebGLRenderTarget: Attached DepthTexture is initialized to the incorrect size.' );

}

textures.setupDepthRenderbuffer( renderTarget );

}

const texture = renderTarget.texture;
Expand Down
7 changes: 7 additions & 0 deletions src/renderers/webgl/WebGLProperties.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,12 @@ function WebGLProperties() {

let properties = new WeakMap();

function has( object ) {

return properties.has( object );

}

function get( object ) {

let map = properties.get( object );
Expand Down Expand Up @@ -36,6 +42,7 @@ function WebGLProperties() {
}

return {
has: has,
get: get,
remove: remove,
update: update,
Expand Down
36 changes: 31 additions & 5 deletions src/renderers/webgl/WebGLTextures.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1609,16 +1609,42 @@ function WebGLTextures( _gl, extensions, state, properties, capabilities, utils,
for ( let i = 0; i < 6; i ++ ) {

state.bindFramebuffer( _gl.FRAMEBUFFER, renderTargetProperties.__webglFramebuffer[ i ] );
renderTargetProperties.__webglDepthbuffer[ i ] = _gl.createRenderbuffer();
setupRenderBufferStorage( renderTargetProperties.__webglDepthbuffer[ i ], renderTarget, false );

if ( renderTargetProperties.__webglDepthbuffer[ i ] === undefined ) {

renderTargetProperties.__webglDepthbuffer[ i ] = _gl.createRenderbuffer();
setupRenderBufferStorage( renderTargetProperties.__webglDepthbuffer[ i ], renderTarget, false );

} else {

// attach buffer if it's been created already
const glAttachmentType = renderTarget.stencilBuffer ? _gl.DEPTH_STENCIL_ATTACHMENT : _gl.DEPTH_ATTACHMENT;
const renderbuffer = renderTargetProperties.__webglDepthbuffer[ i ];
_gl.bindRenderbuffer( _gl.RENDERBUFFER, renderbuffer );
_gl.framebufferRenderbuffer( _gl.FRAMEBUFFER, glAttachmentType, _gl.RENDERBUFFER, renderbuffer );

}

}

} else {

state.bindFramebuffer( _gl.FRAMEBUFFER, renderTargetProperties.__webglFramebuffer );
renderTargetProperties.__webglDepthbuffer = _gl.createRenderbuffer();
setupRenderBufferStorage( renderTargetProperties.__webglDepthbuffer, renderTarget, false );

if ( renderTargetProperties.__webglDepthbuffer === undefined ) {

renderTargetProperties.__webglDepthbuffer = _gl.createRenderbuffer();
setupRenderBufferStorage( renderTargetProperties.__webglDepthbuffer, renderTarget, false );

} else {

// attach buffer if it's been created already
const glAttachmentType = renderTarget.stencilBuffer ? _gl.DEPTH_STENCIL_ATTACHMENT : _gl.DEPTH_ATTACHMENT;
const renderbuffer = renderTargetProperties.__webglDepthbuffer;
_gl.bindRenderbuffer( _gl.RENDERBUFFER, renderbuffer );
_gl.framebufferRenderbuffer( _gl.FRAMEBUFFER, glAttachmentType, _gl.RENDERBUFFER, renderbuffer );

}

}

Expand Down Expand Up @@ -1763,7 +1789,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 •
edited
Loading

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.


renderTargetProperties.__webglDepthRenderbuffer = _gl.createRenderbuffer();
setupRenderBufferStorage( renderTargetProperties.__webglDepthRenderbuffer, renderTarget, true );
Expand Down