Skip to content

Runtime ImGui Shader Controls#12

Merged
K1ngst0m merged 13 commits intomainfrom
feature/add-imgui-preset-reload
Jan 7, 2026
Merged

Runtime ImGui Shader Controls#12
K1ngst0m merged 13 commits intomainfrom
feature/add-imgui-preset-reload

Conversation

@K1ngst0m
Copy link
Copy Markdown
Collaborator

@K1ngst0m K1ngst0m commented Dec 24, 2025 •
edited by BANANASJIM
Loading

Summary

  • ImGui shader controls panel with tree view preset browser
  • Quick search filter for presets
  • Bypass flag for instant shader toggle (no recompilation)
  • Runtime preset reload with async compilation
  • Shader parameter sliders with reset to defaults

Completed

  • 1.1 ImGui docking branch integration
  • 1.2 Shader Controls panel with tree view
  • 1.3 Runtime preset reload
  • 1.4 Bypass flag for instant toggle
  • 1.5 Shader parameter sliders
  • 2.1 Async shader compilation
  • 2.2 Deferred destruction for old FilterChain
  • 2.3 UI parameter sync after chain swap

Pending

  • 1.6 Tests/docs

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Dec 24, 2025 •
edited
Loading

📝 Walkthrough

Walkthrough

Adds a Dear ImGui runtime UI and wiring to the SDL3+Vulkan app: vendor/import ImGui, new goggles_ui target and ImGuiLayer, main-loop integration, UI-aware Vulkan render paths with async shader-preset reload/swap and deferred destroy/rollback, filter-chain passthrough toggle, runtime parameter APIs, packaging and docs.

Changes

Cohort / File(s) Summary
Build & Packaging
cmake/Dependencies.cmake, packages/imgui/recipe.yaml, packages/imgui/pixi.toml, pixi.toml, packages/wlroots_0_18/recipe.yaml		 Adds imported imgui target (ENV CONDA_PREFIX), new imgui package recipe/manifest and pixi entry, adds patchelf build dependency.
Build Integration
src/CMakeLists.txt, src/app/CMakeLists.txt, src/ui/CMakeLists.txt		 Adds ui subdirectory, declares goggles_ui static lib, links it into app; goggles_ui links imgui, Vulkan and SDL3 and enables tooling hooks.
UI Implementation
src/ui/imgui_layer.hpp, src/ui/imgui_layer.cpp, .gitignore		 New ImGuiLayer API/impl: lifecycle, SDL3 event forwarding, per-frame begin/end, Vulkan command recording of ImGui draw data, preset catalog/state, parameter controls, visibility/input-capture; ignores imgui.ini.
Application Integration
src/app/main.cpp		 Creates and wires ImGuiLayer into main loop: preset scanning/catalog, event forwarding conditional on UI capture, F1 UI toggle, and use of UI-aware backend render calls.
Render Backend (Vulkan)
src/render/backend/vulkan_backend.hpp, src/render/backend/vulkan_backend.cpp, src/render/backend/vulkan_error.hpp		 Adds UI-enabled render entry points (render_frame_with_ui, render_clear_with_ui), async reload_shader_preset with pending-swap and deferred-destroy, set_shader_enabled, accessors, frame tracking, and changes to VK_TRY error return wrapping.
Filter Chain & Passes
src/render/chain/filter_chain.hpp, src/render/chain/filter_chain.cpp, src/render/chain/filter_pass.hpp, src/render/chain/filter_pass.cpp		 Adds bypass/passthrough (set_bypass/is_bypass), resize guard, parameter query/manipulation APIs (get_all_parameters, set_parameter, reset_parameter, clear_parameter_overrides), per-pass parameter accessors and UBO override handling.
UI Build Manifest
packages/imgui/pixi.toml		 New local package manifest for imgui included in repo packages.
Design & Specs / Docs
openspec/changes/add-imgui-runtime-preset-control/...		 Adds design, proposal, specs and tasks documenting ImGui integration, preset discovery, reload protocol, passthrough semantics, parameter editing API, risks and migration plan.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant App as Application (main.cpp)
    participant UI as ImGuiLayer
    participant Backend as VulkanBackend
    participant Chain as FilterChain
    participant Vulkan as Vulkan Driver

    App->>UI: begin_frame()
    App->>UI: process_event(SDL_Event)
    UI-->>App: user actions (reload, parameter change, passthrough)

    alt Preset reload requested
        App->>Backend: reload_shader_preset(path)
        Backend->>Vulkan: spawn async compile/build
        Backend->>Chain: prepare pending filter chain (async)
        Backend->>Backend: mark pending chain ready
        Backend->>Backend: check_pending_chain_swap()
        Backend->>Chain: swap in new chain, schedule old chain deferred destroy
        alt load failure
            Backend->>Backend: rollback to previous chain
            Backend-->>App: error
            App->>UI: show error
        end
    end

    alt Passthrough toggle
        UI-->>App: toggle passthrough
        App->>Backend: set_shader_enabled(enabled)
        Backend->>Chain: set_bypass(enabled)
    end

    alt Parameter change
        UI-->>App: parameter change callback
        App->>Chain: set_parameter(name,value) (applies next frame)
    end

    App->>Backend: render_frame_with_ui(frame, ui_callback)
    Backend->>UI: ui_callback(cmd, view, extent)
    UI->>Vulkan: record ImGui draw commands into command buffer
    Backend->>Vulkan: submit & present
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested labels

Review effort 4/5

Poem

🐰 Hopping in with sliders bright,

Presets bloom and shaders light,
Passthrough twitches, reloads sing,
ImGui burrows into rendering,
A rabbit cheers for runtime spring!

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 1.18% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Runtime ImGui Shader Controls' clearly and concisely describes the primary change: adding ImGui-based shader control UI with runtime preset management. It accurately reflects the main objective of the PR without being vague or generic.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai bot mentioned this pull request Dec 25, 2025
Merged
@K1ngst0m K1ngst0m force-pushed the main branch 2 times, most recently from 0efd0ff to 3bb1787 Compare December 30, 2025 04:09
@BANANASJIM BANANASJIM force-pushed the feature/add-imgui-preset-reload branch from 54a3afe to edd04a0 Compare January 6, 2026 17:54
@BANANASJIM BANANASJIM marked this pull request as ready for review January 6, 2026 17:55
@qodo-code-review
Copy link
Copy Markdown

qodo-code-review bot commented Jan 6, 2026 •
edited
Loading

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
⚪
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
⚪
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

🔴
Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Unhandled FS exceptions: scan_presets uses std::filesystem::recursive_directory_iterator without handling
exceptions (e.g., permission errors), which can terminate the app instead of degrading
gracefully.

Referred Code 
static auto scan_presets(const std::filesystem::path& dir) -> std::vector<std::filesystem::path> {
    std::vector<std::filesystem::path> presets;
    if (!std::filesystem::exists(dir)) {
        return presets;
    }
    for (const auto& entry : std::filesystem::recursive_directory_iterator(dir)) {
        if (entry.is_regular_file() && entry.path().extension() == ".slangp") {
            presets.push_back(entry.path());
        }
    }
    std::ranges::sort(presets);
    return presets;

Learn more about managing compliance generic rules or creating your own custom rules

⚪
Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
Preset path trust: reload_shader_preset accepts an arbitrary preset_path and loads it without visible
allowlisting/canonicalization checks, so reviewers should confirm only trusted preset
paths can reach this API and that preset parsing is hardened.

Referred Code 
auto VulkanBackend::reload_shader_preset(const std::filesystem::path& preset_path) -> Result<void> {
    GOGGLES_PROFILE_FUNCTION();

    if (!m_device || !m_filter_chain) {
        return make_error<void>(ErrorCode::vulkan_init_failed, "Backend not initialized");
    }

    VK_TRY(m_device->waitIdle(), ErrorCode::vulkan_device_lost,
           "waitIdle failed before shader reload");

    auto previous_path = m_preset_path;

    m_filter_chain->shutdown();
    GOGGLES_TRY(init_filter_chain());

    if (!preset_path.empty()) {
        auto result = m_filter_chain->load_preset(preset_path);
        if (!result) {
            GOGGLES_LOG_WARN("Failed to load '{}': {} - rolling back to previous",
                             preset_path.string(), result.error().message);
            if (!previous_path.empty()) {


 ... (clipped 13 lines)

Learn more about managing compliance generic rules or creating your own custom rules

  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link
Copy Markdown

qodo-code-review bot commented Jan 6, 2026 •
edited
Loading

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
✅ Avoid runtime stalls during preset reloads
Suggestion Impact:The commit removes the blocking device->waitIdle() and synchronous teardown/recreate in reload_shader_preset, replacing it with an async std::future-based background compilation/loading of a new ShaderRuntime/FilterChain. It then checks readiness each frame (check_pending_chain_swap), swaps the new chain in when ready, and defers destruction of the old chain (cleanup_deferred_destroys) to avoid runtime stalls during reload.

code diff:

@@ -1311,33 +1324,114 @@
         return make_error<void>(ErrorCode::vulkan_init_failed, "Backend not initialized");
     }
 
-    VK_TRY(m_device->waitIdle(), ErrorCode::vulkan_device_lost,
-           "waitIdle failed before shader reload");
-
-    auto previous_path = m_preset_path;
-
-    m_filter_chain->shutdown();
-    GOGGLES_TRY(init_filter_chain());
-
-    if (!preset_path.empty()) {
-        auto result = m_filter_chain->load_preset(preset_path);
+    if (m_pending_chain_ready.load(std::memory_order_acquire)) {
+        GOGGLES_LOG_WARN("Shader reload already pending, ignoring request");
+        return {};
+    }
+
+    if (m_pending_load_future.valid() &&
+        m_pending_load_future.wait_for(std::chrono::seconds(0)) != std::future_status::ready) {
+        GOGGLES_LOG_WARN("Shader compilation in progress, ignoring request");
+        return {};
+    }
+
+    m_pending_preset_path = preset_path;
+
+    // Capture values needed by the async task
+    auto swapchain_format = m_swapchain_format;
+    auto shader_dir = m_shader_dir;
+    auto device = *m_device;
+    auto physical_device = m_physical_device;
+    auto command_pool = *m_command_pool;
+    auto graphics_queue = m_graphics_queue;
+
+    m_pending_load_future = std::async(std::launch::async, [=, this]() -> Result<void> {
+        GOGGLES_PROFILE_SCOPE("AsyncShaderLoad");
+
+        auto runtime_result = ShaderRuntime::create();
+        if (!runtime_result) {
+            GOGGLES_LOG_ERROR("Failed to create shader runtime: {}",
+                              runtime_result.error().message);
+            return make_error<void>(runtime_result.error().code, runtime_result.error().message);
+        }
+
+        VulkanContext vk_ctx{
+            .device = device,
+            .physical_device = physical_device,
+            .command_pool = command_pool,
+            .graphics_queue = graphics_queue,
+        };
+
+        auto chain_result = FilterChain::create(vk_ctx, swapchain_format, MAX_FRAMES_IN_FLIGHT,
+                                                *runtime_result.value(), shader_dir);
+        if (!chain_result) {
+            GOGGLES_LOG_ERROR("Failed to create filter chain: {}", chain_result.error().message);
+            return make_error<void>(chain_result.error().code, chain_result.error().message);
+        }
+
+        if (!preset_path.empty()) {
+            auto load_result = chain_result.value()->load_preset(preset_path);
+            if (!load_result) {
+                GOGGLES_LOG_ERROR("Failed to load preset '{}': {}", preset_path.string(),
+                                  load_result.error().message);
+                return make_error<void>(load_result.error().code, load_result.error().message);
+            }
+        }
+
+        m_pending_shader_runtime = std::move(runtime_result.value());
+        m_pending_filter_chain = std::move(chain_result.value());
+        m_pending_chain_ready.store(true, std::memory_order_release);
+
+        GOGGLES_LOG_INFO("Shader preset compiled: {}",
+                         preset_path.empty() ? "(passthrough)" : preset_path.string());
+        return {};
+    });
+
+    return {};
+}
+
+void VulkanBackend::check_pending_chain_swap() {
+    if (!m_pending_chain_ready.load(std::memory_order_acquire)) {
+        return;
+    }
+
+    // Check if async task completed successfully
+    if (m_pending_load_future.valid()) {
+        auto result = m_pending_load_future.get();
         if (!result) {
-            GOGGLES_LOG_WARN("Failed to load '{}': {} - rolling back to previous",
-                             preset_path.string(), result.error().message);
-            if (!previous_path.empty()) {
-                auto restore = m_filter_chain->load_preset(previous_path);
-                if (restore) {
-                    m_preset_path = previous_path;
-                }
-            }
-            return result;
-        }
-    }
-
-    m_preset_path = preset_path;
-    GOGGLES_LOG_INFO("Shader preset reloaded: {}",
-                     preset_path.empty() ? "(passthrough)" : preset_path.string());
-    return {};
+            GOGGLES_LOG_ERROR("Async shader load failed: {}", result.error().message);
+            m_pending_filter_chain.reset();
+            m_pending_shader_runtime.reset();
+            m_pending_chain_ready.store(false, std::memory_order_release);
+            return;
+        }
+    }
+
+    // Queue old chain for deferred destruction
+    m_deferred_destroys.push_back({
+        .chain = std::move(m_filter_chain),
+        .runtime = std::move(m_shader_runtime),
+        .destroy_after_frame = m_frame_count + MAX_FRAMES_IN_FLIGHT + 1,
+    });
+
+    // Swap in the new chain
+    m_filter_chain = std::move(m_pending_filter_chain);
+    m_shader_runtime = std::move(m_pending_shader_runtime);
+    m_preset_path = m_pending_preset_path;
+    m_pending_chain_ready.store(false, std::memory_order_release);
+
+    GOGGLES_LOG_INFO("Shader chain swapped: {}",
+                     m_preset_path.empty() ? "(passthrough)" : m_preset_path.string());
+}
+
+void VulkanBackend::cleanup_deferred_destroys() {
+    std::erase_if(m_deferred_destroys, [this](const DeferredDestroy& d) {
+        if (m_frame_count >= d.destroy_after_frame) {
+            GOGGLES_LOG_DEBUG("Destroying deferred filter chain");
+            return true;
+        }
+        return false;
+    });
 }

The reload_shader_preset function currently freezes the application by calling
vkDeviceWaitIdle(). This should be replaced with an asynchronous,
double-buffered system to prepare new shaders in the background and hot-swap
them without causing stalls.

Examples:

src/render/backend/vulkan_backend.cpp [1307-1341] 
auto VulkanBackend::reload_shader_preset(const std::filesystem::path& preset_path) -> Result<void> {
    GOGGLES_PROFILE_FUNCTION();

    if (!m_device || !m_filter_chain) {
        return make_error<void>(ErrorCode::vulkan_init_failed, "Backend not initialized");
    }

    VK_TRY(m_device->waitIdle(), ErrorCode::vulkan_device_lost,
           "waitIdle failed before shader reload");


 ... (clipped 25 lines)

Solution Walkthrough:

Before:

auto VulkanBackend::reload_shader_preset(const std::filesystem::path& preset_path) -> Result<void> {
    // This call blocks the entire application until all GPU work is finished.
    VK_TRY(m_device->waitIdle(), ...);

    // The old chain is destroyed.
    m_filter_chain->shutdown();
    
    // A new chain is created and loaded, which can be a slow process
    // involving shader compilation and resource allocation, further stalling the app.
    GOGGLES_TRY(init_filter_chain());
    auto result = m_filter_chain->load_preset(preset_path);
    
    // ... error handling and rollback logic ...
    
    m_preset_path = preset_path;
    return {};
}

After:

class VulkanBackend {
    // ...
    std::future<ResultPtr<FilterChain>> m_next_filter_chain_future;
};

// Triggered by UI
void VulkanBackend::request_shader_reload(const std::filesystem::path& preset_path) {
    // Asynchronously prepare the new filter chain without blocking the render thread.
    m_next_filter_chain_future = std::async(std::launch::async, [this, preset_path] {
        auto new_chain = FilterChain::create(...);
        GOGGLES_TRY(new_chain->load_preset(preset_path));
        return new_chain;
    });
}

// In the main render loop
void main_loop() {
    // Check if the new chain is ready to be swapped in.
    if (m_next_filter_chain_future.valid() && future_is_ready(m_next_filter_chain_future)) {
        auto new_chain_result = m_next_filter_chain_future.get();
        if (new_chain_result) {
            // Briefly wait and then hot-swap the chains.
            m_device->waitIdle();
            m_filter_chain = std::move(new_chain_result.value());
        }
    }
    // Continue rendering with the current (old or new) chain.
    render_frame(...);
}
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a call to vkDeviceWaitIdle() in reload_shader_preset that causes significant application stalls, proposing a superior asynchronous architecture to prevent stuttering, which is a critical performance issue for the new UI feature.

High
Possible issue
✅ Synchronize UI state after reload
Suggestion Impact:The commit removed the UI-side assignment of `state.current_preset = preset` inside `handle_ui_state()` (which could desync on failed reload), and instead updates the UI's current preset from `vulkan_backend.current_preset_path()` when the backend reports the filter chain was swapped. This effectively re-syncs UI state to backend state after reload-related changes, though via a different mechanism than the suggestion (not calling `set_current_preset()` immediately after the reload attempt).

code diff:

@@ -57,9 +89,6 @@
     if (!result) {
         GOGGLES_LOG_ERROR("Failed to load preset '{}': {}", preset.string(),
                           result.error().message);
-    } else {
-        state.current_preset = preset;
-        GOGGLES_LOG_INFO("Loaded preset: {}", preset.string());
     }
 }
 
@@ -205,6 +234,11 @@
             }
         }
 
+        if (imgui_layer && vulkan_backend.consume_chain_swapped()) {
+            imgui_layer->state().current_preset = vulkan_backend.current_preset_path();
+            update_ui_parameters(vulkan_backend, *imgui_layer);
+        }

Synchronize the UI state with the backend's actual current preset after any
reload attempt, ensuring the UI accurately reflects the active preset even if a
reload fails and rolls back.

src/app/main.cpp [35-64]

 static void handle_ui_state(goggles::render::VulkanBackend& vulkan_backend,
                             goggles::ui::ImGuiLayer& imgui_layer) {
     static bool last_shader_enabled = false;
     auto& state = imgui_layer.state();
 
     if (state.shader_enabled != last_shader_enabled) {
         vulkan_backend.set_shader_enabled(state.shader_enabled);
         last_shader_enabled = state.shader_enabled;
     }
 
     if (!state.reload_requested) {
         return;
     }
     state.reload_requested = false;
 
     if (state.selected_preset_index < 0 ||
         state.selected_preset_index >= static_cast<int>(state.preset_catalog.size())) {
         return;
     }
 
     const auto& preset = state.preset_catalog[static_cast<size_t>(state.selected_preset_index)];
     auto result = vulkan_backend.reload_shader_preset(preset);
     if (!result) {
         GOGGLES_LOG_ERROR("Failed to load preset '{}': {}", preset.string(),
                           result.error().message);
     } else {
-        state.current_preset = preset;
-        GOGGLES_LOG_INFO("Loaded preset: {}", preset.string());
+        GOGGLES_LOG_INFO("Loaded preset: {}", vulkan_backend.current_preset_path().string());
     }
+    // Always sync UI state with backend state after reload attempt
+    imgui_layer.set_current_preset(vulkan_backend.current_preset_path());
 }

[Suggestion processed]

Suggestion importance[1-10]: 8

__

Why: This suggestion fixes a definite bug where the UI state becomes desynchronized from the backend state after a failed shader reload, which would lead to a confusing user experience.

Medium
✅ Improve shader reload rollback logic
Suggestion Impact:The commit updates the shader preset reload error path to handle rollback failure: if restoring the previous preset fails, it now logs a warning and clears m_preset_path to fall back to passthrough, preventing inconsistent state. This aligns with the suggestion’s intent, though it logs WARN (not ERROR) and does not mirror the exact suggested structure.

code diff:

@@ -1328,8 +1329,12 @@
                 auto restore = m_filter_chain->load_preset(previous_path);
                 if (restore) {
                     m_preset_path = previous_path;
+                    return result;
                 }
+                GOGGLES_LOG_WARN("Rollback to '{}' also failed, falling back to passthrough",
+                                 previous_path.string());
             }
+            m_preset_path.clear();
             return result;
         }

Improve the shader preset rollback logic to correctly handle cases where
restoring the previous preset also fails, ensuring the application state remains
consistent.

src/render/backend/vulkan_backend.cpp [1307-1341]

 auto VulkanBackend::reload_shader_preset(const std::filesystem::path& preset_path) -> Result<void> {
     GOGGLES_PROFILE_FUNCTION();
 
     if (!m_device || !m_filter_chain) {
         return make_error<void>(ErrorCode::vulkan_init_failed, "Backend not initialized");
     }
 
     VK_TRY(m_device->waitIdle(), ErrorCode::vulkan_device_lost,
            "waitIdle failed before shader reload");
 
     auto previous_path = m_preset_path;
 
     m_filter_chain->shutdown();
     GOGGLES_TRY(init_filter_chain());
 
     if (!preset_path.empty()) {
         auto result = m_filter_chain->load_preset(preset_path);
         if (!result) {
             GOGGLES_LOG_WARN("Failed to load '{}': {} - rolling back to previous",
                              preset_path.string(), result.error().message);
             if (!previous_path.empty()) {
                 auto restore = m_filter_chain->load_preset(previous_path);
                 if (restore) {
                     m_preset_path = previous_path;
+                } else {
+                    m_preset_path.clear(); // Rollback failed, no preset is active
+                    GOGGLES_LOG_ERROR("Failed to restore previous preset '{}': {}",
+                                      previous_path.string(), restore.error().message);
                 }
+            } else {
+                m_preset_path.clear(); // No previous preset to restore
             }
             return result;
         }
     }
 
     m_preset_path = preset_path;
     GOGGLES_LOG_INFO("Shader preset reloaded: {}",
                      preset_path.empty() ? "(passthrough)" : preset_path.string());
     return {};
 }

[Suggestion processed]

Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a flaw in the error handling logic where a failed rollback leaves the system in an inconsistent state, and provides a good fix to ensure state integrity.

Medium
✅ Handle filesystem errors during preset scan
Suggestion Impact:The commit improves robustness of preset scanning by switching to std::filesystem APIs that take std::error_code and by propagating/checking the error_code during exists() and recursive_directory_iterator iteration/increment, preventing exceptions from escaping during filesystem traversal. This addresses the same underlying issue (filesystem operations may fail) but uses error_code-based handling rather than a try/catch + logging approach.

code diff:

 static auto scan_presets(const std::filesystem::path& dir) -> std::vector<std::filesystem::path> {
     std::vector<std::filesystem::path> presets;
-    if (!std::filesystem::exists(dir)) {
+    std::error_code ec;
+    if (!std::filesystem::exists(dir, ec) || ec) {
         return presets;
     }
-    for (const auto& entry : std::filesystem::recursive_directory_iterator(dir)) {
-        if (entry.is_regular_file() && entry.path().extension() == ".slangp") {
-            presets.push_back(entry.path());
+    for (auto it = std::filesystem::recursive_directory_iterator(dir, ec);
+         it != std::filesystem::recursive_directory_iterator() && !ec; it.increment(ec)) {
+        if (it->is_regular_file(ec) && !ec && it->path().extension() == ".slangp") {
+            presets.push_back(it->path());
         }
     }
     std::ranges::sort(presets);

Add error handling to the scan_presets function to catch and log potential
std::filesystem::filesystem_error exceptions that may occur during directory
iteration.

src/app/main.cpp [21-33]

+#include <algorithm> // for std::ranges::sort
+#include <ranges>
 static auto scan_presets(const std::filesystem::path& dir) -> std::vector<std::filesystem::path> {
     std::vector<std::filesystem::path> presets;
-    if (!std::filesystem::exists(dir)) {
-        return presets;
+    try {
+        if (!std::filesystem::exists(dir)) {
+            return presets;
+        }
+        for (const auto& entry : std::filesystem::recursive_directory_iterator(dir)) {
+            if (entry.is_regular_file() && entry.path().extension() == ".slangp") {
+                presets.push_back(entry.path());
+            }
+        }
+        std::ranges::sort(presets);
+    } catch (const std::filesystem::filesystem_error& e) {
+        GOGGLES_LOG_WARN("Preset scanning failed for '{}': {}", dir.string(), e.what());
     }
-    for (const auto& entry : std::filesystem::recursive_directory_iterator(dir)) {
-        if (entry.is_regular_file() && entry.path().extension() == ".slangp") {
-            presets.push_back(entry.path());
-        }
-    }
-    std::ranges::sort(presets);
     return presets;
 }

[Suggestion processed]

Suggestion importance[1-10]: 6

__

Why: The suggestion correctly points out that filesystem operations can throw exceptions and adds a try-catch block to handle them gracefully, improving the application's robustness.

Low
  • Update

coderabbitai[bot]
coderabbitai bot reviewed Jan 6, 2026
View reviewed changes
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 Fix all issues with AI Agents 
In @openspec/changes/add-imgui-runtime-preset-control/design.md:
- Line 15: Update the "Dear ImGui dependency scope" sentence to reflect the
actual acquisition method used in the implementation: replace "vendored or
fetched via CPM" with the Pixi-local package approach (referencing
packages/imgui/recipe.yaml) and clarify that the ImGui docking tag (v1.91+
docking) is linked only to the Goggles app target while the capture layer
remains unchanged and does not link ImGui symbols.

In @packages/imgui/recipe.yaml:
- Around line 14-22: The build snippet runs the $CXX compile step then
unconditionally archives with ar, which can produce an incomplete libimgui.a if
compilation fails; ensure the script checks compilation success by either
enabling strict failure (set -euo pipefail) at the top of the shell block or
chaining the commands so ar rcs and mv only run on success (e.g., run the $CXX
... invocation and then && ar rcs libimgui.a *.o && mv libimgui.a $PREFIX/lib/),
or wrap the compile in an if-check that exits with non-zero on failure.

In @src/render/backend/vulkan_backend.cpp:
- Around line 1307-1341: The rollback path in
VulkanBackend::reload_shader_preset leaves m_preset_path pointing to the old
preset even when restoring previous_path fails or when previous_path is empty,
causing an inconsistent state with the filter chain; after a failed
load_preset(preset_path) and a failed restore
(m_filter_chain->load_preset(previous_path) returns false) or when
previous_path.empty(), clear m_preset_path (e.g., m_preset_path.clear()) and
emit a warning before returning the original error to keep the preset state
consistent with the passthrough filter chain.

In @src/render/chain/filter_chain.cpp:
- Line 303: m_bypass_enabled is a plain bool which can race between UI calls to
set_bypass() and per-frame reads in record() / is_bypass(); change its type to
std::atomic<bool> in the FilterChain class, update any initialization
(constructor) and assignments to use atomic operations (e.g., store) and reads
(load) in set_bypass(), is_bypass(), and where record() checks it so access is
lock-free and thread-safe, and include <atomic> in the header.
🧹 Nitpick comments (4)
pixi.toml (1)

128-128: Consider pinning the patchelf version for consistency.

While patchelf is a build-time tool, most other dependencies in this file use at least major version constraints (e.g., cmake = "3.31.*", ninja = "1.12.*"). Using a wildcard version "*" may reduce build reproducibility.

🔎 Suggested version constraint 
-patchelf = "*"
+patchelf = ">=0.17"
src/app/main.cpp (1)

21-33: Consider exception handling for filesystem operations.

std::filesystem::recursive_directory_iterator can throw exceptions (e.g., permission denied, symlink loops). Since the codebase uses VULKAN_HPP_NO_EXCEPTIONS, consider wrapping the iteration with std::error_code overloads to maintain consistency.

🔎 Proposed fix using error_code 
 static auto scan_presets(const std::filesystem::path& dir) -> std::vector<std::filesystem::path> {
     std::vector<std::filesystem::path> presets;
-    if (!std::filesystem::exists(dir)) {
+    std::error_code ec;
+    if (!std::filesystem::exists(dir, ec) || ec) {
         return presets;
     }
-    for (const auto& entry : std::filesystem::recursive_directory_iterator(dir)) {
+    for (const auto& entry : std::filesystem::recursive_directory_iterator(dir, ec)) {
+        if (ec) {
+            GOGGLES_LOG_WARN("Error scanning presets directory: {}", ec.message());
+            break;
+        }
         if (entry.is_regular_file() && entry.path().extension() == ".slangp") {
             presets.push_back(entry.path());
         }
     }
     std::ranges::sort(presets);
     return presets;
 }
src/render/backend/vulkan_backend.cpp (1)

1158-1238: Significant code duplication with render_frame and record_render_commands.

This function duplicates ~80 lines from render_frame (lines 1112-1134) and record_render_commands (lines 910-968). Consider extracting common logic into a shared helper to reduce maintenance burden.

🔎 Suggested refactoring approach

Extract the common barrier setup and filter chain recording into a private helper:

auto VulkanBackend::record_render_core(vk::CommandBuffer cmd, uint32_t image_index) -> Result<void> {
    // Common barrier setup and filter chain recording
    // ...
    return {};
}

Then both paths can use:

// render_frame / render_frame_with_ui
GOGGLES_TRY(record_render_core(cmd, image_index));
if (ui_callback) {
    ui_callback(cmd, *m_swapchain_image_views[image_index], m_swapchain_extent);
}
// final barrier and end
src/ui/imgui_layer.hpp (1)

34-43: Remove unused parameters_dirty field or add a TODO comment.

The parameters_dirty field in ShaderControlState is declared but never referenced anywhere in the codebase. If this is intended for future functionality, add a clarifying TODO comment; otherwise, remove it to reduce unnecessary state.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5d9ba1c and edd04a0.

⛔ Files ignored due to path filters (1)
  • pixi.lock is excluded by !**/*.lock
📒 Files selected for processing (21)
  • cmake/Dependencies.cmake
  • openspec/changes/add-imgui-runtime-preset-control/design.md
  • openspec/changes/add-imgui-runtime-preset-control/proposal.md
  • openspec/changes/add-imgui-runtime-preset-control/specs/app-window/spec.md
  • openspec/changes/add-imgui-runtime-preset-control/specs/render-pipeline/spec.md
  • openspec/changes/add-imgui-runtime-preset-control/tasks.md
  • packages/imgui/pixi.toml
  • packages/imgui/recipe.yaml
  • packages/wlroots_0_18/recipe.yaml
  • pixi.toml
  • src/CMakeLists.txt
  • src/app/CMakeLists.txt
  • src/app/main.cpp
  • src/render/backend/vulkan_backend.cpp
  • src/render/backend/vulkan_backend.hpp
  • src/render/backend/vulkan_error.hpp
  • src/render/chain/filter_chain.cpp
  • src/render/chain/filter_chain.hpp
  • src/ui/CMakeLists.txt
  • src/ui/imgui_layer.cpp
  • src/ui/imgui_layer.hpp
🧰 Additional context used
📓 Path-based instructions (3)
openspec/**/*.md

📄 CodeRabbit inference engine (openspec/AGENTS.md)

openspec/**/*.md: Use #### Scenario: format (4 hashtags) for scenario headers in requirements, not bullets or bold text, with at least one scenario per requirement
Use format - **WHEN** [condition] - **THEN** [expected result] for scenario steps in requirements

Files:

  • openspec/changes/add-imgui-runtime-preset-control/design.md
  • openspec/changes/add-imgui-runtime-preset-control/proposal.md
  • openspec/changes/add-imgui-runtime-preset-control/specs/app-window/spec.md
  • openspec/changes/add-imgui-runtime-preset-control/tasks.md
  • openspec/changes/add-imgui-runtime-preset-control/specs/render-pipeline/spec.md
openspec/changes/**/proposal.md

📄 CodeRabbit inference engine (openspec/AGENTS.md)

List spec deltas in proposal.md under 'What Changes' section, marking breaking changes with BREAKING

Files:

  • openspec/changes/add-imgui-runtime-preset-control/proposal.md
openspec/changes/**/specs/**/*.md

📄 CodeRabbit inference engine (openspec/AGENTS.md)

openspec/changes/**/specs/**/*.md: Write spec deltas using ## ADDED|MODIFIED|REMOVED|RENAMED Requirements format with at least one #### Scenario: per requirement in spec files
When modifying existing requirements in a MODIFIED delta, paste the full requirement block including the header and all scenarios, as the archiver will replace the entire requirement
Use ADDED for new capabilities that can stand alone; use MODIFIED for changes to existing requirement behavior, scope, or acceptance criteria; use RENAMED for name-only changes

Files:

  • openspec/changes/add-imgui-runtime-preset-control/specs/app-window/spec.md
  • openspec/changes/add-imgui-runtime-preset-control/specs/render-pipeline/spec.md
🧠 Learnings (25)
📚 Learning: 2026-01-02T07:02:32.720Z 
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Vulkan layer code in `src/capture/vk_layer/` has strict logging constraints: use only `error` and `critical` levels, no file I/O or blocking operations in hot paths, prefix all logs with `[goggles_vklayer]`, never log in `vkQueuePresentKHR` hot path.

Applied to files:

  • src/ui/CMakeLists.txt
  • openspec/changes/add-imgui-runtime-preset-control/specs/app-window/spec.md
  • src/app/main.cpp
  • src/render/backend/vulkan_backend.cpp
  • src/render/backend/vulkan_error.hpp
  • src/ui/imgui_layer.hpp
  • src/ui/imgui_layer.cpp
📚 Learning: 2026-01-02T07:02:32.720Z 
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/**/*.{cpp,hpp} : Use namespace `goggles` at top-level, `goggles::<module>` for modules (e.g., `goggles::capture`, `goggles::render`, `goggles::util`), `goggles::<module>::<submodule>` for sub-modules. Never use `using namespace` in headers. Prefer explicit namespace qualification or scoped `using` declarations.

Applied to files:

  • src/ui/CMakeLists.txt
  • src/app/CMakeLists.txt
📚 Learning: 2026-01-02T07:02:32.720Z 
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/**/*.{cpp,hpp} : Define project-specific logging macros (`GOGGLES_LOG_TRACE`, `GOGGLES_LOG_DEBUG`, `GOGGLES_LOG_INFO`, `GOGGLES_LOG_WARN`, `GOGGLES_LOG_ERROR`, `GOGGLES_LOG_CRITICAL`) wrapping spdlog for consistent usage.

Applied to files:

  • src/ui/CMakeLists.txt
📚 Learning: 2026-01-02T07:02:32.720Z 
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Exception - Capture Layer Code (`src/capture/vk_layer/`) is exempt and MUST use the raw Vulkan C API because layer dispatch tables require C function pointers, layer must intercept C API calls from host applications, and minimal dependencies required.

Applied to files:

  • src/ui/CMakeLists.txt
  • src/app/main.cpp
  • src/render/backend/vulkan_error.hpp
  • src/render/backend/vulkan_backend.hpp
  • src/ui/imgui_layer.hpp
  • 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/CMakeLists.txt
  • src/app/main.cpp
  • src/render/backend/vulkan_backend.hpp
  • src/ui/imgui_layer.hpp
  • 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} : Application code MUST use vulkan-hpp with `VULKAN_HPP_NO_EXCEPTIONS` and dynamic dispatch

Applied to files:

  • src/ui/CMakeLists.txt
  • src/render/backend/vulkan_backend.cpp
  • src/render/backend/vulkan_error.hpp
  • src/render/backend/vulkan_backend.hpp
📚 Learning: 2026-01-02T07:02:32.720Z 
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/src/**/*.{cpp,hpp} : Application code MUST use `vulkan-hpp` (C++ bindings), NOT the raw Vulkan C API. Use `vk::` types (e.g., `vk::Instance`, `vk::Device`, `vk::Image`). Never use raw C handles (`VkInstance`, `VkDevice`, etc.) in application code. Required configuration: `#define VULKAN_HPP_NO_EXCEPTIONS` and `#define VULKAN_HPP_DISPATCH_LOADER_DYNAMIC 1`.

Applied to files:

  • src/ui/CMakeLists.txt
  • cmake/Dependencies.cmake
  • src/app/main.cpp
  • src/render/backend/vulkan_error.hpp
  • src/render/backend/vulkan_backend.hpp
📚 Learning: 2025-12-19T08:54:18.412Z 
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-12-19T08:54:18.412Z
Learning: Create a `design.md` file only when the change is cross-cutting (multiple services/modules), introduces new external dependencies, has security/performance/migration complexity, or has ambiguity requiring technical decisions

Applied to files:

  • openspec/changes/add-imgui-runtime-preset-control/design.md
📚 Learning: 2026-01-02T07:02:32.720Z 
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/config/goggles.toml : Development/testing configuration: `config/goggles.toml` in repository root. Structure should include sections for [capture], [pipeline], [render], and [logging].

Applied to files:

  • openspec/changes/add-imgui-runtime-preset-control/design.md
  • openspec/changes/add-imgui-runtime-preset-control/specs/app-window/spec.md
  • src/app/CMakeLists.txt
📚 Learning: 2026-01-02T07:02:32.720Z 
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Use hybrid dependency management: CPM.cmake for simple and header-only dependencies, Conan for complex compiled dependencies, system packages for platform-specific libraries.

Applied to files:

  • cmake/Dependencies.cmake
📚 Learning: 2026-01-02T07:02:32.720Z 
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/tests/**/*.cpp : Only non-GPU logic is tested initially: in scope are utility functions, configuration parsing, error handling, pipeline graph logic. Out of scope: Vulkan initialization, GPU resource management, rendering, capture layer.

Applied to files:

  • openspec/changes/add-imgui-runtime-preset-control/specs/app-window/spec.md
  • openspec/changes/add-imgui-runtime-preset-control/tasks.md
  • src/app/main.cpp
  • src/ui/imgui_layer.cpp
📚 Learning: 2026-01-02T07:02:32.720Z 
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Vulkan layer runs in host application threads (no control). Hooks execute in calling thread (usually render thread). No blocking in hooks, especially `vkQueuePresentKHR`. Use atomics or locks for thread-safe shared layer state.

Applied to files:

  • src/app/main.cpp
  • src/render/backend/vulkan_backend.cpp
  • src/ui/imgui_layer.hpp
  • 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/app/main.cpp
  • src/render/backend/vulkan_backend.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/app/main.cpp
  • src/render/backend/vulkan_backend.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: Default to single-threaded render loop on main thread; use central job system (goggles::util::JobSystem) for any task parallelism

Applied to files:

  • src/app/main.cpp
📚 Learning: 2026-01-02T07:02:32.720Z 
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/src/**/*.{cpp,hpp} : Due to async GPU execution, RAII handles (`vk::Unique*`) cannot automatically handle synchronization. Use selectively: `vk::Unique*` for Instance, Device, Surface (long-lived singletons), Swapchain, Pipelines, Layouts (created once). Use plain handles for Command buffers, per-frame sync primitives (Fence, Semaphore), Imported external images.

Applied to files:

  • src/app/main.cpp
  • src/render/backend/vulkan_backend.cpp
  • src/render/backend/vulkan_backend.hpp
📚 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/app/main.cpp
  • src/render/backend/vulkan_backend.cpp
  • src/render/backend/vulkan_error.hpp
  • src/render/backend/vulkan_backend.hpp
📚 Learning: 2026-01-02T07:02:32.720Z 
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Initialize logger once at application startup on the main thread. Capture layer should initialize logger lazily on first `vkCreateInstance` hook.

Applied to files:

  • src/app/main.cpp
📚 Learning: 2026-01-02T07:02:32.720Z 
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/src/**/*.{cpp,hpp} : For Vulkan resources, follow RAII guidelines. Use `vk::Unique*` only for appropriate resource types (per D.2 table). Call `device.waitIdle()` or wait on appropriate fences before destroying resources with GPU-async lifetime. Store creation info with resources for debugging. Never leak Vulkan objects. Declare members in reverse destruction order (device before instance).

Applied to files:

  • src/render/backend/vulkan_backend.cpp
  • src/render/backend/vulkan_error.hpp
  • src/render/backend/vulkan_backend.hpp
  • src/ui/imgui_layer.cpp
📚 Learning: 2026-01-02T07:02:32.720Z 
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/**/*.{cpp,hpp} : All `vulkan-hpp` calls returning `vk::Result` must be explicitly checked. Prohibited: masking with `static_cast<void>()`. Use `VK_TRY` macro for early return with `vk::Result`, `GOGGLES_TRY` for Result<T> propagation (expression-style), `GOGGLES_MUST` for internal invariants (aborts on failure).

Applied to files:

  • src/render/backend/vulkan_backend.cpp
  • src/render/backend/vulkan_error.hpp
📚 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: The shader pipeline is a multi-pass model designed for compatibility with RetroArch shader semantics

Applied to files:

  • openspec/changes/add-imgui-runtime-preset-control/specs/render-pipeline/spec.md
📚 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/app/CMakeLists.txt
📚 Learning: 2026-01-02T07:02:32.720Z 
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/**/*.{cpp,hpp} : All fallible operations must return `tl::expected<T, Error>` using the martinmoene/expected-lite library. No exceptions for expected failures (file I/O, resource creation, parsing). Exceptions allowed only for programming errors (assertions, contract violations).

Applied to files:

  • src/render/backend/vulkan_error.hpp
📚 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/**/*.{cpp,hpp} : All fallible operations MUST return `tl::expected<T, Error>` for error handling

Applied to files:

  • src/render/backend/vulkan_error.hpp
📚 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/**/*.{cpp,hpp} : Exceptions are only for unrecoverable programming errors; use `tl::expected` for recoverable errors

Applied to files:

  • src/render/backend/vulkan_error.hpp
🧬 Code graph analysis (3)
src/render/backend/vulkan_backend.cpp (7)
src/render/backend/vulkan_backend.hpp (10)
  • frame (36-36)
  • frame (50-51)
  • frame (87-87)
  • ui_callback (52-52)
  • image_index (95-95)
  • cmd (91-92)
  • cmd (93-94)
  • preset_path (40-40)
  • preset_path (41-42)
  • enabled (47-47)
src/util/error.hpp (1)
  • make_error (50-52)
src/capture/vk_layer/wsi_virtual.cpp (2)
  • acquire_next_image (413-466)
  • acquire_next_image (413-416)
src/render/chain/filter_chain.hpp (5)
  • cmd (42-45)
  • cmd (73-73)
  • preset_path (40-40)
  • enabled (53-53)
  • enabled (53-53)
src/render/chain/output_pass.hpp (1)
  • cmd (28-28)
src/render/chain/pass.hpp (1)
  • cmd (44-44)
src/render/chain/preset_parser.hpp (2)
  • preset_path (59-59)
  • preset_path (62-64)
src/render/backend/vulkan_backend.hpp (5)
src/render/chain/filter_chain.hpp (5)
  • nodiscard (51-51)
  • nodiscard (54-54)
  • preset_path (40-40)
  • enabled (53-53)
  • enabled (53-53)
src/capture/capture_receiver.hpp (7)
  • nodiscard (34-34)
  • nodiscard (35-35)
  • nodiscard (36-36)
  • nodiscard (38-38)
  • nodiscard (39-39)
  • nodiscard (40-42)
  • nodiscard (43-43)
src/util/config.hpp (1)
  • nodiscard (19-31)
src/render/chain/filter_pass.hpp (1)
  • nodiscard (87-89)
src/render/chain/preset_parser.hpp (2)
  • preset_path (59-59)
  • preset_path (62-64)
src/ui/imgui_layer.cpp (1)
src/util/error.hpp (2)
  • make_result_ptr_error (63-64)
  • make_result_ptr (57-57)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: Build and Test (test preset)
  • GitHub Check: Static Analysis (clang-tidy)
🔇 Additional comments (34)
packages/wlroots_0_18/recipe.yaml (1)

61-61: LGTM! Standard practice for conda binary packages.

Adding patchelf as a build dependency is appropriate for conda/rattler-build packages that install pre-built shared libraries. The tool is typically used to fix RPATH/RUNPATH entries to ensure libraries can locate their dependencies within the conda environment.

pixi.toml (1)

143-143: LGTM! Consistent with existing local package pattern.

The imgui local package reference follows the established pattern for other local packages in this file and properly integrates the new ImGui dependency for the shader controls UI.

openspec/changes/add-imgui-runtime-preset-control/tasks.md (1)

1-7: LGTM! Clear and well-structured task breakdown.

The implementation tasks are clearly defined with appropriate scope. The completed items align with the code changes in this PR, and the pending tasks (parameter sliders and tests) are logical follow-ups.

src/render/chain/filter_chain.hpp (2)

99-99: LGTM! Appropriate default initialization.

The bypass flag is correctly default-initialized to false, ensuring the filter chain is active by default. The placement and initialization style follow modern C++ best practices.

53-54: Bypass API is properly implemented and used.

The getter and setter follow conventions and are const-correct. Verification confirms m_bypass_enabled is checked in FilterChain processing logic (line 303 of filter_chain.cpp) to skip filtering when bypass is enabled.

packages/imgui/pixi.toml (1)

1-6: LGTM! Well-formed package manifest.

The package manifest correctly defines the imgui package with version 1.91.8 (a stable Dear ImGui release from December 2024) and uses an appropriately pinned build backend.

src/app/CMakeLists.txt (1)

15-15: LGTM - UI library integration.

The addition of goggles_ui to the link libraries correctly integrates the new ImGui-based UI module with the main application executable.

src/CMakeLists.txt (1)

8-8: LGTM - UI module integration.

The UI subdirectory addition follows the established pattern and correctly includes the new module in the build system.

src/render/chain/filter_chain.cpp (1)

446-449: LGTM - Defensive guard for uninitialized state.

The early return correctly prevents resize calculations when no source frame has been rendered yet, avoiding potential issues with zero-dimension extents.

openspec/changes/add-imgui-runtime-preset-control/specs/render-pipeline/spec.md (1)

1-53: LGTM - Well-structured requirements specification.

The spec correctly follows the required format with ## ADDED Requirements, proper #### Scenario: headers, and GIVEN-WHEN-THEN scenario steps. The requirements comprehensively cover runtime reload, passthrough mode, and parameter access with clear, testable acceptance criteria that align with the PR objectives.

src/render/backend/vulkan_error.hpp (1)

9-16: [rewritten review comment]
[classification tag]

openspec/changes/add-imgui-runtime-preset-control/proposal.md (1)

1-14: LGTM! Proposal structure follows guidelines.

The proposal correctly uses the "What Changes" section format and appropriately omits BREAKING markers since all changes are additions without API breakage.

cmake/Dependencies.cmake (1)

65-71: LGTM! ImGui integration follows Pixi dependency pattern.

The IMPORTED target correctly references the Pixi-built library and aligns with the project's hybrid dependency management approach. The INTERFACE linkage to SDL3 and Vulkan is appropriate for ImGui's backend requirements.

Based on learnings, using Pixi for this complex compiled dependency (with multiple backends) is the right choice rather than CPM.

openspec/changes/add-imgui-runtime-preset-control/design.md (1)

1-39: Well-structured design document.

The design comprehensively covers integration approach, preset discovery, reload protocol, UI state management, risks, and migration plan. The structure appropriately justifies the cross-cutting nature of this change.

src/ui/CMakeLists.txt (2)

1-22: Library target structure is well-organized.

The goggles_ui target correctly exposes its public API, links necessary dependencies (imgui, Vulkan, SDL3), and enables appropriate build tooling (clang-tidy, sanitizers).

17-19: Add VULKAN_HPP_DISPATCH_LOADER_DYNAMIC to goggles_ui target.

The target currently defines VULKAN_HPP_NO_EXCEPTIONS but is missing the required VULKAN_HPP_DISPATCH_LOADER_DYNAMIC=1 definition. The code uses vulkan-hpp extensively (vk::Device, vk::DescriptorPool, etc.) and both definitions are required for proper dynamic dispatch configuration:

target_compile_definitions(goggles_ui PRIVATE
    VULKAN_HPP_NO_EXCEPTIONS
    VULKAN_HPP_DISPATCH_LOADER_DYNAMIC=1
)
⛔ Skipped due to learnings 
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} : Application code MUST use vulkan-hpp with `VULKAN_HPP_NO_EXCEPTIONS` and dynamic dispatch

Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/src/**/*.{cpp,hpp} : Application code MUST use `vulkan-hpp` (C++ bindings), NOT the raw Vulkan C API. Use `vk::` types (e.g., `vk::Instance`, `vk::Device`, `vk::Image`). Never use raw C handles (`VkInstance`, `VkDevice`, etc.) in application code. Required configuration: `#define VULKAN_HPP_NO_EXCEPTIONS` and `#define VULKAN_HPP_DISPATCH_LOADER_DYNAMIC 1`.

Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Exception - Capture Layer Code (`src/capture/vk_layer/`) is exempt and MUST use the raw Vulkan C API because layer dispatch tables require C function pointers, layer must intercept C API calls from host applications, and minimal dependencies required.

Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/**/*.{cpp,hpp} : All `vulkan-hpp` calls returning `vk::Result` must be explicitly checked. Prohibited: masking with `static_cast<void>()`. Use `VK_TRY` macro for early return with `vk::Result`, `GOGGLES_TRY` for Result<T> propagation (expression-style), `GOGGLES_MUST` for internal invariants (aborts on failure).

Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Vulkan layer code in `src/capture/vk_layer/` has strict logging constraints: use only `error` and `critical` levels, no file I/O or blocking operations in hot paths, prefix all logs with `[goggles_vklayer]`, never log in `vkQueuePresentKHR` hot path.

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

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

Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/src/**/*.{cpp,hpp} : For Vulkan resources, follow RAII guidelines. Use `vk::Unique*` only for appropriate resource types (per D.2 table). Call `device.waitIdle()` or wait on appropriate fences before destroying resources with GPU-async lifetime. Store creation info with resources for debugging. Never leak Vulkan objects. Declare members in reverse destruction order (device before instance).

Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/src/**/*.{cpp,hpp} : Due to async GPU execution, RAII handles (`vk::Unique*`) cannot automatically handle synchronization. Use selectively: `vk::Unique*` for Instance, Device, Surface (long-lived singletons), Swapchain, Pipelines, Layouts (created once). Use plain handles for Command buffers, per-frame sync primitives (Fence, Semaphore), Imported external images.

Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/**/*.{cpp,hpp} : Define project-specific logging macros (`GOGGLES_LOG_TRACE`, `GOGGLES_LOG_DEBUG`, `GOGGLES_LOG_INFO`, `GOGGLES_LOG_WARN`, `GOGGLES_LOG_ERROR`, `GOGGLES_LOG_CRITICAL`) wrapping spdlog for consistent usage.

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

Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/**/*.{cpp,hpp} : Use namespace `goggles` at top-level, `goggles::<module>` for modules (e.g., `goggles::capture`, `goggles::render`, `goggles::util`), `goggles::<module>::<submodule>` for sub-modules. Never use `using namespace` in headers. Prefer explicit namespace qualification or scoped `using` declarations.

Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/tests/**/*.cpp : Only non-GPU logic is tested initially: in scope are utility functions, configuration parsing, error handling, pipeline graph logic. Out of scope: Vulkan initialization, GPU resource management, rendering, capture layer.
src/app/main.cpp (5)

35-64: LGTM!

The function correctly handles shader state synchronization with the UI. Error handling for preset reload is appropriate, and the early return pattern keeps the logic clean.

79-133: LGTM!

The event handling correctly forwards SDL events to ImGui first, then conditionally forwards to the input forwarder based on wants_capture_keyboard() and wants_capture_mouse(). The F1 toggle logic is clean and the pattern is consistent across all input types.

173-206: LGTM!

The UI integration into the render loop is well-structured:

  • State handling before begin_frame() ensures UI state is applied before rendering
  • The callback lambda correctly captures end_frame() and record() calls
  • Conditional render paths (render_frame_with_ui vs render_frame) avoid overhead when UI is not active

299-320: LGTM!

Good defensive programming with graceful degradation - the application continues to function if ImGui initialization fails. The warning log provides visibility, and the conditional UI usage throughout the codebase handles the nullptr case.

354-358: LGTM!

The shutdown sequence correctly resets the ImGui layer before the Vulkan backend, ensuring proper resource cleanup order.

openspec/changes/add-imgui-runtime-preset-control/specs/app-window/spec.md (1)

1-53: LGTM!

The spec document follows the required format:

  • Uses ## ADDED Requirements format as specified
  • Each requirement has proper #### Scenario: headers
  • Scenario steps use **GIVEN**, **WHEN**, **THEN** format
  • Multiple scenarios per requirement cover key use cases

As per coding guidelines for openspec/**/*.md.

src/render/backend/vulkan_backend.cpp (2)

1240-1305: Same duplication pattern as render_frame_with_ui.

This shares the duplication pattern flagged above. The refactoring suggestion would also apply here.

1343-1347: LGTM!

Simple and correct implementation. The inversion of enabled to !enabled for bypass is the expected behavior.

src/render/backend/vulkan_backend.hpp (2)

41-52: LGTM!

The new API surface is well-designed:

  • reload_shader_preset returns Result<void> for error handling
  • current_preset_path returns const reference to avoid copies
  • UiRenderCallback type alias clearly documents the expected signature
  • UI-enabled render methods follow the existing render_frame/render_clear pattern

54-63: LGTM!

These accessors are necessary for initializing the ImGui Vulkan backend. They follow the existing patterns in the codebase and are appropriately marked [[nodiscard]] and const.

src/ui/imgui_layer.cpp (5)

25-100: LGTM!

The creation factory has proper error handling:

  • Each failure path cleans up previously created resources
  • Descriptor pool with generous sizes supports ImGui's internal allocations
  • Dynamic rendering setup is correctly configured for Vulkan 1.3

Based on learnings, using raw vk::Device instead of vk::UniqueDevice is appropriate here since the device lifetime is managed externally.

106-119: LGTM!

Proper shutdown sequence with waitIdle() before destroying ImGui resources. The null check on m_device provides idempotency for double-shutdown protection.

163-180: LGTM!

The tree-building logic correctly handles:

  • Path decomposition via iterator
  • Distinguishing directories (intermediate nodes) from files (leaf nodes with preset_index)
  • Automatic creation of parent nodes

259-305: LGTM!

Well-structured UI with:

  • Sensible defaults for window position/size
  • Clear separation of concerns (enable toggle, preset browser, parameters)
  • Search filter that switches display mode
  • Consistent button behavior

307-328: LGTM!

Parameter controls correctly:

  • Compare old/new values to avoid redundant callbacks
  • Respect min/max bounds via SliderFloat
  • Show helpful tooltips for parameter descriptions
  • Support reset to defaults via callback
src/ui/imgui_layer.hpp (3)

13-16: LGTM!

Simple and effective tree structure using std::map for ordered children traversal. The -1 sentinel for preset_index clearly distinguishes directories from preset files.

18-26: LGTM!

ImGuiConfig cleanly encapsulates all Vulkan handles needed for ImGui initialization, matching the requirements of ImGui_ImplVulkan_InitInfo. Using vk:: types aligns with project policies.

49-101: LGTM!

The ImGuiLayer class follows project patterns:

  • Factory method returns ResultPtr for error handling
  • Deleted copy/move constructors prevent resource management issues
  • Private default constructor enforces factory usage
  • Clear separation between public API and private implementation details

openspec/changes/add-imgui-runtime-preset-control/design.md
- No generalized file picker; we only need preset discovery from known directories for now.

## Decisions
1. **Dear ImGui dependency scope** – Use the latest docking tag (`v1.91+ docking`) vendored or fetched via CPM, but link it solely with the Goggles app target. The capture layer build remains unchanged and never links ImGui symbols.
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Update dependency acquisition method to match implementation.

The design mentions "vendored or fetched via CPM," but the actual implementation uses a Pixi local package (packages/imgui/recipe.yaml). Update this line to reflect the Pixi-based approach.

🔎 Suggested correction 
-1. **Dear ImGui dependency scope** – Use the latest docking tag (`v1.91+ docking`) vendored or fetched via CPM, but link it solely with the Goggles app target. The capture layer build remains unchanged and never links ImGui symbols.
+1. **Dear ImGui dependency scope** – Use the latest docking tag (`v1.91.8-docking`) built via Pixi local package, but link it solely with the Goggles app target. The capture layer build remains unchanged and never links ImGui symbols.
📝 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.

Suggested change
1. **Dear ImGui dependency scope** – Use the latest docking tag (`v1.91+ docking`) vendored or fetched via CPM, but link it solely with the Goggles app target. The capture layer build remains unchanged and never links ImGui symbols.
1. **Dear ImGui dependency scope** – Use the latest docking tag (`v1.91.8-docking`) built via Pixi local package, but link it solely with the Goggles app target. The capture layer build remains unchanged and never links ImGui symbols.
🤖 Prompt for AI Agents 
In @openspec/changes/add-imgui-runtime-preset-control/design.md at line 15,
Update the "Dear ImGui dependency scope" sentence to reflect the actual
acquisition method used in the implementation: replace "vendored or fetched via
CPM" with the Pixi-local package approach (referencing
packages/imgui/recipe.yaml) and clarify that the ImGui docking tag (v1.91+
docking) is linked only to the Goggles app target while the capture layer
remains unchanged and does not link ImGui symbols.

packages/imgui/recipe.yaml
Comment on lines +14 to +22
- |
# Build static library
$CXX $CXXFLAGS -c -fPIC -I. \
imgui.cpp imgui_demo.cpp imgui_draw.cpp imgui_tables.cpp imgui_widgets.cpp \
backends/imgui_impl_sdl3.cpp backends/imgui_impl_vulkan.cpp \
-I$PREFIX/include -I$PREFIX/include/SDL3

ar rcs libimgui.a *.o
mv libimgui.a $PREFIX/lib/
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Add error handling to the build script.

The build script doesn't verify compilation success before archiving. If any source file fails to compile, the script will continue and create an incomplete libimgui.a, leading to cryptic linker errors downstream.

🔎 Proposed fix to add error checking 
     - |
       # Build static library
       $CXX $CXXFLAGS -c -fPIC -I. \
         imgui.cpp imgui_demo.cpp imgui_draw.cpp imgui_tables.cpp imgui_widgets.cpp \
         backends/imgui_impl_sdl3.cpp backends/imgui_impl_vulkan.cpp \
-        -I$PREFIX/include -I$PREFIX/include/SDL3
+        -I$PREFIX/include -I$PREFIX/include/SDL3 || exit 1

-      ar rcs libimgui.a *.o
+      ar rcs libimgui.a *.o || exit 1
       mv libimgui.a $PREFIX/lib/
📝 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.

Suggested change
- |
# Build static library
$CXX $CXXFLAGS -c -fPIC -I. \
imgui.cpp imgui_demo.cpp imgui_draw.cpp imgui_tables.cpp imgui_widgets.cpp \
backends/imgui_impl_sdl3.cpp backends/imgui_impl_vulkan.cpp \
-I$PREFIX/include -I$PREFIX/include/SDL3
ar rcs libimgui.a *.o
mv libimgui.a $PREFIX/lib/
- |
# Build static library
$CXX $CXXFLAGS -c -fPIC -I. \
imgui.cpp imgui_demo.cpp imgui_draw.cpp imgui_tables.cpp imgui_widgets.cpp \
backends/imgui_impl_sdl3.cpp backends/imgui_impl_vulkan.cpp \
-I$PREFIX/include -I$PREFIX/include/SDL3 || exit 1
ar rcs libimgui.a *.o || exit 1
mv libimgui.a $PREFIX/lib/
🤖 Prompt for AI Agents 
In @packages/imgui/recipe.yaml around lines 14 - 22, The build snippet runs the
$CXX compile step then unconditionally archives with ar, which can produce an
incomplete libimgui.a if compilation fails; ensure the script checks compilation
success by either enabling strict failure (set -euo pipefail) at the top of the
shell block or chaining the commands so ar rcs and mv only run on success (e.g.,
run the $CXX ... invocation and then && ar rcs libimgui.a *.o && mv libimgui.a
$PREFIX/lib/), or wrap the compile in an if-check that exits with non-zero on
failure.

@BANANASJIM
fix: clang-tidy warnings and search filter
35d5e4d 
- Use std::error_code for filesystem operations
- Make bypass flag atomic for thread safety
- Search filter matches filename only
- Add format change detection flag
coderabbitai[bot]
coderabbitai bot reviewed Jan 6, 2026
View reviewed changes
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (5)
src/render/backend/vulkan_backend.cpp (2)

1159-1239: Consider extracting shared command recording logic.

render_frame_with_ui duplicates ~80 lines from render_frame and record_render_commands, differing only by the UI callback invocation (lines 1224-1226). While the duplication is localized and functional, extracting the shared barrier setup and command recording into a helper would improve maintainability.

🔎 Potential refactor approach

Extract a shared record_frame_commands helper that accepts an optional UI callback parameter:

auto VulkanBackend::record_frame_commands(vk::CommandBuffer cmd, uint32_t image_index,
                                          const std::optional<UiRenderCallback>& ui_callback)
    -> Result<void> {
    // Common barrier setup and filter chain recording
    // ...
    m_filter_chain->record(cmd, ...);
    
    if (ui_callback) {
        (*ui_callback)(cmd, *m_swapchain_image_views[image_index], m_swapchain_extent);
    }
    
    // Final barrier and end
    // ...
}

Then both render_frame and render_frame_with_ui can call this helper.

1241-1306: Consider extracting shared clear command recording logic.

Similar to render_frame_with_ui, this method duplicates render_clear and record_clear_commands (~65 lines), differing only by the UI callback invocation (lines 1291-1293). Extracting shared logic would reduce duplication.

🔎 Potential refactor approach

Extract a shared record_clear_commands_with_callback helper:

auto VulkanBackend::record_clear_commands_internal(vk::CommandBuffer cmd, uint32_t image_index,
                                                   const std::optional<UiRenderCallback>& ui_callback)
    -> Result<void> {
    // Barrier setup, clear rendering
    // ...
    cmd.beginRendering(rendering_info);
    cmd.endRendering();
    
    if (ui_callback) {
        (*ui_callback)(cmd, *m_swapchain_image_views[image_index], m_swapchain_extent);
    }
    
    // Final barrier and end
    // ...
}
src/ui/imgui_layer.cpp (3)

14-21: Consider using std::transform for more idiomatic C++.

The current implementation is correct and properly avoids undefined behavior with the unsigned char cast. However, a more idiomatic approach would use std::transform:

auto to_lower(std::string_view str) -> std::string {
    std::string result;
    result.resize(str.size());
    std::transform(str.begin(), str.end(), result.begin(), [](unsigned char c) {
        return static_cast<char>(std::tolower(c));
    });
    return result;
}

43-61: Consider making descriptor pool sizes configurable.

The descriptor pool sizes are hardcoded to 1000 for each descriptor type. This could be wasteful for simple UIs or insufficient for complex ones. Consider making these configurable via ImGuiConfig or calculating them based on expected UI complexity.

182-190: Linear search may impact performance for large preset catalogs.

The function performs a linear O(n) search through the preset catalog. For catalogs with hundreds of presets, this could introduce noticeable lag during preset selection.

Consider maintaining a std::unordered_map<std::filesystem::path, int> index alongside the catalog:

// In header
std::unordered_map<std::filesystem::path, int> m_preset_index_map;

// In set_preset_catalog
void ImGuiLayer::set_preset_catalog(std::vector<std::filesystem::path> presets) {
    m_state.preset_catalog = std::move(presets);
    m_preset_index_map.clear();
    for (size_t i = 0; i < m_state.preset_catalog.size(); ++i) {
        m_preset_index_map[m_state.preset_catalog[i]] = static_cast<int>(i);
    }
    rebuild_preset_tree();
}

// In set_current_preset
void ImGuiLayer::set_current_preset(const std::filesystem::path& path) {
    m_state.current_preset = path;
    if (auto it = m_preset_index_map.find(path); it != m_preset_index_map.end()) {
        m_state.selected_preset_index = it->second;
    }
}
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between edd04a0 and 35d5e4d.

📒 Files selected for processing (7)
  • .gitignore
  • src/app/main.cpp
  • src/render/backend/vulkan_backend.cpp
  • src/render/backend/vulkan_backend.hpp
  • src/render/chain/filter_chain.cpp
  • src/render/chain/filter_chain.hpp
  • src/ui/imgui_layer.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/render/chain/filter_chain.cpp
🧰 Additional context used
🧠 Learnings (15)
📚 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/render/backend/vulkan_backend.cpp
  • src/app/main.cpp
  • src/render/backend/vulkan_backend.hpp
📚 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/render/backend/vulkan_backend.cpp
  • src/app/main.cpp
  • src/render/backend/vulkan_backend.hpp
📚 Learning: 2026-01-02T07:02:32.720Z 
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/src/**/*.{cpp,hpp} : Due to async GPU execution, RAII handles (`vk::Unique*`) cannot automatically handle synchronization. Use selectively: `vk::Unique*` for Instance, Device, Surface (long-lived singletons), Swapchain, Pipelines, Layouts (created once). Use plain handles for Command buffers, per-frame sync primitives (Fence, Semaphore), Imported external images.

Applied to files:

  • src/render/backend/vulkan_backend.cpp
  • src/app/main.cpp
  • src/render/backend/vulkan_backend.hpp
📚 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} : Application code MUST use vulkan-hpp with `VULKAN_HPP_NO_EXCEPTIONS` and dynamic dispatch

Applied to files:

  • src/render/backend/vulkan_backend.cpp
  • src/render/backend/vulkan_backend.hpp
📚 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/render/backend/vulkan_backend.cpp
  • src/app/main.cpp
📚 Learning: 2026-01-02T07:02:32.720Z 
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Vulkan layer runs in host application threads (no control). Hooks execute in calling thread (usually render thread). No blocking in hooks, especially `vkQueuePresentKHR`. Use atomics or locks for thread-safe shared layer state.

Applied to files:

  • src/render/backend/vulkan_backend.cpp
  • src/app/main.cpp
  • src/ui/imgui_layer.cpp
📚 Learning: 2026-01-02T07:02:32.720Z 
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/tests/**/*.cpp : Only non-GPU logic is tested initially: in scope are utility functions, configuration parsing, error handling, pipeline graph logic. Out of scope: Vulkan initialization, GPU resource management, rendering, capture layer.

Applied to files:

  • src/render/backend/vulkan_backend.cpp
  • src/app/main.cpp
  • src/render/backend/vulkan_backend.hpp
📚 Learning: 2026-01-02T07:02:32.720Z 
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Vulkan layer code in `src/capture/vk_layer/` has strict logging constraints: use only `error` and `critical` levels, no file I/O or blocking operations in hot paths, prefix all logs with `[goggles_vklayer]`, never log in `vkQueuePresentKHR` hot path.

Applied to files:

  • src/render/backend/vulkan_backend.cpp
  • src/app/main.cpp
  • src/render/backend/vulkan_backend.hpp
  • src/ui/imgui_layer.cpp
📚 Learning: 2026-01-02T07:02:32.720Z 
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/**/*.{cpp,hpp} : All `vulkan-hpp` calls returning `vk::Result` must be explicitly checked. Prohibited: masking with `static_cast<void>()`. Use `VK_TRY` macro for early return with `vk::Result`, `GOGGLES_TRY` for Result<T> propagation (expression-style), `GOGGLES_MUST` for internal invariants (aborts on failure).

Applied to files:

  • src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-02T07:02:32.720Z 
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/src/**/*.{cpp,hpp} : For Vulkan resources, follow RAII guidelines. Use `vk::Unique*` only for appropriate resource types (per D.2 table). Call `device.waitIdle()` or wait on appropriate fences before destroying resources with GPU-async lifetime. Store creation info with resources for debugging. Never leak Vulkan objects. Declare members in reverse destruction order (device before instance).

Applied to files:

  • src/render/backend/vulkan_backend.cpp
  • src/render/backend/vulkan_backend.hpp
  • 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: Default to single-threaded render loop on main thread; use central job system (goggles::util::JobSystem) for any task parallelism

Applied to files:

  • src/app/main.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/app/main.cpp
  • src/render/backend/vulkan_backend.hpp
  • src/ui/imgui_layer.cpp
📚 Learning: 2026-01-02T07:02:32.720Z 
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Exception - Capture Layer Code (`src/capture/vk_layer/`) is exempt and MUST use the raw Vulkan C API because layer dispatch tables require C function pointers, layer must intercept C API calls from host applications, and minimal dependencies required.

Applied to files:

  • src/app/main.cpp
  • src/render/backend/vulkan_backend.hpp
  • src/ui/imgui_layer.cpp
📚 Learning: 2026-01-02T07:02:32.720Z 
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Initialize logger once at application startup on the main thread. Capture layer should initialize logger lazily on first `vkCreateInstance` hook.

Applied to files:

  • src/app/main.cpp
📚 Learning: 2026-01-02T07:02:32.720Z 
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/src/**/*.{cpp,hpp} : Application code MUST use `vulkan-hpp` (C++ bindings), NOT the raw Vulkan C API. Use `vk::` types (e.g., `vk::Instance`, `vk::Device`, `vk::Image`). Never use raw C handles (`VkInstance`, `VkDevice`, etc.) in application code. Required configuration: `#define VULKAN_HPP_NO_EXCEPTIONS` and `#define VULKAN_HPP_DISPATCH_LOADER_DYNAMIC 1`.

Applied to files:

  • src/render/backend/vulkan_backend.hpp
🧬 Code graph analysis (2)
src/render/backend/vulkan_backend.cpp (7)
src/render/backend/vulkan_backend.hpp (10)
  • frame (36-36)
  • frame (50-51)
  • frame (93-93)
  • ui_callback (52-52)
  • image_index (101-101)
  • cmd (97-98)
  • cmd (99-100)
  • preset_path (40-40)
  • preset_path (41-42)
  • enabled (47-47)
src/util/error.hpp (1)
  • make_error (50-52)
src/render/chain/filter_chain.hpp (5)
  • cmd (43-46)
  • cmd (76-76)
  • preset_path (41-41)
  • enabled (54-54)
  • enabled (54-54)
src/ui/imgui_layer.hpp (1)
  • cmd (66-66)
src/render/chain/output_pass.hpp (1)
  • cmd (28-28)
src/render/chain/pass.hpp (1)
  • cmd (44-44)
src/render/chain/preset_parser.hpp (2)
  • preset_path (59-59)
  • preset_path (62-64)
src/ui/imgui_layer.cpp (1)
src/util/error.hpp (2)
  • make_result_ptr_error (63-64)
  • make_result_ptr (57-57)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: Build and Test (test preset)
  • GitHub Check: Static Analysis (clang-tidy)
🔇 Additional comments (20)
.gitignore (1)

122-122: LGTM: Standard ImGui state file exclusion.

The imgui.ini entry correctly excludes ImGui's per-user window layout state from version control.

src/render/chain/filter_chain.hpp (1)

9-9: LGTM: Atomic bypass flag with appropriate memory ordering.

The atomic bypass flag uses memory_order_relaxed, which is appropriate given the project's single-threaded render loop (main thread handles both UI state updates and rendering). The atomic type provides thread-safe access without unnecessary synchronization overhead.

Based on learnings, the default threading model uses a single-threaded render loop on the main thread.

Also applies to: 54-57, 102-102

src/app/main.cpp (5)

22-36: LGTM: Robust filesystem traversal with proper error handling.

The function correctly uses std::error_code for exception-free filesystem operations and gracefully returns an empty vector on failure. Silent failure is appropriate here since the preset catalog is optional functionality.

38-67: LGTM: Clean UI state management with proper bounds checking.

The function correctly:

  • Tracks shader enablement changes with a function-local static
  • Uses std::cmp_greater_equal for safe signed/unsigned comparison (line 54)
  • Handles preset reload errors gracefully with logging
  • Updates UI state consistently after operations

82-110: LGTM: Proper ImGui event handling and input capture logic.

The implementation correctly:

  • Forwards events to ImGui first (lines 82-84)
  • Implements F1 toggle for UI visibility (lines 92-93)
  • Gates input forwarding based on ImGui's capture state (lines 94-101, 103-105)
  • Prevents input leakage to the application when UI is focused

176-209: LGTM: Clean UI integration into render loop.

The UI callback pattern (lines 182-187) cleanly separates UI rendering concerns from the main loop. The conditional rendering paths (lines 191-209) correctly select between render_frame_with_ui/render_clear_with_ui and their non-UI counterparts based on ImGui availability.

302-323: LGTM: Robust ImGui initialization with graceful fallback.

The initialization correctly:

  • Constructs ImGuiConfig from Vulkan backend state (lines 302-310)
  • Handles creation failure gracefully without crashing (lines 314-316)
  • Synchronizes preset catalog and current state with the backend (lines 318-321)
  • Treats ImGui as optional functionality with appropriate logging
src/render/backend/vulkan_backend.cpp (3)

503-503: LGTM: Format change tracking flag.

The flag correctly signals that a swapchain format change has occurred, consumed later via the consume_format_change() accessor.

1308-1346: LGTM: Rollback logic correctly handles failure states.

The preset reload logic properly:

  • Waits for device idle before chain shutdown (lines 1315-1316)
  • Attempts rollback to previous preset on failure (lines 1328-1333)
  • Clears m_preset_path when rollback fails (line 1337), maintaining consistency with the passthrough filter chain
  • Logs appropriate warnings at each failure stage (lines 1326, 1334)

The past review comment concern about inconsistent m_preset_path state is now resolved by line 1337.

1348-1352: LGTM: Correct shader enablement logic.

The method correctly inverts the enabled flag to set bypass state (enabled=true → bypass=false), allowing shaders to be toggled via the UI.

src/render/backend/vulkan_backend.hpp (2)

41-74: LGTM: Clean public API extensions for UI integration.

The additions provide a cohesive interface for runtime UI functionality:

  • Preset management: reload_shader_preset, current_preset_path (lines 41-45)
  • Shader control: set_shader_enabled (line 47)
  • UI rendering: UiRenderCallback, render_frame_with_ui, render_clear_with_ui (lines 49-52)
  • ImGui configuration: Vulkan object accessors (lines 54-63)
  • Format change tracking: consume_format_change (lines 70-74)

All accessors appropriately use [[nodiscard]] and const-correctness.

157-157: LGTM: Format change tracking flag.

The m_format_changed flag backs the consume_format_change() public accessor, supporting the consume-once pattern for format change notifications.

src/ui/imgui_layer.cpp (8)

25-41: Validate config parameters before use.

The create() method does not validate the provided config parameters before using them. Invalid handles (e.g., null device, invalid queue family) could lead to crashes or undefined behavior during ImGui initialization.

Consider adding validation checks:

 auto ImGuiLayer::create(SDL_Window* window, const ImGuiConfig& config) -> ResultPtr<ImGuiLayer> {
+    if (!window) {
+        return make_result_ptr_error<ImGuiLayer>(ErrorCode::invalid_argument, "Window is null");
+    }
+    if (!config.device || !config.physical_device || !config.instance) {
+        return make_result_ptr_error<ImGuiLayer>(ErrorCode::invalid_argument, "Invalid Vulkan config");
+    }
+
     auto layer = std::unique_ptr<ImGuiLayer>(new ImGuiLayer());

102-119: LGTM! Proper Vulkan resource cleanup.

The shutdown sequence correctly calls device.waitIdle() before destroying Vulkan resources, follows the proper cleanup order (ImGui Vulkan → SDL3 → descriptor pool → context), and sets handles to nullptr. This aligns with RAII guidelines for Vulkan resources.

Based on learnings, per Vulkan RAII guidelines: wait for device idle before destroying resources with GPU-async lifetime.

139-156: LGTM! Correct dynamic rendering setup for UI overlay.

The command buffer recording correctly uses AttachmentLoadOp::eLoad to preserve existing frame content, which is appropriate for rendering the UI overlay on top of the scene. The dynamic rendering setup matches the initialization in create().

221-257: LGTM! Proper ImGui tree and list rendering.

The preset rendering correctly uses PushID/PopID for unique widget IDs, shows tooltips with full paths for usability, and properly distinguishes between leaf nodes (presets) and folders using preset_index >= 0. The recursive tree traversal is well-structured.

307-328: LGTM! Efficient parameter control implementation.

The parameter controls correctly:

  • Invoke callbacks only when values actually change (line 319 comparison)
  • Show tooltips from parameter descriptions for better UX
  • Provide a reset-to-defaults button

212-219: No action required. The search_filter is correctly declared as std::array<char, 256> in the state structure, making the .data() call safe and idiomatic. The array is zero-initialized and properly sized for use with ImGui::InputTextWithHint, ensuring null-termination is maintained.

163-180: Edge cases are handled correctly by std::filesystem::path iteration.

Paths come from std::filesystem::recursive_directory_iterator, which returns valid std::filesystem::path objects that are automatically normalized. The iteration logic using path.begin()/path.end() is robust for all valid path types—empty components are skipped, absolute/relative paths are handled identically, and single-component paths work correctly. No changes needed.

281-281: No action required. The code at line 281 correctly uses ImGuiChildFlags_Borders, which is supported in ImGui v1.90.0 and later. The project's ImGui dependency is managed via Conda, ensuring a compatible version is in use.

coderabbitai[bot]
coderabbitai bot reviewed Jan 6, 2026
View reviewed changes
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
src/ui/imgui_layer.cpp (2)

65-72: Consider using modern vulkan-hpp result handling.

The structured binding auto [pool_result, pool] = config.device.createDescriptorPool(...) works but is legacy style. Modern vulkan-hpp typically uses:

auto result = config.device.createDescriptorPool(pool_info);
if (result.result != vk::Result::eSuccess) {
    // error handling
}
layer->m_descriptor_pool = result.value;

Or if the project has a VK_TRY macro (as suggested by the mention of VK_TRY in vulkan_error.hpp changes), use that pattern instead.

🔎 Proposed refactor using result/value pattern 
-    auto [pool_result, pool] = config.device.createDescriptorPool(pool_info);
-    if (pool_result != vk::Result::eSuccess) {
+    auto pool_result = config.device.createDescriptorPool(pool_info);
+    if (pool_result.result != vk::Result::eSuccess) {
         ImGui_ImplSDL3_Shutdown();
         ImGui::DestroyContext();
         return make_result_ptr_error<ImGuiLayer>(ErrorCode::vulkan_init_failed,
                                                  "Failed to create ImGui descriptor pool");
     }
-    layer->m_descriptor_pool = pool;
+    layer->m_descriptor_pool = pool_result.value;

261-307: UI structure is well-organized.

The Shader Controls window layout is logical and the interaction flow is clear. Note that shader_enabled changes (line 266) are handled via direct state mutation while parameter changes use callbacks. This is acceptable if the application polls shader_enabled, but consider adding a callback for consistency if you add other toggles in the future.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 35d5e4d and 37932db.

📒 Files selected for processing (1)
  • src/ui/imgui_layer.cpp
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2026-01-02T07:02:32.720Z 
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Vulkan layer code in `src/capture/vk_layer/` has strict logging constraints: use only `error` and `critical` levels, no file I/O or blocking operations in hot paths, prefix all logs with `[goggles_vklayer]`, never log in `vkQueuePresentKHR` hot path.

Applied to files:

  • src/ui/imgui_layer.cpp
📚 Learning: 2026-01-02T07:02:32.720Z 
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Exception - Capture Layer Code (`src/capture/vk_layer/`) is exempt and MUST use the raw Vulkan C API because layer dispatch tables require C function pointers, layer must intercept C API calls from host applications, and minimal dependencies required.

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-02T07:02:32.720Z 
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Vulkan layer runs in host application threads (no control). Hooks execute in calling thread (usually render thread). No blocking in hooks, especially `vkQueuePresentKHR`. Use atomics or locks for thread-safe shared layer state.

Applied to files:

  • src/ui/imgui_layer.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: Static Analysis (clang-tidy)
  • GitHub Check: Build and Test (test preset)
🔇 Additional comments (7)
src/ui/imgui_layer.cpp (7)

108-121: Cleanup order and error handling look good.

The shutdown sequence properly waits for the device to idle before destroying ImGui resources, and the cleanup order is correct. Note that waitIdle() errors are intentionally discarded during shutdown, which is acceptable since recovery options are limited in a destructor/shutdown context.

141-158: LGTM: Correct dynamic rendering setup for UI overlay.

The rendering configuration properly uses LoadOp::eLoad to render the UI on top of existing content without clearing, which is correct for an overlay.

165-182: Tree building logic is correct.

The preset tree construction properly iterates through path components and creates the hierarchical structure, setting preset_index only for leaf nodes (actual presets).

223-239: Well-implemented filtered preset list.

Good use of std::cmp_equal for safe signed/unsigned comparison and proper ImGui ID scoping. The tooltip showing the full path is a nice UX touch.

241-259: Recursive tree rendering is correctly implemented.

The distinction between leaf nodes (presets) and directory nodes is handled properly, and TreeNode/TreePop pairing is correct.

309-330: Parameter controls implemented correctly.

The defensive check for actual value change (line 321) before invoking the callback is good practice, even though SliderFloat's return value typically indicates user interaction. The tooltip display for parameter descriptions is a nice UX touch.

214-221: No issues found. The search_filter is declared as std::array<char, 256> and is zero-initialized, providing ample space for ImGui::InputTextWithHint to write strings with null-termination. ImGui's InputTextWithHint ensures null-termination of the buffer, making the use of search_filter.data() in to_lower() safe.

qodo-code-review[bot]
qodo-code-review bot reviewed Jan 6, 2026
View reviewed changes
src/app/main.cpp Outdated
Comment on lines +60 to +64
if (!result) {
GOGGLES_LOG_ERROR("Failed to load preset '{}': {}", preset.string(),
result.error().message);
} else {
state.current_preset = preset;
Copy link
Copy Markdown

@qodo-code-review qodo-code-review bot Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Synchronize UI state after reload

Suggested change
if (!result) {
GOGGLES_LOG_ERROR("Failed to load preset '{}': {}", preset.string(),
result.error().message);
} else {
state.current_preset = preset;
} else {
GOGGLES_LOG_INFO("Loaded preset: {}", vulkan_backend.current_preset_path().string());
}
// Always sync UI state with backend state after reload attempt
imgui_layer.set_current_preset(vulkan_backend.current_preset_path());
}

coderabbitai[bot]
coderabbitai bot reviewed Jan 6, 2026
View reviewed changes
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI Agents 
In @src/render/backend/vulkan_backend.cpp:
- Around line 1348-1388: The async shader-load lambda captures this and writes
to members (m_pending_shader_runtime, m_pending_filter_chain,
m_pending_chain_ready), risking use-after-free because shutdown() only calls
m_device->waitIdle() and doesn't wait for m_pending_load_future; fix by making
shutdown() (and the VulkanBackend destructor as a safeguard) wait for the
pending future to finish (check m_pending_load_future.valid() then call wait()
or get()) before allowing member destruction, ensuring the async task cannot
touch freed members.
🧹 Nitpick comments (1)
src/render/backend/vulkan_backend.cpp (1)

1163-1247: Significant code duplication with render_frame() and record_render_commands().

This function duplicates the frame lifecycle logic from render_frame() (lines 1171-1181) and the barrier setup from record_render_commands() (lines 1194-1226). The only difference is the UI callback insertion point.

Consider extracting common logic into a private helper or template to reduce maintenance burden. For example, a record_render_commands_impl that accepts an optional callback could be used by both paths.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 37932db and 0486de7.

📒 Files selected for processing (2)
  • src/render/backend/vulkan_backend.cpp
  • src/render/backend/vulkan_backend.hpp
🧰 Additional context used
🧠 Learnings (14)
📓 Common learnings 
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Vulkan layer code in `src/capture/vk_layer/` has strict logging constraints: use only `error` and `critical` levels, no file I/O or blocking operations in hot paths, prefix all logs with `[goggles_vklayer]`, never log in `vkQueuePresentKHR` hot path.
📚 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/render/backend/vulkan_backend.cpp
  • src/render/backend/vulkan_backend.hpp
📚 Learning: 2026-01-02T07:02:32.720Z 
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/src/**/*.{cpp,hpp} : Due to async GPU execution, RAII handles (`vk::Unique*`) cannot automatically handle synchronization. Use selectively: `vk::Unique*` for Instance, Device, Surface (long-lived singletons), Swapchain, Pipelines, Layouts (created once). Use plain handles for Command buffers, per-frame sync primitives (Fence, Semaphore), Imported external images.

Applied to files:

  • src/render/backend/vulkan_backend.cpp
  • src/render/backend/vulkan_backend.hpp
📚 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/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-02T07:02:32.720Z 
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/tests/**/*.cpp : Only non-GPU logic is tested initially: in scope are utility functions, configuration parsing, error handling, pipeline graph logic. Out of scope: Vulkan initialization, GPU resource management, rendering, capture layer.

Applied to files:

  • src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-02T07:02:32.720Z 
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/src/**/*.{cpp,hpp} : For Vulkan resources, follow RAII guidelines. Use `vk::Unique*` only for appropriate resource types (per D.2 table). Call `device.waitIdle()` or wait on appropriate fences before destroying resources with GPU-async lifetime. Store creation info with resources for debugging. Never leak Vulkan objects. Declare members in reverse destruction order (device before instance).

Applied to files:

  • src/render/backend/vulkan_backend.cpp
  • src/render/backend/vulkan_backend.hpp
📚 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} : Application code MUST use vulkan-hpp with `VULKAN_HPP_NO_EXCEPTIONS` and dynamic dispatch

Applied to files:

  • src/render/backend/vulkan_backend.cpp
  • src/render/backend/vulkan_backend.hpp
📚 Learning: 2026-01-02T07:02:32.720Z 
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Vulkan layer code in `src/capture/vk_layer/` has strict logging constraints: use only `error` and `critical` levels, no file I/O or blocking operations in hot paths, prefix all logs with `[goggles_vklayer]`, never log in `vkQueuePresentKHR` hot path.

Applied to files:

  • src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-02T07:02:32.720Z 
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Vulkan layer runs in host application threads (no control). Hooks execute in calling thread (usually render thread). No blocking in hooks, especially `vkQueuePresentKHR`. Use atomics or locks for thread-safe shared layer state.

Applied to files:

  • src/render/backend/vulkan_backend.cpp
  • src/render/backend/vulkan_backend.hpp
📚 Learning: 2026-01-02T07:02:32.720Z 
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/**/*.{cpp,hpp} : Per-frame code paths (main thread render loop) MUST NOT perform dynamic memory allocations (`new`, `malloc`, `std::make_shared`), use blocking synchronization primitives (`std::mutex`, `std::condition_variable`), or exceed 8ms CPU time budget (excluding GPU sync).

Applied to files:

  • src/render/backend/vulkan_backend.cpp
  • src/render/backend/vulkan_backend.hpp
📚 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/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-02T07:02:32.720Z 
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/src/**/*.{cpp,hpp} : Application code MUST use `vulkan-hpp` (C++ bindings), NOT the raw Vulkan C API. Use `vk::` types (e.g., `vk::Instance`, `vk::Device`, `vk::Image`). Never use raw C handles (`VkInstance`, `VkDevice`, etc.) in application code. Required configuration: `#define VULKAN_HPP_NO_EXCEPTIONS` and `#define VULKAN_HPP_DISPATCH_LOADER_DYNAMIC 1`.

Applied to files:

  • src/render/backend/vulkan_backend.hpp
📚 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/app/**/*.{cpp,hpp} : Do not perform dynamic memory allocations or use blocking synchronization primitives in per-frame main render loop code paths

Applied to files:

  • src/render/backend/vulkan_backend.hpp
📚 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: Default to single-threaded render loop on main thread; use central job system (goggles::util::JobSystem) for any task parallelism

Applied to files:

  • src/render/backend/vulkan_backend.hpp
🧬 Code graph analysis (1)
src/render/backend/vulkan_backend.hpp (4)
src/render/chain/filter_chain.hpp (5)
  • nodiscard (52-52)
  • nodiscard (55-57)
  • preset_path (41-41)
  • enabled (54-54)
  • enabled (54-54)
src/capture/capture_receiver.hpp (7)
  • nodiscard (34-34)
  • nodiscard (35-35)
  • nodiscard (36-36)
  • nodiscard (38-38)
  • nodiscard (39-39)
  • nodiscard (40-42)
  • nodiscard (43-43)
src/util/config.hpp (1)
  • nodiscard (19-31)
src/render/chain/preset_parser.hpp (2)
  • preset_path (59-59)
  • preset_path (62-64)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: Build and Test (test preset)
  • GitHub Check: Static Analysis (clang-tidy)
🔇 Additional comments (9)
src/render/backend/vulkan_backend.cpp (5)

1249-1318: LGTM with the same refactor consideration.

The UI callback integration is correctly placed after the clear rendering and before the present barrier. The implementation is functionally correct. The duplication with render_clear() is similar to the pattern noted above.

1393-1425: LGTM—error handling maintains consistent state.

The error path correctly resets the pending chain state without modifying m_preset_path, ensuring the active preset path remains consistent with the running filter chain. The release-acquire ordering on the atomic flag properly synchronizes access to the pending chain objects.

1427-1435: LGTM!

The deferred destruction logic correctly waits for MAX_FRAMES_IN_FLIGHT + 1 frames before destroying old chains, ensuring GPU work referencing them has completed.

1437-1441: LGTM!

Clean delegation with appropriate null check.

1120-1123: Per-frame dynamic allocation in check_pending_chain_swap().

The call to check_pending_chain_swap() performs m_deferred_destroys.push_back() (line 1411), which triggers dynamic memory allocation on the per-frame render path. Based on learnings, per-frame code paths must not perform dynamic memory allocations.

Consider pre-reserving capacity for m_deferred_destroys during initialization (e.g., reserve 2-4 slots which should cover typical shader reload frequency) to avoid allocations in the hot path.

🔎 Proposed mitigation

In init_filter_chain() or create():

m_deferred_destroys.reserve(4); // Pre-allocate for typical usage
⛔ Skipped due to learnings 
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/**/*.{cpp,hpp} : Per-frame code paths (main thread render loop) MUST NOT perform dynamic memory allocations (`new`, `malloc`, `std::make_shared`), use blocking synchronization primitives (`std::mutex`, `std::condition_variable`), or exceed 8ms CPU time budget (excluding GPU sync).

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/app/**/*.{cpp,hpp} : Do not perform dynamic memory allocations or use blocking synchronization primitives in per-frame main render loop code paths
src/render/backend/vulkan_backend.hpp (4)

51-54: LGTM—std::function callback is appropriate here.

The UiRenderCallback is passed by const reference and invoked once per frame. The type-erasure overhead of std::function is acceptable for this use case where flexibility (supporting lambdas, function pointers, bound methods) outweighs the minor indirection cost.

56-65: LGTM!

Appropriate accessors for ImGui integration. Returning raw pointers for non-owning access (filter_chain()) follows the project conventions.

72-76: LGTM!

The consume-once pattern is appropriate for single-threaded render loop usage.

159-177: LGTM!

The async reload state is well-structured. Using std::atomic<bool> for cross-thread signaling with release-acquire semantics (as implemented in .cpp) is correct. The DeferredDestroy struct properly bundles the chain and runtime with the destruction threshold. Based on learnings, unique_ptr is appropriate for these long-lived shader resources.

coderabbitai[bot]
coderabbitai bot reviewed Jan 6, 2026
View reviewed changes
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Fix all issues with AI Agents 
In @src/render/backend/vulkan_backend.cpp:
- Around line 1321-1392: The async lambda in reload_shader_preset captures this
and writes to members (m_pending_shader_runtime, m_pending_filter_chain,
m_pending_chain_ready) causing potential use-after-free if VulkanBackend is
destroyed while m_pending_load_future is running; modify shutdown() to check
m_pending_load_future.valid() and if so wait on it (e.g., get() or wait())
before destroying resources (after calling m_device->waitIdle()), and add the
same safe-wait in the destructor as a safeguard so the background job finishes
before member destruction.
- Around line 1164-1248: The methods render_frame_with_ui, render_frame, and
render_clear_with_ui duplicate ~80 lines of command recording/submit logic;
refactor by adding an optional UiRenderCallback parameter to
record_render_commands (or create a private do_render_frame helper) and move the
shared logic—acquire_next_image, command buffer reset/begin, image memory
barriers setup, m_filter_chain->record invocation, the conditional UI callback
invocation (ui_callback(cmd, *m_swapchain_image_views[image_index],
m_swapchain_extent)), final barrier, cmd.end and submit_and_present—into that
single helper; then update render_frame, render_frame_with_ui and
render_clear_with_ui to call the new record_render_commands/do_render_frame with
or without the UiRenderCallback so duplicate code is removed.
- Around line 1250-1319: render_clear_with_ui duplicates logic from render_clear
and record_clear_commands; refactor by extracting the shared acquisition,
command buffer setup, image barrier, rendering begin/end and barrier teardown
into a single helper (e.g., VulkanBackend::record_clear_commands or reuse
record_clear_commands) that returns the command buffer and image_index or
accepts a lambda to inject UI work; then have render_clear_with_ui call that
helper, invoke ui_callback(cmd, *m_swapchain_image_views[image_index],
m_swapchain_extent) only when provided, finish with VK_TRY(cmd.end(), ...) and
call submit_and_present(image_index); update render_clear to call the same
helper so duplication is eliminated and ensure symbols referenced include
VulkanBackend::render_clear_with_ui, VulkanBackend::render_clear,
record_clear_commands, ui_callback, submit_and_present, acquire_next_image and
m_frames[m_current_frame].command_buffer.
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0486de7 and 4d2619b.

📒 Files selected for processing (1)
  • src/render/backend/vulkan_backend.cpp
🧰 Additional context used
🧠 Learnings (17)
📓 Common learnings 
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Vulkan layer code in `src/capture/vk_layer/` has strict logging constraints: use only `error` and `critical` levels, no file I/O or blocking operations in hot paths, prefix all logs with `[goggles_vklayer]`, never log in `vkQueuePresentKHR` hot path.
📚 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/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-02T07:02:32.720Z 
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/src/**/*.{cpp,hpp} : Due to async GPU execution, RAII handles (`vk::Unique*`) cannot automatically handle synchronization. Use selectively: `vk::Unique*` for Instance, Device, Surface (long-lived singletons), Swapchain, Pipelines, Layouts (created once). Use plain handles for Command buffers, per-frame sync primitives (Fence, Semaphore), Imported external images.

Applied to files:

  • src/render/backend/vulkan_backend.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/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-02T07:02:32.720Z 
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/tests/**/*.cpp : Only non-GPU logic is tested initially: in scope are utility functions, configuration parsing, error handling, pipeline graph logic. Out of scope: Vulkan initialization, GPU resource management, rendering, capture layer.

Applied to files:

  • src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-02T07:02:32.720Z 
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Vulkan layer code in `src/capture/vk_layer/` has strict logging constraints: use only `error` and `critical` levels, no file I/O or blocking operations in hot paths, prefix all logs with `[goggles_vklayer]`, never log in `vkQueuePresentKHR` hot path.

Applied to files:

  • src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-02T07:02:32.720Z 
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/**/*.{cpp,hpp} : Per-frame code paths (main thread render loop) MUST NOT perform dynamic memory allocations (`new`, `malloc`, `std::make_shared`), use blocking synchronization primitives (`std::mutex`, `std::condition_variable`), or exceed 8ms CPU time budget (excluding GPU sync).

Applied to files:

  • src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-02T07:02:32.720Z 
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/src/**/*.{cpp,hpp} : For Vulkan resources, follow RAII guidelines. Use `vk::Unique*` only for appropriate resource types (per D.2 table). Call `device.waitIdle()` or wait on appropriate fences before destroying resources with GPU-async lifetime. Store creation info with resources for debugging. Never leak Vulkan objects. Declare members in reverse destruction order (device before instance).

Applied to files:

  • src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-02T07:02:32.720Z 
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Vulkan layer runs in host application threads (no control). Hooks execute in calling thread (usually render thread). No blocking in hooks, especially `vkQueuePresentKHR`. Use atomics or locks for thread-safe shared layer state.

Applied to files:

  • src/render/backend/vulkan_backend.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} : Application code MUST use vulkan-hpp with `VULKAN_HPP_NO_EXCEPTIONS` and dynamic dispatch

Applied to files:

  • src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-02T07:02:32.720Z 
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/**/*.{cpp,hpp} : All resources must be managed via RAII. Wrap Vulkan objects in RAII types (custom or from libraries like `vk-bootstrap`, `vulkan-hpp`). Use RAII wrappers for file handles, sockets, etc. (`std::fstream`, `UniqueFd`, custom types). No manual cleanup in destructors unless encapsulated in RAII.

Applied to files:

  • src/render/backend/vulkan_backend.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/app/**/*.{cpp,hpp} : Do not perform dynamic memory allocations or use blocking synchronization primitives in per-frame main render loop code paths

Applied to files:

  • src/render/backend/vulkan_backend.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: Default to single-threaded render loop on main thread; use central job system (goggles::util::JobSystem) for any task parallelism

Applied to files:

  • src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-02T07:02:32.720Z 
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/src/**/*.{cpp,hpp} : All concurrent processing MUST use the project's central job system. Phase 1 library: BS::thread_pool. Phase 2 library: Taskflow. Wrapper interface: `goggles::util::JobSystem`. Direct use of `std::thread` or `std::jthread` for pipeline work is PROHIBITED. Exception: External integration code (networking, IPC) outside real-time path may use `std::jthread` with RAII guarantees.

Applied to files:

  • src/render/backend/vulkan_backend.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/**/*.{cpp,hpp} : Use inter-thread communication via `goggles::util::SPSCQueue` (lock-free SPSC queue) for task parallelism

Applied to files:

  • src/render/backend/vulkan_backend.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/tests/**/*.{cpp,hpp} : Focus unit tests on non-GPU logic: utility functions, configuration parsing, and pipeline graph logic

Applied to files:

  • src/render/backend/vulkan_backend.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/render/backend/vulkan_backend.cpp
🧬 Code graph analysis (1)
src/render/backend/vulkan_backend.cpp (13)
src/render/backend/vulkan_backend.hpp (10)
  • frame (38-38)
  • frame (52-53)
  • frame (95-95)
  • ui_callback (54-54)
  • image_index (103-103)
  • cmd (99-100)
  • cmd (101-102)
  • preset_path (42-42)
  • preset_path (43-44)
  • enabled (49-49)
src/util/error.hpp (1)
  • make_error (50-52)
src/render/chain/filter_chain.hpp (6)
  • cmd (43-46)
  • cmd (76-76)
  • preset_path (41-41)
  • vk_ctx (29-32)
  • enabled (54-54)
  • enabled (54-54)
src/ui/imgui_layer.hpp (1)
  • cmd (66-66)
src/render/chain/filter_pass.hpp (2)
  • cmd (50-50)
  • vk_ctx (39-40)
src/render/chain/output_pass.hpp (2)
  • cmd (28-28)
  • vk_ctx (17-18)
src/render/chain/pass.hpp (1)
  • cmd (44-44)
src/render/texture/texture_loader.hpp (1)
  • cmd (59-60)
src/render/chain/preset_parser.hpp (2)
  • preset_path (59-59)
  • preset_path (62-64)
src/capture/vk_layer/wsi_virtual.hpp (2)
  • device (65-66)
  • device (67-67)
src/render/chain/framebuffer.hpp (1)
  • device (10-12)
src/render/chain/filter_chain.cpp (2)
  • create (52-77)
  • create (52-54)
src/render/shader/shader_runtime.cpp (2)
  • create (190-257)
  • create (190-190)
🔇 Additional comments (5)
src/render/backend/vulkan_backend.cpp (5)

9-9: LGTM: JobSystem integration follows project policy.

The include is necessary for async shader compilation at line 1349.

Based on learnings, all concurrent processing must use the project's central job system, which this change correctly implements.

1121-1123: LGTM: Frame lifecycle management for async shader reload.

These operations correctly manage the async shader reload state: incrementing the frame counter for deferred destruction timing, checking for completed async shader compilations, and cleaning up resources that are safe to destroy.

The ordering is appropriate: counter increment, pending swap check, then cleanup.

1394-1426: LGTM: Correct pending chain swap with deferred destruction.

The acquire-release memory ordering (line 1395 ↔ line 1384 in reload_shader_preset) ensures visibility of the pending resources before swapping. The future.get() call at line 1401 is safe because the flag guarantees task completion.

The deferred destruction timing (m_frame_count + MAX_FRAMES_IN_FLIGHT + 1) correctly ensures GPU-async resources are no longer in use before destruction, aligning with project RAII policies for Vulkan resources.

Based on learnings, this approach properly handles the GPU-async resource lifetime constraint.

1428-1436: LGTM: Clean deferred destruction cleanup.

The std::erase_if pattern correctly removes entries when the frame delay has elapsed, triggering RAII destruction of the old filter chain and shader runtime.

1438-1442: LGTM: Straightforward shader toggle.

The bypass flag inversion correctly maps the enabled state to the filter chain's bypass behavior.

coderabbitai[bot]
coderabbitai bot reviewed Jan 6, 2026
View reviewed changes
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI Agents 
In @src/render/backend/vulkan_backend.cpp:
- Around line 1398-1430: The push_back on m_deferred_destroys inside
VulkanBackend::check_pending_chain_swap can allocate during per-frame execution;
change this to avoid dynamic allocations by either pre-reserving capacity for
m_deferred_destroys (call reserve(...) in the VulkanBackend constructor or
init_filter_chain) or replace m_deferred_destroys with a fixed-size ring buffer
(e.g., std::array<DeferredDestroy, N> plus a size/index counter like
m_deferred_count) and write into that buffer instead of calling push_back,
ensuring deferred entries are safely wrapped/overwritten or rejected; update all
code that reads m_deferred_destroys accordingly.
🧹 Nitpick comments (1)
src/render/backend/vulkan_backend.cpp (1)

1432-1440: Consider per-frame performance of std::erase_if.

While std::erase_if primarily deallocates, it still performs linear scanning and element shifting on each frame. If m_deferred_destroys grows large during rapid preset changes, this could impact the per-frame CPU budget (8ms constraint).

Consider tracking the removal index during insertion and processing deletions in O(1) time, or move cleanup to a separate timer-based tick outside the frame loop.

Based on learnings, per-frame paths should not exceed the 8ms CPU time budget.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4d2619b and 96ca4b7.

📒 Files selected for processing (1)
  • src/render/backend/vulkan_backend.cpp
🧰 Additional context used
🧠 Learnings (17)
📓 Common learnings 
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Vulkan layer code in `src/capture/vk_layer/` has strict logging constraints: use only `error` and `critical` levels, no file I/O or blocking operations in hot paths, prefix all logs with `[goggles_vklayer]`, never log in `vkQueuePresentKHR` hot path.
📚 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/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-02T07:02:32.720Z 
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/src/**/*.{cpp,hpp} : Due to async GPU execution, RAII handles (`vk::Unique*`) cannot automatically handle synchronization. Use selectively: `vk::Unique*` for Instance, Device, Surface (long-lived singletons), Swapchain, Pipelines, Layouts (created once). Use plain handles for Command buffers, per-frame sync primitives (Fence, Semaphore), Imported external images.

Applied to files:

  • src/render/backend/vulkan_backend.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/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-02T07:02:32.720Z 
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/tests/**/*.cpp : Only non-GPU logic is tested initially: in scope are utility functions, configuration parsing, error handling, pipeline graph logic. Out of scope: Vulkan initialization, GPU resource management, rendering, capture layer.

Applied to files:

  • src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-02T07:02:32.720Z 
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/src/**/*.{cpp,hpp} : For Vulkan resources, follow RAII guidelines. Use `vk::Unique*` only for appropriate resource types (per D.2 table). Call `device.waitIdle()` or wait on appropriate fences before destroying resources with GPU-async lifetime. Store creation info with resources for debugging. Never leak Vulkan objects. Declare members in reverse destruction order (device before instance).

Applied to files:

  • src/render/backend/vulkan_backend.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} : Application code MUST use vulkan-hpp with `VULKAN_HPP_NO_EXCEPTIONS` and dynamic dispatch

Applied to files:

  • src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-02T07:02:32.720Z 
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Vulkan layer runs in host application threads (no control). Hooks execute in calling thread (usually render thread). No blocking in hooks, especially `vkQueuePresentKHR`. Use atomics or locks for thread-safe shared layer state.

Applied to files:

  • src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-02T07:02:32.720Z 
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/**/*.{cpp,hpp} : Per-frame code paths (main thread render loop) MUST NOT perform dynamic memory allocations (`new`, `malloc`, `std::make_shared`), use blocking synchronization primitives (`std::mutex`, `std::condition_variable`), or exceed 8ms CPU time budget (excluding GPU sync).

Applied to files:

  • src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-02T07:02:32.720Z 
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/src/capture/vk_layer/**/*.{cpp,hpp} : Vulkan layer code in `src/capture/vk_layer/` has strict logging constraints: use only `error` and `critical` levels, no file I/O or blocking operations in hot paths, prefix all logs with `[goggles_vklayer]`, never log in `vkQueuePresentKHR` hot path.

Applied to files:

  • src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-02T07:02:32.720Z 
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/**/*.{cpp,hpp} : All resources must be managed via RAII. Wrap Vulkan objects in RAII types (custom or from libraries like `vk-bootstrap`, `vulkan-hpp`). Use RAII wrappers for file handles, sockets, etc. (`std::fstream`, `UniqueFd`, custom types). No manual cleanup in destructors unless encapsulated in RAII.

Applied to files:

  • src/render/backend/vulkan_backend.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/app/**/*.{cpp,hpp} : Do not perform dynamic memory allocations or use blocking synchronization primitives in per-frame main render loop code paths

Applied to files:

  • src/render/backend/vulkan_backend.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: Default to single-threaded render loop on main thread; use central job system (goggles::util::JobSystem) for any task parallelism

Applied to files:

  • src/render/backend/vulkan_backend.cpp
📚 Learning: 2026-01-02T07:02:32.720Z 
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: docs/project_policies.md:0-0
Timestamp: 2026-01-02T07:02:32.720Z
Learning: Applies to docs/src/**/*.{cpp,hpp} : All concurrent processing MUST use the project's central job system. Phase 1 library: BS::thread_pool. Phase 2 library: Taskflow. Wrapper interface: `goggles::util::JobSystem`. Direct use of `std::thread` or `std::jthread` for pipeline work is PROHIBITED. Exception: External integration code (networking, IPC) outside real-time path may use `std::jthread` with RAII guarantees.

Applied to files:

  • src/render/backend/vulkan_backend.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/**/*.{cpp,hpp} : Use inter-thread communication via `goggles::util::SPSCQueue` (lock-free SPSC queue) for task parallelism

Applied to files:

  • src/render/backend/vulkan_backend.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/tests/**/*.{cpp,hpp} : Focus unit tests on non-GPU logic: utility functions, configuration parsing, and pipeline graph logic

Applied to files:

  • src/render/backend/vulkan_backend.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/render/backend/vulkan_backend.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: Static Analysis (clang-tidy)
  • GitHub Check: Build and Test (test preset)
🔇 Additional comments (4)
src/render/backend/vulkan_backend.cpp (4)

90-93: LGTM—Async task safety addressed.

The addition of the future wait in shutdown() correctly prevents use-after-free when the async shader-load task (lines 1353-1393) writes to m_pending_shader_runtime and m_pending_filter_chain. This ensures the background job completes before member destruction.

1325-1396: Well-structured async preset reload.

The implementation correctly uses util::JobSystem (per project policy) and safely captures values for the async task. The atomic m_pending_chain_ready flag with acquire/release ordering ensures proper synchronization. The earlier fix in shutdown() (lines 90-93) prevents use-after-free when writing to pending members (lines 1386-1388).

Based on learnings, using the central job system for concurrent processing.

1442-1446: LGTM—Clean shader enable/disable toggle.

Simple passthrough to the filter chain's bypass flag. Implementation is straightforward and correct.

508-508: Remove this comment — m_format_changed is properly consumed.

The flag is read and consumed by the public consume_format_change() method (lines 72-76 in vulkan_backend.hpp), which returns its value and resets it. This is a standard pattern for signaling state changes to external code.

BANANASJIM and others added 5 commits January 6, 2026 11:41
@BANANASJIM
perf: use fixed-size array for deferred destroys
ccabfcf
@BANANASJIM
feat(ui): expose shader parameters via FilterChain API
2761653
@BANANASJIM
fix: handle UBO update errors and skip dummy parameters
fe248a5
@BANANASJIM
refactor: async reload and render method deduplication
e938126 
- Add consume_chain_swapped() for UI parameter sync after chain swap
- Refactor record_*_commands to accept optional UI callback
- Use 3s timeout in shutdown for pending async tasks
- Make m_chain_swapped atomic for thread safety
- Update OpenSpec documentation
@BANANASJIM
Merge branch 'main' into feature/add-imgui-preset-reload
03d628c
coderabbitai[bot]
coderabbitai bot reviewed Jan 7, 2026
View reviewed changes
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Fix all issues with AI agents 
In @src/render/backend/vulkan_backend.hpp:
- Line 165: m_format_changed is a plain bool and can race with other threads;
change its declaration to std::atomic<bool> (matching m_chain_swapped) and
update any access sites—especially consume_format_change() and places like
check_pending_chain_swap() or reload logic—to use atomic operations
(load/exchange/compare_exchange as appropriate) rather than non-atomic
reads/writes so reads/writes are thread-safe and consistent across threads.

In @src/ui/imgui_layer.cpp:
- Line 110: The call static_cast<void>(m_device.waitIdle()) suppresses the
vk::Result; replace it with explicit error handling for m_device.waitIdle():
either invoke the VK_TRY macro on m_device.waitIdle() (e.g.,
VK_TRY(m_device.waitIdle())) or capture the vk::Result r = m_device.waitIdle()
and check if r != vk::Result::eSuccess then handle appropriately
(log/error/abort) consistent with project policy; ensure you reference
m_device.waitIdle() and vk::Result in the fix so the return value is never
ignored.
🧹 Nitpick comments (2)
src/render/chain/filter_chain.cpp (1)

517-532: Inefficient parameter update - consider optimizing pass iteration.

The current implementation calls set_parameter_override and update_ubo_parameters on ALL passes, even those that don't declare the parameter. This wastes CPU cycles on passes that don't use the parameter.

While this occurs only during user interaction (not per-frame), it could be optimized to update only the passes that declare the parameter.

♻️ Suggested optimization 
 void FilterChain::set_parameter(const std::string& name, float value) {
     bool found = false;
     for (auto& pass : m_passes) {
+        bool param_exists = false;
         for (const auto& param : pass->parameters()) {
             if (param.name == name) {
                 found = true;
+                param_exists = true;
                 break;
             }
         }
-        pass->set_parameter_override(name, value);
-        GOGGLES_MUST(pass->update_ubo_parameters());
+        if (param_exists) {
+            pass->set_parameter_override(name, value);
+            GOGGLES_MUST(pass->update_ubo_parameters());
+        }
     }
     if (!found) {
         GOGGLES_LOG_WARN("set_parameter: '{}' not found in any pass", name);
     }
 }
src/render/backend/vulkan_backend.hpp (1)

175-183: Deferred destruction design is sound; consider overflow handling.

The fixed-size m_deferred_destroys array with MAX_DEFERRED_DESTROYS = 4 is appropriate for typical usage. However, if rapid consecutive reloads exceed this limit before old resources are cleaned up (e.g., user spamming reload), you could silently drop destroy requests or overwrite active entries.

Consider adding overflow detection in the implementation (e.g., assert or log a warning if m_deferred_count >= MAX_DEFERRED_DESTROYS).

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2761653 and 03d628c.

📒 Files selected for processing (9)
  • cmake/Dependencies.cmake
  • openspec/changes/add-imgui-runtime-preset-control/design.md
  • openspec/changes/add-imgui-runtime-preset-control/tasks.md
  • src/app/main.cpp
  • src/render/backend/vulkan_backend.cpp
  • src/render/backend/vulkan_backend.hpp
  • src/render/chain/filter_chain.cpp
  • src/render/chain/filter_pass.cpp
  • src/ui/imgui_layer.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/render/chain/filter_pass.cpp
  • cmake/Dependencies.cmake
🧰 Additional context used
📓 Path-based instructions (1)
openspec/**/*.md

📄 CodeRabbit inference engine (openspec/AGENTS.md)

openspec/**/*.md: Use #### Scenario: format (4 hashtags) for scenario headers in requirements, not bullets or bold text, with at least one scenario per requirement
Use format - **WHEN** [condition] - **THEN** [expected result] for scenario steps in requirements

Files:

  • openspec/changes/add-imgui-runtime-preset-control/tasks.md
  • openspec/changes/add-imgui-runtime-preset-control/design.md
🧠 Learnings (29)
📓 Common learnings 
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.
📚 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: Testing scope: Only non-GPU logic tested initially. In scope: utility functions, configuration parsing, error handling, pipeline graph logic. Out of scope: Vulkan initialization, GPU resource management, rendering, capture layer.

Applied to files:

  • openspec/changes/add-imgui-runtime-preset-control/tasks.md
  • openspec/changes/add-imgui-runtime-preset-control/design.md
  • src/app/main.cpp
  • src/render/backend/vulkan_backend.cpp
  • src/render/backend/vulkan_backend.hpp
📚 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:

  • openspec/changes/add-imgui-runtime-preset-control/tasks.md
  • src/app/main.cpp
  • src/render/backend/vulkan_backend.cpp
  • src/render/backend/vulkan_backend.hpp
📚 Learning: 2025-12-19T08:54:18.412Z 
Learnt from: CR
Repo: K1ngst0m/Goggles PR: 0
File: openspec/AGENTS.md:0-0
Timestamp: 2025-12-19T08:54:18.412Z
Learning: Create a `design.md` file only when the change is cross-cutting (multiple services/modules), introduces new external dependencies, has security/performance/migration complexity, or has ambiguity requiring technical decisions

Applied to files:

  • openspec/changes/add-imgui-runtime-preset-control/design.md
📚 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/**/{pixi.toml,pixi.lock} : Use Pixi as the single source of truth for dependencies. Use `pixi.toml` + `pixi.lock` for toolchains and third-party libraries. Local packages pulled into Pixi environment by path for pinned/forked builds. System packages acceptable only when not in Pixi and not practical to vendor; must be documented.

Applied to files:

  • openspec/changes/add-imgui-runtime-preset-control/design.md
📚 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/**/{pixi.toml,pixi.lock} : All dependencies must have explicit versions. Pin versions in `pixi.toml` and commit `pixi.lock`. Pin local packages by repository state or explicit version. Document rationale for version changes in commit messages.

Applied to files:

  • openspec/changes/add-imgui-runtime-preset-control/design.md
📚 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: Before adding new dependencies: (1) Justify necessity, (2) Check license compatibility, (3) Assess maintenance status, (4) Consider alternatives, (5) Discuss with team. Selection criteria: Prefer Pixi packages, use local package for pinned forks, use system packages only when necessary and documented.

Applied to files:

  • openspec/changes/add-imgui-runtime-preset-control/design.md
📚 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 : Single-threaded render loop on main thread by default. Render backend (`goggles::render`) runs on main thread. Pipeline execution runs on main thread. Job system for non-render work on worker threads. Threading introduced only when profiling justifies it (main thread CPU consistently >8ms).

Applied to files:

  • src/app/main.cpp
  • src/render/backend/vulkan_backend.cpp
  • src/render/backend/vulkan_backend.hpp
📚 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/app/main.cpp
  • src/render/backend/vulkan_backend.cpp
  • src/render/backend/vulkan_backend.hpp
📚 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/src/capture/vk_layer/**/*.{cpp,hpp} : Vulkan capture layer logging constraints: Use only `error` and `critical` levels. No file I/O or blocking operations in hot paths. Prefix all logs with `[goggles_vklayer]`. Never log in `vkQueuePresentKHR` hot path.

Applied to files:

  • src/app/main.cpp
  • 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/app/main.cpp
  • src/render/backend/vulkan_backend.cpp
  • src/render/chain/filter_chain.cpp
  • src/render/backend/vulkan_backend.hpp
📚 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/app/main.cpp
  • src/render/backend/vulkan_backend.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/app/main.cpp
  • src/render/backend/vulkan_backend.cpp
  • src/render/chain/filter_chain.cpp
  • src/render/backend/vulkan_backend.hpp
  • 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/app/main.cpp
  • 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/app/main.cpp
  • 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/app/main.cpp
  • src/render/backend/vulkan_backend.cpp
  • src/render/backend/vulkan_backend.hpp
📚 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/app/main.cpp
  • src/render/backend/vulkan_backend.cpp
  • src/render/backend/vulkan_backend.hpp
📚 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 : Initialize logger once at application startup for console output (development) or file output (optional). For capture layer, initialize logger lazily on first `vkCreateInstance` hook.

Applied to files:

  • src/app/main.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/render/backend/vulkan_backend.cpp
  • src/render/backend/vulkan_backend.hpp
  • 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/render/backend/vulkan_backend.cpp
  • src/render/chain/filter_chain.cpp
  • src/render/backend/vulkan_backend.hpp
📚 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/render/backend/vulkan_backend.cpp
  • src/render/backend/vulkan_backend.hpp
📚 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,hpp} : All resources must be managed via RAII. Wrap Vulkan objects in RAII types. Use RAII wrappers for file handles and sockets. No manual cleanup in destructors unless encapsulated in RAII.

Applied to files:

  • src/render/backend/vulkan_backend.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/app/**/*.{cpp,hpp} : Do not perform dynamic memory allocations or use blocking synchronization primitives in per-frame main render loop code paths

Applied to files:

  • src/render/backend/vulkan_backend.cpp
  • src/render/chain/filter_chain.cpp
  • src/render/backend/vulkan_backend.hpp
📚 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,hpp} : Object initialization policy: Use factory pattern `static auto create(...) -> ResultPtr<T>` for: (1) Manager/singleton objects (VulkanBackend, FilterChain), (2) Objects owning multiple interdependent resources, (3) Objects where uninitialized state causes undefined behavior. Banned pattern: No two-phase initialization (no constructor + `init()`).

Applied to files:

  • src/render/backend/vulkan_backend.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} : Application code MUST use vulkan-hpp with `VULKAN_HPP_NO_EXCEPTIONS` and dynamic dispatch

Applied to files:

  • src/render/backend/vulkan_backend.cpp
  • src/render/backend/vulkan_backend.hpp
📚 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: Default to single-threaded render loop on main thread; use central job system (goggles::util::JobSystem) for any task parallelism

Applied to files:

  • src/render/backend/vulkan_backend.cpp
  • src/render/backend/vulkan_backend.hpp
📚 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 : All concurrent processing MUST use the project's central job system (`goggles::util::JobSystem`). Phase 1: BS::thread_pool. Phase 2: Taskflow. Direct use of `std::thread` or `std::jthread` for pipeline work is PROHIBITED. Exception: External integration code (networking, IPC) outside real-time path may use `std::jthread` with RAII.

