From ea11c23b6f80dac0b54019b94e02569d14381daa Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 8 Mar 2023 13:20:21 +0100 Subject: [PATCH 1/7] Remove redundant code --- crates/fj-kernel/src/builder/edge.rs | 79 ++-------------------------- 1 file changed, 3 insertions(+), 76 deletions(-) diff --git a/crates/fj-kernel/src/builder/edge.rs b/crates/fj-kernel/src/builder/edge.rs index 4b7febb579..26e2f92cc0 100644 --- a/crates/fj-kernel/src/builder/edge.rs +++ b/crates/fj-kernel/src/builder/edge.rs @@ -2,10 +2,7 @@ use fj_interop::ext::ArrayExt; use fj_math::{Point, Scalar}; use crate::{ - geometry::{ - curve::{Curve, GlobalPath}, - surface::SurfaceGeometry, - }, + geometry::{curve::Curve, surface::SurfaceGeometry}, objects::HalfEdge, partial::{Partial, PartialHalfEdge}, }; @@ -133,80 +130,10 @@ impl HalfEdgeBuilder for PartialHalfEdge { fn update_from_other_edge( &mut self, - other: &Partial, + _: &Partial, other_prev: &Partial, - surface: &SurfaceGeometry, + _: &SurfaceGeometry, ) { - self.curve = other.read().curve.as_ref().and_then(|path| { - // We have information about the other edge's surface available. We - // need to use that to interpret what the other edge's curve path - // means for our curve path. - match surface.u { - GlobalPath::Circle(_) => { - // The other surface is curved. We're entering some dodgy - // territory here, as only some edge cases can be - // represented using our current curve/surface - // representation. - match path { - Curve::Line(_) => { - // We're dealing with a line on a rounded surface. - // - // Based on the current uses of this method, we can - // make some assumptions: - // - // 1. The line is parallel to the u-axis of the - // other surface. - // 2. The surface that *our* edge is in is a plane - // that is parallel to the the plane of the - // circle that defines the curvature of the other - // surface. - // - // These assumptions are necessary preconditions for - // the following code to work. But unfortunately, I - // see no way to check those preconditions here, as - // neither the other line nor our surface is - // necessarily defined yet. - // - // Handling this case anyway feels like a grave sin, - // but I don't know what else to do. If you tracked - // some extremely subtle and annoying bug back to - // this code, I apologize. - // - // I hope that I'll come up with a better curve/ - // surface representation before this becomes a - // problem. - None - } - _ => { - // The other edge is a line segment in a curved - // surface. No idea how to deal with this. - todo!( - "Can't connect edge to circle on curved \ - surface" - ) - } - } - } - GlobalPath::Line(_) => { - // The other edge is defined on a plane. - match path { - Curve::Line(_) => { - // The other edge is a line segment on a plane. That - // means our edge must be a line segment too. - None - } - _ => { - // The other edge is a circle or arc on a plane. I'm - // actually not sure what that means for our edge. - // We might be able to represent it somehow, but - // let's leave that as an exercise for later. - todo!("Can't connect edge to circle on plane") - } - } - } - } - }); - self.start_vertex = other_prev.read().start_vertex.clone(); } } From 0af39b62530004692976f57bc42cc9b6fa1b7991 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 8 Mar 2023 13:22:08 +0100 Subject: [PATCH 2/7] Remove unused trait method arguments --- crates/fj-kernel/src/builder/cycle.rs | 6 +++--- crates/fj-kernel/src/builder/edge.rs | 16 +++------------- 2 files changed, 6 insertions(+), 16 deletions(-) diff --git a/crates/fj-kernel/src/builder/cycle.rs b/crates/fj-kernel/src/builder/cycle.rs index 388a27a322..b281403b6b 100644 --- a/crates/fj-kernel/src/builder/cycle.rs +++ b/crates/fj-kernel/src/builder/cycle.rs @@ -91,15 +91,15 @@ impl CycleBuilder for PartialCycle { fn connect_to_closed_edges( &mut self, edges: O, - surface: &SurfaceGeometry, + _: &SurfaceGeometry, objects: &mut Service, ) -> O::SameSize> where O: ObjectArgument>, { - edges.map_with_prev(|other, prev| { + edges.map_with_prev(|_, prev| { let mut this = self.add_half_edge(objects); - this.write().update_from_other_edge(&other, &prev, surface); + this.write().update_from_other_edge(&prev); this }) } diff --git a/crates/fj-kernel/src/builder/edge.rs b/crates/fj-kernel/src/builder/edge.rs index 26e2f92cc0..0295abb0c5 100644 --- a/crates/fj-kernel/src/builder/edge.rs +++ b/crates/fj-kernel/src/builder/edge.rs @@ -2,7 +2,7 @@ use fj_interop::ext::ArrayExt; use fj_math::{Point, Scalar}; use crate::{ - geometry::{curve::Curve, surface::SurfaceGeometry}, + geometry::curve::Curve, objects::HalfEdge, partial::{Partial, PartialHalfEdge}, }; @@ -44,12 +44,7 @@ pub trait HalfEdgeBuilder { /// under various circumstances. As long as you're only dealing with lines /// and planes, you should be fine. Otherwise, please read the code of this /// method carefully, to make sure you don't run into trouble. - fn update_from_other_edge( - &mut self, - other: &Partial, - other_prev: &Partial, - surface: &SurfaceGeometry, - ); + fn update_from_other_edge(&mut self, other_prev: &Partial); } impl HalfEdgeBuilder for PartialHalfEdge { @@ -128,12 +123,7 @@ impl HalfEdgeBuilder for PartialHalfEdge { path } - fn update_from_other_edge( - &mut self, - _: &Partial, - other_prev: &Partial, - _: &SurfaceGeometry, - ) { + fn update_from_other_edge(&mut self, other_prev: &Partial) { self.start_vertex = other_prev.read().start_vertex.clone(); } } From 02567603847ea51820a813b88889a97011b5cf5b Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 8 Mar 2023 13:24:39 +0100 Subject: [PATCH 3/7] Remove unused trait method argument --- crates/fj-kernel/src/algorithms/sweep/face.rs | 10 ++++------ crates/fj-kernel/src/builder/cycle.rs | 3 --- 2 files changed, 4 insertions(+), 9 deletions(-) diff --git a/crates/fj-kernel/src/algorithms/sweep/face.rs b/crates/fj-kernel/src/algorithms/sweep/face.rs index 9ed217a3a9..32b29e4057 100644 --- a/crates/fj-kernel/src/algorithms/sweep/face.rs +++ b/crates/fj-kernel/src/algorithms/sweep/face.rs @@ -58,7 +58,7 @@ impl Sweep for Handle { bottom_face.surface().clone().translate(path, objects); let mut top_face = PartialFace::new(objects); - top_face.surface = Some(top_surface.clone()); + top_face.surface = Some(top_surface); top_face.color = Some(self.color()); for (i, cycle) in bottom_face.all_cycles().cloned().enumerate() { @@ -89,11 +89,9 @@ impl Sweep for Handle { top_edges.push(Partial::from(top_edge)); } - top_cycle.write().connect_to_closed_edges( - top_edges, - &top_surface.geometry(), - objects, - ); + top_cycle + .write() + .connect_to_closed_edges(top_edges, objects); for (bottom, top) in original_edges .into_iter() diff --git a/crates/fj-kernel/src/builder/cycle.rs b/crates/fj-kernel/src/builder/cycle.rs index b281403b6b..06ad6707db 100644 --- a/crates/fj-kernel/src/builder/cycle.rs +++ b/crates/fj-kernel/src/builder/cycle.rs @@ -2,7 +2,6 @@ use fj_math::Point; use itertools::Itertools; use crate::{ - geometry::surface::SurfaceGeometry, objects::{HalfEdge, Objects}, partial::{Partial, PartialCycle}, services::Service, @@ -45,7 +44,6 @@ pub trait CycleBuilder { fn connect_to_closed_edges( &mut self, edges: O, - surface: &SurfaceGeometry, objects: &mut Service, ) -> O::SameSize> where @@ -91,7 +89,6 @@ impl CycleBuilder for PartialCycle { fn connect_to_closed_edges( &mut self, edges: O, - _: &SurfaceGeometry, objects: &mut Service, ) -> O::SameSize> where From 0635d0507c334dd59f66f08b231dd18fce38da05 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 8 Mar 2023 13:26:43 +0100 Subject: [PATCH 4/7] Inline trait method into only call site --- crates/fj-kernel/src/builder/cycle.rs | 2 +- crates/fj-kernel/src/builder/edge.rs | 21 +-------------------- 2 files changed, 2 insertions(+), 21 deletions(-) diff --git a/crates/fj-kernel/src/builder/cycle.rs b/crates/fj-kernel/src/builder/cycle.rs index 06ad6707db..c6bd499ff0 100644 --- a/crates/fj-kernel/src/builder/cycle.rs +++ b/crates/fj-kernel/src/builder/cycle.rs @@ -96,7 +96,7 @@ impl CycleBuilder for PartialCycle { { edges.map_with_prev(|_, prev| { let mut this = self.add_half_edge(objects); - this.write().update_from_other_edge(&prev); + this.write().start_vertex = prev.read().start_vertex.clone(); this }) } diff --git a/crates/fj-kernel/src/builder/edge.rs b/crates/fj-kernel/src/builder/edge.rs index 0295abb0c5..a214a232ee 100644 --- a/crates/fj-kernel/src/builder/edge.rs +++ b/crates/fj-kernel/src/builder/edge.rs @@ -1,11 +1,7 @@ use fj_interop::ext::ArrayExt; use fj_math::{Point, Scalar}; -use crate::{ - geometry::curve::Curve, - objects::HalfEdge, - partial::{Partial, PartialHalfEdge}, -}; +use crate::{geometry::curve::Curve, partial::PartialHalfEdge}; /// Builder API for [`PartialHalfEdge`] pub trait HalfEdgeBuilder { @@ -34,17 +30,6 @@ pub trait HalfEdgeBuilder { start: Point<2>, end: Point<2>, ) -> Curve; - - /// Update this edge from another - /// - /// Infers as much information about this edge from the other, under the - /// assumption that the other edge is on a different surface. - /// - /// This method is quite fragile. It might panic, or even silently fail, - /// under various circumstances. As long as you're only dealing with lines - /// and planes, you should be fine. Otherwise, please read the code of this - /// method carefully, to make sure you don't run into trouble. - fn update_from_other_edge(&mut self, other_prev: &Partial); } impl HalfEdgeBuilder for PartialHalfEdge { @@ -122,8 +107,4 @@ impl HalfEdgeBuilder for PartialHalfEdge { path } - - fn update_from_other_edge(&mut self, other_prev: &Partial) { - self.start_vertex = other_prev.read().start_vertex.clone(); - } } From 04aefe4d94213d688a8cb81010acd2e371e37636 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 8 Mar 2023 13:27:38 +0100 Subject: [PATCH 5/7] Improve variable name --- crates/fj-kernel/src/builder/cycle.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/fj-kernel/src/builder/cycle.rs b/crates/fj-kernel/src/builder/cycle.rs index c6bd499ff0..15a1199843 100644 --- a/crates/fj-kernel/src/builder/cycle.rs +++ b/crates/fj-kernel/src/builder/cycle.rs @@ -95,9 +95,9 @@ impl CycleBuilder for PartialCycle { O: ObjectArgument>, { edges.map_with_prev(|_, prev| { - let mut this = self.add_half_edge(objects); - this.write().start_vertex = prev.read().start_vertex.clone(); - this + let mut edge = self.add_half_edge(objects); + edge.write().start_vertex = prev.read().start_vertex.clone(); + edge }) } } From 0dfbc542ca3b1b9ec9fc31ccb70b57f5e01e3990 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 8 Mar 2023 13:30:13 +0100 Subject: [PATCH 6/7] Fix word in doc comment --- crates/fj-kernel/src/builder/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/fj-kernel/src/builder/mod.rs b/crates/fj-kernel/src/builder/mod.rs index f88f695c24..0facf369ae 100644 --- a/crates/fj-kernel/src/builder/mod.rs +++ b/crates/fj-kernel/src/builder/mod.rs @@ -47,7 +47,7 @@ pub trait ObjectArgument: IntoIterator { /// Create a return value by mapping the implementing type /// - /// Provides access to the (circular) next item. + /// Provides access to the (circular) previous item. fn map_with_prev(self, f: F) -> Self::SameSize where F: FnMut(T, T) -> R, From 319892251a0fcc507113eb07000b48c512c8f984 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 8 Mar 2023 13:46:04 +0100 Subject: [PATCH 7/7] Simplify trait method name There used to be another similarly-named (and similarly-functioning) method right next to it, but that no longer exists. --- crates/fj-kernel/src/algorithms/sweep/face.rs | 4 +--- crates/fj-kernel/src/builder/cycle.rs | 4 ++-- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/crates/fj-kernel/src/algorithms/sweep/face.rs b/crates/fj-kernel/src/algorithms/sweep/face.rs index 32b29e4057..155cf5db62 100644 --- a/crates/fj-kernel/src/algorithms/sweep/face.rs +++ b/crates/fj-kernel/src/algorithms/sweep/face.rs @@ -89,9 +89,7 @@ impl Sweep for Handle { top_edges.push(Partial::from(top_edge)); } - top_cycle - .write() - .connect_to_closed_edges(top_edges, objects); + top_cycle.write().connect_to_edges(top_edges, objects); for (bottom, top) in original_edges .into_iter() diff --git a/crates/fj-kernel/src/builder/cycle.rs b/crates/fj-kernel/src/builder/cycle.rs index 15a1199843..62aadad7f7 100644 --- a/crates/fj-kernel/src/builder/cycle.rs +++ b/crates/fj-kernel/src/builder/cycle.rs @@ -41,7 +41,7 @@ pub trait CycleBuilder { /// equivalents of this cycle, form a cycle themselves. /// /// Returns the local equivalents of the provided half-edges. - fn connect_to_closed_edges( + fn connect_to_edges( &mut self, edges: O, objects: &mut Service, @@ -86,7 +86,7 @@ impl CycleBuilder for PartialCycle { half_edges } - fn connect_to_closed_edges( + fn connect_to_edges( &mut self, edges: O, objects: &mut Service,