-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Bring immediate api in line with spec #8724
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
Conversation
ce1b27b to
e20cf9b
Compare
|
Does this fix #5683 ? |
|
@JMS55 no, but I'll add that to the tracking issue. |
c52d0b8 to
7348c48
Compare
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 brings the immediate data API (formerly known as push constants) in line with the WebGPU specification. The changes simplify the API by removing per-stage configuration in favor of a unified size, and eliminate the stages parameter from upload functions.
Key changes:
- Replaced
immediates_ranges: &[ImmediateRange]withimmediate_size: u32inPipelineLayoutDescriptor - Removed
stages: ShaderStagesparameter from allset_immediates()function calls - Renamed
IMMEDIATES_ALIGNMENTtoIMMEDIATE_DATA_ALIGNMENTthroughout the codebase
Reviewed changes
Copilot reviewed 100 out of 100 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
wgpu/src/api/pipeline_layout.rs |
Updated descriptor to use single size field instead of ranges |
wgpu/src/api/render_pass.rs |
Simplified set_immediates API, removed stage parameter |
wgpu/src/api/render_bundle_encoder.rs |
Updated set_immediates signature |
wgpu/src/util/encoder.rs |
Updated trait definition for set_immediates |
wgpu/src/lib.rs |
Removed ImmediateRange export, renamed constant |
wgpu-types/src/lib.rs |
Removed ImmediateRange struct, renamed constant |
wgpu-core/src/binding_model.rs |
Simplified validation logic and error types |
wgpu-core/src/device/resource.rs |
Updated pipeline layout creation and validation |
wgpu-core/src/command/*.rs |
Updated command encoding to use new API |
wgpu-hal/src/*/device.rs |
Updated backend implementations for new descriptor |
wgpu-hal/src/*/command.rs |
Removed stages parameter from hal command encoders |
tests/**/*.rs |
Updated all test code to use new API |
examples/**/*.rs |
Updated all example code to use new API |
CHANGELOG.md |
Added migration guide for the API change |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
7348c48 to
ab62833
Compare
ab62833 to
a153db7
Compare
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
Copilot reviewed 100 out of 100 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
wgpu/src/backend/webgpu.rs:1
- Corrected spelling of 'multi_draw_indexed_indirect' to 'set_immediates' in panic message.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
teoxoy
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.
Nice! Much simpler 👀
Connections
#8556
Description
This brings the api in line with the spec. Notably we still do zeroing, which is not spec compliant. This will be fixed in a future change.
Diff is great.
Mac setup is a bit wasteful as we upload to all stages, even ones not part of the current pipeline, but we would have to refactor our handling of immediates quite a bit to get stage information in wgpu-hal as the set_immediate call would need to always happen after we know the pipeline we're referring to, and it's not clear to me that this is worth bothering with.
Testing
See Eye, Locally
Squash or Rebase?
COMMIT
TODO