Storage buffer support for Configurators and ShaderSet

[theodoregoetz]

We would like to create a (large) fragment shader storage buffer along side the GraphicsPipelineConfigurator and ShaderSet classes in the same way we handle uniform buffers. From the interface it looks like only attributes, uniforms and textures are suppported.

We see two possible solutions:

1. Allow the VK_DESCRIPTOR_TYPE_STORAGE_BUFFER descriptorType to propagate through the Configurators in GraphicsPipelineConfigurator.cpp

ShaderSet accepts a storage buffer descriptorType in ShaderSet::addUniformBinding(). To use this in the Configurators, we found that it need only be passed to DescriptorBuffer::create() in the two DescriptorConfigurator::assignUniform() methods found in GraphicsPipelineConfigurator.cpp.

 bool DescriptorConfigurator::assignUniform(
     const std::string& name, 
     ref_ptr<Data> data, 
     uint32_t dstArrayElement)
 {
     if (auto& uniformBinding = shaderSet->getUniformBinding(name))
     {
         assigned.insert(name);
 
         // set up bindings
         if (!uniformBinding.define.empty()) 
             defines.insert(uniformBinding.define);
 
         // create uniform and associated DescriptorSets and binding
         return assignDescriptor(
             uniformBinding.set, 
             uniformBinding.binding, 
             uniformBinding.descriptorType, 
             uniformBinding.descriptorCount, 
             uniformBinding.stageFlags,
             DescriptorBuffer::create(
                 data ? data : uniformBinding.data, 
                 uniformBinding.binding,
                 dstArrayElement,
+                uniformBinding.descriptorType
             )
         );
     }
     return false;
 }

While this does work, it's not ideal since we are using methods like assignUniform() for buffers that are not actually "uniform" type.

2. Add ShaderSet::addStorageBinding() and GraphicsPipelineConfigurator::assignStorage() methods

This involves adding a StorageBinding struct to the ShaderSet class along with several new methods and possibly asserting on the descriptorTypes passed in to ensure that a compatible value is used (i.e. disallow using VK_DESCRIPTOR_TYPE_STORAGE_BUFFER with ShaderSet::addUniformBinding().

Note

We are happy to put together a PR for either (or any) solution the VSG devs think is best. We have a small example program that demonstrates the first solution, but it seems to us that solution 2 may be the preferred route.

[robertosfield]

On first pass I don't have any immediate opinions on what would be best for the VSG/your usage case.
Do you have a link to your example program? This would help provide a better insight in the issue.

As a general comment vsg::ShaderSet/GraphicsPipelineConfigurator is something I expect to evolve over time as we expand the usage cases that we want to use it with. We don't have any other Vulkan scene graphs to take inspiration from, so it's a case of trying to solve real-world application tasks and seeing what works well and what doesn't and figuring out better ways of doing things when things don't work well. So raising red flags for particular usage cases that don't yet work well is exactly what we want.

[theodoregoetz]

@robertosfield with your approval, I'd like to propose and submit a PR that adds ShaderSet::addStorageBinding() and related methods. That is, unless you think VSG could treat storage buffers as a special case of uniform buffers (maintaining the existing interface, but adding the correction to DescriptorConfigurator::assignUniform() as described above.

[robertosfield]

Is the issue partly down to the naming of the ShaderSet and GraphicsPIpelineConfiguration methods and missing descriptorType assignment when setting up of the DescriptorBuffer?

i.e. uniform -> DescriptorBuffer
storage -> DescriptorBuffer
texture -> DescriptorImage

My choice of using uniform and texture naming is partly a hang up of coming from OpenGL/GLSL -> Vulkan.

The VkDescriptorType does overlap a bit, but perhaps it and DescriptorBuffer/DescriptorImage should take precedent?

[robertosfield]

I have fixed the the DescriptorConfigurator desriptorType assignment bug with VSG master commit : 62b2e8e, and have merged as a branch your example posted to vsgExamples, and have written up details on the PR.

[theodoregoetz]

That's great, thank you. Broadly speaking with regard to VSG, I'd vote for landing hard on the vulkan naming scheme with ShaderSet::addBufferBinding(), addImageBinding() and DescriptorConfigurator::assignBuffer(), assignImage() and then having some document to describe the transition to vulkan (this probably already exists somewhere). That way, VSG isn't holding on to legacy terminology that will have to be documented in the future. That said, I can't imagine the term texture going away anytime soon...

[robertosfield]

I agree with adopting Vulkan naming for ShaderSet/DescriptorConfigurator/GraphicsPipelineConfigurator. We can maintain the old Uniform/Texture API for the short term for backwards compatibility. If you feel the itch then feel free to create a PR against the VSG to do this, otherwise I'll get to this at some point in the future once I have cleared my task list.