diff --git a/src/scene/sketch_solve.rs b/src/scene/sketch_solve.rs index 3959c1db1..d2e36a030 100644 --- a/src/scene/sketch_solve.rs +++ b/src/scene/sketch_solve.rs @@ -50,6 +50,32 @@ //! `Normal`'s own inline matches (which don't go through `whole_circle`) //! have explicit `EntityGeom::Arc` arms alongside their `Circle` ones for //! the same reason. +//! +//! An `Ellipse` registers as `ocs_gcs::geo::Ellipse { center, focus1, +//! radmin }` (`register_entity`'s `Ellipse` arm converts acadrust's +//! center/major-axis-vector/ratio parametrization to this one). Unlike +//! Arc's `start`/`end`, `focus1` isn't an extra derived point on top of the +//! "real" shape params — it *is* one, jointly with `radmin`, encoding +//! orientation and eccentricity. But it's stored as an absolute point, not +//! an offset from `center`, so a constraint that pulls only on `center` +//! (Concentric/CenterPoint being the natural case) leaves `focus1` +//! genuinely untouched — correct for the solver, wrong for the geometry: +//! `center` moved, `focus1` didn't, so the vector `focus1 - center` (what +//! write-back actually derives `major_axis`/`minor_axis_ratio` from) has +//! changed, and the ellipse appears to rotate and reshape even though +//! nothing asked it to. `solve_scope` adds two ellipse-rules +//! `ocs_gcs::constraints::point_line::Difference` constraints per +//! registered ellipse — `focus1.x - center.x` and `focus1.y - center.y` +//! pinned to their pre-solve values — so `focus1` translates rigidly with +//! `center` whenever nothing else constrains it, the same "stays put +//! unless something actually pulls on it" behavior Arc's own spare DOF +//! already has. `radmin` needs no equivalent: as a lone scalar it isn't +//! coupled to `center`'s movement, so it already stays at its seed value +//! once unreferenced by anything. Because these rules already pin `focus1` +//! once `center` is pinned, `Fixed`'s `Ellipse` arm only pins `center`/ +//! `radmin` directly, not `focus1` — pinning it too would be redundant +//! with the rules, same as `Fixed`'s `Arc` arm not re-pinning `start`/ +//! `end`. use std::collections::HashMap; use std::rc::Rc; @@ -85,10 +111,12 @@ enum EntityGeom { /// for every registered arc. See this module's doc comment. Arc(GArc), /// Center-and-shape support only — see `register_entity`'s `Ellipse` - /// arm for the acadrust-to-`ocs_gcs` parametrization conversion, and - /// this module's doc comment for what isn't wired up yet (major/minor - /// axis dimensional constraints, `PointOnEllipse`-based Coincident/ - /// Tangent). + /// arm for the acadrust-to-`ocs_gcs` parametrization conversion, the + /// ellipse-rules `Difference` constraints `solve_scope` adds for every + /// registered ellipse to keep `focus1` translating rigidly with + /// `center` (this module's doc comment), and that same doc comment for + /// what isn't wired up yet (major/minor axis dimensional constraints, + /// `PointOnEllipse`-based Coincident/Tangent). Ellipse(GEllipse), } @@ -412,16 +440,20 @@ fn build_constraint( Rc::new(Equal::new(a.end_angle, sys.add_param(ea, true), 1.0)), ] } + // Pinning center + radmin is enough — `focus1` is already + // tied to `center` by the ellipse-rules `Difference` + // constraints `solve_scope` adds for every registered + // ellipse (see this module's doc comment), so with `center` + // pinned here too, pinning `focus1` again would just be + // redundant (same reasoning as Arc's `Fixed` arm above). EntityGeom::Ellipse(el) => { - let (cx, cy, fx, fy, b) = { + let (cx, cy, b) = { let store = sys.store(); - (store.get(el.center.x), store.get(el.center.y), store.get(el.focus1.x), store.get(el.focus1.y), store.get(el.radmin)) + (store.get(el.center.x), store.get(el.center.y), store.get(el.radmin)) }; vec![ Rc::new(Equal::new(el.center.x, sys.add_param(cx, true), 1.0)), Rc::new(Equal::new(el.center.y, sys.add_param(cy, true), 1.0)), - Rc::new(Equal::new(el.focus1.x, sys.add_param(fx, true), 1.0)), - Rc::new(Equal::new(el.focus1.y, sys.add_param(fy, true), 1.0)), Rc::new(Equal::new(el.radmin, sys.add_param(b, true), 1.0)), ] } @@ -615,6 +647,33 @@ fn solve_scope( sys.add_constraint(Rc::new(CurveValue::new(arc.end, arc.end.y, curve, arc.end_angle))); } + // Ellipse rules (same motivation as arc rules, different mechanism): + // `ocs_gcs::geo::Ellipse` stores `focus1` as an absolute point, not an + // offset from `center`. If some constraint (e.g. Concentric) pulls only + // on `center` and nothing references `focus1`/`radmin`, the solver + // correctly leaves `focus1`'s *absolute* coordinates untouched — but + // `center` moved and `focus1` didn't, so the derived vector + // `focus1 - center` (which is what `major_axis` direction/length and + // `minor_axis_ratio` are actually computed from on write-back) changes + // anyway, making the ellipse appear to rotate and reshape even though + // no shape parameter was itself pulled on. Pinning `focus1 - center` to + // its pre-solve (seed) value forces `focus1` to translate rigidly with + // `center`, keeping orientation/eccentricity invariant unless something + // actually constrains `focus1`/`radmin` directly (only `Fixed` does + // today). `radmin` needs no equivalent rule — as a lone scalar it isn't + // coupled to `center`'s movement, so it already stays put on its own. + for geom in cache.values() { + let EntityGeom::Ellipse(el) = geom else { continue }; + let (cx, cy, fx, fy) = { + let store = sys.store(); + (store.get(el.center.x), store.get(el.center.y), store.get(el.focus1.x), store.get(el.focus1.y)) + }; + let dx = sys.add_param(fx - cx, true); + let dy = sys.add_param(fy - cy, true); + sys.add_constraint(Rc::new(Difference::new(el.center.x, el.focus1.x, dx))); + sys.add_constraint(Rc::new(Difference::new(el.center.y, el.focus1.y, dy))); + } + let partitions = sys.partition(); for sub in &partitions { solve_dl(sub, sys.store_mut()); diff --git a/tests/sketch_constraints_solve.rs b/tests/sketch_constraints_solve.rs index 194bcd597..0c326db38 100644 --- a/tests/sketch_constraints_solve.rs +++ b/tests/sketch_constraints_solve.rs @@ -947,6 +947,48 @@ fn a_tilted_ellipse_round_trips_through_the_solver_without_drifting() { assert!((ratio - 0.6).abs() < 1e-6, "minor/major ratio should not have drifted: {ratio}"); } +#[test] +fn concentric_constraint_does_not_reshape_the_ellipse_when_its_center_actually_moves() { + // Regression test for a real solver bug: `a_tilted_ellipse_round_trips_ + // through_the_solver_without_drifting` (above) uses a circle that's + // ALREADY concentric with the ellipse, so Concentric's equations start + // out satisfied and the ellipse's center never actually has to move — + // it doesn't exercise the bug at all. Here the circle starts elsewhere, + // forcing a real center move, which is exactly when `ocs_gcs::geo:: + // Ellipse`'s absolute `focus1` point used to get left behind: `center` + // moved to meet the circle, `focus1` didn't (nothing else referenced + // it), so the derived major-axis direction/length and minor/major + // ratio changed even though only a center-to-center constraint was + // ever applied. The fix ties `focus1`'s offset from `center` to its + // seeded value, so it translates along with `center` instead. + let mut scene = Scene::new(); + let ellipse = add_ellipse(&mut scene, 0.0, 0.0, (4.0, 0.0), 0.5); + let circle = add_circle(&mut scene, 10.0, 7.0, 1.5); + + scene.sketch_constraint_set_mut(SketchScope::ModelSpace).add( + ConstraintKind::Concentric, + vec![SketchRef::center(ellipse), SketchRef::center(circle)], + None, + ); + scene.bump_entities(&[(ellipse, ChangeKind::Modified), (circle, ChangeKind::Modified)]); + + let (ellipse_center, major_axis, ratio) = ellipse_geom(&scene, ellipse); + let (circle_center, _) = circle_geom(&scene, circle); + + assert!( + (ellipse_center.x - circle_center.x).abs() < 1e-6 && (ellipse_center.y - circle_center.y).abs() < 1e-6, + "concentric constraint should still hold: ellipse={ellipse_center:?} circle={circle_center:?}" + ); + // The whole point: the center was free to move (nothing else pinned + // either entity), so it's not expected to still sit at (0,0) — but the + // ellipse's *shape* must be exactly what it was before solving. + assert!( + (major_axis.x - 4.0).abs() < 1e-6 && (major_axis.y - 0.0).abs() < 1e-6, + "major axis must not rotate or resize just because the center moved: {major_axis:?}" + ); + assert!((ratio - 0.5).abs() < 1e-6, "minor/major ratio must not change just because the center moved: {ratio}"); +} + #[test] fn fixed_constraint_holds_an_ellipse_in_place_despite_a_connected_edit() { let mut scene = Scene::new();