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.

Author: gpx1000

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<MemoryPool::Allocation> 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.

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<std::shared_mutex> 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)

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();

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;
             }