refactor(skeleton): clean up targetchain

5 files changed, 179 insertions, 221 deletions

diff --git a/stockton-skeleton/src/builders/pipeline.rs b/stockton-skeleton/src/builders/pipeline.rs
index 9f4937f..82673a2 100644
--- a/stockton-skeleton/src/builders/pipeline.rs
+++ b/stockton-skeleton/src/builders/pipeline.rs
@@ -1,7 +1,5 @@
 use super::{renderpass::RenderpassSpec, shader::ShaderDesc};
-use crate::{
-    error::EnvironmentError, target::SwapchainProperties, types::*, utils::get_pixel_size,
-};
+use crate::{error::EnvironmentError, types::*, utils::get_pixel_size};
 
 use std::{mem::ManuallyDrop, ops::Range};
 
@@ -114,7 +112,6 @@ impl PipelineSpec {
         self,
         device: &mut DeviceT,
         extent: hal::image::Extent,
-        _swapchain_properties: &SwapchainProperties,
         set_layouts: T,
     ) -> Result<CompletePipeline> {
         // Renderpass
diff --git a/stockton-skeleton/src/context.rs b/stockton-skeleton/src/context.rs
index d1997d3..348ddbd 100644
--- a/stockton-skeleton/src/context.rs
+++ b/stockton-skeleton/src/context.rs
@@ -10,7 +10,14 @@ use std::{
 };
 
 use anyhow::{Context, Result};
-use hal::{pool::CommandPoolCreateFlags, PhysicalDeviceProperties};
+use hal::{
+    format::{ChannelType, Format, ImageFeature},
+    image::{Extent, FramebufferAttachment, Usage, ViewCapabilities},
+    pool::CommandPoolCreateFlags,
+    pso::Viewport,
+    window::{CompositeAlphaMode, PresentMode},
+    PhysicalDeviceProperties,
+};
 use log::debug;
 
 use winit::window::Window;
@@ -18,7 +25,7 @@ use winit::window::Window;
 use super::{
     draw_passes::{DrawPass, IntoDrawPass},
     queue_negotiator::{DrawQueue, QueueNegotiator},
-    target::{SwapchainProperties, TargetChain},
+    target::TargetChain,
 };
 use crate::{
     draw_passes::Singular,
@@ -62,6 +69,9 @@ pub struct RenderingContext {
 
     /// The list of memory pools
     memory_pools: HashMap<TypeId, Box<dyn Any>>,
+
+    /// Shared properties for this context
+    properties: ContextProperties,
 }
 
 impl RenderingContext {
@@ -138,9 +148,9 @@ impl RenderingContext {
 
         queue_negotiator.set_queue_groups(queue_groups);
 
-        // Figure out what our swapchain will look like
-        let swapchain_properties = SwapchainProperties::find_best(&adapter, &surface)
-            .context("Error getting properties for swapchain")?;
+        // Context properties
+        let properties = ContextProperties::find_best(&adapter, &surface)
+            .context("Error getting context properties")?;
 
         // Lock device
         let mut device = device_lock
@@ -148,7 +158,7 @@ impl RenderingContext {
             .map_err(|_| LockPoisoned::Device)
             .context("Error getting device lock")?;
 
-        debug!("Detected swapchain properties: {:?}", swapchain_properties);
+        debug!("Detected swapchain properties: {:?}", properties);
 
         // Command pool
         let mut cmd_pool = unsafe {
@@ -162,14 +172,8 @@ impl RenderingContext {
         .context("Error creating draw command pool")?;
 
         // Swapchain and associated resources
-        let target_chain = TargetChain::new(
-            &mut device,
-            &adapter,
-            surface,
-            &mut cmd_pool,
-            swapchain_properties,
-        )
-        .context("Error creating target chain")?;
+        let target_chain = TargetChain::new(&mut device, surface, &mut cmd_pool, &properties)
+            .context("Error creating target chain")?;
 
         // Unlock device
         drop(device);
@@ -193,6 +197,7 @@ impl RenderingContext {
 
             pixels_per_point: window.scale_factor() as f32,
             memory_pools: HashMap::new(),
+            properties,
         })
     }
 
@@ -213,18 +218,12 @@ impl RenderingContext {
         let surface = ManuallyDrop::into_inner(read(&self.target_chain))
             .deactivate_with_recyling(&mut device, &mut self.cmd_pool);
 
-        let properties = SwapchainProperties::find_best(&self.adapter, &surface)
+        self.properties = ContextProperties::find_best(&self.adapter, &surface)
             .context("Error finding best swapchain properties")?;
 
         self.target_chain = ManuallyDrop::new(
-            TargetChain::new(
-                &mut device,
-                &self.adapter,
-                surface,
-                &mut self.cmd_pool,
-                properties,
-            )
-            .context("Error creating target chain")?,
+            TargetChain::new(&mut device, surface, &mut self.cmd_pool, &self.properties)
+                .context("Error creating target chain")?,
         );
         Ok(())
     }
@@ -309,6 +308,11 @@ impl RenderingContext {
             .get(&TypeId::of::<P>())
             .map(|x| x.downcast_ref().unwrap())
     }
+
+    /// Get a reference to the rendering context's properties.
+    pub fn properties(&self) -> &ContextProperties {
+        &self.properties
+    }
 }
 
 impl core::ops::Drop for RenderingContext {
@@ -332,3 +336,116 @@ impl core::ops::Drop for RenderingContext {
         }
     }
 }
+
+/// Common properties shared by this entire context
+#[derive(Debug, Clone)]
+pub struct ContextProperties {
+    /// Format to be used by colour attachments. Used by swapchain images.
+    pub color_format: Format,
+
+    /// Recommended format to be used by depth attachments.
+    pub depth_format: Format,
+
+    /// The present mode being used by the context
+    pub present_mode: PresentMode,
+
+    /// How the swapchain is handling alpha values in the end image
+    pub composite_alpha_mode: CompositeAlphaMode,
+
+    pub viewport: Viewport,
+    pub extent: Extent,
+
+    /// The maximum number of frames we queue at once.
+    pub image_count: u32,
+}
+
+impl ContextProperties {
+    /// Find the best properties for the given adapter and surface
+    pub fn find_best(
+        adapter: &Adapter,
+        surface: &SurfaceT,
+    ) -> Result<ContextProperties, EnvironmentError> {
+        let caps = surface.capabilities(&adapter.physical_device);
+        let formats = surface.supported_formats(&adapter.physical_device);
+
+        // Use the first SRGB format our surface prefers
+        let color_format = match formats {
+            Some(formats) => formats
+                .iter()
+                .find(|format| format.base_format().1 == ChannelType::Srgb)
+                .copied()
+                .ok_or(EnvironmentError::ColorFormat),
+            None => Ok(Format::Rgba8Srgb),
+        }?;
+
+        // Use the most preferable format our adapter prefers.
+        let depth_format = *[
+            Format::D32SfloatS8Uint,
+            Format::D24UnormS8Uint,
+            Format::D32Sfloat,
+        ]
+        .iter()
+        .find(|format| {
+            format.is_depth()
+                && adapter
+                    .physical_device
+                    .format_properties(Some(**format))
+                    .optimal_tiling
+                    .contains(ImageFeature::DEPTH_STENCIL_ATTACHMENT)
+        })
+        .ok_or(EnvironmentError::DepthFormat)?;
+
+        // V-Sync if possible
+        let present_mode = [
+            PresentMode::MAILBOX,
+            PresentMode::FIFO,
+            PresentMode::RELAXED,
+            PresentMode::IMMEDIATE,
+        ]
+        .iter()
+        .cloned()
+        .find(|pm| caps.present_modes.contains(*pm))
+        .ok_or(EnvironmentError::PresentMode)?;
+
+        // Prefer opaque
+        let composite_alpha_mode = [
+            CompositeAlphaMode::OPAQUE,
+            CompositeAlphaMode::INHERIT,
+            CompositeAlphaMode::PREMULTIPLIED,
+            CompositeAlphaMode::POSTMULTIPLIED,
+        ]
+        .iter()
+        .cloned()
+        .find(|ca| caps.composite_alpha_modes.contains(*ca))
+        .ok_or(EnvironmentError::CompositeAlphaMode)?;
+
+        let extent = caps.extents.end().to_extent(); // Size
+        let viewport = Viewport {
+            rect: extent.rect(),
+            depth: 0.0..1.0,
+        };
+
+        Ok(ContextProperties {
+            color_format,
+            depth_format,
+            present_mode,
+            composite_alpha_mode,
+            extent,
+            viewport,
+            image_count: if present_mode == PresentMode::MAILBOX {
+                ((*caps.image_count.end()) - 1).min((*caps.image_count.start()).max(3))
+            } else {
+                ((*caps.image_count.end()) - 1).min((*caps.image_count.start()).max(2))
+            },
+        })
+    }
+
+    /// Get the framebuffer attachment to use for swapchain images
+    pub fn swapchain_framebuffer_attachment(&self) -> FramebufferAttachment {
+        FramebufferAttachment {
+            usage: Usage::COLOR_ATTACHMENT,
+            format: self.color_format,
+            view_caps: ViewCapabilities::empty(),
+        }
+    }
+}
diff --git a/stockton-skeleton/src/lib.rs b/stockton-skeleton/src/lib.rs
index 785fb30..b514531 100644
--- a/stockton-skeleton/src/lib.rs
+++ b/stockton-skeleton/src/lib.rs
@@ -72,7 +72,7 @@ impl<DP: DrawPass<Singular>> Renderer<DP> {
     }
 
     pub fn get_aspect_ratio(&self) -> f32 {
-        let e = self.context.target_chain().properties().extent;
+        let e = self.context.properties().extent;
         e.width as f32 / e.height as f32
     }
 
diff --git a/stockton-skeleton/src/mem.rs b/stockton-skeleton/src/mem.rs
index 85bf295..03d74ea 100644
--- a/stockton-skeleton/src/mem.rs
+++ b/stockton-skeleton/src/mem.rs
@@ -198,7 +198,7 @@ mod rendy {
                     .create_image(
                         Kind::D2(16, 16, 1, 1),
                         1,
-                        context.target_chain().properties().depth_format,
+                        context.properties().depth_format,
                         Tiling::Optimal,
                         Usage::SAMPLED,
                         SparseFlags::empty(),
diff --git a/stockton-skeleton/src/target.rs b/stockton-skeleton/src/target.rs
index c3a212e..647aa96 100644
--- a/stockton-skeleton/src/target.rs
+++ b/stockton-skeleton/src/target.rs
@@ -1,4 +1,12 @@
 //! Resources needed for drawing on the screen, including sync objects
+//! You likely won't need to interact with this directly
+
+use crate::{
+    context::ContextProperties,
+    draw_passes::{DrawPass, Singular},
+    types::*,
+};
+use stockton_types::Session;
 
 use std::{
     borrow::Borrow,
@@ -8,177 +16,53 @@ use std::{
 
 use hal::{
     command::CommandBufferFlags,
-    format::{ChannelType, Format, ImageFeature},
-    image::{Extent, FramebufferAttachment, Usage as ImgUsage, ViewCapabilities},
-    pso::Viewport,
-    window::{CompositeAlphaMode, Extent2D, PresentMode, SwapchainConfig},
+    image::Usage as ImgUsage,
+    window::{Extent2D, SwapchainConfig},
 };
 
-use super::draw_passes::DrawPass;
-use crate::{draw_passes::Singular, error::EnvironmentError, types::*};
 use anyhow::{Context, Result};
-use stockton_types::Session;
-
-#[derive(Debug, Clone)]
-pub struct SwapchainProperties {
-    pub format: Format,
-    pub depth_format: Format,
-    pub present_mode: PresentMode,
-    pub composite_alpha_mode: CompositeAlphaMode,
-    pub viewport: Viewport,
-    pub extent: Extent,
-    pub image_count: u32,
-}
-
-impl SwapchainProperties {
-    pub fn find_best(
-        adapter: &Adapter,
-        surface: &SurfaceT,
-    ) -> Result<SwapchainProperties, EnvironmentError> {
-        let caps = surface.capabilities(&adapter.physical_device);
-        let formats = surface.supported_formats(&adapter.physical_device);
-
-        // Find which settings we'll actually use based on preset preferences
-        let format = match formats {
-            Some(formats) => formats
-                .iter()
-                .find(|format| format.base_format().1 == ChannelType::Srgb)
-                .copied()
-                .ok_or(EnvironmentError::ColorFormat),
-            None => Ok(Format::Rgba8Srgb),
-        }?;
-
-        let depth_format = *[
-            Format::D32SfloatS8Uint,
-            Format::D24UnormS8Uint,
-            Format::D32Sfloat,
-        ]
-        .iter()
-        .find(|format| {
-            format.is_depth()
-                && adapter
-                    .physical_device
-                    .format_properties(Some(**format))
-                    .optimal_tiling
-                    .contains(ImageFeature::DEPTH_STENCIL_ATTACHMENT)
-        })
-        .ok_or(EnvironmentError::DepthFormat)?;
-
-        let present_mode = [
-            PresentMode::MAILBOX,
-            PresentMode::FIFO,
-            PresentMode::RELAXED,
-            PresentMode::IMMEDIATE,
-        ]
-        .iter()
-        .cloned()
-        .find(|pm| caps.present_modes.contains(*pm))
-        .ok_or(EnvironmentError::PresentMode)?;
-
-        let composite_alpha_mode = [
-            CompositeAlphaMode::OPAQUE,
-            CompositeAlphaMode::INHERIT,
-            CompositeAlphaMode::PREMULTIPLIED,
-            CompositeAlphaMode::POSTMULTIPLIED,
-        ]
-        .iter()
-        .cloned()
-        .find(|ca| caps.composite_alpha_modes.contains(*ca))
-        .ok_or(EnvironmentError::CompositeAlphaMode)?;
-
-        let extent = caps.extents.end().to_extent(); // Size
-        let viewport = Viewport {
-            rect: extent.rect(),
-            depth: 0.0..1.0,
-        };
-
-        Ok(SwapchainProperties {
-            format,
-            depth_format,
-            present_mode,
-            composite_alpha_mode,
-            extent,
-            viewport,
-            image_count: if present_mode == PresentMode::MAILBOX {
-                ((*caps.image_count.end()) - 1).min((*caps.image_count.start()).max(3))
-            } else {
-                ((*caps.image_count.end()) - 1).min((*caps.image_count.start()).max(2))
-            },
-        })
-    }
-
-    pub fn framebuffer_attachment(&self) -> FramebufferAttachment {
-        FramebufferAttachment {
-            usage: ImgUsage::COLOR_ATTACHMENT,
-            format: self.format,
-            view_caps: ViewCapabilities::empty(),
-        }
-    }
-}
 
+/// Holds our swapchain and other resources for drawing each frame
 pub struct TargetChain {
     /// Surface we're targeting
     surface: ManuallyDrop<SurfaceT>,
-    properties: SwapchainProperties,
-
-    /// Resources tied to each target frame in the swapchain
-    targets: Box<[TargetResources]>,
-
-    /// Sync objects used in drawing
-    /// These are seperated from the targets because we don't necessarily always match up indexes
-    sync_objects: Box<[SyncObjects]>,
 
-    /// The last set of sync objects used
-    last_syncs: usize,
+    /// Command buffers and sync objects used when drawing
+    resources: Box<[(CommandBufferT, SyncObjects)]>,
 
     /// Last image index of the swapchain drawn to
-    last_image: u32,
+    last_resources: usize,
 }
 
 impl TargetChain {
     pub fn new(
         device: &mut DeviceT,
-        adapter: &Adapter,
         mut surface: SurfaceT,
         cmd_pool: &mut CommandPoolT,
-        properties: SwapchainProperties,
+        properties: &ContextProperties,
     ) -> Result<TargetChain> {
-        let caps = surface.capabilities(&adapter.physical_device);
-
-        // Number of frames to pre-render
-        let image_count = if properties.present_mode == PresentMode::MAILBOX {
-            ((*caps.image_count.end()) - 1).min((*caps.image_count.start()).max(3))
-        } else {
-            ((*caps.image_count.end()) - 1).min((*caps.image_count.start()).max(2))
-        };
-
-        // Swap config
+        // Create swapchain
         let swap_config = SwapchainConfig {
             present_mode: properties.present_mode,
             composite_alpha_mode: properties.composite_alpha_mode,
-            format: properties.format,
+            format: properties.color_format,
             extent: Extent2D {
                 width: properties.extent.width,
                 height: properties.extent.height,
             },
-            image_count,
+            image_count: properties.image_count,
             image_layers: 1,
             image_usage: ImgUsage::COLOR_ATTACHMENT,
         };
 
-        let _fat = swap_config.framebuffer_attachment();
-        let mut targets: Vec<TargetResources> =
-            Vec::with_capacity(swap_config.image_count as usize);
-        let mut sync_objects: Vec<SyncObjects> =
-            Vec::with_capacity(swap_config.image_count as usize);
+        // Create command buffers and sync objects
+        let mut resources = Vec::with_capacity(swap_config.image_count as usize);
 
         for _ in 0..swap_config.image_count {
-            targets.push(
-                TargetResources::new(device, cmd_pool, &properties)
-                    .context("Error creating target resources")?,
-            );
-
-            sync_objects.push(SyncObjects::new(device).context("Error creating sync objects")?);
+            resources.push((
+                unsafe { cmd_pool.allocate_one(hal::command::Level::Primary) },
+                SyncObjects::new(device).context("Error creating sync objects")?,
+            ));
         }
 
         // Configure Swapchain
@@ -190,11 +74,8 @@ impl TargetChain {
 
         Ok(TargetChain {
             surface: ManuallyDrop::new(surface),
-            targets: targets.into_boxed_slice(),
-            sync_objects: sync_objects.into_boxed_slice(),
-            properties,
-            last_syncs: (image_count - 1) as usize, // This means the next one to be used is index 0
-            last_image: 0,
+            resources: resources.into_boxed_slice(),
+            last_resources: (properties.image_count - 1) as usize, // This means the next one to be used is index 0
         })
     }
 
@@ -218,12 +99,10 @@ impl TargetChain {
     ) -> SurfaceT {
         use core::ptr::read;
         unsafe {
-            for i in 0..self.targets.len() {
-                read(&self.targets[i]).deactivate(device, cmd_pool);
-            }
-
-            for i in 0..self.sync_objects.len() {
-                read(&self.sync_objects[i]).deactivate(device);
+            for i in 0..self.resources.len() {
+                let (cmd_buf, syncs) = read(&self.resources[i]);
+                cmd_pool.free(once(cmd_buf));
+                syncs.deactivate(device);
             }
 
             self.surface.unconfigure_swapchain(device);
@@ -239,11 +118,9 @@ impl TargetChain {
         dp: &mut DP,
         session: &Session,
     ) -> Result<()> {
-        self.last_syncs = (self.last_syncs + 1) % self.sync_objects.len();
-        self.last_image = (self.last_image + 1) % self.targets.len() as u32;
+        self.last_resources = (self.last_resources + 1) % self.resources.len();
 
-        let syncs = &mut self.sync_objects[self.last_syncs];
-        let target = &mut self.targets[self.last_image as usize];
+        let (cmd_buffer, syncs) = &mut self.resources[self.last_resources];
 
         // Get the image
         let (img, _) = unsafe {
@@ -264,18 +141,18 @@ impl TargetChain {
 
         // Record commands
         unsafe {
-            target.cmd_buffer.begin_primary(CommandBufferFlags::empty());
+            cmd_buffer.begin_primary(CommandBufferFlags::empty());
 
-            dp.queue_draw(session, img.borrow(), &mut target.cmd_buffer)
+            dp.queue_draw(session, img.borrow(), cmd_buffer)
                 .context("Error in draw pass")?;
 
-            target.cmd_buffer.finish();
+            cmd_buffer.finish();
         }
 
         // Submit it
         unsafe {
             command_queue.submit(
-                once(&*target.cmd_buffer),
+                once(&*cmd_buffer),
                 empty(),
                 once(&*syncs.render_complete),
                 Some(&mut syncs.present_complete),
@@ -287,39 +164,6 @@ impl TargetChain {
 
         Ok(())
     }
-
-    /// Get a reference to the target chain's properties.
-    pub fn properties(&self) -> &SwapchainProperties {
-        &self.properties
-    }
-}
-
-/// Resources for a single target frame, including sync objects
-pub struct TargetResources {
-    /// Command buffer to use when drawing
-    pub cmd_buffer: ManuallyDrop<CommandBufferT>,
-}
-
-impl TargetResources {
-    pub fn new(
-        _device: &mut DeviceT,
-        cmd_pool: &mut CommandPoolT,
-        _properties: &SwapchainProperties,
-    ) -> Result<TargetResources> {
-        // Command Buffer
-        let cmd_buffer = unsafe { cmd_pool.allocate_one(hal::command::Level::Primary) };
-
-        Ok(TargetResources {
-            cmd_buffer: ManuallyDrop::new(cmd_buffer),
-        })
-    }
-
-    pub fn deactivate(self, _device: &mut DeviceT, cmd_pool: &mut CommandPoolT) {
-        use core::ptr::read;
-        unsafe {
-            cmd_pool.free(once(ManuallyDrop::into_inner(read(&self.cmd_buffer))));
-        }
-    }
 }
 
 pub struct SyncObjects {