fix(queues): fix validation errors & docs

6 files changed, 116 insertions, 48 deletions

diff --git a/stockton-render/src/level.rs b/stockton-render/src/level.rs
index ceb361b..fc5111e 100644
--- a/stockton-render/src/level.rs
+++ b/stockton-render/src/level.rs
@@ -15,7 +15,7 @@ use stockton_skeleton::{
     },
     context::RenderingContext,
     draw_passes::{util::TargetSpecificResources, DrawPass, IntoDrawPass, PassPosition},
-    error::{EnvironmentError, LevelError, LockPoisoned},
+    error::{LevelError, LockPoisoned},
     mem::{DataPool, DepthBufferPool, StagingPool, TexturesPool},
     queue_negotiator::QueueNegotiator,
     texture::{resolver::TextureResolver, TexLoadQueue, TextureLoadConfig, TextureRepo},
@@ -453,8 +453,8 @@ where
     ) -> Result<Vec<(&'c QueueFamilyT, Vec<f32>)>> {
         queue_negotiator.find(adapter, &TexLoadQueue)?;
 
-        Ok(vec![queue_negotiator
-            .family_spec::<TexLoadQueue>(&adapter.queue_families, 1)
-            .ok_or(EnvironmentError::NoSuitableFamilies)?])
+        Ok(vec![
+            queue_negotiator.family_spec::<TexLoadQueue>(&adapter.queue_families, 1)?
+        ])
     }
 }
diff --git a/stockton-render/src/ui.rs b/stockton-render/src/ui.rs
index 2c8818c..ad53d80 100644
--- a/stockton-render/src/ui.rs
+++ b/stockton-render/src/ui.rs
@@ -9,7 +9,7 @@ use stockton_skeleton::{
     },
     context::RenderingContext,
     draw_passes::{util::TargetSpecificResources, DrawPass, IntoDrawPass, PassPosition},
-    error::{EnvironmentError, LockPoisoned},
+    error::LockPoisoned,
     mem::{DataPool, StagingPool, TexturesPool},
     queue_negotiator::QueueNegotiator,
     texture::{
@@ -331,9 +331,9 @@ impl<'a, P: PassPosition> IntoDrawPass<UiDrawPass<'a>, P> for () {
     ) -> Result<Vec<(&'c QueueFamilyT, Vec<f32>)>> {
         queue_negotiator.find(adapter, &TexLoadQueue)?;
 
-        Ok(vec![queue_negotiator
-            .family_spec::<TexLoadQueue>(&adapter.queue_families, 1)
-            .ok_or(EnvironmentError::NoSuitableFamilies)?])
+        Ok(vec![
+            queue_negotiator.family_spec::<TexLoadQueue>(&adapter.queue_families, 1)?
+        ])
     }
 }
 
diff --git a/stockton-skeleton/src/context.rs b/stockton-skeleton/src/context.rs
index be627f2..bbd0164 100644
--- a/stockton-skeleton/src/context.rs
+++ b/stockton-skeleton/src/context.rs
@@ -111,10 +111,20 @@ impl RenderingContext {
         // Device & Queue groups
         let (device_lock, queue_groups) = {
             // TODO: This sucks, but hal is restrictive on how we can pass this specific argument.
-            let queue_families_specs_real: Vec<_> = queue_families_specs
-                .iter()
-                .map(|(qf, ns)| (*qf, ns.as_slice()))
-                .collect();
+
+            // Deduplicate families & convert to specific type.
+            let mut queue_families_specs_real = Vec::with_capacity(queue_families_specs.len());
+            for (qf, ns) in queue_families_specs.iter_mut() {
+                if let Some(existing_family_spec) = queue_families_specs_real
+                    .iter()
+                    .position(|(qf2, _): &(&QueueFamilyT, &[f32])| qf2.id() == qf.id())
+                {
+                    ns.extend(queue_families_specs_real[existing_family_spec].1.iter());
+                    queue_families_specs_real[existing_family_spec] = (*qf, ns.as_slice());
+                } else {
+                    queue_families_specs_real.push((*qf, ns.as_slice()))
+                }
+            }
 
             let gpu = unsafe {
                 adapter
@@ -166,7 +176,6 @@ impl RenderingContext {
 
         let queue = queue_negotiator
             .get_queue::<DrawQueue>()
-            .ok_or(EnvironmentError::NoQueues)
             .context("Error getting draw queue")?;
 
         Ok(RenderingContext {
diff --git a/stockton-skeleton/src/error.rs b/stockton-skeleton/src/error.rs
index 6d7e7ad..15df7ff 100644
--- a/stockton-skeleton/src/error.rs
+++ b/stockton-skeleton/src/error.rs
@@ -55,6 +55,9 @@ pub enum EnvironmentError {
 pub enum UsageError {
     #[error("Attempt to create mappable memory block from non-mappable memory")]
     NonMappableMemory,
+
+    #[error("Called get_queue without properly requesting the queue beforehand.")]
+    QueueNegotiatorMisuse,
 }
 
 /// Indicates an issue with the level object being used
diff --git a/stockton-skeleton/src/queue_negotiator.rs b/stockton-skeleton/src/queue_negotiator.rs
index 879a935..f3e38fd 100644
--- a/stockton-skeleton/src/queue_negotiator.rs
+++ b/stockton-skeleton/src/queue_negotiator.rs
@@ -1,6 +1,40 @@
-use crate::{error::EnvironmentError, types::*};
+//! Used for requesting appropriate queue families, and sharing/allocating queues as necessary.
+//! This is created by `RenderingContext`, and should mostly be accessed from [`crate::draw_passes::IntoDrawPass`].
+//!
+//! For example, to use a `TexLoadQueue` in your drawpass:
+//! ```
+//! # use crate::{types::*, texture::TexLoadQueue};
+//! fn find_aux_queues<'c>(
+//!    adapter: &'c Adapter,
+//!    queue_negotiator: &mut QueueNegotiator,
+//! ) -> Result<Vec<(&'c QueueFamilyT, Vec<f32>)>> {
+//!     queue_negotiator.find(adapter, &TexLoadQueue)?;
+//!
+//!     Ok(vec![queue_negotiator
+//!         .family_spec::<TexLoadQueue>(&adapter.queue_families, 1)
+//!         .ok_or(EnvironmentError::NoSuitableFamilies)?])
+//! }
+//! ```
+//!
+//! Then get your queue in [`crate::draw_passes::IntoDrawPass::init`]
+//! ```
+//! # use crate::{types::*, context::RenderingContext, texture::TexLoadQueue};
+//! # use stockton_types::Session;
+//! fn init(
+//!     self,
+//!     session: &mut Session,
+//!     context: &mut RenderingContext,
+//! ) -> Result<LevelDrawPass<'a, M>> {
+//!     let queue = context.queue_negotiator_mut().get_queue::<TexLoadQueue>().unwrap();
+//! }
+//! ```
+
+use crate::{
+    error::{EnvironmentError, UsageError},
+    types::*,
+};
 
-use anyhow::{Error, Result};
+use anyhow::{bail, Error, Result};
 use hal::queue::family::QueueFamilyId;
 use std::{
     any::TypeId,
@@ -8,7 +42,8 @@ use std::{
     sync::{Arc, RwLock},
 };
 
-type SharedQueue = Arc<RwLock<QueueT>>;
+/// A queue, possibly shared between threads.
+pub type SharedQueue = Arc<RwLock<QueueT>>;
 
 /// Used to find appropriate queue families and share queues from them as needed.
 pub struct QueueNegotiator {
@@ -17,13 +52,16 @@ pub struct QueueNegotiator {
     all: Vec<QueueGroup>,
 }
 
-/// Can be used to select a specific queue family
+/// Can be used to select an appropriate queue family
 pub trait QueueFamilySelector: 'static {
-    /// Check if the given family is suitable
+    /// Return true if the given family is suitable
     fn is_suitable(&self, family: &QueueFamilyT) -> bool;
 }
 
 impl QueueNegotiator {
+    /// Attempt to find an appropriate queue family using the given selector.
+    /// Returns early if the *type* of the selector has already been allocated a family.
+    /// This should usually be called by [`crate::draw_passes::IntoDrawPass::find_aux_queues`].
     pub fn find<T: QueueFamilySelector>(&mut self, adapter: &Adapter, filter: &T) -> Result<()> {
         if self.family_ids.contains_key(&TypeId::of::<T>()) {
             return Ok(());
@@ -53,46 +91,79 @@ impl QueueNegotiator {
         Ok(())
     }
 
-    pub fn set_queue_groups(&mut self, queue_groups: Vec<QueueGroup>) {
-        self.all = queue_groups
-    }
-
-    pub fn get_queue<T: QueueFamilySelector>(&mut self) -> Option<Arc<RwLock<QueueT>>> {
+    /// Get a (possibly shared) queue. You should prefer to call this once and store the result.
+    /// You should already have called [`self::QueueNegotiator::find`] and [`self::QueueNegotiator::family_spec`],
+    /// otherwise this will return an error.
+    ///
+    /// Round-robin allocation is used to try to fairly distribute work between each queue.
+    /// The family of the queue returned is guaranteed to meet the spec of the `QueueFamilySelector` originally used by `find`.
+    pub fn get_queue<T: QueueFamilySelector>(&mut self) -> Result<Arc<RwLock<QueueT>>> {
         let tid = TypeId::of::<T>();
-        let family_id = self.family_ids.get(&tid)?;
-        log::debug!("{:?}", self.all);
-        log::debug!("{:?}", self.already_allocated);
+        let family_id = self
+            .family_ids
+            .get(&tid)
+            .ok_or(UsageError::QueueNegotiatorMisuse)?;
+
         match self
             .all
             .iter()
             .position(|x| !x.queues.is_empty() && x.family == *family_id)
         {
             Some(idx) => {
-                // At least one remaining queue
+                // At least one remaining unused queue
                 let queue = self.all[idx].queues.pop().unwrap();
                 let queue = Arc::new(RwLock::new(queue));
 
                 self.add_to_allocated::<T>(queue.clone());
 
-                Some(queue)
+                Ok(queue)
             }
             None => match self.already_allocated.get_mut(&tid) {
                 Some((queues, next_share)) => {
                     let queue = (&queues[*next_share]).clone();
 
-                    *next_share += 1;
+                    *next_share = (*next_share + 1) % queues.len();
 
-                    Some(queue)
+                    Ok(queue)
                 }
-                None => None,
+                None => bail!(EnvironmentError::NoQueues),
             },
         }
     }
 
+    /// Convenience function to get a queue spec for the given selector.
+    /// You should probably call this from [`crate::draw_passes::IntoDrawPass::find_aux_queues`].
+    /// `count` is the maximum number of individual queues to request. You may get less than this, in which case they will be shared.
+    /// This will return an error if you haven't called [`self::QueueNegotiator::find`] beforehand, or if there were no suitable queue families.
+    pub fn family_spec<'a, T: QueueFamilySelector>(
+        &self,
+        queue_families: &'a [QueueFamilyT],
+        count: usize,
+    ) -> Result<(&'a QueueFamilyT, Vec<f32>)> {
+        let qf_id = self
+            .family::<T>()
+            .ok_or(UsageError::QueueNegotiatorMisuse)?;
+
+        let qf = queue_families
+            .iter()
+            .find(|x| x.id() == qf_id)
+            .ok_or(EnvironmentError::NoSuitableFamilies)?;
+        let v = vec![1.0; count];
+
+        Ok((qf, v))
+    }
+
+    /// Get the queue family ID being used by the given selector
     pub fn family<T: QueueFamilySelector>(&self) -> Option<QueueFamilyId> {
         self.family_ids.get(&TypeId::of::<T>()).cloned()
     }
 
+    /// Used internally to get the queue groups from the adapter.
+    pub(crate) fn set_queue_groups(&mut self, queue_groups: Vec<QueueGroup>) {
+        self.all = queue_groups
+    }
+
+    /// Used internally to mark that we've started sharing a queue
     fn add_to_allocated<T: QueueFamilySelector>(&mut self, queue: Arc<RwLock<QueueT>>) {
         let tid = TypeId::of::<T>();
         match self.already_allocated.get_mut(&tid) {
@@ -104,19 +175,6 @@ impl QueueNegotiator {
             }
         }
     }
-
-    pub fn family_spec<'a, T: QueueFamilySelector>(
-        &self,
-        queue_families: &'a [QueueFamilyT],
-        count: usize,
-    ) -> Option<(&'a QueueFamilyT, Vec<f32>)> {
-        let qf_id = self.family::<T>()?;
-
-        let qf = queue_families.iter().find(|x| x.id() == qf_id)?;
-        let v = vec![1.0; count];
-
-        Some((qf, v))
-    }
 }
 
 impl Default for QueueNegotiator {
@@ -129,6 +187,7 @@ impl Default for QueueNegotiator {
     }
 }
 
+/// A queue suitable for drawing to a given surface with.
 pub struct DrawQueue {
     pub surface: SurfaceT,
 }
diff --git a/stockton-skeleton/src/texture/loader.rs b/stockton-skeleton/src/texture/loader.rs
index 7f630ab..ea42c29 100644
--- a/stockton-skeleton/src/texture/loader.rs
+++ b/stockton-skeleton/src/texture/loader.rs
@@ -216,10 +216,7 @@ where
             .queue_negotiator_mut()
             .family::<Q>()
             .ok_or(EnvironmentError::NoSuitableFamilies)?;
-        let queue_lock = context
-            .queue_negotiator_mut()
-            .get_queue::<Q>()
-            .ok_or(EnvironmentError::NoQueues)?;
+        let queue_lock = context.queue_negotiator_mut().get_queue::<Q>()?;
 
         // Memory pools
         let tex_mempool = context.memory_pool()?.clone();