From 23b7e8854d1d91e815e3e13493d686d05bc04a6d Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 8 Sep 2022 12:04:45 +0200 Subject: [PATCH 1/7] Move variables to where they are used --- crates/fj-kernel/src/algorithms/approx/face.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/fj-kernel/src/algorithms/approx/face.rs b/crates/fj-kernel/src/algorithms/approx/face.rs index 30b902a62..6286e1db6 100644 --- a/crates/fj-kernel/src/algorithms/approx/face.rs +++ b/crates/fj-kernel/src/algorithms/approx/face.rs @@ -76,12 +76,12 @@ impl Approx for &Face { // it have nothing to do with its curvature. let mut exteriors = Vec::new(); - let mut interiors = BTreeSet::new(); - for cycle in self.exteriors() { let cycle = cycle.approx(tolerance); exteriors.push(cycle); } + + let mut interiors = BTreeSet::new(); for cycle in self.interiors() { let cycle = cycle.approx(tolerance); interiors.insert(cycle); From 84e42829f28b49d9e2768c83fbcc42d45dd80b7c Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 8 Sep 2022 12:20:50 +0200 Subject: [PATCH 2/7] Fix function name --- crates/fj-kernel/src/algorithms/reverse/face.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/fj-kernel/src/algorithms/reverse/face.rs b/crates/fj-kernel/src/algorithms/reverse/face.rs index 75ec2ae0e..39547a162 100644 --- a/crates/fj-kernel/src/algorithms/reverse/face.rs +++ b/crates/fj-kernel/src/algorithms/reverse/face.rs @@ -10,8 +10,8 @@ impl Reverse for Face { fn reverse(self) -> Self { let surface = self.surface().reverse(); - let exteriors = reverse_local_coordinates_in_cycle(self.exteriors()); - let interiors = reverse_local_coordinates_in_cycle(self.interiors()); + let exteriors = reverse_local_coordinates_in_cycles(self.exteriors()); + let interiors = reverse_local_coordinates_in_cycles(self.interiors()); Face::new(surface) .with_exteriors(exteriors) @@ -33,7 +33,7 @@ impl Reverse for Face { /// concept in the kernel. We kind of treat `Edge` as a half-edge, but in an /// inconsistent way that causes problems. This issue has some context on that: /// -fn reverse_local_coordinates_in_cycle<'r>( +fn reverse_local_coordinates_in_cycles<'r>( cycles: impl IntoIterator + 'r, ) -> impl Iterator + 'r { cycles.into_iter().map(|cycle| { From cdac8d647f5b30daeda5fe691ef88b8203f7f8ae Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 8 Sep 2022 12:27:27 +0200 Subject: [PATCH 3/7] Refactor --- .../fj-kernel/src/algorithms/reverse/face.rs | 122 +++++++++--------- 1 file changed, 59 insertions(+), 63 deletions(-) diff --git a/crates/fj-kernel/src/algorithms/reverse/face.rs b/crates/fj-kernel/src/algorithms/reverse/face.rs index 39547a162..dd92724ba 100644 --- a/crates/fj-kernel/src/algorithms/reverse/face.rs +++ b/crates/fj-kernel/src/algorithms/reverse/face.rs @@ -20,6 +20,12 @@ impl Reverse for Face { } } +fn reverse_local_coordinates_in_cycles<'r>( + cycles: impl IntoIterator + 'r, +) -> impl Iterator + 'r { + cycles.into_iter().map(reverse_local_coordinates_in_cycle) +} + /// Reverse local coordinates within the cycle, leaving global ones as-is /// /// # Implementation Note @@ -33,76 +39,66 @@ impl Reverse for Face { /// concept in the kernel. We kind of treat `Edge` as a half-edge, but in an /// inconsistent way that causes problems. This issue has some context on that: /// -fn reverse_local_coordinates_in_cycles<'r>( - cycles: impl IntoIterator + 'r, -) -> impl Iterator + 'r { - cycles.into_iter().map(|cycle| { - let surface = cycle.surface().reverse(); - - let edges = cycle.edges().map(|edge| { - let curve = { - let local = match edge.curve().kind() { - CurveKind::Circle(circle) => { - let center = Point::from([ - circle.center().u, - -circle.center().v, - ]); - - let a = Vector::from([circle.a().u, -circle.a().v]); - let b = Vector::from([circle.b().u, -circle.b().v]); - - CurveKind::Circle(Circle::new(center, a, b)) - } - CurveKind::Line(line) => { - let origin = - Point::from([line.origin().u, -line.origin().v]); - let direction = Vector::from([ - line.direction().u, - -line.direction().v, - ]); - - CurveKind::Line(Line::from_origin_and_direction( - origin, direction, - )) - } - }; - - Curve::new( - edge.curve().surface().reverse(), - local, - *edge.curve().global_form(), - ) +fn reverse_local_coordinates_in_cycle(cycle: &Cycle) -> Cycle { + let surface = cycle.surface().reverse(); + + let edges = cycle.edges().map(|edge| { + let curve = { + let local = match edge.curve().kind() { + CurveKind::Circle(circle) => { + let center = + Point::from([circle.center().u, -circle.center().v]); + + let a = Vector::from([circle.a().u, -circle.a().v]); + let b = Vector::from([circle.b().u, -circle.b().v]); + + CurveKind::Circle(Circle::new(center, a, b)) + } + CurveKind::Line(line) => { + let origin = + Point::from([line.origin().u, -line.origin().v]); + let direction = + Vector::from([line.direction().u, -line.direction().v]); + + CurveKind::Line(Line::from_origin_and_direction( + origin, direction, + )) + } }; - let vertices = edge.vertices().map(|vertex| { - let surface_vertex = { - let vertex = vertex.surface_form(); - - let position = Point::from([ - vertex.position().u, - -vertex.position().v, - ]); - - SurfaceVertex::new( - position, - vertex.surface().reverse(), - *vertex.global_form(), - ) - }; - - Vertex::new( - vertex.position(), - curve, - surface_vertex, + Curve::new( + edge.curve().surface().reverse(), + local, + *edge.curve().global_form(), + ) + }; + + let vertices = edge.vertices().map(|vertex| { + let surface_vertex = { + let vertex = vertex.surface_form(); + + let position = + Point::from([vertex.position().u, -vertex.position().v]); + + SurfaceVertex::new( + position, + vertex.surface().reverse(), *vertex.global_form(), ) - }); + }; - Edge::from_curve_and_vertices(curve, vertices) + Vertex::new( + vertex.position(), + curve, + surface_vertex, + *vertex.global_form(), + ) }); - Cycle::new(surface, edges) - }) + Edge::from_curve_and_vertices(curve, vertices) + }); + + Cycle::new(surface, edges) } #[cfg(test)] From c0ddcb4e188f0ee7135c698e3f179f27f78fd395 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 8 Sep 2022 12:29:23 +0200 Subject: [PATCH 4/7] Refactor --- crates/fj-kernel/src/builder/face.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/crates/fj-kernel/src/builder/face.rs b/crates/fj-kernel/src/builder/face.rs index e68d61dc4..035e53c40 100644 --- a/crates/fj-kernel/src/builder/face.rs +++ b/crates/fj-kernel/src/builder/face.rs @@ -22,10 +22,8 @@ impl FaceBuilder { &self, points: impl IntoIterator>>, ) -> FacePolygon { - let face = Face::new(self.surface) - .with_exteriors([ - Cycle::build(self.surface).polygon_from_points(points) - ]); + let cycle = Cycle::build(self.surface).polygon_from_points(points); + let face = Face::new(self.surface).with_exteriors([cycle]); FacePolygon { face } } From c86baca64a8ce966a3f6141d24985bd2e8fecc0b Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 8 Sep 2022 12:40:52 +0200 Subject: [PATCH 5/7] Limit faces to one exterior That they could have multiple exteriors was a special case, when a face connected to itself. This is no longer possible, as faces are bound by edges on all sides now. --- .../fj-kernel/src/algorithms/approx/face.rs | 18 +-------- .../fj-kernel/src/algorithms/reverse/face.rs | 5 +-- crates/fj-kernel/src/algorithms/sweep/edge.rs | 2 +- crates/fj-kernel/src/algorithms/transform.rs | 5 +-- crates/fj-kernel/src/builder/face.rs | 2 +- crates/fj-kernel/src/objects/face.rs | 39 +++---------------- crates/fj-operations/src/difference_2d.rs | 28 +++++++++---- crates/fj-operations/src/sketch.rs | 4 +- 8 files changed, 34 insertions(+), 69 deletions(-) diff --git a/crates/fj-kernel/src/algorithms/approx/face.rs b/crates/fj-kernel/src/algorithms/approx/face.rs index 6286e1db6..872dfe4a7 100644 --- a/crates/fj-kernel/src/algorithms/approx/face.rs +++ b/crates/fj-kernel/src/algorithms/approx/face.rs @@ -75,11 +75,7 @@ impl Approx for &Face { // would need to provide its own approximation, as the edges that bound // it have nothing to do with its curvature. - let mut exteriors = Vec::new(); - for cycle in self.exteriors() { - let cycle = cycle.approx(tolerance); - exteriors.push(cycle); - } + let exterior = self.exterior().approx(tolerance); let mut interiors = BTreeSet::new(); for cycle in self.interiors() { @@ -87,18 +83,6 @@ impl Approx for &Face { interiors.insert(cycle); } - // Only polygons with exactly one exterior cycle are supported. - // - // See this issue for some background: - // https://github.com/hannobraun/Fornjot/issues/250 - let exterior = exteriors - .pop() - .expect("Can't approximate face without exterior cycle"); - assert!( - exteriors.is_empty(), - "Approximation only supports faces with one exterior cycle", - ); - FaceApprox { exterior, interiors, diff --git a/crates/fj-kernel/src/algorithms/reverse/face.rs b/crates/fj-kernel/src/algorithms/reverse/face.rs index dd92724ba..5aa38da79 100644 --- a/crates/fj-kernel/src/algorithms/reverse/face.rs +++ b/crates/fj-kernel/src/algorithms/reverse/face.rs @@ -10,11 +10,10 @@ impl Reverse for Face { fn reverse(self) -> Self { let surface = self.surface().reverse(); - let exteriors = reverse_local_coordinates_in_cycles(self.exteriors()); + let exterior = reverse_local_coordinates_in_cycle(self.exterior()); let interiors = reverse_local_coordinates_in_cycles(self.interiors()); - Face::new(surface) - .with_exteriors(exteriors) + Face::new(surface, exterior) .with_interiors(interiors) .with_color(self.color()) } diff --git a/crates/fj-kernel/src/algorithms/sweep/edge.rs b/crates/fj-kernel/src/algorithms/sweep/edge.rs index 9300729bc..0120f5681 100644 --- a/crates/fj-kernel/src/algorithms/sweep/edge.rs +++ b/crates/fj-kernel/src/algorithms/sweep/edge.rs @@ -177,6 +177,6 @@ impl Sweep for Edge { Cycle::new(surface, edges) }; - Face::new(surface).with_exteriors([cycle]).with_color(color) + Face::new(surface, cycle).with_color(color) } } diff --git a/crates/fj-kernel/src/algorithms/transform.rs b/crates/fj-kernel/src/algorithms/transform.rs index 46ca2a157..6bdbc8262 100644 --- a/crates/fj-kernel/src/algorithms/transform.rs +++ b/crates/fj-kernel/src/algorithms/transform.rs @@ -67,13 +67,12 @@ impl TransformObject for Face { fn transform(self, transform: &Transform) -> Self { let surface = self.surface().transform(transform); - let exteriors = transform_cycles(self.exteriors(), transform); + let exterior = self.exterior().clone().transform(transform); let interiors = transform_cycles(self.interiors(), transform); let color = self.color(); - Face::new(surface) - .with_exteriors(exteriors) + Face::new(surface, exterior) .with_interiors(interiors) .with_color(color) } diff --git a/crates/fj-kernel/src/builder/face.rs b/crates/fj-kernel/src/builder/face.rs index 035e53c40..1275e627a 100644 --- a/crates/fj-kernel/src/builder/face.rs +++ b/crates/fj-kernel/src/builder/face.rs @@ -23,7 +23,7 @@ impl FaceBuilder { points: impl IntoIterator>>, ) -> FacePolygon { let cycle = Cycle::build(self.surface).polygon_from_points(points); - let face = Face::new(self.surface).with_exteriors([cycle]); + let face = Face::new(self.surface, cycle); FacePolygon { face } } diff --git a/crates/fj-kernel/src/objects/face.rs b/crates/fj-kernel/src/objects/face.rs index e0ecec6b3..f57b5b77b 100644 --- a/crates/fj-kernel/src/objects/face.rs +++ b/crates/fj-kernel/src/objects/face.rs @@ -47,7 +47,7 @@ impl<'a> IntoIterator for &'a Faces { #[derive(Clone, Debug, Eq, PartialEq, Hash, Ord, PartialOrd)] pub struct Face { surface: Surface, - exteriors: Vec, + exterior: Cycle, interiors: Vec, color: Color, } @@ -62,39 +62,15 @@ impl Face { /// /// Creates the face with no exteriors, no interiors and the default color. /// This can be overridden using the `with_` methods. - pub fn new(surface: Surface) -> Self { + pub fn new(surface: Surface, exterior: Cycle) -> Self { Self { surface, - exteriors: Vec::new(), + exterior, interiors: Vec::new(), color: Color::default(), } } - /// Add exterior cycles to the face - /// - /// Consumes the face and returns the updated instance. - /// - /// # Panics - /// - /// Panics, if the added cycles are not defined in the face's surface. - pub fn with_exteriors( - mut self, - exteriors: impl IntoIterator, - ) -> Self { - for cycle in exteriors.into_iter() { - assert_eq!( - self.surface(), - cycle.surface(), - "Cycles that bound a face must be in face's surface" - ); - - self.exteriors.push(cycle); - } - - self - } - /// Add interior cycles to the face /// /// Consumes the face and returns the updated instance. @@ -133,8 +109,8 @@ impl Face { } /// Access the cycles that bound the face on the outside - pub fn exteriors(&self) -> impl Iterator + '_ { - self.exteriors.iter() + pub fn exterior(&self) -> &Cycle { + &self.exterior } /// Access the cycles that bound the face on the inside @@ -145,11 +121,8 @@ impl Face { } /// Access all cycles of this face - /// - /// This is equivalent to chaining the iterators returned by - /// [`Face::exteriors`] and [`Face::interiors`]. pub fn all_cycles(&self) -> impl Iterator + '_ { - self.exteriors().chain(self.interiors()) + [self.exterior()].into_iter().chain(self.interiors()) } /// Access the color of the face diff --git a/crates/fj-operations/src/difference_2d.rs b/crates/fj-operations/src/difference_2d.rs index b0ec6b769..dc8c7e400 100644 --- a/crates/fj-operations/src/difference_2d.rs +++ b/crates/fj-operations/src/difference_2d.rs @@ -49,9 +49,7 @@ impl Shape for fj::Difference2d { "Trying to subtract faces with different surfaces.", ); - for cycle in face.exteriors() { - exteriors.push(cycle.clone()); - } + exteriors.push(face.exterior().clone()); for cycle in face.interiors() { interiors.push(cycle.clone().reverse()); } @@ -64,14 +62,28 @@ impl Shape for fj::Difference2d { "Trying to subtract faces with different surfaces.", ); - for cycle in face.exteriors() { - interiors.push(cycle.clone().reverse()); - } + interiors.push(face.exterior().clone().reverse()); } + // Faces only support one exterior, while the code here comes from + // the time when a face could have multiple exteriors. This was only + // a special case, i.e. faces that connected to themselves, and I + // have my doubts that this code was ever correct in the first + // place. + // + // Anyway, the following should make sure that at least any problems + // this code causes become obvious. I don't know if this can ever + // trigger, but better safe than sorry. + let exterior = exteriors + .pop() + .expect("Can't construct face without an exterior"); + assert!( + exteriors.is_empty(), + "Can't construct face with multiple exteriors" + ); + faces.push( - Face::new(*surface) - .with_exteriors(exteriors) + Face::new(*surface, exterior) .with_interiors(interiors) .with_color(Color(self.color())), ); diff --git a/crates/fj-operations/src/sketch.rs b/crates/fj-operations/src/sketch.rs index 39162105b..edbc908f5 100644 --- a/crates/fj-operations/src/sketch.rs +++ b/crates/fj-operations/src/sketch.rs @@ -30,9 +30,7 @@ impl Shape for fj::Sketch { .circle_from_radius(Scalar::from_f64(circle.radius())); let cycle = Cycle::new(surface, [edge]); - Face::new(surface) - .with_exteriors([cycle]) - .with_color(Color(self.color())) + Face::new(surface, cycle).with_color(Color(self.color())) } fj::Chain::PolyChain(poly_chain) => { let points = From 4151e2334d12eeb70adbc14b18f8a81c840e4752 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 8 Sep 2022 12:42:03 +0200 Subject: [PATCH 6/7] Update doc comment --- crates/fj-kernel/src/objects/face.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/fj-kernel/src/objects/face.rs b/crates/fj-kernel/src/objects/face.rs index f57b5b77b..a6b41e2d7 100644 --- a/crates/fj-kernel/src/objects/face.rs +++ b/crates/fj-kernel/src/objects/face.rs @@ -60,8 +60,8 @@ impl Face { /// Construct a new instance of `Face` /// - /// Creates the face with no exteriors, no interiors and the default color. - /// This can be overridden using the `with_` methods. + /// Creates the face with no interiors and the default color. This can be + /// overridden using the `with_` methods. pub fn new(surface: Surface, exterior: Cycle) -> Self { Self { surface, From 8521b7616593f0e50d1f3d553f7f8a4acbcff420 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 8 Sep 2022 12:43:01 +0200 Subject: [PATCH 7/7] Inline function --- crates/fj-kernel/src/algorithms/transform.rs | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/crates/fj-kernel/src/algorithms/transform.rs b/crates/fj-kernel/src/algorithms/transform.rs index 6bdbc8262..fe66dd1e3 100644 --- a/crates/fj-kernel/src/algorithms/transform.rs +++ b/crates/fj-kernel/src/algorithms/transform.rs @@ -68,7 +68,9 @@ impl TransformObject for Face { let surface = self.surface().transform(transform); let exterior = self.exterior().clone().transform(transform); - let interiors = transform_cycles(self.interiors(), transform); + let interiors = self + .interiors() + .map(|cycle| cycle.clone().transform(transform)); let color = self.color(); @@ -157,12 +159,3 @@ impl TransformObject for Vertex { ) } } - -fn transform_cycles<'a>( - cycles: impl IntoIterator + 'a, - transform: &'a Transform, -) -> impl Iterator + 'a { - cycles - .into_iter() - .map(|cycle| cycle.clone().transform(transform)) -}