Merge pull request #1640 from tangency-labs/fork/0cd9d08-daba678

fix: opt-in torus check reads its V interval and reports a spindle apex (WR-538, WR-539; fork 0cd9d08, daba678)

crates/solidcore/src/geom/assert_array.rs

@@ -269,6 +269,17 @@ impl AssertReport {
     /// (upstream reads out of bounds).
     pub fn new(spec: ReportSpec) -> Result<Self> {
         let list = assert_label_list(spec.test_class, spec.list_index);
+        Self::with_labels(spec, list)
+    }
+
+    /// [`AssertReport::new`] with check `spec.test_index` taken from `list` instead of
+    /// the class's table (not upstream: a check an opt-in fix adds to a class, such as
+    /// [`labels::TORUS_WITH_SPINDLE_APEX`], reports from its own extended table, so
+    /// `SM_GetAssertLabelList` keeps upstream's).
+    ///
+    /// # Errors
+    /// [`SolidError::InvalidInput`] when `spec.test_index` is past the end of `list`.
+    pub fn with_labels(spec: ReportSpec, list: &'static [AssertReportLabel]) -> Result<Self> {
         let label = usize::try_from(spec.test_index)
             .ok()
             .and_then(|i| list.get(i))

crates/solidcore/src/geom/assert_array/labels.rs

@@ -595,6 +595,43 @@ pub const TORUS: &[AssertReportLabel] = &[
     ),
 ];
 
+/// Not upstream (WR-539, the fork's tangency-labs/smlib@daba678): [`TORUS`] with the
+/// fork's test 5, "Self Intersection". Only the torus check with
+/// `Kernel::fixes.torus_check_spindle_apex` reports from it; `SM_GetAssertLabelList`
+/// ([`label_list`]) keeps upstream's [`TORUS`].
+pub const TORUS_WITH_SPINDLE_APEX: &[AssertReportLabel] = &[
+    l(
+        AssertType::Radius,
+        "Major Radius",
+        Some("m_dMajorRadius is greater than zero"),
+    ),
+    l(
+        AssertType::Radius,
+        "Minor Radius",
+        Some("m_dMinorRadius is greater than zero"),
+    ),
+    l(
+        AssertType::Parameterization,
+        "Same Parameterization",
+        Some("Curve and Surface have same paramterization"),
+    ),
+    l(
+        AssertType::Geometric,
+        "Overlapping Domain",
+        Some("Analytic U Direction Domain too large: Tori can't overlap themselves"),
+    ),
+    l(
+        AssertType::Geometric,
+        "Overlapping Domain",
+        Some("Analytic V Direction Domain too large: Tori can't overlap themselves"),
+    ),
+    l(
+        AssertType::Geometric,
+        "Self Intersection",
+        Some("Spindle torus: Analytic V Direction Domain runs through an apex on the axis"),
+    ),
+];
+
 /// `sAssertSurfOfRevolution_list` (`source/SMLib/src/SmSurfOfRevolution.cpp`).
 pub const SURF_OF_REVOLUTION: &[AssertReportLabel] = &[
     l(AssertType::Pointer, "Context", Some("Bad m_pGenCurve does not share same context")),

crates/solidcore/src/kernel.rs

@@ -209,6 +209,10 @@ pub struct UpstreamFixes {
     pub refine_cc_range_ends: bool,
     /// `SmSurface::GetFlatCorners` tests each corner bit instead of clearing the others (WR-384, #525).
     pub flat_corners_test_bits: bool,
+    /// `SmTorus::AssertValid` tests its V (minor) interval, not the U interval twice, and reports its U and V overlap tests as 3 and 4 (and `SmSurfOfRevolution`'s U test as 13), the ids their labels give them, where upstream reports them as 2 and 3 (and 2) (WR-538, #482; the fork's tangency-labs/smlib@0cd9d08).
+    pub torus_check_reads_v_interval: bool,
+    /// `SmTorus::AssertValid` reports a spindle torus whose V interval runs through an apex (`V = ±(90 + asin(R/r))`) as test 5, "Self Intersection", where upstream clears its result with no report and tests the U interval against `90 + acos(R/r)`, failing every spindle torus with a whole turn of U (WR-539; the fork's tangency-labs/smlib@daba678).
+    pub torus_check_spindle_apex: bool,
     /// `SmCone::RebuildSTEPFromNURBParameters` gives a cylinder the mean of near-equal radii (semi-angle 0) (WR-385, #713).
     pub cylinder_rebuild_mean_radius: bool,
     /// `SmCylinder::IsNurbSurfaceCylinder` gives an inside-out NURB (its iso circles turning against the line through their centres) the frame `IsNurbSurfaceCone` gives one: Z against the NURB's linear direction, origin at its top-knot circle; upstream keeps the bottom-knot circle and the NURB's direction, so the cylinder's Z is reversed and its STEP evaluation is mirrored in height (WR-496, #1463).

crates/solidcore/src/surface/surface/assert_valid.rs

@@ -89,9 +89,11 @@ use crate::geom::a_object;
 use crate::geom::assert_array::labels::{
     AXIS2_PLACEMENT_TYPE, B_SPLINE_SURFACE_TYPE, CONE_TYPE, CRV_ON_SURF_TYPE, CYLINDER_TYPE,
     FACE_TYPE, OFFSET_SURFACE_TYPE, PLANE_TYPE, SPHERE_TYPE, SRF_IN_VOLUME_TYPE, SURFACE_TYPE,
-    SURF_OF_EXTRUSION_TYPE, SURF_OF_REVOLUTION_TYPE, TORUS_TYPE,
+    SURF_OF_EXTRUSION_TYPE, SURF_OF_REVOLUTION_TYPE, TORUS_TYPE, TORUS_WITH_SPINDLE_APEX,
+};
+use crate::geom::assert_array::{
+    assert_label_list, AssertArray, AssertReport, AssertReportLabel, ObjectKey, ReportSpec,
 };
-use crate::geom::assert_array::{AssertArray, AssertReport, ObjectKey, ReportSpec};
 use crate::geom::axis2_placement::Axis2Placement;
 use crate::geom::extent1d::Extent1d;
 use crate::geom::extent3d::Extent3d;
@@ -105,6 +107,7 @@ use crate::geom::tol::{
     ANG_TOL_DEG, ZONE_TOL_3D,
 };
 use crate::geom::vector3d::acos as sm_acos;
+use crate::geom::vector3d::asin as sm_asin;
 use crate::kernel::Kernel;
 use crate::math::{
     max2, min2, rad_to_deg, Point2, Point3, Vec3, EFF_ZERO, EFF_ZERO_DEG, EFF_ZERO_RAD,
@@ -165,6 +168,36 @@ impl Reporter<'_> {
         passed: bool,
         tol: f64,
         val: f64,
+    ) -> Result<bool> {
+        let labels = assert_label_list(class, 0);
+        self.report_from(
+            labels,
+            class,
+            owner_type,
+            owner_string,
+            test,
+            test_level,
+            passed,
+            tol,
+            val,
+        )
+    }
+
+    /// [`Reporter::report_as`] with check `test` labelled from `labels` (not upstream:
+    /// an opt-in check reporting from its class's extended table,
+    /// [`AssertReport::with_labels`]).
+    #[allow(clippy::too_many_arguments)]
+    fn report_from(
+        &mut self,
+        labels: &'static [AssertReportLabel],
+        class: i64,
+        owner_type: i64,
+        owner_string: &'static str,
+        test: u64,
+        test_level: u8,
+        passed: bool,
+        tol: f64,
+        val: f64,
     ) -> Result<bool> {
         if self.level < test_level || passed {
             if let Some(l) = self.list.as_deref_mut() {
@@ -173,7 +206,7 @@ impl Reporter<'_> {
             return Ok(true);
         }
         if let Some(l) = self.list.as_deref_mut() {
-            let r = AssertReport::new(ReportSpec {
+            let spec = ReportSpec {
                 reporter: Some(self.key),
                 test_index: test,
                 test_class: class,
@@ -187,8 +220,8 @@ impl Reporter<'_> {
                 value: val,
                 list_index: 0,
                 message: None,
-            })?;
-            l.add_unique_delete_duplicate(r);
+            };
+            l.add_unique_delete_duplicate(AssertReport::with_labels(spec, labels)?);
         }
         Ok(false)
     }
@@ -1472,11 +1505,16 @@ fn surf_of_revolution_assert_valid(k: &Kernel, s: &Surface, r: &mut Reporter<'_>
     }
 
     /* 13 */
-    // Analytic U Direction Domain too large (reported as test 2, as upstream)
+    // Analytic U Direction Domain too large (reported as test 2, as upstream; as 13, its
+    // label, with WR-538's `torus_check_reads_v_interval`)
     let ivl_u = anal.u_interval();
     rtn &= r.report(
         CLASS,
-        2,
+        if k.fixes.torus_check_reads_v_interval {
+            13
+        } else {
+            2
+        },
         0,
         ivl_u.length() <= 360.0 + EFF_ZERO_DEG,
         EFF_ZERO_DEG,
@@ -2023,7 +2061,11 @@ fn sphere_assert_valid(k: &Kernel, s: &Surface, r: &mut Reporter<'_>) -> Result<
 }
 
 /// `SmTorus::AssertValid` (pinned upstream: test 4 reads the U interval, and the U
-/// and V tests report as 2 and 3).
+/// and V tests report as 2 and 3; with `Kernel::fixes.torus_check_reads_v_interval`,
+/// WR-538, they read U and V and report as 3 and 4. A spindle torus's self-intersection
+/// test clears the result with no report and tests the U interval against an `acos`
+/// range; with `Kernel::fixes.torus_check_spindle_apex`, WR-539, it reports test 5 for an
+/// apex inside the V interval).
 fn torus_assert_valid(k: &Kernel, s: &Surface, r: &mut Reporter<'_>) -> Result<bool> {
     const CLASS: i64 = TORUS_TYPE;
     let mut rtn = true;
@@ -2042,20 +2084,52 @@ fn torus_assert_valid(k: &Kernel, s: &Surface, r: &mut Reporter<'_>) -> Result<b
 
     let anal = t.base().step_uv_domain();
     if major < minor {
-        let angle_limit_deg = 90.0 + sm_acos(major / minor) * 180.0 / PI;
-        let self_intersect = Extent1d::from_min_max(angle_limit_deg, 360.0 - angle_limit_deg);
-        if !self_intersect.are_disjoint_periodic(&anal.u_interval(), 360.0, -1.0) {
-            rtn &= false;
+        if k.fixes.torus_check_spindle_apex {
+            /*  5 */
+            // Not upstream (WR-539, the fork's tangency-labs/smlib@daba678): a spindle
+            // torus's minor circles cross the axis where R + r cos(V) = 0, at the apexes
+            // V = +-(90 + asin(R/r)) degrees; like a sphere's poles (its tests 4 and 5),
+            // an apex may bound the V domain but not lie inside it.
+            let apex_deg = 90.0 + sm_asin(major / minor) * 180.0 / PI;
+            let apex_ivl_v = anal.v_interval();
+            for apex in [apex_deg, -apex_deg] {
+                let c = apex_ivl_v.contains_periodic_value(apex, 360.0, EFF_ZERO_DEG);
+                // (test 5 is the fork's: labelled from the extended table, so the
+                // class's `SM_GetAssertLabelList` table stays upstream's)
+                let class_string = r.class_string;
+                rtn &= r.report_from(
+                    TORUS_WITH_SPINDLE_APEX,
+                    CLASS,
+                    CLASS,
+                    class_string,
+                    5,
+                    0,
+                    !c.contained
+                        || is_zero_to_tol(c.dist_to_min, EFF_ZERO_DEG)
+                        || is_zero_to_tol(c.dist_to_max, EFF_ZERO_DEG),
+                    EFF_ZERO_DEG,
+                    min2(c.dist_to_min, c.dist_to_max),
+                )?;
+            }
+        } else {
+            let angle_limit_deg = 90.0 + sm_acos(major / minor) * 180.0 / PI;
+            let self_intersect = Extent1d::from_min_max(angle_limit_deg, 360.0 - angle_limit_deg);
+            if !self_intersect.are_disjoint_periodic(&anal.u_interval(), 360.0, -1.0) {
+                rtn &= false;
+            }
         }
     }
 
     let same = generator_has_same_parameterization(k, s)?;
     rtn &= r.report(CLASS, 2, 0, same, EFF_ZERO, UNDEF)?;
 
+    // Not upstream (WR-538, the fork's tangency-labs/smlib@0cd9d08, #482): the U and V
+    // tests report as 3 and 4 (their labels), and the V test reads the V interval
+    let fixed_ids = k.fixes.torus_check_reads_v_interval;
     let ivl_u = anal.u_interval();
     rtn &= r.report(
         CLASS,
-        2,
+        if fixed_ids { 3 } else { 2 },
         0,
         ivl_u.length() <= 360.0 + EFF_ZERO_DEG,
         EFF_ZERO_DEG,
@@ -2063,10 +2137,14 @@ fn torus_assert_valid(k: &Kernel, s: &Surface, r: &mut Reporter<'_>) -> Result<b
     )?;
 
     // (upstream reads the U interval again)
-    let ivl_v = anal.u_interval();
+    let ivl_v = if fixed_ids {
+        anal.v_interval()
+    } else {
+        anal.u_interval()
+    };
     rtn &= r.report(
         CLASS,
-        3,
+        if fixed_ids { 4 } else { 3 },
         0,
         ivl_v.length() <= 360.0 + EFF_ZERO_DEG,
         EFF_ZERO_DEG,
@@ -2303,6 +2381,69 @@ mod tests {
         );
     }
 
+    /// The failed reports' (class, test) of `s` at level 0 on kernel `k`.
+    fn failed_tests(k: &Kernel, s: &Surface) -> (bool, Vec<(i64, u64)>) {
+        let mut list = AssertArray::new();
+        let ok = s.assert_valid(k, Some(&mut list), 0, ObjectKey(1)).unwrap();
+        let failed = list
+            .reports()
+            .iter()
+            .filter(|r| !r.ok)
+            .map(|r| (r.reporting_type, r.test_index))
+            .collect();
+        (ok, failed)
+    }
+
+    fn torus_over(v0: f64, v1: f64, major: f64, minor: f64) -> Surface {
+        let d = Extent2d::from_bounds(0.0, v0, 360.0, v1);
+        let opts = TorusOptions {
+            swap_uv: false,
+            inside_out: false,
+            circle_nurb: None,
+        };
+        Surface::from(Torus::new(&Kernel::default(), O, X, Y, d, major, minor, opts).unwrap())
+    }
+
+    /// WR-538 (the fork's tangency-labs/smlib@0cd9d08, #482): a torus whose V interval
+    /// runs past a turn. Upstream tests the U interval twice and never the V one; with
+    /// `torus_check_reads_v_interval` the V test (4) fails.
+    #[test]
+    fn wr_538_torus_check_reads_its_v_interval() {
+        let s = torus_over(0.0, 400.0, 5.0, 1.0);
+        let mut k = Kernel::default();
+        let (_, upstream) = failed_tests(&k, &s);
+        assert!(!upstream.contains(&(TORUS_TYPE, 4)), "{upstream:?}");
+        k.fixes.torus_check_reads_v_interval = true;
+        let (ok, fixed) = failed_tests(&k, &s);
+        assert!(!ok);
+        assert!(fixed.contains(&(TORUS_TYPE, 4)), "{fixed:?}");
+        assert!(!fixed.contains(&(TORUS_TYPE, 3)), "{fixed:?}");
+    }
+
+    /// WR-539 (the fork's tangency-labs/smlib@daba678): the spindle torus R 1, r 2 has its
+    /// apexes at V = +-120. Upstream fails every spindle torus with a whole turn of U and
+    /// reports nothing; with `torus_check_spindle_apex` V [0, 360] reports test 5 and the
+    /// outer sheet V [-120, 120] passes.
+    #[test]
+    fn wr_539_spindle_torus_reports_an_apex_inside() {
+        let whole = torus_over(0.0, 360.0, 1.0, 2.0);
+        let outer = torus_over(-120.0, 120.0, 1.0, 2.0);
+        let mut k = Kernel::default();
+        for s in [&whole, &outer] {
+            let (ok, upstream) = failed_tests(&k, s);
+            assert!(!ok);
+            assert!(!upstream.contains(&(TORUS_TYPE, 5)), "{upstream:?}");
+        }
+        k.fixes.torus_check_spindle_apex = true;
+        let (ok, fixed) = failed_tests(&k, &whole);
+        assert!(!ok);
+        assert!(fixed.contains(&(TORUS_TYPE, 5)), "{fixed:?}");
+        let (_, outer_fixed) = failed_tests(&k, &outer);
+        assert!(!outer_fixed.contains(&(TORUS_TYPE, 5)), "{outer_fixed:?}");
+        // the class's table stays upstream's five checks (SM_GetAssertLabelList)
+        assert_eq!(assert_label_list(TORUS_TYPE, 0).len(), 5);
+    }
+
     /// tangency-labs/solidcore#1515 (WR-505): a horn torus (major radius = minor radius)
     /// touches its axis, where the corner rows of its NURB coincide. Upstream's
     /// `HasInternalPole` tests one row one step too far at each repeated knot and reports

plan/upstream-issues/WR-538--torus-check-tests-u-interval-twice.md

@@ -0,0 +1,9 @@
+#### WR-538. `SmTorus::AssertValid` tests its U interval twice and never its V interval, under the wrong report ids
+- **Severity:** Wrong result (validation gap)
+- **Area:** Surfaces (tori)
+- **Location:** `source/SMLib/src/SmTorus.cpp`, `SmTorus::AssertValid` (def. `:1661`): the V overlap test reads `m_vAnalUVDomain.GetUInterval()`. The U and V tests report as 2 ("Same Parameterization") and 3 (the U label). `source/SMLib/src/SmSurfOfRevolution.cpp`, `SmSurfOfRevolution::AssertValid` (def. `:6152`): test 13 reports as 2.
+- **What happens:** A torus whose V (minor) interval runs past a turn passes the overlap test, which reads the U interval again. The two overlap tests and the surface of revolution's U test also report under ids whose labels describe other tests.
+- **Trigger:** a torus with V in [0, 400]. With the fix it fails test 4 (offset 40); before, only the unrelated parameterization test notices it.
+- **How found:** tangency-labs/solidcore#482, at the time closed as not reached by the port. The fork's fix is tangency-labs/smlib@0cd9d08 ("the torus check tests its minor interval"), which is not merged upstream.
+- **Confirmed:** by reading the pinned source (`3fd5b95`). The port's `torus_assert_valid` reproduces it.
+- **Rust port:** behaves as upstream by default. With `Kernel::fixes.torus_check_reads_v_interval`, the torus's V test reads the V interval, and the U and V tests report as 3 and 4. The surface of revolution's U test reports as 13. Test: `wr_538_torus_check_reads_its_v_interval` in `surface/surface/assert_valid.rs`.

plan/upstream-issues/WR-539--torus-check-spindle-self-intersection-has-no-report.md

@@ -0,0 +1,9 @@
+#### WR-539. `SmTorus::AssertValid` fails every spindle torus with a whole turn of U, with no report
+- **Severity:** Wrong result (validation: a false failure with no report, and a missed one)
+- **Area:** Surfaces (tori)
+- **Location:** `source/SMLib/src/SmTorus.cpp`, `SmTorus::AssertValid` (def. `:1661`): the self-intersection branch (`m_dMajorRadius < m_dMinorRadius`).
+- **What happens:** The branch clears `bRtn` with no report, so a failing torus gives `FALSE` with nothing to show. Its test is also wrong in two ways. The minor circles cross the axis where `R + r cos(V) = 0`, at `V = ±(90 + asin(R/r))`, but the test uses `90 + acos(R/r)`. It also tests the U (major) interval against that V range. So every spindle torus with a whole turn of U fails, including its outer sheet alone.
+- **Trigger:** spindle torus R 1, r 2 (apexes at ±120): V [0, 360], V [-120, 150], V [-120, 120] (the outer sheet) and V [120, 240] (the inner sheet) all fail with no report.
+- **How found:** SolidCraft; the fork's fix is tangency-labs/smlib@daba678 ("the torus check reports a spindle torus running through its apex"), which is not merged upstream.
+- **Confirmed:** by reading the pinned source (`3fd5b95`). The port's `torus_assert_valid` reproduces it.
+- **Rust port:** behaves as upstream by default. With `Kernel::fixes.torus_check_spindle_apex`, each apex is tested as `SmSphere`'s poles are (its tests 4 and 5). An apex may bound the V interval but not lie inside it, and an apex inside is reported as test 5, "Self Intersection", with the distance to the nearer V bound. The report's label comes from `labels::TORUS_WITH_SPINDLE_APEX`, the torus table with the fork's sixth entry. `SM_GetAssertLabelList` (`assert_label_list`) keeps upstream's five-entry `TORUS`, as the pinned C++ does. Test: `wr_539_spindle_torus_reports_an_apex_inside` in `surface/surface/assert_valid.rs`.