-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add support for transient textures on Vulkan and Metal #8247
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
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.
Thanks for taking this on! It just so happened we just talked about wanting this in the WebGPU face to face, so super happy to have this! There's a few things that need to be added, but I don't think it will end up being too much work:
- At the face to face, we've decided that transient textures wouldn't be a feature. It would be available everywhere, but a no-op on unsupported platforms. If we wanted to expose to the user if this is supported, we can do this in the AdapterInfo as a hint.
- We need two pieces of validation:
a. If you add the transient attachment usage, the usage must be exactly RENDER_ATTACHMENT | TRANSIENT
b. When writing a renderpass to these attachments, the StoreOp must be discard. - With point 1 implemented, we should stick these flags on as many of the examples as relevant as we really want to encourage its use.
- On VK, we need to add
VK_MEMORY_PROPERTY_LAZILY_ALLOCATED_BIT
to the memory so that it may be lazily allocated. I think you may be getting this already implicitly without explicitly asking for it, if you're seeing memory improvements. - We should add some tests
a. For all the validation I mentioned, these can be simple tests in wgpu-validation.
b. We should have at least one test in the wgpu-gpu tests which ensures we don't get validation errors from vk/metal.
Let me know if you have any questions or if any of this is unclear!
Thanks for your input @cwfitzgerald! For:
|
Re 3: |
Yeah this is unfortunately a mobile only feature really, but hopefully the validation layers can save us on desktop. Apple is the main one that really benefits from it on "desktop". |
I did make it so that a if a memory type with the |
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.
Some comments and needs tests, but moving in the right direction!
Alright, I looked through it more carefully again, we should be able to apply |
Co-authored-by: Connor Fitzgerald <[email protected]>
Co-authored-by: Connor Fitzgerald <[email protected]>
Renders a single triangle with transient texture as target to make sure nothing is wrong
Confirmed that this is working as expected on mac - will do another review. |
(I'm doing this to see how it does, I'll do a full human review tomorrow, no action is needed based on its comments). |
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.
Pull Request Overview
This PR adds support for transient textures on Vulkan and Metal backends by introducing TextureUsages::TRANSIENT
. This optimization allows textures to use MTLStorageMode.memoryless
on Metal and VK_MEMORY_PROPERTY_FLAG_BITS_LAZILY_ALLOCATED
on Vulkan, potentially reducing memory usage and bandwidth for textures that aren't reused between passes.
Key changes include:
- Added
TextureUsages::TRANSIENT
flag with validation requiringStoreOp::Discard
and incompatibility with other usages - Extended backend support detection via
AdapterInfo.supports_transient
- Updated examples to demonstrate transient texture usage for depth and MSAA textures
Reviewed Changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
wgpu-types/src/lib.rs | Added TRANSIENT texture usage flag, validation, and adapter support field |
wgpu-hal/src/vulkan/ | Implemented Vulkan lazily allocated memory support for transient textures |
wgpu-hal/src/metal/ | Added Metal memoryless storage mode support for transient textures |
wgpu-core/src/ | Added validation logic and error handling for transient texture usage |
tests/ | Added validation tests and GPU render tests for transient texture functionality |
examples/ | Updated depth and MSAA texture usage to include TRANSIENT flag |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
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, I thought of one more potential problem:
We need to validate out creating a texture with only TRANSIENT usage. I'm not sure what metal would do with this, but it can't be good 😆
One final thing, you should do a pass over the backends and make sure transient is advertised https://github.com/gfx-rs/wgpu/blob/trunk/wgpu-hal/src/lib.rs#L670-L673 wherever RENDER_ATTACHMENT is advertised.
Thanks for being so helpful with this process, we should be almost finished!!
driver: String::from("wgpu"), | ||
driver_info: String::new(), | ||
backend: wgt::Backend::Noop, | ||
supports_transient: true, |
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.
supports_transient: true, | |
supports_transient: false, |
See the change in the doc comment on this field for the doc 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.
This needs to be updated here too
/// Backend used for device | ||
pub backend: Backend, | ||
/// Supports [`TextureUsages::TRANSIENT`] | ||
pub supports_transient: bool, |
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.
pub supports_transient: bool, | |
pub native_transient_texture: bool, |
Really not set on this name, but what I'm trying to avoid is the implication that TRANSIENT is not allowed
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.
This still needs to be updated
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.
Oops, been pretty busy recently. I should be able to finish the rest of the changes this weekend. Sorry for the wait.
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.
No worries or rush! Just re-request a review from me whenever you're ready!
Co-authored-by: Connor Fitzgerald <[email protected]>
Co-authored-by: Connor Fitzgerald <[email protected]>
Co-authored-by: Connor Fitzgerald <[email protected]>
What's the use case for this? One thing I do in Bevy is use a dummy render target for a render pass, since I use storage atomics for my output. I have wanted render passes with no render attachments for this reason, so that we don't need to waste memory on a dummy render target. Would this be a suitable replacement? |
Tilers where you need an attachment but never actually read from it from memory. You can avoid allocating that memory.
This is better than nothing, but not really. It would only actually reduce memory on tilers. |
Connections
Fixes #8239
Description
Exposes
MTLStorageMode.memoryless
for Metal andvk::MemoryPropertyFlags::LAZILY_ALLOCATED
for Vulkan by addingTextureUsages::TRANSIENT
.Testing
Added corresponding
TextureUsage
flag to relevant examples.Also added some render tests and validation testing.
Squash or Rebase?
Squash
Checklist
cargo fmt
.taplo format
.cargo clippy --tests
. If applicable, add:--target wasm32-unknown-unknown
cargo xtask test
to run tests.CHANGELOG.md
entry.