Add check to ensure vulkan::CommandEncoder::discard_encoding is not called multiple times in a row
#5557
Conversation
|
I'm actually -0.5 on this PR. The idea is for |
There was a problem hiding this comment.
Having thought about this more, I'd like to put a temporary pause on this PR.
At least given my understanding of the role wgpu's core/hal distinction is supposed to play in keeping wgpu maintainable, adding obligations that wgpu-hal trait implementors must meet to make life easier for their users is putting the code in the wrong place. State tracking, validation, and resource management should be handled in common code within wgpu-core, not repeated in each wgpu_hal backend.
@villuna, apologies for the back-and-forth - there's nothing wrong with the code at all, and it looks like it correctly implements the intended change. I just want a day or so for the maintainers to talk things over and get a shared understanding.
|
For what it's worth, I've been looking into |
|
Thanks for the comments, I've made the changes you requested. I've also removed the drop implementation, since it was calling discard_encoding twice and triggering the assert panic 😅. I guess it's working |
villuna
changed the title
wgpu_hal::vulkan::CommandEncoder::discard_encoding idempotent and implement Drop for the encoder.vulkan::CommandEncoder::discard_encoding is not called multiple times in a row
jimblandy
dismissed
cwfitzgerald’s stale review
Superseded by subsequent reviews.
Connections
Addresses #5255
Description
I just followed the (very helpful) todo list in Erich's issue, so the info is mostly there.
The vulkan implementation for
CommandEncoderwas previously pushing its active CommandBuffer handle to its discard vector every time thediscard_encodingfunction was called, even if the handle was null. So I made it only do this if the active handle is non-null, so as to make the function idempotent.Testing
No new tests but I ran the existing test suite on Vulkan (on an Nvidia GTX1050) and it ran the same as on the trunk branch.
Checklist
cargo fmt.cargo clippy. If applicable, add:--target wasm32-unknown-unknown--target wasm32-unknown-emscriptencargo xtask testto run tests - I got one error every now and then, but this also happened on the main branch so I think it's fine.CHANGELOG.md. See simple instructions inside file.