Applied to files:

  • src/render/backend/vulkan_backend.cpp
  • src/render/chain/filter_chain.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,hpp} : All real-time inter-thread communication MUST use `goggles::util::SPSCQueue` for fixed producer-consumer pairs (wait-free guarantee). Never use MPMC queues in latency-critical paths. Operations must be non-blocking (`try_push`, `try_pop`). Use pointer passing with pre-allocated buffers.

Applied to files:

  • src/render/chain/filter_chain.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/**/*.{cpp,hpp} : Use inter-thread communication via `goggles::util::SPSCQueue` (lock-free SPSC queue) for task parallelism

Applied to files:

  • src/render/chain/filter_chain.cpp
🧬 Code graph analysis (4)
src/render/backend/vulkan_backend.cpp (5)
src/render/backend/vulkan_backend.hpp (9)
  • cmd (103-105)
  • cmd (106-108)
  • image_index (109-109)
  • ui_callback (54-54)
  • frame (38-38)
  • frame (52-53)
  • frame (99-99)
  • preset_path (42-42)
  • preset_path (43-44)
src/render/chain/filter_chain.hpp (4)
  • cmd (53-56)
  • cmd (91-91)
  • preset_path (51-51)
  • vk_ctx (39-42)
src/util/error.hpp (1)
  • make_error (50-52)
