refactor(render): remove Vulkan RAII wrappers

[K1ngst0m]

User description

• Convert backend, filter chain, passes, framebuffer, and texture loader from vk::Unique* to plain handles
• Implement explicit destroy/free logic in shutdown() and resize/reload paths
• Update move semantics for framebuffer and debug messenger to preserve safe manual ownership transfer
• Add failure rollback for image/buffer/memory creation and descriptor/pipeline setup paths
• Keep public Vulkan context accessors and call sites consistent with non-owning handle usage

---

PR Type

Enhancement, Refactoring

---

Description

• Replace Vulkan-Hpp RAII wrappers (vk::Unique*) with plain handles and explicit lifetime management

• Implement deterministic destroy/free calls in shutdown and error paths

• Add proper error rollback for resource creation failures

• Update move semantics for framebuffer and debug messenger to preserve manual ownership

• Update project policies to mandate plain Vulkan handles instead of RAII wrappers

---

Diagram Walkthrough

flowchart LR
  A["vk::Unique* RAII Wrappers"] -->|Convert to| B["Plain vk:: Handles"]
  B -->|Add explicit| C["destroy/free Calls"]
  C -->|In shutdown paths| D["Deterministic Cleanup"]
  E["Error Paths"] -->|Add rollback| F["Resource Deallocation"]
  G["Move Semantics"] -->|Update to| H["Manual Ownership Transfer"]

Loading

File Walkthrough

	Relevant files
Refactoring	15 files vulkan_backend.cpp Convert instance/device/swapchain to plain handles              +100/-76 vulkan_backend.hpp Change member types from Unique* to plain handles                +8/-8      filter_pass.cpp Replace RAII wrappers with explicit destroy calls                +85/-53  filter_pass.hpp Update pipeline/descriptor/buffer member types                      +9/-9      downsample_pass.cpp Implement explicit cleanup for pipeline resources                +47/-27  downsample_pass.hpp Change pipeline/descriptor members to plain handles            +5/-5      output_pass.cpp Add explicit destroy calls in shutdown method                        +46/-26  output_pass.hpp Update pipeline/descriptor member declarations                      +5/-5      framebuffer.cpp Implement explicit image/memory/view cleanup                          +45/-20  framebuffer.hpp Change image/memory/view to plain handle types                      +5/-5      filter_chain.cpp Add texture registry cleanup and error handling                    +66/-15  filter_chain.hpp Update LoadedTexture sampler to plain handle                          +3/-2      texture_loader.hpp Change TextureData members to plain handle types                  +7/-7      vulkan_debug.cpp Implement explicit messenger destruction and move semantics +36/-4    vulkan_debug.hpp Add instance member and explicit reset method                        +7/-5
Error handling	1 files texture_loader.cpp Add error rollback for staging/image resource creation      +52/-32
Documentation	1 files project_policies.md Update RAII policy to mandate plain Vulkan handles              +16/-16

---

Summary by CodeRabbit

• Refactor

  • Internal refactoring of rendering system resource management to improve explicit lifetime control and deterministic cleanup across Vulkan resources.

• Documentation

  • Updated project policies to clarify resource ownership and lifecycle management guidelines.

[coderabbitai]

Caution

Review failed

The pull request is closed.

📝 Walkthrough

Walkthrough

Systematic conversion from Vulkan-Hpp RAII smart wrappers (Unique*) to plain vk:: handles across backend and render-chain components, replacing automatic cleanup with explicit destruction, including updated resource lifetimes, move semantics, and shutdown ordering in policy documentation and implementation.

Changes

Cohort / File(s)	Summary
Policy & Documentation docs/project_policies.md	Updated resource ownership and Vulkan handle management policies to mandate plain vk:: handles over Unique* wrappers, explicit teardown ordering, and deterministic cleanup semantics.
Vulkan Backend src/render/backend/vulkan_backend.cpp, src/render/backend/vulkan_backend.hpp	Converted UniqueInstance, UniqueDevice, UniqueSwapchainKHR, and related handles to plain vk:: equivalents. Updated dispatcher initialization, creation paths, and shutdown to use explicit destroy calls with proper ordering.
Debug Messenger src/render/backend/vulkan_debug.cpp, src/render/backend/vulkan_debug.hpp	Reworked ownership model from Unique wrappers to explicit instance/messenger pair. Added user-defined destructor, move semantics, and reset() method for deterministic cleanup.
Render Passes & Core Components src/render/chain/downsample_pass.*, src/render/chain/filter_pass.*, src/render/chain/output_pass.*	Converted pipeline layout, pipeline, descriptor layout/pool, sampler, buffer, and memory members from Unique* to plain handles. Replaced smart-pointer resets with explicit device-guarded destruction in shutdown paths.
Filter Chain & Framebuffer src/render/chain/filter_chain.cpp, src/render/chain/filter_chain.hpp, src/render/chain/framebuffer.cpp, src/render/chain/framebuffer.hpp	Updated resource storage from Unique* wrappers to raw handles. Added explicit cleanup routines (cleanup_texture_registry), updated move semantics to null out source handles, and adjusted creation/binding paths to use direct handle access.
Texture Loading src/render/texture/texture_loader.cpp, src/render/texture/texture_loader.hpp	Converted staging and texture resources from Unique* to plain handles. Updated staging buffer/image creation to use non-unique variants and added explicit error-path cleanup for partially allocated resources.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• feat: improve HiDPI behavior across SDL, Vulkan, and ImGui #37: Modifies swapchain creation/recreation in the same Vulkan backend implementation, directly overlapping code paths and resource management logic.
• Add RetroArch CRT Royale Support #3: Modifies the same render-chain files (filter_chain, filter_pass, texture_loader, framebuffer) with conflicting Vulkan handle ownership representation patterns.
• feat(layer): cross-process semaphore export #16: Touches Vulkan backend resource/synchronization handling in vulkan_backend.* with overlapping modifications to submit/present logic.

