From b0edf98cfa3d69507e600b49cbb9fb9b2be1fccd Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 28 Jun 2022 17:33:47 +0200 Subject: [PATCH 1/8] Remove obsolete comment --- crates/fj-kernel/src/builder.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/crates/fj-kernel/src/builder.rs b/crates/fj-kernel/src/builder.rs index 8478d7c29..56939a140 100644 --- a/crates/fj-kernel/src/builder.rs +++ b/crates/fj-kernel/src/builder.rs @@ -75,8 +75,6 @@ impl<'r> EdgeBuilder<'r> { self, vertices: [impl Into>; 2], ) -> Handle> { - // 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 .map(|point| Vertex::builder(self.shape).build_from_point(point)); From c7fdf22bf7a5b25efbf62e59955f0ac0ea695384 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 28 Jun 2022 17:34:53 +0200 Subject: [PATCH 2/8] Simplify arguments of `EdgeBuilder` method --- crates/fj-kernel/src/builder.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/crates/fj-kernel/src/builder.rs b/crates/fj-kernel/src/builder.rs index 56939a140..cccb3d26b 100644 --- a/crates/fj-kernel/src/builder.rs +++ b/crates/fj-kernel/src/builder.rs @@ -75,8 +75,10 @@ impl<'r> EdgeBuilder<'r> { self, vertices: [impl Into>; 2], ) -> Handle> { - let vertices = vertices - .map(|point| Vertex::builder(self.shape).build_from_point(point)); + let vertices = vertices.map(|point| { + let point = point.into(); + Vertex { point } + }); self.build_line_segment_from_vertices(vertices) } @@ -84,14 +86,16 @@ impl<'r> EdgeBuilder<'r> { /// Build a line segment from two vertices pub fn build_line_segment_from_vertices( self, - [a, b]: [Handle; 2], + [a, b]: [Vertex; 2], ) -> Handle> { let curve = { - let points = [&a, &b].map(|vertex| vertex.get().point); + let points = [&a, &b].map(|vertex| vertex.point); let curve = Curve::Line(Line::from_points(points)); self.shape.get_handle_or_insert(curve) }; + let [a, b] = [a, b].map(|vertex| self.shape.insert(vertex)); + let vertices = [ LocalForm::new(Point::from([0.]), a), LocalForm::new(Point::from([1.]), b), From ab3ee7bf0dfcb51556cee8ce1ce3347b58d43c2c Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 28 Jun 2022 17:37:18 +0200 Subject: [PATCH 3/8] Refactor --- crates/fj-kernel/src/builder.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/fj-kernel/src/builder.rs b/crates/fj-kernel/src/builder.rs index cccb3d26b..c1393e734 100644 --- a/crates/fj-kernel/src/builder.rs +++ b/crates/fj-kernel/src/builder.rs @@ -89,7 +89,7 @@ impl<'r> EdgeBuilder<'r> { [a, b]: [Vertex; 2], ) -> Handle> { let curve = { - let points = [&a, &b].map(|vertex| vertex.point); + let points = [a, b].map(|vertex| vertex.point); let curve = Curve::Line(Line::from_points(points)); self.shape.get_handle_or_insert(curve) }; From a17e400b2bbfa32add957ad55fb78c4c2dba2074 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 28 Jun 2022 17:40:55 +0200 Subject: [PATCH 4/8] Inline method call --- crates/fj-kernel/src/objects/face.rs | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/crates/fj-kernel/src/objects/face.rs b/crates/fj-kernel/src/objects/face.rs index 55f90387e..1a3834e53 100644 --- a/crates/fj-kernel/src/objects/face.rs +++ b/crates/fj-kernel/src/objects/face.rs @@ -211,12 +211,7 @@ impl CyclesInFace { /// Access an iterator over the canonical forms of the cycles pub fn as_canonical(&self) -> impl Iterator> + '_ { - self.as_handle().map(|cycle| cycle.get()) - } - - /// Access an iterator over handles to the cycles - pub fn as_handle(&self) -> impl Iterator>> + '_ { - self.0.iter().map(|cycle| cycle.canonical()) + self.0.iter().map(|cycle| cycle.canonical().get()) } /// Access an iterator over local forms of the cycles From 68da99913aad8a84aa24cc47918745c243988831 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 28 Jun 2022 17:50:45 +0200 Subject: [PATCH 5/8] Give `LocalForm` ownership of the canonical form We're entering the last stretch of the work towards removing `Shape`. Now, nothing should really depend on `Shape` any more, so it's a matter of replacing `Handle`s where we can find them with direct ownership of the previously referred to object. --- .../fj-kernel/src/algorithms/approx/edges.rs | 6 +-- crates/fj-kernel/src/algorithms/sweep.rs | 19 +++----- crates/fj-kernel/src/algorithms/transform.rs | 33 ++++++------- crates/fj-kernel/src/builder.rs | 32 ++++++------- crates/fj-kernel/src/iter.rs | 3 +- crates/fj-kernel/src/objects/cycle.rs | 6 +-- crates/fj-kernel/src/objects/edge.rs | 6 +-- crates/fj-kernel/src/objects/face.rs | 2 +- crates/fj-kernel/src/shape/api.rs | 4 +- crates/fj-kernel/src/shape/local.rs | 17 ++++--- crates/fj-kernel/src/shape/object.rs | 24 ++++++---- crates/fj-kernel/src/validation/coherence.rs | 2 +- crates/fj-kernel/src/validation/mod.rs | 46 +++++++++++-------- crates/fj-kernel/src/validation/structural.rs | 2 +- crates/fj-operations/src/circle.rs | 2 +- crates/fj-operations/src/difference_2d.rs | 15 +++--- 16 files changed, 106 insertions(+), 113 deletions(-) diff --git a/crates/fj-kernel/src/algorithms/approx/edges.rs b/crates/fj-kernel/src/algorithms/approx/edges.rs index 73a6046bf..f570ff843 100644 --- a/crates/fj-kernel/src/algorithms/approx/edges.rs +++ b/crates/fj-kernel/src/algorithms/approx/edges.rs @@ -13,7 +13,7 @@ pub fn approx_edge( // the same vertex would be understood to refer to very close, but distinct // vertices. let vertices = vertices.convert(|vertex| { - geometry::Point::new(*vertex.local(), vertex.canonical().get().point) + geometry::Point::new(*vertex.local(), vertex.canonical().point) }); if let Some([a, b]) = vertices { points.insert(0, a); @@ -49,8 +49,8 @@ mod test { let c = Point::from([3., 5., 8.]); let d = Point::from([5., 8., 13.]); - let v1 = Vertex::builder(&mut shape).build_from_point(a); - let v2 = Vertex::builder(&mut shape).build_from_point(d); + let v1 = Vertex::builder(&mut shape).build_from_point(a).get(); + let v2 = Vertex::builder(&mut shape).build_from_point(d).get(); let vertices = VerticesOfEdge::from_vertices([ LocalForm::new(Point::from([0.]), v1), diff --git a/crates/fj-kernel/src/algorithms/sweep.rs b/crates/fj-kernel/src/algorithms/sweep.rs index 0ee491af3..ddca4106b 100644 --- a/crates/fj-kernel/src/algorithms/sweep.rs +++ b/crates/fj-kernel/src/algorithms/sweep.rs @@ -166,17 +166,15 @@ fn create_non_continuous_side_face( let [[a, b], [c, d]] = [vertices_bottom, vertices_top]; - let vertices = if is_sweep_along_negative_direction { + if is_sweep_along_negative_direction { [b, a, c, d] } else { [a, b, d, c] - }; - - vertices.map(|vertex| tmp.get_handle_or_insert(vertex)) + } }; let surface = { - let [a, b, _, c] = vertices.clone().map(|vertex| vertex.get().point); + let [a, b, _, c] = vertices.map(|vertex| vertex.point); Surface::plane_from_points([a, b, c]) }; let surface = tmp.get_handle_or_insert(surface); @@ -201,16 +199,15 @@ fn create_non_continuous_side_face( let curve = { let local = Curve::line_from_points([a.0, b.0]); - let global = [a, b].map(|vertex| vertex.1.get().point); + let global = [a, b].map(|vertex| vertex.1.point); let global = Curve::line_from_points(global); - let global = tmp.get_handle_or_insert(global); LocalForm::new(local, global) }; let vertices = VerticesOfEdge::from_vertices([ - LocalForm::new(Point::from([0.]), a.1.clone()), - LocalForm::new(Point::from([1.]), b.1.clone()), + LocalForm::new(Point::from([0.]), a.1), + LocalForm::new(Point::from([1.]), b.1), ]); let edge = { @@ -223,7 +220,6 @@ fn create_non_continuous_side_face( curve: LocalForm::canonical_only(curve.canonical()), vertices, }; - let global = tmp.get_handle_or_insert(global); LocalForm::new(local, global) }; @@ -236,7 +232,6 @@ fn create_non_continuous_side_face( let global = Cycle::new(local.edges.iter().map(|edge| edge.canonical())); - let global = tmp.get_handle_or_insert(global); LocalForm::new(local, global) }; @@ -257,8 +252,6 @@ fn create_continuous_side_face( ) { let translation = Transform::translation(path); - let mut tmp = Shape::new(); - let edge = tmp.merge(edge); let cycle = Cycle::new(vec![edge]); let approx = CycleApprox::new(&cycle, tolerance); diff --git a/crates/fj-kernel/src/algorithms/transform.rs b/crates/fj-kernel/src/algorithms/transform.rs index bd148a9ae..4b9bf019f 100644 --- a/crates/fj-kernel/src/algorithms/transform.rs +++ b/crates/fj-kernel/src/algorithms/transform.rs @@ -49,8 +49,6 @@ pub fn transform_cycles( cycles: &CyclesInFace, transform: &Transform, ) -> CyclesInFace { - let mut tmp = Shape::new(); - let cycles = cycles.as_local_form().map(|cycle| { let edges_local = cycle .local() @@ -58,65 +56,62 @@ pub fn transform_cycles( .iter() .map(|edge| { let curve_local = *edge.local().curve.local(); - let curve_canonical = tmp - .merge(edge.canonical().get().curve().transform(transform)); + let curve_canonical = + edge.canonical().curve().transform(transform); - let vertices = edge.canonical().get().vertices.map(|vertex| { - let point = vertex.canonical().get().point; + let vertices = edge.canonical().vertices.map(|vertex| { + let point = vertex.canonical().point; let point = transform.transform_point(&point); let local = *vertex.local(); - let canonical = tmp.merge(Vertex { point }); + let canonical = Vertex { point }; LocalForm::new(local, canonical) }); let edge_local = Edge { - curve: LocalForm::new(curve_local, curve_canonical.clone()), + curve: LocalForm::new(curve_local, curve_canonical), vertices: vertices.clone(), }; - let edge_canonical = tmp.merge(Edge { + let edge_canonical = Edge { curve: LocalForm::canonical_only(curve_canonical), vertices, - }); + }; LocalForm::new(edge_local, edge_canonical) }) .collect(); let edges_canonical = cycle .canonical() - .get() .edges .iter() .map(|edge| { - let edge = edge.canonical().get(); + let edge = edge.canonical(); let curve = { let curve = edge.curve().transform(transform); - - let curve = tmp.merge(curve); LocalForm::canonical_only(curve) }; let vertices = edge.vertices.map(|vertex| { - let point = vertex.canonical().get().point; + let point = vertex.canonical().point; let point = transform.transform_point(&point); let local = *vertex.local(); - let canonical = tmp.merge(Vertex { point }); + let canonical = Vertex { point }; LocalForm::new(local, canonical) }); - let edge = tmp.merge(Edge { curve, vertices }); + let edge = Edge { curve, vertices }; LocalForm::canonical_only(edge) }) .collect(); let cycle_local = Cycle { edges: edges_local }; - let cycle_canonical = tmp.merge(Cycle { + let cycle_canonical = Cycle { edges: edges_canonical, - }); + }; LocalForm::new(cycle_local, cycle_canonical) }); diff --git a/crates/fj-kernel/src/builder.rs b/crates/fj-kernel/src/builder.rs index c1393e734..d1d5d24a2 100644 --- a/crates/fj-kernel/src/builder.rs +++ b/crates/fj-kernel/src/builder.rs @@ -51,21 +51,20 @@ impl<'r> EdgeBuilder<'r> { a: Vector::from([radius, Scalar::ZERO]), b: Vector::from([Scalar::ZERO, radius]), }); - let curve_canonical = - self.shape.get_handle_or_insert(Curve::Circle(Circle { - center: Point::origin(), - a: Vector::from([radius, Scalar::ZERO, Scalar::ZERO]), - b: Vector::from([Scalar::ZERO, radius, Scalar::ZERO]), - })); + let curve_canonical = Curve::Circle(Circle { + center: Point::origin(), + a: Vector::from([radius, Scalar::ZERO, Scalar::ZERO]), + b: Vector::from([Scalar::ZERO, radius, Scalar::ZERO]), + }); let edge_local = Edge { - curve: LocalForm::new(curve_local, curve_canonical.clone()), + curve: LocalForm::new(curve_local, curve_canonical), vertices: VerticesOfEdge::none(), }; - let edge_canonical = self.shape.get_handle_or_insert(Edge { + let edge_canonical = Edge { curve: LocalForm::canonical_only(curve_canonical), vertices: VerticesOfEdge::none(), - }); + }; LocalForm::new(edge_local, edge_canonical) } @@ -90,12 +89,9 @@ impl<'r> EdgeBuilder<'r> { ) -> Handle> { let curve = { let points = [a, b].map(|vertex| vertex.point); - let curve = Curve::Line(Line::from_points(points)); - self.shape.get_handle_or_insert(curve) + Curve::Line(Line::from_points(points)) }; - let [a, b] = [a, b].map(|vertex| self.shape.insert(vertex)); - let vertices = [ LocalForm::new(Point::from([0.]), a), LocalForm::new(Point::from([1.]), b), @@ -144,14 +140,15 @@ impl<'r> CycleBuilder<'r> { let points_canonical = points .map(|point| self.surface.point_from_surface_coords(point)); let edge_canonical = Edge::builder(self.shape) - .build_line_segment_from_points(points_canonical); + .build_line_segment_from_points(points_canonical) + .get(); let edge_local = Edge { curve: LocalForm::new( Curve::Line(Line::from_points(points)), - edge_canonical.get().curve.canonical(), + edge_canonical.curve.canonical(), ), - vertices: edge_canonical.get().vertices, + vertices: edge_canonical.vertices.clone(), }; edges.push(LocalForm::new(edge_local, edge_canonical)); @@ -162,8 +159,7 @@ impl<'r> CycleBuilder<'r> { }; let edges_canonical = edges.into_iter().map(|edge| edge.canonical()); - let canonical = - self.shape.get_handle_or_insert(Cycle::new(edges_canonical)); + let canonical = Cycle::new(edges_canonical); LocalForm::new(local, canonical) } diff --git a/crates/fj-kernel/src/iter.rs b/crates/fj-kernel/src/iter.rs index 1afdf6c87..30b4939ae 100644 --- a/crates/fj-kernel/src/iter.rs +++ b/crates/fj-kernel/src/iter.rs @@ -463,8 +463,7 @@ mod tests { let mut shape = Shape::new(); let cycle = Cycle::builder(Surface::xy_plane(), &mut shape) .build_polygon([[0., 0.], [1., 0.], [0., 1.]]) - .canonical() - .get(); + .canonical(); assert_eq!(3, cycle.curve_iter().count()); assert_eq!(1, cycle.cycle_iter().count()); diff --git a/crates/fj-kernel/src/objects/cycle.rs b/crates/fj-kernel/src/objects/cycle.rs index dc2927cb2..05da906ee 100644 --- a/crates/fj-kernel/src/objects/cycle.rs +++ b/crates/fj-kernel/src/objects/cycle.rs @@ -1,6 +1,6 @@ use crate::{ builder::CycleBuilder, - shape::{Handle, LocalForm, Shape}, + shape::{LocalForm, Shape}, }; use super::{Edge, Surface}; @@ -28,7 +28,7 @@ pub struct Cycle { impl Cycle<3> { /// Construct a `Cycle` - pub fn new(edges: impl IntoIterator>>) -> Self { + pub fn new(edges: impl IntoIterator>) -> Self { let edges = edges.into_iter().map(LocalForm::canonical_only).collect(); Self { edges } @@ -44,6 +44,6 @@ impl Cycle<3> { /// This is a convenience method that saves the caller from dealing with the /// [`Handle`]s. pub fn edges(&self) -> impl Iterator> + '_ { - self.edges.iter().map(|handle| handle.canonical().get()) + self.edges.iter().map(|handle| handle.canonical()) } } diff --git a/crates/fj-kernel/src/objects/edge.rs b/crates/fj-kernel/src/objects/edge.rs index a08582adf..061e24962 100644 --- a/crates/fj-kernel/src/objects/edge.rs +++ b/crates/fj-kernel/src/objects/edge.rs @@ -43,7 +43,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.canonical().get() + self.curve.canonical() } /// Access the vertices that the edge refers to @@ -54,7 +54,7 @@ impl Edge { self.vertices .0 .as_ref() - .map(|[a, b]| [a.canonical().get(), b.canonical().get()]) + .map(|[a, b]| [a.canonical(), b.canonical()]) } } @@ -114,7 +114,7 @@ impl VerticesOfEdge { [a.canonical(), b.canonical()] }; - return [a.clone(), b.clone()] == other || [b, a] == other; + return [a, b] == other || [b, a] == other; } } diff --git a/crates/fj-kernel/src/objects/face.rs b/crates/fj-kernel/src/objects/face.rs index 1a3834e53..a04652bce 100644 --- a/crates/fj-kernel/src/objects/face.rs +++ b/crates/fj-kernel/src/objects/face.rs @@ -211,7 +211,7 @@ impl CyclesInFace { /// Access an iterator over the canonical forms of the cycles pub fn as_canonical(&self) -> impl Iterator> + '_ { - self.0.iter().map(|cycle| cycle.canonical().get()) + self.0.iter().map(|cycle| cycle.canonical()) } /// Access an iterator over local forms of the cycles diff --git a/crates/fj-kernel/src/shape/api.rs b/crates/fj-kernel/src/shape/api.rs index 733944715..cd8ea89c7 100644 --- a/crates/fj-kernel/src/shape/api.rs +++ b/crates/fj-kernel/src/shape/api.rs @@ -177,7 +177,7 @@ mod tests { let vertex = Vertex { point }; let edge = Edge { - curve: LocalForm::canonical_only(curve), + curve: LocalForm::canonical_only(curve.get()), vertices: VerticesOfEdge::none(), }; @@ -190,7 +190,7 @@ mod tests { assert!(shape.get_handle(&vertex.get()).as_ref() == Some(&vertex)); assert!(shape.get_handle(&edge.get()).as_ref() == Some(&edge)); - let cycle = Cycle::new(vec![edge]); + let cycle = Cycle::new(vec![edge.get()]); assert!(shape.get_handle(&cycle).is_none()); let cycle = shape.insert(cycle); diff --git a/crates/fj-kernel/src/shape/local.rs b/crates/fj-kernel/src/shape/local.rs index a56814318..ed13b9a89 100644 --- a/crates/fj-kernel/src/shape/local.rs +++ b/crates/fj-kernel/src/shape/local.rs @@ -1,6 +1,6 @@ use std::hash::{Hash, Hasher}; -use super::{Handle, Object}; +use super::Object; /// A reference to an object, which includes a local form /// @@ -17,7 +17,7 @@ use super::{Handle, Object}; #[derive(Clone, Debug, Eq, Ord, PartialOrd)] pub struct LocalForm { local: Local, - canonical: Handle, + canonical: Canonical, } impl LocalForm { @@ -25,7 +25,7 @@ impl LocalForm { /// /// It is the caller's responsibility to make sure that the local and /// canonical forms passed to this method actually match. - pub fn new(local: Local, canonical: Handle) -> Self { + pub fn new(local: Local, canonical: Canonical) -> Self { Self { local, canonical } } @@ -35,7 +35,7 @@ impl LocalForm { } /// Access the canonical form of the referenced object - pub fn canonical(&self) -> Handle { + pub fn canonical(&self) -> Canonical { self.canonical.clone() } } @@ -46,8 +46,8 @@ impl LocalForm { /// 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) + pub fn canonical_only(canonical: Canonical) -> Self { + Self::new(canonical.clone(), canonical) } } @@ -57,8 +57,7 @@ where Canonical: PartialEq, { fn eq(&self, other: &Self) -> bool { - self.local == other.local - && self.canonical.get() == other.canonical.get() + self.local == other.local && self.canonical == other.canonical } } @@ -69,6 +68,6 @@ where { fn hash(&self, state: &mut H) { self.local.hash(state); - self.canonical.get().hash(state); + self.canonical.hash(state); } } diff --git a/crates/fj-kernel/src/shape/object.rs b/crates/fj-kernel/src/shape/object.rs index 5193ecacf..8d438418c 100644 --- a/crates/fj-kernel/src/shape/object.rs +++ b/crates/fj-kernel/src/shape/object.rs @@ -44,12 +44,12 @@ impl Object for Edge<3> { let vertices = self.vertices.convert(|vertex| { let canonical = vertex.canonical(); - let canonical = canonical.get().merge_into(shape); - LocalForm::new(*vertex.local(), canonical) + let canonical = canonical.merge_into(shape); + LocalForm::new(*vertex.local(), canonical.get()) }); shape.get_handle_or_insert(Edge { - curve: LocalForm::canonical_only(curve), + curve: LocalForm::canonical_only(curve.get()), vertices: VerticesOfEdge::new(vertices), }) } @@ -60,8 +60,8 @@ impl Object for Cycle<3> { let mut edges = Vec::new(); for edge in self.edges { let edge = edge.canonical(); - let edge = edge.get().merge_into(shape); - edges.push(edge); + let edge = edge.merge_into(shape); + edges.push(edge.get()); } shape.get_handle_or_insert(Cycle::new(edges)) @@ -76,14 +76,20 @@ impl Object for Face { let mut exts = Vec::new(); for cycle in face.exteriors.as_local_form() { - let merged = cycle.canonical().get().merge_into(shape); - exts.push(LocalForm::new(cycle.local().clone(), merged)); + let merged = cycle.canonical().merge_into(shape); + exts.push(LocalForm::new( + cycle.local().clone(), + merged.get(), + )); } let mut ints = Vec::new(); for cycle in face.interiors.as_local_form() { - let merged = cycle.canonical().get().merge_into(shape); - ints.push(LocalForm::new(cycle.local().clone(), merged)); + let merged = cycle.canonical().merge_into(shape); + ints.push(LocalForm::new( + cycle.local().clone(), + merged.get(), + )); } shape.get_handle_or_insert(Face::new( diff --git a/crates/fj-kernel/src/validation/coherence.rs b/crates/fj-kernel/src/validation/coherence.rs index fa7a56bef..10d9ecb52 100644 --- a/crates/fj-kernel/src/validation/coherence.rs +++ b/crates/fj-kernel/src/validation/coherence.rs @@ -19,7 +19,7 @@ pub fn validate_edge( for vertex in edge.vertices.iter() { let local = *vertex.local(); let local_as_canonical = edge.curve().point_from_curve_coords(local); - let canonical = vertex.canonical().get().point; + let canonical = vertex.canonical().point; let distance = (local_as_canonical - canonical).magnitude(); if distance > max_distance { diff --git a/crates/fj-kernel/src/validation/mod.rs b/crates/fj-kernel/src/validation/mod.rs index a3e9b78ff..ad27aa345 100644 --- a/crates/fj-kernel/src/validation/mod.rs +++ b/crates/fj-kernel/src/validation/mod.rs @@ -232,18 +232,16 @@ mod tests { #[test] fn coherence_edge() { - let mut tmp = Shape::new(); - let a = Point::from([0., 0., 0.]); let b = Point::from([1., 0., 0.]); let curve = { - let curve = tmp.insert(Curve::line_from_points([a, b])); + let curve = Curve::line_from_points([a, b]); LocalForm::canonical_only(curve) }; - let a = tmp.insert(Vertex { point: a }); - let b = tmp.insert(Vertex { point: b }); + let a = Vertex { point: a }; + let b = Vertex { point: b }; let deviation = Scalar::from_f64(0.25); @@ -279,15 +277,17 @@ mod tests { // Trying to refer to edge that is not from the same shape. Should fail. let edge = Edge::builder(&mut other) - .build_line_segment_from_points([[0., 0., 0.], [1., 0., 0.]]); + .build_line_segment_from_points([[0., 0., 0.], [1., 0., 0.]]) + .get(); shape.insert(Cycle::new(vec![edge.clone()])); let err = validate(shape.clone(), &ValidationConfig::default()).unwrap_err(); - assert!(err.missing_edge(&edge.get())); + assert!(err.missing_edge(&edge)); // Referring to edge that *is* from the same shape. Should work. let edge = Edge::builder(&mut shape) - .build_line_segment_from_points([[0., 0., 0.], [1., 0., 0.]]); + .build_line_segment_from_points([[0., 0., 0.], [1., 0., 0.]]) + .get(); shape.insert(Cycle::new(vec![edge])); } @@ -296,27 +296,35 @@ mod tests { let mut shape = Shape::new(); let mut other = Shape::new(); - let curve = other.insert(Curve::x_axis()); - let a = Vertex::builder(&mut other).build_from_point([1., 0., 0.]); - let b = Vertex::builder(&mut other).build_from_point([2., 0., 0.]); + let curve = Curve::x_axis(); + let a = Vertex::builder(&mut other) + .build_from_point([1., 0., 0.]) + .get(); + let b = Vertex::builder(&mut other) + .build_from_point([2., 0., 0.]) + .get(); let a = LocalForm::new(Point::from([1.]), a); let b = LocalForm::new(Point::from([2.]), b); // Shouldn't work. Nothing has been added to `shape`. shape.insert(Edge { - curve: LocalForm::canonical_only(curve.clone()), + curve: LocalForm::canonical_only(curve), vertices: VerticesOfEdge::from_vertices([a.clone(), b.clone()]), }); let err = validate(shape.clone(), &ValidationConfig::default()).unwrap_err(); - assert!(err.missing_curve(&curve.get())); - assert!(err.missing_vertex(&a.canonical().get())); - assert!(err.missing_vertex(&b.canonical().get())); + assert!(err.missing_curve(&curve)); + assert!(err.missing_vertex(&a.canonical())); + assert!(err.missing_vertex(&b.canonical())); - let curve = shape.insert(Curve::x_axis()); - let a = Vertex::builder(&mut shape).build_from_point([1., 0., 0.]); - let b = Vertex::builder(&mut shape).build_from_point([2., 0., 0.]); + let curve = Curve::x_axis(); + let a = Vertex::builder(&mut shape) + .build_from_point([1., 0., 0.]) + .get(); + let b = Vertex::builder(&mut shape) + .build_from_point([2., 0., 0.]) + .get(); let a = LocalForm::new(Point::from([1.]), a); let b = LocalForm::new(Point::from([2.]), b); @@ -349,7 +357,7 @@ mod tests { let err = validate(shape.clone(), &ValidationConfig::default()).unwrap_err(); assert!(err.missing_surface(&surface.get())); - assert!(err.missing_cycle(&cycle.canonical().get())); + assert!(err.missing_cycle(&cycle.canonical())); let surface = shape.insert(Surface::xy_plane()); let cycle = diff --git a/crates/fj-kernel/src/validation/structural.rs b/crates/fj-kernel/src/validation/structural.rs index f6fe0ca4a..484ea418d 100644 --- a/crates/fj-kernel/src/validation/structural.rs +++ b/crates/fj-kernel/src/validation/structural.rs @@ -11,7 +11,7 @@ pub fn validate_edge( let mut missing_vertices = HashSet::new(); if !curves.contains(&edge.curve()) { - missing_curve = Some(edge.curve.canonical().get()); + missing_curve = Some(edge.curve.canonical()); } for vertex in edge.vertices().into_iter().flatten() { if !vertices.contains(&vertex) { diff --git a/crates/fj-operations/src/circle.rs b/crates/fj-operations/src/circle.rs index cebc6d7da..e036eff27 100644 --- a/crates/fj-operations/src/circle.rs +++ b/crates/fj-operations/src/circle.rs @@ -27,7 +27,7 @@ impl ToShape for fj::Circle { let cycle_local = Cycle { edges: vec![edge.clone()], }; - let cycle_canonical = tmp.insert(Cycle::new(vec![edge.canonical()])); + let cycle_canonical = Cycle::new(vec![edge.canonical()]); let surface = tmp.insert(Surface::xy_plane()); let face = tmp diff --git a/crates/fj-operations/src/difference_2d.rs b/crates/fj-operations/src/difference_2d.rs index 2b4dc09f4..af23f4b42 100644 --- a/crates/fj-operations/src/difference_2d.rs +++ b/crates/fj-operations/src/difference_2d.rs @@ -3,7 +3,7 @@ use fj_kernel::{ algorithms::Tolerance, iter::ObjectIters, objects::{Cycle, Edge, Face}, - shape::{LocalForm, Shape}, + shape::LocalForm, validation::{validate, Validated, ValidationConfig, ValidationError}, }; use fj_math::Aabb; @@ -95,8 +95,6 @@ fn add_cycle( cycle: LocalForm, Cycle<3>>, reverse: bool, ) -> LocalForm, Cycle<3>> { - let mut tmp = Shape::new(); - let mut edges = Vec::new(); for edge in cycle.local().edges.clone() { let curve_local = *edge.local().curve.local(); @@ -106,13 +104,12 @@ fn add_cycle( curve_local }; - let curve_canonical = edge.canonical().get().curve(); + let curve_canonical = edge.canonical().curve(); let curve_canonical = if reverse { curve_canonical.reverse() } else { curve_canonical }; - let curve_canonical = tmp.insert(curve_canonical); let vertices = if reverse { edge.local().vertices.clone().reverse() @@ -121,13 +118,13 @@ fn add_cycle( }; let edge_local = Edge { - curve: LocalForm::new(curve_local, curve_canonical.clone()), + curve: LocalForm::new(curve_local, curve_canonical), vertices: vertices.clone(), }; - let edge_canonical = tmp.merge(Edge { + let edge_canonical = Edge { curve: LocalForm::canonical_only(curve_canonical), vertices, - }); + }; edges.push(LocalForm::new(edge_local, edge_canonical)); } @@ -140,7 +137,7 @@ fn add_cycle( edges: edges.clone(), }; let cycle_canonical = - tmp.insert(Cycle::new(edges.into_iter().map(|edge| edge.canonical()))); + Cycle::new(edges.into_iter().map(|edge| edge.canonical())); LocalForm::new(cycle_local, cycle_canonical) } From f4f9ce2d89a765285c4c91aa8c730fba7aedd06d Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 28 Jun 2022 17:53:27 +0200 Subject: [PATCH 6/8] Replace manual implementations with `derive`s --- crates/fj-kernel/src/shape/local.rs | 25 +------------------------ 1 file changed, 1 insertion(+), 24 deletions(-) diff --git a/crates/fj-kernel/src/shape/local.rs b/crates/fj-kernel/src/shape/local.rs index ed13b9a89..e9df83444 100644 --- a/crates/fj-kernel/src/shape/local.rs +++ b/crates/fj-kernel/src/shape/local.rs @@ -1,5 +1,3 @@ -use std::hash::{Hash, Hasher}; - use super::Object; /// A reference to an object, which includes a local form @@ -14,7 +12,7 @@ use super::Object; /// terms that are useful to those objects. Two instances of `LocalForm` are /// equal, if both the local and the canonical forms are equal. The equality of /// the handle that refers to the canonical form is disregarded. -#[derive(Clone, Debug, Eq, Ord, PartialOrd)] +#[derive(Clone, Debug, Eq, PartialEq, Hash, Ord, PartialOrd)] pub struct LocalForm { local: Local, canonical: Canonical, @@ -50,24 +48,3 @@ impl LocalForm { Self::new(canonical.clone(), canonical) } } - -impl PartialEq for LocalForm -where - Local: PartialEq, - Canonical: PartialEq, -{ - fn eq(&self, other: &Self) -> bool { - self.local == other.local && self.canonical == other.canonical - } -} - -impl Hash for LocalForm -where - Local: Hash, - Canonical: Hash, -{ - fn hash(&self, state: &mut H) { - self.local.hash(state); - self.canonical.hash(state); - } -} From 62de46b5e267db203f5eac0922dbbfb6a46799f0 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 28 Jun 2022 17:53:51 +0200 Subject: [PATCH 7/8] Update doc comments --- crates/fj-kernel/src/objects/cycle.rs | 5 ----- crates/fj-kernel/src/objects/edge.rs | 5 ----- crates/fj-kernel/src/objects/vertex.rs | 5 ----- crates/fj-kernel/src/shape/local.rs | 7 ------- 4 files changed, 22 deletions(-) diff --git a/crates/fj-kernel/src/objects/cycle.rs b/crates/fj-kernel/src/objects/cycle.rs index 05da906ee..fe6c31ad3 100644 --- a/crates/fj-kernel/src/objects/cycle.rs +++ b/crates/fj-kernel/src/objects/cycle.rs @@ -11,11 +11,6 @@ use super::{Edge, Surface}; /// edge. The end of the last edge must connect to the beginning of the first /// one. /// -/// # Equality -/// -/// Please refer to [`crate::kernel::topology`] for documentation on the -/// equality of topological objects. -/// /// # Validation /// /// A cycle that is part of a [`Shape`] must be structurally sound. That means diff --git a/crates/fj-kernel/src/objects/edge.rs b/crates/fj-kernel/src/objects/edge.rs index 061e24962..9d62bfc10 100644 --- a/crates/fj-kernel/src/objects/edge.rs +++ b/crates/fj-kernel/src/objects/edge.rs @@ -11,11 +11,6 @@ use super::{Curve, Vertex}; /// An edge of a shape /// -/// # Equality -/// -/// Please refer to [`crate::kernel::topology`] for documentation on the -/// equality of topological objects. -/// /// # Validation /// /// An edge that is part of a [`Shape`] must be structurally sound. That means diff --git a/crates/fj-kernel/src/objects/vertex.rs b/crates/fj-kernel/src/objects/vertex.rs index 064987653..1f719a882 100644 --- a/crates/fj-kernel/src/objects/vertex.rs +++ b/crates/fj-kernel/src/objects/vertex.rs @@ -13,11 +13,6 @@ use crate::{builder::VertexBuilder, shape::Shape}; /// Points, on the other hand, might be used to approximate a shape for various /// purposes, without presenting any deeper truth about the shape's structure. /// -/// # Equality -/// -/// Please refer to [`crate::kernel::topology`] for documentation on the -/// equality of topological objects. -/// /// # Validation /// /// Vertices must be unique within a shape, meaning an identical vertex must not diff --git a/crates/fj-kernel/src/shape/local.rs b/crates/fj-kernel/src/shape/local.rs index e9df83444..2e6ec3159 100644 --- a/crates/fj-kernel/src/shape/local.rs +++ b/crates/fj-kernel/src/shape/local.rs @@ -5,13 +5,6 @@ use super::Object; /// This type is used by topological objects to reference other objects, while /// also keeping track of a local representation of that object, which is often /// more appropriate for various tasks. -/// -/// # Equality -/// -/// Since `LocalForm` is used by topological objects, its equality is defined in -/// terms that are useful to those objects. Two instances of `LocalForm` are -/// equal, if both the local and the canonical forms are equal. The equality of -/// the handle that refers to the canonical form is disregarded. #[derive(Clone, Debug, Eq, PartialEq, Hash, Ord, PartialOrd)] pub struct LocalForm { local: Local, From 8438f8a3c71b63bda09dab876cc5553969aded18 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 28 Jun 2022 17:59:18 +0200 Subject: [PATCH 8/8] Give `Face` ownership of its surface --- crates/fj-kernel/src/algorithms/sweep.rs | 12 +----------- crates/fj-kernel/src/algorithms/transform.rs | 6 ++---- .../fj-kernel/src/algorithms/triangulation/mod.rs | 2 +- crates/fj-kernel/src/builder.rs | 2 +- crates/fj-kernel/src/objects/face.rs | 8 ++++---- crates/fj-kernel/src/shape/api.rs | 3 ++- crates/fj-kernel/src/shape/object.rs | 7 +++++-- crates/fj-kernel/src/validation/mod.rs | 14 ++++++-------- crates/fj-kernel/src/validation/structural.rs | 2 +- crates/fj-operations/src/circle.rs | 2 +- crates/fj-operations/src/difference_2d.rs | 6 +++--- 11 files changed, 27 insertions(+), 37 deletions(-) diff --git a/crates/fj-kernel/src/algorithms/sweep.rs b/crates/fj-kernel/src/algorithms/sweep.rs index ddca4106b..3a56d057b 100644 --- a/crates/fj-kernel/src/algorithms/sweep.rs +++ b/crates/fj-kernel/src/algorithms/sweep.rs @@ -5,7 +5,7 @@ use crate::{ objects::{ Curve, Cycle, CyclesInFace, Edge, Face, Surface, Vertex, VerticesOfEdge, }, - shape::{LocalForm, Shape}, + shape::LocalForm, }; use super::{transform::transform_cycles, CycleApprox, Tolerance}; @@ -61,8 +61,6 @@ fn create_bottom_faces( is_sweep_along_negative_direction: bool, target: &mut Vec, ) { - let mut tmp = Shape::new(); - let mut surface = face.surface(); let mut exteriors = face.brep().exteriors.clone(); @@ -75,8 +73,6 @@ fn create_bottom_faces( interiors = reverse_local_coordinates_in_cycle(&interiors); }; - let surface = tmp.insert(surface); - let face = Face::new( surface, exteriors.as_local_form().cloned(), @@ -111,9 +107,6 @@ fn create_top_face( interiors = reverse_local_coordinates_in_cycle(&interiors); }; - let mut tmp = Shape::new(); - let surface = tmp.insert(surface); - let face = Face::new( surface, exteriors.as_local_form().cloned(), @@ -156,8 +149,6 @@ fn create_non_continuous_side_face( color: [u8; 4], target: &mut Vec, ) { - let mut tmp = Shape::new(); - let vertices = { let vertices_top = vertices_bottom.map(|vertex| { let point = vertex.point + path; @@ -177,7 +168,6 @@ fn create_non_continuous_side_face( let [a, b, _, c] = vertices.map(|vertex| vertex.point); Surface::plane_from_points([a, b, c]) }; - let surface = tmp.get_handle_or_insert(surface); let cycle = { let [a, b, c, d] = vertices; diff --git a/crates/fj-kernel/src/algorithms/transform.rs b/crates/fj-kernel/src/algorithms/transform.rs index 4b9bf019f..11e88e4f3 100644 --- a/crates/fj-kernel/src/algorithms/transform.rs +++ b/crates/fj-kernel/src/algorithms/transform.rs @@ -2,7 +2,7 @@ use fj_math::Transform; use crate::{ objects::{Cycle, CyclesInFace, Edge, Face, FaceBRep, Vertex}, - shape::{LocalForm, Shape}, + shape::LocalForm, }; /// Transform a shape @@ -12,9 +12,7 @@ pub fn transform(faces: &[Face], transform: &Transform) -> Vec { for face in faces { let face = match face { Face::Face(face) => { - let mut tmp = Shape::new(); - let surface = face.surface.get().transform(transform); - let surface = tmp.insert(surface); + let surface = face.surface.transform(transform); let exteriors = transform_cycles(&face.exteriors, transform); let interiors = transform_cycles(&face.interiors, transform); diff --git a/crates/fj-kernel/src/algorithms/triangulation/mod.rs b/crates/fj-kernel/src/algorithms/triangulation/mod.rs index f921233fe..cdede849a 100644 --- a/crates/fj-kernel/src/algorithms/triangulation/mod.rs +++ b/crates/fj-kernel/src/algorithms/triangulation/mod.rs @@ -22,7 +22,7 @@ pub fn triangulate( for face in faces { match &face { Face::Face(brep) => { - let surface = brep.surface.get(); + let surface = brep.surface; let approx = FaceApprox::new(&face, tolerance); let points: Vec<_> = approx diff --git a/crates/fj-kernel/src/builder.rs b/crates/fj-kernel/src/builder.rs index d1d5d24a2..e2dad7397 100644 --- a/crates/fj-kernel/src/builder.rs +++ b/crates/fj-kernel/src/builder.rs @@ -223,7 +223,7 @@ impl<'r> FaceBuilder<'r> { /// Build the face pub fn build(self) -> Handle { - let surface = self.shape.get_handle_or_insert(self.surface); + let surface = self.surface; let mut exteriors = Vec::new(); if let Some(points) = self.exterior { diff --git a/crates/fj-kernel/src/objects/face.rs b/crates/fj-kernel/src/objects/face.rs index a04652bce..acd462e66 100644 --- a/crates/fj-kernel/src/objects/face.rs +++ b/crates/fj-kernel/src/objects/face.rs @@ -5,7 +5,7 @@ use fj_math::Triangle; use crate::{ builder::FaceBuilder, - shape::{Handle, LocalForm, Shape}, + shape::{LocalForm, Shape}, }; use super::{Cycle, Surface}; @@ -41,7 +41,7 @@ pub enum Face { impl Face { /// Construct a new instance of `Face` pub fn new( - surface: Handle, + surface: Surface, exteriors: impl IntoIterator, Cycle<3>>>, interiors: impl IntoIterator, Cycle<3>>>, color: [u8; 4], @@ -119,7 +119,7 @@ impl Face { #[derive(Clone, Debug, Eq, Ord, PartialOrd)] pub struct FaceBRep { /// The surface that defines this face - pub surface: Handle, + pub surface: Surface, /// The cycles that bound the face on the outside /// @@ -152,7 +152,7 @@ impl FaceBRep { /// This is a convenience method that saves the caller from dealing with the /// [`Handle`]. pub fn surface(&self) -> Surface { - self.surface.get() + self.surface } /// Access the exterior cycles that the face refers to diff --git a/crates/fj-kernel/src/shape/api.rs b/crates/fj-kernel/src/shape/api.rs index cd8ea89c7..6e239f12d 100644 --- a/crates/fj-kernel/src/shape/api.rs +++ b/crates/fj-kernel/src/shape/api.rs @@ -196,7 +196,8 @@ mod tests { let cycle = shape.insert(cycle); assert!(shape.get_handle(&cycle.get()).as_ref() == Some(&cycle)); - let face = Face::new(surface, Vec::new(), Vec::new(), [0, 0, 0, 0]); + let face = + Face::new(surface.get(), Vec::new(), Vec::new(), [0, 0, 0, 0]); assert!(shape.get_handle(&face).is_none()); let face = shape.insert(face); diff --git a/crates/fj-kernel/src/shape/object.rs b/crates/fj-kernel/src/shape/object.rs index 8d438418c..54f7d0369 100644 --- a/crates/fj-kernel/src/shape/object.rs +++ b/crates/fj-kernel/src/shape/object.rs @@ -72,7 +72,7 @@ impl Object for Face { fn merge_into(self, shape: &mut Shape) -> Handle { match self { Face::Face(face) => { - let surface = face.surface.get().merge_into(shape); + let surface = face.surface.merge_into(shape); let mut exts = Vec::new(); for cycle in face.exteriors.as_local_form() { @@ -93,7 +93,10 @@ impl Object for Face { } shape.get_handle_or_insert(Face::new( - surface, exts, ints, face.color, + surface.get(), + exts, + ints, + face.color, )) } Face::Triangles(_) => shape.get_handle_or_insert(self), diff --git a/crates/fj-kernel/src/validation/mod.rs b/crates/fj-kernel/src/validation/mod.rs index ad27aa345..744a6e7ea 100644 --- a/crates/fj-kernel/src/validation/mod.rs +++ b/crates/fj-kernel/src/validation/mod.rs @@ -343,25 +343,23 @@ mod tests { let triangle = [[0., 0.], [1., 0.], [0., 1.]]; - let surface = other.insert(Surface::xy_plane()); - let cycle = - Cycle::builder(surface.get(), &mut other).build_polygon(triangle); + let surface = Surface::xy_plane(); + let cycle = Cycle::builder(surface, &mut other).build_polygon(triangle); // Nothing has been added to `shape`. Should fail. shape.insert(Face::new( - surface.clone(), + surface, vec![cycle.clone()], Vec::new(), [255, 0, 0, 255], )); let err = validate(shape.clone(), &ValidationConfig::default()).unwrap_err(); - assert!(err.missing_surface(&surface.get())); + assert!(err.missing_surface(&surface)); assert!(err.missing_cycle(&cycle.canonical())); - let surface = shape.insert(Surface::xy_plane()); - let cycle = - Cycle::builder(surface.get(), &mut shape).build_polygon(triangle); + let surface = Surface::xy_plane(); + let cycle = Cycle::builder(surface, &mut shape).build_polygon(triangle); // Everything has been added to `shape` now. Should work! shape.insert(Face::new( diff --git a/crates/fj-kernel/src/validation/structural.rs b/crates/fj-kernel/src/validation/structural.rs index 484ea418d..403c1dee0 100644 --- a/crates/fj-kernel/src/validation/structural.rs +++ b/crates/fj-kernel/src/validation/structural.rs @@ -61,7 +61,7 @@ pub fn validate_face( let mut missing_cycles = HashSet::new(); if !surfaces.contains(&face.surface()) { - missing_surface = Some(face.surface.get()); + missing_surface = Some(face.surface); } for cycle in face.all_cycles() { if !cycles.contains(&cycle) { diff --git a/crates/fj-operations/src/circle.rs b/crates/fj-operations/src/circle.rs index e036eff27..daba5ea25 100644 --- a/crates/fj-operations/src/circle.rs +++ b/crates/fj-operations/src/circle.rs @@ -29,7 +29,7 @@ impl ToShape for fj::Circle { }; let cycle_canonical = Cycle::new(vec![edge.canonical()]); - let surface = tmp.insert(Surface::xy_plane()); + let surface = Surface::xy_plane(); let face = tmp .insert(Face::new( surface, diff --git a/crates/fj-operations/src/difference_2d.rs b/crates/fj-operations/src/difference_2d.rs index af23f4b42..ef6784042 100644 --- a/crates/fj-operations/src/difference_2d.rs +++ b/crates/fj-operations/src/difference_2d.rs @@ -36,13 +36,13 @@ impl ToShape for fj::Difference2d { if let Some(face) = a.face_iter().next() { // If there's at least one face to subtract from, we can proceed. - let surface = face.brep().surface.clone(); + let surface = face.brep().surface; for face in a.face_iter() { let face = face.brep(); assert_eq!( - surface.get(), + surface, face.surface(), "Trying to subtract faces with different surfaces.", ); @@ -61,7 +61,7 @@ impl ToShape for fj::Difference2d { let face = face.brep(); assert_eq!( - surface.get(), + surface, face.surface(), "Trying to subtract faces with different surfaces.", );