leaks dependency on include_absolute_path, proposal for new WgslShaderSourceType
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.
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?
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?
@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");
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 dependenciesload_naga_module_from_path()- loads modules with relative pathscreate_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.