-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Update WGPU to version 22 #14401
Update WGPU to version 22 #14401
Conversation
Welcome, new contributor! Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨ |
@@ -29,7 +29,7 @@ zstd = ["ruzstd"] | |||
|
|||
trace = ["profiling"] | |||
tracing-tracy = [] | |||
wgpu_trace = ["wgpu/trace"] | |||
wgpu_trace = [] |
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.
wgpu_trace was temporarily removed gfx-rs/wgpu#5974. We should remove our feature for now too imo (and the corresponding docs we have referencing it).
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.
Hi, I was browsing around and looking at Bevy's PRs, so please excuse me if I seem a bit out of place.
I assume that if the wgpu_trace
feature is removed, it'll be re-added back eventually? If so, then I might recommend keeping the wgpu_trace
feature, but have a note somewhere (in the docs? during compilation? as a compile error? etc.) that the feature does not currently work, linking to the wgpu issue you mentioned.
That said, if you actually mean to remove it permanently, then my suggestion would be nil.
(P.S. Personally, I don't see the use in this feature, compared to just saying "pull in wgpu
and include the trace
feature". But that's me - maybe bevy has the wgpu_trace
feature for a reason I'm not aware of?)
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.
I'm in favor of just removing it outright, but if we really wanted to keep it, putting in a compile error with a link to an issue seems like a good idea to me.
@@ -355,6 +355,7 @@ pub async fn initialize_renderer( | |||
label: options.device_label.as_ref().map(AsRef::as_ref), | |||
required_features: features, | |||
required_limits: limits, | |||
memory_hints: wgpu::MemoryHints::default(), |
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.
Probably want to expose this in the render plugin settings like we do with other things.
naga_oil has been updated to wgpu 22 and is now published. Can you update this PR with the new naga_oil version? |
PR for that callym#1 edit: It should be good to go after that. |
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.
LGTM, just 1 tiny change.
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.
LGTM, I tested a few examples and it all seems to work as expected
Upgrading to WGPU 22.
Needs
naga_oil
to upgrade first, I've got a fork that compiles but fails tests, so until that's fixed and the crate is officially updated/released this will be blocked.