Skip to content

Initial naga changes for mesh shaders #7930

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

Draft
wants to merge 103 commits into
base: trunk
Choose a base branch
from

Conversation

SupaMaggie70Incorporated
Copy link
Contributor

@SupaMaggie70Incorporated SupaMaggie70Incorporated commented Jul 11, 2025

Connections
#7197

Description
This PR isn't meant to be merged itself, its obviously way too large for that. Instead I will be breaking off chunks into separate PRs.

Testing
There is a test WGSL shader that is parsed and then written to SPIR-V

Squash or Rebase?
Absolutely squash

Checklist

  • Run cargo fmt.
  • Run taplo format.
  • Run cargo clippy --tests. If applicable, add:
    • --target wasm32-unknown-unknown
  • Run cargo xtask test to run tests.
  • If this contains user-facing changes, add a CHANGELOG.md entry.

 Please enter a commit message to explain why this merge is necessary,
The CI pooped itself, hopefully this fixes that. Will probably be undone either way.
@SupaMaggie70Incorporated
Copy link
Contributor Author

@andyleiserson Preferably I would want the toml configuration logic to be shared and not copy/pasted between different crates. But I am not familiar enough with the project structure to know where it should live, or if the naga benchmarks should be moved to the crate in naga/tests/naga. So if you or someone else could provide an opinion about how to handle that I'd be happy to do the leg work (probably in a separate PR).

That being said, I get the impression that refactors of code/moving code around should only be done by trusted people, as other individuals could hide something malicious in the large change. Not sure about wgpu's process in that regard but something to keep in mind probably.

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.

This is an absolutely herculean effort! I didn't see much that is would be of major concern.

I would advise however splitting up the PR, either into multiple PRs or multiple independent commits. A possible suggestion for splitting

  • IR changes + ron test output changes
  • WGSL frontend
  • Spirv backend

The wgsl frontend and spirv backend could be completely independent PRs, both dependent on the fairly minor IR changes. I think that would put diff sizes in the more reasonable range.

return_info: &super::MeshReturnInfo,
body: &mut Vec<Instruction>,
) -> Result<(), Error> {
let vert_info = self.mesh_shader_output_variable(
Copy link
Member

@cwfitzgerald cwfitzgerald Aug 14, 2025

Choose a reason for hiding this comment

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

This could use a number of comments explaining what we're doing here and why we need to do it for mesh shaders. This is a very long and important function, so comments throughout explaining it all (will also help).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I attempted to write some comments. Let me know if you think I should do more.

@SupaMaggie70Incorporated
Copy link
Contributor Author

@cwfitzgerald Thanks for the review. I've tried to sprinkle a few more comments around the code in addition to addressing your comments, so if you could ensure that the code is understandable to yourself and others that would be great. Your idea for splitting up the PR is pretty much exactly what I was thinking. I will probably break off the naga IR changes tomorrow. I'll also hopefully find the motivation to fix the benchmark issue before WGSL parsing gets merged in.

@SupaMaggie70Incorporated SupaMaggie70Incorporated marked this pull request as draft August 15, 2025 17:37
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