Suggested labels

Review effort 4/5

Poem

🐰 From Unique to plain, a clearer way,
Explicit cleanup ends the day,
No RAII magic, just honest code,
Resource lifetimes now well-bestowed. ✨

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch dev/remove-vk-raii

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[qodo-code-review]

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: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities 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: Ignored error result: The result of m_device.waitIdle() is explicitly ignored during semaphore cleanup, which can silently hide device-loss or other failure conditions. Referred Code static_cast<void>(m_device.waitIdle()); } 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: Log format unclear: Newly added log messages appear to be unstructured (non-JSON), but determining whether structured logging is required/implemented elsewhere cannot be confirmed from the diff alone. Referred Code GOGGLES_LOG_WARN("waitIdle failed during shutdown: {}", vk::to_string(wait_result)); } Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend

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

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Implement custom move semantics for all handle-owning classes Implement custom move constructors and assignment operators for classes like FilterPass, DownsamplePass, and OutputPass. This is necessary to prevent double-free errors since they now manage raw Vulkan handles and default move operations are unsafe. Examples: src/render/chain/filter_pass.hpp [129-140] vk::PipelineLayout m_pipeline_layout; vk::Pipeline m_pipeline; vk::DescriptorSetLayout m_descriptor_layout; vk::DescriptorPool m_descriptor_pool; std::vector<vk::DescriptorSet> m_descriptor_sets; vk::Sampler m_sampler; vk::Buffer m_vertex_buffer; vk::DeviceMemory m_vertex_buffer_memory; ... (clipped 2 lines) src/render/chain/filter_pass.cpp [105-143] void FilterPass::shutdown() { if (m_device) { if (m_pipeline) { m_device.destroyPipeline(m_pipeline); m_pipeline = nullptr; } if (m_pipeline_layout) { m_device.destroyPipelineLayout(m_pipeline_layout); m_pipeline_layout = nullptr; } ... (clipped 29 lines) Solution Walkthrough: Before: class FilterPass : public Pass { public: // Uses default move constructor and assignment FilterPass(FilterPass&&) noexcept = default; FilterPass& operator=(FilterPass&&) noexcept = default; void shutdown() { if (m_device && m_pipeline) { m_device.destroyPipeline(m_pipeline); } // ... other resources } private: vk::Pipeline m_pipeline; // Raw handle // ... other raw handles }; After: class FilterPass : public Pass { public: // Custom move constructor FilterPass(FilterPass&& other) noexcept { // ... move all members m_pipeline = other.m_pipeline; other.m_pipeline = nullptr; // Nullify source } // Custom move assignment FilterPass& operator=(FilterPass&& other) noexcept { shutdown(); // Release current resources // ... move all members from other m_pipeline = other.m_pipeline; other.m_pipeline = nullptr; // Nullify source return *this; } void shutdown(); private: vk::Pipeline m_pipeline; // ... other raw handles }; Suggestion importance[1-10]: 10 __ Why: This suggestion identifies a critical bug where default move semantics on classes now owning raw Vulkan handles would lead to double-free errors, which is a major correctness issue.	High
Possible issue	Fix resource leak on texture reload Fix a resource leak in load_preset_textures by moving the TextureData from tex_data_result instead of copying it. This ensures correct ownership transfer of the newly loaded texture resources and prevents them from being leaked when a texture is reloaded. src/render/chain/filter_chain.cpp [868-921] auto FilterChain::load_preset_textures() -> Result<void> { GOGGLES_PROFILE_SCOPE("LoadPresetTextures"); for (const auto& tex_config : m_preset.textures) { TextureLoadConfig load_cfg{.generate_mipmaps = tex_config.mipmap, .linear = tex_config.linear}; auto tex_data_result = m_texture_loader->load_from_file(tex_config.path, load_cfg); if (!tex_data_result) { return nonstd::make_unexpected(tex_data_result.error()); } auto sampler_result = create_texture_sampler(tex_config); if (!sampler_result) { auto& loaded = tex_data_result.value(); if (loaded.view) { m_vk_ctx.device.destroyImageView(loaded.view); } if (loaded.memory) { m_vk_ctx.device.freeMemory(loaded.memory); } if (loaded.image) { m_vk_ctx.device.destroyImage(loaded.image); } return nonstd::make_unexpected(sampler_result.error()); } auto sampler = sampler_result.value(); if (auto existing = m_texture_registry.find(tex_config.name); existing != m_texture_registry.end()) { if (existing->second.sampler) { m_vk_ctx.device.destroySampler(existing->second.sampler); } if (existing->second.data.view) { m_vk_ctx.device.destroyImageView(existing->second.data.view); } if (existing->second.data.memory) { m_vk_ctx.device.freeMemory(existing->second.data.memory); } if (existing->second.data.image) { m_vk_ctx.device.destroyImage(existing->second.data.image); } m_texture_registry.erase(existing); } - auto texture_data = tex_data_result.value(); + auto texture_data = std::move(tex_data_result.value()); m_texture_registry[tex_config.name] = LoadedTexture{.data = texture_data, .sampler = sampler}; GOGGLES_LOG_DEBUG("Loaded texture '{}' from {}", tex_config.name, tex_config.path.filename().string()); } return {}; } [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a resource leak. The TextureData struct contains raw Vulkan handles, and by copying it from tex_data_result.value() instead of moving, ownership is not transferred. The TextureData within tex_data_result is destructed at the end of the loop without its resources being released, leading to a leak.	High
	Prevent pipeline leak on creation failure In FilterPass::create_pipeline, add cleanup for partially created pipelines when createGraphicsPipelines fails. Iterate over the returned pipelines vector and destroy any valid handles to prevent resource leaks in this error scenario. src/render/chain/filter_pass.cpp [629-756] auto FilterPass::create_pipeline(const std::vector<uint32_t>& vertex_spirv, const std::vector<uint32_t>& fragment_spirv) -> Result<void> { GOGGLES_PROFILE_SCOPE("CreatePipeline"); vk::ShaderModuleCreateInfo vert_module_info{}; vert_module_info.codeSize = vertex_spirv.size() * sizeof(uint32_t); vert_module_info.pCode = vertex_spirv.data(); auto [vert_mod_result, vert_module] = m_device.createShaderModule(vert_module_info); if (vert_mod_result != vk::Result::eSuccess) { return make_error<void>(ErrorCode::vulkan_init_failed, "Failed to create vertex shader module: " + vk::to_string(vert_mod_result)); } vk::ShaderModuleCreateInfo frag_module_info{}; frag_module_info.codeSize = fragment_spirv.size() * sizeof(uint32_t); frag_module_info.pCode = fragment_spirv.data(); auto [frag_mod_result, frag_module] = m_device.createShaderModule(frag_module_info); if (frag_mod_result != vk::Result::eSuccess) { m_device.destroyShaderModule(vert_module); return make_error<void>(ErrorCode::vulkan_init_failed, "Failed to create fragment shader module: " + vk::to_string(frag_mod_result)); } std::array<vk::PipelineShaderStageCreateInfo, 2> stages{}; stages[0].stage = vk::ShaderStageFlagBits::eVertex; stages[0].module = vert_module; stages[0].pName = "main"; stages[1].stage = vk::ShaderStageFlagBits::eFragment; stages[1].module = frag_module; stages[1].pName = "main"; vk::PipelineVertexInputStateCreateInfo vertex_input{}; vk::VertexInputBindingDescription binding_desc{}; ... create_info.layout = m_pipeline_layout; auto [result, pipelines] = m_device.createGraphicsPipelines(nullptr, create_info); m_device.destroyShaderModule(frag_module); m_device.destroyShaderModule(vert_module); if (result != vk::Result::eSuccess) { + for (const auto& p : pipelines) { + if (p) { + m_device.destroyPipeline(p); + } + } return make_error<void>(ErrorCode::vulkan_init_failed, "Failed to create graphics pipeline: " + vk::to_string(result)); } m_pipeline = pipelines[0]; return {}; } [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 8 __ Why: This suggestion correctly identifies a potential resource leak. According to Vulkan documentation, even if createGraphicsPipelines returns a failure code, it may have partially created some pipelines that need to be destroyed. The proposed change correctly adds cleanup logic for this failure case, preventing a leak.	Medium
	Prevent resource leak on transfer failure In TextureLoader::upload_to_gpu, prevent a resource leak by cleaning up image_resources if the subsequent record_and_submit_transfer call fails. This ensures the allocated vk::Image and vk::DeviceMemory are properly destroyed in the error path. src/render/texture/texture_loader.cpp [277-320] auto TextureLoader::upload_to_gpu(const uint8_t* pixels, uint32_t width, uint32_t height, uint32_t mip_levels, bool linear) -> Result<TextureData> { vk::DeviceSize image_size = static_cast<vk::DeviceSize>(width) * height * RGBA_CHANNELS; vk::Format format = linear ? vk::Format::eR8G8B8A8Unorm : vk::Format::eR8G8B8A8Srgb; auto staging = GOGGLES_TRY(create_staging_buffer(image_size, pixels)); auto image_resources = GOGGLES_TRY( create_texture_image(ImageSize{.width = width, .height = height}, mip_levels, linear)); auto transfer_result = record_and_submit_transfer(staging.buffer, image_resources.image, ImageSize{.width = width, .height = height}, mip_levels, format); m_device.freeMemory(staging.memory); m_device.destroyBuffer(staging.buffer); - GOGGLES_TRY(transfer_result); + + if (!transfer_result) { + m_device.freeMemory(image_resources.memory); + m_device.destroyImage(image_resources.image); + return nonstd::make_unexpected(transfer_result.error()); + } vk::ImageViewCreateInfo view_info{}; view_info.image = image_resources.image; view_info.viewType = vk::ImageViewType::e2D; view_info.format = format; view_info.subresourceRange.aspectMask = vk::ImageAspectFlagBits::eColor; view_info.subresourceRange.baseMipLevel = 0; view_info.subresourceRange.levelCount = mip_levels; view_info.subresourceRange.baseArrayLayer = 0; view_info.subresourceRange.layerCount = 1; auto [view_result, view] = m_device.createImageView(view_info); if (view_result != vk::Result::eSuccess) { m_device.freeMemory(image_resources.memory); m_device.destroyImage(image_resources.image); return make_error<TextureData>(ErrorCode::vulkan_init_failed, "Failed to create image view: " + vk::to_string(view_result)); } return TextureData{ .image = image_resources.image, .memory = image_resources.memory, .view = view, .extent = vk::Extent2D{width, height}, .mip_levels = mip_levels, }; } Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly points out a resource leak. If record_and_submit_transfer fails, the GOGGLES_TRY macro causes an early exit, leaking the image_resources (image and memory) that were allocated just before. The proposed fix adds the necessary cleanup logic for this error path.	Medium
General	Wait for device idle before cleaning imported image In VulkanBackend::cleanup_imported_image, add a call to m_device.waitIdle() before destroying the image resources. This ensures the GPU has finished all operations involving the image, preventing race conditions and crashes during cleanup. src/render/backend/vulkan_backend.cpp [1091-1106] void VulkanBackend::cleanup_imported_image() { if (m_device) { + static_cast<void>(m_device.waitIdle()); if (m_import.view) { m_device.destroyImageView(m_import.view); m_import.view = nullptr; } if (m_import.memory) { m_device.freeMemory(m_import.memory); m_import.memory = nullptr; } if (m_import.image) { m_device.destroyImage(m_import.image); m_import.image = nullptr; } } } Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly proposes adding m_device.waitIdle() to cleanup_imported_image to ensure synchronization before destroying resources. This is a good practice to prevent destroying resources that might still be in use by the GPU.	Medium
	Wait for device idle in pass shutdown In DownsamplePass::shutdown, add a call to m_device.waitIdle() before destroying Vulkan resources. This ensures that any in-flight commands using these resources have completed, preventing validation errors or crashes. src/render/chain/downsample_pass.cpp [50-79] void DownsamplePass::shutdown() { if (m_device) { + static_cast<void>(m_device.waitIdle()); if (m_pipeline) { m_device.destroyPipeline(m_pipeline); m_pipeline = nullptr; } if (m_pipeline_layout) { m_device.destroyPipelineLayout(m_pipeline_layout); m_pipeline_layout = nullptr; } if (m_descriptor_pool) { m_device.destroyDescriptorPool(m_descriptor_pool); m_descriptor_pool = nullptr; } if (m_descriptor_layout) { m_device.destroyDescriptorSetLayout(m_descriptor_layout); m_descriptor_layout = nullptr; } if (m_sampler) { m_device.destroySampler(m_sampler); m_sampler = nullptr; } } m_descriptor_sets.clear(); m_target_format = vk::Format::eUndefined; m_device = nullptr; } Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly proposes adding m_device.waitIdle() to DownsamplePass::shutdown to ensure synchronization before destroying resources. This is a good practice to prevent destroying resources that might still be in use by the GPU.	Medium
More