-
-
Notifications
You must be signed in to change notification settings - Fork 35.7k
WebGPURenderer: Array-Based RenderTarget Refactor #30959
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
WebGPURenderer: Array-Based RenderTarget Refactor #30959
Conversation
📦 Bundle sizeFull ESM build, minified and gzipped.
🌳 Bundle size after tree-shakingMinimal build including a renderer, camera, empty scene, and dependencies.
|
Are you okay if we move this PR to the |
@@ -147,7 +138,6 @@ class Textures extends DataMap { | |||
|
|||
const texture = textures[ i ]; | |||
|
|||
texture.isTextureArray = renderTarget.multiview === true && size.depth > 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fear this removal breaks multiview. When rendering in XR, the framebuffer render target is resized with a depth value larger 1
.
three.js/src/renderers/common/Renderer.js
Line 1246 in 6e56458
frameBufferTarget.setSize( width, height, outputRenderTarget !== null ? outputRenderTarget.depth : 1 ); |
But since the isTextureArray
/isArrayTexture
is only set in the ctor, the flag isn't set correctly. So there are two options: Restore this line or make sure Texture.isArrayTexture
is also set in RenderTarget.setSize()
for all render target textures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make sure Texture.isArrayTexture is also set in RenderTarget.setSize() for all render target textures.
I think this option makes a lot more sense I will try.
*/ | ||
constructor( width, height, type = UnsignedIntType, mapping, wrapS, wrapT, magFilter = NearestFilter, minFilter = NearestFilter, anisotropy, format = DepthFormat ) { | ||
constructor( width, height, type = UnsignedIntType, mapping, wrapS, wrapT, magFilter = NearestFilter, minFilter = NearestFilter, anisotropy, format = DepthFormat, depth = 1 ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's at times like these that I think that object-based function parameters are better for optional values, functions with more than 4 parameters start to get confusing for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could just remove the parameter and leave it as in the example below?
const depthMap = new THREE.DepthTexture( ... );
depthMap.depth = value;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we go this route we would need to apply the isArrayTexture
patch later in the code, for example in the Textures
class, but I don't even think it would work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know we are dealing with limitations but the function signature is inappropriate. Until we adopt a convention we can make this more explicit perhaps even with DepthArrayTexture
again. The class name and parameters also has the function of making the code easier to read.
I think the |
I think that |
I think we can revisit these specific details in a separate PR. For now, this PR focuses on cleaning up some confusing parts of the code. By simply adding an extra depth argument, it not only simplifies the logic but also unlocks support for very advanced features like Multi Render Target Arrays. All that while removing some code, which is a nice demonstration of how necessary this refactor is. @Mugen87 It would be great if you could confirm that XR is still working, as I currently don't have access to a device for testing. |
Do you mind temporarily updating the builds so I can test with the following URL? |
I pushed the build files @Mugen87 👍 |
The example does not work anymore with the new build. It's not even related to multiview. XR does not work in general. I have to debug so I can extract the error message. |
Okay, the error is:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you remove the build files from the PR, it is ready to be merged.
Related issue: #30155 #30920 #30830
Description
This PR refactors the concept of TextureArray and
RenderTargetArray
, adding capabilities for more advanced use cases such as Multi Render Target Arrays.Following the same idea behind deprecating
WebGLMultipleRenderTargets
in favor ofRenderTarget
using theoptions.count
property, this PR leverages the existingimage.depth
property in Texture to introduce support for array-like behavior inRenderTarget
.By adding a new
options.depth
parameter toRenderTarget
, it’s now possible to create a RenderTargetArray in a much simpler and more intuitive way:This opens the door for defining multi-layered render targets using both count and depth:
As a result, redundant classes such as
DepthArrayTexture
become unnecessary. Equivalent functionality is now achieved more cleanly:I made updates to parts of the code that were affected by your multiview PR, @rcabanier. Since I don’t currently have access to an XR device, I’d appreciate it if you could verify whether everything still works as expected with this change.
This contribution is funded by Renaud Rohlinger @ Utsubo