From 3cac27eddb382d45abf602d035987b9a95fc47d5 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Fri, 3 Feb 2023 12:22:18 +0100 Subject: [PATCH 1/6] Fix path inference when updating edge from another --- crates/fj-kernel/src/builder/edge.rs | 106 +++++++++++++++++++++++++-- 1 file changed, 99 insertions(+), 7 deletions(-) diff --git a/crates/fj-kernel/src/builder/edge.rs b/crates/fj-kernel/src/builder/edge.rs index 247addb01..790127426 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}, }; @@ -196,13 +197,104 @@ 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(_) => { + // 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) + } + _ => { + // 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 From 08d399cf60a7de791accb7d338003a65b68c2523 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Fri, 3 Feb 2023 12:24:42 +0100 Subject: [PATCH 2/6] Update doc comment --- crates/fj-kernel/src/builder/edge.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/crates/fj-kernel/src/builder/edge.rs b/crates/fj-kernel/src/builder/edge.rs index 790127426..27ca41868 100644 --- a/crates/fj-kernel/src/builder/edge.rs +++ b/crates/fj-kernel/src/builder/edge.rs @@ -52,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); } From 27b57b6691983b41100c1012b1cd5378e0bc5b4e Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Fri, 3 Feb 2023 12:26:38 +0100 Subject: [PATCH 3/6] Update variable name --- crates/fj-kernel/src/builder/face.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/fj-kernel/src/builder/face.rs b/crates/fj-kernel/src/builder/face.rs index 95e639f16..d3bc12e6c 100644 --- a/crates/fj-kernel/src/builder/face.rs +++ b/crates/fj-kernel/src/builder/face.rs @@ -125,7 +125,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 +153,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); } From 25fb27c868ba12f9222fa49605a62447bf573740 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 7 Feb 2023 13:21:04 +0100 Subject: [PATCH 4/6] Remove unused code --- crates/fj-kernel/src/partial/objects/curve.rs | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/crates/fj-kernel/src/partial/objects/curve.rs b/crates/fj-kernel/src/partial/objects/curve.rs index c041384a3..36f4a9b02 100644 --- a/crates/fj-kernel/src/partial/objects/curve.rs +++ b/crates/fj-kernel/src/partial/objects/curve.rs @@ -61,22 +61,6 @@ pub enum MaybeSurfacePath { 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) From eb8411a8d221cb69ae4836eb171b49fd317d3d8b Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 7 Feb 2023 14:12:15 +0100 Subject: [PATCH 5/6] Store radius of undefined circle --- crates/fj-kernel/src/builder/edge.rs | 8 ++++++-- crates/fj-kernel/src/builder/face.rs | 2 +- crates/fj-kernel/src/partial/objects/curve.rs | 7 ++++++- 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/crates/fj-kernel/src/builder/edge.rs b/crates/fj-kernel/src/builder/edge.rs index 27ca41868..6ad4a737f 100644 --- a/crates/fj-kernel/src/builder/edge.rs +++ b/crates/fj-kernel/src/builder/edge.rs @@ -210,7 +210,7 @@ impl HalfEdgeBuilder for PartialHalfEdge { // 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(_) => { + 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 @@ -251,7 +251,11 @@ impl HalfEdgeBuilder for PartialHalfEdge { // I hope that I'll come up with a // better curve/surface representation // before this becomes a problem. - Some(MaybeSurfacePath::UndefinedCircle) + Some( + MaybeSurfacePath::UndefinedCircle { + radius: circle.radius(), + }, + ) } _ => { // The other edge is a line segment in a diff --git a/crates/fj-kernel/src/builder/face.rs b/crates/fj-kernel/src/builder/face.rs index d3bc12e6c..75d336cee 100644 --- a/crates/fj-kernel/src/builder/face.rs +++ b/crates/fj-kernel/src/builder/face.rs @@ -173,7 +173,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 36f4a9b02..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,7 +57,10 @@ 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, From 419921a165164dd7a9324fec55303b421e08b91d Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 8 Feb 2023 13:06:24 +0100 Subject: [PATCH 6/6] Move methods from `FaceBuilder` to `CycleBuilder` This makes those methods more flexible, as they can no longer just be used for face exteriors. --- crates/fj-kernel/src/builder/cycle.rs | 68 ++++++++++++++++++++++++++ crates/fj-kernel/src/builder/face.rs | 70 +-------------------------- 2 files changed, 70 insertions(+), 68 deletions(-) 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/face.rs b/crates/fj-kernel/src/builder/face.rs index 75d336cee..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(),