-
-
Notifications
You must be signed in to change notification settings - Fork 35.8k
WebGPURenderer: Add Storage3DTexture
and StorageArrayTexture
#31175
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
base: dev
Are you sure you want to change the base?
WebGPURenderer: Add Storage3DTexture
and StorageArrayTexture
#31175
Conversation
📦 Bundle sizeFull ESM build, minified and gzipped.
🌳 Bundle size after tree-shakingMinimal build including a renderer, camera, empty scene, and dependencies.
|
I've started by creating a StorageTexture node for array textures. I see in the code that some things like This code at the end of StorageTextureNode.js makes me think.
Here, however, the possibility of 3 dimensionality is already hinted at |
I can now use the texture with the textureStorage node in the compute shader with texture_storage_2d_array. However, the extension for correctly reading it into the fragment shader with the texture node is still missing. I'm working on that. Currently, it's still recognized as texture_2d. |
@Spiri0 What do you think about creating just a new parameter (depth) in |
I originally had it that way. Then I just imagined that you might have preferred to separate it. I had depth = 0 in the constructor so that it would be treated as a 2D texture by default. With depth greater than 0, it would be treated as a texture array. I can continue working on it this evening (CET). I'll then look for the reason why the fragment shader recognizes it as texture_2d. I already have an idea where to look. Here, I'm using texture arrays and copyTextureToTexture to stream large numbers of tiles (4 tiles per layer). Please excuse the poor video quality. I'm new to making videos. All textures are upscaled to 32k. Unfortunately, I can't find any models with very high-quality, detailed textures. I would like to have a city as a model. |
Now it's working. But I'm not satisfied with it yet. I admit I still don't understand why there are two indicators for texture arrays. isArrayTexture I used isTextureArray because I saw it in the StorageTexture when reading it from the console. However, isArrayTexture is from Texture. I would prefer to use isArrayTexture if that's okay. If so, I'll rework that |
@@ -208,7 +208,7 @@ class WebGPUBindingUtils { | |||
|
|||
texture.viewDimension = GPUTextureViewDimension.Cube; | |||
|
|||
} else if ( binding.texture.isArrayTexture || binding.texture.isDataArrayTexture || binding.texture.isCompressedArrayTexture ) { | |||
} else if ( binding.texture.isArrayTexture || binding.texture.isDataArrayTexture || binding.texture.isCompressedArrayTexture || binding.texture.isTextureArray ) { |
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.
Why do we have isArrayTexture
and isTextureArray
now? Can't you use the existing isArrayTexture
?
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 generally only use what is already available as a parameter, so I used isTextureArray.
screenshot from a StorageTexture node with r176 without my extension
If isArrayTexture were available, I would have used it. I can, of course, implement it, and that's why I'm asking. But then there would be two indicators for the array state in the StorageTextures, which I don't think is right, because then both would have to be set.
I assume there were reasons why isTextureArray was introduced, but I don't know them, and that's why I'm asking. One indicator is certainly better than two.
I could trace isTextureArray back in the code to where it plays a role. Perhaps it was simply created but not used anywhere yet, since storage textures aren't array-capable yet. Then we could remove isTextureArray from the code and replace it with isArrayTexture.
If you want a challenge... https://disneyanimation.com/resources/moana-island-scene/ 😁 |
Wow, I'm downloading this right now. I'm curious to see what it looks like. Thank you very much mrdoob |
@Mugen87 I think I've found the cause. On the left is the current release, r176, and on the right is the current dev. I updated before I started the PR 4 days ago. The best thing to do will be to wait for r177 and then I test it with all the changes made then, and with isArrayTexture to ensure it's ready for r178. ![]() |
@sunag Now it's ready for review. Thanks for changing isTextureArray to isArrayTexture. I assume you did that. I didn't want to change a global state parameter without asking. Now the PR looks much more elegant |
@sunag Can it be merged as it is now or do you have any changes you would like? |
*/ | ||
constructor( width = 1, height = 1 ) { | ||
constructor( width = 1, height = 1, depth = 0 ) { |
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 think the default should be 1? and array more than 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.
The reason I chose 0 was because someone might come up with the idea of wanting to define an array with a length of 1, for example, when providing an app with a dynamic image array length, because that's possible, and then complain that it doesn't work.
If you're working with a TextureArray on the Threejs side and the WGSLNodeBuilder suddenly wants to use a normal texture because of the length 1, it crashes because of the wrong shader texture format.
A value of 0 clearly defines a normal texture, and a depth > 0 clearly defines a TextureArray.
But if you'd like, I can set that to 1.
In this case, it can be argued that the user has to take this into account on the threejs side, but this significantly increases the effort.
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.
Following this spec, 3DTexture
does not support array, so the fact that it has count
does not seem to make sense. *ArrayTexture
and *3DTexture
are distinct and should exist individually.
I think that #30959 needs to be reviewed or the other textures need to be, as there seems to be an incompatibility.
Textures
Property | Description | Type |
---|---|---|
*Texture | width, height | 2D |
*ArrayTexture | height, height, depth | Array |
*3DTexture | height, height, depth | 3D |
RenderTarget
Property | Description | Type |
---|---|---|
*RenderTarget | width, height, { count } | 2D |
*RenderTargetArray | height, height, depth, { count } | Array |
*RenderTarget3D | height, height, depth | 3D |
RenderTarget (Currently)
Property | Description | Type |
---|---|---|
*RenderTarget | width, height, { count, depth } | 2D/Array |
*RenderTarget3D | height, height, depth | 3D |
Property | Type |
---|---|
width | 2D |
height | 2D |
depth | Array/3D |
count | RenderTargets count |
I have a feeling we should go back to RenderTargetArray
and its logic but now preserve the functionality of the count
property so we can have Multi Render Target Arrays.
So I would go back in the same way so that we have a StorageArrayTexture
and soon Storage3DTexture
too.
Changing texture properties I imagine could be a lot of work for us and the users, and this seems like it can be avoided.
@Mugen87 @RenaudRohlinger any thoughts on this?
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.
So, should we go back to a separate class like I originally had?
I would prefer that because it allows the user to clearly define the texture format by choosing the respective class:
StorageTexture,
StorageArrayTexture
Storage3DTexture
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.
As far as I can see, the places for the case distinction in the WGSLNodeBuilder and WebGPUBindingUtils are the same, which makes things easier.
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.
So, should we go back to a separate class like I originally had?
Yes, it seems like the best way to 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.
I'll take care of it in the new week. I'll also validate the Storage3DTexture in my app.
Then we have elegantly covered all three variants, because I think the separate classes make things very clear for the user.
…the viewDimension whethersampled or not
…the viewDimension whethersampled or not
…the viewDimension whethersampled or not
Okay, both are working as they should. The 3DTexture was a good idea, @sunag. It's important for fog and clouds, like in my first repo, which I made in threejs with WebGL: https://github.com/Spiri0/volumetric-clouds?tab=readme-ov-file I changed the order in "getUniforms(shaderStage)" in the WGSLNodeBuilder. The reason was that if normal 3DTextures or arrayTextures were checked first, the storage condition would not be met, and this required complicated extra conditions and restrictions. The changed order makes it simple. isSampledTexture3D replaced by is3DTexture, since it does not matter for the viewDimension whether sampled or not. This avoids unnecessary extra conditions |
…the viewDimension whethersampled or not
Storage3DTexture
and StorageArrayTexture
@@ -459,6 +459,8 @@ export const storage = TSL.storage; | |||
export const storageBarrier = TSL.storageBarrier; | |||
export const storageObject = TSL.storageObject; | |||
export const storageTexture = TSL.storageTexture; | |||
export const storage3DTexture = TSL.storage3DTexture; |
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.
These TSL functions were declared in some point?
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.
Ah, a valid point. For WebGPU, only the export from src/Three.WebGPU.js is necessary.
I can remove the export from src/Three.TSL.js or, what seems more appealing to me, add it for Three.TSL.js, because it should be available there too. But I have to look where.
#31167
This is initially a draft to be able to use texture_starage_2d_array in threejs.webgpu.
I see in the code that preparations for the extension have already been made in some places and therefore I would like to share my ideas here before I unnecessarily go in the wrong direction.