diff --git a/crates/fj-kernel/src/algorithms/sweep/face.rs b/crates/fj-kernel/src/algorithms/sweep/face.rs index 9ed217a3a..155cf5db6 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,7 @@ 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_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 388a27a32..62aadad7f 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, @@ -42,10 +41,9 @@ 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, - surface: &SurfaceGeometry, objects: &mut Service, ) -> O::SameSize> where @@ -88,19 +86,18 @@ impl CycleBuilder for PartialCycle { half_edges } - fn connect_to_closed_edges( + fn connect_to_edges( &mut self, edges: O, - surface: &SurfaceGeometry, objects: &mut Service, ) -> O::SameSize> where O: ObjectArgument>, { - edges.map_with_prev(|other, prev| { - let mut this = self.add_half_edge(objects); - this.write().update_from_other_edge(&other, &prev, surface); - this + edges.map_with_prev(|_, prev| { + let mut edge = self.add_half_edge(objects); + edge.write().start_vertex = prev.read().start_vertex.clone(); + edge }) } } diff --git a/crates/fj-kernel/src/builder/edge.rs b/crates/fj-kernel/src/builder/edge.rs index 4b7febb57..a214a232e 100644 --- a/crates/fj-kernel/src/builder/edge.rs +++ b/crates/fj-kernel/src/builder/edge.rs @@ -1,14 +1,7 @@ use fj_interop::ext::ArrayExt; use fj_math::{Point, Scalar}; -use crate::{ - geometry::{ - curve::{Curve, GlobalPath}, - surface::SurfaceGeometry, - }, - objects::HalfEdge, - partial::{Partial, PartialHalfEdge}, -}; +use crate::{geometry::curve::Curve, partial::PartialHalfEdge}; /// Builder API for [`PartialHalfEdge`] pub trait HalfEdgeBuilder { @@ -37,22 +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: &Partial, - other_prev: &Partial, - surface: &SurfaceGeometry, - ); } impl HalfEdgeBuilder for PartialHalfEdge { @@ -130,83 +107,4 @@ impl HalfEdgeBuilder for PartialHalfEdge { path } - - fn update_from_other_edge( - &mut self, - other: &Partial, - other_prev: &Partial, - surface: &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(); - } } diff --git a/crates/fj-kernel/src/builder/mod.rs b/crates/fj-kernel/src/builder/mod.rs index f88f695c2..0facf369a 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,