Merge pull request #176 from 0xlillith/fix/coincident-line-picks

fix: connect closest endpoints when Coincident picks two lines

crates/engine/src/cmd/sketch.rs

@@ -114,7 +114,7 @@ pub static COMMANDS: &[CommandSpec] = &[
         .at("SKETCH", "CONSTRAINTS")
         .icon("c_coincident")
         .enabled(in_sketch)
-        .params("a: point, b: point or curve"),
+        .params("a, b: points, a point and a curve, or two distinct lines (joins their closest endpoints)"),
     CommandSpec::new("sketch.constraint.tangent", "Tangent", c_tangent)
         .at("SKETCH", "CONSTRAINTS")
         .icon("c_tangent")
@@ -1235,13 +1235,17 @@ fn c_coincident(s: &mut Session, p: &Value) -> Result<Value> {
     // Already tied (e.g. connected when it was drawn there): nothing to add.
     let sk = s.doc.sketch(target_sketch(s, p, cmd)?)?;
     let pt = |k: &str| p.get(k).and_then(Value::as_str).and_then(|r| sk.resolve_point(r));
-    if let (Some(a), Some(b)) = (pt("a"), pt("b"))
+    let line_points = coincident_line_points(sk, p, cmd)?;
+    if let Some((a, b)) = line_points.or_else(|| pt("a").zip(pt("b")))
         && let Some(c) = sk.constraints.iter().find(|c| matches!(c.kind, ConstraintKind::Coincident { p, q } if (p, q) == (a, b) || (p, q) == (b, a)))
     {
         let rep = solve(&mut sk.clone());
         return Ok(json!({"constraint": c.id, "existing": true, "sketch": {"solved": rep.ok(), "dof": rep.dof}}));
     }
     constrain(s, p, cmd, |sk| {
+        if let Some((a, b)) = line_points {
+            return Ok(ConstraintKind::Coincident { p: a, q: b });
+        }
         let aref = p.get("a").and_then(Value::as_str).unwrap_or_default();
         let bref = p.get("b").and_then(Value::as_str).ok_or_else(|| bad(cmd, "`b` must be a point or curve"))?;
         // A curve picked first and a point second: the point goes onto the curve.
@@ -1257,6 +1261,37 @@ fn c_coincident(s: &mut Session, p: &Value) -> Result<Value> {
     })
 }
 
+/// Four endpoint pairs, ordered independently of pick order when distances tie.
+fn coincident_line_points(sk: &Sketch, p: &Value, cmd: &str) -> Result<Option<(usize, usize)>> {
+    let curve = |key: &str| p.get(key).and_then(Value::as_str).and_then(|id| sk.curve_index(id));
+    let (Some(a), Some(b)) = (curve("a"), curve("b")) else { return Ok(None) };
+    if a == b {
+        return Err(bad(cmd, "choose two distinct lines"));
+    }
+    let ends = |i: usize| match sk.curves.get(i).map(|curve| &curve.kind) {
+        Some(CurveKind::Line { a, b }) => Some([*a, *b]),
+        _ => None,
+    };
+    let (Some(first), Some(second)) = (ends(a), ends(b)) else {
+        return Err(bad(cmd, "two curves must both be straight lines; otherwise choose a point and a curve"));
+    };
+    let mut best: Option<(f64, usize, usize)> = None;
+    for a in first {
+        for b in second {
+            let (Some(pa), Some(pb)) = (sk.point(a), sk.point(b)) else { return Err(bad(cmd, "line endpoint is missing")) };
+            let candidate = (pa.dist(pb), a.min(b), a.max(b));
+            if best.is_none_or(|old| candidate < old) {
+                best = Some(candidate);
+            }
+        }
+    }
+    let (_, a, b) = best.ok_or_else(|| bad(cmd, "line endpoints are missing"))?;
+    if a == b {
+        return Err(bad(cmd, "the lines already share an endpoint"));
+    }
+    Ok(Some((a, b)))
+}
+
 fn two_curves(sk: &Sketch, p: &Value, cmd: &str) -> Result<(usize, usize)> {
     Ok((curve_ref(sk, p.get("a"), cmd, "a")?, curve_ref(sk, p.get("b"), cmd, "b")?))
 }

crates/engine/src/cmd/sketch_ux_tests.rs

@@ -401,6 +401,39 @@ fn point_constraints_keep_the_first_pick() {
     assert!(pt(&s, "p2").dist(solvecraft_geom::Vec2::new(3.0, -7.0)) < 1e-6, "{:?}", pt(&s, "p2"));
 }
 
+#[test]
+fn coincident_line_pairs_resolve_ties_independently_of_pick_order() {
+    for (a, b) in [("l1", "l2"), ("l2", "l1")] {
+        let mut s = new_sketch();
+        run(&mut s, "sketch.line", json!({"points":[[10,10],[30,10]],"infer":false,"connect":false}));
+        run(&mut s, "sketch.line", json!({"points":[[10,20],[30,20]],"infer":false,"connect":false}));
+        run(&mut s, "sketch.constraint.coincident", json!({"a":a,"b":b}));
+        let sk = sketch(&s);
+        let p = sk.resolve_point("l1.start").unwrap();
+        let q = sk.resolve_point("l2.start").unwrap();
+        assert!(sk.point(p).unwrap().dist(sk.point(q).unwrap()) < 1e-6);
+        let count = sk.constraints.len();
+        let result = run(&mut s, "sketch.constraint.coincident", json!({"a":b,"b":a}));
+        assert_eq!(result["existing"], true);
+        assert_eq!(sketch(&s).constraints.len(), count);
+    }
+}
+
+#[test]
+fn coincident_refuses_invalid_curve_pairs_atomically() {
+    let mut s = new_sketch();
+    run(&mut s, "sketch.line", json!({"points":[[10,10],[30,10]],"infer":false,"connect":false}));
+    run(&mut s, "sketch.circle.center", json!({"center":[40,40],"radius":5}));
+    let before = serde_json::to_value(&s.doc).unwrap();
+    let undo = s.undo.len();
+    for (a, b, message) in [("l1", "l1", "distinct lines"), ("l1", "c1", "straight lines"), ("c1", "l1", "straight lines")] {
+        let error = s.execute("sketch.constraint.coincident", &json!({"a":a,"b":b})).unwrap_err();
+        assert!(error.to_string().contains(message), "{error}");
+        assert_eq!(serde_json::to_value(&s.doc).unwrap(), before);
+        assert_eq!(s.undo.len(), undo);
+    }
+}
+
 // ---------------------------------------------------------------------------------------------
 // Sketch Palette options
 

crates/ui-egui/tests/coincident_lines.rs

@@ -0,0 +1,63 @@
+//! Coincident accepts clicked line bodies as well as explicit sketch points.
+
+use serde_json::json;
+use solvecraft_engine::sketch::ConstraintKind;
+use solvecraft_ui_egui::scenario::Harness;
+
+fn setup() -> Harness {
+    let mut h = Harness::new();
+    h.run(&json!([
+        {"call":"engine.execute","params":{"command":"sketch.create","params":{"plane":"XY"}}},
+        {"call":"engine.execute","params":{"command":"sketch.line","params":{"points":[[10,10],[30,10]],"infer":false,"connect":false}}},
+        {"call":"engine.execute","params":{"command":"sketch.line","params":{"points":[[35,15],[35,40]],"infer":false,"connect":false}}},
+        {"call":"ui.view","params":{"view":"top"}}
+    ]))
+    .unwrap();
+    h
+}
+
+#[test]
+fn clicked_lines_join_closest_endpoints_in_both_orders() {
+    for reverse in [false, true] {
+        let mut h = setup();
+        let before = serde_json::to_value(&h.app.session.doc).unwrap();
+        let undo = h.app.session.undo.len();
+        let picks = if reverse { [[35.0, 27.5], [20.0, 10.0]] } else { [[20.0, 10.0], [35.0, 27.5]] };
+        h.run(
+            &json!([{"start":"sketch.constraint.coincident"}, {"click":{"sketch":picks[0]}}, {"click":{"sketch":picks[1]}}, {"expect":{"errors":0}}]),
+        )
+        .unwrap();
+        assert!(!h.app.status.as_ref().is_some_and(|(_, _, error)| *error));
+        let sk = h.app.session.doc.sketch(h.app.session.active_sketch.unwrap()).unwrap();
+        let a = sk.resolve_point("l1.end").unwrap();
+        let b = sk.resolve_point("l2.start").unwrap();
+        assert!(sk.point(a).unwrap().dist(sk.point(b).unwrap()) < 1e-6);
+        assert!(sk.constraints.iter().any(|c| matches!(c.kind, ConstraintKind::Coincident {p,q} if (p,q)==(a,b) || (p,q)==(b,a))));
+        let fixed_ref = if reverse { "l2.end" } else { "l1.start" };
+        let expected = if reverse { [35.0, 40.0] } else { [10.0, 10.0] };
+        let fixed = sk.point(sk.resolve_point(fixed_ref).unwrap()).unwrap();
+        assert!((fixed.x - expected[0]).abs() < 1e-6 && (fixed.y - expected[1]).abs() < 1e-6);
+        assert_eq!(h.app.session.undo.len(), undo + 1);
+        let after = serde_json::to_value(&h.app.session.doc).unwrap();
+        h.run(&json!([{"call":"engine.execute","params":{"command":"edit.undo","params":{}}}])).unwrap();
+        assert_eq!(serde_json::to_value(&h.app.session.doc).unwrap(), before);
+        h.run(&json!([{"call":"engine.execute","params":{"command":"edit.redo","params":{}}}])).unwrap();
+        assert_eq!(serde_json::to_value(&h.app.session.doc).unwrap(), after);
+    }
+}
+
+#[test]
+fn fixed_line_pair_rejects_without_changing_document_or_history() {
+    let mut h = setup();
+    h.run(&json!([
+        {"call":"engine.execute","params":{"command":"sketch.constraint.fix","params":{"entity":"l1"}}},
+        {"call":"engine.execute","params":{"command":"sketch.constraint.fix","params":{"entity":"l2"}}}
+    ]))
+    .unwrap();
+    let before = serde_json::to_value(&h.app.session.doc).unwrap();
+    let undo = h.app.session.undo.len();
+    h.run(&json!([{"start":"sketch.constraint.coincident"},{"click":{"sketch":[20,10]}},{"click":{"sketch":[35,27.5]}}])).unwrap();
+    assert!(h.app.status.as_ref().is_some_and(|(_, _, error)| *error));
+    assert_eq!(serde_json::to_value(&h.app.session.doc).unwrap(), before);
+    assert_eq!(h.app.session.undo.len(), undo);
+}

docs/book/src/guide/sketching.md

@@ -91,6 +91,11 @@ midpoint, symmetry, fix, curvature (G2) and polygon. **Sketch Dimension** (D) ad
 aligned, horizontal, vertical, radius, diameter and angle dimensions. Each is a parameter
 (`d1`, `d2`, …), so it can hold an expression.
 
+For **Coincident**, pick two points, a point and a curve (in either order), or two
+distinct straight lines. Two lines join their closest pair of endpoints; the first
+picked line stays in place when the existing constraints allow it. Pick endpoints
+explicitly when you want to connect a different pair.
+
 For Parallel, Perpendicular, Equal, Tangent, Concentric and Collinear, select exactly two
 sketch curves (Shift-click to add the second) and choose the constraint to apply it immediately.
 You can also start the constraint first and then pick its curves. An incomplete or mixed