-
Notifications
You must be signed in to change notification settings - Fork 1.2k
staging belt: allow creation with custom buffer usages #8580
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
base: trunk
Are you sure you want to change the base?
Conversation
|
I think the new constructor would need a |
tests/tests/wgpu-validation/util.rs
Outdated
| //! Tests of [`wgpu::util`]. | ||
| use nanorand::Rng; | ||
| use wgpu::BufferUsages; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we have a set style on this yet, but since the file currently uses all qualified paths for wgpu, please stick to that; that is, remove this use and update the code to work without it.
tests/tests/wgpu-validation/util.rs
Outdated
| let (device, _queue) = wgpu::Device::noop(&wgpu::DeviceDescriptor::default()); | ||
| let _belt = wgpu::util::StagingBelt::new_with_buffer_usages(device.clone(), 512, usage); | ||
| }) | ||
| .is_err() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest checking at least a substring match of the panic message. In my experience, tests need to check for the specific error they are looking for, or they will start falsely passing in the future when changes accidentally make them trigger a different error.
| let message = if let Some(message) = panic.downcast_ref::<&str>() { | ||
| *message | ||
| } else if let Some(message) = panic.downcast_ref::<String>() { | ||
| message.as_str() | ||
| } else { | ||
| // don't know what this panic is, but it's not ours | ||
| std::panic::resume_unwind(panic); | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: There's another version of something like this code in tests/src/run.rs and another just added in #8595, so it's probably time to make it into one helper function. Not as part of this PR, though.
cwfitzgerald
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, some comments about comments and docs.
wgpu/src/util/belt.rs
Outdated
| /// supported. Because [`MAP_WRITE`](BufferUsages::MAP_WRITE) is implied, only | ||
| /// [`COPY_SRC`](BufferUsages::COPY_SRC) can be used, except if | ||
| /// [`Features::MAPPABLE_PRIMARY_BUFFERS`] is enabled. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// supported. Because [`MAP_WRITE`](BufferUsages::MAP_WRITE) is implied, only | |
| /// [`COPY_SRC`](BufferUsages::COPY_SRC) can be used, except if | |
| /// [`Features::MAPPABLE_PRIMARY_BUFFERS`] is enabled. | |
| /// supported. Because [`MAP_WRITE`](BufferUsages::MAP_WRITE) is implied, the allowed usages | |
| /// depends on if [`Features::MAPPABLE_PRIMARY_BUFFERS`] is enabled. | |
| /// - If enabled: any usage is valid. | |
| /// - If disabled: only [`COPY_SRC`](BufferUsages::COPY_SRC) can be used. |
How does that sound, I think this is a bit clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better :)
I strictly any usage allowed with MAPPABLE_PRIMARY_BUFFERS? And what if that would change in the future? Maybe we should just link to the documentation of that feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any anything is allowed with MAPPABLE_PRIMARY_BUFFERS - that shouldn't change in the future. This was one of the first features we had and has remained largely unchanged since.
Connections
This addresses #8575.
allocatewas introduced in #6900Description
StagingBeltcan give you the buffer slice for more advanced usecases. The documentation incorrectly states that you could e.g. use it in a compute pass. Because the buffers are created withMAP_WRITE | COPY_SRCthis is incorrect.Creating buffers with custom buffer usages is possible, although
MAPPABLE_PRIMARY_BUFFERSmust be enabled.This PR adds a
new_with_buffer_usagesmethod with a third argument for the intended buffer usages. We could also just extend the currentnewmethod, if API breakage is possible. The new method verifies that the combination ofBufferUsages is possible depending on theMAPPABLE_PRIMARY_BUFFERSfeature.The
MAP_WRITEusage is always added to the supplied buffer usages.I've also stripped the mention of using this with a compute pass from the documentation of
allocate. In my opinion using the staging belt with mappable primary buffers is really a niece case.Testing
I've added 2 test:
staging_belt_panics_with_invalid_buffer_usagesgoes through all buffer usages exceptCOPY_SRCandMAP_WRITEand checks that the constructor panics.staging_belt_works_with_non_exclusive_buffer_usagesthat the new constructor works withCOPY_SRC,COPY_SRC | MAP_WRITEandMAP_WRITE.Furthermore
staging_belt_random_testverifies that any previous functionality isn't lost.Merge or Squash
This should be squashed.
Checklist
cargo fmt.taplo format.cargo clippy --tests. If applicable, add:--target wasm32-unknown-unknowncargo xtask testto run tests.CHANGELOG.mdentry.