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

#import "file" without list of names should err/warn #97

Open
kornelski opened this issue Jul 5, 2024 · 13 comments
Open

#import "file" without list of names should err/warn #97

kornelski opened this issue Jul 5, 2024 · 13 comments

Comments

@kornelski
Copy link
Contributor

Import with a file path, but without a list of names, "fails" silently:

#import "common.wgsl"

This syntax is very similar to C's #include that just imports everything, so I did not think of specifying individual type names, and was puzzled why this directive doesn't seem to do anything, and doesn't even cause a compilation error.

@robtfm
Copy link
Collaborator

robtfm commented Jul 5, 2024

It would make sense for this to be an “unused” warning.

It isn’t failing, it’s just not necessary since we added the ability to refer to fully qualified items without importing.

If you don’t import any items you’re importing the module, which means you can use items from it with mod::item syntax. But since you can refer to fully qualified items directly there’s never a need to import top-level modules. It can still be useful to import nested modules (#import a::b and then b::item).
we also don’t currently support import mod::* which is what you want from the sounds of it. That’s also something I’d like to add in future.

@robtfm
Copy link
Collaborator

robtfm commented Jul 5, 2024

Oh, also to note, particularly for custom bevy shaders which are quoted like your “common.wgsl” it’s useful to alias them - #import “common.wgsl” as common and let x = common::func() is nicer than having let x = “common.wgsl”::func() which is pretty clunky.

@kornelski
Copy link
Contributor Author

I didn't even know such syntax existed. The readme doesn't explain the "file" case.

@robtfm
Copy link
Collaborator

robtfm commented Jul 5, 2024

Which bit is not explained? I thought it was all in there.

@kornelski
Copy link
Contributor Author

The readme has examples for #import predefined::name, but zero uses of #import "./path".

https://docs.rs/naga_oil/ has nothing about imports and looks empty.

@robtfm
Copy link
Collaborator

robtfm commented Jul 5, 2024

Ah, right. The treatment/resolution of quoted names is bevy-specific, naga oil itself doesn’t treat quoted names any differently.

@kornelski
Copy link
Contributor Author

Do you think naga should get a first-class support for path names, or should complain about handling of these to Bevy?

@robtfm
Copy link
Collaborator

robtfm commented Aug 6, 2024

i think path names is way out of scope for naga.

could you clarify what it is about bevy's handling you don't like?

@kornelski
Copy link
Contributor Author

kornelski commented Aug 7, 2024

In Bevy, #import "path" is a no-op, which is surprising and unintuitive to me.
The syntax is very similar to C/ObjC preprocessor that imports everything from the file, naga/Bevy it imports nothing at all.

I don't like that there's no warning/error when the import directive is useless. I've wasted a lot of time trying to use includes incorrectly, and was puzzled why nothing is included even though the #import directive doesn't report any errors.

@robtfm
Copy link
Collaborator

robtfm commented Aug 8, 2024

Ok. So as I mentioned we could add a warning, but I don’t personally think it’s necessary.

  • The import syntax is rust-like, and with that lens this should not really be surprising (you’re just importing the module)
  • The read me already notes that “import module::*” is not supported
  • It does “work”, just is unnecessary since fully qualified items can be used directly (again, same as rust)

@robtfm
Copy link
Collaborator

robtfm commented Aug 8, 2024

To be clear, if you (or anyone else) wanted to make a pr to emit a warning, I wouldn’t object, I just don’t plan to do it myself.

@kornelski
Copy link
Contributor Author

Rust-like import module makes sense (and I guess module::item works?), but the version with quotes does not look like Rust, and I would not expect "file"::item to work.

Which is why I'm asking whether this should be changed in naga or Bevy, assuming that "file" syntax is a hack by Bevy.

@robtfm
Copy link
Collaborator

robtfm commented Aug 8, 2024

Import “file”::item does indeed work, as does using “file”::item inline in the shader code.

The quotes are a bevy-specific thing, naga_oil doesn’t interpret quoted names in any special way (though we have made some efforts to allow them to be used).

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