Expose StagingBelt allocation directly.
#6900
Conversation
DJMcNab
left a comment
DJMcNab
left a comment
There was a problem hiding this comment.
This makes StagingBelt much more compelling for me as a user of wgpu.
wgpu/src/util/belt.rs
Outdated
| } | ||
|
|
||
| struct Chunk { | ||
| buffer: Arc<Buffer>, |
There was a problem hiding this comment.
Does this still need to be Arc?
There was a problem hiding this comment.
No, it doesn't. There also doesn't need to be a size field any more. Removed those things.
cwfitzgerald
left a comment
cwfitzgerald
left a comment
There was a problem hiding this comment.
One comment, looks good otherwise
|
@kpreid is this rebaseable? If so I can rebase away the merge commit |
This allows using `StagingBelt` for copying to textures or any other operation where copying to an existing buffer is not the desired outcome. One unfortunate feature of the API is that `allocate()` returns the entire buffer and an offset, so applications could accidentally touch parts of the belt buffer outside the intended allocation. It might make more sense to return `wgpu::BufferSlice`, but that struct cannot be used in operations like `copy_buffer_to_texture` and does not have getters, so it is not currently suitable for that purpose. I also moved `Exclusive` into a module so that its unsafe-to-access field is properly private, and made various improvements to the `StagingBelt` documentation, such as acknowledging that `write_buffer_with()` exists.
`Buffer` now implements `Clone` on its own.
|
I’m not sure what you mean by “rebaseable” but I’ve rebased it. |
|
I mean, mergable by rebase-and-merge as opposed to squash and merge |
|
Oh, sorry. Yes, this one does not need squashing. Not much would be lost by squashing it, but the commits are individually coherent and should pass tests. In general, I don't publish broken commits. I’ll make sure to specify this in future PR descriptions. |
|
Sounds good! We have a change to the pull request template in #6879 which will also explicitly ask this. |
3 participants
Connections
Description
Added a method
StagingBelt::allocate(). This allows usingStagingBeltfor copying to textures or any other operation where copying to an existing buffer is not the desired outcome.One unfortunate feature of the API is that
allocate()returns the entire buffer and an offset, so applications could accidentally touch parts of the belt buffer outside the intended allocation. It might make more sense to returnwgpu::BufferSlice, but that struct cannot be used in operations likecopy_buffer_to_textureand does not have getters, so it is not currently suitable for that purpose.I also moved
Exclusiveinto a module so that its unsafe-to-access field is properly private, and made various improvements to theStagingBeltdocumentation, such as acknowledging thatwrite_buffer_with()exists.Testing
I have manually tested the new function with my application which uses
StagingBeltheavily. There are no unit-tests ofStagingBeltand I did not add any.Checklist
cargo fmt.taplo format.cargo clippy. If applicable, add:--target wasm32-unknown-unknown--target wasm32-unknown-emscriptencargo xtask testto run tests.CHANGELOG.md. See simple instructions inside file.