Skip to content
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

[BREAKING] Make Shader(Module)Source::Wgsl optional #2890

Merged
merged 1 commit into from
Oct 8, 2022

Conversation

daxpedda
Copy link
Contributor

@daxpedda daxpedda commented Jul 17, 2022

Checklist

  • Run cargo clippy.
  • Run RUSTFLAGS=--cfg=web_sys_unstable_apis cargo clippy --target wasm32-unknown-unknown if applicable.
  • Add change to CHANGELOG.md. See simple instructions inside file.

Connections
Discussed in Matrix.

Description
This is totally a breaking change! It also might turn out entirely unnecessary if the compile-time savings aren't worth the added complexity.

At the moment the naga wgsl-in crate feature is active even when completely unnecessary. This addresses it by gating Shader(Module)Source::Wgsl behind a new wgsl crate feature, like with ShaderSource::Glsl for example. The added wgsl crate feature is also active by default.

Code quality is definitely a bit worse with all of these #[cfg(...)] now. CI time is increased because --no-default-features has to be tested.

I also didn't know what to do about Shader(Module)Sources lifetime, at the moment the lifetime is not present when the only active variant is Naga, which I consider very user unfriendly and could only be fixed by either stop using Cow or adding a dummy lifetime to the Naga variant. This could also be solved by #2903, so maybe we should merge that first. (can't bind 'a to the 'static in Cow)

I also had to increase the MSRV to 1.60 to not unnecessarily pull in wgpu-core on WASM by using the weak dependency feature.

Testing
Added additional runs with --no-default-features in the CI.

@daxpedda daxpedda closed this Jul 17, 2022
@daxpedda daxpedda reopened this Jul 17, 2022
@daxpedda
Copy link
Contributor Author

Okay, CI just has issues installing Rust, I will try again later.

@daxpedda daxpedda force-pushed the naga-wgsl-optional branch from 479bbe8 to 0ddb055 Compare July 17, 2022 19:05
@daxpedda daxpedda marked this pull request as ready for review July 17, 2022 19:19
@daxpedda daxpedda marked this pull request as draft July 17, 2022 19:19
@daxpedda daxpedda closed this Jul 17, 2022
@daxpedda daxpedda reopened this Jul 17, 2022
@daxpedda daxpedda changed the title Make Shader(Module)Source::Wgsl optional [BREAKING] Make Shader(Module)Source::Wgsl optional Jul 21, 2022
@daxpedda daxpedda marked this pull request as ready for review July 21, 2022 11:24
@daxpedda
Copy link
Contributor Author

daxpedda commented Sep 3, 2022

Rebased after #2872.

wgpu/src/lib.rs Outdated Show resolved Hide resolved
wgpu/Cargo.toml Show resolved Hide resolved
@daxpedda daxpedda force-pushed the naga-wgsl-optional branch 2 times, most recently from 213f9fd to 4235155 Compare September 22, 2022 23:10
@daxpedda daxpedda force-pushed the naga-wgsl-optional branch 3 times, most recently from 763f68c to 5150380 Compare September 22, 2022 23:37
@daxpedda daxpedda requested a review from crowlKats as a code owner September 22, 2022 23:37
@daxpedda daxpedda force-pushed the naga-wgsl-optional branch from 5150380 to 74bfaf5 Compare October 6, 2022 08:00
@daxpedda
Copy link
Contributor Author

daxpedda commented Oct 6, 2022

Rebased again.
Too bad it didn't make the cut for 0.14. @cwfitzgerald can we get this in before it gets forgotten again?

Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack! So sorry about this! I didn't realize the "changes requested" text doesn't get reset when the changes get addressed.

Small comments but then g2g

wgpu/Cargo.toml Show resolved Hide resolved
wgpu/src/lib.rs Show resolved Hide resolved
@daxpedda daxpedda force-pushed the naga-wgsl-optional branch 2 times, most recently from 63c2c8d to 1cdf0bf Compare October 7, 2022 07:59
@daxpedda daxpedda requested review from cwfitzgerald and removed request for crowlKats October 7, 2022 08:00
@daxpedda daxpedda force-pushed the naga-wgsl-optional branch from 1cdf0bf to 2149d9b Compare October 7, 2022 08:13
Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

G2G! Please just add a changelog entry that mentions there's a new feature, but it's default.

@daxpedda daxpedda force-pushed the naga-wgsl-optional branch 3 times, most recently from 71f5b10 to d8780ef Compare October 7, 2022 17:11
@daxpedda
Copy link
Contributor Author

daxpedda commented Oct 7, 2022

Done.

@daxpedda daxpedda requested a review from cwfitzgerald October 7, 2022 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants