-
Notifications
You must be signed in to change notification settings - Fork 1.1k
[wgsl-in, wgsl-out, glsl-in] WebGPU compliant dual source blending feature #7146
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
d101739
to
ccbb722
Compare
b14e3e1
to
a15afa0
Compare
a15afa0
to
615acda
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
I've done some significant reviewing today, and I intend to finish on my Monday. Looking mostly good so far, with most of my nits on diagnostics. 🙂 |
Sigh, I haven't been able to finish this review because of some resource conflicts in other parts of my day job and personal life. I do intend to finish this soon, though. |
14585a9
to
66bcdf3
Compare
This comment was marked as resolved.
This comment was marked as resolved.
66bcdf3
to
ec47c29
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.
Approving, conditional on outstanding conversations being resolved. Since most conversations should be resolvable without code changes here directly, I want to unblock merging.
I used Conventional Comments in this review! I hope they help with clarity and tone. 🙂
6ad7bd8
to
9a0c1e1
Compare
Connections
CHANGELOG
a bit #7274impl From<ImplementedEnableExtension> for EnableExtension
#7275enable
extensions #7276Description
Makes the dual source implementation in wgpu WebGPU spec compliant.
This means changing the wgsl syntax & associated validation to match what's described here and here and here.
Furthermore, makes the dual source blending extension available when targeting WebGPU.
In order to reliably test wgsl output I incorrectly assumed that the way to go would be to add a glsl input to the snapshot tests (mostly to check for the
enable
directive being written out correctly), didn't realize that there's wgsl->wgsl snapshot testing as well, so I ended up implementing glslindex
parsing as collateral😄Testing
Added a slew of naga tests. Did manual testing in a pet project against native & WebGPU
Draft TODO
[help wanted] [partially orthogonal]: While trying this out on my pet project I couldn't get it to fly in WebGPU because I was using naga_oil (with a hack, see also Support wgsl diagnostic directives. bevyengine/naga_oil#113 (comment)) where I have to rely onModule
->write_wgsl
which right now ignores all directives 😱 . Fixing that isn't entirely straight forward sinceModule
is language agnostic so I have to detect the need for thoseChecklist
cargo fmt
.taplo format
.cargo clippy
. If applicable, add:--target wasm32-unknown-unknown
cargo xtask test
to run tests.CHANGELOG.md
. See simple instructions inside file.