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: Fix vec2 and vec3 for storageObject in StorageBufferNode #27697

Merged
merged 4 commits into from
Feb 8, 2024

Conversation

RenaudRohlinger
Copy link
Collaborator

@RenaudRohlinger RenaudRohlinger commented Feb 7, 2024

I've made some significant updates to how we handle data packing into textures for Pixel Buffer Object fallback of storage buffers.
Previously, we were tackling this with RGBA channels, but it turned out to be broken and doing like so would make things more complex than necessary, especially with overlapping values. For example when extracting vec2 and vec3 from texels, with vec3 data overlapping across texels making the code hard to maintain and scale.

To make things easier we're now packing data directly into their proper formats (Red, RG, RGB, RGBA), which makes the code much cleaner and more straightforward for all cases. Might even bring a small boost performance.

Also updated the example to improve E2E testing:
https://raw.githack.com/renaudrohlinger/three.js/improve-test/examples/webgpu_storage_buffer.htmlScreenshot 2024-02-08 at 0 15 34

This contribution is funded by Utsubo

@RenaudRohlinger RenaudRohlinger changed the title improve tests Feb 7, 2024
@LeviPesin
Copy link
Contributor

Hm... Would it be beneficial to re-add the example from #25273?

@RenaudRohlinger
Copy link
Collaborator Author

The way we were packing the data was wrong as it was actually a lot more difficult than I thought to handle overlapping values when packing everything into RGBA. For example vec2 needed to be something like this:

vec4 texel = texelFetch(textureName, uv, 0);
vec2 data;
if ((index / 2) % 2 == 0) {
    data = texel.rg;
} else {
    data = texel.ba;
}

and vec3 would be even worst when handling data overlapping between 2 texels (texel1.ba+texel2.r).

Also if we were keep going on that unpacking logic via complex texelFetch the code would become too difficult to maintain in the future I'm worried and the shaders way too complex for not much.

I fixed it by packing the datas directly to their proper formats (Red,RG,RGB,RGBA) so the code becomes that simple for all cases:

const { itemSize } = attribute;

const channel = '.' + vectorComponents.join( '' ).slice( 0, itemSize );
const uvSnippet = `ivec2(${indexSnippet} % ${ propertySizeName }, ${indexSnippet} / ${ propertySizeName })`;

const snippet = this.generateTextureLoad( null, textureName, uvSnippet, null, '0' );

//

this.addLineFlowCode( `${ propertyName } = ${ snippet + channel }` );

Also I believe this will drastically the code in the shader and even slightly improve performances.

Since the WebGLRenderer removed support for RGBFormat because of performance reasons🤔 I harcoded it pretty much in the code and didn't added to src/constants. Also Here gl.RGB is only used for packing optimization and not for drawing pixels so anyway its usage is accurate here.

/cc @sunag

@RenaudRohlinger RenaudRohlinger marked this pull request as ready for review February 7, 2024 15:12
@RenaudRohlinger RenaudRohlinger added this to the r162 milestone Feb 7, 2024
@RenaudRohlinger RenaudRohlinger changed the title WebGPURenderer: Improve E2E testing Feb 7, 2024
@sunag
Copy link
Collaborator

sunag commented Feb 7, 2024

What do you think about guide the user to create buffers with padding similar to what happens in std140 layout, and deal with float and vec4 in the buffer for example, perhaps it would simplify the process? .. Other formats could be available when creating the snippet only.

@RenaudRohlinger
Copy link
Collaborator Author

Hum, I think it would complexify things for developers as difficult as using interleaved array buffers (no one like these things 😄).
I don't see the downside of using the native formats made available by graphics cards but I do see the downsides of having to guide users for a specific way of packing up their datas in order to support only a specific WebGL fallback.

Also maybe I'm missing something here?

By the way is it antipattern to try to read a buffer through arraybuffernode.element(instanceIndex) in a vertex when it was previously used as storage buffer in a compute shader?
I wanted to implement a test here by using arrayBufferNodes[ 0 ].element( instanceIndex ) in a vertex shader but got this error:

Read-write storage buffer binding is used with a visibility (ShaderStage::Vertex) that contains ShaderStage::Vertex (note that read-only storage buffer bindings are allowed).
 - While validating entries[0]
 - While validating [BindGroupLayoutDescriptor]
 - While calling [Device].CreateBindGroupLayout([BindGroupLayoutDescriptor]).

Or should we add something like arrayBufferNodes[ 0 ].element( instanceIndex ).visibility('read') or .read() or just fix something in the WebGPU Backend?

Currently all examples are using toAttribute() but WebGL will probably not like it. I could try to investigate on that support (some rebinding post-PBO of the array buffer when calling toAttribute() I guess?)

@sunag
Copy link
Collaborator

sunag commented Feb 7, 2024

I imagined something simpler for this, similar to the way RangeNode deals with buffers for example. Maybe the way I explained it is to understand something much more difficult than it really is, using vec4 instead of vec3 to get more performance I think is a benefit.

@sunag
Copy link
Collaborator

sunag commented Feb 7, 2024

@RenaudRohlinger Is this happening to you too? It seems like a loss of data on the part of the WebGPU (happens after a few seconds )

image

@sunag
Copy link
Collaborator

sunag commented Feb 7, 2024

Currently all examples are using toAttribute() but WebGL will probably not like it. I could try to investigate on that support (some rebinding post-PBO of the array buffer when calling toAttribute() I guess?)

I think an intermediate Node in toAttribute could solve this by handling setup(). Maybe you already have a minimal example for share with us? I could be helping with this implementation....


} else if ( itemSize === 3 ) {

format = 6407; // patch since legacy doesn't use RGBFormat for rendering but here it's needed for packing optimization
Copy link
Contributor

Choose a reason for hiding this comment

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

@Mugen87 Maybe we can introduce the RGBFormat constant back?

@RenaudRohlinger
Copy link
Collaborator Author

RenaudRohlinger commented Feb 8, 2024

Thanks @sunag! I actually just set back the dualAttributeData.switchBuffers(); to always happen even in PBO setup which fixes all the issues toAttribute() included by re-binding the correct array to the program.

Maybe you already have a minimal example for share with us?

Try to replace any compute example (webgpu_compute_particles_rain for example) from storage() to storageObject() on dev and they will all break.
This PR fixe all these issues, now you can safely use PBO via storageObject on any example and they will work just fine.

Regarding the decision to replace vec3 with vec4, I don't quite understand the logic behind it in terms of optimization. Using vec4 will only result in a larger ArrayBuffer and consume more memory unnecessarily if the user needs to utilize a vec3.
I would not advise to drop vec3 support to storage_buffer especially for very rare case such as storage_buffer that tries to fetch other elements in a WebGL context.
The only justification I could see for not using RGB is that GPUs might fill the last texel, thereby treating it as RGBA, which isn't a significant issue considering the inefficiency of trying to pack and unpack vec3.

About the precision issue in the WebGPU side I had it when using non power-of-2 array buffers. Maybe the issue is GPU related and related to how I weirdly fetch indexes in colorNode (const index = uint( uv().x.mul( size ).floor() ).toVar();).

@RenaudRohlinger RenaudRohlinger merged commit 762a134 into mrdoob:dev Feb 8, 2024
11 checks passed
@Methuselah96 Methuselah96 mentioned this pull request Feb 17, 2024
32 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants