Swoorup / wgsl-bindgen

Generate typesafe Rust bindings for wgsl shaders in wgpu
https://crates.io/crates/wgsl_bindgen
MIT License
33 stars 6 forks source link

leaks dependency on include_absolute_path, proposal for new WgslShaderSourceType #45

Open EriKWDev opened 3 weeks ago

EriKWDev commented 3 weeks ago

Thanks for this awesome tool!

when used with option WgslShaderSourceType::UseComposerWithPath not only naga_oil is required, but also include_absolute_path

Not only was this unexpected as it is undocumented, but its also extra annoying since the crate happens to require a nightly rust toolchain.

While having the absolute correct path is ofc nice for that version, I think it would be better if the emitted path was instead relative and it was up to the user to provide the correct base path.

I propose introducing WgslShaderSourceType::UseComposerWithRelativePath which could then simply output

pub const SHADER_ENTRY_PATH: &str = "main.wgsl";
pub const COMMON_PATH: &str = "common.wgsl";

instead of

pub const SHADER_ENTRY_PATH: &str = include_absolute_path::include_absolute_path!("main.wgsl");
pub const COMMON_PATH: &str = include_absolute_path::include_absolute_path!("common.wgsl");

Given that you are also the author of the include_absolute_path, I see there could be a bias towards having it in this library when it might not be necessary.

Requiring a nightly toolchain for such a trivial thing is a high ask on the user for me at least.

EriKWDev commented 3 weeks ago

I'm a little bit sceptical about the generated function even forcing the use of std::fs::read_to_string. In the game engine at our company at least we would rather be completely in charge of all IO tasks. Could the generated functions perhaps just take a source: &str as input?

    pub fn load_naga_module_from_path(
        composer: &mut naga_oil::compose::Composer,
        shader_defs: std::collections::HashMap<String, naga_oil::compose::ShaderDefValue>,
    ) -> Result<wgpu::naga::Module, naga_oil::compose::ComposerError> {
        composer.make_naga_module(naga_oil::compose::NagaModuleDescriptor {
            source: &std::fs::read_to_string(SHADER_ENTRY_PATH).unwrap(),
            file_path: "main.wgsl",
            shader_defs,
            ..Default::default()
        })
    }

Thoughts?

Swoorup commented 2 weeks ago

Sounds fair. At the time, I only needed this feature to hot reload the shaders for development. As my final build just uses the embedded mode.

We could probably just split the load_naga_module_from_path function, and have it passed the source string as you mentioned. And &std::fs::read_to_string(SHADER_ENTRY_PATH).unwrap() could be done higher up. And then have a option to load from a directory which reuses this path?