src/render/chain/preset_parser.hpp (2)
  • preset_path (59-59)
  • preset_path (62-64)
src/render/chain/framebuffer.hpp (1)
  • device (10-12)
src/render/chain/filter_chain.cpp (2)
src/render/chain/filter_chain.hpp (3)
  • pass (89-90)
  • name (75-75)
  • name (76-76)
src/render/chain/filter_pass.hpp (5)
  • name (74-76)
  • name (74-74)
  • name (79-81)
  • name (79-79)
  • name (87-87)
src/render/backend/vulkan_backend.hpp (8)
src/render/chain/filter_pass.hpp (3)
  • nodiscard (84-86)
  • nodiscard (92-94)
  • cmd (50-50)
src/util/config.hpp (1)
  • nodiscard (19-31)
src/capture/capture_receiver.hpp (7)
  • nodiscard (34-34)
  • nodiscard (35-35)
  • nodiscard (36-36)
  • nodiscard (38-38)
  • nodiscard (39-39)
  • nodiscard (40-42)
  • nodiscard (43-43)
src/render/chain/preset_parser.hpp (2)
  • preset_path (59-59)
  • preset_path (62-64)
src/capture/vk_layer/ipc_socket.hpp (1)
  • frame_ready_fd (21-21)
src/ui/imgui_layer.hpp (1)
  • cmd (66-66)
