Make it easier to use separate samplers and textures

[AnyOldName3]

Description

Vulkan optionally lets you set up textures and samplers separately, and only combine them in the shader, just like is mandatory in older Direct3D versions (I've not checked D3D12, but I'd assume it's optional just like in Vulkan).

This was already possible with the VSG, but was a pain to set up as there was nothing saying which hoops needed to be jumped through. This adds a couple of convenience constructors to DescriptorImage that do the hoop-jumping automatically.

Fixes nothing on the tracker - it seemed faster to just make the PR

Type of change

Please delete options that are not relevant.

• New feature (non-breaking change which adds functionality)
• 🤷 This change requires a documentation update

How Has This Been Tested?

• I converted the shadow maps in ViewDependentState to use a separate sampler (as soft shadow techniques like PCSS typically require the same texture to be sampled with both a sampler2D to access raw depth values and a sampler2Dshadow to do hardware-PCF depth comparisons. It still worked.

Test Configuration:

• Firmware version:
• Hardware:
• Toolchain:
• SDK:

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 think it's self-documenting, at least in comparison to the surrounding code.
• I have made corresponding changes to the documentation I've not found any documentation for DescriptorImage that goes into enough detail to be affected by this change.
• My changes generate no new warnings There are no warnings enabled with MSVC - I might fix that at some point.
• I have added tests that prove my fix is effective or that my feature works I can't even find tests. Should this line be changed? I've not added anything to vsgExamples yet.
• New and existing unit tests pass locally with my changes N/A - this just adds functions.
• Any dependent changes have been merged and published in downstream modules N/A

[robertosfield]

Couldn't we just change the implementation of the DescriptorImage(ref_ptr sampler, ref_ptr data, uint32_t in_dstBinding, uint32_t in_dstArrayElement, VkDescriptorType in_descriptorType) constructor to allow null for sampler and data and allow the usage case you want?

This is the current code:

DescriptorImage::DescriptorImage(ref_ptr<Sampler> sampler, ref_ptr<Data> data, uint32_t in_dstBinding, uint32_t in_dstArrayElement, VkDescriptorType in_descriptorType) :
    Inherit(in_dstBinding, in_dstArrayElement, in_descriptorType)
{
    if (sampler && data)
    {
        imageInfoList.push_back(ImageInfo::create(sampler, data));
    }
}

[AnyOldName3]

I'd still say doing things as this PR does would be easier as that doesn't deal with the different default arguments for the descriptor type. If you're setting up the descriptor type manually, then I think it's a small step to create the ImageInfo object manually, too, which already makes this use case possible. My priority was making Vulkan easy, not possible.

An alternative way to make it easy would be to deduce the descriptor type based on which pointers are null, but that'd have a runtime cost, potentially make it more of a hassle to implement any future types provided by extensions, and block something that works in pure Vulkan where the same ImageInfo can be reused for a sampler, a texture, or a combined sampler depending on what type you said it had. I'm not sure why you'd want to do that, so I'm not confident saying it should be blocked.

[robertosfield]

It doesn't immediately jump out as a really helpful addition. Do you have any code that illustrates the difference it makes?

One of my concerns is that we already have a lot of different constructors and they hide quite a bit of setup, while this might be convenient it also obscures what is actually happening under the hood. So I am already uneasy with the current state of play.

[AnyOldName3]

With or without this PR, binding a combined image sampler looks like:

    combinedDescriptor = DescriptorImage::create(theSampler, theImage, 2);

    DescriptorSetLayoutBindings descriptorBindings{
        ...
        VkDescriptorSetLayoutBinding{2, VK_DESCRIPTOR_TYPE_COMBINED_IMAGE_SAMPLER, 1, VK_SHADER_STAGE_FRAGMENT_BIT, nullptr},
        ...
    };

    descriptorSetLayout = DescriptorSetLayout::create(descriptorBindings);
    descriptorSet = DescriptorSet::create(descriptorSetLayout, Descriptors{..., combinedDescriptor, ...);

Without this PR, binding a separate sampler and image looks like:

    vsg::ref_ptr<ImageInfo> imageImageInfo = ImageInfo::create(vsg::ref_ptr<Sampler>(), theImage, VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL);
    imageDescriptor = DescriptorImage::create(imageImageInfo, 2, 0, VK_DESCRIPTOR_TYPE_SAMPLED_IMAGE);
    vsg::ref_ptr<ImageInfo> samplerImageInfo = ImageInfo::create(theSampler, vsg::ref_ptr<ImageView>(), VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL);
    samplerDescriptor = DescriptorImage::create(samplerImageInfo, 3, 0, VK_DESCRIPTOR_TYPE_SAMPLER);

    DescriptorSetLayoutBindings descriptorBindings{
        ...
        VkDescriptorSetLayoutBinding{2, VK_DESCRIPTOR_TYPE_SAMPLED_IMAGE, 1, VK_SHADER_STAGE_FRAGMENT_BIT, nullptr},
        VkDescriptorSetLayoutBinding{3, VK_DESCRIPTOR_TYPE_SAMPLER, 1, VK_SHADER_STAGE_FRAGMENT_BIT, nullptr},
        ...
    };

    descriptorSetLayout = DescriptorSetLayout::create(descriptorBindings);
    descriptorSet = DescriptorSet::create(descriptorSetLayout, Descriptors{..., imageDescriptor, samplerDescriptor, ...);

With this PR, binding a separate sampler and image looks like

    imageDescriptor = DescriptorImage::create(theImage, 2);
    samplerDescriptor = DescriptorImage::create(theSampler, 3);

    DescriptorSetLayoutBindings descriptorBindings{
        ...
        VkDescriptorSetLayoutBinding{2, VK_DESCRIPTOR_TYPE_SAMPLED_IMAGE, 1, VK_SHADER_STAGE_FRAGMENT_BIT, nullptr},
        VkDescriptorSetLayoutBinding{3, VK_DESCRIPTOR_TYPE_SAMPLER, 1, VK_SHADER_STAGE_FRAGMENT_BIT, nullptr},
        ...
    };

    descriptorSetLayout = DescriptorSetLayout::create(descriptorBindings);
    descriptorSet = DescriptorSet::create(descriptorSetLayout, Descriptors{..., imageDescriptor, samplerDescriptor, ...);

The result is something that looks a lot closer to when a combined image sampler is used, and just like with a combined image sampler, the boilerplate that's always going to be the same is set up automatically with sensible values. All of this is handled at build time as the type system has all the information needed to set the default values.

Of particular note, with the existing approach, it's not obvious where the necessary parameters need to go without a decent amount of digging, and if anything's wrong, it won't work. Default-constructed ref_ptrs have to be passed in instead of nullptr as the templates involved won't resolve properly otherwise, which adds extra hassle to discovering the right answer. Things don't accept null pointers that look like they could (although as you've mentioned, this could be addressed). People won't get it right the first time, and that's unnecessarily frustrating. I also think it's clearer for people who don't want the default values these new functions set as they give something another implementation can be copied from - you can see you need to specifically pass a null correctly-typed ref_ptr to the ImageInfo constructor because you can see that's what these are doing.

Another way to look at it is that people would be irritated if the combined image sampler DescriptorImage constructor specialisation (and default argument for all the other constructors) was removed. They'd need to copy and paste identical boilerplate to every use site, and as I've said already, the information in the boilerplate is basically implied by the information they're already passing, so it's not useful to say it again.

[robertosfield]

Thanks for the explanation.

Another approach that might work is to have helper functions that create the DescriptorImage with the required settings for the different usage cases. The naming of the helper function could provide users guidance on what type usage of the DescriptorImage it'll provide, and doxygen docs could explain the details.

[robertosfield]

Looking at the Vulkan docs on DescrtorType:

https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/VkDescriptorType.html

I wonder if convenience functions that make the type of descriptor clearer i.e.

createSamplerDescriptor(...)
createCombinedSamplerImageDescriptor(..)
createSampedImageDescriptor(..)

etc.

[AnyOldName3]

Yeah, that sounds like a reasonable option.

[AnyOldName3]

Okay, how's that look?

[robertosfield]

I think this is probably more helpful for guiding users. Only thing missing is doxygen comments ;-)

I'll go ahead and merge.