Open arcashka opened 1 month ago
This is more a bevy issue than a naga oil issue. I think building the module would work in raw naga oil without ever importing A. But in bevy for asset loading to be complete we need all possible dependents loaded, and your dependent doesn’t load successfully.
The obvious workaround is to put all of A behind the same directive.
Sorry, I don't think I understand :( I wrote a test with this scenario and it fails in naga_oil repository. It doesn't have bevy as a dependency according to Cargo.toml. Or you are saying it behaves as it does because it's required for bevy?
Ah sorry I misunderstood. And you’re right, it looks like we iterate and pull used items from all possible imports (which isn’t necessary in this case since there are none).
it would be a trivial fix to not try and load modules with no used items, but that would break virtual functions (no imported items would appear to be used so the module would be ignored). These might be broken anyway but I’d like to revive them if they are.
So we’d instead need to get preprocessor data with defines context … which is also a bit messy because modules can add shaderdefs themselves, and we can end up in an inconsistent state.
There is maybe a clean fix, but I wonder why you’re in this situation? Cant you remove the module import if it’s not valid, or fix it so it is?
Thank you for fast responses! :)
For my situation it's a little more complicated. My module is valid when correct shader defs are provided. It uses them for binding indices. So in my code I either provide these indices or hide import under ifdef. But hiding fails, and leads to an error
error: expected expression, found '#'
┌─ /home/arcashka/Documents/projects/other/bevy/crates/bevy_core_pipeline/src/tonemapping/tonemapping_shared.wgsl:13:20
│
13 │ @group(0) @binding(#TONEMAPPING_LUT_TEXTURE_BINDING_INDEX) var dt_lut_texture: texture_3d<f32>;
│ ^ expected expression
│
= expected expression, found '#'
There is more information regarding my scenario in the description. And you can also check PR where I encountered this behavior https://github.com/bevyengine/bevy/pull/13262
Also, right now it's not really an issue for me, I managed to bypass it by moving bindings into nested module like this https://github.com/bevyengine/bevy/pull/13262/files#diff-49ec0e7e6216fe88b72aa04b3ef5684978a425f9afbd8f09c5b1c025440c163f I just thought it's better to at least document the issue
Ok that makes sense. Then you could wrap the binding declaration itself in an ifdef on the def for the binding index? I don’t think nesting achieves anything that you can’t do with a flatter structure.
The other alternative is to use a consistent binding across all the bevy use points, like 99 or something. Binding indexes don’t need to be sequential.
You are right, wrapping bindings in ifdef's should work, but it has it's own issues related to some specifics. Right now I feel like nesting is the cleanest solution. Consistent binding is also an option, but I tried to do bindings sequential based on https://discord.com/channels/691052431525675048/743663924229963868/1237084482780270655 :laughing:
imported module is being validated even though it should never be imported
results in
I have a branch with this test here: https://github.com/arcashka/naga_oil/blob/failing_import_test/src/compose/test.rs#L1098
This example is silly looking because I tried to make it as minimal as possible. I found this issue when I used shader defs as an index for tonemapping lut bindings.
Conditional import
in
tonemapping_shared.wgsl
i hadwhich resulted in
when
TONEMAP_IN_SHADER
is undefined