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

Compute shaders can read / write index and vertex buffers #6226

Merged
merged 3 commits into from
Apr 5, 2024

Conversation

mvaligursky
Copy link
Contributor

@mvaligursky mvaligursky commented Apr 4, 2024

adjusted APIs to allow vertex / index buffer to be marked as accessible by compute shaders:

  • IndexBuffer and VertexBuffer constructor now take options parameter, and can specify:
    options.storage = true;

  • Mesh constructor now takes options parameter, which can be:
    options.storageVertex = true;
    options.storageIndex = true;

  • procedural API - createMesh, createSphere and similar have additional valid options in the options class:
    options.storageVertex = true;
    options.storageIndex = true;

  • vertex or index buffer can be attached to a compute shader:

    compute.setParameter('vb', mesh.vertexBuffer);

Example modifying the vertex buffer:

potato.mov

@mvaligursky mvaligursky self-assigned this Apr 4, 2024
@mvaligursky mvaligursky added feature area: graphics Graphics related issue labels Apr 4, 2024
@mvaligursky mvaligursky requested a review from a team April 4, 2024 16:00
*/
constructor(graphicsDevice, format, numVertices, usage = BUFFER_STATIC, initialData) {
constructor(graphicsDevice, format, numVertices, usage = BUFFER_STATIC, initialData, options) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. This constructor signature is getting a bit messy. If you pass options, you need to set a value of initialData. which a lot of devs never pass. Feels like if this was written from scratch, usage and initialData would be in options. IIRC, initialData was added originally as a bit of a hack...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, but the devs can pass uninitialised for those to get the default values.
But maybe it's time we move last 3 or so parameters to options?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at this, I think a separate PR would be a good idea here too.

Comment on lines +235 to +238
* @param {boolean} [opts.storageVertex] - Defines if the vertex buffer of the mesh can be used as
* a storage buffer by a compute shader. Defaults to false. Only supported on WebGPU.
* @param {boolean} [opts.storageIndex] - Defines if the index buffer of the mesh can be used as
* a storage buffer by a compute shader. Defaults to false. Only supported on WebGPU.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why I feel uneasy about adding new WebGPU-only options to this API. I almost feel like shape creation should be very simple and pure. Like maybe it should return just bare position, normal, uv and index arrays. And you create the mesh yourself in whatever form you like (e.g. whether you want it to be used by a compute shader etc).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great idea! Happy with a separate PR for this?

@willeastcott
Copy link
Contributor

Approving despite my reservations - but sounds like things can be addressed in follow up PRs. 😄

@mvaligursky mvaligursky merged commit d66c121 into main Apr 5, 2024
7 checks passed
@mvaligursky mvaligursky deleted the mv-compute-vb-ib branch April 5, 2024 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: graphics Graphics related issue feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants