Add a Vulkan renderer backend to imgui-lwjgl3 - #427
Conversation
|
After a quick glance, I can confidently say that the work generally looks solid. The limited scope, implementation within the app module, and working example are exactly what I haven't typically seen in previous implementations. I'll need a bit more time for a detailed review (as I'm simultaneously undertaking a major rewrite of the internal binding structure), but I don't anticipate your PR will be held up for long. At least from this initial look, I don't see anything that would be a blocker. P.S. I want to specifically mention one thing. Given current trends, if any part of your work was done with the help of AI, please note it somewhere. Either as "co-authored" in the commit message, or if the commit is already pushed, simply add a line in the PR description. I have no bias against this whatsoever and actively use AI in my own work, but I just want transparency on the matter. If all the code was written manually, then all I can say is "impressive, very nice" 😅 In that case, there's no need to mention anything anywhere. |
|
Thank you for the quick look and the kind words! To address your note about AI usage: yes, this work was AI-assisted. The Vulkan backend port and the imgui-app integration were written with the help of DeepSeek, and the commit carries a Quick status for reference:
|
Port of the Dear ImGui Vulkan renderer (imgui_impl_vulkan) into the imgui-lwjgl3 module: - ImGuiImplVulkan renderer with descriptor sets, pipeline creation and font atlas upload; shader SPIR-V shipped as classpath resources - ImGuiImplVulkanH helper structures - createVulkanSurface() helpers for the GLFW and SDL3 backends - imgui-app: VULKAN backend with WindowVulkan (swapchain, render pass, per-frame sync objects and resize handling) - example: ExampleVulkan smoke test Co-authored-by: DeepSeek <noreply@deepseek.com>
SpaiR
left a comment
There was a problem hiding this comment.
Thanks — this is solid work overall, and I'd like to get it in. Three things before I merge; everything else I'll take as follow-ups on my side, so don't worry about the long tail.
-
WindowVulkanbreaks the build —AvoidStarImportfires on lines 13, 20, 21, 22 (the rule only excludesGL32andGLFW). Please expand those imports. -
The font texture leaks on shutdown.
createFontsTexture()declaresfontImage,fontMemory,fontImageViewandfontDescriptorSetas locals atImGuiImplVulkan.java:1234-1237, which shadow the fields at260-263. The fields stay zero, sodestroyFontsTexture()destroys nothing. Deleting the four local declarations should be all it takes. -
Please align the descriptor design with our pinned upstream. We're on 1.92.7 (
b1bcb12a6), whereimgui_impl_vulkan.cppuses a single descriptor set withCOMBINED_IMAGE_SAMPLERat set 0 (1106-1116), one pool (1123-1127), and one bind per draw (683). This PR splits texture and sampler into two sets, and the shipped SPIR-V follows the split — the fragment blob is 900 bytes against upstream's 772, so it isn't upstream's shader. I'd like the port to match the pin instead: it keeps the next submodule bump reviewable, and it means you don't have to maintain shaders at all — copy upstream's__glsl_shader_vert_spv/__glsl_shader_frag_spvbyte-for-byte, the wayImGuiImplSdlGpu3Shadersdoes. The deprecatedaddTextureoverload, the second pool-size constant and the "sampler bound once insetupRenderState" caveat from your description should all fall out with it.
I realize 3 is the largest of the three — happy to talk it through if you'd rather split it off, but I'd prefer not to ship a descriptor layout we can't trace back to a revision.
Once these are in, I'll merge and clean up the rest myself.
Match the pinned Dear ImGui 1.92.7 backend: single descriptor set with COMBINED_IMAGE_SAMPLER at set 0, one pool size, one bind per draw, and the upstream shader bytecode shipped as resources. Drops the separate sampler descriptor set, the deprecated addTexture overload and the second pool-size constant. Co-authored-by: DeepSeek <noreply@deepseek.com>
createFontsTexture() declared locals shadowing the fontImage/fontMemory/ fontImageView/fontDescriptorSet fields, so destroyFontsTexture() never released the atlas resources. Also expand the wildcard Vulkan imports in WindowVulkan to satisfy AvoidStarImport. Co-authored-by: DeepSeek <noreply@deepseek.com>
|
Addressed all three review points:
Verified locally: |


Summary
Adds a Vulkan renderer backend (
ImGuiImplVulkan, port ofimgui_impl_vulkan) to theimgui-lwjgl3module, along withImGuiImplVulkanHhelper structures andcreateVulkanSurface()helpers for the GLFW and SDL3 backends. SPIR-V shaders ship as classpath resources.imgui-appgains aVULKANbackend (WindowVulkan) with swapchain, render pass, per-frame sync objects and resize handling, and the example module gets anExampleVulkansmoke test.Type of change
Notes for reviewer
Co-authored-by: DeepSeek <noreply@deepseek.com>trailer.Platform_CreateVkSurfacecallback yet); the sampler descriptor set is bound once insetupRenderState(no per-draw sampler switching).p*setters do not always set the matching*Countfields (VkPresentInfoKHR.pSwapchainsin particular), so counts are set explicitly via native setters.GLFW_SCALE_FRAMEBUFFERis disabled on Wayland to avoid GLFW's viewport interfering with the Vulkan WSI.Fixes #397