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

[naga-cli] Add input-kind and shader-stage args #5411

Merged
merged 8 commits into from
Apr 18, 2024

Conversation

ratmice
Copy link
Contributor

@ratmice ratmice commented Mar 18, 2024

Connections
None

Description
When dealing with .glsl files which have just a plain .glsl extension in git.
Renaming things such that they include the shader stage can be inconvenient.

This adds an argument --shader-stage and uses the FromStr implementation of that for both extension parsing and command line arguments.

This also allows fragment, vertex, compute to be accepted as well.

In addition this also adds --input-kind argument which accepts glsl, etc...

🍀

Testing

touch foo{,{,.{bar,frag,fragment}}.glsl}
cargo run -p naga-cli -- foo.frag.glsl
cargo run -p naga-cli -- --shader-stage frag foo.glsl
cargo run -p naga-cli -- --shader-stage frag --input-kind glsl foo
cargo run -p naga-cli -- foo.fragment.glsl
# should error
cargo run -p naga-cli -- foo.bar.glsl
cargo run -p naga-cli -- --shader-stage bar foo.glsl

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.
  • Add change to CHANGELOG.md. See simple instructions inside file.

@ratmice ratmice requested a review from a team as a code owner March 18, 2024 00:15
naga-cli/src/bin/naga.rs Outdated Show resolved Hide resolved
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 definitely needed functionality! Have some comments about errors and code structure.

naga-cli/src/bin/naga.rs Outdated Show resolved Hide resolved
naga-cli/src/bin/naga.rs Outdated Show resolved Hide resolved
naga-cli/src/bin/naga.rs Outdated Show resolved Hide resolved
naga-cli/src/bin/naga.rs Outdated Show resolved Hide resolved
@ratmice ratmice requested a review from a team as a code owner March 21, 2024 06:10
@ratmice
Copy link
Contributor Author

ratmice commented Mar 21, 2024

@cwfitzgerald I believe I have resolved all of your review concerns.

This leaves on the table some additional clean-ups regarding existing errors, and replacing them with anyhow usage.
I figured these should be left for a follow up patch.

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.

Waiting for anyhow is fine, though it may just be easier to switch without needing to build out the anyhow-like infra. Up to you.

Requesting changes as some changes weren't made.

(Reminder: please re-request a review from me once the changes are addressed to make sure I see it!)

@ratmice ratmice requested a review from cwfitzgerald April 17, 2024 22:46
@ratmice
Copy link
Contributor Author

ratmice commented Apr 17, 2024

Sorry about the branch mixup, this switches it to use anyhow, and should contain the fixes I was referring to previously.

What i'd meant previously is that there are further cleanups to existing code that can probably benefit from using anyhow.
But this patch just does the minimal changes needed for the new code to use context() etc. With the intent to do those cleanups in a follow-up.

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.

Looking a lot better, one more round, then just need a naga person to sign off on it, then we're good to land!

naga-cli/src/bin/naga.rs Outdated Show resolved Hide resolved
naga-cli/src/bin/naga.rs Outdated Show resolved Hide resolved
naga-cli/src/bin/naga.rs Outdated Show resolved Hide resolved
Cargo.toml Show resolved Hide resolved
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.

Great - going to boop the naga people

Copy link
Member

@teoxoy teoxoy left a comment

Choose a reason for hiding this comment

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

Nice!

@teoxoy teoxoy changed the title Shader stage arg [naga-cli] Add input-kind and shader-stage args Apr 18, 2024
@teoxoy teoxoy merged commit e0ac24a into gfx-rs:trunk Apr 18, 2024
25 checks passed
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