diff --git a/crates/solidcore/src/geom/assert_array.rs b/crates/solidcore/src/geom/assert_array.rs index c1f7a23121..4a100b6880 100644 --- a/crates/solidcore/src/geom/assert_array.rs +++ b/crates/solidcore/src/geom/assert_array.rs @@ -269,6 +269,17 @@ impl AssertReport { /// (upstream reads out of bounds). pub fn new(spec: ReportSpec) -> Result { 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 { let label = usize::try_from(spec.test_index) .ok() .and_then(|i| list.get(i)) diff --git a/crates/solidcore/src/geom/assert_array/labels.rs b/crates/solidcore/src/geom/assert_array/labels.rs index 1eee579d3a..8979359d02 100644 --- a/crates/solidcore/src/geom/assert_array/labels.rs +++ b/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")), diff --git a/crates/solidcore/src/kernel.rs b/crates/solidcore/src/kernel.rs index b3fe060542..70e423c498 100644 --- a/crates/solidcore/src/kernel.rs +++ b/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). diff --git a/crates/solidcore/src/surface/surface/assert_valid.rs b/crates/solidcore/src/surface/surface/assert_valid.rs index bf904d0cbb..93520dd940 100644 --- a/crates/solidcore/src/surface/surface/assert_valid.rs +++ b/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 { + 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 { 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 { 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) -> Result (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 diff --git a/plan/upstream-issues/WR-538--torus-check-tests-u-interval-twice.md b/plan/upstream-issues/WR-538--torus-check-tests-u-interval-twice.md new file mode 100644 index 0000000000..9fa8435f1e --- /dev/null +++ b/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`. diff --git a/plan/upstream-issues/WR-539--torus-check-spindle-self-intersection-has-no-report.md b/plan/upstream-issues/WR-539--torus-check-spindle-self-intersection-has-no-report.md new file mode 100644 index 0000000000..d552bc05c1 --- /dev/null +++ b/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`.