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

ShaderModule::single_entry_point() #2301

Merged
merged 4 commits into from
Aug 25, 2023

Conversation

Firestar99
Copy link
Contributor

Almost all of your userbase* will have a single glsl file, containing a single shader, generating a single *.spv file which contains the sole entry point main. And it's loaded like this:

vs::load(device.clone())
            .unwrap()
            .entry_point("main")
            .unwrap();

I'm annoyed at constantly having to write that the entry point is main. I know there is only a single entry point in that file, so just give me that one, without me having to write main every time. This PR adds a function just for that, simplifying the code above to:

vs::load(device.clone())
            .unwrap()
            .single_entry_point()
            .unwrap();

rust-gpu, or why I actually wanted this

Rust-gpu by default compiles all shaders into a single *.spv with varying entry points. However your shader! macro does not like that, so typically users will turn on "multimodule" which generates one *.spv per shader. But the entry point name will still be the rust module path to the shader function, eg. renderer::lodobj::vs, and specifying that is even more annoying. I've solved it by codegening all the shader! macro calls in the build script, where I have access to both the spv file path and entry point name, so I won't benefit from this PR anymore, but others using glsl will probably like this as well.

Testing

The other entry_point() methods are only covered by unit-tests through shader cache tests. And with the shader! macro not being available in tests it seems pretty difficult to automatically test.
So I've just reverted to manually testing the single_entry_point method in the triangle example, but testing the single_entry_point_of_execution may prove difficult as glslc will not generate an spv with multiple entry points from what I can tell.

Checklist

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

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

  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
- Added `ShaderModule::single_entry_point()` which may replace `entry_point("main")` calls in common setups.

@Firestar99 Firestar99 marked this pull request as draft August 23, 2023 17:08
@Firestar99 Firestar99 marked this pull request as ready for review August 23, 2023 17:23
@Firestar99
Copy link
Contributor Author

Firestar99 commented Aug 23, 2023

I wonder if most ShaderModules are just a single entry point anyway, if ShaderModule::entry_point_map: HashMap could be removed and entry points searched linearly though the entry_point_infos vec? Also should it be a smallvec of size 1?

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.

We put self in backquotes everywhere else in the library (and parameters in general).

vulkano/src/shader/mod.rs Outdated Show resolved Hide resolved
vulkano/src/shader/mod.rs Outdated Show resolved Hide resolved
vulkano/src/shader/mod.rs Outdated Show resolved Hide resolved
@marc0246
Copy link
Contributor

marc0246 commented Aug 24, 2023

I wonder if most ShaderModules are just a single entry point anyway, if ShaderModule::entry_point_map: HashMap could be removed and entry points searched linearly though the entry_point_infos vec? Also should it be a smallvec of size 1?

Sure, I don't see a reason to have both fields, even when it you have multiple entry points. The end goal is to use ShaderModule to get your EntryPoints out once and then never again. No point caching anything inside ShaderModule itself. But I don't see the point of the SmallVec since this is the last place to optimize.

@Firestar99
Copy link
Contributor Author

about SmallVec: if I'm already removing the entry_point_map may as well also integrate SmallVec and save a heap allocation, need to change the same code for both anyways.

@marc0246
Copy link
Contributor

LGTM, thanks!

@marc0246 marc0246 merged commit 80023d2 into vulkano-rs:master Aug 25, 2023
3 checks passed
marc0246 added a commit that referenced this pull request Aug 25, 2023
@Firestar99 Firestar99 deleted the shader_single_entry_point branch September 12, 2023 10:45
hakolao pushed a commit to hakolao/vulkano that referenced this pull request Feb 20, 2024
* added ShaderModule::single_entry_point() functions

* single_entry_point() always verifies it's the sole entry point, search optimizations

* single_entry_point() docs adjustments

* ShaderModule removed entry_point_map, instead entry points are searched linearly search though a Vec

---------

Co-authored-by: Firestar99 <[email protected]>
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