renamed "Uniform" to "DescriptorBuffer"

[theodoregoetz]

Using the name "DescriptorBuffer" instead of "Uniform" with ShaderSet and GraphicsPipelineConfigurator better conveys that these methods work with both uniform and storage (and sometimes even image/texture) bindings.

Part of #954

This is a rename only and no functionality was changed. The public member variable ShaderSet::uniformBindings was renamed to ShaderSet::bufferBindings and a reference was created to keep the the API from breaking. Also, updated names in the read/write methods are set to be used when the version hits 1.0.10.

• This change requires a documentation update

Testing

See the associated storage buffer example: https://github.com/vsg-dev/vsgExamples/tree/master/examples/utils/vsgstoragebuffer

Checklist:

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• Any dependent changes have been merged and published in downstream modules

[theodoregoetz]

Looking at the documentation (vsg-dev/vsgTutorial), I don't think anything there needs to change.

[robertosfield]

Thanks for the changes. I have done a first pass review and it all looks consistent with what I had in mind with a renaming.

One thing that has just struct me is the changes introduce a new naming conflict. In Vulkan and the VSG we have the vk/vsg::Buffer which is used for a far wider usage than just DescriptorBuffer, so we are solving one naming issue but introducing a new one that could cause further confusion.

I will need to reflect on this overlap and doing further passes over the changes, happy to take suggestions.

[robertosfield]

After a second read through I'm wondering if DescriptorBinding would be better than UniformBinding/BufferBinding. This would also fit with the DescriptorConfigurator naming, and the DescriptorBuffer/DescrptorImage naming.

Thoughts?

[theodoregoetz]

Agreed. I'll rework this for "Uniform" -> "Descriptor".

[robertosfield]

Thanks.. I am sorry that using Descriptor as the base name didn't occur to me in discussions, alas sometimes only when you see something implemented does how coherent it is come to fore.

[theodoregoetz]

After a closer look, I figured that the Buffer term was good to have in GraphicsPipelineConfigurator because it differentiates the descriptor from the images (textures) and vertex attributes (arrays). But for sure the ShaderSet's methods were better using the term "descriptor" because it holds onto textures, uniforms and storage buffers. I also fixed up the read/write to be backwards compatible and triggered on version 1.0.10 which doesn't exist yet - I'm not really sure how to want to handle that.

Additionally, I started to use the [[deprecated]] C++ attribute and found that the errors I get from gcc are quite nice. Let me know if this causes any problems.

[robertosfield]

Shouldn't this be enableDescriptor()?

W.r.t [[deprecated]], do you know whether doxygen can handle it in a sensible way?

[theodoregoetz]

no. GraphicsPipelineConfigurator::enableBuffer() is still good in my opinion. see my previous comment posted at the same time as yours just now.

[robertosfield]

I think enableBuffer() is more confusing that the original enableUniform(). Buffer has a distinct mean in VSG and Vulkan, it refers to a whole buffer not a subset that is can be as a uniform and storage.

enableDescriptor() is more generic than enableTexture()/enableUniform() but it does at least still map to DescriptorBuffer/DescriptorImage. If we want to replace enableUniform()/enableTexture() then it would probably be best as enableDescriptorBuffer()/enableDescriptorImage().

[theodoregoetz]

Right, I came to nearly the same conclusion but decided at the time that shorter method names would be better. Note that the word "descriptor" shows up twice in DescriptorConfigurator::enableDescriptorBuffer() so why not just make it DescriptorConfigurator::enableBuffer()?

But really I could have gone either way. DescriptorConfigurator::enableDescriptorBuffer() is not too bad in length.

[theodoregoetz]

Also, I want to keep this PR to the uniform/storage issue and follow-up with a rename for the texture/image methods right after.

[robertosfield]

Perhaps just DescriptorConfigurator::enable(..) then?

Though, as we'll need to keep it consistent with the GraphicsPipelineConfigurator API.

[theodoregoetz]

If you can stand the slight inconsistency for this PR, I'd like to make this change first, keeping it as is at the moment (enableDescriptorBuffer(), etc) and come back with another PR to rename enableTexture() to enableDescriptorImage(). This is because the down-stream changes are many and separating this out keeps it all straight in my head.

[robertosfield]

Is the warning redundant if we are using the [[deprecated]] compiler hint?

[theodoregoetz]

