Fix large modeled threads and thin crest reconstruction (#2209)

* Fix large modeled threads and thin crest reconstruction

Helix stations follow the pitch (radial chord error under 1% of it, 48 to
720 stations), and the faceted rebuild retries at the BSP's precision when
the usual weld tolerance collapses a thin crest facet.

Rebased onto main as one commit (the branch's own commits touched UI files
that later moved to #2263 and #2264). Adapted to main's UnitVec3 thread
axis: `*k` in thread_groove and `*cylinder.axis` in the new flange test.

* Fix icon review revisions for same-timestamp edits

ROADMAP.md

@@ -101,6 +101,8 @@ completed drawings publish atomically. The browser still exports synchronously.
   drag continues, then catch up to the exact final value. Changes to the selected geometry,
   operation or feature still discard obsolete results. Distance arrows and their value boxes
   follow the current frame's input.
+- Large fine modelled threads use pitch-based helix resolution and tighter faceted
+  reconstruction fallbacks, preserving thin crests and closed shells on large shafts.
 
 ## Physical material data
 

assets/icons/ui/design/tools/icon_status.py

@@ -37,8 +37,11 @@ def art_rev():
     paths += glob.glob(HERE + '/*.py')
     paths += [D + '/palette.toml', D + '/aliases.toml', __file__]
     for path in sorted(paths):
-        st = os.stat(path)
-        h.update(('%s %d %d;' % (os.path.relpath(path, ROOT), st.st_mtime_ns, st.st_size)).encode())
+        # Equal-length edits can share an mtime. Only the content identifies what
+        # the reviewer saw; file metadata must never authorize stale acceptance.
+        h.update(os.path.relpath(path, ROOT).replace(os.sep, '/').encode() + b'\0')
+        with open(path, 'rb') as source:
+            h.update(hashlib.sha256(review_state.lf(source.read())).digest())
     return h.hexdigest()[:16]
 
 

crates/doc/src/eval.rs

@@ -407,8 +407,19 @@ fn thread_groove(t: &ThreadInfo, radius: f64, open: (bool, bool)) -> Result<soli
     let base = t.axis_point + k * s0;
     let plane = Plane { origin: base, x: u, y: *k };
     let section = [Vec2::new(r0, -w0), Vec2::new(r1, -w1), Vec2::new(r1, w1), Vec2::new(r0, w0)];
-    // Sections 7.5° apart: the chords sag 0.2% of the radius.
-    Ok(kernel::sweep_helix_mesh(&plane, &section, base, *k, p, (s1 - s0) / p, 48)?)
+    // Keep radial chord error below 1% of the pitch, rather than a fixed fraction of the
+    // diameter. At 48 stations a large fine thread cuts too deeply; its outer tool facets
+    // can even dip under the shaft surface. The existing coarse sizes still use 48.
+    let outer = r0.abs().max(r1.abs());
+    if !outer.is_finite() {
+        return Err(DocError::Invalid("thread dimensions are too large".into()));
+    }
+    let angle = (1.0 - (p * 0.01 / outer).min(1.0)).acos();
+    let stations = (std::f64::consts::PI / angle).ceil().max(48.0);
+    if !stations.is_finite() || stations > 720.0 {
+        return Err(DocError::Invalid("the modelled thread is too fine for this diameter; use a cosmetic thread".into()));
+    }
+    Ok(kernel::sweep_helix_mesh(&plane, &section, base, *k, p, (s1 - s0) / p, stations as usize)?)
 }
 
 /// A constant fillet of `edges`; with [`FilletFit::Reduce`](crate::FilletFit::Reduce), a radius too

crates/engine/src/thread_tests.rs

@@ -1,4 +1,5 @@
-//! Modelled threads: the ISO metric profile cut along a helix, M3 to M20, outside and inside.
+//! Modelled threads: the ISO metric profile cut along a helix, inside and outside,
+//! including fine pitches on large shafts and the sample flange.
 
 use std::f64::consts::PI;
 
@@ -22,6 +23,75 @@ fn volume(s: &mut Session, b: &str) -> f64 {
 
 const SIZES: [(f64, f64); 9] = [(3.0, 0.5), (4.0, 0.7), (5.0, 0.8), (6.0, 1.0), (8.0, 1.25), (10.0, 1.5), (12.0, 1.75), (16.0, 2.0), (20.0, 2.5)];
 
+/// Keep the original 48-station reproduction of the shell error even though new fine
+/// threads now use more stations. The same cut polygons must rebuild as a closed solid.
+#[test]
+fn faceted_flange_thread_preserves_thin_crest_boundaries() {
+    use crate::geom::{Plane, Vec2, Vec3};
+    use crate::kernel;
+
+    let mut s = Session::default();
+    let sample = crate::sample::samples().into_iter().find(|s| s.name == "Bolted Flange").unwrap();
+    s.run_script(&(sample.script)()).unwrap();
+    let state = s.model.state();
+    let body = &state.body("Flange").unwrap().body;
+    let face = Vec3::new(-19.034766861657907, -11.03075929038123, 19.574640342897094);
+    let cylinder = kernel::cylinder_face_at(body, face).unwrap();
+    let pitch = 1.5;
+    let height = pitch * 3f64.sqrt() / 2.0;
+    let root = 22.0 - 5.0 * height / 8.0;
+    let outer = 22.0 + 0.05 * pitch;
+    let half_width = (pitch / 4.0 + 2.0 * (outer - root) * (PI / 6.0).tan()) / 2.0;
+    let section = [Vec2::new(root, -pitch / 8.0), Vec2::new(outer, -half_width), Vec2::new(outer, half_width), Vec2::new(root, pitch / 8.0)];
+    let (start, end) = (cylinder.start - pitch, cylinder.end + pitch);
+    let base = cylinder.axis_point + cylinder.axis * start;
+    let plane = Plane { origin: base, x: cylinder.axis.cross(Vec3::new(1.0, 0.0, 0.0)).normalized().unwrap(), y: *cylinder.axis };
+    let groove = kernel::sweep_helix_mesh(&plane, &section, base, *cylinder.axis, pitch, (end - start) / pitch, 48).unwrap();
+    let cut = kernel::faceted_boolean(body, &groove, kernel::BoolOp::Cut, pitch * 0.005).unwrap().unwrap();
+    assert!(cut.validity().is_empty(), "{:?}", cut.validity());
+    let before = body.tessellate(0.001).unwrap().measure().volume;
+    let after = cut.tessellate(0.001).unwrap().measure().volume;
+    assert!(before - after > 700.0 && before - after < 1800.0, "removed {}", before - after);
+}
+
+#[test]
+fn modelled_full_length_m44_flange_thread() {
+    let mut s = Session::default();
+    let sample = crate::sample::samples().into_iter().find(|s| s.name == "Bolted Flange").unwrap();
+    s.run_script(&(sample.script)()).unwrap();
+    let before = volume(&mut s, "Flange");
+    run(
+        &mut s,
+        "solid.thread",
+        json!({
+            "face": [-19.034766861657907, -11.03075929038123, 19.574640342897094],
+            "designation": "M44x1.5", "modeled": true
+        }),
+    );
+    let state = s.model.state();
+    let body = &state.body("Flange").unwrap().body;
+    assert!(body.validity().is_empty(), "{:?}", body.validity());
+    let removed = before - volume(&mut s, "Flange");
+    assert!(removed > 700.0 && removed < 1800.0, "a full-length helical cut removes material: {removed}");
+}
+
+#[test]
+fn modelled_full_length_fine_threads_on_large_shafts() {
+    for (d, p, len) in [(44.0, 1.0, 5.0), (44.0, 1.5, 16.0), (100.0, 1.5, 6.0)] {
+        let mut s = Session::default();
+        run(&mut s, "solid.cylinder", json!({"radius":d / 2.0,"height":len,"body_name":"Shaft"}));
+        let before = volume(&mut s, "Shaft");
+        run(&mut s, "solid.thread", json!({"face":[d / 2.0,0,len / 2.0],"designation":format!("M{d}x{p}"),"modeled":true}));
+        let state = s.model.state();
+        let body = &state.body("Shaft").unwrap().body;
+        assert!(body.validity().is_empty(), "M{d}x{p}: {:?}", body.validity());
+        let root = d / 2.0 - 5.0 * p * 3f64.sqrt() / 16.0;
+        let want = per_turn(root, d / 2.0, |r| p / 4.0 + 2.0 * (r - root) * (PI / 6.0).tan()) * len / p;
+        let got = before - volume(&mut s, "Shaft");
+        assert!((got - want).abs() < want * 0.04, "M{d}x{p}: removed {got}, ISO profile {want}");
+    }
+}
+
 #[test]
 fn non_finite_thread_designations_are_refused_without_changing_the_design() {
     let mut s = Session::default();

crates/kernel/src/polybool.rs

@@ -398,10 +398,25 @@ fn faceted_boolean_untimed(a: &Body, b: &Mesh, op: BoolOp, tol: f64) -> Result<O
         if out.is_empty() {
             return Ok(None);
         }
-        match rebuild(out, size, &sources) {
+        let weld = size * 1e-7;
+        match rebuild(&out, size, weld, &sources) {
             Ok(b) => return Ok(Some(b)),
             Err(e) => last = e,
         }
+        // Thin facets at a thread crest can be narrower than the usual reconstruction
+        // tolerance. Welding/splitting at that coarser tolerance collapses their boundaries
+        // into a non-manifold edge. Retry the same volume-checked polygons at the BSP's
+        // finer precision. Allow a small margin for points classified on either side of
+        // a splitter, then try the exact BSP tolerance if that still collapses a facet.
+        // Never return a shell that fails the closed-solid check.
+        for precision in [3.0 * eps, eps] {
+            if precision < weld {
+                match rebuild(&out, size, precision, &sources) {
+                    Ok(b) => return Ok(Some(b)),
+                    Err(e) => last = e,
+                }
+            }
+        }
     }
     Err(last)
 }
@@ -685,7 +700,7 @@ fn polys_boolean(pa: Vec<Poly>, pb: Vec<Poly>, size: f64, op: BoolOp, eps: f64,
     if polys.is_empty() {
         return Ok(None);
     }
-    rebuild(polys, size, sources).map(Some)
+    rebuild(&polys, size, size * 1e-7, sources).map(Some)
 }
 
 /// What a rebuilt body's history is over: each operand's face and edge counts and, where it is
@@ -837,8 +852,7 @@ fn key(p: Vec3, q: f64) -> (i64, i64, i64) {
 /// Its history over the operands the polygons are pieces of (`sources`): each face the
 /// operand faces whose pieces it joins (pieces joined across the edges between them), each edge
 /// what the pieces either side of it say (see [`Sources::edge`]).
-fn rebuild(polys: Vec<Poly>, size: f64, sources: &Sources) -> Result<Body> {
-    let q = size * 1e-7;
+fn rebuild(polys: &[Poly], size: f64, q: f64, sources: &Sources) -> Result<Body> {
     // Global vertex table.
     let mut index: BTreeMap<(i64, i64, i64), usize> = BTreeMap::new();
     let mut verts: Vec<Vec3> = Vec::new();
@@ -862,7 +876,7 @@ fn rebuild(polys: Vec<Poly>, size: f64, sources: &Sources) -> Result<Body> {
     };
     let mut loops: Vec<(PlaneEq, Vec<usize>)> = Vec::new();
     let mut srcs: Vec<FaceOrigin> = Vec::new();
-    for p in &polys {
+    for p in polys {
         let mut l: Vec<usize> = p.v.iter().map(|v| id(*v, &mut verts)).collect();
         l.dedup();
         if l.first() == l.last() && l.len() > 1 {
@@ -905,7 +919,7 @@ fn rebuild(polys: Vec<Poly>, size: f64, sources: &Sources) -> Result<Body> {
             let (Some(&pa), Some(&pb)) = (verts.get(a), verts.get(b)) else { continue };
             let d = pb - pa;
             let len2 = d.len2();
-            let (lo, hi) = (ckey(pa.min(pb) - Vec3::new(tol, tol, tol)), ckey(pa.max(pb) + Vec3::new(tol, tol, tol)));
+            let (lo, hi) = (ckey(pa.min(pb) - Vec3::new(q, q, q)), ckey(pa.max(pb) + Vec3::new(q, q, q)));
             let cells = (hi.0 - lo.0 + 1) * (hi.1 - lo.1 + 1) * (hi.2 - lo.2 + 1);
             let candidates: Vec<usize> = if cells > 4096 {
                 (0..verts.len()).collect()
@@ -928,7 +942,7 @@ fn rebuild(polys: Vec<Poly>, size: f64, sources: &Sources) -> Result<Body> {
                 .filter_map(|k| {
                     let v = verts.get(k)?;
                     let t = (*v - pa).dot(d) / len2.max(1e-300);
-                    (t > 1e-9 && t < 1.0 - 1e-9 && v.dist_to_segment(pa, pb) < tol).then_some((t, k))
+                    (t > 1e-9 && t < 1.0 - 1e-9 && v.dist_to_segment(pa, pb) < q).then_some((t, k))
                 })
                 .collect();
             on.sort_by(|x, y| x.0.total_cmp(&y.0));

tests/tools/test_icon_review.py

@@ -1,6 +1,7 @@
 """Long-term discovery and acceptance behavior of the repository-owned review app."""
 import contextlib
 import json
+import os
 import re
 from pathlib import Path
 import shutil
@@ -247,7 +248,14 @@ class IconReviewTest(unittest.TestCase):
             self.assertNotIn('box', changed['final'])
             self.assertIn('changed since acceptance', get('/'))
             self.assertEqual(old_hash, json.loads((design / 'review.json').read_text(encoding='utf-8'))['final']['box'])
-            (design / 'svg/model/box.svg').write_text(SVG.replace('r="20"', 'r="17"'), encoding='utf-8')
+            artwork = design / 'svg/model/box.svg'
+            before = artwork.stat()
+            artwork.write_text(SVG.replace('r="20"', 'r="17"'), encoding='utf-8')
+            # Equal-length edits can share a filesystem timestamp on fast CI hosts.
+            # Reproduce that deterministically: acceptance must track the content.
+            os.utime(artwork, ns=(before.st_atime_ns, before.st_mtime_ns))
+            self.assertEqual(artwork.stat().st_size, before.st_size)
+            self.assertEqual(artwork.stat().st_mtime_ns, before.st_mtime_ns)
             with self.assertRaises(urllib.error.HTTPError) as error:
                 post(changed, final=['box'])
             self.assertEqual(error.exception.code, 400)