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

Allow for setting the array length in a shader using a specialization constant #2345

Merged
merged 1 commit into from
Oct 3, 2023

Conversation

rohdealx
Copy link
Contributor

@rohdealx rohdealx commented Oct 2, 2023

To allow for something like this using vulkano-shaders:

layout(constant_id = 0) const uint n = 64;
layout(std430, set = 0, binding = 0) uniform layout_sample { float sample[n]; };

Fixes #1956

Changelog:

### Addditions
- Vulkano-shaders: support for specialization-constant-sized arrays in structs (they are generated with the size specified as fallback in the specialization constant initializer).

@Rua
Copy link
Contributor

Rua commented Oct 2, 2023

That should be already implemented by #2329. Does it not work for you?

@marc0246
Copy link
Contributor

marc0246 commented Oct 2, 2023

I'm assuming what they meant to say is to allow this usage in vulkano-shaders. In other words, it's attempting to fix #1956. Using the default value of the specialization constant as the length is certainly a solution and one that I considered, but personally I would prefer if the struct wasn't generated at all. It's going to be useless anyway and I bet cause more confusion when it's generated like that. Nevertheless this would be the simplest solution.

@Rua
Copy link
Contributor

Rua commented Oct 2, 2023

What about generating a struct with a const parameter for the length?

@marc0246
Copy link
Contributor

marc0246 commented Oct 2, 2023

That's also a good option. shaderc vomits up array length 1 in places where runtime lengths are illegal without warning. We could do something similar, length 0 ideally.

@marc0246
Copy link
Contributor

marc0246 commented Oct 2, 2023

Wait I misread your question. I thought you meant generating it with a const length. Generating a const parameter won't make it runtime-sized.

@marc0246
Copy link
Contributor

marc0246 commented Oct 2, 2023

Also you would have to propagate the const parameter(s) throughout all uses of the structs.

@rohdealx
Copy link
Contributor Author

rohdealx commented Oct 2, 2023

I'm assuming what they meant to say is to allow this usage in vulkano-shaders. In other words, it's attempting to fix #1956. Using the default value of the specialization constant as the length is certainly a solution and one that I considered, but personally I would prefer if the struct wasn't generated at all. It's going to be useless anyway and I bet cause more confusion when it's generated like that. Nevertheless this would be the simplest solution.

Yes, sorry i did not check if this issue existed already. If you would rather not generate a struct at all you can just close this pull request.

@marc0246
Copy link
Contributor

marc0246 commented Oct 3, 2023

I think it would be best to use 0 as the array length. Hopefully it won't cause confusion, as SPIR-V doesn't allow it, so it fits nicely as a kind of sentinel value. The issue with the other options is that it would require a lot of logic because things need to be propagated through structs that use the problematic structs. Like not generating a struct using spec-constant-sized arrays, we would also have to not generate any structs using those structs, and structs using those... This on the other hand requires little to no effort, also for any kind of spec constant.

This would of course also require changing the Option<NonZeroUsize> to an Option<usize>. Would you be willing to make the change? All that's needed is to add a separate arm for the spec constant and return 0 as the length.

@rohdealx
Copy link
Contributor Author

rohdealx commented Oct 3, 2023

@marc0246
Copy link
Contributor

marc0246 commented Oct 3, 2023

You make a very good point, I had forgotten about that. That would complicate things quite a bit. Knowing this, I think your PR is perfect just the way it is. :D

Thank you for looking into this!

@marc0246 marc0246 merged commit ba4d11a into vulkano-rs:master Oct 3, 2023
3 checks passed
marc0246 added a commit that referenced this pull request Oct 3, 2023
hakolao pushed a commit to hakolao/vulkano that referenced this pull request Feb 20, 2024
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.

Cannot set array length with Specialization Constants
3 participants