From 5e69bdeffd5e81e26322d8892cb9b2c3e7d9cd95 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Mon, 13 Jun 2022 16:08:47 +0200 Subject: [PATCH 1/3] Update order of `impl` blocks Other code has the generic `impl` block first, then followed by specific ones. This change thus improves consistency. --- crates/fj-kernel/src/topology/edge.rs | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/crates/fj-kernel/src/topology/edge.rs b/crates/fj-kernel/src/topology/edge.rs index 153a079a3..d3ca3800d 100644 --- a/crates/fj-kernel/src/topology/edge.rs +++ b/crates/fj-kernel/src/topology/edge.rs @@ -37,19 +37,6 @@ pub struct Edge { pub vertices: VerticesOfEdge, } -impl Edge<3> { - /// Construct an instance of `Edge` - pub fn new(curve: Handle>, vertices: VerticesOfEdge) -> Self { - let curve = LocalForm::canonical_only(curve); - Self { curve, vertices } - } - - /// Build an edge using the [`EdgeBuilder`] API - pub fn builder(shape: &mut Shape) -> EdgeBuilder { - EdgeBuilder::new(shape) - } -} - impl Edge { /// Access the curve that the edge refers to /// @@ -71,6 +58,19 @@ impl Edge { } } +impl Edge<3> { + /// Construct an instance of `Edge` + pub fn new(curve: Handle>, vertices: VerticesOfEdge) -> Self { + let curve = LocalForm::canonical_only(curve); + Self { curve, vertices } + } + + /// Build an edge using the [`EdgeBuilder`] API + pub fn builder(shape: &mut Shape) -> EdgeBuilder { + EdgeBuilder::new(shape) + } +} + impl fmt::Display for Edge { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self.vertices() { From a723d38893e2b91adeb967fc8934a3123cea2e9b Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Mon, 13 Jun 2022 16:19:16 +0200 Subject: [PATCH 2/3] Make `Edge::new` generic --- crates/fj-kernel/src/shape/api.rs | 11 +++++++---- crates/fj-kernel/src/shape/object.rs | 2 +- crates/fj-kernel/src/topology/builder.rs | 9 +++++---- crates/fj-kernel/src/topology/edge.rs | 16 +++++++++------- crates/fj-operations/src/difference_2d.rs | 6 ++++-- 5 files changed, 26 insertions(+), 18 deletions(-) diff --git a/crates/fj-kernel/src/shape/api.rs b/crates/fj-kernel/src/shape/api.rs index 2608577fd..c5c5f16ba 100644 --- a/crates/fj-kernel/src/shape/api.rs +++ b/crates/fj-kernel/src/shape/api.rs @@ -319,7 +319,8 @@ mod tests { assert!(shape.get_handle(&surface.get()).as_ref() == Some(&surface)); let vertex = Vertex { point }; - let edge = Edge::new(curve, VerticesOfEdge::none()); + let edge = + Edge::new(LocalForm::canonical_only(curve), VerticesOfEdge::none()); assert!(shape.get_handle(&vertex).is_none()); assert!(shape.get_handle(&edge).is_none()); @@ -376,7 +377,7 @@ mod tests { // Shouldn't work. Nothing has been added to `shape`. let err = shape .insert(Edge::new( - curve.clone(), + LocalForm::canonical_only(curve.clone()), VerticesOfEdge::from_vertices([a.clone(), b.clone()]), )) .unwrap_err(); @@ -392,8 +393,10 @@ mod tests { let b = LocalForm::new(Point::from([2.]), b); // Everything has been added to `shape` now. Should work! - shape - .insert(Edge::new(curve, VerticesOfEdge::from_vertices([a, b])))?; + shape.insert(Edge::new( + LocalForm::canonical_only(curve), + VerticesOfEdge::from_vertices([a, b]), + ))?; Ok(()) } diff --git a/crates/fj-kernel/src/shape/object.rs b/crates/fj-kernel/src/shape/object.rs index 265f7f416..4e72844ba 100644 --- a/crates/fj-kernel/src/shape/object.rs +++ b/crates/fj-kernel/src/shape/object.rs @@ -109,7 +109,7 @@ impl Object for Edge<3> { })?; let merged = shape.get_handle_or_insert(Edge::new( - curve, + LocalForm::canonical_only(curve), VerticesOfEdge::new(vertices), ))?; diff --git a/crates/fj-kernel/src/topology/builder.rs b/crates/fj-kernel/src/topology/builder.rs index 09b6d5bf8..2bb79d27a 100644 --- a/crates/fj-kernel/src/topology/builder.rs +++ b/crates/fj-kernel/src/topology/builder.rs @@ -66,9 +66,10 @@ impl<'r> EdgeBuilder<'r> { curve: LocalForm::new(curve_local, curve_canonical.clone()), vertices: VerticesOfEdge::none(), }; - let edge_canonical = self - .shape - .insert(Edge::new(curve_canonical, VerticesOfEdge::none()))?; + let edge_canonical = self.shape.insert(Edge::new( + LocalForm::canonical_only(curve_canonical), + VerticesOfEdge::none(), + ))?; Ok(LocalForm::new(edge_local, edge_canonical)) } @@ -109,7 +110,7 @@ impl<'r> EdgeBuilder<'r> { ]; let edge = self.shape.insert(Edge::new( - curve, + LocalForm::canonical_only(curve), VerticesOfEdge::from_vertices(vertices), ))?; diff --git a/crates/fj-kernel/src/topology/edge.rs b/crates/fj-kernel/src/topology/edge.rs index d3ca3800d..f2dd8e08d 100644 --- a/crates/fj-kernel/src/topology/edge.rs +++ b/crates/fj-kernel/src/topology/edge.rs @@ -4,7 +4,7 @@ use fj_math::Point; use crate::{ geometry::Curve, - shape::{Handle, LocalForm, Shape}, + shape::{LocalForm, Shape}, }; use super::{EdgeBuilder, Vertex}; @@ -38,6 +38,14 @@ pub struct Edge { } impl Edge { + /// Construct an instance of `Edge` + pub fn new( + curve: LocalForm, Curve<3>>, + vertices: VerticesOfEdge, + ) -> Self { + Self { curve, vertices } + } + /// Access the curve that the edge refers to /// /// This is a convenience method that saves the caller from dealing with the @@ -59,12 +67,6 @@ impl Edge { } impl Edge<3> { - /// Construct an instance of `Edge` - pub fn new(curve: Handle>, vertices: VerticesOfEdge) -> Self { - let curve = LocalForm::canonical_only(curve); - Self { curve, vertices } - } - /// Build an edge using the [`EdgeBuilder`] API pub fn builder(shape: &mut Shape) -> EdgeBuilder { EdgeBuilder::new(shape) diff --git a/crates/fj-operations/src/difference_2d.rs b/crates/fj-operations/src/difference_2d.rs index a134441c1..cc7d8e37e 100644 --- a/crates/fj-operations/src/difference_2d.rs +++ b/crates/fj-operations/src/difference_2d.rs @@ -121,8 +121,10 @@ fn add_cycle( curve: LocalForm::new(curve_local, curve_canonical.clone()), vertices: vertices.clone(), }; - let edge_canonical = - shape.merge(Edge::new(curve_canonical, vertices))?; + let edge_canonical = shape.merge(Edge::new( + LocalForm::canonical_only(curve_canonical), + vertices, + ))?; edges.push(LocalForm::new(edge_local, edge_canonical)); } From a4c6a71f3d836ceb0db7bf369a1804ce8d79da89 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Mon, 13 Jun 2022 16:21:36 +0200 Subject: [PATCH 3/3] Remove redundant constructor --- crates/fj-kernel/src/shape/api.rs | 22 ++++++++++++---------- crates/fj-kernel/src/shape/object.rs | 8 ++++---- crates/fj-kernel/src/topology/builder.rs | 16 ++++++++-------- crates/fj-kernel/src/topology/edge.rs | 8 -------- crates/fj-operations/src/difference_2d.rs | 6 +++--- 5 files changed, 27 insertions(+), 33 deletions(-) diff --git a/crates/fj-kernel/src/shape/api.rs b/crates/fj-kernel/src/shape/api.rs index c5c5f16ba..eb02f3242 100644 --- a/crates/fj-kernel/src/shape/api.rs +++ b/crates/fj-kernel/src/shape/api.rs @@ -319,8 +319,10 @@ mod tests { assert!(shape.get_handle(&surface.get()).as_ref() == Some(&surface)); let vertex = Vertex { point }; - let edge = - Edge::new(LocalForm::canonical_only(curve), VerticesOfEdge::none()); + let edge = Edge { + curve: LocalForm::canonical_only(curve), + vertices: VerticesOfEdge::none(), + }; assert!(shape.get_handle(&vertex).is_none()); assert!(shape.get_handle(&edge).is_none()); @@ -376,10 +378,10 @@ mod tests { // Shouldn't work. Nothing has been added to `shape`. let err = shape - .insert(Edge::new( - LocalForm::canonical_only(curve.clone()), - VerticesOfEdge::from_vertices([a.clone(), b.clone()]), - )) + .insert(Edge { + curve: LocalForm::canonical_only(curve.clone()), + vertices: VerticesOfEdge::from_vertices([a.clone(), b.clone()]), + }) .unwrap_err(); assert!(err.missing_curve(&curve)); assert!(err.missing_vertex(&a.canonical())); @@ -393,10 +395,10 @@ mod tests { let b = LocalForm::new(Point::from([2.]), b); // Everything has been added to `shape` now. Should work! - shape.insert(Edge::new( - LocalForm::canonical_only(curve), - VerticesOfEdge::from_vertices([a, b]), - ))?; + shape.insert(Edge { + curve: LocalForm::canonical_only(curve), + vertices: VerticesOfEdge::from_vertices([a, b]), + })?; Ok(()) } diff --git a/crates/fj-kernel/src/shape/object.rs b/crates/fj-kernel/src/shape/object.rs index 4e72844ba..29142939f 100644 --- a/crates/fj-kernel/src/shape/object.rs +++ b/crates/fj-kernel/src/shape/object.rs @@ -108,10 +108,10 @@ impl Object for Edge<3> { Ok(LocalForm::new(*vertex.local(), canonical)) })?; - let merged = shape.get_handle_or_insert(Edge::new( - LocalForm::canonical_only(curve), - VerticesOfEdge::new(vertices), - ))?; + let merged = shape.get_handle_or_insert(Edge { + curve: LocalForm::canonical_only(curve), + vertices: VerticesOfEdge::new(vertices), + })?; if let Some(handle) = handle { mapping.edges.insert(handle, merged.clone()); diff --git a/crates/fj-kernel/src/topology/builder.rs b/crates/fj-kernel/src/topology/builder.rs index 2bb79d27a..d52832f31 100644 --- a/crates/fj-kernel/src/topology/builder.rs +++ b/crates/fj-kernel/src/topology/builder.rs @@ -66,10 +66,10 @@ impl<'r> EdgeBuilder<'r> { curve: LocalForm::new(curve_local, curve_canonical.clone()), vertices: VerticesOfEdge::none(), }; - let edge_canonical = self.shape.insert(Edge::new( - LocalForm::canonical_only(curve_canonical), - VerticesOfEdge::none(), - ))?; + let edge_canonical = self.shape.insert(Edge { + curve: LocalForm::canonical_only(curve_canonical), + vertices: VerticesOfEdge::none(), + })?; Ok(LocalForm::new(edge_local, edge_canonical)) } @@ -109,10 +109,10 @@ impl<'r> EdgeBuilder<'r> { LocalForm::new(Point::from([1.]), b), ]; - let edge = self.shape.insert(Edge::new( - LocalForm::canonical_only(curve), - VerticesOfEdge::from_vertices(vertices), - ))?; + let edge = self.shape.insert(Edge { + curve: LocalForm::canonical_only(curve), + vertices: VerticesOfEdge::from_vertices(vertices), + })?; Ok(edge) } diff --git a/crates/fj-kernel/src/topology/edge.rs b/crates/fj-kernel/src/topology/edge.rs index f2dd8e08d..cf0446af8 100644 --- a/crates/fj-kernel/src/topology/edge.rs +++ b/crates/fj-kernel/src/topology/edge.rs @@ -38,14 +38,6 @@ pub struct Edge { } impl Edge { - /// Construct an instance of `Edge` - pub fn new( - curve: LocalForm, Curve<3>>, - vertices: VerticesOfEdge, - ) -> Self { - Self { curve, vertices } - } - /// Access the curve that the edge refers to /// /// This is a convenience method that saves the caller from dealing with the diff --git a/crates/fj-operations/src/difference_2d.rs b/crates/fj-operations/src/difference_2d.rs index cc7d8e37e..5a36d1120 100644 --- a/crates/fj-operations/src/difference_2d.rs +++ b/crates/fj-operations/src/difference_2d.rs @@ -121,10 +121,10 @@ fn add_cycle( curve: LocalForm::new(curve_local, curve_canonical.clone()), vertices: vertices.clone(), }; - let edge_canonical = shape.merge(Edge::new( - LocalForm::canonical_only(curve_canonical), + let edge_canonical = shape.merge(Edge { + curve: LocalForm::canonical_only(curve_canonical), vertices, - ))?; + })?; edges.push(LocalForm::new(edge_local, edge_canonical)); }