diff --git a/Cargo.lock b/Cargo.lock index 064601ad1..d72d4d487 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -679,6 +679,7 @@ dependencies = [ "parking_lot 0.12.0", "parry2d-f64", "parry3d-f64", + "slotmap", "spade", "thiserror", ] diff --git a/fj-kernel/Cargo.toml b/fj-kernel/Cargo.toml index c2fa657c2..c3a7ae6d8 100644 --- a/fj-kernel/Cargo.toml +++ b/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" diff --git a/fj-kernel/src/shape/geometry.rs b/fj-kernel/src/shape/geometry.rs index d3cbba05d..34b725a49 100644 --- a/fj-kernel/src/shape/geometry.rs +++ b/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> { - 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 { - 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 { - Iter::new(self.surfaces) + self.surfaces.iter() } } diff --git a/fj-kernel/src/shape/handle.rs b/fj-kernel/src/shape/handle.rs deleted file mode 100644 index 02aec5660..000000000 --- a/fj-kernel/src/shape/handle.rs +++ /dev/null @@ -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` 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 { - storage: Storage, -} - -impl Handle { - /// 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 { - &self.storage - } -} - -/// Internal type used in collections within [`Shape`] -#[derive(Debug)] -pub struct Storage(Arc>); - -impl Storage { - /// 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 { - Handle { - storage: self.clone(), - } - } - - pub(super) fn get(&self) -> T - where - T: Clone, - { - self.0.read().clone() - } - - pub(super) fn get_mut(&self) -> RefMut { - 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 Clone for Storage { - fn clone(&self) -> Self { - Self(self.0.clone()) - } -} - -impl PartialEq for Storage { - fn eq(&self, other: &Self) -> bool { - Arc::ptr_eq(&self.0, &other.0) - } -} - -impl Eq for Storage {} - -impl Ord for Storage { - fn cmp(&self, other: &Self) -> std::cmp::Ordering { - self.ptr().cmp(&other.ptr()) - } -} - -impl PartialOrd for Storage { - fn partial_cmp(&self, other: &Self) -> Option { - Some(self.cmp(other)) - } -} - -impl Hash for Storage { - fn hash(&self, state: &mut H) { - self.ptr().hash(state); - } -} - -pub struct RefMut<'r, T>(RwLockWriteGuard<'r, T>); - -impl Deref for RefMut<'_, T> { - type Target = T; - - fn deref(&self) -> &Self::Target { - self.0.deref() - } -} - -impl DerefMut for RefMut<'_, T> { - fn deref_mut(&mut self) -> &mut Self::Target { - self.0.deref_mut() - } -} diff --git a/fj-kernel/src/shape/iter.rs b/fj-kernel/src/shape/iter.rs deleted file mode 100644 index 5579cee5e..000000000 --- a/fj-kernel/src/shape/iter.rs +++ /dev/null @@ -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) -> 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 + 'r - where - T: Clone, - { - self.inner.map(|handle| handle.get().clone()) - } -} - -impl Iterator for Iter<'_, T> { - type Item = Handle; - - fn next(&mut self) -> Option { - self.inner.next() - } -} diff --git a/fj-kernel/src/shape/mod.rs b/fj-kernel/src/shape/mod.rs index b47d90e91..a99f7772b 100644 --- a/fj-kernel/src/shape/mod.rs +++ b/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}, }; diff --git a/fj-kernel/src/shape/stores.rs b/fj-kernel/src/shape/stores.rs index 6b065a29f..27e590b85 100644 --- a/fj-kernel/src/shape/stores.rs +++ b/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>; pub type Curves = Store; pub type Surfaces = Store; @@ -21,39 +21,180 @@ pub type Edges = Store; pub type Cycles = Store; pub type Faces = Store; -#[derive(Clone, Debug)] +#[derive(Debug)] pub struct Store { - inner: Vec>, + objects: Arc>>, } impl Store { pub fn new() -> Self { - Self { inner: Vec::new() } + Self { + objects: Arc::new(RwLock::new(SlotMap::new())), + } } pub fn contains(&self, object: &Handle) -> bool { - self.inner.contains(object.storage()) + object.store() == self && self.objects.read().contains_key(object.key()) } pub fn add(&mut self, object: T) -> Handle { - let storage = Storage::new(object); - let handle = storage.handle(); - - self.inner.push(storage); + let key = self.objects.write().insert(object); + Handle::new(key, self.clone()) + } - handle + pub fn read(&self) -> RwLockReadGuard> { + self.objects.read() } pub fn iter(&self) -> Iter { - 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 update(&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 _ + } +} + +// Deriving `Clone` would only derive `Clone` where `T: Clone`. This +// implementation doesn't have that limitation, providing `Clone` for all +// `Store`s instead. +impl Clone for Store { + fn clone(&self) -> Self { + Self { + objects: self.objects.clone(), + } + } +} + +impl PartialEq for Store { + fn eq(&self, other: &Self) -> bool { + self.ptr().eq(&other.ptr()) } +} + +impl Eq for Store {} - pub fn iter_mut(&mut self) -> IterMut { - self.inner.iter_mut().map(|storage| storage.get_mut()) +impl Ord for Store { + fn cmp(&self, other: &Self) -> std::cmp::Ordering { + self.ptr().cmp(&other.ptr()) } } -pub type Iter<'r, T> = - iter::Map>, fn(&Storage) -> Handle>; -pub type IterMut<'r, T> = - iter::Map>, fn(&mut Storage) -> RefMut>; +impl PartialOrd for Store { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } +} + +impl Hash for Store { + fn hash(&self, state: &mut H) { + self.ptr().hash(state); + } +} + +pub type Objects = SlotMap; + +/// 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` 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 { + key: DefaultKey, + store: Store, +} + +impl Handle { + pub(super) fn new(key: DefaultKey, store: Store) -> Self { + Self { key, store } + } + + pub(super) fn key(&self) -> DefaultKey { + self.key + } + + pub(super) fn store(&self) -> &Store { + &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 { + elements: Vec>, +} + +impl 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 + where + T: Clone, + { + self.elements.into_iter().map(|handle| handle.get()) + } +} + +impl Iterator for Iter { + type Item = Handle; + + fn next(&mut self) -> Option { + self.elements.pop() + } +} diff --git a/fj-kernel/src/shape/topology.rs b/fj-kernel/src/shape/topology.rs index 39e2d8c63..09d53b2e1 100644 --- a/fj-kernel/src/shape/topology.rs +++ b/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 { - 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 { - 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 { - 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 { - 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}, };