Merge pull request #2026 from hannobraun/handles

Introduce `HandleSet`, `HandleIter`

crates/fj-core/src/algorithms/approx/face.rs

@@ -7,15 +7,15 @@ use std::{collections::BTreeSet, ops::Deref};
 use fj_interop::mesh::Color;
 
 use crate::{
-    objects::{Face, FaceSet, Handedness},
+    objects::{Face, Handedness, HandleIter},
     validate::ValidationConfig,
 };
 
 use super::{
     cycle::CycleApprox, edge::EdgeApproxCache, Approx, ApproxPoint, Tolerance,
 };
 
-impl Approx for &FaceSet {
+impl Approx for HandleIter<'_, Face> {
     type Approximation = BTreeSet<FaceApprox>;
     type Cache = EdgeApproxCache;
 

crates/fj-core/src/algorithms/sweep/sketch.rs

@@ -1,7 +1,7 @@
 use fj_math::Vector;
 
 use crate::{
-    objects::{Sketch, Solid, Surface},
+    objects::{Face, Sketch, Solid, Surface},
     operations::Insert,
     services::Services,
     storage::Handle,
@@ -18,10 +18,13 @@ impl Sweep for (Handle<Sketch>, Handle<Surface>) {
         cache: &mut SweepCache,
         services: &mut Services,
     ) -> Self::Swept {
+        let (sketch, surface) = self;
         let path = path.into();
 
         let mut shells = Vec::new();
-        for face in self.0.faces(self.1, services) {
+        for region in sketch.regions() {
+            let face =
+                Face::new(surface.clone(), region.clone()).insert(services);
             let shell = face.sweep_with_cache(path, cache, services);
             shells.push(shell);
         }

crates/fj-core/src/algorithms/transform/face.rs

@@ -1,7 +1,7 @@
 use fj_math::Transform;
 
 use crate::{
-    objects::{Face, FaceSet, Region},
+    objects::{Face, Region},
     operations::Insert,
     services::Services,
 };
@@ -36,20 +36,3 @@ impl TransformObject for Face {
         Self::new(surface, region)
     }
 }
-
-impl TransformObject for FaceSet {
-    fn transform_with_cache(
-        self,
-        transform: &Transform,
-        services: &mut Services,
-        cache: &mut TransformCache,
-    ) -> Self {
-        let mut faces = Self::new();
-        faces.extend(
-            self.into_iter().map(|face| {
-                face.transform_with_cache(transform, services, cache)
-            }),
-        );
-        faces
-    }
-}

crates/fj-core/src/algorithms/transform/shell.rs

@@ -11,10 +11,10 @@ impl TransformObject for Shell {
         services: &mut Services,
         cache: &mut TransformCache,
     ) -> Self {
-        let faces =
-            self.faces().clone().into_iter().map(|face| {
-                face.transform_with_cache(transform, services, cache)
-            });
+        let faces = self
+            .faces()
+            .cloned()
+            .map(|face| face.transform_with_cache(transform, services, cache));
 
         Self::new(faces)
     }

crates/fj-core/src/objects/handles.rs

@@ -0,0 +1,115 @@
+use std::{collections::BTreeSet, fmt::Debug};
+
+use crate::storage::Handle;
+
+/// An ordered set of object handles
+///
+/// This is an internal data structure that is used within objects that
+/// reference multiple other objects of the same type. It does not contain any
+/// duplicate elements, and it maintains the insertion order of those elements.
+///
+/// `HandleSet` implement `FromIterator`, but it must never be constructed from
+/// an iterator that contains duplicate handles. This will result in a panic.
+#[derive(Clone, Debug, Eq, PartialEq, Hash, Ord, PartialOrd)]
+pub struct HandleSet<T> {
+    // This is supposed to be a set data structure, so what is that `Vec` doing
+    // here? Well, it's here because we need it to preserve insertion order, but
+    // that doesn't explain why it is here *alone*.
+    //
+    // If you look closely, you'll notice that this is an immutable data
+    // structure (since it is used in objects, and objects themselves are
+    // immutable). We make sure there are no duplicates when this is
+    // constructed (see the `FromIterator` implementation below), but after
+    // that, we're fine.
+    inner: Vec<Handle<T>>,
+}
+
+impl<T> HandleSet<T> {
+    pub fn len(&self) -> usize {
+        self.inner.len()
+    }
+
+    pub fn is_empty(&self) -> bool {
+        self.inner.is_empty()
+    }
+
+    pub fn nth(&self, index: usize) -> Option<&Handle<T>> {
+        self.inner.get(index)
+    }
+
+    pub fn iter(&self) -> HandleIter<T> {
+        HandleIter {
+            handles: &self.inner,
+            next_index: 0,
+        }
+    }
+}
+
+impl<O> FromIterator<Handle<O>> for HandleSet<O>
+where
+    O: Debug + Ord,
+{
+    fn from_iter<T: IntoIterator<Item = Handle<O>>>(handles: T) -> Self {
+        let mut added = BTreeSet::new();
+        let mut inner = Vec::new();
+
+        for handle in handles {
+            if added.contains(&handle) {
+                panic!(
+                    "Constructing `HandleSet` with duplicate handle: {:?}",
+                    handle
+                );
+            }
+
+            added.insert(handle.clone());
+            inner.push(handle);
+        }
+
+        Self { inner }
+    }
+}
+
+/// An iterator over handles to objects
+///
+/// This struct is returned by the respective methods of all objects that
+/// reference multiple objects of the same type.
+pub struct HandleIter<'r, T> {
+    handles: &'r Vec<Handle<T>>,
+    next_index: usize,
+}
+
+impl<'r, T> Iterator for HandleIter<'r, T> {
+    // You might wonder why we're returning references to handles here, when
+    // `Handle` already is kind of reference, and easily cloned.
+    //
+    // Most of the time that doesn't make a difference, but there are use cases
+    // where dealing with owned `Handle`s is inconvenient, for example when
+    // using iterator adapters. You can't return a reference to the argument of
+    // an adapter's closure, if you own that argument. You can, if you just
+    // reference the argument.
+    type Item = &'r Handle<T>;
+
+    fn next(&mut self) -> Option<Self::Item> {
+        let handle = self.handles.get(self.next_index);
+        self.next_index += 1;
+        handle
+    }
+
+    fn size_hint(&self) -> (usize, Option<usize>) {
+        let size = self.handles.len();
+        (size, Some(size))
+    }
+}
+
+impl<T> ExactSizeIterator for HandleIter<'_, T> {}
+
+// Deriving won't work, as that only derives `Clone` where `T: Clone`. But
+// `HandleIter` can be `Clone`d unconditionally.
+impl<T> Clone for HandleIter<'_, T> {
+    fn clone(&self) -> Self {
+        Self {
+            handles: self.handles,
+            next_index: self.next_index,
+        }
+    }
+}

crates/fj-core/src/objects/kinds/cycle.rs

@@ -1,23 +1,27 @@
 use fj_math::{Scalar, Winding};
 use itertools::Itertools;
 
-use crate::{geometry::SurfacePath, objects::Edge, storage::Handle};
+use crate::{
+    geometry::SurfacePath,
+    objects::{handles::HandleSet, Edge, HandleIter},
+    storage::Handle,
+};
 
 /// A cycle of connected edges
 #[derive(Clone, Debug, Eq, PartialEq, Hash, Ord, PartialOrd)]
 pub struct Cycle {
-    edges: Vec<Handle<Edge>>,
+    edges: HandleSet<Edge>,
 }
 
 impl Cycle {
     /// Create an instance of `Cycle`
     pub fn new(edges: impl IntoIterator<Item = Handle<Edge>>) -> Self {
-        let edges = edges.into_iter().collect::<Vec<_>>();
+        let edges = edges.into_iter().collect();
         Self { edges }
     }
 
     /// Access the edges that make up the cycle
-    pub fn edges(&self) -> impl Iterator<Item = &Handle<Edge>> {
+    pub fn edges(&self) -> HandleIter<Edge> {
         self.edges.iter()
     }
 
@@ -30,7 +34,7 @@ impl Cycle {
 
     /// Access the edge with the provided index
     pub fn nth_edge(&self, index: usize) -> Option<&Handle<Edge>> {
-        self.edges.get(index)
+        self.edges.nth(index)
     }
 
     /// Access the edge after the provided one
@@ -39,7 +43,7 @@ impl Cycle {
     pub fn edge_after(&self, edge: &Handle<Edge>) -> Option<&Handle<Edge>> {
         self.index_of(edge).map(|index| {
             let next_index = (index + 1) % self.edges.len();
-            &self.edges[next_index]
+            self.edges.nth(next_index).unwrap()
         })
     }
 

crates/fj-core/src/objects/kinds/face.rs

@@ -1,5 +1,3 @@
-use std::collections::{btree_set, BTreeSet};
-
 use fj_math::Winding;
 
 use crate::{
@@ -71,51 +69,6 @@ impl Face {
     }
 }
 
-/// A collection of faces
-#[derive(Clone, Debug, Default, Eq, PartialEq, Hash, Ord, PartialOrd)]
-pub struct FaceSet {
-    inner: BTreeSet<Handle<Face>>,
-}
-
-impl FaceSet {
-    /// Create an empty instance of `Faces`
-    pub fn new() -> Self {
-        Self::default()
-    }
-}
-
-impl Extend<Handle<Face>> for FaceSet {
-    fn extend<T: IntoIterator<Item = Handle<Face>>>(&mut self, iter: T) {
-        self.inner.extend(iter);
-    }
-}
-
-impl FromIterator<Handle<Face>> for FaceSet {
-    fn from_iter<T: IntoIterator<Item = Handle<Face>>>(iter: T) -> Self {
-        let mut faces = Self::new();
-        faces.extend(iter);
-        faces
-    }
-}
-
-impl IntoIterator for FaceSet {
-    type Item = Handle<Face>;
-    type IntoIter = btree_set::IntoIter<Handle<Face>>;
-
-    fn into_iter(self) -> Self::IntoIter {
-        self.inner.into_iter()
-    }
-}
-
-impl<'a> IntoIterator for &'a FaceSet {
-    type Item = &'a Handle<Face>;
-    type IntoIter = btree_set::Iter<'a, Handle<Face>>;
-
-    fn into_iter(self) -> Self::IntoIter {
-        self.inner.iter()
-    }
-}
-
 /// The handedness of a face's coordinate system
 ///
 /// See [`Face::coord_handedness`].

crates/fj-core/src/objects/kinds/region.rs

@@ -1,7 +1,10 @@
 //! A single, continues 2d region
 use fj_interop::mesh::Color;
 
-use crate::{objects::Cycle, storage::Handle};
+use crate::{
+    objects::{handles::HandleSet, Cycle, HandleIter},
+    storage::Handle,
+};
 
 /// A single, continuous 2d region, may contain holes
 ///
@@ -14,7 +17,7 @@ use crate::{objects::Cycle, storage::Handle};
 #[derive(Clone, Debug, Eq, PartialEq, Hash, Ord, PartialOrd)]
 pub struct Region {
     exterior: Handle<Cycle>,
-    interiors: Vec<Handle<Cycle>>,
+    interiors: HandleSet<Cycle>,
     color: Option<Color>,
 }
 
@@ -40,12 +43,17 @@ impl Region {
     /// Access the cycles that bound the region on the inside
     ///
     /// Each of these cycles defines a hole in the region .
-    pub fn interiors(&self) -> impl Iterator<Item = &Handle<Cycle>> + '_ {
+    pub fn interiors(&self) -> HandleIter<Cycle> {
         self.interiors.iter()
     }
 
     /// Access all cycles of the region (both exterior and interior)
-    pub fn all_cycles(&self) -> impl Iterator<Item = &Handle<Cycle>> + '_ {
+    pub fn all_cycles(&self) -> impl Iterator<Item = &Handle<Cycle>> {
+        // It would be nice to return `HandleIter` here, but there's no
+        // straight-forward way to chain it to another iterator, given it's
+        // current implementation.
+        //
+        // Maybe this can be addressed, once the need arises.
         [self.exterior()].into_iter().chain(self.interiors())
     }
 

crates/fj-core/src/objects/kinds/shell.rs

@@ -1,12 +1,12 @@
 use crate::{
-    objects::{Face, FaceSet},
+    objects::{handles::HandleSet, Face, HandleIter},
     storage::Handle,
 };
 
 /// A 3-dimensional closed shell
 #[derive(Clone, Debug, Eq, PartialEq, Hash, Ord, PartialOrd)]
 pub struct Shell {
-    faces: FaceSet,
+    faces: HandleSet<Face>,
 }
 
 impl Shell {
@@ -18,7 +18,7 @@ impl Shell {
     }
 
     /// Access the faces of the shell
-    pub fn faces(&self) -> &FaceSet {
-        &self.faces
+    pub fn faces(&self) -> HandleIter<Face> {
+        self.faces.iter()
     }
 }