Vulkan: Adding leak prevention if the fonts need to be recreated

[benbatya-fb]

Issue:
I implemented support for DPI awareness in our ImGui based application by following #3925 .
It was adapted for Win32 and Vulkan.

ImGui_ImplVulkan_CreateFontsTexture() needs to be called each time the DPI changes
but I noticed that the font resources are never cleaned up creating a resource leak.
This diff prevents the leak from occurring.

Thanks!

[benbatya-fb]

I will create a minimal example if required. Thanks!

[ocornut]

Also see #6715 #6327 #4618 and mostly #3743

[benbatya-fb]

Also see #6715 #6327 #4618 and mostly #3743

Seems like there are a ton of different approaches to this problem.
I hope that you can choose one.

~~The benefit of my approach is that the resources are guaranteed to free if ImGui_ImplVulkan_CreateFontsTexture() ~~
~~is called in the render thread before rendering begins. ~~
And it doesn't require modifications of any of the other code.
However, I haven't made an example and validation to prove it

#4618 seems like the better approach

[ocornut]

I appreciate your edits and looking into this.

My problem at this point is that every PR has variations and confusion or unanswered questions.
If I look at #4618 I see a different thing in the actual PR than in the code @helynranta last posted in comments, and that code doesn't follow coding style, nor is applied to all examples, and is a test bed and not a repro.. I believe you are all over-assuming that I intimately understand Vulkan, which I don't.

I realize it isn't a complicated problem but I think one clean PR, _and a repro of well-intending user code (ex: patch in one of main.cpp) exhibiting the bug, I don't think I've had one. (That could be a separate commit in the same PR that I would avoid merging)

[ocornut]

I'm looking into this now and trying to understand the differences between all 5 versions.

• Why does your version Vulkan: Adding leak prevention if the fonts need to be recreated #6943 calls ImGui_ImplVulkan_DestroyFontUploadObjects() at the end of ImGui_ImplVulkan_DestroyFontsTexture(), but other versions don't do it? I can't seem to notice a leak or validation error with/without.
• I am now making sense of the code (helynranta/imgui_font_example@5a5d5d2#diff-3fdb9aa8186e3e6af535b1a155eae4a6f6cfc5fb6ade239b1a8d439b1a9dce06) mentioned in Backends: Vulkan: Added missing font object destruction #4618 (comment) by @helynranta but then I don't understand what that code couldn't be moved to the backend itself ImGui_ImplVulkan_NewFrame() ? Seems like it would make the backend more standard.

[ocornut]

I have merged

6e7b43b Adding ImGui_ImplVulkan_DestroyFontsTexture(), calling it from ImGui_ImplVulkan_CreateFontsTexture(), verifying no validation error, rearranged example code using a UploadFonts() functions, which is more or less the last post of #4618, and similar to #6943, #6715, #6327, #3743)

79a9e2f Following the above and the realization that UploadFonts() didn't need to rely on data created by the example code, I moved it to the backends. Which results in: (1) Removing parameter from ImGui_ImplVulkan_CreateFontsTexture() as backend creates the buffer. (2) Removing ImGui_ImplVulkan_DestroyFontUploadObjects() which is irrelevant. (3) Backend can now automatically create the font texture from its NewFrame() function. (4) It becomes possible to recreate the font texture anytime by calling ImGui_ImplVulkan_CreateFontsTexture() again.

I'm honestly a bit embarrassed at the ongoing complexity of the Vulkan backend: this simplification has been at reach for years and nobody seemingly investigated the thing deeply enough (me included, as I barely understand many of the concepts here). If this was possible to be overlooked, god knows what other healthy simplifications could be done to the backend. Anyway, it's done now AFAIK, it both fixes the leak/validation error, makes examples/user code simpler, and makes reloading font simpler.

My quick testbed was adding this at the very beginning of main loop (before the NewFrame() calls):

        if (ImGui::IsKeyPressed(ImGuiKey_F1))
        {
            static float size = 13.0f;
            size += 1.0f;
            io.Fonts->Clear();
            ImFontConfig cfg;
            cfg.SizePixels = size;
            io.Fonts->AddFontDefault(&cfg);
            io.Fonts->Build();
            ImGui_ImplVulkan_CreateFontsTexture();
        }

Closing 5 pr/issues, yay! Thanks all!

[benbatya-fb]

Congrats!