From 1f7a01fc2289c8fe331c1f6207b05a7ecedceced Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 24 May 2022 13:53:27 +0200 Subject: [PATCH 1/5] Simplify local representation of edge vertices --- crates/fj-kernel/src/algorithms/approx/edges.rs | 10 ++++++---- crates/fj-kernel/src/topology/edge.rs | 5 +++-- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/crates/fj-kernel/src/algorithms/approx/edges.rs b/crates/fj-kernel/src/algorithms/approx/edges.rs index 89fce48b4..930fb23a3 100644 --- a/crates/fj-kernel/src/algorithms/approx/edges.rs +++ b/crates/fj-kernel/src/algorithms/approx/edges.rs @@ -1,7 +1,9 @@ +use fj_math::Point; + use crate::{geometry, shape::LocalForm, topology::Vertex}; pub fn approximate_edge( - vertices: Option<[LocalForm, Vertex<3>>; 2]>, + vertices: Option<[LocalForm, Vertex<3>>; 2]>, points: &mut Vec>, ) { // Insert the exact vertices of this edge into the approximation. This means @@ -15,7 +17,7 @@ pub fn approximate_edge( let vertices = vertices.map(|vertices| { vertices.map(|vertex| { geometry::Point::new( - *vertex.local().point.local(), + *vertex.local(), vertex.canonical().get().point(), ) }) @@ -58,8 +60,8 @@ mod test { let v2 = Vertex::builder(&mut shape).build_from_point(d)?; let vertices = [ - LocalForm::new(v1.get().with_local_form([0.]), v1), - LocalForm::new(v2.get().with_local_form([1.]), v2), + LocalForm::new(Point::from([0.]), v1), + LocalForm::new(Point::from([1.]), v2), ]; let a = geometry::Point::new([0.0], a); diff --git a/crates/fj-kernel/src/topology/edge.rs b/crates/fj-kernel/src/topology/edge.rs index 6fd0be827..8075a91b0 100644 --- a/crates/fj-kernel/src/topology/edge.rs +++ b/crates/fj-kernel/src/topology/edge.rs @@ -1,5 +1,7 @@ use std::fmt; +use fj_math::Point; + use crate::{ geometry::Curve, shape::{Handle, LocalForm, Shape}, @@ -32,7 +34,7 @@ pub struct Edge { /// /// If there are no such vertices, that means that both the curve and the /// edge are continuous (i.e. connected to themselves). - pub vertices: Option<[LocalForm, Vertex<3>>; 2]>, + pub vertices: Option<[LocalForm, Vertex<3>>; 2]>, } impl Edge<3> { @@ -69,7 +71,6 @@ impl Edge<3> { .get() .point_to_curve_coords(canonical.get().point()) .local(); - let local = canonical.get().with_local_form(local); LocalForm::new(local, canonical) }) }); From a4c090605cb5127f772eb66f57c115a0e9c3ac40 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 24 May 2022 13:55:40 +0200 Subject: [PATCH 2/5] Remove unused method It is no longer used from within the Fornjot code, and further simplifications will soon make it completely redundant. --- crates/fj-kernel/src/topology/vertex.rs | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/crates/fj-kernel/src/topology/vertex.rs b/crates/fj-kernel/src/topology/vertex.rs index ef7f1e6e1..cc9407980 100644 --- a/crates/fj-kernel/src/topology/vertex.rs +++ b/crates/fj-kernel/src/topology/vertex.rs @@ -56,16 +56,6 @@ impl Vertex<3> { pub fn point(&self) -> Point<3> { self.point.canonical().get() } - - /// Add a local form of the point to the vertex - pub fn with_local_form( - self, - local: impl Into>, - ) -> Vertex { - Vertex { - point: LocalForm::new(local.into(), self.point.canonical()), - } - } } impl PartialEq for Vertex { From 12ded5d89ab344c4d379d83b1e690585a8795389 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 24 May 2022 14:10:18 +0200 Subject: [PATCH 3/5] Simplify `Vertex` Thanks to the simplification in `Edge`, it no longer needs a local representation. --- crates/fj-kernel/src/shape/object.rs | 7 ++----- crates/fj-kernel/src/shape/validate/structural.rs | 6 ++---- crates/fj-kernel/src/topology/vertex.rs | 14 ++++++-------- 3 files changed, 10 insertions(+), 17 deletions(-) diff --git a/crates/fj-kernel/src/shape/object.rs b/crates/fj-kernel/src/shape/object.rs index 0e8ebae92..1371fd907 100644 --- a/crates/fj-kernel/src/shape/object.rs +++ b/crates/fj-kernel/src/shape/object.rs @@ -89,11 +89,8 @@ impl Object for Vertex<3> { shape: &mut Shape, mapping: &mut Mapping, ) -> ValidationResult { - let point = self.point().merge_into( - Some(self.point.canonical()), - shape, - mapping, - )?; + let point = + self.point().merge_into(Some(self.point), shape, mapping)?; let merged = shape.get_handle_or_insert(Vertex::new(point))?; if let Some(handle) = handle { diff --git a/crates/fj-kernel/src/shape/validate/structural.rs b/crates/fj-kernel/src/shape/validate/structural.rs index 20af25919..fdb37883a 100644 --- a/crates/fj-kernel/src/shape/validate/structural.rs +++ b/crates/fj-kernel/src/shape/validate/structural.rs @@ -12,11 +12,9 @@ pub fn validate_vertex( vertex: &Vertex<3>, stores: &Stores, ) -> Result<(), StructuralIssues> { - let point = vertex.point.canonical(); - - if !stores.points.contains(&point) { + if !stores.points.contains(&vertex.point) { return Err(StructuralIssues { - missing_point: Some(point), + missing_point: Some(vertex.point.clone()), ..StructuralIssues::default() }); } diff --git a/crates/fj-kernel/src/topology/vertex.rs b/crates/fj-kernel/src/topology/vertex.rs index cc9407980..ecc8513dc 100644 --- a/crates/fj-kernel/src/topology/vertex.rs +++ b/crates/fj-kernel/src/topology/vertex.rs @@ -2,7 +2,7 @@ use std::hash::Hash; use fj_math::Point; -use crate::shape::{Handle, LocalForm, Shape}; +use crate::shape::{Handle, Shape}; use super::VertexBuilder; @@ -33,15 +33,13 @@ use super::VertexBuilder; #[derive(Clone, Debug, Eq, Ord, PartialOrd)] pub struct Vertex { /// The point that defines the location of the vertex - pub point: LocalForm, Point<3>>, + pub point: Handle>, } impl Vertex<3> { /// Construct a new instance of `Vertex` pub fn new(point: Handle>) -> Self { - Self { - point: LocalForm::new(point.get(), point), - } + Self { point } } /// Build a vertex using the [`VertexBuilder`] API @@ -54,18 +52,18 @@ impl Vertex<3> { /// This is a convenience method that saves the caller from dealing with the /// [`Handle`]. pub fn point(&self) -> Point<3> { - self.point.canonical().get() + self.point.get() } } impl PartialEq for Vertex { fn eq(&self, other: &Self) -> bool { - self.point == other.point + self.point.get() == other.point.get() } } impl Hash for Vertex { fn hash(&self, state: &mut H) { - self.point.hash(state); + self.point.get().hash(state); } } From 87588956c101d1806b2b41755094a6154842acc6 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 24 May 2022 14:13:00 +0200 Subject: [PATCH 4/5] Remove unused generic argument --- crates/fj-kernel/src/algorithms/approx/edges.rs | 2 +- crates/fj-kernel/src/shape/api.rs | 2 +- crates/fj-kernel/src/shape/mapping.rs | 4 ++-- crates/fj-kernel/src/shape/object.rs | 4 ++-- crates/fj-kernel/src/shape/stores.rs | 2 +- crates/fj-kernel/src/shape/validate/mod.rs | 4 ++-- crates/fj-kernel/src/shape/validate/structural.rs | 4 ++-- crates/fj-kernel/src/shape/validate/uniqueness.rs | 4 ++-- crates/fj-kernel/src/topology/builder.rs | 4 ++-- crates/fj-kernel/src/topology/edge.rs | 6 +++--- crates/fj-kernel/src/topology/vertex.rs | 12 ++++++------ 11 files changed, 24 insertions(+), 24 deletions(-) diff --git a/crates/fj-kernel/src/algorithms/approx/edges.rs b/crates/fj-kernel/src/algorithms/approx/edges.rs index 930fb23a3..538081b12 100644 --- a/crates/fj-kernel/src/algorithms/approx/edges.rs +++ b/crates/fj-kernel/src/algorithms/approx/edges.rs @@ -3,7 +3,7 @@ use fj_math::Point; use crate::{geometry, shape::LocalForm, topology::Vertex}; pub fn approximate_edge( - vertices: Option<[LocalForm, Vertex<3>>; 2]>, + vertices: Option<[LocalForm, Vertex>; 2]>, points: &mut Vec>, ) { // Insert the exact vertices of this edge into the approximation. This means diff --git a/crates/fj-kernel/src/shape/api.rs b/crates/fj-kernel/src/shape/api.rs index 523fd6125..2e1966279 100644 --- a/crates/fj-kernel/src/shape/api.rs +++ b/crates/fj-kernel/src/shape/api.rs @@ -226,7 +226,7 @@ impl Shape { /// Access iterator over all vertices /// /// The caller must not make any assumptions about the order of vertices. - pub fn vertices(&self) -> Iter> { + pub fn vertices(&self) -> Iter { self.stores.vertices.iter() } diff --git a/crates/fj-kernel/src/shape/mapping.rs b/crates/fj-kernel/src/shape/mapping.rs index 98b8c2e87..9e2c18aa2 100644 --- a/crates/fj-kernel/src/shape/mapping.rs +++ b/crates/fj-kernel/src/shape/mapping.rs @@ -14,7 +14,7 @@ pub struct Mapping { pub(super) points: OneMapping>, pub(super) curves: OneMapping>, pub(super) surfaces: OneMapping, - pub(super) vertices: OneMapping>, + pub(super) vertices: OneMapping, pub(super) edges: OneMapping>, pub(super) cycles: OneMapping>, pub(super) faces: OneMapping, @@ -49,7 +49,7 @@ impl Mapping { } /// Access iterator over the mapped vertices - pub fn vertices(&self) -> &OneMapping> { + pub fn vertices(&self) -> &OneMapping { &self.vertices } diff --git a/crates/fj-kernel/src/shape/object.rs b/crates/fj-kernel/src/shape/object.rs index 1371fd907..7d715538a 100644 --- a/crates/fj-kernel/src/shape/object.rs +++ b/crates/fj-kernel/src/shape/object.rs @@ -26,7 +26,7 @@ impl private::Sealed for Point<3> {} impl private::Sealed for Curve<3> {} impl private::Sealed for Surface {} -impl private::Sealed for Vertex<3> {} +impl private::Sealed for Vertex {} impl private::Sealed for Edge<3> {} impl private::Sealed for Cycle<3> {} impl private::Sealed for Face {} @@ -82,7 +82,7 @@ impl Object for Surface { } } -impl Object for Vertex<3> { +impl Object for Vertex { fn merge_into( self, handle: Option>, diff --git a/crates/fj-kernel/src/shape/stores.rs b/crates/fj-kernel/src/shape/stores.rs index 22361dd42..8a3f57768 100644 --- a/crates/fj-kernel/src/shape/stores.rs +++ b/crates/fj-kernel/src/shape/stores.rs @@ -22,7 +22,7 @@ pub struct Stores { pub curves: Store>, pub surfaces: Store, - pub vertices: Store>, + pub vertices: Store, pub edges: Store>, pub cycles: Store>, pub faces: Store, diff --git a/crates/fj-kernel/src/shape/validate/mod.rs b/crates/fj-kernel/src/shape/validate/mod.rs index 2d68bd11b..0c2c9a6f4 100644 --- a/crates/fj-kernel/src/shape/validate/mod.rs +++ b/crates/fj-kernel/src/shape/validate/mod.rs @@ -56,7 +56,7 @@ impl Validate for Surface { } } -impl Validate for Vertex<3> { +impl Validate for Vertex { /// Validate the vertex /// /// # Implementation note @@ -163,7 +163,7 @@ impl ValidationError { /// Indicate whether validation found a missing vertex #[cfg(test)] - pub fn missing_vertex(&self, vertex: &Handle>) -> bool { + pub fn missing_vertex(&self, vertex: &Handle) -> bool { if let Self::Structural(StructuralIssues { missing_vertices, .. }) = self diff --git a/crates/fj-kernel/src/shape/validate/structural.rs b/crates/fj-kernel/src/shape/validate/structural.rs index fdb37883a..83a3f6b2d 100644 --- a/crates/fj-kernel/src/shape/validate/structural.rs +++ b/crates/fj-kernel/src/shape/validate/structural.rs @@ -9,7 +9,7 @@ use crate::{ }; pub fn validate_vertex( - vertex: &Vertex<3>, + vertex: &Vertex, stores: &Stores, ) -> Result<(), StructuralIssues> { if !stores.points.contains(&vertex.point) { @@ -117,7 +117,7 @@ pub struct StructuralIssues { pub missing_curve: Option>>, /// Missing vertices found in edge validation - pub missing_vertices: HashSet>>, + pub missing_vertices: HashSet>, /// Missing edges found in cycle validation pub missing_edges: HashSet>>, diff --git a/crates/fj-kernel/src/shape/validate/uniqueness.rs b/crates/fj-kernel/src/shape/validate/uniqueness.rs index 2e6032a4a..bc5a3d97e 100644 --- a/crates/fj-kernel/src/shape/validate/uniqueness.rs +++ b/crates/fj-kernel/src/shape/validate/uniqueness.rs @@ -8,8 +8,8 @@ use crate::{ }; pub fn validate_vertex( - vertex: &Vertex<3>, - handle: Option<&Handle>>, + vertex: &Vertex, + handle: Option<&Handle>, min_distance: Scalar, stores: &Stores, ) -> Result<(), UniquenessIssues> { diff --git a/crates/fj-kernel/src/topology/builder.rs b/crates/fj-kernel/src/topology/builder.rs index 6acd2f7cd..e178b453e 100644 --- a/crates/fj-kernel/src/topology/builder.rs +++ b/crates/fj-kernel/src/topology/builder.rs @@ -26,7 +26,7 @@ impl<'r> VertexBuilder<'r> { pub fn build_from_point( self, point: impl Into>, - ) -> ValidationResult> { + ) -> ValidationResult { let point = self.shape.get_handle_or_insert(point.into())?; let vertex = self.shape.get_handle_or_insert(Vertex::new(point))?; @@ -80,7 +80,7 @@ impl<'r> EdgeBuilder<'r> { /// Build a line segment from two vertices pub fn build_line_segment_from_vertices( self, - vertices: [Handle>; 2], + vertices: [Handle; 2], ) -> 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/edge.rs b/crates/fj-kernel/src/topology/edge.rs index 8075a91b0..424ef0218 100644 --- a/crates/fj-kernel/src/topology/edge.rs +++ b/crates/fj-kernel/src/topology/edge.rs @@ -34,7 +34,7 @@ pub struct Edge { /// /// If there are no such vertices, that means that both the curve and the /// edge are continuous (i.e. connected to themselves). - pub vertices: Option<[LocalForm, Vertex<3>>; 2]>, + pub vertices: Option<[LocalForm, Vertex>; 2]>, } impl Edge<3> { @@ -60,7 +60,7 @@ impl Edge<3> { /// pub fn new( curve: Handle>, - vertices: Option<[Handle>; 2]>, + vertices: Option<[Handle; 2]>, ) -> Self { let curve = LocalForm::canonical_only(curve); @@ -97,7 +97,7 @@ impl Edge { /// /// This is a convenience method that saves the caller from dealing with the /// [`Handle`]s. - pub fn vertices(&self) -> Option<[Vertex<3>; 2]> { + pub fn vertices(&self) -> Option<[Vertex; 2]> { self.vertices .as_ref() .map(|[a, b]| [a.canonical().get(), b.canonical().get()]) diff --git a/crates/fj-kernel/src/topology/vertex.rs b/crates/fj-kernel/src/topology/vertex.rs index ecc8513dc..3894107b6 100644 --- a/crates/fj-kernel/src/topology/vertex.rs +++ b/crates/fj-kernel/src/topology/vertex.rs @@ -31,12 +31,12 @@ use super::VertexBuilder; /// between distinct vertices can be configured using /// [`Shape::with_minimum_distance`]. #[derive(Clone, Debug, Eq, Ord, PartialOrd)] -pub struct Vertex { +pub struct Vertex { /// The point that defines the location of the vertex pub point: Handle>, } -impl Vertex<3> { +impl Vertex { /// Construct a new instance of `Vertex` pub fn new(point: Handle>) -> Self { Self { point } @@ -56,14 +56,14 @@ impl Vertex<3> { } } -impl PartialEq for Vertex { +impl PartialEq for Vertex { fn eq(&self, other: &Self) -> bool { - self.point.get() == other.point.get() + self.point() == other.point() } } -impl Hash for Vertex { +impl Hash for Vertex { fn hash(&self, state: &mut H) { - self.point.get().hash(state); + self.point().hash(state); } } From 407026178da44aa9b4e102b4b9c949f81940c731 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 24 May 2022 14:15:54 +0200 Subject: [PATCH 5/5] Remove redundant constructor --- crates/fj-kernel/src/shape/api.rs | 10 +++++----- crates/fj-kernel/src/shape/object.rs | 2 +- crates/fj-kernel/src/topology/builder.rs | 2 +- crates/fj-kernel/src/topology/vertex.rs | 5 ----- 4 files changed, 7 insertions(+), 12 deletions(-) diff --git a/crates/fj-kernel/src/shape/api.rs b/crates/fj-kernel/src/shape/api.rs index 2e1966279..ca144dcb9 100644 --- a/crates/fj-kernel/src/shape/api.rs +++ b/crates/fj-kernel/src/shape/api.rs @@ -293,7 +293,7 @@ mod tests { assert!(shape.get_handle(&curve.get()).as_ref() == Some(&curve)); assert!(shape.get_handle(&surface.get()).as_ref() == Some(&surface)); - let vertex = Vertex::new(point); + let vertex = Vertex { point }; let edge = Edge::new(curve, None); assert!(shape.get_handle(&vertex).is_none()); @@ -326,23 +326,23 @@ mod tests { let mut other = Shape::new(); let point = shape.insert(Point::from([0., 0., 0.]))?; - shape.insert(Vertex::new(point))?; + shape.insert(Vertex { point })?; // Should fail, as `point` is not part of the shape. let point = other.insert(Point::from([1., 0., 0.]))?; - let result = shape.insert(Vertex::new(point)); + let result = shape.insert(Vertex { point }); assert!(matches!(result, Err(ValidationError::Structural(_)))); // `point` is too close to the original point. `assert!` is commented, // because that only causes a warning to be logged right now. let point = shape.insert(Point::from([5e-8, 0., 0.]))?; - let result = shape.insert(Vertex::new(point)); + let result = shape.insert(Vertex { point }); assert!(matches!(result, Err(ValidationError::Uniqueness(_)))); // `point` is farther than `MIN_DISTANCE` away from original point. // Should work. let point = shape.insert(Point::from([5e-6, 0., 0.]))?; - shape.insert(Vertex::new(point))?; + shape.insert(Vertex { point })?; Ok(()) } diff --git a/crates/fj-kernel/src/shape/object.rs b/crates/fj-kernel/src/shape/object.rs index 7d715538a..a08cd4636 100644 --- a/crates/fj-kernel/src/shape/object.rs +++ b/crates/fj-kernel/src/shape/object.rs @@ -91,7 +91,7 @@ impl Object for Vertex { ) -> ValidationResult { let point = self.point().merge_into(Some(self.point), shape, mapping)?; - let merged = shape.get_handle_or_insert(Vertex::new(point))?; + let merged = shape.get_handle_or_insert(Vertex { point })?; if let Some(handle) = handle { mapping.vertices.insert(handle, merged.clone()); diff --git a/crates/fj-kernel/src/topology/builder.rs b/crates/fj-kernel/src/topology/builder.rs index e178b453e..a79de2a6b 100644 --- a/crates/fj-kernel/src/topology/builder.rs +++ b/crates/fj-kernel/src/topology/builder.rs @@ -28,7 +28,7 @@ impl<'r> VertexBuilder<'r> { point: impl Into>, ) -> ValidationResult { let point = self.shape.get_handle_or_insert(point.into())?; - let vertex = self.shape.get_handle_or_insert(Vertex::new(point))?; + let vertex = self.shape.get_handle_or_insert(Vertex { point })?; Ok(vertex) } diff --git a/crates/fj-kernel/src/topology/vertex.rs b/crates/fj-kernel/src/topology/vertex.rs index 3894107b6..4e1cc44f4 100644 --- a/crates/fj-kernel/src/topology/vertex.rs +++ b/crates/fj-kernel/src/topology/vertex.rs @@ -37,11 +37,6 @@ pub struct Vertex { } impl Vertex { - /// Construct a new instance of `Vertex` - pub fn new(point: Handle>) -> Self { - Self { point } - } - /// Build a vertex using the [`VertexBuilder`] API pub fn builder(shape: &mut Shape) -> VertexBuilder { VertexBuilder::new(shape)