Backends: Vulkan: Added missing font object destruction

[thomasherzog]

If you use the Vulkan backend and create the font texture twice, three object are not destroyed properly. This problem was already observed in issue #3743. Unfortunately, there wasn't a pull request which is why I'm making one now. Thanks to @guybrush77 who already solved this problem!

For completeness, below are the relevant validation layers without the fix.

Error: { Validation }:
    Message ID Name   : VUID-vkDestroyDevice-device-00378
    Message ID Number : 1901072314
    Message           : Validation Error: [ VUID-vkDestroyDevice-device-00378 ] Object 0: handle = 0x2700dc42480, ty
pe = VK_OBJECT_TYPE_DEVICE; Object 1: handle = 0xab64de0000000020, type = VK_OBJECT_TYPE_IMAGE; | MessageID = 0x71500fba
 | OBJ ERROR : For VkDevice 0x2700dc42480[], VkImage 0xab64de0000000020[] has not been destroyed. The Vulkan spec states
: All child objects created on device must have been destroyed prior to destroying device (https://vulkan.lunarg.com/doc
/view/1.2.182.0/windows/1.2-extensions/vkspec.html#VUID-vkDestroyDevice-device-00378)

    Objects:
            Object 0:
                    objectType   = Device
                    objectHandle = 2680290550912
            Object 1:
                    objectType   = Image
                    objectHandle = 12350240169738108960


Error: { Validation }:
    Message ID Name   : VUID-vkDestroyDevice-device-00378
    Message ID Number : 1901072314
    Message           : Validation Error: [ VUID-vkDestroyDevice-device-00378 ] Object 0: handle = 0x2700dc42480, ty
pe = VK_OBJECT_TYPE_DEVICE; Object 1: handle = 0xc4f3070000000021, type = VK_OBJECT_TYPE_DEVICE_MEMORY; | MessageID = 0x
71500fba | OBJ ERROR : For VkDevice 0x2700dc42480[], VkDeviceMemory 0xc4f3070000000021[] has not been destroyed. The Vul
kan spec states: All child objects created on device must have been destroyed prior to destroying device (https://vulkan
.lunarg.com/doc/view/1.2.182.0/windows/1.2-extensions/vkspec.html#VUID-vkDestroyDevice-device-00378)

    Objects:
            Object 0:
                    objectType   = Device
                    objectHandle = 2680290550912
            Object 1:
                    objectType   = DeviceMemory
                    objectHandle = 14191694547355959329


Error: { Validation }:
    Message ID Name   : VUID-vkDestroyDevice-device-00378
    Message ID Number : 1901072314
    Message           : Validation Error: [ VUID-vkDestroyDevice-device-00378 ] Object 0: handle = 0x2700dc42480, ty
pe = VK_OBJECT_TYPE_DEVICE; Object 1: handle = 0x301e6c0000000022, type = VK_OBJECT_TYPE_IMAGE_VIEW; | MessageID = 0x715
00fba | OBJ ERROR : For VkDevice 0x2700dc42480[], VkImageView 0x301e6c0000000022[] has not been destroyed. The Vulkan sp
ec states: All child objects created on device must have been destroyed prior to destroying device (https://vulkan.lunar
g.com/doc/view/1.2.182.0/windows/1.2-extensions/vkspec.html#VUID-vkDestroyDevice-device-00378)

    Objects:
            Object 0:
                    objectType   = Device
                    objectHandle = 2680290550912
            Object 1:
                    objectType   = ImageView
                    objectHandle = 3467327510377660450

[ocornut]

Hello @thomasherzog,
Thanks for the PR.
It however seems to ignore the whole conversation in #3743 tho?
In #3743 (comment) I mention this approach gets me a validation error? Hence @guybrush77 lengthy answer there (which I haven't got around to take care of yet).

[thomasherzog]

Hello,
I added a commit according to my problem regarding my comment in issue #3743. It'd be great if could take a look at it and perform tests as needed. During my tests, none of the previous validation errors occurred anymore.

[helynranta]

Hi! Do you think this PR will be merged or is there any concerns? In comments on that linked issue it was mentioned that those resources might have been in use by recorded command buffet.

One way to solve this issue is to expose ImGui_ImplVulkan_DestroyFontsTexture to user and let it be users responsibility to sync state. I think it might be more "Vulkan way" to do it anyway. If you expose create function for user, in which case use might create new reaources you need to also give them way to destroy them. I dont think there is any generic way to solve this so that it works in every possible way. User knows at which point of execution they attempt to recreate those textures so they know if they need to wait for command buffers or not. It is then their responsibilty to remember to clean up after themselves?

That is just another way to look at it, this way you dont need to force device wait idle when it is not needed (first time textures are created)

[ocornut]

Here's my suggestion, but consider I don't fully understand the situation:

Add

void ImGui_ImplVulkan_DestroyFontsTexture()
{
    ImGui_ImplVulkan_Data* bd = ImGui_ImplVulkan_GetBackendData();
    ImGui_ImplVulkan_InitInfo* v = &bd->VulkanInitInfo;

    if (bd->FontView)
    {
        vkDestroyImageView(v->Device, bd->FontView, v->Allocator);
        bd->FontView = VK_NULL_HANDLE;
    }
    if (bd->FontImage)
    {
        vkDestroyImage(v->Device, bd->FontImage, v->Allocator);
        bd->FontImage = VK_NULL_HANDLE;
    }
    if (bd->FontMemory)
    {
        vkFreeMemory(v->Device, bd->FontMemory, v->Allocator);
        bd->FontMemory = VK_NULL_HANDLE;
    }
}

Add in ImGui_ImplVulkan_CreateFontsTexture()

    // Destroy existing texture (if any)
    if (bd->FontView || bd->FontImage || bd->FontMemory)
    {
        vkQueueWaitIdle(v->Queue);
        ImGui_ImplVulkan_DestroyFontsTexture();
    }

This way one can prevent the wait idle by destroying themselves.
What do you think?

[ocornut]

Tagging @thomasherzog @lerppana @guybrush77

Basically I would like a proof-of-concept font reload (based on #3743 (comment)) that passes validation error, so I can merge a change.

[helynranta]

Ok I wish I committed this before I started editing for better diff. I had to do a bit of messy fix to get sdl2_vulkan example to work with MoltenVK, it was missing the portability extensions etc... Those are somewhere in SetupVulkan function and you can ignore them I quess.

however it is now working, and it is running fine, recreating font textures every frame.
@ocornut here is more cleaned up version with diff on what has been changed on sdl_vulkan_example. It should work as drop in replacement, however I have only tested it with my APPLE macros that fix the portability for MoltenVK.

• I moved font uploading into own function. Point here is that font upload has its own command pool and buffer, so that it can happen completely independently from frame rendering. I think this is good practice anyway (?). OR you could use same pool but allocate a new buffer for this operation. As long as you are not using same buffer as rendering like example used to do.
• You also need to free the descriptor set, otherwise you will run out of memory on your internal pool (at least if you recreate your font every frame)
• Remember to clean the dedicated pool before g_Device

Now unless you are rocking Apple laptop and are familiar with cmake + vcpkg it might be a bit complicated to get this thing running... If needed I'll clean up the example if you are good with this solution.

[ocornut]

I have pushed a fix based on your last linked code + subsequent simplifications, see #6943 (comment) for details. Thanks all !