src/render/chain/frame_history.hpp (1)
  • cmd (19-19)
src/render/chain/output_pass.hpp (1)
  • cmd (28-28)
src/ui/imgui_layer.cpp (1)
src/util/error.hpp (2)
  • make_result_ptr_error (63-64)
  • make_result_ptr (57-57)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: Build and Test (test preset)
  • GitHub Check: Static Analysis (clang-tidy)
🔇 Additional comments (23)
src/render/chain/filter_chain.cpp (4)

446-449: LGTM - Good defensive check.

The guard prevents resize operations when no source has been rendered yet, avoiding unnecessary framebuffer allocations and potential issues with zero-sized extents.

489-515: LGTM - Parameter collection implementation is sound.

The method correctly deduplicates parameters by name across passes and provides current values for UI display. The allocations (map and vector) are acceptable since this is only called at startup and after chain swaps, not in per-frame paths.

534-544: LGTM - Parameter reset correctly handles multiple passes.

The implementation correctly resets the parameter to its default value in all passes that declare it. The inner break (Line 540) exits the parameter iteration once found, while continuing to check remaining passes.

546-551: LGTM - Clean implementation of parameter override reset.

The method correctly clears all parameter overrides across all passes and updates the UBOs accordingly. Called only from UI reset action, so the iteration overhead is acceptable.

