diff --git a/crates/fj-kernel/src/builder/cycle.rs b/crates/fj-kernel/src/builder/cycle.rs index e35c95eb4..158b057bb 100644 --- a/crates/fj-kernel/src/builder/cycle.rs +++ b/crates/fj-kernel/src/builder/cycle.rs @@ -1,3 +1,5 @@ +use std::collections::VecDeque; + use fj_interop::ext::ArrayExt; use fj_math::Point; @@ -76,6 +78,33 @@ pub trait CycleBuilder { &mut self, points: [impl Into>; 3], ) -> [Partial; 3]; + + /// Connect the cycle to the provided half-edges + /// + /// Assumes that the provided half-edges, once translated into local + /// equivalents of this cycle, will not form a cycle themselves. + /// + /// Returns the local equivalents of the provided half-edges and, as the + /// last entry, an additional half-edge that closes the cycle. + fn connect_to_open_edges( + &mut self, + edges: O, + ) -> O::SizePlusOne> + where + O: ObjectArgument>; + + /// Connect the cycles to the provided half-edges + /// + /// Assumes that the provided half-edges, once translated into local + /// equivalents of this cycle, form a cycle themselves. + /// + /// Returns the local equivalents of the provided half-edges. + fn connect_to_closed_edges( + &mut self, + edges: O, + ) -> O::SameSize> + where + O: ObjectArgument>; } impl CycleBuilder for PartialCycle { @@ -197,4 +226,43 @@ impl CycleBuilder for PartialCycle { half_edges } + + fn connect_to_open_edges( + &mut self, + edges: O, + ) -> O::SizePlusOne> + where + O: ObjectArgument>, + { + // We need to create the additional half-edge last, but at the same time + // need to provide it to the `map_plus_one` method first. Really no + // choice but to create them all in one go, as we do here. + let mut half_edges = VecDeque::new(); + for _ in 0..edges.num_objects() { + half_edges.push_back(self.add_half_edge()); + } + let additional_half_edge = self.add_half_edge(); + + edges.map_plus_one(additional_half_edge, |other| { + let mut this = half_edges.pop_front().expect( + "Pushed correct number of half-edges; should be able to pop", + ); + this.write().update_from_other_edge(&other); + this + }) + } + + fn connect_to_closed_edges( + &mut self, + edges: O, + ) -> O::SameSize> + where + O: ObjectArgument>, + { + edges.map(|other| { + let mut this = self.add_half_edge(); + this.write().update_from_other_edge(&other); + this + }) + } } diff --git a/crates/fj-kernel/src/builder/edge.rs b/crates/fj-kernel/src/builder/edge.rs index 247addb01..6ad4a737f 100644 --- a/crates/fj-kernel/src/builder/edge.rs +++ b/crates/fj-kernel/src/builder/edge.rs @@ -2,6 +2,7 @@ use fj_interop::ext::ArrayExt; use fj_math::{Point, Scalar}; use crate::{ + geometry::path::{GlobalPath, SurfacePath}, objects::{GlobalEdge, HalfEdge, Surface}, partial::{MaybeSurfacePath, Partial, PartialGlobalEdge, PartialHalfEdge}, }; @@ -51,6 +52,11 @@ pub trait HalfEdgeBuilder { /// /// 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); } @@ -196,13 +202,108 @@ impl HalfEdgeBuilder for PartialHalfEdge { self.curve.write().global_form = global_curve.clone(); self.global_form.write().curve = global_curve; - self.curve.write().path = other - .read() - .curve - .read() - .path - .as_ref() - .map(MaybeSurfacePath::to_undefined); + self.curve.write().path = + other.read().curve.read().path.as_ref().and_then(|path| { + match other.read().curve.read().surface.read().geometry { + Some(surface) => { + // 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(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 { + MaybeSurfacePath::Defined( + SurfacePath::Line(_), + ) + | MaybeSurfacePath::UndefinedLine => { + // 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. + Some( + MaybeSurfacePath::UndefinedCircle { + radius: circle.radius(), + }, + ) + } + _ => { + // 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 { + MaybeSurfacePath::Defined( + SurfacePath::Line(_), + ) + | MaybeSurfacePath::UndefinedLine => { + // The other edge is a line segment on + // a plane. That means our edge must be + // a line segment too. + Some(MaybeSurfacePath::UndefinedLine) + } + _ => { + // 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" + ) + } + } + } + } + } + None => { + // We know nothing about the surface the other edge is + // on. This means we can't infer anything about our + // curve from the other curve. + None + } + } + }); for (this, other) in self .vertices diff --git a/crates/fj-kernel/src/builder/face.rs b/crates/fj-kernel/src/builder/face.rs index 95e639f16..34bde9efc 100644 --- a/crates/fj-kernel/src/builder/face.rs +++ b/crates/fj-kernel/src/builder/face.rs @@ -4,41 +4,14 @@ use fj_interop::ext::ArrayExt; use crate::{ geometry::path::SurfacePath, - objects::{Cycle, HalfEdge, Surface}, + objects::{Cycle, Surface}, partial::{MaybeSurfacePath, Partial, PartialCycle, PartialFace}, }; -use super::{CycleBuilder, HalfEdgeBuilder, ObjectArgument, SurfaceBuilder}; +use super::SurfaceBuilder; /// Builder API for [`PartialFace`] pub trait FaceBuilder { - /// Connect the face to other faces at the provided half-edges - /// - /// Assumes that the provided half-edges, once translated into local - /// equivalents of this face, will not form a cycle. - /// - /// Returns the local equivalents of the provided half-edges and, as the - /// last entry, an additional half-edge that closes the cycle. - fn connect_to_open_edges( - &mut self, - edges: O, - ) -> O::SizePlusOne> - where - O: ObjectArgument>; - - /// Connect the face to other faces at the provided half-edges - /// - /// Assumes that the provided half-edges, once translated into local - /// equivalents of this face, form a cycle. - /// - /// Returns the local equivalents of the provided half-edges. - fn connect_to_closed_edges( - &mut self, - edges: O, - ) -> O::SameSize> - where - O: ObjectArgument>; - /// Add an interior cycle fn add_interior(&mut self) -> Partial; @@ -59,45 +32,6 @@ pub trait FaceBuilder { } impl FaceBuilder for PartialFace { - fn connect_to_open_edges( - &mut self, - edges: O, - ) -> O::SizePlusOne> - where - O: ObjectArgument>, - { - // We need to create the additional half-edge last, but at the same time - // need to provide it to the `map_plus_one` method first. Really no - // choice but to create them all in one go, as we do here. - let mut half_edges = VecDeque::new(); - for _ in 0..edges.num_objects() { - half_edges.push_back(self.exterior.write().add_half_edge()); - } - let additional_half_edge = self.exterior.write().add_half_edge(); - - edges.map_plus_one(additional_half_edge, |other| { - let mut this = half_edges.pop_front().expect( - "Pushed correct number of half-edges; should be able to pop", - ); - this.write().update_from_other_edge(&other); - this - }) - } - - fn connect_to_closed_edges( - &mut self, - edges: O, - ) -> O::SameSize> - where - O: ObjectArgument>, - { - edges.map(|other| { - let mut this = self.exterior.write().add_half_edge(); - this.write().update_from_other_edge(&other); - this - }) - } - fn add_interior(&mut self) -> Partial { let cycle = Partial::from_partial(PartialCycle { surface: self.exterior.read().surface.clone(), @@ -125,7 +59,7 @@ impl FaceBuilder for PartialFace { }) .collect::>(); - let (first_three_vertices, surface) = { + let (significant_vertices, surface) = { let first_three_vertices = array::from_fn(|_| { vertices .pop_front() @@ -153,7 +87,7 @@ impl FaceBuilder for PartialFace { }); for (mut surface_vertex, point) in - first_three_vertices.into_iter().chain(rest_of_vertices) + significant_vertices.into_iter().chain(rest_of_vertices) { surface_vertex.write().position = Some(point); } @@ -173,7 +107,7 @@ impl FaceBuilder for PartialFace { MaybeSurfacePath::Defined(_) => { // Path is already defined. Nothing to infer. } - MaybeSurfacePath::UndefinedCircle => todo!( + MaybeSurfacePath::UndefinedCircle { .. } => todo!( "Inferring undefined circles is not supported yet" ), MaybeSurfacePath::UndefinedLine => { diff --git a/crates/fj-kernel/src/partial/objects/curve.rs b/crates/fj-kernel/src/partial/objects/curve.rs index c041384a3..e1924b692 100644 --- a/crates/fj-kernel/src/partial/objects/curve.rs +++ b/crates/fj-kernel/src/partial/objects/curve.rs @@ -1,3 +1,5 @@ +use fj_math::Scalar; + use crate::{ geometry::path::SurfacePath, objects::{Curve, GlobalCurve, Objects, Surface}, @@ -55,28 +57,15 @@ pub enum MaybeSurfacePath { Defined(SurfacePath), /// The surface path is undefined, but we know it is a circle - UndefinedCircle, + UndefinedCircle { + /// The radius of the undefined circle + radius: Scalar, + }, /// The surface path is undefined, but we know it is a line UndefinedLine, } -impl MaybeSurfacePath { - /// Convert into an undefined variant - /// - /// If `self` is defined, it is converted into the applicable undefined - /// variant. If it is undefined, a copy is returned. - pub fn to_undefined(&self) -> Self { - match self { - Self::Defined(path) => match path { - SurfacePath::Circle(_) => Self::UndefinedCircle, - SurfacePath::Line(_) => Self::UndefinedLine, - }, - undefined => undefined.clone(), - } - } -} - impl From for MaybeSurfacePath { fn from(path: SurfacePath) -> Self { Self::Defined(path)