Transition resources #6678
Transition resources #6678
Conversation
There was a problem hiding this comment.
love it, I long wanted this loophole. Obviously wgpu tracking should get better, but having this "expert level" fallback is imho an important asset regardless.
Exposing this directly via hal types at first felt a bit too much like exposing the guts, but that's what this feature is about, so I actually appreciate that it's not usable without dropping into a different namespace.
However, speaking of interfaces: I really don't want us to add a new method to the wgpu api without ever calling it in a test that ensures that it doesn't randomly break down completely. Can you please add a short test that users the new method? Naturally, for the happy case there's not much more to test than "didn't fail".
Ideally it would also come with failure case checking as well (ensuring the right errors are raised), but I'm happy to skip that.
| /// * Use [`CommandEncoder::transition_resources`] to transition resources X and Y from TextureUses::COLOR_TARGET to TextureUses::RESOURCE | ||
| /// * Use resources X and Y in a bind group | ||
| /// | ||
| /// At submission time, wgpu will record and insert some new command buffers, resulting in a submission that looks like `queue.submit(&[0, a, b, 1, c])`: | ||
| /// * CommandBuffer 0: Barrier to transition resources X and Y from TextureUses::RESOURCE (from last frame) to TextureUses::COLOR_TARGET | ||
| /// * CommandBuffer A: Use resource X as a render pass attachment | ||
| /// * CommandBuffer B: Use resource Y as a render pass attachment | ||
| /// * CommandBuffer 1: Barrier to transition resources X and Y from TextureUses::COLOR_TARGET to TextureUses::RESOURCE | ||
| /// * CommandBuffer C: Use resources X and Y in a bind group |
There was a problem hiding this comment.
Why is CommandBuffer 1 still separated out? This was described as adding the resource transition to RESOURCE manually as part of CommandBuffer C, therefore wgpu should see COLOR_TARGET as expected start state of C.
If this is another limitation we have today it would be good to capture this here. Also, the transition_resources call in C is not doing anything then.
There was a problem hiding this comment.
Oh whoops, I think that's a mistake. I don't actually know what will happen with the second transition_resources() call.
Tbh how wgpu's barrier tracking system works still confuses me. Like the fact that it's still inserting CommandBuffer 0 to begin with. I was hoping it could just be a part of A directly.
Can you adjust the docs for me to match how this is supposed to work?
There was a problem hiding this comment.
Hmmm yeah actually CommandBuffer 0 shouldn't be there either 🤔
Let's wait for @cwfitzgerald to have a look - my knowledge of the tracker is quite shaky as well.
|
I would like to review this before it lands |
| for (texture_id, selector, state) in texture_transitions { | ||
| let texture = hub.textures.get(texture_id).get()?; | ||
| texture.same_device_as(cmd_buf.as_ref())?; | ||
|
|
||
| unsafe { | ||
| usage_scope | ||
| .textures | ||
| .merge_single(&texture, selector.clone(), state) | ||
| }?; | ||
| } |
There was a problem hiding this comment.
This might be related to the superfluous command buffers you're still seeing: merge_single on textures requires a call to set_size with the largest texture id ahead of time
There was a problem hiding this comment.
If you forget to call set_size you just explode in debug and get UB in release
There was a problem hiding this comment.
What does this mean? Am I missing a necessary function call?
looks like you found it (: |
There was a problem hiding this comment.
thanks for adding the test! 👍
letting @cwfitzgerald take over as requested
| pub selector: Option<wgc::TextureSelector>, | ||
| /// The new state to transition to. | ||
| pub state: hal::TextureUses, |
There was a problem hiding this comment.
I feel decently strongly that we probably should move these into wgpu types so this api is cross platform not just wgpu-core. The webgpu impl would be a no-op. This is harder to use because you need to start digging into wgc/hal instead of just exposing this to the user normally as part of the interface.
There was a problem hiding this comment.
I had that initially, but it's a lot more of a mess, and dosen't really seem worth it to me given that the function kinda depends on wgpu-core implementation details anyways.
There was a problem hiding this comment.
Discussing this at next meeting.
There was a problem hiding this comment.
Trying again this meeting :)
There was a problem hiding this comment.
Alright, the resolution here from the meeting is that we should not have wgc/hal exposed in this api, we should move both of these types to wgpu-types, rewire core and hal. BufferUses/TextureUses due to the similarity to BufferUsages/TextureUsages, should be right next to their counterparts and have comments explaining why you'd want to use one over the other.
For maintainership purposes, this move should be done as a single commit, so it can be easy to review.
Connections
Description
Wgpu often needs to generate barriers when submitting multiple command buffers.
This PR provides a new API that advanced users can use to generate the barriers manually, so that wgpu won't have to patch them in later (suboptimally).
See the API doc comment.
Testing
Ran Bevy with wgpu main and calling transition_resources().
Before:
After:
No more barriers (vertical blue lines) between the shadow passes.
I also checked the event list, and wgpu correctly batches all 6 of the resources (I submitted 6 resources to transition) into one barrier.
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.