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

Overlap with my rust-gpu-cli project? #12

Open
tombh opened this issue Nov 12, 2024 · 19 comments
Open

Overlap with my rust-gpu-cli project? #12

tombh opened this issue Nov 12, 2024 · 19 comments

Comments

@tombh
Copy link
Contributor

tombh commented Nov 12, 2024

For the last couple of Rust GPU projects I've been building my shaders using a standalone compiler, https://github.com/tombh/rust-gpu-cli, which is based on Bevy's https://github.com/Bevy-Rust-GPU/rust-gpu-builder.

My own reason for this is that I found it hard maintaining both my own application code and the shader code in the same repo. I think mostly because of rust-gpu's requirement for a specific rust-toolchain.toml.

I suspect cargo gpu will also solve this problem? In which case I'd like to contribute and focus my efforts here instead 🤓

So some reflections as both a rust-gpu and rust-gpu CLI frontend user:

  • Should rust-gpu always support both ways of compiling shaders? Therefore auto-magically in a monorepo and with a standalone CLI frontend? Personally I'm currently leaning towards only supporting the latter. There are so many unconventional moving parts in compiling Rust shaders that I think hiding too much of the magic can lead to more harm than good.
  • I see that cargo gpu will be published to crates.io. I wonder if it can be taken one step further and pre-compiled to static, cross-platform binaries? Possibly that's what you were discussing in Investigate rustup component for cargo-gpu #2? I've actually already had success with this on Github Actions: see https://github.com/tombh/rust-gpu-cli/pull/1/files It compiles librustc_codegen_spirv.{so,dylib,dll}. And then the actual final rust-gpu-cli app dynamically sets the LD_LIBRARY_PATH (or DYLD_FALLBACK_LIBRARY_PATH etc depending on the OS). The only problem I faced was that rustup can't cross-compile rustc-dev so I haven't been able to get aarch64 binaries for Linux compiling yet.
  • Related to the previous point: is rust-toolchain.toml needed for compiling librustc_codegen_spirv.so or for compiling shaders or for both? Because if it's just for compiling librustc_codegen_spirv.so then does that mean that pre-compiled versions of librustc_codegen_spirv.so would avoid the need for cargo gpu to install its own rustup toolchains?
  • Also related to the above, am I understanding correctly that this project installs (or plans to install) Rust toolchains under the hood? I think it'd be good to mention that in the README. Especially if it installs to ~/.rustup/toolchains. Also I think it'd be good to have the CLI be verbose by default about what it's doing. Even the simple fact that cargo gpu build is itself calling cargo build/rustc shouldn't be assumed knowledge. It took me a long time to arrive at even the basic understanding I currently have about how rust-gpu works. So as much as a turnkey solution is a great idea, in my experience hiding all the inner workings wasn't so useful.
  • It's great to have bagged the cargo gpu namespace, but my first thought was that the gpu namespace could refer to a lot more than just compiling shaders. I wonder if something like cargo rust-gpu (whilst redundantly mentioning "rust") might be more appropriate?
@tombh
Copy link
Contributor Author

tombh commented Nov 12, 2024

Oh and some more ideas...

@tombh
Copy link
Contributor Author

tombh commented Nov 12, 2024

@LegNeato
Copy link

Awesome, I hadn't seen your project (but now I see I have it starred so I must have at some point). We should definitely join forces 🚀

FWIW my plan for cargo gpu (and really even the rust-gpu GitHub org) is more than just the rust-gpu project. In fact, I've argued we should change the project name in order to make the distinction a lot clearer (and keep rust-gpu as the umbrella org / big tent). For example, if/when rust-cuda is rebooted it should natively slot into cargo gpu as well

@LegNeato
Copy link

