Skip to content

XRManager: More fixes. #31198

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

Merged
Mugen87 merged 1 commit into from
May 29, 2025
+12 −6
Merged

XRManager: More fixes. #31198

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions src/renderers/common/Renderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1260,6 +1260,7 @@ class Renderer {
frameBufferTarget.scissorTest = this._scissorTest;
frameBufferTarget.multiview = outputRenderTarget !== null ? outputRenderTarget.multiview : false;
frameBufferTarget.resolveDepthBuffer = outputRenderTarget !== null ? outputRenderTarget.resolveDepthBuffer : true;
frameBufferTarget.autoAllocateDepthBuffer = outputRenderTarget !== null ? outputRenderTarget.autoAllocateDepthBuffer : false;
Copy link
Collaborator

@Mugen87 Mugen87 May 29, 2025

Choose a reason for hiding this comment

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

I think we should rename setOutputRenderTarget() to _setXROutputRenderTarget(). autoAllocateDepthBuffer only exists for XRRenderTarget and setOutputRenderTarget() is only relevant for XR. We should not allow to use this method as a part of the public API.

Copy link
Owner

@mrdoob mrdoob May 30, 2025

Choose a reason for hiding this comment

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

Agreed 👍

Copy link
Collaborator

@sunag sunag May 30, 2025

Choose a reason for hiding this comment

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

setOutputRenderTarget() is only relevant for XR..

I thought we could use it for something like this too. #27628

Copy link
Collaborator

@Mugen87 Mugen87 May 30, 2025

Choose a reason for hiding this comment

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

In this case let's wait before make more decisions.

The bit that might need refactoring is how autoAllocateDepthBuffer is implemented here. It's not a property of RenderTarget but frameBufferTarget gets it assigned.

Ideally, we can mark the XR code path more clearly so it's easier to maintain it in the future. Having XRRenderTarget and dedicated XR methods (like _setXRLayerSize()) and properties helped to detect XR exclusive logic.

Copy link
Contributor Author

@cabanier cabanier May 30, 2025

Choose a reason for hiding this comment

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

I think we should rename setOutputRenderTarget() to _setXROutputRenderTarget().

do you want me to make that change? I agree that the xr code is a bit mixed. Some of it could also apply to regular rendering (ie auto-allocating depth and discarding it before tone mapping)


return frameBufferTarget;

Expand Down
2 changes: 1 addition & 1 deletion src/renderers/common/XRManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1021,7 +1021,7 @@ class XRManager extends EventDispatcher {
session.updateRenderState( { baseLayer: glBaseLayer } );

renderer.setPixelRatio( 1 );
renderer.setSize( glBaseLayer.framebufferWidth, glBaseLayer.framebufferHeight, false );
renderer._setXRLayerSize( glBaseLayer.framebufferWidth, glBaseLayer.framebufferHeight );

this._xrRenderTarget = new XRRenderTarget(
glBaseLayer.framebufferWidth,
Expand Down
15 changes: 10 additions & 5 deletions src/renderers/webgl-fallback/WebGLBackend.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -595,7 +595,7 @@ class WebGLBackend extends Backend {

}

} else if ( resolveDepthBuffer === false ) {
} else if ( resolveDepthBuffer === false && renderTargetContextData.framebuffers ) {

const fb = renderTargetContextData.framebuffers[ renderContext.getCacheKey() ];
state.bindFramebuffer( gl.DRAW_FRAMEBUFFER, fb );
Expand Down Expand Up @@ -2075,7 +2075,7 @@ class WebGLBackend extends Backend {

} else {

if ( hasExternalTextures && useMultisampledRTT ) {
if ( useMultisampledRTT ) {

multisampledRTTExt.framebufferTexture2DMultisampleEXT( gl.FRAMEBUFFER, attachment, gl.TEXTURE_2D, textureData.textureGPU, 0, samples );

Expand All @@ -2093,21 +2093,26 @@ class WebGLBackend extends Backend {

}

if ( renderTarget.isXRRenderTarget && renderTarget.autoAllocateDepthBuffer === true ) {
const depthStyle = stencilBuffer ? gl.DEPTH_STENCIL_ATTACHMENT : gl.DEPTH_ATTACHMENT;

if ( renderTarget.autoAllocateDepthBuffer === true ) {

const renderbuffer = gl.createRenderbuffer();
this.textureUtils.setupRenderBufferStorage( renderbuffer, descriptor, 0, useMultisampledRTT );
renderTargetContextData.xrDepthRenderbuffer = renderbuffer;
depthInvalidationArray.push( stencilBuffer ? gl.DEPTH_STENCIL_ATTACHMENT : gl.DEPTH_ATTACHMENT );

gl.bindRenderbuffer( gl.RENDERBUFFER, renderbuffer );
gl.framebufferRenderbuffer( gl.FRAMEBUFFER, depthStyle, gl.RENDERBUFFER, renderbuffer );


} else {

if ( descriptor.depthTexture !== null ) {

depthInvalidationArray.push( stencilBuffer ? gl.DEPTH_STENCIL_ATTACHMENT : gl.DEPTH_ATTACHMENT );

const textureData = this.get( descriptor.depthTexture );
const depthStyle = stencilBuffer ? gl.DEPTH_STENCIL_ATTACHMENT : gl.DEPTH_ATTACHMENT;
textureData.renderTarget = descriptor.renderTarget;
textureData.cacheKey = cacheKey; // required for copyTextureToTexture()

Expand Down Expand Up @@ -2166,7 +2171,7 @@ class WebGLBackend extends Backend {

// rebind external XR textures

if ( ( isXRRenderTarget && hasExternalTextures ) || renderTarget.multiview ) {
if ( isXRRenderTarget || useMultisampledRTT || renderTarget.multiview ) {

state.bindFramebuffer( gl.FRAMEBUFFER, fb );

Expand Down