Merge pull request #414 from hannobraun/store

Simplify `Store`

Cargo.lock

@@ -679,6 +679,7 @@ dependencies = [
  "parking_lot 0.12.0",
  "parry2d-f64",
  "parry3d-f64",
+ "slotmap",
  "spade",
  "thiserror",
 ]

fj-kernel/Cargo.toml

@@ -19,6 +19,7 @@ nalgebra    = "0.30.0"
 parking_lot = "0.12.0"
 parry2d-f64 = "0.8.0"
 parry3d-f64 = "0.8.0"
+slotmap     = "1.0.6"
 spade       = "2.0.0"
 thiserror   = "1.0.30"
 

fj-kernel/src/shape/geometry.rs

@@ -6,9 +6,8 @@ use crate::{
 };
 
 use super::{
-    handle::Handle,
     stores::{Curves, Faces, Points, Surfaces},
-    Iter,
+    Handle, Iter,
 };
 
 /// API to access a shape's geometry
@@ -62,46 +61,43 @@ impl Geometry<'_> {
     /// Since the topological types refer to geometry, and don't contain any
     /// geometry themselves, this transforms the whole shape.
     pub fn transform(&mut self, transform: &Transform) {
-        for mut point in self.points.iter_mut() {
-            *point = transform.transform_point(&point);
-        }
-        for mut curve in self.curves.iter_mut() {
-            *curve = curve.transform(transform);
-        }
-        for mut surface in self.surfaces.iter_mut() {
-            *surface = surface.transform(transform);
-        }
+        self.points
+            .update(|point| *point = transform.transform_point(point));
+        self.curves
+            .update(|curve| *curve = curve.transform(transform));
+        self.surfaces
+            .update(|surface| *surface = surface.transform(transform));
 
         // While some faces use triangle representation, we need this weird
         // workaround here.
-        for mut face in self.faces.iter_mut() {
+        self.faces.update(|mut face| {
             use std::ops::DerefMut as _;
             if let Face::Triangles(triangles) = face.deref_mut() {
                 for triangle in triangles {
                     *triangle = transform.transform_triangle(triangle);
                 }
             }
-        }
+        });
     }
 
     /// Access an iterator over all points
     ///
     /// The caller must not make any assumptions about the order of points.
     pub fn points(&self) -> Iter<Point<3>> {
-        Iter::new(self.points)
+        self.points.iter()
     }
 
     /// Access an iterator over all curves
     ///
     /// The caller must not make any assumptions about the order of curves.
     pub fn curves(&self) -> Iter<Curve> {
-        Iter::new(self.curves)
+        self.curves.iter()
     }
 
     /// Access an iterator over all surfaces
     ///
     /// The caller must not make any assumptions about the order of surfaces.
     pub fn surfaces(&self) -> Iter<Surface> {
-        Iter::new(self.surfaces)
+        self.surfaces.iter()
     }
 }

fj-kernel/src/shape/handle.rs

@@ -1,134 +0,0 @@
-use std::{
-    hash::{Hash, Hasher},
-    ops::{Deref, DerefMut},
-    sync::Arc,
-};
-
-use parking_lot::{RwLock, RwLockWriteGuard};
-
-/// A handle to an object stored within [`Shape`]
-///
-/// If an object of type `T` (this could be `Curve`, `Vertex`, etc.) is added to
-/// `Shape`, a `Handle<T>` is returned. This handle is then used in topological
-/// types to refer to the object, instead of having those types own an instance
-/// of the object.
-///
-/// This approach has two advantages:
-///
-/// 1. The object can't be mutated through the handle. Since an object can be
-///    referred to by multiple other objects, mutating it locally would have no
-///    effect on those other references. `Handle` preventing that removes this
-///    source of errors.
-/// 2. The object is guaranteed to be in `Shape`, as `Handle`s can't be created
-///    any other way. This means that if the `Shape` needs to be modified, any
-///    objects can be updated once, without requiring an update of all the other
-///    objects that reference it.
-///
-/// # Equality
-///
-/// The equality of [`Handle`] is very strictly defined in terms of identity.
-/// Two [`Handle`]s are considered equal, if they refer to objects in the same
-/// memory location.
-#[derive(Clone, Debug, Eq, PartialEq, Hash, Ord, PartialOrd)]
-pub struct Handle<T> {
-    storage: Storage<T>,
-}
-
-impl<T> Handle<T> {
-    /// Access the object that the handle references
-    pub fn get(&self) -> T
-    where
-        T: Clone,
-    {
-        self.storage.get()
-    }
-
-    /// Internal method to access the [`Storage`] this handle refers to
-    pub(super) fn storage(&self) -> &Storage<T> {
-        &self.storage
-    }
-}
-
-/// Internal type used in collections within [`Shape`]
-#[derive(Debug)]
-pub struct Storage<T>(Arc<RwLock<T>>);
-
-impl<T> Storage<T> {
-    /// Create a [`Storage`] instance that wraps the provided object
-    pub(super) fn new(value: T) -> Self {
-        Self(Arc::new(RwLock::new(value)))
-    }
-
-    /// Create a handle that refers to this [`Storage`] instance
-    pub(super) fn handle(&self) -> Handle<T> {
-        Handle {
-            storage: self.clone(),
-        }
-    }
-
-    pub(super) fn get(&self) -> T
-    where
-        T: Clone,
-    {
-        self.0.read().clone()
-    }
-
-    pub(super) fn get_mut(&self) -> RefMut<T> {
-        RefMut(self.0.write())
-    }
-
-    fn ptr(&self) -> *const () {
-        Arc::as_ptr(&self.0) as _
-    }
-}
-
-// Deriving `Clone` would only derive `Clone` where `T: Clone`. This
-// implementation doesn't have that limitation, providing `Clone` for all
-// `Handle`s instead.
-impl<T> Clone for Storage<T> {
-    fn clone(&self) -> Self {
-        Self(self.0.clone())
-    }
-}
-
-impl<T> PartialEq for Storage<T> {
-    fn eq(&self, other: &Self) -> bool {
-        Arc::ptr_eq(&self.0, &other.0)
-    }
-}
-
-impl<T> Eq for Storage<T> {}
-
-impl<T> Ord for Storage<T> {
-    fn cmp(&self, other: &Self) -> std::cmp::Ordering {
-        self.ptr().cmp(&other.ptr())
-    }
-}
-
-impl<T> PartialOrd for Storage<T> {
-    fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
-        Some(self.cmp(other))
-    }
-}
-
-impl<T> Hash for Storage<T> {
-    fn hash<H: Hasher>(&self, state: &mut H) {
-        self.ptr().hash(state);
-    }
-}
-
-pub struct RefMut<'r, T>(RwLockWriteGuard<'r, T>);
-
-impl<T> Deref for RefMut<'_, T> {
-    type Target = T;
-
-    fn deref(&self) -> &Self::Target {
-        self.0.deref()
-    }
-}
-
-impl<T> DerefMut for RefMut<'_, T> {
-    fn deref_mut(&mut self) -> &mut Self::Target {
-        self.0.deref_mut()
-    }
-}

fj-kernel/src/shape/iter.rs

@@ -1,37 +0,0 @@
-use super::{
-    handle::Handle,
-    stores::{self, Store},
-};
-
-/// An iterator over geometric or topological objects
-///
-/// Returned by various methods of the [`Shape`] API.
-pub struct Iter<'r, T> {
-    inner: stores::Iter<'r, T>,
-}
-
-impl<'r, T> Iter<'r, T> {
-    pub(super) fn new(store: &'r Store<T>) -> Self {
-        Self {
-            inner: store.iter(),
-        }
-    }
-
-    /// Convert this iterator over handles into an iterator over values
-    ///
-    /// This is a convenience method, for cases where no `Handle` is needed.
-    pub fn values(self) -> impl Iterator<Item = T> + 'r
-    where
-        T: Clone,
-    {
-        self.inner.map(|handle| handle.get().clone())
-    }
-}
-
-impl<T> Iterator for Iter<'_, T> {
-    type Item = Handle<T>;
-
-    fn next(&mut self) -> Option<Self::Item> {
-        self.inner.next()
-    }
-}

fj-kernel/src/shape/mod.rs

@@ -4,8 +4,6 @@
 
 mod api;
 mod geometry;
-mod handle;
-mod iter;
 mod stores;
 mod topology;
 mod validate;
@@ -13,8 +11,7 @@ mod validate;
 pub use self::{
     api::Shape,
     geometry::Geometry,
-    handle::Handle,
-    iter::Iter,
+    stores::{Handle, Iter},
     topology::Topology,
     validate::{StructuralIssues, ValidationError, ValidationResult},
 };

fj-kernel/src/shape/stores.rs

@@ -1,17 +1,17 @@
-use std::{iter, slice};
+use std::{
+    hash::{Hash, Hasher},
+    sync::Arc,
+};
 
 use fj_math::Point;
+use parking_lot::{RwLock, RwLockReadGuard};
+use slotmap::{DefaultKey, SlotMap};
 
 use crate::{
     geometry::{Curve, Surface},
     topology::{Cycle, Edge, Face, Vertex},
 };
 
-use super::{
-    handle::{RefMut, Storage},
-    Handle,
-};
-
 pub type Points = Store<Point<3>>;
 pub type Curves = Store<Curve>;
 pub type Surfaces = Store<Surface>;
@@ -21,39 +21,180 @@ pub type Edges = Store<Edge>;
 pub type Cycles = Store<Cycle>;
 pub type Faces = Store<Face>;
 
-#[derive(Clone, Debug)]
+#[derive(Debug)]
 pub struct Store<T> {
-    inner: Vec<Storage<T>>,
+    objects: Arc<RwLock<Objects<T>>>,
 }
 
 impl<T> Store<T> {
     pub fn new() -> Self {
-        Self { inner: Vec::new() }
+        Self {
+            objects: Arc::new(RwLock::new(SlotMap::new())),
+        }
     }
 
     pub fn contains(&self, object: &Handle<T>) -> bool {
-        self.inner.contains(object.storage())
+        object.store() == self && self.objects.read().contains_key(object.key())
     }
 
     pub fn add(&mut self, object: T) -> Handle<T> {
-        let storage = Storage::new(object);
-        let handle = storage.handle();
+        let key = self.objects.write().insert(object);
+        Handle::new(key, self.clone())
+    }
 
-        self.inner.push(storage);
-
-        handle
+    pub fn read(&self) -> RwLockReadGuard<Objects<T>> {
+        self.objects.read()
     }
 
     pub fn iter(&self) -> Iter<T> {
-        self.inner.iter().map(|storage| storage.handle())
+        // The allocation here is unfortunate, but I think it's fine for now. If
+        // this turns into a performance issue, it should be possible to avoid
+        // it by adding methods to `Store`, that are geared towards implementing
+        // iterators.
+        Iter {
+            elements: self
+                .objects
+                .read()
+                .iter()
+                .map(|(key, _)| Handle::new(key, self.clone()))
+                .collect(),
+        }
     }
 
-    pub fn iter_mut(&mut self) -> IterMut<T> {
-        self.inner.iter_mut().map(|storage| storage.get_mut())
+    pub fn update<F>(&mut self, mut f: F)
+    where
+        F: FnMut(&mut T),
+    {
+        for (_, object) in self.objects.write().iter_mut() {
+            f(object);
+        }
+    }
+
+    fn ptr(&self) -> *const () {
+        Arc::as_ptr(&self.objects) as _
     }
 }
 
-pub type Iter<'r, T> =
-    iter::Map<slice::Iter<'r, Storage<T>>, fn(&Storage<T>) -> Handle<T>>;
-pub type IterMut<'r, T> =
-    iter::Map<slice::IterMut<'r, Storage<T>>, fn(&mut Storage<T>) -> RefMut<T>>;
+// Deriving `Clone` would only derive `Clone` where `T: Clone`. This
+// implementation doesn't have that limitation, providing `Clone` for all
+// `Store`s instead.
+impl<T> Clone for Store<T> {
+    fn clone(&self) -> Self {
+        Self {
+            objects: self.objects.clone(),
+        }
+    }
+}
+
+impl<T> PartialEq for Store<T> {
+    fn eq(&self, other: &Self) -> bool {
+        self.ptr().eq(&other.ptr())
+    }
+}
+
+impl<T> Eq for Store<T> {}
+
+impl<T> Ord for Store<T> {
+    fn cmp(&self, other: &Self) -> std::cmp::Ordering {
+        self.ptr().cmp(&other.ptr())
+    }
+}
+
+impl<T> PartialOrd for Store<T> {
+    fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
+        Some(self.cmp(other))
+    }
+}
+
+impl<T> Hash for Store<T> {
+    fn hash<H: Hasher>(&self, state: &mut H) {
+        self.ptr().hash(state);
+    }
+}
+
+pub type Objects<T> = SlotMap<DefaultKey, T>;
+
+/// A handle to an object stored within [`Shape`]
+///
+/// If an object of type `T` (this could be `Curve`, `Vertex`, etc.) is added to
+/// `Shape`, a `Handle<T>` is returned. This handle is then used in topological
+/// types to refer to the object, instead of having those types own an instance
+/// of the object.
+///
+/// This approach has two advantages:
+///
+/// 1. The object can't be mutated through the handle. Since an object can be
+///    referred to by multiple other objects, mutating it locally would have no
+///    effect on those other references. `Handle` preventing that removes this
+///    source of errors.
+/// 2. The object is guaranteed to be in `Shape`, as `Handle`s can't be created
+///    any other way. This means that if the `Shape` needs to be modified, any
+///    objects can be updated once, without requiring an update of all the other
+///    objects that reference it.
+///
+/// # Equality
+///
+/// The equality of [`Handle`] is very strictly defined in terms of identity.
+/// Two [`Handle`]s are considered equal, if they refer to objects in the same
+/// memory location.
+#[derive(Clone, Debug, Eq, PartialEq, Hash, Ord, PartialOrd)]
+pub struct Handle<T> {
+    key: DefaultKey,
+    store: Store<T>,
+}
+
+impl<T> Handle<T> {
+    pub(super) fn new(key: DefaultKey, store: Store<T>) -> Self {
+        Self { key, store }
+    }
+
+    pub(super) fn key(&self) -> DefaultKey {
+        self.key
+    }
+
+    pub(super) fn store(&self) -> &Store<T> {
+        &self.store
+    }
+
+    /// Access the object that the handle references
+    pub fn get(&self) -> T
+    where
+        T: Clone,
+    {
+        self.store
+            .read()
+            .get(self.key)
+            // Can't panic, unless the handle was invalid in the first place.
+            // Objects are never removed from `Store`, so if we have a handle
+            // pointing to it, it should be there.
+            .unwrap()
+            .clone()
+    }
+}
+
+/// An iterator over geometric or topological objects
+///
+/// Returned by various methods of the [`Shape`] API.
+pub struct Iter<T> {
+    elements: Vec<Handle<T>>,
+}
+
+impl<T> Iter<T> {
+    /// Convert this iterator over handles into an iterator over values
+    ///
+    /// This is a convenience method, for cases where no `Handle` is needed.
+    pub fn values(self) -> impl Iterator<Item = T>
+    where
+        T: Clone,
+    {
+        self.elements.into_iter().map(|handle| handle.get())
+    }
+}
+
+impl<T> Iterator for Iter<T> {
+    type Item = Handle<T>;
+
+    fn next(&mut self) -> Option<Self::Item> {
+        self.elements.pop()
+    }
+}

fj-kernel/src/shape/topology.rs

@@ -176,28 +176,28 @@ impl Topology<'_> {
     ///
     /// The caller must not make any assumptions about the order of vertices.
     pub fn vertices(&self) -> Iter<Vertex> {
-        Iter::new(self.vertices)
+        self.vertices.iter()
     }
 
     /// Access iterator over all edges
     ///
     /// The caller must not make any assumptions about the order of edges.
     pub fn edges(&self) -> Iter<Edge> {
-        Iter::new(self.edges)
+        self.edges.iter()
     }
 
     /// Access an iterator over all cycles
     ///
     /// The caller must not make any assumptions about the order of cycles.
     pub fn cycles(&self) -> Iter<Cycle> {
-        Iter::new(self.cycles)
+        self.cycles.iter()
     }
 
     /// Access an iterator over all faces
     ///
     /// The caller must not make any assumptions about the order of faces.
     pub fn faces(&self) -> Iter<Face> {
-        Iter::new(self.geometry.faces)
+        self.geometry.faces.iter()
     }
 }
 
@@ -209,7 +209,7 @@ mod tests {
 
     use crate::{
         geometry::{Curve, Surface},
-        shape::{handle::Handle, Shape, ValidationError},
+        shape::{Handle, Shape, ValidationError},
         topology::{Cycle, Edge, Face, Vertex},
     };