(as an aside I've been trying to get a hold of @Shfty for a while with no luck)

@schell
Copy link
Collaborator

schell commented Nov 13, 2024

Should rust-gpu always support both ways of compiling shaders?

I think this will always be the case. Likely because wrangling the cargo invocation to build shaders with the rustc_codegen_spirv backend is so unwieldy, it makes a lot of sense to wrap it with a library. That library is spirv-builder, and I think you'll always be able to depend on that in your project, if you like. There's been a little bit of discussion about cutting it up into pieces - but I imagine you could always have your project depend on whatever library those pieces end up in.

I tend to prefer the latter case you mentioned, where a standalone cli program compiles the shaders. This is mostly because I don't want to pin my project to an older nightly and also because the compile times can be a bit long.

@schell
Copy link
Collaborator

schell commented Nov 13, 2024

I see that cargo gpu will be published to crates.io. I wonder if it can be taken one step further and pre-compiled to static, cross-platform binaries?

We have been talking a bit about that. I'm for it, but it's a lot more effort than I have at the moment.

It's a good idea - I see that building rustc_codegen_spirv from scratch on my machine (an M1 Macbook Pro) takes ~1.5 minutes. That's not all that long, but it's definitely not "snappy".

I do feel that it's a nice-to-have but compiling locally is the 20/80 IMO. (The 20% effort that gets you 80% value).

@schell
Copy link
Collaborator

schell commented Nov 13, 2024

Related to the previous point: is rust-toolchain.toml needed for compiling librustc_codegen_spirv.so or for compiling shaders or for both?

Unfortunately the nightly toolchain is needed for compiling both. It all has to align. sigh

The good news is that with cargo-gpu, the shader crates themselves don't have to depend on a nightly, or spirv-builder.

@schell
Copy link
Collaborator

schell commented Nov 13, 2024

Also related to the above, am I understanding correctly that this project installs (or plans to install) Rust toolchains under the hood?

Yes, it installs toolchains and components.

I agree with you that this should probably be more explicit since these are pretty big downloads. Maybe we should take some confirmation from the user by default?

@schell
Copy link
Collaborator

schell commented Nov 13, 2024

Adding all the SpirvBuilder::new() builder pattern arguments as CLI arguments?

Yeah, that's probably a good thing to have. Good call.

@schell
Copy link
Collaborator

schell commented Nov 13, 2024

Running as a daemon using rust-gpu's builder.watch()

I know cargo watch is unmaintained, but that's what I use for this case, eg cargo watch -x 'gpu build ...'

@schell
Copy link
Collaborator

schell commented Nov 13, 2024

And how could I forget, @schell's very own optional naga validation code

Haha! 🙇

Well I did have this in cargo-gpu originally, but took it out because I didn't want to assume that anyone would want to use cargo-gpu this way. I'm also a bit wary of bloat, as well as the maintenance burden.

Also, it seemed more correct to have cargo-gpu generate a manifest containing all the shader endpoint names and paths, that way downstream projects could use this in their build scripts for this kind of tooling. I do this for my project renderling here. That script translates the .spv files into .wgsl files, validates them and then creates linkage/bindings as .rs files that are included in the main project. It works pretty well, I think.

@schell
Copy link
Collaborator

schell commented Nov 13, 2024

Lastly but not leastly - I'm looking forward to your contributions, @tombh ! Thank you!

@tombh
Copy link
Contributor Author

tombh commented Nov 16, 2024

  • @LegNeato That makes more sense then that there are bigger plans for the cargo gpu namespace 😏 And hopefully that bevy-rust-gpu dev responds soon!

  • I tend to prefer the latter case you mentioned, where a standalone cli program compiles the shaders.

    @schell Go Team CLI haha. This leads on to a whole other topic: documentation. Maybe I can start a thread on Discussions, but what do you think about documenting cargo gpu build as the recommended method for compiling shaders?

  • I see that building rustc_codegen_spirv from scratch on my machine (an M1 Macbook Pro) takes ~1.5 minutes.

    Yeah, I don't mind that first-time compile delay so much, would be great to avoid it, but that'd be just a nice-to-have.

  • The good news is that with cargo-gpu, the shader crates themselves don't have to depend on a nightly, or spirv-builder.

    As in compiling shaders for CPU doesn't require nightly?

  • I know cargo watch is unmaintained, but that's what I use for this case, eg cargo watch -x 'gpu build ...'

    What do you think about removing SpirvBuilder::watch() then? I see Bacon is the new recommended watcher, maybe we suggest that in the docs?

  • Well I did have this in cargo-gpu originally, but took it out because I didn't want to assume that anyone would want to use cargo-gpu this way. I'm also a bit wary of bloat, as well as the maintenance burden.

    The main point for me is the relationship rust-gpu has with wgpu and therefore naga. I know it's not the only way of consuming the final SPIRV shaders (ie ash, etc), but wgpu is the de facto Rust library for talking to graphics cards. And it seems to me that naga is somewhat lagging behind in its SPIRV support. There is of course wgpu::Features::SPIRV_SHADER_PASSTHROUGH but that's presented as a workaround and hopefully something that will one day no longer be needed, once naga catches up. As things stand at he moment I actually think it'd be best for rust-gpu to recommend SPIRV_SHADER_PASSTHROUGH as the default, I've just had too many problems with naga's SPIRV support. But that seems a shame, that it dissuades users for reporting naga shortcomings, and undermines best practices around validation. In conclusion I think what I'm trying to say is that it'd be great if there was a more explicit story around validation in rust-gpu. It's fine to have rough edges, just as long as the current state of affairs is acknowledged and best practices and workarounds are well supported and documented.

  • I do this for my project renderling here. That script translates the .spv files into .wgsl files, validates them and then creates linkage/bindings as .rs files that are included in the main project. It works pretty well, I think.

    Woah, that's awesome! But you don't feel that'd be appropriate to have that in rust-gpu somewhere? It wouldn't be possible to generate these .rs linkers in a generic way for say wgpu and ash?

@schell
Copy link
Collaborator

schell commented Nov 16, 2024

what do you think about documenting cargo gpu build as the recommended method for compiling shaders?

I'm in favor of that! If you're on the happy path then it's definitely the shortest path to compiling Rust shaders.

As in compiling shaders for CPU doesn't require nightly?

Yes, exactly.

What do you think about removing SpirvBuilder::watch() then? I see Bacon is the new recommended watcher, maybe we suggest that in the docs?

I'm in favor of this, but that module is behind a feature, so we're not paying for it in any way besides maintenance - and probably not even that, I wouldn't be surprised if that module has bit rotted.

Lastly about the relationship between rust-gpu and wgpu. It's true that the SPIR-V frontend in naga is lacking. That's why I've spent a good amount of my free time improving it. I hope to keep doing this. I personally think wgpu should be the default runtime for rust-gpu shaders because it's the only one that works on the web. But I'm wary of making this decision for everyone. For example I don't like the idea of translating all shaders to WGSL automatically and validating them with a specific version of naga because that ties shader compilation to naga, which as we both mentioned earlier the SPIR-V -> naga path has some shortcomings and at some point that pipeline will have problems that prevent WGSL shaders from being validated.

I guess I'm just wary of putting wgpu in the critical path of compiling shaders. That said, I'm all for using wgpu by default in situations that are opt-in or opt-out. For example, I think it would make sense to use @LegNeato 's cargo gpu new subcommand to generate a new project that includes winit and wgpu, and uses a build.rs to validate shaders and generate bindings. This way people can get a project up and running immediately and see a glorious triangle within minutes.

To that end I think it would be great to expose cargo-gpu as a library on crates.io to help create build.rs files.

@LegNeato
Copy link

LegNeato commented Nov 17, 2024

This reminds me, I've been thinking we need a sort of "life before (GPU) main" type API to really make rust-gpu usage ergonomic without tying to a specific host framework. For example, if I want to make a shader have different behavior for different dispatch sizes or do different things on different hardware, there is no standardized way. Heck, even just logging has to be bespoke.

I agree that we don't want to tie to a particular host library or framework, but not having something that is "host aware" means we can't build reusable higher-level abstractions because many of them require the host side to cooperate.

@tombh
Copy link
Contributor Author

tombh commented Nov 27, 2024

I'm in favor of this, but that module (SpirvBuilder::watch()) is behind a feature, so we're not paying for it in any way besides maintenance - and probably not even that, I wouldn't be surprised if that module has bit rotted.

It definitely works, I'm using it in my rust-gpu-cli. Though I don't mind using Bacon either. I'm just nudging to keep things clean, it's always nice to delete code 😏

Lastly about the relationship between rust-gpu and wgpu ...

I totally agree with everything you mention. And what do you think about advertising SPIRV_SHADER_PASSTHROUGH as the default in the docs? That strikes me as the most important thing at the moment. wgpu is definitely the go-to library, but naga just isn't ready for rust-gpu's SPIRV.

That mysterious naga error is a good case in point, it didn't even give a reason. We did finally get to the bottom of it, but it took a while and there's no easy fix. Not to complain at all about the wgpu team of course. I'm just thinking about the importance of SPIRV_SHADER_PASSTHROUGH.

@schell
Copy link
Collaborator

schell commented Nov 27, 2024

These are all great points. The only downside I see to advertising SPIRV_SHADER_PASSTHROUGH as the default is that it is only supported if wgpu's selected backend is Vulkan.

So I guess the question is - how many folks are targeting Vulkan through wgpu, and is that a critical mass of people, or is it more common that folks are targeting Vulkan directly with a library like vulkano or ash?

@tombh
Copy link
Contributor Author

tombh commented Nov 27, 2024

Ahhh yes Vulkan, I forgot that.

how many folks are targeting Vulkan through wgpu

That is indeed a critical question. I'm afraid I couldn't even guess at an answer. Though I'd venture to answer a similar question but asked from the opposite angle: how many developers of rust-gpu have historically considered targets other than Vulkan? I get the feeling not so much. For example I understand that https://github.com/EmbarkStudios/kajiya was the Golden Child for rust-gpu and that's a Vulkan-only project. I feel like supporting drivers other than Vulkan has been a bonus rather than a priority.

And I must say there's absolutely nothing wrong with that. The only thing I'd like to see is that the documentation is more aligned with rust-gpu's status. So not saying that OpenGL and Metal aren't supported, just that Vulkan is currently the happy path.

And of course I'm sure we'll reach parity with OpenGL and Metal soon!.

@schell
Copy link
Collaborator

schell commented Nov 28, 2024

Yes, agreed :)

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

No branches or pull requests

3 participants