Implement bindless buffers on metal - #9081
Conversation
|
I'm excited to see this and plan on checking out the code. Thanks a ton for working on it! |
Vecvec
left a comment
There was a problem hiding this comment.
A couple of things from a read through.
There was a problem hiding this comment.
Ok so lots of thoughts here, mostly questions. Overall looks good. More comments would be great.
Big thing though is snapshot tests. I can't really review this change until I know what it generates and know that what it generates is correct. There might even be existing tests that you can just add METAL as a target to.
Thanks for putting in the work. I look forward to using this myself, and I'd like to get this landed soon!
|
@mate-h This will probably be assigned once you address at least some of the above comments. |
|
Eagerly awaiting this one and will be testing the PR in my own project. I also volunteer to adopt this PR if @mate-h is busy. |
|
The PR appears to work as intended on my end, backported to wgpu v27. Will post back if I notice any issues. |
Port of gfx-rs#9081 to the v27 metal crate API. - naga: emit `device` pointer syntax for struct-based binding arrays in MSL, use `->` for member access, skip address space for BindingArray globals - wgpu-hal: add BUFFER_BINDING_ARRAY feature flag on Metal (MSL 3.0 + Tier 2), use MTLArgumentEncoder to encode buffer pointer arrays in bind groups - wgpu-hal: extend map_resource_usage for Buffer types, add pointer_array_argument_descriptor helper
|
@msvbg feel free to put up a new PR with things updated, just make sure to keep the authorship on the commits so it's properly attributed |
|
Hi everyone quick update, I am working on addressing the remaining feedback by @inner-daemons and making sure the CI passes. Regarding the backport, for my understanding wondering why it is needed? Let me know @msvbg if you'd like to collaborate on this! |
|
Oh my bad, I did not intend to push to this branch! I'm sorry, that was accidental. I didn't even realize I had push access to this. Can you hard reset/remove my commits? |
|
Okay it seems that I misread the Github interface and I did not push to your branch. Phew! I needed a backport because I'm on an older version of wgpu. It's for my own use. Please disregard the confusion :) As for collaboration, I have nothing further to add to the PR! |
|
@msvbg Just FYI, a back port of this is unlikely to be accepted. We only accept back ports for a few of the most critical bug fixes in every release cycle, and even then we mainly do it for the latest release (27 was 3 releases ago). Thanks for your interest though! |
|
All right I am finished addressing all the feedback, ready for review again. checkout this PR locally at wgpu repo. checkout bevy repo at the branch linked above. place this file to last line is a local git clone of naga master branch or set to git dep. |
inner-daemons
left a comment
There was a problem hiding this comment.
I'm gonna tentatively approve this, since the biggest points have been addressed. I haven't tried incredibly hard yet to reason through all of the writer changes since my last review but what I've noticed seems reasonable.
|
@cwfitzgerald Feel free to take a look |
cwfitzgerald
left a comment
There was a problem hiding this comment.
Making solid progress! Some more minor and pedantic issues and a bit more testing
|
This is still waiting on author |
|
Is this still being worked on? |
|
I have not worked on this in a while, I'll come back to it now! I got some more feedback from @cwfitzgerald that I need to address. And also merge in the |
cwfitzgerald
left a comment
There was a problem hiding this comment.
Thanks for the progress, some more comments, mainly around testing
|
@cwfitzgerald thanks for your continued feedback! I have addressed every outstanding comment on this PR, let me know if there's anything else to resolve. |
cwfitzgerald
left a comment
There was a problem hiding this comment.
Approving, looks good with one caveat that we should make an issue for.
| int _e104 = uint(0) < 10 && _buffer_sizes.size0[0u] != 0u && uint(non_uniform_index) < 10 && _buffer_sizes.size0[unsigned(non_uniform_index)] != 0u ? storage_array[non_uniform_index].inner->far[0] : DefaultConstructible(); | ||
| u1_ = _e99 + as_type<uint>(_e104); | ||
| uint _e107 = u1_; | ||
| int _e112 = uint(0) < 10 && _buffer_sizes.size0[0u] != 0u && uint(7) < 10 && _buffer_sizes.size0[7u] != 0u ? storage_array[7].inner->far[0] : DefaultConstructible(); |
There was a problem hiding this comment.
How does this interact with partially bound binding arrays. We're introducing bounds checks on the binding array itself, but with partially bound arrays the size isn't known to the shader. This is definitely an improvement than before where we had no bounds checks at all, so feel free to push this to an issue/follow up, but something to consider.
# Objective [Ray tracing on Metal](gfx-rs/wgpu#8071) has been available since wgpu 29. I shipped a PR to wgpu for [implementing bindless storage buffers](gfx-rs/wgpu#9081) for metal, which was released with wgpu 30. With the [wgpu 30 PR](bevyengine#24841) in bevy merged, we are fully ready to add support to Solari on metal backends! However because there is a limit of 31 buffer slots per shader stage. Running Solari on main on a raytracing compatible Mac results in a crash. ## Solution I had combine the world cache into a single packed buffer. In theory this should not introduce performance regressions since it’s only reorganizing bindings and doesn’t introduce and copy or other passes. Tried keeping the diff fairly minimal and easy to review. Getting the buffer layout for the world cache on the CPU with `ShaderType` was tricky. To avoid blowing up memory I put it in a private module so you can't construct it. ## Testing - Ran the Solari example on mac ``` cargo run --example solari --features bevy_solari,https,free_camera ``` Currently this PR needs a benchmark against main to ensure there's no perf regression. I could use some help with that! --- ## Showcase <img width="2566" height="1514" alt="shot" src="https://github.com/user-attachments/assets/691b9bd3-129f-47e6-8efe-313cbadd6ff5" /> ## Known limitations Tracing spans incorrectly report 0.0ms , tracking in gfx-rs/wgpu#9414 No denoising support yet. Could theoretically use [MetalFX Ray Reconstruction](https://crates.io/crates/objc2-metal-fx) in place of DLSS for cross-platform denoising support in Solari. In the longer term we are also looking at Open Image Denoise 3.0 that promises temporal denoising support. --------- Co-authored-by: Chris Biscardi <chris@christopherbiscardi.com>
Connections
Resolves #6741
Description
Problem: Metal on macOS did not support bindless storage buffers.
In
wgpu-hal, encode buffer binding arrays usingMTLBufferargument encoder. Metal expects encoded pointers, not raw resource IDs. Creates an argument buffer from an encoder, encode each buffer, and register it inresources_to_use.In naga MSL writer, emit
device T*for buffer binding array elements (which are pointers to device memory) instead of the constant layout used for textures and samplers. For struct members behind binding arrays, emit->instead of.because the elements are pointers.In the adapter, expose the
BUFFER_BINDING_ARRAYfeature on supporting devices, with special handling forMTLCaptureDevice. This is a separate fix for Xcode frame capture, when some functions fail to invoke, in these cases fallback to feature table based limits. I referenced the metal feature table PDF.Testing
Tested with Bevy Solari on metal / macOS which makes use of bindless storage buffers and raytracing acceleration structures.
Squash
Checklist
cargo fmt.taplo format.cargo clippy --tests. If applicable, add:--target wasm32-unknown-unknowncargo xtask testto run tests.CHANGELOG.mdentry.