Move cycle validation to new infrastructure

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
- .position()
- .expect("Need surface position to extend cycle");
- let position_next = vertex_next
- .position()
- .expect("Need surface position to extend cycle");
-
- let from = vertex_prev;
- let to = vertex_next;
+ let [position_prev, position_next] =
+ [&vertex_prev, &vertex_next].map(|vertex| {
+ vertex
+ .position()
+ .expect("Need surface position to extend cycle")
+ });
 
- 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;

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() {
- 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<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,
- 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<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,8 +67,14 @@ 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(_)) => {
- 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<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()
- .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));

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,