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

Add preliminary support for Rust as an output format (via rust-gpu) #5175

Closed
wants to merge 7 commits into from

Conversation

LegNeato
Copy link
Contributor

@LegNeato LegNeato commented Jan 31, 2024

Description
This adds preliminary support for Rust as a naga output format. Currently it only supports rust-gpu but the intention is to support CPU as well.

I broke this into commits that touch non-Rust backend things and then a big 'ol commit adding the Rust backend. Happy to split out any changes into different PRs. I kept all the Rust conversion logic in one file for now but intend to break it up for readability in the future.

I studied the existing backends and it looks like the wild west...some use shared abstractions, some don't. I tried to match to the GLSL backend style where possible but didn't worry too much about doing so.

I am still heavily working on this, though it is at the point where it actually does something(ish). I mainly wanted to put this up so that a) folks know this is coming and b) folks can iterate on this with me if they want.

If you'd prefer to wait until functionality is close to complete to land rather than iterating in-repo, I am totally fine with closing this PR and working on the backend in my fork.

Testing

  • cargo test --all-features --release rust
  • RUST_LOG=trace cargo run --release -- /tmp/whatever.frag /tmp/whatever.rs && cat /tmp/whatever.rs

Checklist

  • Run cargo fmt.
  • Run cargo clippy. If applicable, add:
    • --target wasm32-unknown-unknown
    • --target wasm32-unknown-emscripten
  • Run cargo xtask test to run tests.
    Failed on my machine with unrealted error.
  • Add change to CHANGELOG.md. See simple instructions inside file.
    Not adding to changelog as it is still WIP / incomplete.

@LegNeato LegNeato requested review from a team as code owners January 31, 2024 18:30
The goal is for this to output both `rust-gpu` and CPU code,
though currently it just does `rust-gpu`.
Copy link
Contributor

@eddyb eddyb left a comment

Choose a reason for hiding this comment

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

Hi, this is an incredible way to automatically get Rust-GPU source code (I'm guessing the similarities with WGSL help a lot), but also wow, the Rust-GPU team (@fornwall and I, as of late) was not aware of this work.

I don't want to get in the way of anything but I would prefer to be kept in the loop with how this develops, and I will of course try to review this ASAP (at the very least mentioning @eddyb on updates/for requesting feedback/etc. ensures I get notified).

(Off the top of my head, some details may change with the Rust-GPU version being targeted - we still primarily have semver-incompatible releases, just like wgpu/naga themselves - so some strategy is required there, but we have no major breaking changes planned any time soon at least)

Leaving this comment as a review so that it hopefully is easier to see than random comments.

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.

Marking to get it out of the wgpu review queue - will leave this up to the naga reviewers

This isn't great, we should really do this once
at the module or namer level rather than at global
variable and entrypoint conversion time.

That being said, it works for now.
@LegNeato
Copy link
Contributor Author

Update on this: it was suggested in matrix that the backend should be maintained out of tree for now. I've been working on it and filing issues on naga as I hit them. Once it is sufficient, I'll close this and link to where the out of tree backend is.

@cwfitzgerald
Copy link
Member

I'm going to close this PR for bookkeeping purposes, please do drop a link in here and in the readme to your out-of-tree backend so people can find it.

@LegNeato
Copy link
Contributor Author

For those following along I am still working on this locally. There are some patches to other projects that need to land and I will push it publicly and drop a link here.

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.

3 participants