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

leaks dependency on include_absolute_path, proposal for new WgslShaderSourceType #45

Open
EriKWDev opened this issue Aug 27, 2024 · 2 comments

Comments

@EriKWDev
Copy link

EriKWDev commented Aug 27, 2024

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
Copy link
Author

EriKWDev commented Aug 27, 2024

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
Copy link
Owner

Swoorup commented Aug 30, 2024

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?

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

2 participants