Merge pull request #2004 from brett19/fix/plane-recognition-origin-1056

fix: opt-in plane recognition origin after axis correction (WR-652, fork 0286682; takeover of #1111)

crates/solidcore/src/kernel.rs

@@ -145,6 +145,9 @@ impl Caches {
 #[allow(clippy::struct_excessive_bools)]
 #[non_exhaustive]
 pub struct UpstreamFixes {
+    /// `SmPlane::IsNurbSurfacePlane` places a recognised surface's origin after
+    /// orthogonalising its axes (WR-652, upstream issue #1056).
+    pub plane_recognition_origin_after_axes: bool,
     /// `SmGapFunction::BuildSampleSet` samples the B-spline spans inside the requested
     /// curve interval or surface domain, normalizing against that interval and keeping
     /// its exact endpoints. Upstream starts at the first knot and normalizes against

crates/solidcore/src/surface/plane/p2.rs

@@ -46,6 +46,10 @@
 //!   `SmBoolean` (`SER`): upstream's "`TRUE` without plane" is an `Err` here.
 //!   `SmMatrix::SolveLinearSystem`'s near-singular warning, which upstream returns
 //!   through `SER`, is [`SolidError::Upstream`] with `SM_ERR_WARNING`.
+//! - **Upstream fix WR-652 / #1056.** The opt-in
+//!   [`crate::kernel::UpstreamFixes::plane_recognition_origin_after_axes`] places a
+//!   recognised plane's origin using its final orthogonal axes. It is off by default
+//!   to preserve SMLib's original arithmetic.
 //! - **Recognisers** after the plane in `GlobalSurfaceIntersect`: the surface of
 //!   revolution's and the cone's (`SmCone::IntersectWithPlane`, also for a cylinder)
 //!   special cases run; the sphere and extrusion need surface kinds not in
@@ -418,7 +422,10 @@ impl Plane {
     /// near the centre (angle tolerance `tolerance_scale * 1e-4` degrees), and ten test
     /// points lie within `SM_EFF_ZERO_SQRT * (1 + |origin|)` of the plane built from
     /// the `(0, 0)` derivatives. The plane is re-parameterised to the natural domain
-    /// of `test` (which makes its NURB). `None` when `test` is not a plane.
+    /// of `test` (which makes its NURB). `None` when `test` is not a plane. By default,
+    /// the origin is computed before orthogonalizing the Y axis, matching SMLib. Set
+    /// [`crate::kernel::UpstreamFixes::plane_recognition_origin_after_axes`] to use the
+    /// final axes and avoid magnifying a small adjustment across a distant UV minimum.
     ///
     /// # Errors
     ///
@@ -507,7 +514,8 @@ impl Plane {
         let mut y_axis = y_axis.unitize_or_z();
 
         // Move the origin to correspond to the zero [u=0, v=0] parameter.
-        let new_origin = origin
+        // Keep this calculation before axis orthogonalization for upstream-default parity.
+        let mut new_origin = origin
             - uv_domain.min().x * x_axis * uv_scale.x
             - uv_domain.min().y * y_axis * uv_scale.y;
 
@@ -517,6 +525,14 @@ impl Plane {
             y_axis = y_axis.unitize().map_err(|_| SolidError::sm_err(OP_NURB))?;
         }
 
+        // The opt-in upstream fix anchors the origin using the final axes, so a nonzero
+        // domain minimum does not amplify the small Y-axis adjustment.
+        if k.fixes.plane_recognition_origin_after_axes {
+            new_origin = origin
+                - uv_domain.min().x * x_axis * uv_scale.x
+                - uv_domain.min().y * y_axis * uv_scale.y;
+        }
+
         // construct temporary new plane (m_pNurb == NULL)
         let mut plane = Self::new(new_origin, x_axis, y_axis, uv_scale, &uv_domain);
 
@@ -1571,6 +1587,19 @@ pub(crate) fn global_surface_intersect_into<O: SolutionObject + Copy + PartialEq
 mod tests {
     use super::*;
 
+    fn skewed_bilinear(domain: Extent2d, shear: f64) -> BSplineSurface {
+        let dx = 40.0 * shear;
+        let mut surface = BSplineSurface::create_bilinear_surface(
+            Vec3::ZERO,
+            Vec3::new(2.0, 0.0, 0.0),
+            Vec3::new(dx, 40.0, 0.0),
+            Vec3::new(2.0 + dx, 40.0, 0.0),
+        )
+        .unwrap();
+        surface.reparameterize(&domain).unwrap();
+        surface
+    }
+
     fn rect() -> Plane {
         let mut p = Plane::new(
             Vec3::ZERO,
@@ -1644,6 +1673,104 @@ mod tests {
         assert!(Plane::is_nurb_surface_plane(k, &twisted, 1.0)
             .unwrap()
             .is_none());
+        let mut fixed_kernel = Kernel::default();
+        fixed_kernel.fixes.plane_recognition_origin_after_axes = true;
+        assert!(Plane::is_nurb_surface_plane(&fixed_kernel, &twisted, 1.0)
+            .unwrap()
+            .is_none());
+    }
+
+    #[test]
+    fn plane_origin_after_axes_fix_is_opt_in_and_domain_local() {
+        const SHEAR: f64 = 3.2e-8;
+        let domain_cases = [
+            Extent2d::from_bounds(2.0, 4.0, 3.0, 5.0),
+            Extent2d::from_bounds(2.0, -5.0, 3.0, -4.0),
+        ];
+        assert!(!Kernel::default().fixes.plane_recognition_origin_after_axes);
+
+        for domain in domain_cases {
+            let surface = skewed_bilinear(domain, SHEAR);
+            assert_eq!(surface.natural_uv_domain(), domain);
+            let lower_uv = domain.min();
+            let upper_uv = domain.max();
+            let expected_lower = surface.evaluate_point(lower_uv).unwrap();
+
+            let default_kernel = Kernel::default();
+            let default_plane = Plane::is_nurb_surface_plane(&default_kernel, &surface, 1.0)
+                .unwrap()
+                .unwrap();
+            let default_lower_error = default_plane.evaluate_point(lower_uv) - expected_lower;
+            let expected_shift = -domain.min().y * 40.0 * SHEAR;
+            assert!((default_lower_error.x - expected_shift).abs() < 1.0e-12);
+            assert!(default_lower_error.y.abs() < 1.0e-12);
+            assert_eq!(default_plane.natural_uv_domain(), domain);
+            assert_eq!(default_plane.step_uv_domain(), domain);
+            assert!((default_plane.uv_scale().x - 2.0).abs() < 1.0e-12);
+            assert!((default_plane.uv_scale().y - 40.0).abs() < 1.0e-12);
+
+            let mut fixed_kernel = Kernel::default();
+            fixed_kernel.fixes.plane_recognition_origin_after_axes = true;
+            let fixed_plane = Plane::is_nurb_surface_plane(&fixed_kernel, &surface, 1.0)
+                .unwrap()
+                .unwrap();
+            assert_eq!(fixed_plane.natural_uv_domain(), domain);
+            assert_eq!(fixed_plane.step_uv_domain(), domain);
+            assert_eq!(fixed_plane.uv_scale(), default_plane.uv_scale());
+            assert!(
+                fixed_plane
+                    .evaluate_point(lower_uv)
+                    .distance(expected_lower)
+                    < 1.0e-10
+            );
+            assert!(fixed_plane
+                .position()
+                .x_axis()
+                .is_perpendicular_to(fixed_plane.position().y_axis(), 1.0e-10));
+            assert!((fixed_plane.position().x_axis().length() - 1.0).abs() < 1.0e-12);
+            assert!((fixed_plane.position().y_axis().length() - 1.0).abs() < 1.0e-12);
+
+            let expected_upper = surface.evaluate_point(upper_uv).unwrap();
+            let upper_residual = fixed_plane.evaluate_point(upper_uv) - expected_upper;
+            assert!((upper_residual.x + 40.0 * SHEAR).abs() < 1.0e-12);
+            assert!(upper_residual.y.abs() < 1.0e-12);
+        }
+
+        // With a zero minimum, recomputing the origin with the final axis is identical.
+        let zero_domain = Extent2d::from_bounds(0.0, 0.0, 1.0, 1.0);
+        let surface = skewed_bilinear(zero_domain, SHEAR);
+        let baseline = Plane::is_nurb_surface_plane(&Kernel::default(), &surface, 1.0)
+            .unwrap()
+            .unwrap();
+        let mut fixed_kernel = Kernel::default();
+        fixed_kernel.fixes.plane_recognition_origin_after_axes = true;
+        let fixed = Plane::is_nurb_surface_plane(&fixed_kernel, &surface, 1.0)
+            .unwrap()
+            .unwrap();
+        assert_eq!(baseline.position(), fixed.position());
+        assert_eq!(baseline.uv_scale(), fixed.uv_scale());
+        assert_eq!(baseline.step_uv_domain(), fixed.step_uv_domain());
+        assert_eq!(baseline.natural_uv_domain(), fixed.natural_uv_domain());
+        for uv in [zero_domain.min(), zero_domain.max()] {
+            assert_eq!(baseline.evaluate_point(uv), fixed.evaluate_point(uv));
+        }
+
+        // An already orthogonal frame is also unaffected by enabling the correction.
+        let orthogonal_domain = Extent2d::from_bounds(2.0, 4.0, 3.0, 5.0);
+        let orthogonal = skewed_bilinear(orthogonal_domain, 0.0);
+        let baseline = Plane::is_nurb_surface_plane(&Kernel::default(), &orthogonal, 1.0)
+            .unwrap()
+            .unwrap();
+        let fixed = Plane::is_nurb_surface_plane(&fixed_kernel, &orthogonal, 1.0)
+            .unwrap()
+            .unwrap();
+        assert_eq!(baseline.position(), fixed.position());
+        assert_eq!(baseline.uv_scale(), fixed.uv_scale());
+        assert_eq!(baseline.step_uv_domain(), fixed.step_uv_domain());
+        assert_eq!(baseline.natural_uv_domain(), fixed.natural_uv_domain());
+        for uv in [orthogonal_domain.min(), orthogonal_domain.max()] {
+            assert_eq!(baseline.evaluate_point(uv), fixed.evaluate_point(uv));
+        }
     }
 
     #[test]

crates/solidcore/tests/issue_1056_plane_recognition_origin_after_axes.rs

@@ -0,0 +1,95 @@
+//! #1056 (WR-652, the fork's tangency-labs/smlib@0286682): `Plane::is_nurb_surface_plane`
+//! (`SmPlane::IsNurbSurfacePlane`) placed a recognised plane's origin, its point at
+//! (u, v) = (0, 0), along the B-spline's own Y axis, and only then turned that axis to
+//! be perpendicular to X. With a domain far from v = 0 the turn is multiplied by the
+//! distance to the origin.
+//!
+//! The case is SolidCraft rig sweeps/4305's swept groove wall: a planar bilinear patch
+//! over u 0..1, v 4..5, 40 mm along v, whose v direction leans `TURN = 3.2116e-8` rad
+//! off perpendicular to u. Closed form (to first order in `TURN`, the second-order terms
+//! are below 1e-14 mm):
+//! - flag off (upstream): the corner at v = 4 is `4 * 40 * TURN = 5.1386e-6` mm off the
+//!   wall and the corner at v = 5 is `200 * TURN = 6.4232e-6` mm off. Those are the
+//!   vertex/face gaps the SolidCraft check refused against its 5e-6 limit.
+//! - flag on (fixed): the corner at v = 4 is on the wall and the corner at v = 5 is off
+//!   by the wall's own span times the turn, `40 * TURN = 1.2846e-6` mm. This is the
+//!   analytic limit, since an orthogonal plane cannot follow the lean exactly.
+#![allow(
+    clippy::unwrap_used,
+    clippy::expect_used,
+    clippy::panic,
+    clippy::indexing_slicing
+)] // test helpers
+use solidcore::geom::extent2d::Extent2d;
+use solidcore::math::{Point2, Point3};
+use solidcore::nurbs::b_spline_surface::BSplineSurface;
+use solidcore::surface::plane::Plane;
+use solidcore::Kernel;
+
+/// The profile's turn where the path's rounded arcs meet its lines (rad).
+const TURN: f64 = 3.2116e-8;
+/// The wall's length along v (mm).
+const SPAN: f64 = 40.0;
+/// Rounding slack for coordinates of size ~200 mm (a few hundred ulps, far below the gaps).
+const SLACK: f64 = 1.0e-12;
+
+/// The sweeps/4305 wall: 1 mm along u, `SPAN` along v leaning by `TURN`, on u 0..1, v 4..5.
+fn groove_wall() -> BSplineSurface {
+    let lean = SPAN * TURN.tan();
+    let mut wall = BSplineSurface::create_bilinear_surface(
+        Point3::new(0.0, 0.0, 0.0),
+        Point3::new(1.0, 0.0, 0.0),
+        Point3::new(lean, SPAN, 0.0),
+        Point3::new(1.0 + lean, SPAN, 0.0),
+    )
+    .unwrap();
+    wall.reparameterize(&Extent2d::from_bounds(0.0, 4.0, 1.0, 5.0))
+        .unwrap();
+    wall
+}
+
+/// The recognised plane's distance from the wall at the corners (0, 4) and (0, 5).
+fn corner_gaps(k: &Kernel) -> (f64, f64) {
+    let wall = groove_wall();
+    let plane = Plane::is_nurb_surface_plane(k, &wall, 1.0)
+        .unwrap()
+        .expect("the wall is a plane");
+    let gap = |uv: Point2| {
+        plane
+            .evaluate_point(uv)
+            .distance(wall.evaluate_point(uv).unwrap())
+    };
+    (gap(Point2::new(0.0, 4.0)), gap(Point2::new(0.0, 5.0)))
+}
+
+#[test]
+fn flag_off_places_the_origin_along_the_unturned_axis() {
+    let k = Kernel::default();
+    assert!(!k.fixes.plane_recognition_origin_after_axes);
+    let (low, high) = corner_gaps(&k);
+    assert!(
+        (low - 4.0 * SPAN * TURN).abs() < SLACK,
+        "v = 4 corner gap {low:e}, expected {:e}",
+        4.0 * SPAN * TURN
+    );
+    assert!(
+        (high - 5.0 * SPAN * TURN).abs() < SLACK,
+        "v = 5 corner gap {high:e}, expected {:e}",
+        5.0 * SPAN * TURN
+    );
+    // Over SolidCraft's 5e-6 vertex gap limit, as the refused cut reported.
+    assert!(low > 5.0e-6 && high > 5.0e-6);
+}
+
+#[test]
+fn flag_on_places_the_origin_along_the_plane_axes() {
+    let mut k = Kernel::default();
+    k.fixes.plane_recognition_origin_after_axes = true;
+    let (low, high) = corner_gaps(&k);
+    assert!(low < SLACK, "v = 4 corner gap {low:e}, expected 0");
+    assert!(
+        (high - SPAN * TURN).abs() < SLACK,
+        "v = 5 corner gap {high:e}, expected {:e}",
+        SPAN * TURN
+    );
+}

plan/findings/surface.plane_p2--opt-in-recognition-origin-after-axes.md

@@ -0,0 +1,11 @@
+# surface.plane_p2: Opt-in origin correction for plane recognition (2026-10-26)
+
+- **Unit:** `surface.plane_p2`, `Plane::is_nurb_surface_plane`.
+- **Issue:** SolidCore #1056; upstream source defect WR-652 in `plan/upstream-issues/WR-652--plane-recognition-origin-before-axis-correction.md`.
+- **Inputs:** A planar bilinear B-spline with corners `(0,0,0)`, `(2,0,0)`, `(1.28e-6,40,0)`, `(2.00000128,40,0)`, reparameterised to U `[2,3]` and V `[4,5]` or `[-5,-4]`. The upper corner is used when the recogniser calculates its point tolerance, giving about 41e-6 mm so both variants can return planes whose displacement can be checked.
+- **Values:** Source arithmetic predicts lower-corner X errors of `-5.12e-6` and `+6.4e-6` mm respectively in the default path. With `Kernel::fixes.plane_recognition_origin_after_axes = true`, that corner is anchored. The upper corner retains about `-1.28e-6` mm X residual because the orthogonal plane cannot represent the original small parameter shear exactly. These analytic expectations were exercised by the Rust regressions at published head `19c3188442d230642bf9ee5406649191631eaf0b`; they are not opt-in Rust/C++ parity measurements, because pinned SMLib retains the default behavior.
+- **Cause:** SMLib computes the new origin with the pre-correction Y axis but evaluates the plane with the corrected axis. The nonzero V minimum multiplies the axis difference.
+- **Rust choice:** Preserve original arithmetic with the default-false flag; opt in to recomputing the origin after the existing axis correction. This intentionally differs from the pinned C++ behavior only when enabled.
+- **Initial preparation (historical):** Source and regressions were prepared against `002754f4ea652fb864527029a67ee9dcfeac01c6` before the local toolchain and C++ reference were restored. That source-only checkpoint was not test evidence.
+- **Subsequent exact-head validation:** Published PR #1111 head `19c3188442d230642bf9ee5406649191631eaf0b` passed formatting, strict Clippy, WebAssembly, 2,294 debug kernel tests, 2,824 optimized workspace tests (two existing ignored), all eight plane tests, and 2,228 substantive default-mode native comparisons. Its per-unit exact-base ratchet and 2,228/2,228 perturbations passed. The opt-in correction is covered by Rust regressions; native comparisons preserve default SMLib behavior. These results belong to that historical head, not the later main-merged candidate.
+- **Remaining gate:** Candidate-specific Linux validation and required upstream CI/review remain separate gates. The approximately 1.28e-6 mm shear residual remains, and full Boolean/groove acceptance is not established.

plan/upstream-issues/WR-652--plane-recognition-origin-before-axis-correction.md

@@ -0,0 +1,12 @@
+#### WR-652. `SmPlane::IsNurbSurfacePlane` offsets the origin before correcting its axes (#1056)
+- **Severity:** Wrong result
+- **Area:** Surfaces and volumes
+- **Location:** `source/SMLib/src/SmPlane.cpp:2833-2845`, `SmPlane::IsNurbSurfacePlane` (pinned v1.0.1 `3fd5b95`; also in `c9c5979`).
+- **What happens:** The recognised plane uses an origin calculated with the original Y axis, then constructs a plane with the perpendicularised Y axis. Its evaluated point at the input surface's lower UV corner is displaced along X by approximately `-v_min * v_scale * shear` for a small X component in the Y tangent. The correction can also change the residual error across the rest of the patch; it does not make a sheared parameterisation exactly representable by an orthogonal plane.
+- **Trigger:** A nearly perpendicular planar bilinear surface with a nonzero V-domain minimum. For a 40 mm V span, shear `3.2e-8`, and V minimum 4, the analytic corner displacement is `-5.12e-6` mm. A V minimum of -5 gives `+6.4e-6` mm.
+- **How found:** Read during porting and reported in SolidCore issue #1056, based on a SolidCraft swept-groove regression.
+- **Initial confirmation (historical):** Source-only at preparation time; pinned C++ shows this operation order.
+- **Subsequent validation:** PR #1111 published head `19c3188442d230642bf9ee5406649191631eaf0b` passed all eight Rust plane tests, including the opt-in origin/domain regression, and 2,228 substantive default-mode native comparisons with exact-base and sensitivity checks. This validates the bounded Rust correction and default parity; it is not a direct C++ numerical reproduction of the shifted-domain opt-in case or full Boolean/groove acceptance. New main-merged source requires its own validation.
+- **Record identity:** Renumbered from the unmerged PR's WR-441, then WR-444, to WR-652 while merging main: main's WR-441 describes a different UV-trim defect and its WR-444 a point-solve defect (#1164).
+- **Fix (fork):** tangency-labs/smlib@0286682 places the origin after `YAxis` is made perpendicular, along the axes the plane is built with. Found from SolidCraft rig sweeps/4305 (a groove swept round a rounded pocket): wall domain v 4..5, 40 mm along v, `YAxis` turned 3.2116e-8 rad, so the corners at v = 4 and v = 5 sat `160 * 3.2116e-8 = 5.14e-6` and `200 * 3.2116e-8 = 6.42e-6` mm off, over the 5e-6 vertex gap limit.
+- **Rust port:** opt-in `Kernel::fixes.plane_recognition_origin_after_axes` (default `false`, upstream's arithmetic) in `Plane::is_nurb_surface_plane` (`crates/solidcore/src/surface/plane/p2.rs`). Regression: `crates/solidcore/tests/issue_1056_plane_recognition_origin_after_axes.rs` (corner gap `v_min * y_scale * turn` with the flag off, at most `y_span * turn` with it on).