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

Support include directives on shaderc #233

Merged
merged 7 commits into from
Feb 20, 2023

Conversation

fand
Copy link
Contributor

@fand fand commented Feb 13, 2023

image

Hello 👋 Thanks for great library!!

I was tweaking around notan_macro to see if it's possible to support #include.
Then I found shaderc can handle #include directives by passing an additional option.

So, this PR adds an option passed to shaderc to include shader files in following format:

  • Relative includes #include "foo/bar.glsl"
    • This tries to read "foo/bar.glsl" relative to the input shader file.
  • Standard includes #include <foo/bar.glsl>
    • This tries to read "foo/bar.glsl" under the root directory (CARGO_MANIFEST_DIR or .)

Also I added examples/shaders/draw_shader_include.glsl, an example file to test #include.
You can run it by cargo run --example draw_shader_include --features shaderc.

NOTE: include directive only works on include_fragment_shader! / include_vertex_shader! macros, not on fragment_shader! / vertex_shader! because it's difficult to determine the directory to search for the include files.

@fand fand force-pushed the feature/shaderc-include branch from e510325 to 2ae2edf Compare February 13, 2023 08:45
@Nazariglez
Copy link
Owner

This is great, thank you so much!

@Nazariglez
Copy link
Owner

This is failing because shaderc is disabled on the build system by default, mainly because docs were not building for crates, so I need to check how to enable it only for the build system. Another option is avoid the compilation of the example if shaderc is not enabled. I'll take a look today.

@Nazariglez
Copy link
Owner

@fand is it ok to you if we remove the example? Only the example.

I wasn't able to find a way to exclude the example from the cargo test command, it seems that cargo itself have some issues open related to this. The only options I can see is to enable shaderc on the ci machine, however I am not a big fan of this solution, because it will try to compile it on each build, and is a big dependency. So I am more inclined to just remove the example, because is a niche feature.

What do you think?

@fand
Copy link
Contributor Author

fand commented Feb 20, 2023

Thanks for review!

So I am more inclined to just remove the example, because is a niche feature.

Yeah I totally agree 👍 I removed the example

it seems that cargo itself have some issues open related to this

Just curious, could you give me the link of the issue if you have?

@Nazariglez
Copy link
Owner

This is the issue I found: rust-lang/cargo#7233 they are trying to avoid the compilation using a config flag (I did it with not(feature=shaderc)), however this doesn't work. Using cargo test --workspace --exclude my_example does not work either. That's why I thought about to remove the example.

Sorry for the inconveniences and thank you so much for the PR!

@Nazariglez Nazariglez merged commit ee2f67e into Nazariglez:develop Feb 20, 2023
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

Successfully merging this pull request may close these issues.

2 participants