Skip to content

refactor(render): reduce VulkanBackend forwarding surface #141

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Merged
K1ngst0m merged 1 commit into from
Mar 25, 2026
Merged

refactor(render): reduce VulkanBackend forwarding surface #141

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 33 additions & 25 deletions src/app/application.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,8 @@ static auto scan_presets(const std::filesystem::path& dir) -> std::vector<std::f

static void update_ui_parameters(render::VulkanBackend& vulkan_backend,
ui::ImGuiLayer& imgui_layer) {
auto controls = vulkan_backend.list_filter_controls(goggles::fc::FilterControlStage::effect);
auto controls = vulkan_backend.filter_chain_controller().list_filter_controls(
goggles::fc::FilterControlStage::effect);
std::vector<ui::ParameterState> ui_params;
ui_params.reserve(controls.size());

Expand Down Expand Up @@ -115,13 +116,13 @@ auto Application::init_vulkan_backend(const Config& config, const util::AppDirs&

auto Application::init_imgui_layer(const util::AppDirs& app_dirs) -> Result<void> {
ui::ImGuiConfig imgui_config{
.instance = m_vulkan_backend->instance(),
.physical_device = m_vulkan_backend->physical_device(),
.device = m_vulkan_backend->device(),
.queue_family = m_vulkan_backend->graphics_queue_family(),
.queue = m_vulkan_backend->graphics_queue(),
.swapchain_format = m_vulkan_backend->swapchain_format(),
.image_count = m_vulkan_backend->swapchain_image_count(),
.instance = m_vulkan_backend->vulkan_context().instance,
.physical_device = m_vulkan_backend->vulkan_context().physical_device,
.device = m_vulkan_backend->vulkan_context().device,
.queue_family = m_vulkan_backend->vulkan_context().graphics_queue_family,
.queue = m_vulkan_backend->vulkan_context().graphics_queue,
.swapchain_format = m_vulkan_backend->render_output().swapchain_format,
.image_count = m_vulkan_backend->render_output().image_count(),
};

m_imgui_layer = GOGGLES_MUST(ui::ImGuiLayer::create(m_window, imgui_config, app_dirs));
Expand All @@ -142,16 +143,18 @@ auto Application::init_shader_system(const Config& config, const util::AppDirs&
GOGGLES_LOG_INFO("Preset catalog directory: {}", preset_dir.string());

m_imgui_layer->set_preset_catalog(scan_presets(preset_dir));
m_imgui_layer->set_current_preset(m_vulkan_backend->current_preset_path());
m_imgui_layer->set_current_preset(
m_vulkan_backend->filter_chain_controller().current_preset_path());
m_imgui_layer->state().shader_enabled = !config.shader.preset.empty();

m_imgui_layer->set_parameter_change_callback(
[&backend = *m_vulkan_backend](goggles::fc::FilterControlId control_id, float value) {
static_cast<void>(backend.set_filter_control_value(control_id, value));
static_cast<void>(
backend.filter_chain_controller().set_filter_control_value(control_id, value));
});
m_imgui_layer->set_parameter_reset_callback(
[&backend = *m_vulkan_backend, layer = m_imgui_layer.get()]() {
backend.reset_filter_controls();
backend.filter_chain_controller().reset_filter_controls();
update_ui_parameters(backend, *layer);
});
m_imgui_layer->set_prechain_change_callback(
Expand All @@ -160,18 +163,20 @@ auto Application::init_shader_system(const Config& config, const util::AppDirs&
});
m_imgui_layer->set_prechain_parameter_callback(
[&backend = *m_vulkan_backend](goggles::fc::FilterControlId control_id, float value) {
static_cast<void>(backend.set_filter_control_value(control_id, value));
static_cast<void>(
backend.filter_chain_controller().set_filter_control_value(control_id, value));
});
m_imgui_layer->set_prechain_scale_mode_callback([this](ScaleMode mode, uint32_t integer_scale) {
m_vulkan_backend->set_scale_mode(mode);
m_vulkan_backend->set_integer_scale(integer_scale);
});

auto prechain_res = m_vulkan_backend->get_prechain_resolution();
auto prechain_res = m_vulkan_backend->filter_chain_controller().current_prechain_resolution();
m_imgui_layer->set_prechain_state(prechain_res, m_vulkan_backend->get_scale_mode(),
m_vulkan_backend->get_integer_scale());
m_imgui_layer->set_prechain_parameters(
m_vulkan_backend->list_filter_controls(goggles::fc::FilterControlStage::prechain));
m_vulkan_backend->filter_chain_controller().list_filter_controls(
goggles::fc::FilterControlStage::prechain));

update_ui_parameters(*m_vulkan_backend, *m_imgui_layer);
return Result<void>{};
Expand Down Expand Up @@ -475,7 +480,7 @@ void Application::forward_input_event(const SDL_Event& event) {
void Application::sync_prechain_ui() {
auto& prechain = m_imgui_layer->state().prechain;
if (prechain.target_width == 0 && prechain.target_height == 0) {
const auto initial_resolution = m_vulkan_backend->get_captured_extent();
const auto initial_resolution = m_vulkan_backend->frame_importer().import_extent;
if (initial_resolution.width > 0 && initial_resolution.height > 0) {
m_imgui_layer->set_prechain_state(initial_resolution,
m_vulkan_backend->get_scale_mode(),
Expand All @@ -486,8 +491,8 @@ void Application::sync_prechain_ui() {
}

if (prechain.pass_parameters.empty()) {
auto params =
m_vulkan_backend->list_filter_controls(goggles::fc::FilterControlStage::prechain);
auto params = m_vulkan_backend->filter_chain_controller().list_filter_controls(
goggles::fc::FilterControlStage::prechain);
if (!params.empty()) {
m_imgui_layer->set_prechain_parameters(std::move(params));
}
Expand Down Expand Up @@ -594,7 +599,7 @@ void Application::request_surface_resize(uint32_t surface_id, bool maximize) {
return;
}

auto extent = m_vulkan_backend->swapchain_extent();
auto extent = m_vulkan_backend->render_output().swapchain_extent;
if (extent.width == 0 || extent.height == 0) {
return;
}
Expand Down Expand Up @@ -673,7 +678,8 @@ void Application::handle_swapchain_changes() {
static_cast<uint32_t>(height), fmt);
if (result) {
if (fmt != vk::Format::eUndefined) {
m_imgui_layer->rebuild_for_format(m_vulkan_backend->swapchain_format());
m_imgui_layer->rebuild_for_format(
m_vulkan_backend->render_output().swapchain_format);
}
} else {
GOGGLES_LOG_ERROR("Swapchain rebuild failed: {}", result.error().message);
Expand All @@ -698,7 +704,7 @@ void Application::update_frame_sources() {
if (m_surface_frame->image.format != vk::Format::eUndefined) {
auto target_format =
render::VulkanBackend::get_matching_swapchain_format(m_surface_frame->image.format);
if (target_format != m_vulkan_backend->swapchain_format()) {
if (target_format != m_vulkan_backend->render_output().swapchain_format) {
m_pending_format = static_cast<uint32_t>(m_surface_frame->image.format);
m_skip_frame = true;
}
Expand Down Expand Up @@ -734,11 +740,13 @@ void Application::sync_ui_state() {

sync_prechain_ui();

if (m_vulkan_backend->consume_chain_swapped()) {
m_imgui_layer->state().current_preset = m_vulkan_backend->current_preset_path();
if (m_vulkan_backend->filter_chain_controller().consume_chain_swapped()) {
m_imgui_layer->state().current_preset =
m_vulkan_backend->filter_chain_controller().current_preset_path();
update_ui_parameters(*m_vulkan_backend, *m_imgui_layer);
m_imgui_layer->set_prechain_parameters(
m_vulkan_backend->list_filter_controls(goggles::fc::FilterControlStage::prechain));
m_vulkan_backend->filter_chain_controller().list_filter_controls(
goggles::fc::FilterControlStage::prechain));
}

m_imgui_layer->begin_frame();
Expand Down Expand Up @@ -822,11 +830,11 @@ void Application::set_target_fps(uint32_t target_fps) {
}

auto Application::gpu_index() const -> uint32_t {
return m_vulkan_backend->gpu_index();
return m_vulkan_backend->vulkan_context().gpu_index;
}

auto Application::gpu_uuid() const -> std::string {
return m_vulkan_backend->gpu_uuid();
return m_vulkan_backend->vulkan_context().gpu_uuid;
}

void Application::update_pointer_lock_mirror() {
Expand Down
27 changes: 0 additions & 27 deletions src/render/backend/vulkan_backend.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,33 +252,6 @@ void VulkanBackend::set_prechain_resolution(uint32_t width, uint32_t height) {
[this]() { wait_all_frames(); });
}

auto VulkanBackend::get_prechain_resolution() const -> vk::Extent2D {
return m_filter_chain_controller.current_prechain_resolution();
}

auto VulkanBackend::list_filter_controls() const
-> std::vector<goggles::fc::FilterControlDescriptor> {
return m_filter_chain_controller.list_filter_controls();
}

auto VulkanBackend::list_filter_controls(goggles::fc::FilterControlStage stage) const
-> std::vector<goggles::fc::FilterControlDescriptor> {
return m_filter_chain_controller.list_filter_controls(stage);
}

auto VulkanBackend::set_filter_control_value(goggles::fc::FilterControlId control_id, float value)
-> bool {
return m_filter_chain_controller.set_filter_control_value(control_id, value);
}

auto VulkanBackend::reset_filter_control_value(goggles::fc::FilterControlId control_id) -> bool {
return m_filter_chain_controller.reset_filter_control_value(control_id);
}

void VulkanBackend::reset_filter_controls() {
m_filter_chain_controller.reset_filter_controls();
}

auto VulkanBackend::record_render_commands(vk::CommandBuffer cmd, uint32_t image_index,
const UiRenderCallback& ui_callback) -> Result<void> {
GOGGLES_PROFILE_SCOPE("RecordCommands");
Expand Down
53 changes: 13 additions & 40 deletions src/render/backend/vulkan_backend.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,6 @@ class VulkanBackend {
void load_shader_preset(const std::filesystem::path& preset_path);
[[nodiscard]] auto reload_shader_preset(const std::filesystem::path& preset_path)
-> Result<void>;
[[nodiscard]] auto current_preset_path() const -> const std::filesystem::path& {
return m_filter_chain_controller.current_preset_path();
}

void set_filter_chain_policy(const FilterChainStagePolicy& policy);

using UiRenderCallback = std::function<void(vk::CommandBuffer, vk::ImageView, vk::Extent2D)>;
Expand All @@ -74,53 +70,30 @@ class VulkanBackend {
-> Result<void>;
void wait_all_frames();

[[nodiscard]] auto instance() const -> vk::Instance { return m_vulkan_context.instance; }
[[nodiscard]] auto physical_device() const -> vk::PhysicalDevice {
return m_vulkan_context.physical_device;
}
[[nodiscard]] auto device() const -> vk::Device { return m_vulkan_context.device; }
[[nodiscard]] auto graphics_queue() const -> vk::Queue {
return m_vulkan_context.graphics_queue;
void set_prechain_resolution(uint32_t width, uint32_t height);

[[nodiscard]] auto vulkan_context() const -> const backend_internal::VulkanContext& {
return m_vulkan_context;
}
[[nodiscard]] auto graphics_queue_family() const -> uint32_t {
return m_vulkan_context.graphics_queue_family;
[[nodiscard]] auto render_output() const -> const backend_internal::RenderOutput& {
return m_render_output;
}
[[nodiscard]] auto swapchain_format() const -> vk::Format {
return m_render_output.swapchain_format;
[[nodiscard]] auto frame_importer() const -> const backend_internal::ExternalFrameImporter& {
return m_external_frame_importer;
}
[[nodiscard]] auto swapchain_extent() const -> vk::Extent2D {
return m_render_output.swapchain_extent;
[[nodiscard]] auto filter_chain_controller() -> backend_internal::FilterChainController& {
return m_filter_chain_controller;
}
Comment on lines +84 to 86
Copy link
Copy Markdown

@qodo-code-review qodo-code-review bot Mar 25, 2026

Choose a reason for hiding this comment

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

Action required

1. Mutable controller exposed 🐞 Bug ✓ Correctness

VulkanBackend::filter_chain_controller() returns a mutable reference to
backend_internal::FilterChainController, allowing non-backend code to call lifecycle/orchestration
APIs and bypass the sequencing enforced by VulkanBackend::render()/shutdown(). This can violate
GPU synchronization/resource-lifetime invariants and cause incorrect rendering behavior or
hard-to-debug crashes if any future call site uses more than the intended “control” subset of the
controller API.
Agent Prompt 
### Issue description
`VulkanBackend::filter_chain_controller()` currently returns a **mutable** `backend_internal::FilterChainController&`. This exposes lifecycle/orchestration methods that `VulkanBackend` expects to own and sequence safely.

### Issue Context
`VulkanBackend::render()` calls `advance_frame()`, `check_pending_chain_swap(...)`, and `cleanup_retired_adapters()` in a controlled order. Returning a mutable controller reference makes it easy for future call sites to invoke these APIs out-of-order and break backend invariants.

### Fix Focus Areas
- src/render/backend/vulkan_backend.hpp[73-90]
- src/render/backend/vulkan_backend.cpp[398-548]
- src/render/backend/filter_chain_controller.hpp[67-111]

### Recommended change
- Remove the non-const `filter_chain_controller()` accessor, or restrict it so external code can only perform the intended narrow set of operations.
- Re-introduce a small, explicit `VulkanBackend` facade API for UI interactions (e.g., `list_filter_controls(stage)`, `set_filter_control_value(...)`, `reset_filter_controls()`, `current_preset_path()`, `consume_chain_swapped()`, `current_prechain_resolution()`), so orchestration remains owned by `VulkanBackend`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

[[nodiscard]] auto swapchain_image_count() const -> uint32_t {
return m_render_output.image_count();
[[nodiscard]] auto filter_chain_controller() const
-> const backend_internal::FilterChainController& {
return m_filter_chain_controller;
}
[[nodiscard]] auto get_prechain_resolution() const -> vk::Extent2D;
void set_prechain_resolution(uint32_t width, uint32_t height);

[[nodiscard]] auto list_filter_controls() const
-> std::vector<goggles::fc::FilterControlDescriptor>;
[[nodiscard]] auto list_filter_controls(goggles::fc::FilterControlStage stage) const
-> std::vector<goggles::fc::FilterControlDescriptor>;
[[nodiscard]] auto set_filter_control_value(goggles::fc::FilterControlId control_id,
float value) -> bool;
[[nodiscard]] auto reset_filter_control_value(goggles::fc::FilterControlId control_id) -> bool;
void reset_filter_controls();

[[nodiscard]] auto get_captured_extent() const -> vk::Extent2D {
return m_external_frame_importer.import_extent;
}
[[nodiscard]] auto target_fps() const -> uint32_t { return m_render_output.target_fps; }
[[nodiscard]] auto get_scale_mode() const -> ScaleMode { return m_scale_mode; }
[[nodiscard]] auto get_integer_scale() const -> uint32_t { return m_integer_scale; }
void set_target_fps(uint32_t target_fps) { update_target_fps(target_fps); }
void set_scale_mode(ScaleMode mode) { m_scale_mode = mode; }
void set_integer_scale(uint32_t scale) { m_integer_scale = scale; }
[[nodiscard]] auto gpu_index() const -> uint32_t { return m_vulkan_context.gpu_index; }
[[nodiscard]] auto gpu_uuid() const -> const std::string& { return m_vulkan_context.gpu_uuid; }

[[nodiscard]] auto consume_chain_swapped() -> bool {
return m_filter_chain_controller.consume_chain_swapped();
}

private:
VulkanBackend() = default;
Expand Down
27 changes: 12 additions & 15 deletions tests/render/test_filter_boundary_contracts.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,20 +71,18 @@ TEST_CASE("Filter chain boundary control contract coverage", "[filter_chain][bou
using goggles::fc::FilterControlStage;
using goggles::render::VulkanBackend;

using ListAllSig = std::vector<FilterControlDescriptor> (VulkanBackend::*)() const;
using ListStageSig =
std::vector<FilterControlDescriptor> (VulkanBackend::*)(FilterControlStage) const;
using SetSig = bool (VulkanBackend::*)(FilterControlId, float);
using ResetSig = bool (VulkanBackend::*)(FilterControlId);
using FCC = goggles::render::backend_internal::FilterChainController;
using ListAllSig = std::vector<FilterControlDescriptor> (FCC::*)() const;
using ListStageSig = std::vector<FilterControlDescriptor> (FCC::*)(FilterControlStage) const;
using SetSig = bool (FCC::*)(FilterControlId, float);
using ResetSig = bool (FCC::*)(FilterControlId);

static_assert(
std::is_same_v<decltype(static_cast<ListAllSig>(&VulkanBackend::list_filter_controls)),
ListAllSig>);
static_assert(
std::is_same_v<decltype(static_cast<ListStageSig>(&VulkanBackend::list_filter_controls)),
ListStageSig>);
static_assert(std::is_same_v<decltype(&VulkanBackend::set_filter_control_value), SetSig>);
static_assert(std::is_same_v<decltype(&VulkanBackend::reset_filter_control_value), ResetSig>);
std::is_same_v<decltype(static_cast<ListAllSig>(&FCC::list_filter_controls)), ListAllSig>);
static_assert(std::is_same_v<decltype(static_cast<ListStageSig>(&FCC::list_filter_controls)),
ListStageSig>);
static_assert(std::is_same_v<decltype(&FCC::set_filter_control_value), SetSig>);
static_assert(std::is_same_v<decltype(&FCC::reset_filter_control_value), ResetSig>);

static_assert(std::is_same_v<goggles::ui::ParameterChangeCallback,
std::function<void(FilterControlId, float)>>);
Expand Down Expand Up @@ -116,7 +114,7 @@ TEST_CASE("Filter chain boundary control contract coverage", "[filter_chain][bou
REQUIRE(app_text->find("set_filter_control_value(") != std::string::npos);
REQUIRE((app_text->find("reset_filter_control_value(") != std::string::npos ||
app_text->find("reset_filter_controls(") != std::string::npos));
REQUIRE(app_text->find("filter_chain(") == std::string::npos);
REQUIRE(app_text->find("m_filter_chain_controller.") == std::string::npos);

const auto imgui_path = std::filesystem::path(GOGGLES_SOURCE_DIR) / "src/ui/imgui_layer.cpp";
auto imgui_text = read_text_file(imgui_path);
Expand Down Expand Up @@ -323,8 +321,7 @@ TEST_CASE("Async swap and resize safety contract coverage", "[filter_chain][asyn
auto header_text = read_text_file(backend_hpp);
REQUIRE(header_text.has_value());
REQUIRE(header_text->find("return m_render_output.needs_resize;") != std::string::npos);
REQUIRE(header_text->find("return m_filter_chain_controller.consume_chain_swapped();") !=
std::string::npos);
REQUIRE(header_text->find("filter_chain_controller()") != std::string::npos);
}

TEST_CASE("Filter chain standalone API boundary contract coverage",
Expand Down
Loading