Merge pull request #151 from EmDecay/fix/143-construction-plane-recursion

fix: continue the plane depth count through model geometry

apps/solvecraft-cli/tests/design_corpus.rs

@@ -150,6 +150,23 @@ fn try_bytes(bytes: &[u8]) {
     }
 }
 
+/// Hostile shapes that no mutation of a saved file produces: plane references that close a
+/// cycle, which resolve through the model and back into the document.
+fn generated() -> Vec<(String, Vec<u8>)> {
+    let to = |name: &str| json!({"type": "plane", "plane": {"type": "construction", "name": name}});
+    let plane = |id: u64, name: &str, side: Value| {
+        json!({"id": id, "name": name, "suppressed": false, "type": "construction_plane",
+               "plane": {"type": "midplane", "a": side, "b": {"type": "plane", "plane": {"type": "origin", "name": "XZ"}}}})
+    };
+    let design = |name: &str, features: Value| {
+        json!({"format": "solvecraft/11", "name": name, "units": "mm", "params": [], "next_id": 9, "features": features}).to_string().into_bytes()
+    };
+    vec![
+        ("construction plane is its own side".into(), design("selfplane", json!([plane(1, "P1", to("P1"))]))),
+        ("two construction planes refer to each other".into(), design("twoplanes", json!([plane(1, "P1", to("P2")), plane(2, "P2", to("P1"))]))),
+    ]
+}
+
 #[test]
 fn damaged_and_hostile_design_files_never_panic() {
     let mut rng = Rng(0x5eed_cafe_f00d_d00d);
@@ -201,6 +218,7 @@ fn damaged_and_hostile_design_files_never_panic() {
         cases.push(("binary".into(), (0..4096u32).map(|i| (i.wrapping_mul(2_654_435_761) >> 24) as u8).collect()));
         all_cases.extend(cases.into_iter().map(|(what, b)| (name.clone(), what, b)));
     }
+    all_cases.extend(generated().into_iter().map(|(what, b)| ("generated".to_string(), what, b)));
     let next = std::sync::atomic::AtomicUsize::new(0);
     let threads = std::thread::available_parallelism().map_or(4, |n| n.get());
     let failures: Vec<String> = std::thread::scope(|scope| {

crates/doc/src/construct.rs

@@ -105,6 +105,10 @@ pub(crate) struct Ctx<'a> {
     pub doc: &'a Document,
     pub vals: &'a BTreeMap<String, Value>,
     pub st: &'a ModelState,
+    /// How deep the plane resolution that led here already is. A plane built from model
+    /// geometry resolves its sides through the document again, so the count has to continue
+    /// across that hop; restarting it lets a plane that refers to itself recurse forever.
+    pub depth: usize,
 }
 
 fn bad(m: impl Into<String>) -> DocError {
@@ -281,7 +285,7 @@ pub(crate) fn line(ctx: &Ctx, g: &GeoRef) -> Result<(Vec3, Vec3)> {
 /// A reference as a plane.
 pub(crate) fn plane(ctx: &Ctx, g: &GeoRef) -> Result<Plane> {
     match g {
-        GeoRef::Plane { plane } => ctx.doc.resolve_plane_in(ctx.vals, plane, 0, Some(ctx.st)),
+        GeoRef::Plane { plane } => ctx.doc.resolve_plane_in(ctx.vals, plane, ctx.depth + 1, Some(ctx.st)),
         GeoRef::Face { at, name } => {
             let (b, fi, q) = face_of(ctx.st, *at, name)?;
             match kernel::face_surface(&b.body, fi) {
@@ -527,7 +531,7 @@ pub(crate) fn resolve_geo_plane(ctx: &Ctx, p: &PlaneRef) -> Result<Plane> {
             let (p, tan) = along(&path_points(ctx, path)?, t).ok_or_else(|| bad("the path has no length"))?;
             Plane::from_normal(p, tan).ok_or_else(degenerate)
         }
-        _ => ctx.doc.resolve_plane_in(ctx.vals, p, 0, Some(ctx.st)),
+        _ => ctx.doc.resolve_plane_in(ctx.vals, p, ctx.depth + 1, Some(ctx.st)),
     }
 }
 

crates/doc/src/document.rs

@@ -2132,7 +2132,7 @@ impl Document {
             | PlaneRef::AlongPath { .. }
             | PlaneRef::Perpendicular { .. } => {
                 let st = st.ok_or_else(|| DocError::Invalid("this plane is built from the model: evaluate first".into()))?;
-                crate::construct::resolve_geo_plane(&crate::construct::Ctx { doc: self, vals, st }, p)
+                crate::construct::resolve_geo_plane(&crate::construct::Ctx { doc: self, vals, st, depth }, p)
             }
         }
     }

crates/doc/src/eval.rs

@@ -2932,7 +2932,7 @@ fn eval_feature(doc: &Document, vals: &BTreeMap<String, Value>, f: &Feature, st:
             Ok(())
         }
         FeatureKind::ConstructionAxis { def } => {
-            let (origin, dir) = crate::construct::resolve_axis(&crate::construct::Ctx { doc, vals, st }, def)?;
+            let (origin, dir) = crate::construct::resolve_axis(&crate::construct::Ctx { doc, vals, st, depth: 0 }, def)?;
             st.construct.push(crate::construct::Construct {
                 feature: f.id,
                 name: f.name.clone(),
@@ -2941,7 +2941,7 @@ fn eval_feature(doc: &Document, vals: &BTreeMap<String, Value>, f: &Feature, st:
             Ok(())
         }
         FeatureKind::ConstructionPoint { def } => {
-            let p = crate::construct::resolve_point(&crate::construct::Ctx { doc, vals, st }, def)?;
+            let p = crate::construct::resolve_point(&crate::construct::Ctx { doc, vals, st, depth: 0 }, def)?;
             st.construct.push(crate::construct::Construct { feature: f.id, name: f.name.clone(), geom: crate::construct::ConstructGeom::Point(p) });
             Ok(())
         }

crates/doc/src/tests.rs

@@ -367,3 +367,36 @@ fn timeline_clones_share_meshes_even_when_cloned_before_tessellation() {
     assert!(std::sync::Arc::ptr_eq(&a.mesh(), &b.mesh()));
     assert!(!a.mesh().triangles.is_empty());
 }
+
+/// A construction plane that is its own side resolves through the model and back into the
+/// document. The depth count has to survive that hop, or the recursion overflows the stack and
+/// aborts the process, which no panic guard can intercept.
+#[test]
+fn self_referencing_construction_planes_are_errors() {
+    let selfref = |name: &str| Box::new(construct::GeoRef::Plane { plane: PlaneRef::Construction { name: name.into() } });
+    let xz = || Box::new(construct::GeoRef::Plane { plane: PlaneRef::Origin { name: "XZ".into() } });
+
+    // Directly its own first side.
+    let mut doc = Document::new("selfplane");
+    doc.add_feature(FeatureKind::ConstructionPlane { plane: PlaneRef::Midplane { a: selfref("P1"), b: xz() } }, Some("P1")).unwrap();
+    let mut m = Model::new();
+    m.evaluate(&doc);
+    assert!(m.results.iter().any(|r| r.error.is_some()), "a plane that is its own side must report an error: {:?}", m.results);
+
+    // Two planes that are each other's side.
+    let mut doc = Document::new("twoplanes");
+    doc.add_feature(FeatureKind::ConstructionPlane { plane: PlaneRef::Midplane { a: selfref("P2"), b: xz() } }, Some("P1")).unwrap();
+    doc.add_feature(FeatureKind::ConstructionPlane { plane: PlaneRef::Midplane { a: selfref("P1"), b: xz() } }, Some("P2")).unwrap();
+    let mut m = Model::new();
+    m.evaluate(&doc);
+    assert!(m.results.iter().any(|r| r.error.is_some()), "a two-plane cycle must report an error: {:?}", m.results);
+
+    // A perpendicular plane whose base is itself, and a sketch placed on it.
+    let mut doc = Document::new("perpplane");
+    let line = Box::new(construct::GeoRef::World { axis: "X".into() });
+    doc.add_feature(FeatureKind::ConstructionPlane { plane: PlaneRef::Perpendicular { plane: selfref("P1"), line } }, Some("P1")).unwrap();
+    doc.add_feature(FeatureKind::Sketch { plane: PlaneRef::Construction { name: "P1".into() }, sketch: Sketch::new() }, Some("Sketch1")).unwrap();
+    let mut m = Model::new();
+    m.evaluate(&doc);
+    assert!(m.results.iter().any(|r| r.error.is_some()), "a cyclic perpendicular plane must report an error: {:?}", m.results);
+}