Merge pull request #1326 from hannobraun/validate

Move cycle validation to new infrastructure

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"

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"

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
+                let [position_prev, position_next] =
+                    [&vertex_prev, &vertex_next].map(|vertex| {
+                        vertex
                             .position()
-                    .expect("Need surface position to extend cycle");
+                            .expect("Need surface position to extend cycle")
-                let position_next = vertex_next
+                    });
-                    .position()
-                    .expect("Need surface position to extend cycle");
 
-                let from = vertex_prev;
+                previous = Some(vertex_next.clone());
-                let to = vertex_next;
-
-                previous = Some(to.clone());
 
                 let curve = Curve::partial()
                     .with_surface(Some(surface.clone()))
                     .update_as_line_from_points([position_prev, position_next]);
 
-                let [from, to] =
+                let vertices = [(0., vertex_prev), (1., vertex_next)].map(
-                    [(0., from), (1., to)].map(|(position, surface_form)| {
+                    |(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;

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<MaybePartial<Vertex>>) -> Self;
+
+    /// Update the partial half-edge with the given front vertex
+    fn with_front_vertex(self, front: impl Into<MaybePartial<Vertex>>) -> 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<MaybePartial<Vertex>>) -> Self {
+        let [_, front] = self.vertices();
+        self.with_vertices([back.into(), front])
+    }
+
+    fn with_front_vertex(self, front: impl Into<MaybePartial<Vertex>>) -> Self {
+        let [back, _] = self.vertices();
+        self.with_vertices([back, front.into()])
+    }
+
     fn update_as_circle_from_radius(
         self,
         radius: impl Into<Scalar>,
@@ -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`]

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<Item = Handle<HalfEdge>>) -> Self {
         let half_edges = half_edges.into_iter().collect::<Vec<_>>();
 
-        let surface = match half_edges.first() {
+        // This is not a validation check, and thus not part of the validation
-            Some(half_edge) => half_edge.surface().clone(),
+        // infrastructure. The property being checked here is inherent to the
-            None => panic!("Cycle must contain at least one half-edge"),
+        // validity of a `Cycle`, as methods of `Cycle` might assume that there
-        };
+        // is at least one edge.
-
+        assert!(
-        // Verify, that the curves of all edges are defined in the correct
+            !half_edges.is_empty(),
-        // surface.
+            "Cycle must contain at least one half-edge"
-        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"
-                );
-            }
-        }
 
         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<Item = &Handle<HalfEdge>> + '_ {
+    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<HalfEdge>>;

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,
+        CycleValidationError, HalfEdgeValidationError,
-        VertexValidationError,
+        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<Handle<Cycle>, Infallible> {
+    pub fn insert(
+        &self,
+        cycle: Cycle,
+    ) -> Result<Handle<Cycle>, CycleValidationError> {
         cycle.validate()?;
         Ok(self.store.insert(cycle))
     }

crates/fj-kernel/src/partial/maybe_partial.rs

@@ -67,9 +67,15 @@ impl<T: HasPartial> MaybePartial<T> {
     /// Merge this `MaybePartial` with another of the same type
     pub fn merge_with(self, other: impl Into<Self>) -> Self {
         match (self, other.into()) {
-            (Self::Full(_), Self::Full(_)) => {
+            (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),
             (Self::Partial(a), Self::Partial(b)) => {
@@ -218,6 +224,14 @@ impl MaybePartial<SurfaceVertex> {
             Self::Partial(partial) => partial.surface(),
         }
     }
+
+    /// Access the global form
+    pub fn global_form(&self) -> MaybePartial<GlobalVertex> {
+        match self {
+            Self::Full(full) => full.global_form().clone().into(),
+            Self::Partial(partial) => partial.global_form(),
+        }
+    }
 }
 
 impl MaybePartial<Vertex> {

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<Handle<Cycle>, 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()
+                PartialHalfEdge::default().with_vertices([
-                    .with_back_vertex(
                     PartialVertex::default().with_surface_form(back_vertex),
-                    )
-                    .with_front_vertex(
                     PartialVertex::default()
                         .with_surface_form(front_vertex.clone()),
-                    ),
+                ]),
             );
 
             previous_vertex = Some(MaybePartial::from(front_vertex));

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<MaybePartial<Vertex>>,
-    ) -> 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<MaybePartial<Vertex>>,
-    ) -> 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,

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<SurfaceVertex>,
+
+        /// The back vertex of the next half-edge
+        next: Handle<SurfaceVertex>,
+    },
+}
+
+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::<Vec<_>>();
+
+            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::<Result<Vec<_>, _>>()?;
+
+            Cycle::new(half_edges)
+        };
+
+        assert!(valid.validate().is_ok());
+        assert!(invalid.validate().is_err());
+
         Ok(())
     }
 }

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(())
     }
 }

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),