fix(renderer): correct resource teardown ordering in Drop

Replace ptr::read-based field consumption with Option<T> + take() for
sync primitives, GPU allocations, and the allocator itself. The prior
pattern left bitwise-duplicated values in the original fields, so Rust's
automatic field drop after Drop::drop returned freed the same heap
buffers (Vec backings inside SyncPrimitives) and Allocations twice.

The allocator is now explicitly dropped before destroy_device so its
own Drop impl, which releases vk::DeviceMemory blocks, runs while the
logical device is still valid. Swapchain image views are also now
destroyed before the swapchain, as required by the Vulkan spec.
This commit is contained in:
Serkyo 2026-05-16 19:28:47 +02:00
parent 5659469e17
commit 19f79080b1
3 changed files with 73 additions and 49 deletions

View file

@ -23,4 +23,10 @@ pub enum RendererError {
/// An invalid string was encountered. /// An invalid string was encountered.
#[error("Invalid string")] #[error("Invalid string")]
InvalidString, InvalidString,
/// The renderer's synchronization primitives were unavailable.
#[error("Synchronization primitives missing")]
SyncPrimitivesMissing,
/// The renderer's GPU memory allocator was unavailable.
#[error("GPU allocator missing")]
AllocatorMissing,
} }

View file

@ -143,18 +143,18 @@ impl Renderer {
swapchain_image_views: image_views, swapchain_image_views: image_views,
command_pool, command_pool,
command_buffers, command_buffers,
allocator, allocator: Some(allocator),
index_buffer, index_buffer,
index_allocation, index_allocation: Some(index_allocation),
index_count, index_count,
depth_image, depth_image,
depth_allocation, depth_allocation: Some(depth_allocation),
depth_image_view, depth_image_view,
pipeline_layout, pipeline_layout,
graphics_pipeline, graphics_pipeline,
vertex_buffer, vertex_buffer,
vertex_allocation, vertex_allocation: Some(vertex_allocation),
sync, sync: Some(sync),
current_frame: 0, current_frame: 0,
}) })
} }

View file

@ -50,24 +50,24 @@ pub struct Renderer {
/// The compiled graphics pipeline state. /// The compiled graphics pipeline state.
pub(crate) graphics_pipeline: vk::Pipeline, pub(crate) graphics_pipeline: vk::Pipeline,
/// Memory manager for GPU allocations. /// Memory manager for GPU allocations.
pub(crate) allocator: Allocator, pub(crate) allocator: Option<Allocator>,
/// Buffer containing the vertex data for the initial triangle. /// Buffer containing the vertex data for the initial triangle.
pub(crate) vertex_buffer: vk::Buffer, pub(crate) vertex_buffer: vk::Buffer,
/// Memory allocation for the vertex buffer. /// Memory allocation for the vertex buffer.
pub(crate) vertex_allocation: Allocation, pub(crate) vertex_allocation: Option<Allocation>,
/// Buffer containing the index data for indexed drawing. /// Buffer containing the index data for indexed drawing.
pub(crate) index_buffer: vk::Buffer, pub(crate) index_buffer: vk::Buffer,
/// Memory allocation for the index buffer. /// Memory allocation for the index buffer.
pub(crate) index_allocation: Allocation, pub(crate) index_allocation: Option<Allocation>,
pub(crate) index_count: u32, pub(crate) index_count: u32,
/// The depth image used for depth testing. /// The depth image used for depth testing.
pub(crate) depth_image: vk::Image, pub(crate) depth_image: vk::Image,
/// Image view for the depth buffer. /// Image view for the depth buffer.
pub(crate) depth_image_view: vk::ImageView, pub(crate) depth_image_view: vk::ImageView,
/// Memory allocation for the depth image. /// Memory allocation for the depth image.
pub(crate) depth_allocation: Allocation, pub(crate) depth_allocation: Option<Allocation>,
/// Synchronization primitives for frame-by-frame execution. /// Synchronization primitives for frame-by-frame execution.
pub(crate) sync: SyncPrimitives, pub(crate) sync: Option<SyncPrimitives>,
/// Index of the current frame being processed (0 to `crate::MAX_FRAMES_IN_FLIGHT` - 1). /// Index of the current frame being processed (0 to `crate::MAX_FRAMES_IN_FLIGHT` - 1).
pub(crate) current_frame: usize, pub(crate) current_frame: usize,
} }
@ -75,8 +75,12 @@ pub struct Renderer {
impl Renderer { impl Renderer {
/// Renders a single frame. /// Renders a single frame.
pub fn draw_frame(&mut self) -> Result<(), RendererError> { pub fn draw_frame(&mut self) -> Result<(), RendererError> {
let in_flight_fence = self.sync.in_flight[self.current_frame]; let sync = self
let image_available_semaphore = self.sync.image_available[self.current_frame]; .sync
.as_ref()
.ok_or(RendererError::SyncPrimitivesMissing)?;
let in_flight_fence = sync.in_flight[self.current_frame];
let image_available_semaphore = sync.image_available[self.current_frame];
let cmd = self.command_buffers[self.current_frame]; let cmd = self.command_buffers[self.current_frame];
// 1. Wait for the current frame's GPU work to finish // 1. Wait for the current frame's GPU work to finish
@ -97,7 +101,11 @@ impl Renderer {
}; };
// Use the semaphore tied to this specific swapchain image for rendering completion // Use the semaphore tied to this specific swapchain image for rendering completion
let render_finished_semaphore = self.sync.render_finished[image_index as usize]; let render_finished_semaphore = self
.sync
.as_ref()
.ok_or(RendererError::SyncPrimitivesMissing)?
.render_finished[image_index as usize];
// 3. Reset and begin recording the command buffer // 3. Reset and begin recording the command buffer
unsafe { unsafe {
@ -352,20 +360,30 @@ impl Renderer {
unsafe { unsafe {
let _ = self.device.device_wait_idle(); let _ = self.device.device_wait_idle();
let _ = self let allocator = self
.allocator .allocator
.free(std::ptr::read(&raw const self.vertex_allocation)); .as_mut()
.ok_or(RendererError::AllocatorMissing)?;
if let Some(alloc) = self.vertex_allocation.take() {
let _ = allocator.free(alloc);
}
self.device.destroy_buffer(self.vertex_buffer, None); self.device.destroy_buffer(self.vertex_buffer, None);
let _ = self if let Some(alloc) = self.index_allocation.take() {
.allocator let _ = allocator.free(alloc);
.free(std::ptr::read(&raw const self.index_allocation)); }
self.device.destroy_buffer(self.index_buffer, None); self.device.destroy_buffer(self.index_buffer, None);
} }
let allocator = self
.allocator
.as_mut()
.ok_or(RendererError::AllocatorMissing)?;
let (v_buf, v_alloc) = create_gpu_buffer( let (v_buf, v_alloc) = create_gpu_buffer(
&self.device, &self.device,
&mut self.allocator, allocator,
bytemuck::cast_slice(vertices), bytemuck::cast_slice(vertices),
vk::BufferUsageFlags::VERTEX_BUFFER, vk::BufferUsageFlags::VERTEX_BUFFER,
"Chunk Vertex Buffer", "Chunk Vertex Buffer",
@ -373,16 +391,16 @@ impl Renderer {
let (i_buf, i_alloc) = crate::create_gpu_buffer( let (i_buf, i_alloc) = crate::create_gpu_buffer(
&self.device, &self.device,
&mut self.allocator, allocator,
bytemuck::cast_slice(indices), bytemuck::cast_slice(indices),
vk::BufferUsageFlags::INDEX_BUFFER, vk::BufferUsageFlags::INDEX_BUFFER,
"Chunk Index Buffer", "Chunk Index Buffer",
)?; )?;
self.vertex_buffer = v_buf; self.vertex_buffer = v_buf;
self.vertex_allocation = v_alloc; self.vertex_allocation = Some(v_alloc);
self.index_buffer = i_buf; self.index_buffer = i_buf;
self.index_allocation = i_alloc; self.index_allocation = Some(i_alloc);
self.index_count = indices.len() as u32; self.index_count = indices.len() as u32;
Ok(()) Ok(())
@ -398,46 +416,46 @@ impl Drop for Renderer {
self.device self.device
.destroy_pipeline_layout(self.pipeline_layout, None); .destroy_pipeline_layout(self.pipeline_layout, None);
if let Err(e) = self if let Some(allocator) = self.allocator.as_mut() {
.allocator if let Some(alloc) = self.vertex_allocation.take()
.free(std::ptr::read(&raw const self.vertex_allocation)) && let Err(e) = allocator.free(alloc)
{ {
tracing::error!("Failed to free vertex buffer allocation: {e}"); tracing::error!("Failed to free vertex buffer allocation: {e}");
} }
self.device.destroy_buffer(self.vertex_buffer, None); if let Some(alloc) = self.index_allocation.take()
&& let Err(e) = allocator.free(alloc)
if let Err(e) = self
.allocator
.free(std::ptr::read(&raw const self.index_allocation))
{ {
tracing::error!("Failed to free index buffer allocation: {e}"); tracing::error!("Failed to free index buffer allocation: {e}");
} }
self.device.destroy_buffer(self.index_buffer, None); if let Some(alloc) = self.depth_allocation.take()
&& let Err(e) = allocator.free(alloc)
self.device.destroy_image_view(self.depth_image_view, None);
if let Err(e) = self
.allocator
.free(std::ptr::read(&raw const self.depth_allocation))
{ {
tracing::error!("Failed to free depth image allocation: {e}"); tracing::error!("Failed to free depth image allocation: {e}");
} }
}
self.device.destroy_buffer(self.vertex_buffer, None);
self.device.destroy_buffer(self.index_buffer, None);
self.device.destroy_image_view(self.depth_image_view, None);
self.device.destroy_image(self.depth_image, None); self.device.destroy_image(self.depth_image, None);
// Drop the allocator before destroying the logical device so it can
// release any remaining memory blocks while the device is still valid.
drop(self.allocator.take());
self.device.destroy_command_pool(self.command_pool, None); self.device.destroy_command_pool(self.command_pool, None);
// Use the safe cleanup function from sync module if let Some(sync) = self.sync.take() {
let sync = std::ptr::read(&raw const self.sync);
crate::sync::destroy_sync_primitives(&self.device, sync); crate::sync::destroy_sync_primitives(&self.device, sync);
}
// Destroy the swapchain // Destroy image views before the swapchain that owns the underlying images.
self.swapchain_loader
.destroy_swapchain(self.swapchain, None);
// Destroy image views
for &view in &self.swapchain_image_views { for &view in &self.swapchain_image_views {
self.device.destroy_image_view(view, None); self.device.destroy_image_view(view, None);
} }
self.swapchain_loader
.destroy_swapchain(self.swapchain, None);
// Destroy the logical device // Destroy the logical device
self.device.destroy_device(None); self.device.destroy_device(None);