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

Create a BufferBinding from a BufferSlice #6309

Open
DCNick3 opened this issue Sep 22, 2024 · 7 comments
Open

Create a BufferBinding from a BufferSlice #6309

DCNick3 opened this issue Sep 22, 2024 · 7 comments
Labels
area: api Issues related to API surface type: enhancement New feature or request

Comments

@DCNick3
Copy link

DCNick3 commented Sep 22, 2024

TL;DR: I pass a BufferSlice in a bunch of places, but I can't use it to bind a buffer, since BufferBinding needs a Buffer reference.

Is your feature request related to a problem? Please describe.
While implementing a more opinionated graphics API wrapper around wgpu for my game engine, I've started using wgpu's BufferSlice to pass different buffers without giving up the ownership in a generic way.

It works fine for passing vertex and index buffers, but a problem occurred when I tried to use it to pass uniform buffers: AFAIU,
the only way to bind a uniform buffer is to construct a BufferBinding struct and pass it to the bind group descriptor.

BufferBinding, however, wants a reference to a Buffer, not BufferSlice. With BufferSlice not providing an API to get to its internally stored buffer reference, offset and size (the exact three things BufferBinding wants!), I have no good option to do it this way.

Implementation-wise they seem to be exactly the same: they store a reference to a buffer, start offset and an optional size. They just provide different APIs to use them with.

Describe the solution you'd like
I'd like any solution allowing me use BufferSlice to bind a uniform buffer without me having to refer back to the Buffer it originated from.

The easiest & non-backcompat breaking solution would be to expose the innards of the BufferSlice, allowing to construct a BufferBinding from them. Not sure it the best design, though.

An IMO better solution is replacing the usage of BufferBinding with BufferSlice. This would be a breaking change, but the expressiveness of API wouldn't be changed (instead of providing the offset and size directly, you would have to slice the buffer before passing it to the descriptor). If BufferBinding is to be removed, there will be no rust equivalent to WebGPU's GPUBufferBinding, so not sure if it will work for you.

Describe alternatives you've considered
Implementing a BufferSlice myself and adding conversion operations to wgpu BufferSlice and BufferBinding where needed.

@Wumpf
Copy link
Member

Wumpf commented Sep 22, 2024

You're right, they're pretty much the same, the semantics are just a little bit different of what they represent. One is just a buffer ref + slice, the other is a descriptor to build up bindings.
As pointed out by the docs, the BufferSlice is the "special" api here, serving as a shortcut for buffer + size + offset.

/// The `BufferSlice` type is unique to the Rust API of `wgpu`. In the WebGPU
/// specification, an offset and size are specified as arguments to each call
/// working with the [`Buffer`], instead.

Whereas BufferBinding as you pointed out has a direct equivalent. I'm not unhappy with this distinction because not only is it nicer for people trying to understand wgpu from looking at the WebGPU spec, it allows the docs to be more expressive on the fields (take for example all the notes on BufferBinding::offset).
That, said we're quite ofc flexible what to do with it, since it's already just a piece of sugarcoating :)

I'd suggest:

  • make fields of BufferSlice pub
  • add Into for BufferSlice -> BufferBinding for quality of life

@DCNick3 What do you think, would that make things sufficiently nicer for you?
@cwfitzgerald I'm sure you've been wading in this way more. Do you agree with the suggestion?

@DCNick3
Copy link
Author

DCNick3 commented Sep 22, 2024

Yep, this would work for me

@cwfitzgerald
Copy link
Member

Honestly haven't ever thought about this api :)

@cwfitzgerald
Copy link
Member

I think the best solution here is just removing the buffer slice api all together

@cwfitzgerald cwfitzgerald added type: enhancement New feature or request area: api Issues related to API surface labels Jan 21, 2025
@kpreid
Copy link
Contributor

kpreid commented Jan 22, 2025

I’d like to speak up for another use case for keeping BufferSlice and making it more flexible: in my PR #6900 for StagingBelt::allocate(), I return a buffer and an offset into it. This would be less confusing as a BufferSlice (especially because there is also a buffer mapping involved that should not be offset), but it doesn’t because BufferSlice can't be used as the source of a copy operation. Now, one could take this as “BufferSlice is not pulling its weight”, but I think that BufferSlice-like operations will keep coming up, and therefore the better solution here is that BufferSlice should be made usable in more places (whether that is by conversion operations or by extracting its parts).

Besides the specific use case, continuing to have (and expand on) a convenient way to express “this part of this buffer” may encourage users to use sub-allocation instead of multiple buffers when it’s applicable, which can improve efficiency.

@kpreid
Copy link
Contributor

kpreid commented Jan 22, 2025

P.S. I was specifically planning to add getters to BufferSlice as a followup PR after #6900.

@cwfitzgerald
Copy link
Member

Interesting! I thought it was pretty universally hated - sounds like we need to have a discussion about it. Will file issue tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: api Issues related to API surface type: enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants