Check for allocated descriptor set (Vulkan backend)

[micb25]

This PR fixes an issue with vkUpdateDescriptorSets() in ImGui_ImplVulkan_AddTexture() in the Vulkan backend. vkUpdateDescriptorSets() gets called in cases for which the previous vkAllocateDescriptorSets() failed (returning VK_NULL_HANDLE in descriptor_set). This leads to the situation that vkUpdateDescriptorSets() gets called with an invalid handle and gets reported by the validation layer:

[    18.02136] [192902] [default:D] vkDebug: Validation Error: [ UNASSIGNED-GeneralParameterError-RequiredHandle ] | MessageID = 0x8fdcb17b | vkUpdateDescriptorSets(): pDescriptorWrites[0].dstSet is VK_NULL_HANDLE.

I can reproduce this issue on macOS using Vulkan/MoltenVK by rendering multiple textures (viewports) that rapidly change in size (e.g. by resizing the main window). This triggers calls to ImGui_ImplVulkan_AddTexture() in my program very frequently.

Edit: spelling

[ocornut]

Hello Michael,
Thanks for your PR.

I am not sure I understand this correctly. If vkAllocateDescriptorSets() fails, we are in an error state already, and I imagine the subsequent check_vk_result() call would log an error anyhow. How does silently not performing the next step would improve the situation? I believe the pool should be configured for allocate to never fail in the first place ?

[PathogenDavid]

I can reproduce this issue on macOS using Vulkan/MoltenVK by rendering multiple textures (viewports) that rapidly change in size (e.g. by resizing the main window). This triggers calls to ImGui_ImplVulkan_AddTexture() in my program very frequently.

I would be concerned that this is the sign of a different underlying bug (possibly a race between the CPU and GPU somewhere) and this PR would just be hiding it.

[micb25]

Hello Omar,

you are completely right that in such a case we are already in an error state. However, (unless the CheckVkResultFn is noreturn) the execution will return to ImGui_ImplVulkan_AddTexture() and call vkUpdateDescriptorSets() in any way.

According to the Vulkan standard a VkWriteDescriptorSet structure must contain a valid descriptor set. This is basically fixed by this PR by avoiding the subsequent call to vkUpdateDescriptorSets(). Please also note that the allocation can fail due to various reasons (VK_ERROR_OUT_OF_HOST_MEMORY, VK_ERROR_OUT_OF_DEVICE_MEMORY, VK_ERROR_FRAGMENTED_POOL, VK_ERROR_OUT_OF_POOL_MEMORY).

@PathogenDavid You are right, there was an issue in my implementation that lead to that specific situation. Nevertheless, I disagree that there will be something hidden by this PR. In a failed state, the ImGui_ImplVulkan_AddTexture() would still call CheckVkResultFn and return VK_NULL_HANDLE. No changes here! It's just the call to vkUpdateDescriptorSets() that is avoided in a fail state by this PR which is in accordance to the standard.