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.

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: Read Only and Read/Write Storage Textures #28455

Merged
merged 8 commits into from
May 29, 2024

Conversation

cmhhelgeson
Copy link
Contributor

Follow-up to #28435, with the similar purpose of allowing for the creation of storage textures with the additional read-only and read-write access modes in Chrome r124.

Changes include:

  • Renaming parameters and files to better match their StorageBuffer equivalents (for instance TextureStorageNode.js is now StorageTextureNode, and it's parameter .isStoreTextureNode is now .isStorageTextureNode
  • Adding GPUStorageTextureAccess to WebGPUConstants
  • Editing WGSLNodeBuilder and and WebGPUBindingUtils to allow for the creation of equivalent bindings/shader resources, including adding a utility function to WGSLNodeBuilder that translates between GPUStorageTextureAccess types and their equivalent WGSL shader access syntax.

Despite the basic functionality of the pull request being complete, I've left the pull request in draft for a couple of reasons.

  • Aforementioned unfamiliarity with the architectural conventions and preferences of the project maintainers.
  • There needs to be a comprehensive understanding of what texture formats are permitted to be accessed in which ways. For instance, webgpu_compute_texture_ping_pong compiler errors revealed that rgba16float storage textures seemingly can be accessed in 'read-only' and 'write-only' modes but not in 'read-write' modes. Accordingly, we may want to add some checks for certain combinations of textures and access modes depending on how much we want to protect the Three.js end user from creating invalid storage textures within their own code.
  • I've broken from the StorageBufferNode way of doing things by indicating in the example that the user should explicitly specify the access mode they would like to apply to their StorageTextureNode. This cuts down on the mental load of remembering every access variation of a function (storageTextureReadOnly(), storageTextureWriteOnly(), storageTextureReadWrite(), etc), while also cleaning up the number of required imports to utilize the full functionality of the storage textures. However, the project maintainers may prefer an approach similar to that of StorageBufferNode, and setting the access property in such a manner may introduce additional problems that are already sufficiently addressed by the StorageBufferNode approach.

…. Still need to gain a comprehensive understanding of what texture formats are permitted to be accessed in which ways (for instance, testing revealed that rgba16floats seemingly cannot be accessed with a GPUTextureAccess of 'read-write'. Instead, it only works with 'read-only' or 'write-only' according to the wgsl compiler errors
@RenaudRohlinger
Copy link
Collaborator

RenaudRohlinger commented May 23, 2024

Nice! You no longer need to use two textures and a ping-pong pattern, as you can now use the same texture with read-write access for both input and output. I've updated the example accordingly if you want to try it:
https://raw.githack.com/cmhhelgeson/three.js/webgpu_storage_texture_access/examples/webgpu_compute_texture_pingpong.html

Edit: Did revert the example, here for archive purpose:

const pingPong = storageTexture( pingTexture ).setAccess( 'read-write' );

const computeBlurWGSL = wgslFn( `
	fn computeBlurWGSL( readWriteTex: texture_storage_2d<${wgslFormat}, read_write>, index: u32 ) -> void {

		let posX = index % ${ width };
		let posY = index / ${ width };
		let indexUV = vec2i( i32( posX ), i32( posY ) );

		let color = blur( readWriteTex, indexUV ).rgb;

		textureStore( readWriteTex, indexUV, vec4f( color * 1.05, 1 ) );

	}
`, [ rand2 ] );

//

computeToPong = computeBlurWGSL( { readWriteTex: pingPong, index: instanceIndex } ).compute( width * height );

This will probably interest @Spiri0 😄

@RenaudRohlinger
Copy link
Collaborator

Accordingly, we may want to add some checks for certain combinations of textures and access modes depending on how much we want to protect the Three.js end user from creating invalid storage textures within their own code.

I think the WebGPU errors are explicit enough for users to understand what's happening, and trying to catch too many WebGPU errors might simply end up obfuscating the real issue.

The combination of access and formats is indeed quite tricky, and we might at least document somewhere what the currently supported ones are (gpuweb/gpuweb#3838 (comment)).

Regading the last point I'm not sure if @sunag prefers storageTextureReadOnly, storageTextureReadWrite or the current storageTexture().setAccess( 'read-write' ), I personally would use the latter, but I think providing both is totally viable. We could even add storageTextureReadOnly and such afterward as they are basically just shortcuts.

@Spiri0
Copy link
Contributor

Spiri0 commented May 23, 2024

Yes, that sounds very good. I will then update my ocean repository with the next three.js release 😊
This could also be usefull for depthTextures because if you render a depthTexture with a renderTarget and want to use it in a wgslFn with the texture node, there is also a read/write conflict.
But I wanted to take a closer look at the webgpu_backdrop_water example, because perhaps a renderTarget for a depthTexture is no longer necessary.
When I tried to read a depthTexture like in the webgpu_backdrop_water example into a wgslFn, I got the error message not a three texture. But as I said, I need to take a closer look at this. I'm very busy at the moment.

@cmhhelgeson cmhhelgeson marked this pull request as ready for review May 23, 2024 19:04
@sunag sunag added this to the r165 milestone May 23, 2024
@sunag
Copy link
Collaborator

sunag commented May 23, 2024

Seems that we have just the red channel here?

image

@sunag
Copy link
Collaborator

sunag commented May 23, 2024

I think would be better create a new example from this maybe webgpu_compute_texture_readwrite.

@sunag
Copy link
Collaborator

sunag commented May 23, 2024

Regading the last point I'm not sure if @sunag prefers storageTextureReadOnly, storageTextureReadWrite

It's interesting that we have these shortcuts because of the syntax style, is closer to a shader programming language.

@RenaudRohlinger
Copy link
Collaborator

Sure! I just reverted my changes. I will add an example for read-write once its support is a bit better than red channel only or maybe into something more interesting such as postprocess stuff. 😄

Also while playing with this I realized WebGPUTextures are missing a bunch of format its the backend such as UnsignedIntType, IntType and so on. I will PR something to fix this too. /cc @sunag

@RenaudRohlinger
Copy link
Collaborator

RenaudRohlinger commented May 29, 2024

Added a few feedback and did some cleanup. It should be good to merge @sunag 😊

By the way I think we could easily extend this access property feature to full TSL support (for example in webgpu_compute_texture) with a few lines of code.

I quickly tried by introducing something like new StorageTexture( width, height, 'read-write' ) but had a WGSLNodeBuilder issue with textureStore. I might try in another PR one day.

@cmhhelgeson
Copy link
Contributor Author

Added a few feedback and did some cleanup. It should be good to merge @sunag 😊

By the way I think we could easily extend this access property feature to full TSL support (for example in webgpu_compute_texture) with a few lines of code.

I quickly tried by introducing something like new StorageTexture( width, height, 'read-write' ) but had a WGSLNodeBuilder issue with textureStore. I might try in another PR one day.

Thanks for the changes, and apologies for not addressing the formatting issues sooner. I'm also addressing some issues with Read-Only Storage Buffers, so hopefully that can get merged in today as well.

@sunag
Copy link
Collaborator

sunag commented May 29, 2024

Merging... :)

@sunag sunag merged commit 24a29a5 into mrdoob:dev May 29, 2024
11 checks passed
@cmhhelgeson cmhhelgeson deleted the webgpu_storage_texture_access branch May 29, 2024 17:05
@Methuselah96 Methuselah96 mentioned this pull request May 29, 2024
37 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants