Merge pull request #1983 from brett19/fix/loops-turn-normal

fix: opt-in loop ordering turns about one normal per list (WR-591, fork b0ee3f9)

crates/solidcore/src/kernel.rs

@@ -316,7 +316,7 @@ pub struct UpstreamFixes {
     pub revolve_holed_profile_void: bool,
     /// A partial-turn rotational sweep first splits each profile curve where it meets the axis inside itself (`IntersectWithLine` on the axis at the B-rep tolerance, as `CheckCurveSweep` finds them), joining a closed curve's last piece back onto its first; upstream's `CheckCurveSweep` refuses such a curve (`SM_SC_ROT_ON_CURVE`) and the sweep fails with `SM_ERR`, so a whole circle tangent to the axis (a horn torus) turned part way fails unless its seam lies on the axis (WR-590; the fork's tangency-labs/smlib@e4f1377).
     pub revolve_splits_curves_on_axis: bool,
-    /// `SmTrimmingTools::OrderCurvesIntoLoops`, at a vertex where several curve ends meet (a hole whose corner touches the outline), goes on from the end (of a curve it has not used, or its first curve's start) whose leaving tangent comes next round the vertex from the arriving curve's, about the plane those tangents span (ends leaving tangent to each other in the order they bend). Upstream takes the first such end by index in the input, so the input order decides whether the loop closes, ends on a curve it already used (`SM_ERR`) or runs round the hole the wrong way, crossing itself and counting the hole as material (WR-591; the fork's tangency-labs/smlib@9209e76).
+    /// `SmTrimmingTools::OrderCurvesIntoLoops`, at a vertex where several curve ends meet (a hole whose corner touches the outline), goes on from the end (of a curve it has not used, or its first curve's start) whose leaving tangent comes next round the vertex from the arriving curve's, counterclockwise about one normal for the whole list, from the curves' area vectors and turned against the loops' area, so loops touching from outside each other close apart (ends leaving tangent to each other in the order they bend; tangency-labs/smlib@b0ee3f9). Upstream takes the first such end by index in the input, so the input order decides whether the loop closes, ends on a curve it already used (`SM_ERR`) or runs round the hole the wrong way, crossing itself and counting the hole as material (WR-591; the fork's tangency-labs/smlib@9209e76).
     pub loops_go_on_round_the_vertex: bool,
     /// `SmTrimmingTools::MirrorCurve` keeps the Euclidean control points `GetCanonical`
     /// returns instead of dividing each one by its weight a second time, so a rational UV

crates/solidcore/src/topology/trimming_tools.rs

@@ -38,7 +38,8 @@
 //!   SolidCraft change, tangency-labs/smlib 9209e76).** Where several curve ends meet
 //!   at a vertex, [`order_curves_into_loops`] goes on along the end next round the
 //!   vertex from the arriving curve (`next_round_the_vertex`, the fork's
-//!   `sm_NextRoundTheVertex`) instead of the first by index.
+//!   `sm_NextRoundTheVertex`) instead of the first by index, turning about one normal
+//!   for the whole list (`loops_turn_normal`, tangency-labs/smlib b0ee3f9).
 //! - **Object model.** Faces, loops and edge uses are ids into a [`Brep`]; the static
 //!   methods are free functions taking the B-rep (and the [`Kernel`] where tolerances or
 //!   solves are read). Out-params are return values. A UV trim curve is the
@@ -1488,9 +1489,11 @@ fn eval_point(k: &Kernel, c: &Curve, t: f64) -> Result<Point3> {
 /// its end), `close` the start of the loop's first curve, `found` the nearest end to
 /// `arrive`. The ends at that vertex (within `tol_3d` of `found`) that the loop can go
 /// on from are those of curves it has not used, and `close`. Of them it takes the one
-/// whose leaving tangent comes next round the vertex from the arriving curve's (about
-/// the plane those tangents span; ends leaving tangent to each other in the order they
-/// bend), so the loop never crosses itself there. Returns the end to go on from.
+/// whose leaving tangent comes next round the vertex from the arriving curve's,
+/// counterclockwise about `turn_normal` (unit, one for all the vertices of a call, from
+/// `loops_turn_normal`; zero where the curves lie on one line; tangency-labs/smlib
+/// b0ee3f9), ends leaving tangent to each other in the order they bend, so the loop
+/// never crosses itself there. Returns the end to go on from.
 ///
 /// # Errors
 ///
@@ -1500,6 +1503,7 @@ fn next_round_the_vertex(
     curves: &[Curve],
     ends: &[Point3],
     seen: &[bool],
+    turn_normal: Vec3,
     arrive: usize,
     close: usize,
     tol_3d: f64,
@@ -1538,7 +1542,7 @@ fn next_round_the_vertex(
         let tangent = if at_start { d1 } else { -1.0 * d1 };
         Ok((tangent, at(&d, 2)?))
     };
-    let (mut back, back_acc) = leaving(arrive)?;
+    let (mut back, _) = leaving(arrive)?;
     if back.length() <= 0.0 {
         return Err(SolidError::sm_err(OP));
     }
@@ -1555,35 +1559,11 @@ fn next_round_the_vertex(
         accs.push(acc);
     }
 
-    // the curves' plane: its normal from two rays there that are not parallel, else
-    // from the arriving curve's bending
-    let small = |v: Vec3| v.length() <= EFF_ZERO_SQRT;
-    let mut normal = Vec3::new(0.0, 0.0, 0.0);
-    for ray in &rays {
-        if !small(normal) {
-            break;
-        }
-        normal = back.cross(*ray / ray.length());
-    }
-    for (ii, ri) in rays.iter().enumerate() {
-        if !small(normal) {
-            break;
-        }
-        for rj in rays.iter().skip(ii + 1) {
-            if !small(normal) {
-                break;
-            }
-            normal = (*ri / ri.length()).cross(*rj / rj.length());
-        }
-    }
-    if small(normal) {
-        normal = back.cross(back_acc);
-    }
-    if small(normal) {
-        // every curve there on one line: they cannot touch from two sides
+    if turn_normal.length() <= 0.0 {
+        // every curve on one line: they cannot touch from two sides
         return Ok(found);
     }
-    normal /= normal.length();
+    let normal = turn_normal;
 
     // each ray's angle round the vertex from the arriving one, (0, 2 pi], and for rays
     // leaving along one line, its bending towards larger angles (curvature about the
@@ -1612,6 +1592,57 @@ fn next_round_the_vertex(
     at(&candidates, best)
 }
 
+/// Not upstream (WR-591, the fork's turn normal in `OrderCurvesIntoLoops`,
+/// tangency-labs/smlib b0ee3f9): the unit normal [`next_round_the_vertex`] turns about,
+/// one for the whole list so the way a loop goes on at a vertex does not depend on the
+/// order of the ends there. Each curve's samples `p_k` (at quarters of its natural
+/// interval) about the curves' centre `c` add `(p_k - c) x (p_k+1 - c)`: over closed
+/// loops twice the sum of their area vectors, whatever the list's order. Each term is
+/// square to the curves' plane, so the terms each taken the way of the largest give
+/// the plane even where the areas cancel; the normal is turned against the loops'
+/// area (a loop turns clockwise about it, keeping round the region its curves bound),
+/// so loops touching from outside each other close apart. Zero when every term is.
+///
+/// # Errors
+///
+/// Those of the curve evaluations.
+fn loops_turn_normal(k: &Kernel, curves: &[Curve], center: Point3) -> Result<Vec3> {
+    let mut terms = Vec::new();
+    let mut area = Vec3::new(0.0, 0.0, 0.0);
+    let mut largest = Vec3::new(0.0, 0.0, 0.0);
+    for c in curves {
+        let iv = c.natural_interval();
+        let mut prev = eval_point(k, c, iv.min())?;
+        for jj in 1..=4 {
+            let pt = eval_point(k, c, iv.evaluate(0.25 * f64::from(jj)))?;
+            let term = (prev - center).cross(pt - center);
+            terms.push(term);
+            area += term;
+            if term.length() > largest.length() {
+                largest = term;
+            }
+            prev = pt;
+        }
+    }
+    let mut plane = Vec3::new(0.0, 0.0, 0.0);
+    for term in &terms {
+        plane += if term.dot(largest) < 0.0 {
+            -1.0 * *term
+        } else {
+            *term
+        };
+    }
+    let plane_len = plane.length();
+    let mut normal = Vec3::new(0.0, 0.0, 0.0);
+    if plane_len > 0.0 {
+        normal = plane / plane_len;
+        if area.dot(normal) > EFF_ZERO_SQRT * plane_len {
+            normal = -1.0 * normal;
+        }
+    }
+    Ok(normal)
+}
+
 /// `OrderCurvesIntoLoops`: orders `curves` into closed loops by matching their end
 /// points (within `max(tol_3d, radius/1000)` of the curves' bounding sphere), computes the
 /// plane through all loops (average loop point, unit normal from the loops' polygons)
@@ -1684,6 +1715,14 @@ pub fn order_curves_into_loops(k: &Kernel, curves: &[Curve], tol_3d: f64) -> Res
     let face_radius = out.curves_box.sphere_bound().radius;
     let feature_size = face_radius / 1000.0;
 
+    // Not upstream (WR-591, opt-in; tangency-labs/smlib b0ee3f9): the normal the loops
+    // turn about where several curve ends meet, one for the whole call
+    let turn_normal = if k.fixes.loops_go_on_round_the_vertex {
+        loops_turn_normal(k, curves, out.curves_box.sphere_bound().center)?
+    } else {
+        Vec3::new(0.0, 0.0, 0.0)
+    };
+
     let mut plane_base_count: usize = 0;
 
     // start-next-loop while block: find a curve not yet used in a loop
@@ -1771,6 +1810,7 @@ pub fn order_curves_into_loops(k: &Kernel, curves: &[Curve], tol_3d: f64) -> Res
                     curves,
                     &vertex_points,
                     &seen,
+                    turn_normal,
                     target,
                     2 * first_curve_index,
                     tol_3d,

crates/solidcore/tests/wr_591_loops_touching_at_a_vertex.rs

@@ -157,3 +157,49 @@ fn wr_591_loops_apart_are_unchanged() {
     assert_eq!(on.orients, off.orients);
     assert!(on.closed && off.closed);
 }
+
+/// The 32 input orders of two unit squares touching at the corner (1, 1) (the fork's
+/// tangency-labs/smlib@b0ee3f9, capi test `loops_touching_from_outside_close_apart`):
+/// both counterclockwise or both clockwise, either square's lines first, every
+/// rotation of the list.
+fn squares_orders() -> Vec<(bool, bool, usize, Vec<Curve>)> {
+    const LOW: [[f64; 2]; 4] = [[0.0, 0.0], [1.0, 0.0], [1.0, 1.0], [0.0, 1.0]];
+    const HIGH: [[f64; 2]; 4] = [[1.0, 1.0], [2.0, 1.0], [2.0, 2.0], [1.0, 2.0]];
+    let mut out = Vec::new();
+    for clockwise in [false, true] {
+        for high_first in [false, true] {
+            let (a, b) = if high_first { (HIGH, LOW) } else { (LOW, HIGH) };
+            let mut lines = polygon(&a, clockwise);
+            lines.extend(polygon(&b, clockwise));
+            let n = lines.len();
+            for turn in 0..n {
+                let curves = (0..n)
+                    .map(|k| {
+                        let (a, b) = lines[(k + turn) % n];
+                        Curve::from(create_line_segment(a, b).unwrap())
+                    })
+                    .collect();
+                out.push((clockwise, high_first, turn, curves));
+            }
+        }
+    }
+    out
+}
+
+/// WR-591 addendum (fork b0ee3f9): loops touching at a vertex from outside each
+/// other close apart, two loops of area 1, in every order. Turning about a normal
+/// taken from the rays at the vertex in list order gave the figure-eight (one loop
+/// of area 2) in 12 of the 32 orders.
+#[test]
+fn wr_591_loops_touching_from_outside_close_apart() {
+    let mut k = Kernel::default();
+    k.fixes.loops_go_on_round_the_vertex = true;
+    let mut bad = Vec::new();
+    for (clockwise, high_first, turn, curves) in squares_orders() {
+        match loop_areas(&k, &curves) {
+            Some(areas) if areas.len() == 2 && areas.iter().all(|a| (a - 1.0).abs() < 1e-12) => {}
+            other => bad.push((clockwise, high_first, turn, other)),
+        }
+    }
+    assert!(bad.is_empty(), "{} of 32 orders: {bad:?}", bad.len());
+}

plan/upstream-issues/WR-591--addendum-2026-10-31-one-turn-normal-per-list.md

@@ -0,0 +1,9 @@
+Updates: WR-591
+
+#### WR-591 (update, 2026-10-31). The fix turns about one normal for the whole list, so loops touching from outside close apart in every order
+- **What happens (with the 9209e76 fix alone):** `next_round_the_vertex` took the normal it turns about at a vertex from the first pair of rays there that are not parallel, in the order of the curve ends in the list. Its sign changed with that order, so where two loops touch at a vertex from outside each other, `OrderCurvesIntoLoops` gave the two loops or one loop through the vertex twice round both (a figure-eight), depending on how the list was rotated.
+- **Trigger:** `OrderCurvesIntoLoops` (tolerance `1e-6`, `loops_go_on_round_the_vertex` on) of the unit squares (0, 0)-(1, 1) and (1, 1)-(2, 2), touching at (1, 1), as eight lines: both counterclockwise or both clockwise, either square's lines first, every rotation of the list (32 orders). 12 of them gave one loop of area 2 instead of two loops of area 1.
+- **How found:** SolidCraft; fixed in the fork as tangency-labs/smlib@b0ee3f9 ("loops: where loops touch at a vertex, turn about one normal for the whole list").
+- **Confirmed:** yes, in the port with the flag on. The pinned C++ has no round-the-vertex step, so this is a defect of the fork's first fix, not of upstream.
+
+The same opt-in field, `Kernel::fixes.loops_go_on_round_the_vertex`, now also takes the fork's follow-up: before it walks, `order_curves_into_loops` (`crates/solidcore/src/topology/trimming_tools.rs`) computes one turn normal (`loops_turn_normal`): each curve's samples `p_k` at quarters of its natural interval, about the curves' bounding-sphere centre `c`, add `(p_k - c) x (p_k+1 - c)`; the terms each taken the way of the largest give the plane, and the normal is turned against the loops' summed area when that is not negligible. `next_round_the_vertex` turns counterclockwise about that normal (and keeps the found end when it is zero) instead of a normal from the rays at the vertex. Default off: the pinned result is unchanged. `crates/solidcore/tests/wr_591_loops_touching_at_a_vertex.rs` (`wr_591_loops_touching_from_outside_close_apart`) checks the 32 orders give two loops of area 1 each; the 24 orders of the original record still close soundly.