diff --git a/scripts/gui-ladder.py b/scripts/gui-ladder.py index bac00a72c4..1e6547f8f9 100644 --- a/scripts/gui-ladder.py +++ b/scripts/gui-ladder.py @@ -1101,11 +1101,83 @@ def reopen_sketch(): return describe() +def rung_mirror_arcs(): + """C4b — mirror a shape that HAS ARCS. The rung above mirrors three straight lines, which is + why it sat green through the defect a user hit on 2026-08-23: a slot mirrored about a vertical + line came back with its caps at r=32.2 and a 237 deg sweep, one rail collapsed from 62.9 mm to + 2.1 mm, and the ORIGINAL was wrecked along with the copy. An arc has five degrees of freedom + and the copy was bound to its source by its CENTRE alone, so the solver was free to answer with + a different, internally consistent sketch. Circles were unaffected — a circle has no endpoints + to leave free — so the failure read as "circles fine, slots and rounded rectangles destroyed". + + Graded on the property the user actually stated: THE APPLIED RESULT IS THE PREVIEW. The copy is + the source reflected, the source does not move, and no arc comes back reflex. + """ + print("\nC4b mirror — a slot, so the reflection has arcs in it") + enter_sketch("l") + click(*CONSTRUCTION_CHECKBOX) + key("l", 0.6) + draw_line(0, -40, 0, 40, 80, 90) # the axis, on x = 0 + click(*CONSTRUCTION_CHECKBOX) + key("s", 0.6) # slot: two centreline ends, then the width + clickmm(-70, -10); clickmm(-30, -10); clickmm(-30, 0) + values(40, 10, 0) # typed, so the slot is exact before mirroring + d0 = describe()["entities"] + axis = [e for e in d0 if e.get("construction")][0] + slot = [e for e in d0 if not e.get("construction")] + arcs0 = [e for e in slot if e["type"] == "arc"] + check("ARC", len(arcs0) == 2, f"{len(arcs0)} caps on the slot") + + key("m", 0.6) + clickmm(*mid(axis)) + for e in slot: + if e["type"] == "line": + clickmm(*mid(e)) + else: # a point ON the arc, at its mid sweep + a = (e["start_angle"] + e["end_angle"]) / 2.0 + clickmm(e["center"][0] + e["radius"] * math.cos(a), + e["center"][1] + e["radius"] * math.sin(a)) + clickmm(60, 60) # empty space confirms + d1 = describe()["entities"] + check("VERTEX", len(d1) == len(d0) + len(slot), + f"{len(d1) - len(d0)} copies for {len(slot)} picked entities") + + # the sources, entity by entity, must be exactly where they were + def shape_of(e): + if e["type"] == "arc": + return (round(e["radius"], 9), round(abs(e["end_angle"] - e["start_angle"]), 9)) + return (round(math.dist(e["p0"], e["p1"]), 9),) + moved = [i for i, e in enumerate(d0) if shape_of(e) != shape_of(d1[i])] + check("VERTEX", not moved, f"the mirror left every source alone (moved: {moved})") + + # and every copy is its source reflected — endpoints unordered, because a reflection + # reverses orientation and legitimately stores p0/p1 the other way round + (ax, ay), (bx, by) = axis["p0"], axis["p1"] + dx, dy = bx - ax, by - ay + n = math.hypot(dx, dy); dx, dy = dx / n, dy / n + def refl(q): + vx, vy = q[0] - ax, q[1] - ay + k = 2.0 * (vx * dx + vy * dy) + return (ax + k * dx - vx, ay + k * dy - vy) + copies = d1[len(d0):] + worst = 0.0 + for e in slot: + best = min(max(min(max(math.dist(refl(e["p0"]), c["p0"]), math.dist(refl(e["p1"]), c["p1"])), + max(math.dist(refl(e["p0"]), c["p1"]), math.dist(refl(e["p1"]), c["p0"]))), + abs(shape_of(e)[0] - shape_of(c)[0])) + for c in copies if c["type"] == e["type"]) + worst = max(worst, best) + check("SYMMETRY", worst <= 1e-6, f"every copy is the exact reflection (worst {worst:.9f})") + reflex = [c for c in copies + if c["type"] == "arc" and abs(c["end_angle"] - c["start_angle"]) > math.pi + 1e-9] + check("ARC", not reflex, f"{len(reflex)} copied cap(s) came back reflex — the 'cloud' failure") + + RUNGS = {"rect": rung_rect, "circle": rung_circle, "line": rung_line, "arc": rung_arc, "slot": rung_slot, "polygon": rung_polygon, "ellipse": rung_ellipse, "point": rung_point, "spline": rung_spline, "voids": rung_voids, "fillet": rung_fillet, "chamfer": rung_chamfer, "offset": rung_offset, - "mirror": rung_mirror, "trim": rung_trim, "extend": rung_extend, + "mirror": rung_mirror, "mirror_arcs": rung_mirror_arcs, "trim": rung_trim, "extend": rung_extend, "dimension": rung_dimension, "constrain": rung_constrain, "perpendicular": rung_perpendicular, "undo": rung_undo, "feature_undo": rung_feature_undo, "roundtrip": rung_roundtrip, diff --git a/src/slic3r/GUI/CAD/DesignCanvas.hpp b/src/slic3r/GUI/CAD/DesignCanvas.hpp index 1eb70b9eaa..43357dd5e6 100644 --- a/src/slic3r/GUI/CAD/DesignCanvas.hpp +++ b/src/slic3r/GUI/CAD/DesignCanvas.hpp @@ -59,6 +59,9 @@ public: // Open the in-canvas value field on the sketch selection's defining number. bool edit_sketch_selection_value(); int toggle_sketch_construction_selection(); + // Is the sketch tool on Select (as opposed to a draw/edit tool being armed)? The + // Construction box needs it to tell "convert what I picked" from "arm what I draw next". + bool sketch_is_selecting() const { return m_sketch_tool.mode() == DesignSketchTool::Mode::Select; } // Text / SVG art into the LIVE sketch, as ordinary editable lines. False = no session. bool add_sketch_regions(const std::vector>>& regions); void set_sketch_polygon_sides(int n); diff --git a/src/slic3r/GUI/CAD/DesignPanel.cpp b/src/slic3r/GUI/CAD/DesignPanel.cpp index 94d522e6fc..acc0245d4f 100644 --- a/src/slic3r/GUI/CAD/DesignPanel.cpp +++ b/src/slic3r/GUI/CAD/DesignPanel.cpp @@ -1447,9 +1447,35 @@ DesignPanel::DesignPanel(wxWindow* parent) b_poly->Bind(wxEVT_BUTTON, [arm_polygon](wxCommandEvent&) { arm_polygon(); }); sadd(b_poly); // (separator dropped: the group it divided is now reached from the offer) + // Q's semantics, on the control that carries the word. With geometry selected the box + // CONVERTS it — that is what a user who has just selected a construction circle and + // reached for the box labelled "Construction" is asking for, and until now it was the + // only one of the three routes (Q, the offer's Reference row, this box) that could not + // do it: it armed the mode for the NEXT entity, silently, changing nothing about the + // shape on screen and flipping the draw mode behind the user's back. The tick is a MODE + // indicator, so after a conversion it goes back to what it was. m_construction->Bind(wxEVT_CHECKBOX, [this](wxCommandEvent&) { - if (m_viewport && m_viewport->is_sketching()) - m_viewport->set_sketch_construction(m_construction->GetValue()); }); + if (!m_viewport || !m_viewport->is_sketching()) + return; + // ONLY IN SELECT MODE. Drawing auto-selects what was just drawn (draw-then-edit), so + // with a draw tool armed "there is a selection" does not mean the user picked + // anything — it means they finished a line. Converting there turns the box into a + // trap: arm construction, draw the axis, click the box to go back to real geometry, + // and instead of disarming the mode it converts the axis you just drew. The gesture + // ladder's mirror rung does exactly that and reported three construction entities + // where it wanted one. In Select mode the intent is unambiguous. + const int n = m_viewport->sketch_is_selecting() + ? m_viewport->toggle_sketch_construction_selection() : 0; + if (n > 0) { + m_construction->SetValue(!m_construction->GetValue()); // the mode did not move + m_status->SetForegroundColour(wxNullColour); + set_status(wxString::Format( + _L("Converted %d entit%s between construction and real geometry"), + n, n == 1 ? "y" : "ies")); + m_status->Refresh(); + return; + } + m_viewport->set_sketch_construction(m_construction->GetValue()); }); // STAYS on the bar. Construction is not a tool, it is a persistent MODE — the same kind // of thing as the Bed checkbox — and the sketch bar is already shown only in Sketch mode, // so it appears exactly while it can apply. Hiding it left Q and the offer's Construction diff --git a/src/slic3r/GUI/CAD/DesignSketchTool.cpp b/src/slic3r/GUI/CAD/DesignSketchTool.cpp index 9d6a3a223b..ce165e6202 100644 --- a/src/slic3r/GUI/CAD/DesignSketchTool.cpp +++ b/src/slic3r/GUI/CAD/DesignSketchTool.cpp @@ -7727,6 +7727,34 @@ void DesignSketchTool::render_op_gizmo(double unit_per_px) draw_text(m_line_model, dim_text(a), m_op_label, th, dc); } +// Did an entity actually change shape or position? Compares only the fields that define each +// type, so a re-solve that leaves the geometry alone reads as "unchanged" whatever else moved in +// the record. Used by the mirror postcondition below. +static bool entity_moved(const SketchEntity& a, const SketchEntity& b, double tol) +{ + if (a.type != b.type) return true; + auto far = [tol](const Vec2d& p, const Vec2d& q) { return (p - q).norm() > tol; }; + if (far(a.p0, b.p0)) return true; + switch (a.type) { + case SketchEntity::Type::Point: + return false; + case SketchEntity::Type::Line: + return far(a.p1, b.p1); + case SketchEntity::Type::Circle: + return far(a.center, b.center) || std::abs(a.radius - b.radius) > tol; + case SketchEntity::Type::Arc: + return far(a.p1, b.p1) || far(a.center, b.center) + || std::abs(a.radius - b.radius) > tol + || std::abs((a.end_angle - a.start_angle) - (b.end_angle - b.start_angle)) > tol; + case SketchEntity::Type::Ellipse: + case SketchEntity::Type::EllipseArc: + return far(a.center, b.center) || std::abs(a.radius - b.radius) > tol + || std::abs(a.rminor - b.rminor) > tol || std::abs(a.rotation - b.rotation) > tol; + default: + return far(a.p1, b.p1); + } +} + void DesignSketchTool::confirm_op() { if (!op_ready()) return; @@ -7788,24 +7816,79 @@ void DesignSketchTool::confirm_op() if (emit) try_add_constraints({ d }); } else if (m_mode == Mode::Mirror) { const SketchEntity axis = m_entities[m_op_a]; // by value (m_entities grows below) + // The sources as they stand BEFORE any of this op's constraints exist. Two jobs: every + // copy is reflected from the untouched original (so a batch that moves the sketch cannot + // feed a later copy moved geometry), and the invariant at the bottom has something to + // compare against. snaporca-mirror-slot. + const std::vector before = m_entities; + const size_t cmark = m_constraints.size(); + std::vector> fresh; // copy index -> its pristine reflection for (int ti : m_mirror_targets) { - if (ti < 0 || ti >= int(m_entities.size())) continue; - auto out = SketchEngine::mirror_entities({ m_entities[ti] }, axis.p0, axis.p1); + if (ti < 0 || ti >= int(before.size())) continue; + auto out = SketchEngine::mirror_entities({ before[ti] }, axis.p0, axis.p1); if (out.empty()) continue; const int mi = int(m_entities.size()); - for (auto& m : out) m_entities.push_back(m); + for (auto& m : out) { fresh.emplace_back(int(m_entities.size()), m); m_entities.push_back(m); } SketchEntityConstraintDef d; d.type = CT::Symmetric; d.ea = ti; d.eb = mi; d.ec = m_op_a; - const SketchEntity::Type st = m_entities[ti].type; - std::vector cand; + const SketchEntity::Type st = before[ti].type; + std::vector> ladder; if (st == SketchEntity::Type::Line) { - d.ra = R::P0; d.rb = R::P0; cand.push_back(d); - d.ra = R::P1; d.rb = R::P1; cand.push_back(d); - } else if (st == SketchEntity::Type::Arc || st == SketchEntity::Type::Circle) { - d.ra = R::Center; d.rb = R::Center; cand.push_back(d); + d.ra = R::P0; d.rb = R::P0; auto p0 = d; + d.ra = R::P1; d.rb = R::P1; auto p1 = d; + ladder = { { p0, p1 }, { p0 } }; + } else if (st == SketchEntity::Type::Arc) { + // BOTH ENDS AND THE CENTRE. Binding only the centre — which is all this did — + // leaves the copy's endpoints and sweep free while the shape's own coincidences + // still tie them to its neighbours, and the solver then answers with a wildly + // different, internally consistent sketch: a slot's caps came back at r=32.2 and + // a 237 deg sweep, one rail collapsed from 62.9 mm to 2.1 mm, and the ORIGINAL + // moved with them. A circle survived the same code only because a circle has no + // endpoints to leave free, which is why the bug reads as "circles fine, rounded + // rectangles and slots destroyed". + d.ra = R::Center; d.rb = R::Center; auto ct = d; + d.ra = R::P0; d.rb = R::P0; auto p0 = d; + d.ra = R::P1; d.rb = R::P1; auto p1 = d; + // Endpoints BEFORE centre: an arc is five DoF, so {centre, p0, p1} is six + // equations and is refused; {p0, p1} is four and pins the sweep, which is the + // half that was going wild. The postcondition below is what makes the ladder + // safe — any rung that does not reproduce the preview is thrown away whole. + ladder = { { p0, p1 }, { ct, p0 }, { ct } }; + } else if (st == SketchEntity::Type::Circle) { + d.ra = R::Center; d.rb = R::Center; ladder = { { d } }; } else if (st == SketchEntity::Type::Point) { - d.ra = R::P0; d.rb = R::P0; cand.push_back(d); + d.ra = R::P0; d.rb = R::P0; ladder = { { d } }; } - if (!cand.empty()) try_add_constraints(cand); + for (const auto& set : ladder) if (try_add_constraints(set)) break; + } + // A MIRROR MAY NOT MOVE WHAT IT COPIED. try_add_constraints only rolls back when the + // solve FAILS, and the failure here is a solve that succeeds at something else: the + // numbers above came out of a solver that was perfectly happy. So the op checks its own + // postcondition on the geometry, and if a source moved it keeps the copies — which are + // exactly what the preview showed — and drops the whole constraint web that moved them. + // Restoring the sources needs no re-solve: the pre-batch state was itself solved, and a + // failed solve does not write back (snaporca-pl5). + // BOTH HALVES. Watching only the sources caught the slot (whose web dragged everything) + // and missed the rounded rectangle, where the solver held the sources still and put the + // COPIES somewhere else: an arc has five degrees of freedom and Symmetric on centre plus + // both endpoints is six equations, so that batch is refused and the ladder degrades to a + // set that leaves the sweep free. The rule that covers both, and that is what the user + // actually asked for, is: THE APPLIED RESULT IS THE PREVIEW. Anything else drops the web. + bool disturbed = false; + for (size_t i = 0; i < before.size() && !disturbed; ++i) + disturbed = entity_moved(before[i], m_entities[i], 1e-6); + for (const auto& f : fresh) + if (!disturbed && f.first < int(m_entities.size())) + disturbed = entity_moved(f.second, m_entities[f.first], 1e-6); + if (disturbed) { + m_constraints.resize(cmark); + for (size_t i = 0; i < before.size(); ++i) m_entities[i] = before[i]; + // The copies too, and for the same reason: a batch that moved the sketch moved them + // as well, so the ones sitting in m_entities are the solver's answer, not the + // reflection. Restoring only the sources left a slot whose copy came back with a + // 13.8 mm rail and a 308 deg cap — the original was safe and the copy was still + // wrong, which is half a fix. `fresh` is what the preview drew. + for (const auto& f : fresh) + if (f.first < int(m_entities.size())) m_entities[f.first] = f.second; } } reset_op();