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: StorageBufferNode Support reading external elements in the WebGL Backend #27661

Merged
merged 37 commits into from
Feb 6, 2024

Conversation

RenaudRohlinger
Copy link
Collaborator

@RenaudRohlinger RenaudRohlinger commented Feb 1, 2024

Related issue: #27642

Description
Compute shaders in the WebGL backend can now read other elements just like the storage_buffer in WebGPU automatically.
Using storageObject will convert the dual-buffer transform feedback system into a transform feedback system with a single buffer as output and a Pixel Buffer Object (PBO) texture as input.

Here's a basic demo that alternates the order of the array buffer (element(100) becomes element (0) and so on):
https://raw.githack.com/renaudrohlinger/three.js/utsubo/feat/storage_pbo/examples/webgpu_storage_buffer.html
image

What's cool in this example is that the left side is using the WebGPU Backend and the right side is a WebGL Backend. 😁

This contribution is funded by Utsubo

const width = Math.floor( square );
const height = Math.ceil( square );

const pboTexture = new DataTexture( attribute.array, width, height, RGBAFormat, FloatType );
Copy link
Collaborator

Choose a reason for hiding this comment

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

The circular-reference problem is because of UniformNode in ArrayElementNode, UniformNode will use ShaderNode and ShaderNode will include try ArrayElementNode again.

I think it would be better to move this uniform creation block to GLSLNodeBuilder something like builder.getPBOUniform() for example, this could fix the circular reference problem too.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we have to consider a new Node, similar to what was done in BufferAttributeNode and buffer.toAttribute() as usage. It could be buffer.toTexture()

Copy link
Contributor

@LeviPesin LeviPesin Feb 1, 2024

Choose a reason for hiding this comment

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

I agree, this feels like something that should better be placed in GLSLNodeBuilder.

Copy link
Collaborator

@sunag sunag Feb 2, 2024

Choose a reason for hiding this comment

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

Also consider add/move it to StorageBufferNode.element() to avoid checks a little, and return and new Node to keep operations more organized, it can even be a StorageArrayElementNode.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the feedbacks!

The downside of buffer.toTexture() would be that this implies that somehow the developer would expect that something happens in the WebGPU Backend too.
This feature is more like a patch to fix the fact WebGL cannot access other element of array buffers rather than a new feature such as toAttribute().

I think I can try to move all this part to GLSLNodeBuilder but I'm not sure on how to create a Node here. Maybe once everything works we can think about creating a new Node specific to Pixel Buffer Object, but once again it's only a WebGL thing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll be happy to help you with this, let me know when the PR is ready for review so I can analyze it again.

About the example what do you think about renaming it to webgpu_storage_sorting?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good to me! Thanks a lot for this 😊

const propertyNameTexture = builder.getPropertyName( nodeUniform );

