From 8f83793a2cf0c68ef21b8df41dafaf567bbc0de9 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 1 Sep 2022 14:58:51 +0200 Subject: [PATCH 1/3] Require edges on cycle construction It doesn't make a whole lot of sense to add edges after cycle construction, as either before or after the `with_edges` call, the cycle would not be valid. In fact, none of the call sites made use of this capability. --- .../fj-kernel/src/algorithms/reverse/cycle.rs | 2 +- .../fj-kernel/src/algorithms/reverse/face.rs | 2 +- crates/fj-kernel/src/algorithms/sweep/edge.rs | 4 ++-- crates/fj-kernel/src/algorithms/transform.rs | 6 ++++-- crates/fj-kernel/src/builder/cycle.rs | 2 +- crates/fj-kernel/src/objects/cycle.rs | 19 ++++++------------- crates/fj-operations/src/sketch.rs | 2 +- 7 files changed, 16 insertions(+), 21 deletions(-) diff --git a/crates/fj-kernel/src/algorithms/reverse/cycle.rs b/crates/fj-kernel/src/algorithms/reverse/cycle.rs index db73de4bb..7ec00c5bc 100644 --- a/crates/fj-kernel/src/algorithms/reverse/cycle.rs +++ b/crates/fj-kernel/src/algorithms/reverse/cycle.rs @@ -13,6 +13,6 @@ impl Reverse for Cycle { edges.reverse(); - Cycle::new(surface).with_edges(edges) + Cycle::new(surface, edges) } } diff --git a/crates/fj-kernel/src/algorithms/reverse/face.rs b/crates/fj-kernel/src/algorithms/reverse/face.rs index fe3701648..24a2d7462 100644 --- a/crates/fj-kernel/src/algorithms/reverse/face.rs +++ b/crates/fj-kernel/src/algorithms/reverse/face.rs @@ -83,7 +83,7 @@ fn reverse_local_coordinates_in_cycle<'r>( Edge::from_curve_and_vertices(curve, vertices) }); - Cycle::new(surface).with_edges(edges) + Cycle::new(surface, edges) }) } diff --git a/crates/fj-kernel/src/algorithms/sweep/edge.rs b/crates/fj-kernel/src/algorithms/sweep/edge.rs index 25007faa8..e12604e78 100644 --- a/crates/fj-kernel/src/algorithms/sweep/edge.rs +++ b/crates/fj-kernel/src/algorithms/sweep/edge.rs @@ -98,7 +98,7 @@ fn create_non_continuous_side_face( edges.push(edge); } - Cycle::new(surface).with_edges(edges) + Cycle::new(surface, edges) }; Face::new(surface).with_exteriors([cycle]).with_color(color) @@ -118,7 +118,7 @@ fn create_continuous_side_face( // won't start to matter at some point either. let placeholder = Surface::xy_plane(); - let cycle = Cycle::new(placeholder).with_edges([edge]); + let cycle = Cycle::new(placeholder, [edge]); let approx = cycle.approx(tolerance, ()); let mut quads = Vec::new(); diff --git a/crates/fj-kernel/src/algorithms/transform.rs b/crates/fj-kernel/src/algorithms/transform.rs index ebc78c870..52f1828c6 100644 --- a/crates/fj-kernel/src/algorithms/transform.rs +++ b/crates/fj-kernel/src/algorithms/transform.rs @@ -46,8 +46,10 @@ impl TransformObject for Curve { impl TransformObject for Cycle { fn transform(self, transform: &Transform) -> Self { - Self::new(self.surface().transform(transform)) - .with_edges(self.into_edges().map(|edge| edge.transform(transform))) + Self::new( + self.surface().transform(transform), + self.into_edges().map(|edge| edge.transform(transform)), + ) } } diff --git a/crates/fj-kernel/src/builder/cycle.rs b/crates/fj-kernel/src/builder/cycle.rs index 721da597b..89c0f1e7d 100644 --- a/crates/fj-kernel/src/builder/cycle.rs +++ b/crates/fj-kernel/src/builder/cycle.rs @@ -40,6 +40,6 @@ impl CycleBuilder { ); } - Cycle::new(self.surface).with_edges(edges) + Cycle::new(self.surface, edges) } } diff --git a/crates/fj-kernel/src/objects/cycle.rs b/crates/fj-kernel/src/objects/cycle.rs index 34e8e4a83..59ce3b557 100644 --- a/crates/fj-kernel/src/objects/cycle.rs +++ b/crates/fj-kernel/src/objects/cycle.rs @@ -20,19 +20,12 @@ impl Cycle { } /// Create a new cycle - pub fn new(surface: Surface) -> Self { - Self { - surface, - edges: Vec::new(), - } - } - - /// Add edges to the cycle - /// - /// Consumes the cycle and returns the updated instance. - pub fn with_edges(mut self, edges: impl IntoIterator) -> Self { - self.edges.extend(edges); - self + pub fn new( + surface: Surface, + edges: impl IntoIterator, + ) -> Self { + let edges = edges.into_iter().collect(); + Self { surface, edges } } /// Access the surface that this cycle is in diff --git a/crates/fj-operations/src/sketch.rs b/crates/fj-operations/src/sketch.rs index a976812bd..43af4f639 100644 --- a/crates/fj-operations/src/sketch.rs +++ b/crates/fj-operations/src/sketch.rs @@ -26,7 +26,7 @@ impl Shape for fj::Sketch { let edge = Edge::build(surface) .circle_from_radius(Scalar::from_f64(circle.radius())); - let cycle = Cycle::new(surface).with_edges([edge]); + let cycle = Cycle::new(surface, [edge]); Face::new(surface) .with_exteriors([cycle]) From 7641fecea70bad0296bbde32b2886ba94a48b95e Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 1 Sep 2022 15:07:45 +0200 Subject: [PATCH 2/3] Validate `Cycle` on construction --- crates/fj-kernel/src/objects/cycle.rs | 42 ++++++++++++++++++++++++++- 1 file changed, 41 insertions(+), 1 deletion(-) diff --git a/crates/fj-kernel/src/objects/cycle.rs b/crates/fj-kernel/src/objects/cycle.rs index 59ce3b557..9f987956e 100644 --- a/crates/fj-kernel/src/objects/cycle.rs +++ b/crates/fj-kernel/src/objects/cycle.rs @@ -24,7 +24,47 @@ impl Cycle { surface: Surface, edges: impl IntoIterator, ) -> Self { - let edges = edges.into_iter().collect(); + let edges = edges.into_iter().collect::>(); + + if edges.len() != 1 { + // If the length is one, we might have a cycle made up of just one + // circle. If that isn't the case, we are dealing with line segments + // and can be sure that the following `get_or_panic` calls won't + // panic. + + // Verify that all edges connect. + for edges in edges.windows(2) { + // Can't panic, as we passed `2` to `windows`. + // + // Can be cleaned up, once `array_windows` is stable" + // https://doc.rust-lang.org/std/primitive.slice.html#method.array_windows + let [a, b] = [&edges[0], &edges[1]]; + + let [_, prev] = a.vertices().get_or_panic(); + let [next, _] = b.vertices().get_or_panic(); + + assert_eq!( + prev.global(), + next.global(), + "Edges in cycle do not connect" + ); + } + + // Verify that the edges form a cycle + if let Some(first) = edges.first() { + if let Some(last) = edges.last() { + let [first, _] = first.vertices().get_or_panic(); + let [_, last] = last.vertices().get_or_panic(); + + assert_eq!( + first.global(), + last.global(), + "Edges do not form a cycle" + ); + } + } + } + Self { surface, edges } } From 53cfcb6ec84a3eeba676fccbe40c415e4ae23969 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 1 Sep 2022 15:08:51 +0200 Subject: [PATCH 3/3] Update doc comments --- crates/fj-kernel/src/objects/cycle.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/crates/fj-kernel/src/objects/cycle.rs b/crates/fj-kernel/src/objects/cycle.rs index 9f987956e..db453dde7 100644 --- a/crates/fj-kernel/src/objects/cycle.rs +++ b/crates/fj-kernel/src/objects/cycle.rs @@ -3,10 +3,6 @@ use crate::builder::CycleBuilder; use super::{Edge, Surface}; /// A cycle of connected edges -/// -/// The end of each edge in the cycle must connect to the beginning of the next -/// edge. The end of the last edge must connect to the beginning of the first -/// one. #[derive(Clone, Debug, Eq, PartialEq, Hash, Ord, PartialOrd)] pub struct Cycle { surface: Surface, @@ -20,6 +16,11 @@ impl Cycle { } /// Create a new cycle + /// + /// # Panics + /// + /// Panic, if the end of each edge does not connect to the beginning of the + /// next edge. pub fn new( surface: Surface, edges: impl IntoIterator,