From a902e2522e9b282bfcf3dc8540ebe0dd4f2b6a8b Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 8 Nov 2022 12:32:58 +0100 Subject: [PATCH 01/15] Refactor --- crates/fj-kernel/src/builder/cycle.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/crates/fj-kernel/src/builder/cycle.rs b/crates/fj-kernel/src/builder/cycle.rs index a2b5ca081..0eff6841c 100644 --- a/crates/fj-kernel/src/builder/cycle.rs +++ b/crates/fj-kernel/src/builder/cycle.rs @@ -55,12 +55,12 @@ 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; From c71d10893affc633a4df4d836aaef3eb43cf5a58 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 8 Nov 2022 12:35:10 +0100 Subject: [PATCH 02/15] Inline variables --- crates/fj-kernel/src/builder/cycle.rs | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/crates/fj-kernel/src/builder/cycle.rs b/crates/fj-kernel/src/builder/cycle.rs index 0eff6841c..0de1fa0df 100644 --- a/crates/fj-kernel/src/builder/cycle.rs +++ b/crates/fj-kernel/src/builder/cycle.rs @@ -62,22 +62,20 @@ impl CycleBuilder for PartialCycle { .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 [from, to] = [(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() From aeb686c5cdd8b8f91f1791de08b7e7d978362d3a Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 8 Nov 2022 12:35:32 +0100 Subject: [PATCH 03/15] Refactor --- crates/fj-kernel/src/builder/cycle.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/fj-kernel/src/builder/cycle.rs b/crates/fj-kernel/src/builder/cycle.rs index 0de1fa0df..09f25cd30 100644 --- a/crates/fj-kernel/src/builder/cycle.rs +++ b/crates/fj-kernel/src/builder/cycle.rs @@ -68,7 +68,7 @@ impl CycleBuilder for PartialCycle { .with_surface(Some(surface.clone())) .update_as_line_from_points([position_prev, position_next]); - let [from, to] = [(0., vertex_prev), (1., vertex_next)].map( + let vertices = [(0., vertex_prev), (1., vertex_next)].map( |(position, surface_form)| { Vertex::partial() .with_curve(curve.clone()) @@ -80,7 +80,7 @@ impl CycleBuilder for PartialCycle { half_edges.push( HalfEdge::partial() .with_curve(curve) - .with_vertices([from, to]), + .with_vertices(vertices), ); continue; From e24c56b69ad7c5240b0f5e568c8d4d0303d142db Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 8 Nov 2022 13:26:59 +0100 Subject: [PATCH 04/15] Move non-essential methods to builder trait --- crates/fj-kernel/src/builder/edge.rs | 18 ++++++++++++++- crates/fj-kernel/src/partial/objects/cycle.rs | 1 + crates/fj-kernel/src/partial/objects/edge.rs | 22 ------------------- 3 files changed, 18 insertions(+), 23 deletions(-) diff --git a/crates/fj-kernel/src/builder/edge.rs b/crates/fj-kernel/src/builder/edge.rs index f85b1bc45..ba5f3369a 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 @@ -40,6 +46,16 @@ pub trait HalfEdgeBuilder: Sized { } 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, diff --git a/crates/fj-kernel/src/partial/objects/cycle.rs b/crates/fj-kernel/src/partial/objects/cycle.rs index bc8479abf..461c48fdb 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, 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, From 8ede469d00eafd8e22e872d7ebd2d9ab7326a8a3 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 8 Nov 2022 13:28:02 +0100 Subject: [PATCH 05/15] Refactor --- crates/fj-kernel/src/partial/objects/cycle.rs | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/crates/fj-kernel/src/partial/objects/cycle.rs b/crates/fj-kernel/src/partial/objects/cycle.rs index 461c48fdb..366520966 100644 --- a/crates/fj-kernel/src/partial/objects/cycle.rs +++ b/crates/fj-kernel/src/partial/objects/cycle.rs @@ -97,14 +97,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)); From 41b32978b7012931c51ad238c8a5daac5099eb36 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 8 Nov 2022 13:42:03 +0100 Subject: [PATCH 06/15] Don't panic, when merging identical full objects --- crates/fj-kernel/src/partial/maybe_partial.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/crates/fj-kernel/src/partial/maybe_partial.rs b/crates/fj-kernel/src/partial/maybe_partial.rs index 5b9cee73c..ce1ea530c 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), From 5b0b4237ccd2aa1b2c651f7f06ec33251ba22d7b Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 8 Nov 2022 13:42:26 +0100 Subject: [PATCH 07/15] Add `MaybePartial::global_form` --- crates/fj-kernel/src/partial/maybe_partial.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/crates/fj-kernel/src/partial/maybe_partial.rs b/crates/fj-kernel/src/partial/maybe_partial.rs index ce1ea530c..ecc6ad9ee 100644 --- a/crates/fj-kernel/src/partial/maybe_partial.rs +++ b/crates/fj-kernel/src/partial/maybe_partial.rs @@ -224,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 { From 118d2c01fe0ec369e850e44eaa2b4f58ed0923d6 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 8 Nov 2022 14:19:05 +0100 Subject: [PATCH 08/15] Add `HalfEdgeBuilder::infer_global_form` --- crates/fj-kernel/src/builder/edge.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/crates/fj-kernel/src/builder/edge.rs b/crates/fj-kernel/src/builder/edge.rs index ba5f3369a..c97699b4f 100644 --- a/crates/fj-kernel/src/builder/edge.rs +++ b/crates/fj-kernel/src/builder/edge.rs @@ -43,6 +43,9 @@ 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 { @@ -197,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`] From b121f8a1e95d6c668e3cd211a4c42a96c1bf8960 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 8 Nov 2022 14:35:31 +0100 Subject: [PATCH 09/15] Improve super-confusing panic message --- crates/fj-kernel/src/partial/objects/cycle.rs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/crates/fj-kernel/src/partial/objects/cycle.rs b/crates/fj-kernel/src/partial/objects/cycle.rs index 366520966..d27531b53 100644 --- a/crates/fj-kernel/src/partial/objects/cycle.rs +++ b/crates/fj-kernel/src/partial/objects/cycle.rs @@ -87,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. From 7671353c50f047e649083e2f996d6330c8e36c6d Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 1 Nov 2022 14:02:22 +0100 Subject: [PATCH 10/15] Add `CycleValidationError` --- crates/fj-kernel/src/objects/mod.rs | 9 ++++++--- crates/fj-kernel/src/validate/cycle.rs | 8 +++++--- crates/fj-kernel/src/validate/mod.rs | 5 +++++ 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/crates/fj-kernel/src/objects/mod.rs b/crates/fj-kernel/src/objects/mod.rs index dba16edd5..bdba01e72 100644 --- a/crates/fj-kernel/src/objects/mod.rs +++ b/crates/fj-kernel/src/objects/mod.rs @@ -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/validate/cycle.rs b/crates/fj-kernel/src/validate/cycle.rs index 57eb8b348..83ce06b68 100644 --- a/crates/fj-kernel/src/validate/cycle.rs +++ b/crates/fj-kernel/src/validate/cycle.rs @@ -1,11 +1,9 @@ -use std::convert::Infallible; - use crate::objects::Cycle; use super::{Validate2, ValidationConfig}; impl Validate2 for Cycle { - type Error = Infallible; + type Error = CycleValidationError; fn validate_with_config( &self, @@ -14,3 +12,7 @@ impl Validate2 for Cycle { Ok(()) } } + +/// [`Cycle`] validation error +#[derive(Debug, thiserror::Error)] +pub enum CycleValidationError {} 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), From 2dd2c916eba5b834eaa06e2eb19a6d307302b03b Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 8 Nov 2022 11:55:20 +0100 Subject: [PATCH 11/15] Remove redundant validation check We already check that all half-edges are connected by identical surface vertices. Checking the surface on top of that is redundant. --- crates/fj-kernel/src/objects/cycle.rs | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/crates/fj-kernel/src/objects/cycle.rs b/crates/fj-kernel/src/objects/cycle.rs index 78fda9ad8..518064a22 100644 --- a/crates/fj-kernel/src/objects/cycle.rs +++ b/crates/fj-kernel/src/objects/cycle.rs @@ -24,20 +24,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" - ); - } + // 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" + ); if half_edges.len() != 1 { // Verify that all edges connect. From 28b2377eb83adb2dc3beab538cfb46feb789babf Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 8 Nov 2022 12:12:57 +0100 Subject: [PATCH 12/15] Use concrete type as return value I'm currently writing some code that requires more from that type than just `Iterator`. Using a concrete type seems more practical than specifying all kinds of additional bounds. --- crates/fj-kernel/src/objects/cycle.rs | 9 ++++++++- crates/fj-kernel/src/objects/mod.rs | 2 +- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/crates/fj-kernel/src/objects/cycle.rs b/crates/fj-kernel/src/objects/cycle.rs index 518064a22..7a5e12769 100644 --- a/crates/fj-kernel/src/objects/cycle.rs +++ b/crates/fj-kernel/src/objects/cycle.rs @@ -1,3 +1,5 @@ +use std::slice; + use fj_interop::ext::SliceExt; use fj_math::{Scalar, Winding}; use pretty_assertions::assert_eq; @@ -76,7 +78,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() } @@ -138,3 +140,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 bdba01e72..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, From 2b97a6732e9ab7cba13eac08bd5acd534375130c Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 8 Nov 2022 12:40:05 +0100 Subject: [PATCH 13/15] Add dependency on `itertools` to `fj-kernel` --- Cargo.lock | 10 ++++++++++ crates/fj-kernel/Cargo.toml | 1 + 2 files changed, 11 insertions(+) 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" From 1e8c8eeaa4d736b1825cf2b8300c874ef39c3f0b Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 8 Nov 2022 14:19:27 +0100 Subject: [PATCH 14/15] Move validation check to new infrastructure --- crates/fj-kernel/src/objects/cycle.rs | 29 -------- crates/fj-kernel/src/validate/cycle.rs | 97 +++++++++++++++++++++++++- 2 files changed, 95 insertions(+), 31 deletions(-) diff --git a/crates/fj-kernel/src/objects/cycle.rs b/crates/fj-kernel/src/objects/cycle.rs index 7a5e12769..412f07b50 100644 --- a/crates/fj-kernel/src/objects/cycle.rs +++ b/crates/fj-kernel/src/objects/cycle.rs @@ -2,7 +2,6 @@ use std::slice; use fj_interop::ext::SliceExt; use fj_math::{Scalar, Winding}; -use pretty_assertions::assert_eq; use crate::{path::SurfacePath, storage::Handle}; @@ -35,34 +34,6 @@ impl Cycle { "Cycle must contain at least one half-edge" ); - 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" - ); - } - } - Self { half_edges } } diff --git a/crates/fj-kernel/src/validate/cycle.rs b/crates/fj-kernel/src/validate/cycle.rs index 83ce06b68..3e528b68c 100644 --- a/crates/fj-kernel/src/validate/cycle.rs +++ b/crates/fj-kernel/src/validate/cycle.rs @@ -1,4 +1,9 @@ -use crate::objects::Cycle; +use itertools::Itertools; + +use crate::{ + objects::{Cycle, SurfaceVertex}, + storage::Handle, +}; use super::{Validate2, ValidationConfig}; @@ -9,10 +14,98 @@ impl Validate2 for Cycle { &self, _: &ValidationConfig, ) -> Result<(), Self::Error> { + CycleValidationError::check_half_edge_connections(self)?; Ok(()) } } /// [`Cycle`] validation error #[derive(Debug, thiserror::Error)] -pub enum CycleValidationError {} +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(()) + } +} From 39f700374e22a44ed92bf3769dd3f7a1007b9f92 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 8 Nov 2022 11:46:30 +0100 Subject: [PATCH 15/15] Add comments --- crates/fj-kernel/src/validate/cycle.rs | 5 +++++ crates/fj-kernel/src/validate/edge.rs | 5 +++++ 2 files changed, 10 insertions(+) diff --git a/crates/fj-kernel/src/validate/cycle.rs b/crates/fj-kernel/src/validate/cycle.rs index 3e528b68c..22603cd9f 100644 --- a/crates/fj-kernel/src/validate/cycle.rs +++ b/crates/fj-kernel/src/validate/cycle.rs @@ -15,6 +15,11 @@ impl Validate2 for Cycle { _: &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(()) } } 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(()) } }