[FIXED] VK_LOD_CLAMP_NONE computed mipmap levels

[siystar]

Pull Request Template

Description

Fixed incorrect value returned by vsg::computeNumMipMapLevels when maxLod is set to VK_LOD_CLAMP_NONE.

Type of change

• Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Before:

computeNumMipMapLevels(width=256, maxLod=10) = 9
computeNumMipMapLevels(width=256, maxLod=VK_LOD_CLAMP_NONE) = 1000

After:

computeNumMipMapLevels(width=256, maxLod=10)
 = 9
computeNumMipMapLevels(width=256, maxLod=VK_LOD_CLAMP_NONE) = 9

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

[robertosfield]

I have just done quick review, as I wasn't familiar with VK_LOD_CLAMP_NONE so looked it up:

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

I noticed the use of float so looked up the specs on samplers:

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

The relevant part is:

    float                   minLod;
    float                   maxLod;

The vsg::computeNumMipMapLevels(...) function uses the maxLod value to help guide the number of mipLevels:

uint32_t vsg::computeNumMipMapLevels(const Data* data, const Sampler* sampler)
{
uint32_t mipLevels = sampler != nullptr ? static_cast<uint32_t>(ceil(sampler->maxLod)) : 1;
if (mipLevels == 0)
{
mipLevels = 1;
}

It does so via a cast to uin32_t, and you changes compare this against the float VK_LOD_CLAMP_NONE which may work but it's not something I'd recommend. Might I suggest comparing sampler->maxLod to VK_LOD_CLAMP_NONE would be more reliable.

Given this check would also need an additional sampler not null check it may be better to restructure the function so the logic is all clearer w.r.t how mipLevels is set. I can have a bash at this, but I'm in the middle of other work so can't do this right away.