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

Add root_path_env property to shaders macro #2180

Merged
merged 4 commits into from Apr 11, 2023

Conversation

Firestar99
Copy link
Contributor

@Firestar99 Firestar99 commented Apr 10, 2023

In order to load a shader generated by the build script one would need to specify the path from the Cargo.toml into the target directory, but one may not even now where exactly the target directory is located. It may be ./target on a single crate, ../target or ../../target in a workspace environment or even in a totally different folder if the crate is used as a library from some other cargo project. As src/bytes are required to be string symbols one cannot specify them to concat!(env!("OUT_DIR"), "path.spv"), so with this PR I introduce the root_path_env property to the shader! macro.

//! ## `root_path_env: "..."`
//!
//! Instead of searching relative to your `Cargo.toml`, search relative to some other folder
//! specified by this env variable. The intended use case is using `OUT_DIR` to be able to load
//! shaders generated by your build script. Defaults to `CARGO_MANIFEST_DIR` corresponding to the
//! folder of your `Cargo.toml`.

For testing purposes I implemented this into my rust-gpu integration example project in the branch root_path_env_pr now in master:
https://github.com/Firestar99/rust-gpu-vulkano-example/

  1. Update documentation to reflect any user-facing changes - in this repository.

  2. Make sure that the changes are covered by unit-tests.

  3. Run cargo fmt on the changes.

  4. Please put changelog entries in the description of this Pull Request
    if knowledge of this change could be valuable to users. No need to put the
    entries to the changelog directly, they will be transferred to the changelog
    file by maintainers right after the Pull Request merge.

    Please remove any items from the template below that are not applicable.

  5. Describe in common words what is the purpose of this change, related
    Github Issues, and highlight important implementation aspects.

Changelog:

### Additions
- add `root_path_env` property to allow loading shaders generated by a build script

@Firestar99
Copy link
Contributor Author

I'm not exactly sure how to go about adding unit tests. I could of course add a build script that copies an glsl file into the build dir and then use it in a test, but adding that to the shaders crate directly would also cause everyone using that crate to execute that unnecessary build script. I could also add a new crate just for testing this but that also seems a little overkill.
One could add such a "test" if you were to add rust-gpu examples to your repo, but that would also bind your repo to the relatively large rust-gpu stack whenever you'd compile an example.

@Firestar99 Firestar99 marked this pull request as ready for review April 10, 2023 14:12
@marc0246
Copy link
Contributor

I don't think that this can be unit tested. It would need an integration test or a whole system test (like an example, which you mentioned) but that would be total overkill for a change like this I think. There's only so much that can go wrong with a small change like this so I wouldn't worry about it personally.

Copy link
Contributor

@marc0246 marc0246 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the name for the field you ended up with! It's very descriptive. Everything works for me, just a couple changes I would like.

vulkano-shaders/src/lib.rs Outdated Show resolved Hide resolved
vulkano-shaders/src/lib.rs Outdated Show resolved Hide resolved
Co-authored-by: marc0246 <40955683+marc0246@users.noreply.github.com>
@Firestar99
Copy link
Contributor Author

I just applied all your suggestions, don't really have anything to add to them

@Firestar99
Copy link
Contributor Author

Firestar99 commented Apr 11, 2023

Would you deem it valuable to add documentation that one can specify custom rustc env vars in the build script?

build.rs

let out_dir = env::var("OUT_DIR").unwrap();
let shader_out_dir = format!("{out_dir}/../../../../spirv-builder/spirv-unknown-spv1.3/release/deps/example_shader.spvs/");
println!("cargo:rustc-env=SHADER_OUT_DIR={shader_out_dir}");

shader include:

vulkano_shaders::shader! {
	ty: "vertex",
	root_path_env: "SHADER_OUT_DIR",
	bytes: "image_shader-image_vs.spv",
}

from Firestar99/rust-gpu-vulkano-example@2d27af7

@marc0246
Copy link
Contributor

Sure, if you want to add that I think that would be great.

@marc0246
Copy link
Contributor

Awesome, thank you for the work!

@marc0246 marc0246 merged commit 9ba796d into vulkano-rs:master Apr 11, 2023
3 checks passed
marc0246 added a commit that referenced this pull request Apr 11, 2023
hakolao pushed a commit to hakolao/vulkano that referenced this pull request Feb 20, 2024
* shaders: minor fix that some Vecs were always allocated with capacity of 0

* shaders: add root_path_env property to allow loading shaders generated by a build script

* shaders: Apply root_path_env suggestions from code review

Co-authored-by: marc0246 <40955683+marc0246@users.noreply.github.com>

* shaders: added root_path_env docs about env vars specified by a build script

---------

Co-authored-by: Firestar99 <4696087-firestar99@users.noreply.gitlab.com>
Co-authored-by: marc0246 <40955683+marc0246@users.noreply.github.com>
hakolao pushed a commit to hakolao/vulkano that referenced this pull request Feb 20, 2024
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.

None yet

2 participants