andy-thomason / Vookoo

A set of utilities for taking the pain out of Vulkan in header only modern C++
MIT License
524 stars 52 forks source link

Shader specialization #35

Closed FunMiles closed 4 years ago

FunMiles commented 4 years ago

This PR adds shader constant specialization. Other argument syntax opinions are welcome. Also added a mechanism to set and get the color used to clear the window.

lhog commented 4 years ago

This should probably be improved to support spec. const types other than uint32_t

FunMiles commented 4 years ago

I agree. I think the first step would be to design a more general API than what I created. The implementation is definitely not difficult. Looking at the 1.1.158 Vulkan specs, I wonder if creating a structure containing the data, then using a variadic list of (id, structure member pointer) could be a good way to go about it? The only thing I don't like is that the official C++ standard still does not make getting the relative offset of a member in a structure a very pretty thing to get. Though the most trivial approach to get it does work, we are constantly reminded that the standard does not guarantee it... And I am not a fan of using the official macro, especially because you can't apply a macro to variables that have been passed down further.

lhog commented 4 years ago

Feel free to send over your version. I've only managed to come up with this as an interface: https://godbolt.org/z/YMoMxT

FunMiles commented 4 years ago

After re-reading the current implementation I made, I think I have an idea how to achieve the desired result. Several syntactic variations are possible and I'll try to achieve both clarity and compactness.

If I'm reading correctly, only scalar values can be specialized. The official list of basic GLSL types is bool, int, uint, float and double.

I can support the following syntax:

  pipelineMaker.shader(vk::ShaderStageFlagBits::eVertex, final_vert, { 
        { 0, 3u }, // constant_id 0, uint value 3
        { 1, 5   }, // constant_id 1, int value 5
        { 2, 3.14f }, // constant_id 2, float value 3.14
        { 4, true },   // constant_id 4, bool value true
        { 3, 2.7 } // constant_id 4, double value 2.7
     });

And for those who fear (I think when there's only one constant in particular, it is a bit confusing. to see the { { 0, 3u } }) the following should work as well:

  pipelineMaker.shader(vk::ShaderStageFlagBits::eVertex, final_vert, { 
        SpecConst{ 0, 3u }, // constant_id 0, uint value 3
        SpecConst{ 1, 5   }, // constant_id 1, int value 5
        SpecConst{ 2, 3.14f }, // constant_id 2, float value 3.14
        SpecConst{ 4, true },   // constant_id 4, bool value true
        SpecConst{ 3, 2.7 } // constant_id 4, double value 2.7
     });

Note that I used SpecEntry before and think renaming it SpecConst is probably a good thing.

Finally an easy extension would be to allow to create the specialization list to become a variable initialized before, like so:

    SpecList constantSpecializations{
        SpecConst{ 0, 3u }, // constant_id 0, uint value 3
        SpecConst{ 1, 5   }, // constant_id 1, int value 5
        SpecConst{ 2, 3.14f }, // constant_id 2, float value 3.14
        SpecConst{ 4, true },   // constant_id 4, bool value true
        SpecConst{ 3, 2.7 } // constant_id 4, double value 2.7
    };
   pipelineMaker.shader(vk::ShaderStageFlagBits::eVertex, final_vert, constantSpecializations);

That's probably better for those who like or need a very clear explicit forms of writing. Any comments on this before I complete my implementation?

lhog commented 4 years ago

Looks neat!

As for the types, recent additions to vulkan 1.2 introduced more types. I think at the very least int8, int16, float16 were documented.

Whether to support them is a different question. I think int8/int16 can be trivially supported, while float16 has no(?) standard representation in c++, thus unlikely to be seen commonly.

FunMiles commented 4 years ago

Support for both syntaxes ended up to be a bit more painful than I had imagined. The reason being that the object being created holds pointers to data held in vector and unique_ptr., so making it copy constructible is a bit of a pain. At the end, I decided to leave the SpecData as it was and instead not making a SpecList object, but letting the user create a std::vector<SpecConst>. I prefer that idea anyways as there's really no real reason to create a new object type.

FunMiles commented 4 years ago

I created a new pull request #37 with the more general case.