wgsl-bindgen icon indicating copy to clipboard operation
wgsl-bindgen copied to clipboard

leaks dependency on include_absolute_path, proposal for new WgslShaderSourceType

Open EriKWDev opened this issue 1 year ago • 2 comments

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 avatar Aug 27 '24 11:08 EriKWDev

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?

EriKWDev avatar Aug 27 '24 11:08 EriKWDev

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?

Swoorup avatar Aug 30 '24 18:08 Swoorup

@EriKWDev I am in the process of adding an option, ComposerWithRelativePath https://github.com/Swoorup/wgsl-bindgen/pull/73 , which will allow you to load shaders at runtime given a directory and an shader entry point enum variant.

It generates this code

which will allow you to pass a custom loader (in example below: |path| std::fs::read_to_string(path)) which will interpret the concatenated string path to shader and also dynamically load all dependencies reusing the loader and add them to the composer before creating the shader.

Let me know if this suffice your usecase.

    let shader = shader_bindings::fullscreen_effects::create_shader_module_relative_path(
      device,
      crate::SHADER_DIR,
      shader_bindings::ShaderEntry::FullscreenEffects,
      std::collections::HashMap::new(),
      |path| std::fs::read_to_string(path),
    )
    .expect("Failed to create shader module");

sytherax avatar Jul 01 '25 17:07 sytherax

Hi @EriKWDev,

Your suggestion has been implemented in wgsl_bindgen v0.20.0: https://crates.io/crates/wgsl_bindgen/0.20.0

The new ComposerWithRelativePath functionality removes the dependency on include_absolute_path and works with stable Rust. Generated code now outputs relative paths and includes helper functions for dynamic loading:

  • visit_shader_files() - traverses shader dependencies
  • load_naga_module_from_path() - loads modules with relative paths
  • create_shader_module_relative_path() - runtime shader module creation

Generated paths are now simple:

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

Please try v0.20.0 and let me know if this solution works for you.

sytherax avatar Jul 03 '25 04:07 sytherax