XRManager: More fixes. #31198
XRManager: More fixes. #31198
Conversation
📦 Bundle sizeFull ESM build, minified and gzipped.
🌳 Bundle size after tree-shakingMinimal build including a renderer, camera, empty scene, and dependencies.
|
| @@ -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; | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
setOutputRenderTarget() is only relevant for XR..
I thought we could use it for something like this too. #27628
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
Fixed #31193.
This fixes a couple of issues in the XR rendering: