From 0271919d5252f5ef4a1863d33f99cc2104329be8 Mon Sep 17 00:00:00 2001 From: bherbruck Date: Wed, 7 Oct 2026 23:47:25 +0000 Subject: [PATCH] Fix: false "projected face changed shape" warnings after a rollback Re-reading an unchanged projected face could walk its loops in another order or direction, or (from some meshes) return the same half of a split circle twice. The link was then rebuilt, warning and dropping constraints on it. Projected geometry now compares by position regardless of order and direction, and a duplicated half arc is turned into the missing half. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01AS5Tn3xieewUSg3uxrMVdL --- crates/doc/src/project.rs | 25 +++++++++++++- crates/engine/src/tests.rs | 15 ++++++++ crates/sketch/src/link.rs | 71 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 110 insertions(+), 1 deletion(-) diff --git a/crates/doc/src/project.rs b/crates/doc/src/project.rs index 34255563..d9e88332 100644 --- a/crates/doc/src/project.rs +++ b/crates/doc/src/project.rs @@ -147,7 +147,7 @@ pub fn resolve(doc: &Document, vals: &BTreeMap, st: &ModelState, if geom.is_empty() { return Err(DocError::Invalid(format!("the {} projects to nothing on this sketch plane", src.describe()))); } - Ok(Resolved { geom, source, moved }) + Ok(Resolved { geom: merge_split_circles(geom), source, moved }) } /// Re-resolve every link of a sketch (in place). Returns warnings for lost, moved or rebuilt @@ -257,6 +257,29 @@ pub(crate) fn nearest_face(m: &Mesh, p: Vec3) -> Option<(u32, Vec3, f64)> { best } +/// A full circle split by a seam comes out as two arcs between the same two points, but some +/// meshes give the same half twice. That is a circle too: turn the duplicate into the other half +/// so re-reading an unchanged face always gives the same two complementary arcs. +fn merge_split_circles(geom: Vec) -> Vec { + const TOL: f64 = 1e-6; + let near = |p: Vec2, q: Vec2| p.dist(q) <= TOL * (1.0 + p.len().max(q.len())); + let mut out = geom.clone(); + for i in 0..geom.len() { + let Some(LinkGeom::Arc { c, a, b }) = geom.get(i).copied() else { continue }; + for j in i + 1..geom.len() { + if let Some(LinkGeom::Arc { c: c2, a: a2, b: b2 }) = geom.get(j).copied() + && near(c, c2) + && near(a, a2) + && near(b, b2) + && let Some(slot) = out.get_mut(j) + { + *slot = LinkGeom::Arc { c, a: b, b: a }; + } + } + } + out +} + // --------------------------------------------------------------------------------------------- // Projection diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 3440c228..3e192729 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -841,3 +841,18 @@ fn feature_inputs_are_parameters() { let back = solvecraft_doc::Document::from_json(&s.doc.to_json()).unwrap(); assert_eq!(back.features[0].param_names, s.doc.features[0].param_names); } + +#[test] +fn rolling_back_and_forward_leaves_no_projection_warnings() { + let mut s = Session::default(); + s.run_script(&crate::sample::script()).unwrap(); + let warnings = |s: &Session| -> Vec { + s.doc.features.iter().filter_map(|f| s.model.result(f.id).and_then(|r| r.warning.clone()).map(|w| format!("{}: {w}", f.name))).collect() + }; + assert!(warnings(&s).is_empty(), "{:?}", warnings(&s)); + for pos in 0..s.doc.features.len() { + run(&mut s, "timeline.rollTo", json!({ "position": pos })); + run(&mut s, "timeline.rollTo", json!({})); + assert!(warnings(&s).is_empty(), "after rolling to {pos}: {:?}", warnings(&s)); + } +} diff --git a/crates/sketch/src/link.rs b/crates/sketch/src/link.rs index 73306d8e..ceb59709 100644 --- a/crates/sketch/src/link.rs +++ b/crates/sketch/src/link.rs @@ -215,6 +215,54 @@ fn same_shape(a: &LCurve, b: &LCurve) -> bool { ) } +/// Do two curve sets describe the same geometry, regardless of the order the curves come in or +/// the direction a loop was walked? Lines match with either end first; circles by centre and +/// radius; arcs by centre and their (start, end) pair; conics by all their points and rho. +fn same_geometry(a: &[(LCurve, Vec)], b: &[(LCurve, Vec)]) -> bool { + const TOL: f64 = 1e-6; + let near = |p: Vec2, q: Vec2| p.dist(q) <= TOL * (1.0 + p.len().max(q.len())); + let close = |x: f64, y: f64| (x - y).abs() <= TOL * (1.0 + x.abs().max(y.abs())); + let matches = |(ca, pa): &(LCurve, Vec), (cb, pb): &(LCurve, Vec)| -> bool { + if pa.len() != pb.len() { + return false; + } + let all = || pa.iter().zip(pb).all(|(p, q)| near(*p, *q)); + match (ca, cb) { + (LCurve::Line(..), LCurve::Line(..)) => { + all() || matches!((pa.as_slice(), pb.as_slice()), ([a0, a1], [b0, b1]) if near(*a0, *b1) && near(*a1, *b0)) + } + (LCurve::Circle(_, ra), LCurve::Circle(_, rb)) => all() && close(*ra, *rb), + (LCurve::Arc(..), LCurve::Arc(..)) => all(), + (LCurve::Conic(.., ra), LCurve::Conic(.., rb)) => all() && close(*ra, *rb), + _ => false, + } + }; + if a.len() != b.len() { + return false; + } + let mut used = vec![false; b.len()]; + a.iter().all(|x| { + let hit = b.iter().enumerate().find(|(j, y)| !used.get(*j).copied().unwrap_or(true) && matches(x, y)).map(|(j, _)| j); + match hit.and_then(|j| used.get_mut(j)) { + Some(u) => { + *u = true; + true + } + None => false, + } + }) +} + +/// The point indices a layout curve uses, in order. +fn curve_points(c: &LCurve) -> Vec { + match *c { + LCurve::Line(a, b) => vec![a, b], + LCurve::Circle(c, _) => vec![c], + LCurve::Arc(c, a, b) => vec![c, a, b], + LCurve::Conic(a, b, x, _) => vec![a, b, x], + } +} + impl Sketch { pub fn link(&self, id: &str) -> Option<&Link> { self.links.iter().find(|l| l.id == id) @@ -395,6 +443,29 @@ impl Sketch { } return Ok(false); } + // The same geometry walked in another order or direction (e.g. a face loop re-read after + // a rollback) is not a change: keep the entities, and the constraints on them. + if let Some(cur) = &cur { + let pos = |pi: usize| self.points.get(pi).map(|p| p.pos); + let old: Option)>> = cur + .iter() + .map(|c| curve_points(c).into_iter().map(|li| points.get(li).and_then(|pi| pos(*pi))).collect::>>().map(|ps| (*c, ps))) + .collect(); + let fresh: Option)>> = lay + .curves + .iter() + .map(|c| curve_points(c).into_iter().map(|li| lay.pts.get(li).copied()).collect::>>().map(|ps| (*c, ps))) + .collect(); + let lone_old: Vec = points.iter().filter_map(|pi| pos(*pi)).collect(); + if let (Some(old), Some(fresh)) = (old, fresh) + && lay.lone.is_empty() + && points.len() == lay.pts.len() + && lone_old.len() == points.len() + && same_geometry(&old, &fresh) + { + return Ok(false); + } + } // Rebuild: drop the old entities (constraints on them go too) and make new ones. self.drop_link_entities(id); self.build_link(id, &lay)?;