const snippet = /* glsl */`
texelFetch(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can also move to GLSLNodeBuilder similar to what happens in builder.generateTextureLoad().

@RenaudRohlinger
Copy link
Collaborator Author

RenaudRohlinger commented Feb 3, 2024

I finished the implementation. The main purpose of this PR is basically to improve the compute shading support by allowing reading data from external elements in the WebGL Backend.

The example is very dev oriented but I believe this will be useful for E2E tests in the future and prevent breaking things in regards to storage buffers. Also it should help some very specific use case such as simulations for advanced developers in the future.

It does support different formats even though that's the initial PR (float, vec4). The lack of vec2 and vec3 support is due to the fact I don't how to cast a texelFetch into these formats dynamically with the current node builder. Maybe you could advise me there @sunag.

Also my detection of the type of the target node is very weak in ArrayElementNode. The left node is often replaced by the texelFetch value (because of my output !== 'assign' condition in ArrayElementNode) which breaks. I'm not sure how to determine if the left value (a.mulAssign(b)) <-- is a the target of an AssignNode?

I added 2 TODO comments in the PR to point to the current limitations:
image

I believe this can be improved even more in the future but that's already a nice additional to compute shading fallback in my opinion.

It's now ready for review as I lack of knowledge on the node part. 👍

@RenaudRohlinger RenaudRohlinger marked this pull request as ready for review February 3, 2024 06:33
@RenaudRohlinger RenaudRohlinger added this to the r162 milestone Feb 3, 2024
return nodeSnippet;
snippet = nodeSnippet;

// TODO: How to properly detect if the node will be used as an assign target?
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should just check if output is 'void' (i.e. just change 'assign' to 'void' here and in AssignNode).

@LeviPesin
Copy link
Contributor

Now the main issue remaining is how to properly detect if the node will be used as an assign target I think

Is my suggestion above also not working? 🤔

} else {

transformBuffers[ i ].switchBuffers();
//this.textureUtils.copyBufferToTexture( transformBuffers[ i ].transformBuffer, transformBuffers[ i ].pbo );
Copy link
Collaborator

Choose a reason for hiding this comment

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

@RenaudRohlinger I have a performance issue, could you open the example webgpu_compute_particles and uncomment the PBO line here? Perhaps you have any suggestions as to what it could be?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

webgpu_compute_particles shouldn't need to use PBO so it's probably some sort of infinite loop. It's not related to the PBO line, if you comment transformBuffers[ i ].switchBuffers(); the buffers will not be swapped anymore as they should and all compute examples will have these performances issues @sunag

@sunag
Copy link
Collaborator

sunag commented Feb 5, 2024

Is my suggestion above also not working?

@LeviPesin It's not clear to me that this would resolve the issue. There seems to be more related things.

@RenaudRohlinger
Copy link
Collaborator Author

RenaudRohlinger commented Feb 5, 2024

@sunag I just pushed a patch to handle cases where we would use basic dual-buffer compute shaders and then later on, on the same storage buffer, read from other elements and so switch to PBO.

The remaining issue in webgpu_compute_particles and webgpu_compute_particles_rain seems to be shader/node related. Also webgpu_compute_particles_rain isn't working on the WebGPU backend too.

@RenaudRohlinger
Copy link
Collaborator Author

By the way, I just realized that the performance issue in webgpu_compute_particles and webgpu_compute_particles_rain is due to overdraw. This happens because all the particles are positioned at 0,0,0 and are not being properly updated, as the dual-buffer isn't being called. /cc @sunag

@sunag
Copy link
Collaborator

sunag commented Feb 5, 2024

webgpu_compute_particles shouldn't need to use PBO so it's probably some sort of infinite loop

Would it be correct to say that the objective of the PR is to use PBO only in webgpu_compute_audio and webgpu_storage_buffer?

@RenaudRohlinger
Copy link
Collaborator Author

RenaudRohlinger commented Feb 5, 2024

Yes, that is correct. That's why I initially thought it would be safer and easier to introduce a third parameter to the storage buffer.

For instance, we could have accessExternal with the following options: 'none' as the default, 'read' that would use PBO, and in the future, 'write' for full GPGPU fallback.

This approach would makes sense in my opinion because accessing external elements is already quite advanced. /cc @sunag

@sunag
Copy link
Collaborator

sunag commented Feb 5, 2024

Yes, that is correct. That's why I initially thought it would be safer and easier to introduce a third parameter to the storage buffer.

This makes sense, and will be necessary to implement something since we need to have this information to manipulate the PBO, I'm thinking about implementing a toTextureBuffer() / toBufferObject() or similar, this would certainly only have effects in the context of WebGLBackend for enable PBO, and would work as a "bypass" in WebGPUBackend.

@sunag
Copy link
Collaborator

sunag commented Feb 5, 2024

@RenaudRohlinger I added new TSL function storageObject , something similar to PBO, but SBO could say we have a Storage Buffer Object.

Now the examples is working, including webgpu_compute_audio for this I needs to call switchBuffer() to work...

@RenaudRohlinger
Copy link
Collaborator Author

Nice! I love this pattern.
switchBuffer() shouldn't be needed, I fixed the issue of the webgpu_compute_audio example by making sure attributeData gets always the pbo object attached in generatePBO 👍

@RenaudRohlinger RenaudRohlinger changed the title WebGPURenderer: Extend Compute Shading support in the WebGL Backend WebGPURenderer: StorageBufferNode Support reading external elements in the WebGL Backend Feb 6, 2024
@RenaudRohlinger
Copy link
Collaborator Author

Thanks for all the help @sunag! Let's give it a try 😊

I will look into adding writing access via full GPGPU setup and dual-dataTexture in another PR in the future.

@RenaudRohlinger RenaudRohlinger merged commit a2bf250 into mrdoob:dev Feb 6, 2024
11 checks passed
@LeviPesin
Copy link
Contributor

Names storageObject and bufferObject doesn't really seem to be easily understandable... Maybe storageBufferObject and accessMode?

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.

3 participants