diff --git a/Cargo.lock b/Cargo.lock index cac52b3dc..6b3fce539 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1187,6 +1187,7 @@ dependencies = [ "anyhow", "fj-interop", "fj-math", + "itertools", "parking_lot", "pretty_assertions", "robust-predicates", @@ -1836,6 +1837,15 @@ version = "2.5.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f88c5561171189e69df9d98bcf18fd5f9558300f7ea7b801eb8a0fd748bd8745" +[[package]] +name = "itertools" +version = "0.10.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b0fd2260e829bddf4cb6ea802289de2f86d6a7a690192fbe91b3f46e0f2c8473" +dependencies = [ + "either", +] + [[package]] name = "itoa" version = "1.0.4" diff --git a/crates/fj-kernel/Cargo.toml b/crates/fj-kernel/Cargo.toml index 6fab7f3ff..36545b3ad 100644 --- a/crates/fj-kernel/Cargo.toml +++ b/crates/fj-kernel/Cargo.toml @@ -13,6 +13,7 @@ categories.workspace = true [dependencies] fj-interop.workspace = true fj-math.workspace = true +itertools = "0.10.5" parking_lot = "0.12.0" pretty_assertions = "1.3.0" robust-predicates = "0.1.4" diff --git a/crates/fj-kernel/src/builder/cycle.rs b/crates/fj-kernel/src/builder/cycle.rs index a2b5ca081..09f25cd30 100644 --- a/crates/fj-kernel/src/builder/cycle.rs +++ b/crates/fj-kernel/src/builder/cycle.rs @@ -55,34 +55,32 @@ impl CycleBuilder for PartialCycle { .surface() .expect("Need surface to extend cycle with poly-chain"); - let position_prev = vertex_prev - .position() - .expect("Need surface position to extend cycle"); - let position_next = vertex_next - .position() - .expect("Need surface position to extend cycle"); + let [position_prev, position_next] = + [&vertex_prev, &vertex_next].map(|vertex| { + vertex + .position() + .expect("Need surface position to extend cycle") + }); - let from = vertex_prev; - let to = vertex_next; - - previous = Some(to.clone()); + previous = Some(vertex_next.clone()); let curve = Curve::partial() .with_surface(Some(surface.clone())) .update_as_line_from_points([position_prev, position_next]); - let [from, to] = - [(0., from), (1., to)].map(|(position, surface_form)| { + let vertices = [(0., vertex_prev), (1., vertex_next)].map( + |(position, surface_form)| { Vertex::partial() .with_curve(curve.clone()) .with_position(Some([position])) .with_surface_form(surface_form) - }); + }, + ); half_edges.push( HalfEdge::partial() .with_curve(curve) - .with_vertices([from, to]), + .with_vertices(vertices), ); continue; diff --git a/crates/fj-kernel/src/builder/edge.rs b/crates/fj-kernel/src/builder/edge.rs index f85b1bc45..c97699b4f 100644 --- a/crates/fj-kernel/src/builder/edge.rs +++ b/crates/fj-kernel/src/builder/edge.rs @@ -6,7 +6,7 @@ use crate::{ Curve, GlobalVertex, Objects, Surface, SurfaceVertex, Vertex, VerticesInNormalizedOrder, }, - partial::{HasPartial, PartialGlobalEdge, PartialHalfEdge}, + partial::{HasPartial, MaybePartial, PartialGlobalEdge, PartialHalfEdge}, storage::Handle, validate::ValidationError, }; @@ -15,6 +15,12 @@ use super::{CurveBuilder, GlobalVertexBuilder}; /// Builder API for [`PartialHalfEdge`] pub trait HalfEdgeBuilder: Sized { + /// Update the partial half-edge with the given back vertex + fn with_back_vertex(self, back: impl Into>) -> Self; + + /// Update the partial half-edge with the given front vertex + fn with_front_vertex(self, front: impl Into>) -> Self; + /// Update partial half-edge as a circle, from the given radius /// /// # Implementation Note @@ -37,9 +43,22 @@ pub trait HalfEdgeBuilder: Sized { /// Update partial half-edge as a line segment, reusing existing vertices fn update_as_line_segment(self) -> Self; + + /// Infer the global form of the partial half-edge + fn infer_global_form(self) -> Self; } impl HalfEdgeBuilder for PartialHalfEdge { + fn with_back_vertex(self, back: impl Into>) -> Self { + let [_, front] = self.vertices(); + self.with_vertices([back.into(), front]) + } + + fn with_front_vertex(self, front: impl Into>) -> Self { + let [back, _] = self.vertices(); + self.with_vertices([back, front.into()]) + } + fn update_as_circle_from_radius( self, radius: impl Into, @@ -181,6 +200,10 @@ impl HalfEdgeBuilder for PartialHalfEdge { self.with_curve(curve).with_vertices([back, front]) } + + fn infer_global_form(self) -> Self { + self.with_global_form(PartialGlobalEdge::default()) + } } /// Builder API for [`PartialGlobalEdge`] diff --git a/crates/fj-kernel/src/objects/cycle.rs b/crates/fj-kernel/src/objects/cycle.rs index 78fda9ad8..412f07b50 100644 --- a/crates/fj-kernel/src/objects/cycle.rs +++ b/crates/fj-kernel/src/objects/cycle.rs @@ -1,6 +1,7 @@ +use std::slice; + use fj_interop::ext::SliceExt; use fj_math::{Scalar, Winding}; -use pretty_assertions::assert_eq; use crate::{path::SurfacePath, storage::Handle}; @@ -24,48 +25,14 @@ impl Cycle { pub fn new(half_edges: impl IntoIterator>) -> Self { let half_edges = half_edges.into_iter().collect::>(); - let surface = match half_edges.first() { - Some(half_edge) => half_edge.surface().clone(), - None => panic!("Cycle must contain at least one half-edge"), - }; - - // Verify, that the curves of all edges are defined in the correct - // surface. - for edge in &half_edges { - assert_eq!( - surface.id(), - edge.curve().surface().id(), - "Edges in cycle not defined in same surface" - ); - } - - if half_edges.len() != 1 { - // Verify that all edges connect. - for [a, b] in half_edges.as_slice().array_windows_ext() { - let [_, prev] = a.vertices(); - let [next, _] = b.vertices(); - - assert_eq!( - prev.surface_form().id(), - next.surface_form().id(), - "Edges in cycle do not connect" - ); - } - } - - // Verify that the edges form a cycle - if let Some(first) = half_edges.first() { - if let Some(last) = half_edges.last() { - let [first, _] = first.vertices(); - let [_, last] = last.vertices(); - - assert_eq!( - first.surface_form().id(), - last.surface_form().id(), - "Edges do not form a cycle" - ); - } - } + // This is not a validation check, and thus not part of the validation + // infrastructure. The property being checked here is inherent to the + // validity of a `Cycle`, as methods of `Cycle` might assume that there + // is at least one edge. + assert!( + !half_edges.is_empty(), + "Cycle must contain at least one half-edge" + ); Self { half_edges } } @@ -82,7 +49,7 @@ impl Cycle { } /// Access the half-edges that make up the cycle - pub fn half_edges(&self) -> impl Iterator> + '_ { + pub fn half_edges(&self) -> HalfEdgesOfCycle { self.half_edges.iter() } @@ -144,3 +111,8 @@ impl Cycle { unreachable!("Encountered invalid cycle: {self:#?}"); } } + +/// An iterator over the half-edges of a [`Cycle`] +/// +/// Returned by [`Cycle::half_edges`]. +pub type HalfEdgesOfCycle<'a> = slice::Iter<'a, Handle>; diff --git a/crates/fj-kernel/src/objects/mod.rs b/crates/fj-kernel/src/objects/mod.rs index dba16edd5..07a37bd53 100644 --- a/crates/fj-kernel/src/objects/mod.rs +++ b/crates/fj-kernel/src/objects/mod.rs @@ -85,7 +85,7 @@ mod vertex; pub use self::{ curve::{Curve, GlobalCurve}, - cycle::Cycle, + cycle::{Cycle, HalfEdgesOfCycle}, edge::{GlobalEdge, HalfEdge, VerticesInNormalizedOrder}, face::{Face, FaceSet, Handedness}, shell::Shell, @@ -103,8 +103,8 @@ use crate::{ path::GlobalPath, storage::{Handle, Store}, validate::{ - HalfEdgeValidationError, SurfaceVertexValidationError, Validate2, - VertexValidationError, + CycleValidationError, HalfEdgeValidationError, + SurfaceVertexValidationError, Validate2, VertexValidationError, }, }; @@ -187,7 +187,10 @@ pub struct Cycles { impl Cycles { /// Insert a [`Cycle`] into the store - pub fn insert(&self, cycle: Cycle) -> Result, Infallible> { + pub fn insert( + &self, + cycle: Cycle, + ) -> Result, CycleValidationError> { cycle.validate()?; Ok(self.store.insert(cycle)) } diff --git a/crates/fj-kernel/src/partial/maybe_partial.rs b/crates/fj-kernel/src/partial/maybe_partial.rs index 5b9cee73c..ecc6ad9ee 100644 --- a/crates/fj-kernel/src/partial/maybe_partial.rs +++ b/crates/fj-kernel/src/partial/maybe_partial.rs @@ -67,8 +67,14 @@ impl MaybePartial { /// Merge this `MaybePartial` with another of the same type pub fn merge_with(self, other: impl Into) -> Self { match (self, other.into()) { - (Self::Full(_), Self::Full(_)) => { - panic!("Can't merge two full objects") + (Self::Full(a), Self::Full(b)) => { + if a.id() != b.id() { + panic!("Can't merge two full objects") + } + + // If they're equal, which they are, if we reach this point, + // then merging them is a no-op. + Self::Full(a) } (Self::Full(full), Self::Partial(_)) | (Self::Partial(_), Self::Full(full)) => Self::Full(full), @@ -218,6 +224,14 @@ impl MaybePartial { Self::Partial(partial) => partial.surface(), } } + + /// Access the global form + pub fn global_form(&self) -> MaybePartial { + match self { + Self::Full(full) => full.global_form().clone().into(), + Self::Partial(partial) => partial.global_form(), + } + } } impl MaybePartial { diff --git a/crates/fj-kernel/src/partial/objects/cycle.rs b/crates/fj-kernel/src/partial/objects/cycle.rs index bc8479abf..d27531b53 100644 --- a/crates/fj-kernel/src/partial/objects/cycle.rs +++ b/crates/fj-kernel/src/partial/objects/cycle.rs @@ -1,4 +1,5 @@ use crate::{ + builder::HalfEdgeBuilder, objects::{Cycle, HalfEdge, Objects, Surface}, partial::{ util::merge_options, MaybePartial, PartialHalfEdge, PartialVertex, @@ -86,6 +87,22 @@ impl PartialCycle { mut self, objects: &Objects, ) -> Result, ValidationError> { + // Check that the cycle is closed. This will lead to a panic further + // down anyway, but that panic would be super-confusing. This one should + // be a bit more explicit on what is wrong. + if let (Some(first), Some(last)) = + (self.half_edges.first(), self.half_edges.last()) + { + let [first, _] = first.vertices(); + let [_, last] = last.vertices(); + + assert_eq!( + first.surface_form().position(), + last.surface_form().position(), + "Attempting to build un-closed cycle" + ); + } + // To create a cycle, we need to make sure that all its half-edges // connect to each other. Let's start with all the connections between // the first and the last half-edge. @@ -96,14 +113,11 @@ impl PartialCycle { half_edge.front().surface_form().into_full(objects)?; *half_edge = half_edge.clone().merge_with( - PartialHalfEdge::default() - .with_back_vertex( - PartialVertex::default().with_surface_form(back_vertex), - ) - .with_front_vertex( - PartialVertex::default() - .with_surface_form(front_vertex.clone()), - ), + PartialHalfEdge::default().with_vertices([ + PartialVertex::default().with_surface_form(back_vertex), + PartialVertex::default() + .with_surface_form(front_vertex.clone()), + ]), ); previous_vertex = Some(MaybePartial::from(front_vertex)); diff --git a/crates/fj-kernel/src/partial/objects/edge.rs b/crates/fj-kernel/src/partial/objects/edge.rs index 7b5ffe3c9..fb8d268ad 100644 --- a/crates/fj-kernel/src/partial/objects/edge.rs +++ b/crates/fj-kernel/src/partial/objects/edge.rs @@ -73,28 +73,6 @@ impl PartialHalfEdge { self } - /// Update the partial half-edge with the given back vertex - pub fn with_back_vertex( - mut self, - vertex: impl Into>, - ) -> Self { - let [from, _] = &mut self.vertices; - *from = vertex.into(); - - self - } - - /// Update the partial half-edge with the given front vertex - pub fn with_front_vertex( - mut self, - vertex: impl Into>, - ) -> Self { - let [_, to] = &mut self.vertices; - *to = vertex.into(); - - self - } - /// Update the partial half-edge with the given vertices pub fn with_vertices( mut self, diff --git a/crates/fj-kernel/src/validate/cycle.rs b/crates/fj-kernel/src/validate/cycle.rs index 57eb8b348..22603cd9f 100644 --- a/crates/fj-kernel/src/validate/cycle.rs +++ b/crates/fj-kernel/src/validate/cycle.rs @@ -1,16 +1,116 @@ -use std::convert::Infallible; +use itertools::Itertools; -use crate::objects::Cycle; +use crate::{ + objects::{Cycle, SurfaceVertex}, + storage::Handle, +}; use super::{Validate2, ValidationConfig}; impl Validate2 for Cycle { - type Error = Infallible; + type Error = CycleValidationError; fn validate_with_config( &self, _: &ValidationConfig, ) -> Result<(), Self::Error> { + CycleValidationError::check_half_edge_connections(self)?; + + // We don't need to check that all half-edges are defined in the same + // surface. We already check that they are connected by identical + // surface vertices, so that would be redundant. + + Ok(()) + } +} + +/// [`Cycle`] validation error +#[derive(Debug, thiserror::Error)] +pub enum CycleValidationError { + /// Half-edges are not connected + #[error( + "`HalfEdge`s of `Cycle` are not connected\n\ + - Front vertex of previous `HalfEdge`: {prev:#?}\n\ + - Back vertex of next `HalfEdge`: {next:#?}" + )] + HalfEdgeConnection { + /// The front vertex of the previous half-edge + prev: Handle, + + /// The back vertex of the next half-edge + next: Handle, + }, +} + +impl CycleValidationError { + fn check_half_edge_connections(cycle: &Cycle) -> Result<(), Self> { + for (a, b) in cycle.half_edges().circular_tuple_windows() { + let [_, prev] = a.vertices(); + let [next, _] = b.vertices(); + + let prev = prev.surface_form(); + let next = next.surface_form(); + + if prev.id() != next.id() { + return Err(Self::HalfEdgeConnection { + prev: prev.clone(), + next: next.clone(), + }); + } + } + + Ok(()) + } +} + +#[cfg(test)] +mod tests { + use crate::{ + builder::{CycleBuilder, HalfEdgeBuilder, VertexBuilder}, + objects::{Cycle, Objects}, + partial::HasPartial, + validate::Validate2, + }; + + #[test] + fn cycle_half_edge_connections() -> anyhow::Result<()> { + let objects = Objects::new(); + + let valid = Cycle::partial() + .with_poly_chain_from_points( + objects.surfaces.xy_plane(), + [[0., 0.], [1., 0.], [0., 1.]], + ) + .close_with_line_segment() + .build(&objects)?; + let invalid = { + let mut half_edges = valid + .half_edges() + .map(|half_edge| half_edge.to_partial()) + .collect::>(); + + let first_half_edge = &mut half_edges[0]; + let [first_vertex, _] = first_half_edge.vertices(); + + // Sever connection between the last and first half-edge in the + // cycle. + let first_vertex = first_vertex.into_partial().infer_surface_form(); + *first_half_edge = first_half_edge + .clone() + .with_back_vertex(first_vertex) + .infer_global_form(); + + let half_edges = half_edges + .into_iter() + .map(|half_edge| half_edge.build(&objects)) + .collect::, _>>()?; + + Cycle::new(half_edges) + }; + + assert!(valid.validate().is_ok()); + assert!(invalid.validate().is_err()); + Ok(()) } } diff --git a/crates/fj-kernel/src/validate/edge.rs b/crates/fj-kernel/src/validate/edge.rs index 2babcdf94..6db571ebd 100644 --- a/crates/fj-kernel/src/validate/edge.rs +++ b/crates/fj-kernel/src/validate/edge.rs @@ -24,6 +24,11 @@ impl Validate2 for HalfEdge { HalfEdgeValidationError::check_global_curve_identity(self)?; HalfEdgeValidationError::check_global_vertex_identity(self)?; HalfEdgeValidationError::check_vertex_positions(self, config)?; + + // We don't need to check anything about surfaces here. We already check + // curves, which makes sure the vertices are consistent with each other, + // and the validation of those vertices checks the surfaces. + Ok(()) } } diff --git a/crates/fj-kernel/src/validate/mod.rs b/crates/fj-kernel/src/validate/mod.rs index 32abea1c3..5d73d839f 100644 --- a/crates/fj-kernel/src/validate/mod.rs +++ b/crates/fj-kernel/src/validate/mod.rs @@ -26,6 +26,7 @@ mod uniqueness; mod vertex; pub use self::{ + cycle::CycleValidationError, edge::HalfEdgeValidationError, uniqueness::UniquenessIssues, vertex::{SurfaceVertexValidationError, VertexValidationError}, @@ -177,6 +178,10 @@ pub enum ValidationError { #[error("Uniqueness validation failed")] Uniqueness(#[from] UniquenessIssues), + /// `Cycle` validation error + #[error(transparent)] + Cycle(#[from] CycleValidationError), + /// `HalfEdge` validation error #[error(transparent)] HalfEdge(#[from] HalfEdgeValidationError),