src/app/main.cpp (5)

22-36: LGTM - Well-implemented preset scanning.

The function safely scans for preset files using error codes to avoid exceptions, and is called only once at startup. The filesystem iteration overhead is acceptable in this context.

38-65: LGTM - Efficient parameter state synchronization.

The helper correctly converts filter chain parameters to UI state and uses std::move to efficiently transfer ownership. Called only at initialization and after chain swaps, not in per-frame paths.

67-93: LGTM - Clean UI state handling with proper validation.

The function correctly tracks state changes using a static variable, validates preset indices, and handles errors appropriately. The reload is triggered only on user action, not per-frame.

108-163: LGTM - Proper ImGui event handling and input capture logic.

The event processing correctly forwards events to ImGui first, then conditionally forwards to the game input based on capture state. This prevents input conflicts when the UI is active.

202-241: LGTM - Well-structured ImGui frame lifecycle integration.

The per-frame UI integration correctly follows ImGui's begin/end lifecycle, with the UI callback appropriately placed during command recording. The chain swap notification (Line 237) properly updates UI state after asynchronous preset loads complete.

src/render/backend/vulkan_backend.cpp (7)

919-982: LGTM - Clean UI callback integration into render command recording.

The optional UI callback parameter elegantly resolves the code duplication issue flagged in previous reviews. The callback is correctly invoked after filter chain rendering and before the final present barrier.

984-1041: LGTM - Consistent UI callback integration pattern.

The implementation mirrors the render path with the same clean optional callback approach, resolving the duplication issue from previous reviews.

1179-1207: LGTM - render_frame_with_ui correctly integrates UI rendering.

The implementation properly extends the standard render path with UI callback support while maintaining the same frame lifecycle and error handling.

1209-1226: LGTM - render_clear_with_ui maintains consistency.

The clear path correctly includes frame lifecycle management and UI callback integration, maintaining consistency with the render path.

1228-1299: LGTM - Async reload implementation with proper lifecycle management.

The async shader preset loading is well-designed:

  • Early returns prevent overlapping reloads (Lines 1235-1244)
  • Value captures isolate the async task from state changes
  • The shutdown wait (Lines 90-96) ensures task completion before destruction
  • Ready flag synchronization is correct with release/acquire semantics

1301-1356: LGTM - Per-frame chain swap and cleanup adhere to allocation constraints.

Both methods correctly avoid dynamic allocations:

  • check_pending_chain_swap: Uses a fixed-size deferred destroy array, resolving the allocation concern from previous reviews
  • cleanup_deferred_destroys: Performs in-place array compaction without allocations

The atomic synchronization ensures the future is ready before calling .get(), preventing blocking in the per-frame path. Based on learnings, this complies with main thread constraints.

1358-1362: LGTM - Simple and clear shader enable/disable toggle.

The method correctly translates the "enabled" semantic to the filter chain's "bypass" flag with appropriate null safety.

src/ui/imgui_layer.cpp (5)

85-90: Mixing Vulkan C and C++ API for ImGui compatibility.

Line 85 casts vk::Format to VkFormat for the C-style VkPipelineRenderingCreateInfo struct required by ImGui's C API (ImGui_ImplVulkan_Init). This is a necessary compatibility layer and is acceptable here, but be aware that the project otherwise mandates vulkan-hpp (vk:: types) per coding guidelines.

27-102: Initialization logic is correct and follows best practices.

The factory method properly:

  • Uses structured bindings to check vk::Result (line 65-66)
  • Creates the descriptor pool with appropriate flags and pool sizes for ImGui
  • Initializes ImGui with dynamic rendering support (UseDynamicRendering = true)
  • Cleans up resources on failure paths
  • Logs success at line 100

141-158: UI rendering integration with dynamic rendering is implemented correctly.

The record() method:

  • Uses vk::AttachmentLoadOp::eLoad to preserve underlying content (line 145), allowing UI to overlay on top of the rendered frame
  • Properly sets up vk::RenderingInfo for dynamic rendering (lines 148-153)
  • Calls ImGui_ImplVulkan_RenderDrawData within the render pass (line 156)

This is the correct pattern for ImGui integration with Vulkan dynamic rendering.

165-182: Preset tree construction is sound but relatively complex.

The rebuild_preset_tree() method constructs a hierarchical tree from flat file paths. The nested loop (lines 172-180) iterates over path components and builds directory/file nodes. While the logic is correct, the complexity is O(n × depth) where n is the preset count and depth is the average directory depth.

This is acceptable since it's not called per-frame (only on catalog updates). If preset counts grow very large (>1000s), consider caching or incremental updates.

319-322: Dummy parameter skip logic is correct with >= condition.

The condition param.info.min_value >= param.info.max_value correctly identifies both dummy/separator parameters (where min == max) and malformed ranges (where min > max), preventing invalid sliders. The comment at line 318 mentions "min == max" but the code defensively handles the broader case.

src/render/backend/vulkan_backend.hpp (2)

65-65: API design allows pointer caching but current usage is safe due to deferred destruction.

The filter_chain() method returns a raw pointer to the active FilterChain. While check_pending_chain_swap() can replace m_filter_chain during async reloads, the deferred destruction mechanism queues old chains for cleanup MAX_FRAMES_IN_FLIGHT + 1 frames later, keeping them valid during that window. All current usage patterns get the pointer immediately and use it within the same scope—the lambdas in callbacks capture the backend reference rather than caching pointers.

However, this API design is error-prone and invites misuse (e.g., storing the pointer for later use). Consider documenting the pointer's lifetime constraint (valid only until the next call to check_pending_chain_swap()) or returning std::shared_ptr<FilterChain> to make the safety semantics explicit.

168-173: Async reload design is correct and maintains main thread safety.

The implementation properly addresses the concerns:

  1. No blocking in per-frame paths: The m_pending_load_future.get() at line 1308 is safe because m_pending_chain_ready is set to true (line 1292) only after the async task completes. When check_pending_chain_swap() is called in render_frame(), the future is already ready and get() returns immediately without blocking.

  2. No per-frame allocations: The std::move operations in check_pending_chain_swap() perform no allocations. Old resources are queued for deferred destruction via m_deferred_destroys, ensuring their destructors run only after MAX_FRAMES_IN_FLIGHT frames have passed, safely outside the critical timing window.

  3. Proper synchronization: The atomic flag with acquire/release semantics ensures the main thread sees the completed async work before swapping chains.

The pattern is well-designed for background shader compilation without impacting the per-frame budget.

@K1ngst0m K1ngst0m assigned K1ngst0m and BANANASJIM and unassigned K1ngst0m Jan 7, 2026
@K1ngst0m K1ngst0m merged commit fe0ffa7 into main Jan 7, 2026
4 checks passed
@K1ngst0m K1ngst0m deleted the feature/add-imgui-preset-reload branch January 7, 2026 15:14
This was referenced Jan 8, 2026
Merged
Merged
Merged
This was referenced Jan 24, 2026
Merged
Merged
@coderabbitai coderabbitai bot mentioned this pull request Feb 8, 2026
Merged
This was referenced Mar 3, 2026
Merged
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments

@qodo-code-review qodo-code-review[bot] qodo-code-review[bot] left review comments

Assignees

@BANANASJIM BANANASJIM

Labels

Review effort 3/5

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@K1ngst0m @BANANASJIM