Possibly. I don't know how this attribute and how VSG's logging plays with release and debug. I figured you might have some opinion on which is best. I can take this out if you think it best.

[robertosfield]

Once this PR is settled a bit further I'll merge this PR as a branch and then do testing with it.

[theodoregoetz]

for deprecations, I believe it best to be obnoxious about it. Otherwise it's easier to ignore.

[theodoregoetz]

The last commit renames the configurator methods to ...DescriptorBuffer(). I also added /// deprecated: comments to account for doxygen which seems to ignore the [[deprecated]] attribute in my (lmited) tests.

[robertosfield]

FYI, I've checked in some work that includes a bump of the VSG master version to 1.0.10-rc1 so the IO checks in this PR should now work with VSG master.

[theodoregoetz]

I believe this PR is complete now, unless @robertosfield you really want to have the rename from Texture to DescriptorImage done at the exact same time (keep in mind that this would more than double the down-stream updates)

[robertosfield]

I have done another review and want to make a few changes so will merge this PR as a theodoregoetz-buffer-binding branch. I need to decide upon the naming and want to simplify the deprecated entries - I think one is enough, we certainly don't need [[deprecated]], doxygen comment and console message all saying the same thing.

The branch is:
https://github.com/vsg-dev/VulkanSceneGraph/tree/theodoregoetz-buffer-binding

[robertosfield]

I have decided to create a parallel DescriptorBinding branch so I can incrementally cherry-pick changes from the theodoregoetz-buffer-binding branch:

https://github.com/vsg-dev/VulkanSceneGraph/tree/DescriptorBinding

So far I have just copied across the changes to alignment and DescriptorBuffer doxygen comment as these don't affect the API and fix a bug. I'll merge these changes with VSG master right away as they uncontroversial. I'll then copy across the changes that change the API.

[robertosfield]

I now think I have got to the new DescriptorBinding branch parity with this PR and the theodoregoetz-buffer-binding branch, these are the changes so far:

https://github.com/vsg-dev/VulkanSceneGraph/compare/DescriptorBinding

I have updated vsgXchange, vsgPoints and vsgExamples to not use the deprecated methods:

https://github.com/vsg-dev/vsgXchange/compare/DescriporBinding
https://github.com/vsg-dev/vsgPoints/compare/DescriptorBindings
https://github.com/vsg-dev/vsgPoints/compare/DescriptorBindings

The main differences to this PR is that I have only used the [[deprecated]] compile hint and have not assign any doxygen comments or vsg::warn(..) messages, and the enable/assignUniform(..) methods have been replaced by enable/assignDescriptor(..) methods rather than enable/assignDescriptorBuffer(..).

I decided just using enable/assignDescriptor rather than enable/assignDescriptorBuffer as I feel that it fits better with the enable/assignArray(..) naming, and offers the path of just overloading the assignDescriptor(..) methods to have an optional ref_ptr which can then replace the enable/assignTexture(..) method with a standard enableDescriptor/assignDescriptor() naming.

I think the DescriptorBinding branch is ready to merge with master, but will reflect on the possible change of enable/assignTexture(..) before I do anything further. Happy to take suggestions.

[theodoregoetz]

Sounds good to me except I don't quite understand how you will handle textures with just assignDescriptor - uniform/storage takes a BufferInfoList and the texture takes an ImageInfoList.

[robertosfield]

I have had a look at unifying the enableTexture/assignTexture to use the enableDescriptor/assignDescriptor() usage but to make it work well we'll need to check the vkDescriptorType and use that to choose with a DescriptorImage, DescriptorBuffer or DescriptorTexelBufferView should be used.

The vsg::DescriptorTexelBufferView isn't yet supported by ShaderSet/DescriptorConfigurator/GraphicsPipelineConfigurator so this would be best tackled as separate lock of work. I am lots of other work to get on with so I won't go pursue another avenue of unplanned work. Resolving the enableTexture/assignTexture() is a coherent way will also take more time so I'll now merged the respective DescriptorBinding branches with VSG etc. master and leave these matters to future work.

[robertosfield]

Sounds good to me except I don't quite understand how you will handle textures with just assignDescriptor - uniform/storage takes a BufferInfoList and the texture takes an ImageInfoList.

Oh, that's simple - you just have overload assignDescriptor(..) methods. That's the easy part, the harder part is implementing a generate enableDescirptor() that handles all the different Descriptor types.