feat: improve HiDPI behavior across SDL, Vulkan, and ImGui#37
Conversation
- Create SDL window with high pixel density enabled - Drive swapchain dimensions from pixel size (not logical window size) - Handle DPI/scale change events as resize triggers - Ship Roboto Mono and rebuild font atlas when display scale changes - Include SIL Open Font License alongside the font asset
📝 WalkthroughWalkthroughAdds HiDPI support: window created with high-pixel-density flag, SDL pixel-size and display-scale events treated as resizes, Vulkan backend queries window size in pixels with error handling, and ImGuiLayer detects display-scale changes to rebuild and recreate HiDPI fonts dynamically. Changes
Sequence DiagramsequenceDiagram
participant SDL as SDL (Window / Events)
participant App as Application
participant ImGui as ImGuiLayer
participant Vulkan as Vulkan Backend
SDL->>App: WINDOW_DISPLAY_SCALE_CHANGED / WINDOW_PIXEL_SIZE_CHANGED
App->>App: set m_window_resized = true
App->>ImGui: begin_frame()
ImGui->>SDL: query display scale (get_display_scale)
alt display scale changed
ImGui->>ImGui: compute new font size
ImGui->>ImGui: rebuild_fonts(new_scale)
ImGui->>Vulkan: destroy and recreate font texture
end
ImGui->>Vulkan: render frame with scaled fonts
SDL->>App: WINDOW_RESIZED
App->>Vulkan: recreate_swapchain()
Vulkan->>SDL: SDL_GetWindowSizeInPixels()
Vulkan->>Vulkan: create framebuffer with pixel dimensions
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
K1ngst0m
force-pushed
the
dev/refine-imgui
branch
from
835596e to
df8eba3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ui/imgui_layer.cpp (1)
1-416: Pipeline failure: Code requires formatting.The CI pipeline reports that the code needs formatting. Please run
pixi run formatlocally before merging.
🤖 Fix all issues with AI agents
In @src/ui/imgui_layer.cpp:
- Around line 82-84: The call to rebuild_fonts is incorrect:
ImGuiLayer::rebuild_fonts is a member that takes (ImGuiIO&, float) but the code
calls a free function with four args; change the call to use the member on the
layer instance and pass only (io, display_scale), i.e. replace rebuild_fonts(io,
layer->m_font_path, layer->m_font_size_pixels, display_scale) with
layer->rebuild_fonts(io, display_scale) so the member function reads
m_font_path, m_font_size_pixels, and m_font_exists directly and
m_last_display_scale remains updated.
- Around line 38-63: The ImGuiLayer::rebuild_fonts implementation must be moved
out of the anonymous namespace and its signature must match call sites; change
the definition to the class member signature that accepts the font path and font
size (e.g., ImGuiLayer::rebuild_fonts(ImGuiIO& io, const std::filesystem::path&
font_path, float font_size_pixels, float display_scale)) and update the body to
use the passed font_path and font_size_pixels instead of
m_font_path/m_font_size_pixels; also add a member bool m_font_exists to the
ImGuiLayer class (declare in the header and initialize in the constructor by
checking if the font file exists) or otherwise compute existence from the passed
font_path before attempting AddFontFromFileTTF, and ensure the function is
placed outside any anonymous namespace.
🧹 Nitpick comments (1)
src/ui/imgui_layer.cpp (1)
180-204: Consider moving GPU synchronization and font rebuilds out of the per-frame hot path.The
begin_frame()method performswaitIdle()(line 184) and dynamic memory allocations (viarebuild_fonts) when DPI changes are detected. Based on coding guidelines, per-frame code paths should not use blocking synchronization primitives or perform dynamic allocations.While DPI changes are rare events, this still executes within the frame rendering loop and could cause frame drops or stuttering when the user moves the window between monitors.
Consider an alternative approach:
- Set a flag when DPI change is detected in
begin_frame()- Handle the rebuild at the start of the next frame loop iteration (before
begin_frame()is called)- Or defer rendering for one frame while the rebuild completes asynchronously
This would maintain the strict separation of per-frame hot paths from blocking/allocating operations.
Based on learnings, per-frame code paths must not exceed 8ms CPU budget or perform dynamic allocations.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/ui/imgui_layer.cpp
🧰 Additional context used
🧠 Learnings (15)
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.{cpp,hpp} : All vulkan-hpp calls returning `vk::Result` must be explicitly checked. Prohibited pattern: `static_cast<void>(device.waitIdle())`. Use macro `VK_TRY(call, ErrorCode, "message")` for vk::Result early return or manual pattern with error handling.
Applied to files:
src/ui/imgui_layer.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.{cpp,hpp} : Vulkan resource management: Follow RAII guidelines. Use `vk::Unique*` only for appropriate resource types. Call `device.waitIdle()` or wait on fences before destroying GPU-async resources. Store creation info with resources for debugging/recreation. Never leak Vulkan objects. Member ordering: declare in reverse destruction order (device before instance).
Applied to files:
src/ui/imgui_layer.cpp
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Applies to openspec/src/render/**/*.{cpp,hpp} : Use `vk::Unique*` for long-lived Vulkan resources; use plain handles for per-frame/GPU-async resources
Applied to files:
src/ui/imgui_layer.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.cpp : Main thread owns: Vulkan instance, device, swapchain lifecycle; queue submission; window events and user input; job coordination. Main thread MUST NOT: block on I/O, perform heavy computation (>1ms), allocate memory in per-frame code paths.
Applied to files:
src/ui/imgui_layer.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/**/*.cpp : Error handling macros: Use `VK_TRY` for vk::Result early return in Vulkan code. Use `GOGGLES_TRY` for Result<T> propagation (both as statement and expression). Use `GOGGLES_MUST` for internal invariants that should never fail. Use manual pattern for destructors/cleanup functions where propagation is impossible.
Applied to files:
src/ui/imgui_layer.cpp
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Applies to openspec/src/capture/vk_layer/**/*.{cpp,hpp} : Do not perform blocking operations or file I/O in capture layer performance-critical paths (e.g., vkQueuePresentKHR)
Applied to files:
src/ui/imgui_layer.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.{cpp,hpp} : RAII handle guidelines for Vulkan resources: Use `vk::Unique*` only for appropriate resource types: Instance, Device, Surface (long-lived singletons); Swapchain, Pipelines, Layouts (created once). Use plain `vk::` handles for command buffers (pooled lifetime), per-frame sync primitives (reused), and imported external images (requires explicit sync).
Applied to files:
src/ui/imgui_layer.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.cpp : Per-frame code paths on main thread MUST NOT: perform dynamic memory allocations (`new`, `malloc`, `std::make_shared`); use blocking synchronization primitives (`std::mutex`, `std::condition_variable`); exceed 8ms CPU time budget (excluding GPU sync).
Applied to files:
src/ui/imgui_layer.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Vulkan capture layer threading: Hooks execute in calling thread (usually render thread). No blocking in hooks, especially `vkQueuePresentKHR` hot path. Use atomics or locks for thread-safe layer state.
Applied to files:
src/ui/imgui_layer.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/render/**/*.{cpp,hpp} : Application code MUST use vulkan-hpp (C++ bindings), NOT raw Vulkan C API. Use `vk::` types (e.g., `vk::Instance`, `vk::Device`). Do not use raw C handles like `VkInstance`, `VkDevice`. Required configuration: `#define VULKAN_HPP_NO_EXCEPTIONS`, `#define VULKAN_HPP_DISPATCH_LOADER_DYNAMIC 1`, `#include <vulkan/vulkan.hpp>`.
Applied to files:
src/ui/imgui_layer.cpp
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Applies to openspec/src/capture/vk_layer/**/*.{cpp,hpp} : Capture layer (vk_layer/) MUST use raw Vulkan C API, not vulkan-hpp
Applied to files:
src/ui/imgui_layer.cpp
📚 Learning: 2026-01-07T07:20:57.054Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.054Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Capture layer exception: The Vulkan capture layer (`src/capture/vk_layer/`) is exempt and MUST use the raw Vulkan C API because layer dispatch tables require C function pointers and the layer must intercept C API calls.
Applied to files:
src/ui/imgui_layer.cpp
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Vulkan capture layer is the primary frame capture method, intercepting rendered frames before presentation for zero-copy access
Applied to files:
src/ui/imgui_layer.cpp
📚 Learning: 2026-01-07T07:20:57.053Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-07T07:20:57.053Z
Learning: Applies to docs/**/*.{cpp,hpp} : Use top-level namespace `goggles`. Use module namespaces `goggles::<module>` (e.g., `goggles::capture`, `goggles::render`). Use sub-module namespaces `goggles::<module>::<submodule>`. Never use `using namespace` in headers. Prefer explicit namespace qualification or scoped `using` declarations.
Applied to files:
src/ui/imgui_layer.cpp
📚 Learning: 2025-12-29T15:44:30.520Z
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/project.md:0-0
Timestamp: 2025-12-29T15:44:30.520Z
Learning: Applies to openspec/**/*.hpp : Organize namespaces as `goggles` with sub-modules like `goggles::capture`, `goggles::render`; never use `using namespace` in headers
Applied to files:
src/ui/imgui_layer.cpp
🧬 Code graph analysis (1)
src/ui/imgui_layer.cpp (1)
src/ui/imgui_layer.hpp (3)
window(56-57)path(74-74)path(95-95)
🪛 Cppcheck (2.19.0)
src/ui/imgui_layer.cpp
[error] 189-189: Unmatched ')'. Configuration
(syntaxError)
🪛 GitHub Actions: CI
src/ui/imgui_layer.cpp
[error] 1-1: Code needs formatting. Run 'pixi run format' locally.
🔇 Additional comments (3)
src/ui/imgui_layer.cpp (3)
27-36: LGTM! Well-designed helper function.The
get_display_scalefunction correctly handles null window and invalid scale values with appropriate fallbacks.
146-148: Good error handling for font texture creation.Properly checks the return value of
ImGui_ImplVulkan_CreateFontsTexture()and logs a warning on failure, which helps diagnose HiDPI issues.
46-53: This review comment is based on incorrect code. The snippet shown does not match the current state of the file. The actual code at lines 46-53 uses a parameterfont_pathwith an inlinestd::filesystem::exists()check, not am_font_existsmember variable. No such member variable exists in the codebase.The current implementation already properly validates file existence:
if (!font_path.empty() && std::filesystem::exists(font_path, ec) && !ec)No changes are needed for this code section.
Likely an incorrect or invalid review comment.
| float display_scale = get_display_scale(window); | ||
| layer->m_last_display_scale = display_scale; | ||
| rebuild_fonts(io, layer->m_font_path, layer->m_font_size_pixels, display_scale); |
There was a problem hiding this comment.
Critical: Incorrect function call with wrong argument count and missing member access.
Line 84 has two issues:
-
Wrong number of arguments: Calls
rebuild_fontswith 4 arguments(io, layer->m_font_path, layer->m_font_size_pixels, display_scale), but the function signature at line 38 accepts only 2 parameters(ImGuiIO&, float). -
Missing member function call: Should be
layer->rebuild_fonts(io, display_scale)sincerebuild_fontsis a member function (as indicated by theImGuiLayer::qualifier at line 38 and the AI summary).
🔧 Proposed fix
float display_scale = get_display_scale(window);
layer->m_last_display_scale = display_scale;
- rebuild_fonts(io, layer->m_font_path, layer->m_font_size_pixels, display_scale);
+ layer->rebuild_fonts(io, display_scale);The member function will access m_font_path, m_font_size_pixels, and m_font_exists directly as member variables.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| float display_scale = get_display_scale(window); | |
| layer->m_last_display_scale = display_scale; | |
| rebuild_fonts(io, layer->m_font_path, layer->m_font_size_pixels, display_scale); | |
| float display_scale = get_display_scale(window); | |
| layer->m_last_display_scale = display_scale; | |
| layer->rebuild_fonts(io, display_scale); |
🤖 Prompt for AI Agents
In @src/ui/imgui_layer.cpp around lines 82 - 84, The call to rebuild_fonts is
incorrect: ImGuiLayer::rebuild_fonts is a member that takes (ImGuiIO&, float)
but the code calls a free function with four args; change the call to use the
member on the layer instance and pass only (io, display_scale), i.e. replace
rebuild_fonts(io, layer->m_font_path, layer->m_font_size_pixels, display_scale)
with layer->rebuild_fonts(io, display_scale) so the member function reads
m_font_path, m_font_size_pixels, and m_font_exists directly and
m_last_display_scale remains updated.
| GOGGLES_LOG_WARN("waitIdle failed during ImGui DPI font rebuild: {}", | ||
| vk::to_string(wait_result)); | ||
| } | ||
| vk::to_string(wait_result)); | ||
| // Do not proceed with font texture recreation if we can't sync with the GPU. | ||
| return; | ||
| } | ||
| rebuild_fonts(io, m_font_path, m_font_size_pixels, display_scale); |
There was a problem hiding this comment.
Critical: Syntax error with malformed code - duplicate statements and unmatched braces.
Lines 186-193 contain severe syntax errors that will prevent compilation:
-
Duplicate error message: Line 188 completes the log statement, but line 190 has another orphaned
vk::to_string(wait_result));that doesn't belong to any function call. -
Unmatched braces: Line 189 closes the if statement, but lines 191-193 have a comment, return statement, and closing brace that don't match any opening brace.
This appears to be a copy-paste or merge error during development.
🔧 Proposed fix to remove duplicate code
auto wait_result = m_device.waitIdle();
if (wait_result != vk::Result::eSuccess) {
GOGGLES_LOG_WARN("waitIdle failed during ImGui DPI font rebuild: {}",
vk::to_string(wait_result));
+ // Do not proceed with font texture recreation if we can't sync with the GPU.
+ return;
}
- vk::to_string(wait_result));
- // Do not proceed with font texture recreation if we can't sync with the GPU.
- return;
- }
- rebuild_fonts(io, m_font_path, m_font_size_pixels, display_scale);
+
+ auto& io = ImGui::GetIO();
+ rebuild_fonts(io, display_scale);
ImGui_ImplVulkan_DestroyFontsTexture();Note: The rebuild_fonts call also needs correction (see separate comment).
🧰 Tools
🪛 Cppcheck (2.19.0)
[error] 189-189: Unmatched ')'. Configuration
(syntaxError)
1 participant
User description
PR Type
Enhancement
Description
Handle DPI/scale change events as window resize triggers
Use pixel-based dimensions for swapchain instead of logical sizes
Rebuild ImGui font atlas dynamically when display scale changes
Add Roboto Mono font with SIL Open Font License
Enable SDL high pixel density mode for HiDPI support
Diagram Walkthrough
File Walkthrough
application.cpp
Handle DPI and pixel size change eventssrc/app/application.cpp
SDL_EVENT_WINDOW_PIXEL_SIZE_CHANGEDandSDL_EVENT_WINDOW_DISPLAY_SCALE_CHANGEDrecreation
sdl_platform.cpp
Enable high pixel density SDL windowsrc/app/sdl_platform.cpp
SDL_WINDOW_HIGH_PIXEL_DENSITYflag when creating SDL windowvulkan_backend.cpp
Use pixel dimensions for swapchain sizingsrc/render/backend/vulkan_backend.cpp
SDL_GetWindowSizewithSDL_GetWindowSizeInPixelsforpixel-accurate dimensions
SDL_GetWindowSizeInPixelscalls in threelocations
#includeheaderimgui_layer.cpp
Dynamic font atlas rebuild on DPI changessrc/ui/imgui_layer.cpp
get_display_scale()helper function to safely retrieve windowdisplay scale
rebuild_fonts()function to dynamically rebuild font atlaswith proper scaling
begin_frame()to trigger font rebuildwhen scale changes
ImGui_ImplVulkan_CreateFontsTexture()after initialization andDPI changes
variables
imgui_layer.hpp
Add HiDPI state tracking memberssrc/ui/imgui_layer.hpp
to forward declarationsm_font_path,m_font_size_pixels,m_window,m_last_display_scaleOFL.txt
Add SIL Open Font Licenseassets/fonts/OFL.txt
Summary by CodeRabbit
New Features
Improvements
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.