Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ImGui_ImplVulkan_DestroyFontsTexture function #6327

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

albin-johansson
Copy link

This PR contains a prototype implementation of ImGui_ImplVulkan_DestroyFontsTexture, which I recently noticed I needed for a project of my own. The absence of ImGui_ImplVulkan_DestroyFontsTexture is discussed #4618, which requests proof-of-concepts for dynamic font reloading using the Vulkan backend. This PR aims to provide one such proof of concept.

I've developed and tested this patch on an M1 Macbook running macOS Ventura 13.3.1, and a Windows 11 machine with an RTX 2070 SUPER GPU. This solution results in no validation layer errors or warnings when I've tested it locally (but I'm sure there are aspects that can be improved, just let me know).

Problem

Attempting to upload the fonts texture more than once is not possible with the Vulkan backend, since we cannot easily destroy the previous fonts texture. This makes font resizing problematic/impossible.

Changes

  • Added ImGui_ImplVulkan_DestroyFontsTexture function
  • ImGui_ImplVulkan_CreateFontsTexture now checks for existing font resources and automatically calls ImGui_ImplVulkan_DestroyFontsTexture if necessary (this means that users don't need to call ImGui_ImplVulkan_DestroyFontsTexture themselves, so it could potentially be hidden, unless we provide it for consistency with other backends)
  • SDL2/Vulkan: added dynamic font reloading (uses ImGui_ImplVulkan_DestroyFontsTexture and ImGui_ImplVulkan_CreateFontsTexture)
  • SDL2/Vulkan: added CMake script
  • SDL2/Vulkan: now requests Vulkan 1.2, this might be unwanted, please let me know
  • SDL2/Vulkan: the VK_KHR_PORTABILITY_SUBSET_EXTENSION_NAME macro is not defined with MoltenVK, use raw string instead
  • SDL2/Vulkan: fixed an issue regarding GPU selection when no dedicated GPU is available
  • SDL2/Vulkan: added button to manually trigger font reload

See also

ocornut added a commit that referenced this pull request Nov 10, 2023
…ImGui_ImplVulkan_CreateFontsTexture() destroy previous one. (#6943, #6715, #6327, #3743, #4618)
ocornut added a commit that referenced this pull request Nov 10, 2023
…mplVulkan_CreateFontsTexture(), no need for user code to create or provide a command-buffer. Removed ImGui_ImplVulkan_DestroyFontUploadObjects(). (#6943, #6715, #6327, #3743, #4618)

See changes in example_glfw_vulkan/main.cpp and example_sdl2_vulkan/main.cpp for reference.
@ocornut
Copy link
Owner

ocornut commented Nov 10, 2023

Hello,

See resolutions for the main issue pushed here: #6943 (comment)

However your PR has other remaining things:

  • (1) You added a cmakefile. There are dozens of such PR but no one seems to agree on what they should do and how they should do it, so this is stalled but I'll eventually merge something. It's just that every time I tried one I found issues and given up.
  • (2) You added VkApplicationInfo. What is the purpose of adding this in our example?
  • (3) "the VK_KHR_PORTABILITY_SUBSET_EXTENSION_NAME macro is not defined with MoltenVK, use raw string instead". If the macro isn't defined in MoltenVK would be the extension be available? Can you confirm it? (fyi said code was added with ba98667 on April 13).
  • (4) "Fix GPU selection when not using dedicated GPU". This has been fixed with 565aa0b.

(Edited) sorry submitted comment too fast.
Intuitively I imagine (3) is the thing we should merge the most. I wonder if we could have a CI test for MoltenVK ?
Those are the projects we build on MacOS:
https://github.com/ocornut/imgui/actions/runs/6825865629/job/18564593664
(the skipped one in that list are built every day, while the other one every commit)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants