From 3a9e99ba8a6ca48ac61eb09b7ba0d74115f2dbef Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Fri, 13 May 2022 17:26:18 +0200 Subject: [PATCH 1/5] Make `Edge` generic over dimensionality --- crates/fj-kernel/src/algorithms/sweep.rs | 6 +++--- crates/fj-kernel/src/shape/api.rs | 4 ++-- crates/fj-kernel/src/shape/object.rs | 4 ++-- crates/fj-kernel/src/shape/stores.rs | 2 +- crates/fj-kernel/src/shape/validate.rs | 6 +++--- crates/fj-kernel/src/topology/builder.rs | 6 +++--- crates/fj-kernel/src/topology/cycles.rs | 4 ++-- crates/fj-kernel/src/topology/edges.rs | 10 ++++++---- 8 files changed, 22 insertions(+), 20 deletions(-) diff --git a/crates/fj-kernel/src/algorithms/sweep.rs b/crates/fj-kernel/src/algorithms/sweep.rs index 0e21b9a73..290b81e09 100644 --- a/crates/fj-kernel/src/algorithms/sweep.rs +++ b/crates/fj-kernel/src/algorithms/sweep.rs @@ -233,7 +233,7 @@ pub fn sweep_shape( struct Relation { vertices: HashMap>, Handle>>, - edges: HashMap, Handle>, + edges: HashMap>, Handle>>, cycles: HashMap, Handle>, } @@ -248,7 +248,7 @@ impl Relation { fn vertices_for_edge( &self, - edge: &Handle, + edge: &Handle>, ) -> Option<[Handle>; 2]> { edge.get().vertices.map(|vertices| { vertices.map(|vertex| { @@ -257,7 +257,7 @@ impl Relation { }) } - fn edges_for_cycle(&self, cycle: &Handle) -> Vec> { + fn edges_for_cycle(&self, cycle: &Handle) -> Vec>> { cycle .get() .edges diff --git a/crates/fj-kernel/src/shape/api.rs b/crates/fj-kernel/src/shape/api.rs index 92a29f312..5c3b84083 100644 --- a/crates/fj-kernel/src/shape/api.rs +++ b/crates/fj-kernel/src/shape/api.rs @@ -168,7 +168,7 @@ impl Shape { /// Access iterator over all edges /// /// The caller must not make any assumptions about the order of edges. - pub fn edges(&self) -> Iter { + pub fn edges(&self) -> Iter> { self.stores.edges.iter() } @@ -393,7 +393,7 @@ mod tests { self.insert(Surface::xy_plane()).unwrap() } - fn add_edge(&mut self) -> anyhow::Result> { + fn add_edge(&mut self) -> anyhow::Result>> { let points = [(); 2].map(|()| { let point = self.next_point; self.next_point.x += Scalar::ONE; diff --git a/crates/fj-kernel/src/shape/object.rs b/crates/fj-kernel/src/shape/object.rs index e6f6d47a6..4ac2c4462 100644 --- a/crates/fj-kernel/src/shape/object.rs +++ b/crates/fj-kernel/src/shape/object.rs @@ -18,7 +18,7 @@ impl private::Sealed for Curve<3> {} impl private::Sealed for Surface {} impl private::Sealed for Vertex<3> {} -impl private::Sealed for Edge {} +impl private::Sealed for Edge<3> {} impl private::Sealed for Cycle {} impl private::Sealed for Face {} @@ -27,7 +27,7 @@ impl Object for Curve<3> {} impl Object for Surface {} impl Object for Vertex<3> {} -impl Object for Edge {} +impl Object for Edge<3> {} impl Object for Cycle {} impl Object for Face {} diff --git a/crates/fj-kernel/src/shape/stores.rs b/crates/fj-kernel/src/shape/stores.rs index 60c31c2dd..ef7d450e5 100644 --- a/crates/fj-kernel/src/shape/stores.rs +++ b/crates/fj-kernel/src/shape/stores.rs @@ -56,7 +56,7 @@ pub type Curves = Store>; pub type Surfaces = Store; pub type Vertices = Store>; -pub type Edges = Store; +pub type Edges = Store>; pub type Cycles = Store; pub type Faces = Store; diff --git a/crates/fj-kernel/src/shape/validate.rs b/crates/fj-kernel/src/shape/validate.rs index 1a7635a32..72e25255a 100644 --- a/crates/fj-kernel/src/shape/validate.rs +++ b/crates/fj-kernel/src/shape/validate.rs @@ -62,7 +62,7 @@ impl Validate for Vertex<3> { } } -impl Validate for Edge { +impl Validate for Edge<3> { fn validate( &self, _: Scalar, @@ -225,7 +225,7 @@ impl ValidationError { /// Indicate whether validation found a missing edge #[cfg(test)] - pub fn missing_edge(&self, edge: &Handle) -> bool { + pub fn missing_edge(&self, edge: &Handle>) -> bool { if let Self::Structural(StructuralIssues { missing_edges, .. }) = self { return missing_edges.contains(edge); } @@ -276,7 +276,7 @@ pub struct StructuralIssues { pub missing_vertices: HashSet>>, /// Missing edges found in cycle validation - pub missing_edges: HashSet>, + pub missing_edges: HashSet>>, /// Missing surface found in face validation pub missing_surface: Option>, diff --git a/crates/fj-kernel/src/topology/builder.rs b/crates/fj-kernel/src/topology/builder.rs index 6c7d2607d..6f0038b02 100644 --- a/crates/fj-kernel/src/topology/builder.rs +++ b/crates/fj-kernel/src/topology/builder.rs @@ -47,7 +47,7 @@ impl<'r> EdgeBuilder<'r> { } /// Build a circle from a radius - pub fn build_circle(self, radius: Scalar) -> ValidationResult { + pub fn build_circle(self, radius: Scalar) -> ValidationResult> { let curve = self.shape.insert(Curve::Circle(Circle { center: Point::origin(), a: Vector::from([radius, Scalar::ZERO, Scalar::ZERO]), @@ -65,7 +65,7 @@ impl<'r> EdgeBuilder<'r> { pub fn build_line_segment_from_points( self, vertices: [impl Into>; 2], - ) -> ValidationResult { + ) -> ValidationResult> { // Can be cleaned up with `try_map`, once that is stable: // https://doc.rust-lang.org/std/primitive.array.html#method.try_map let vertices = vertices @@ -84,7 +84,7 @@ impl<'r> EdgeBuilder<'r> { pub fn build_line_segment_from_vertices( self, vertices: [Handle>; 2], - ) -> ValidationResult { + ) -> ValidationResult> { let curve = self.shape.insert(Curve::Line(Line::from_points( vertices.clone().map(|vertex| vertex.get().point()), )))?; diff --git a/crates/fj-kernel/src/topology/cycles.rs b/crates/fj-kernel/src/topology/cycles.rs index 1373815ed..db9ae2075 100644 --- a/crates/fj-kernel/src/topology/cycles.rs +++ b/crates/fj-kernel/src/topology/cycles.rs @@ -22,7 +22,7 @@ use super::{CycleBuilder, Edge}; #[derive(Clone, Debug, Eq, Ord, PartialOrd)] pub struct Cycle { /// The edges that make up the cycle - pub edges: Vec>, + pub edges: Vec>>, } impl Cycle { @@ -35,7 +35,7 @@ impl Cycle { /// /// This is a convenience method that saves the caller from dealing with the /// [`Handle`]s. - pub fn edges(&self) -> impl Iterator + '_ { + pub fn edges(&self) -> impl Iterator> + '_ { self.edges.iter().map(|handle| handle.get()) } } diff --git a/crates/fj-kernel/src/topology/edges.rs b/crates/fj-kernel/src/topology/edges.rs index 946912251..52812f5c6 100644 --- a/crates/fj-kernel/src/topology/edges.rs +++ b/crates/fj-kernel/src/topology/edges.rs @@ -20,7 +20,7 @@ use super::{vertices::Vertex, EdgeBuilder}; /// the curve and any vertices that it refers to, must be part of the same /// shape. #[derive(Clone, Debug, Eq, Ord, PartialOrd)] -pub struct Edge { +pub struct Edge { /// Access the curve that defines the edge's geometry /// /// The edge can be a segment of the curve that is bounded by two vertices, @@ -35,7 +35,7 @@ pub struct Edge { pub vertices: Option<[LocalForm, Vertex<3>>; 2]>, } -impl Edge { +impl Edge<3> { /// Construct an instance of `Edge` /// /// # Implementation Note @@ -78,7 +78,9 @@ impl Edge { pub fn builder(shape: &mut Shape) -> EdgeBuilder { EdgeBuilder::new(shape) } +} +impl Edge { /// Access the curve that the edge refers to /// /// This is a convenience method that saves the caller from dealing with the @@ -98,13 +100,13 @@ impl Edge { } } -impl PartialEq for Edge { +impl PartialEq for Edge { fn eq(&self, other: &Self) -> bool { self.curve() == other.curve() && self.vertices == other.vertices } } -impl Hash for Edge { +impl Hash for Edge { fn hash(&self, state: &mut H) { self.curve().hash(state); self.vertices.hash(state); From 9668f288b0b271774e1d98e19e5defccd49a2453 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Mon, 16 May 2022 18:04:34 +0200 Subject: [PATCH 2/5] Use `Edge::new` in more places --- crates/fj-kernel/src/shape/api.rs | 5 +---- crates/fj-kernel/src/topology/builder.rs | 5 +---- 2 files changed, 2 insertions(+), 8 deletions(-) diff --git a/crates/fj-kernel/src/shape/api.rs b/crates/fj-kernel/src/shape/api.rs index 5c3b84083..1e94c99f8 100644 --- a/crates/fj-kernel/src/shape/api.rs +++ b/crates/fj-kernel/src/shape/api.rs @@ -229,10 +229,7 @@ mod tests { assert!(shape.get_handle(&surface.get()).as_ref() == Some(&surface)); let vertex = Vertex::new(point); - let edge = Edge { - curve, - vertices: None, - }; + let edge = Edge::new(curve, None); assert!(shape.get_handle(&vertex).is_none()); assert!(shape.get_handle(&edge).is_none()); diff --git a/crates/fj-kernel/src/topology/builder.rs b/crates/fj-kernel/src/topology/builder.rs index 6f0038b02..e46d4f6d1 100644 --- a/crates/fj-kernel/src/topology/builder.rs +++ b/crates/fj-kernel/src/topology/builder.rs @@ -53,10 +53,7 @@ impl<'r> EdgeBuilder<'r> { a: Vector::from([radius, Scalar::ZERO, Scalar::ZERO]), b: Vector::from([Scalar::ZERO, radius, Scalar::ZERO]), }))?; - let edge = self.shape.insert(Edge { - curve, - vertices: None, - })?; + let edge = self.shape.insert(Edge::new(curve, None))?; Ok(edge) } From 9b80161a140feef6ef99ba8a0ebb788b50baf217 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Mon, 16 May 2022 18:05:01 +0200 Subject: [PATCH 3/5] Add `LocalForm::canonical_only` --- crates/fj-kernel/src/shape/local.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/crates/fj-kernel/src/shape/local.rs b/crates/fj-kernel/src/shape/local.rs index 65b282a05..f2139192f 100644 --- a/crates/fj-kernel/src/shape/local.rs +++ b/crates/fj-kernel/src/shape/local.rs @@ -40,6 +40,17 @@ impl LocalForm { } } +impl LocalForm { + /// Construct a new instance of `LocalForm` without a local form + /// + /// It's possible that an object's local and canonical forms are the same. + /// This is a convenience constructor that constructs a `LocalForm` instance + /// for such a situation. + pub fn canonical_only(canonical: Handle) -> Self { + Self::new(canonical.get(), canonical) + } +} + impl PartialEq for LocalForm where Local: PartialEq, From a66f28ab4d1c560cad6c6406405fbac8a21786ed Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Mon, 16 May 2022 18:05:22 +0200 Subject: [PATCH 4/5] Add local representation of curve to `Edge` --- crates/fj-kernel/src/shape/validate.rs | 4 ++-- crates/fj-kernel/src/topology/edges.rs | 11 +++++++---- crates/fj-operations/src/group.rs | 2 +- 3 files changed, 10 insertions(+), 7 deletions(-) diff --git a/crates/fj-kernel/src/shape/validate.rs b/crates/fj-kernel/src/shape/validate.rs index 72e25255a..918ad9820 100644 --- a/crates/fj-kernel/src/shape/validate.rs +++ b/crates/fj-kernel/src/shape/validate.rs @@ -71,8 +71,8 @@ impl Validate for Edge<3> { let mut missing_curve = None; let mut missing_vertices = HashSet::new(); - if !stores.curves.contains(&self.curve) { - missing_curve = Some(self.curve.clone()); + if !stores.curves.contains(self.curve.canonical()) { + missing_curve = Some(self.curve.canonical().clone()); } for vertices in &self.vertices { for vertex in vertices { diff --git a/crates/fj-kernel/src/topology/edges.rs b/crates/fj-kernel/src/topology/edges.rs index 52812f5c6..2859828c2 100644 --- a/crates/fj-kernel/src/topology/edges.rs +++ b/crates/fj-kernel/src/topology/edges.rs @@ -26,7 +26,7 @@ pub struct Edge { /// The edge can be a segment of the curve that is bounded by two vertices, /// or if the curve is continuous (i.e. connects to itself), the edge could /// be defined by the whole curve, and have no bounding vertices. - pub curve: Handle>, + pub curve: LocalForm, Curve<3>>, /// Access the vertices that bound the edge on the curve /// @@ -60,9 +60,12 @@ impl Edge<3> { curve: Handle>, vertices: Option<[Handle>; 2]>, ) -> Self { + let curve = LocalForm::canonical_only(curve); + let vertices = vertices.map(|vertices| { vertices.map(|canonical| { let local = curve + .canonical() .get() .point_to_curve_coords(canonical.get().point()) .local(); @@ -86,7 +89,7 @@ impl Edge { /// This is a convenience method that saves the caller from dealing with the /// [`Handle`]. pub fn curve(&self) -> Curve<3> { - self.curve.get() + self.curve.canonical().get() } /// Access the vertices that the edge refers to @@ -102,13 +105,13 @@ impl Edge { impl PartialEq for Edge { fn eq(&self, other: &Self) -> bool { - self.curve() == other.curve() && self.vertices == other.vertices + self.curve == other.curve && self.vertices == other.vertices } } impl Hash for Edge { fn hash(&self, state: &mut H) { - self.curve().hash(state); + self.curve.hash(state); self.vertices.hash(state); } } diff --git a/crates/fj-operations/src/group.rs b/crates/fj-operations/src/group.rs index 33fdb20ab..584dbd0be 100644 --- a/crates/fj-operations/src/group.rs +++ b/crates/fj-operations/src/group.rs @@ -63,7 +63,7 @@ fn copy_shape(orig: Shape, target: &mut Shape) { vertices.insert(vertex_orig, vertex); } for edge_orig in orig.edges() { - let curve = curves[&edge_orig.get().curve].clone(); + let curve = curves[edge_orig.get().curve.canonical()].clone(); let vertices = edge_orig.get().vertices.as_ref().map(|vs| { vs.clone() .map(|vertex| vertices[vertex.canonical()].clone()) From 9726984314ae1c25ef2c236037b062aa1a13b9f5 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Mon, 16 May 2022 18:08:25 +0200 Subject: [PATCH 5/5] Refactor --- crates/fj-kernel/src/topology/edges.rs | 17 +---------------- 1 file changed, 1 insertion(+), 16 deletions(-) diff --git a/crates/fj-kernel/src/topology/edges.rs b/crates/fj-kernel/src/topology/edges.rs index 2859828c2..73f590b66 100644 --- a/crates/fj-kernel/src/topology/edges.rs +++ b/crates/fj-kernel/src/topology/edges.rs @@ -1,5 +1,3 @@ -use std::hash::{Hash, Hasher}; - use crate::{ geometry::Curve, shape::{Handle, LocalForm, Shape}, @@ -19,7 +17,7 @@ use super::{vertices::Vertex, EdgeBuilder}; /// An edge that is part of a [`Shape`] must be structurally sound. That means /// the curve and any vertices that it refers to, must be part of the same /// shape. -#[derive(Clone, Debug, Eq, Ord, PartialOrd)] +#[derive(Clone, Debug, Eq, PartialEq, Hash, Ord, PartialOrd)] pub struct Edge { /// Access the curve that defines the edge's geometry /// @@ -102,16 +100,3 @@ impl Edge { .map(|[a, b]| [a.canonical().get(), b.canonical().get()]) } } - -impl PartialEq for Edge { - fn eq(&self, other: &Self) -> bool { - self.curve == other.curve && self.vertices == other.vertices - } -} - -impl Hash for Edge { - fn hash(&self, state: &mut H) { - self.curve.hash(state); - self.vertices.hash(state); - } -}