From 57d64cf4fb8563866d87c79fa33c671adc38fb24 Mon Sep 17 00:00:00 2001 From: swinston Date: Wed, 3 Dec 2025 02:04:30 -0800 Subject: [PATCH] Improve resource cleanup with explicit memory deallocation and exception safety. Add pooled memory handling, warnings for deallocation failures, and enhanced cleanup logic for swapchain and per-entity resources. --- attachments/simple_engine/renderer.h | 3 +- attachments/simple_engine/renderer_core.cpp | 100 +++++++++++++++++- .../simple_engine/renderer_rendering.cpp | 23 +++- .../simple_engine/renderer_resources.cpp | 16 ++- 4 files changed, 136 insertions(+), 6 deletions(-) diff --git a/attachments/simple_engine/renderer.h b/attachments/simple_engine/renderer.h index df3e548e..fe337f2c 100644 --- a/attachments/simple_engine/renderer.h +++ b/attachments/simple_engine/renderer.h @@ -622,9 +622,10 @@ class Renderer { // The texture that will hold a snapshot of the opaque scene vk::raii::Image opaqueSceneColorImage{nullptr}; - vk::raii::DeviceMemory opaqueSceneColorImageMemory{nullptr}; // <-- Standard Vulkan memory vk::raii::ImageView opaqueSceneColorImageView{nullptr}; vk::raii::Sampler opaqueSceneColorSampler{nullptr}; + // Pooled allocation associated with opaqueSceneColorImage (must be explicitly deallocated) + std::unique_ptr opaqueSceneColorImageAllocation = nullptr; // A descriptor set for the opaque scene color texture. We will have one for each frame in flight // to match the swapchain images. diff --git a/attachments/simple_engine/renderer_core.cpp b/attachments/simple_engine/renderer_core.cpp index b6252161..d4b2bb46 100644 --- a/attachments/simple_engine/renderer_core.cpp +++ b/attachments/simple_engine/renderer_core.cpp @@ -235,13 +235,111 @@ void Renderer::Cleanup() { // Wait for the device to be idle before cleaning up device.waitIdle(); + + // Clean up swapchain-bound resources (depth, offscreen color, pipelines, views, etc.) + cleanupSwapChain(); + + // Release per-entity resources and return pooled memory to the MemoryPool for (auto& resources : entityResources | std::views::values) { - // Memory pool handles unmapping automatically, no need to manually unmap + // Descriptor sets are RAII and freed with descriptorPool; just clear holders resources.basicDescriptorSets.clear(); resources.pbrDescriptorSets.clear(); + + // Destroy UBO buffers and return pooled allocations resources.uniformBuffers.clear(); + for (auto& alloc : resources.uniformBufferAllocations) { + if (alloc) { + try { memoryPool->deallocate(std::move(alloc)); } + catch (const std::exception& e) { + std::cerr << "Warning: failed to deallocate UBO allocation during Cleanup: " << e.what() << std::endl; + } + } + } resources.uniformBufferAllocations.clear(); resources.uniformBuffersMapped.clear(); + + // Destroy instance buffer and return pooled allocation + resources.instanceBuffer = nullptr; + if (resources.instanceBufferAllocation) { + try { memoryPool->deallocate(std::move(resources.instanceBufferAllocation)); } + catch (const std::exception& e) { + std::cerr << "Warning: failed to deallocate instance buffer allocation during Cleanup: " << e.what() << std::endl; + } + } + resources.instanceBufferMapped = nullptr; + } + entityResources.clear(); + + // Release light storage buffers + for (auto& lsb : lightStorageBuffers) { + lsb.buffer = nullptr; + if (lsb.allocation) { + try { memoryPool->deallocate(std::move(lsb.allocation)); } + catch (const std::exception& e) { + std::cerr << "Warning: failed to deallocate light storage buffer during Cleanup: " << e.what() << std::endl; + } + } + lsb.mapped = nullptr; + lsb.capacity = 0; + lsb.size = 0; + } + lightStorageBuffers.clear(); + + // Release mesh device-local buffers and return pooled allocations + for (auto& [mesh, mres] : meshResources) { + mres.vertexBuffer = nullptr; + if (mres.vertexBufferAllocation) { + try { memoryPool->deallocate(std::move(mres.vertexBufferAllocation)); } + catch (const std::exception& e) { + std::cerr << "Warning: failed to deallocate vertex buffer allocation during Cleanup: " << e.what() << std::endl; + } + } + mres.indexBuffer = nullptr; + if (mres.indexBufferAllocation) { + try { memoryPool->deallocate(std::move(mres.indexBufferAllocation)); } + catch (const std::exception& e) { + std::cerr << "Warning: failed to deallocate index buffer allocation during Cleanup: " << e.what() << std::endl; + } + } + // Staging buffers are RAII and not pooled + mres.stagingVertexBuffer = nullptr; + mres.stagingVertexBufferMemory = nullptr; + mres.stagingIndexBuffer = nullptr; + mres.stagingIndexBufferMemory = nullptr; + mres.vertexBufferSizeBytes = 0; + mres.indexBufferSizeBytes = 0; + mres.indexCount = 0; + } + meshResources.clear(); + + // Release textures and return pooled allocations + { + std::unique_lock texLock(textureResourcesMutex); + for (auto& [key, tres] : textureResources) { + tres.textureSampler = nullptr; + tres.textureImageView = nullptr; + tres.textureImage = nullptr; + if (tres.textureImageAllocation) { + try { memoryPool->deallocate(std::move(tres.textureImageAllocation)); } + catch (const std::exception& e) { + std::cerr << "Warning: failed to deallocate texture image allocation during Cleanup: " << e.what() << std::endl; + } + } + } + textureResources.clear(); + textureAliases.clear(); + textureToEntities.clear(); + } + + // Release default texture resources if allocated + defaultTextureResources.textureSampler = nullptr; + defaultTextureResources.textureImageView = nullptr; + defaultTextureResources.textureImage = nullptr; + if (defaultTextureResources.textureImageAllocation) { + try { memoryPool->deallocate(std::move(defaultTextureResources.textureImageAllocation)); } + catch (const std::exception& e) { + std::cerr << "Warning: failed to deallocate default texture image allocation during Cleanup: " << e.what() << std::endl; + } } // Also clear global descriptor sets that are allocated from descriptorPool, so they are // destroyed while the pool is still valid (avoid vkFreeDescriptorSets invalid pool errors) diff --git a/attachments/simple_engine/renderer_rendering.cpp b/attachments/simple_engine/renderer_rendering.cpp index 451ae34c..69b5de78 100644 --- a/attachments/simple_engine/renderer_rendering.cpp +++ b/attachments/simple_engine/renderer_rendering.cpp @@ -249,10 +249,29 @@ bool Renderer::createSyncObjects() { // Clean up swap chain void Renderer::cleanupSwapChain() { - // Clean up depth resources + // Ensure GPU is idle before tearing down resources bound to the swapchain + device.waitIdle(); + + // Clean up off-screen opaque scene color resources + opaqueSceneColorSampler = nullptr; + opaqueSceneColorImageView = nullptr; + if (opaqueSceneColorImageAllocation) { + try { memoryPool->deallocate(std::move(opaqueSceneColorImageAllocation)); } + catch (const std::exception& e) { + std::cerr << "Warning: failed to deallocate opaqueSceneColor allocation during cleanupSwapChain: " << e.what() << std::endl; + } + } + opaqueSceneColorImage = nullptr; + + // Clean up depth resources (free pooled allocation explicitly) depthImageView = nullptr; + if (depthImageAllocation) { + try { memoryPool->deallocate(std::move(depthImageAllocation)); } + catch (const std::exception& e) { + std::cerr << "Warning: failed to deallocate depth allocation during cleanupSwapChain: " << e.what() << std::endl; + } + } depthImage = nullptr; - depthImageAllocation = nullptr; // Clean up swap chain image views swapChainImageViews.clear(); diff --git a/attachments/simple_engine/renderer_resources.cpp b/attachments/simple_engine/renderer_resources.cpp index cc9fadf7..0b77055c 100644 --- a/attachments/simple_engine/renderer_resources.cpp +++ b/attachments/simple_engine/renderer_resources.cpp @@ -1473,6 +1473,14 @@ void Renderer::createTransparentFallbackDescriptorSets() { bool Renderer::createOpaqueSceneColorResources() { try { + // If an old pooled allocation exists (e.g., after a resize), free it before creating a new one + if (opaqueSceneColorImageAllocation) { + try { + memoryPool->deallocate(std::move(opaqueSceneColorImageAllocation)); + } catch (const std::exception& e) { + std::cerr << "Warning: failed to deallocate previous opaqueSceneColor allocation: " << e.what() << std::endl; + } + } // Create the image auto [image, allocation] = createImagePooled( swapChainExtent.width, @@ -1483,7 +1491,8 @@ bool Renderer::createOpaqueSceneColorResources() { vk::MemoryPropertyFlagBits::eDeviceLocal); opaqueSceneColorImage = std::move(image); - // We don't need a member for the allocation, it's managed by the unique_ptr + // Store the pooled allocation so it can be explicitly returned to the pool on cleanup + opaqueSceneColorImageAllocation = std::move(allocation); // Create the image view opaqueSceneColorImageView = createImageView(opaqueSceneColorImage, swapChainImageFormat, vk::ImageAspectFlagBits::eColor); @@ -1857,7 +1866,10 @@ bool Renderer::createOrResizeLightStorageBuffers(size_t lightCount) { // Clean up old buffer if it exists (now safe after waitIdle) if (buffer.allocation) { buffer.buffer = nullptr; - buffer.allocation.reset(); + try { memoryPool->deallocate(std::move(buffer.allocation)); } + catch (const std::exception& e) { + std::cerr << "Warning: failed to deallocate previous light storage buffer allocation: " << e.what() << std::endl; + } buffer.mapped = nullptr; }