mirror of
https://github.com/OrcaSlicer/OrcaSlicer.git
synced 2026-09-18 14:32:36 +00:00
Mirror stops destroying arcs, and the Construction box converts what you picked
Two user reports from the same session on the deployed build, 2026-08-23.
FIRST: "if I select a shape (es a circle draw in construction lines) and then I
try to toggle contruction to obtain a full line, does not work". Reproduced: Q
converts the selection and so does the offer's Reference > Construction row —
both run m_keys_sketch['Q'] — but the CHECKBOX, the one control actually
labelled Construction, only ever called set_sketch_construction(), which arms
the mode for the NEXT entity. So the obvious control was the one route that
could not convert existing geometry, and it failed silently while also flipping
the draw mode behind the user's back. It now carries Q's meaning.
Scoped to Select mode, and that scoping is not cosmetic: 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. The first version converted there and turned 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 converted the axis just drawn. The gesture
ladder's C4 rung does exactly that and reported three construction entities
where it wanted one. In Select mode the intent is unambiguous.
SECOND, and this one destroyed work: "after creation of a circle, a round angled
rectangle and a slot, and mirror of those shapes on a vertical line inside a
outer rectangle, preview is ok but application creates errors: the circle is
mirrored, but rectangle and slot are redrawn as pieces of circles screwing both
the original shapes and the copies." A screenshot came with it, and it showed
more than the words did: the ORIGINALS were wrecked too — the rounded rectangle
was drawn as a four-lobed cloud, each corner fillet having gone the long way
round, and the slot had ballooned into two near-full circles.
Measured on the rig, a slot mirrored about a vertical line:
rails 62.873 / 62.873 -> 2.082 / 62.913
caps r=21.554 sweep=-180.00 -> r=32.214 sweep=-237.66
and all four sources moved, the axis line with them
Cause: confirm_op's Mirror branch bound an Arc copy to its source with a
Symmetric constraint on the CENTRE ALONE. An arc has five degrees of freedom;
pinning two of them leaves the endpoints and the sweep free while the shape's
own coincidences still pull on them, and the solver answers with a different,
internally consistent sketch — which is what a reflex cap and a 2 mm rail are.
A circle came through the same code untouched because a circle HAS no endpoints
to leave free, which is exactly why the failure reads as "circles fine, rounded
rectangles and slots destroyed".
Three parts, and each one is here because the measurement caught the previous
one being half a fix:
1. Arcs are bound by BOTH ENDPOINTS. Endpoints before centre in the ladder:
{p0, p1} is four equations against five DoF and pins the sweep, while
{centre, p0, p1} is six and is refused — the refusal is what silently
degraded the batch to a set that left the sweep free.
2. Every copy is reflected from the PRE-BATCH source, so a batch that disturbs
the sketch cannot hand the next copy already-moved geometry.
3. THE APPLIED RESULT IS THE PREVIEW — checked on the sources AND the copies,
and on violation the whole constraint web is dropped and both halves are
restored to the reflection the preview drew. try_add_constraints rolls back
only when a solve FAILS, and every failure here came from a solve that
succeeded at something else. Guarding only the sources fixed the slot and
left the rounded rectangle's copies at a 13.8 mm rail and a 308 degree cap:
the original was safe and the copy was still wrong, which is half a fix.
The parametric link is kept whenever it provably holds the geometry, and dropped
when it does not. A wrong shape is worse than an unlinked one.
WHY NOTHING CAUGHT THIS: the gesture ladder's mirror rung reflects three
straight LINES. It sat green through the whole defect. C4b now mirrors a slot,
so the reflection has arcs in it, and grades the property the user actually
stated: the copy is the source reflected, the source does not move, and no cap
comes back reflex.
VERIFIED against the user's own scene, rebuilt gesture by gesture on the rig —
outer rectangle, circle, rounded rectangle and slot, a vertical CONSTRUCTION
line as the axis, all 17 entities mirrored in one gesture:
ok the mirror axis is a construction line
ok picked the axis and all 17 entities
ok originals unchanged (moved: [])
ok every copy is the exact reflection (worst 0.000000000)
ok no source arc turned reflex — the 'cloud' failure
ok no copied arc turned reflex (6 arcs checked)
One grader correction worth recording, because it cost a round and would cost
the next one too: a reflection REVERSES ORIENTATION, so a copy legitimately
stores p0/p1 the other way round. Comparing p0 to p0 grades the storage order,
not the geometry, and reported a perfect mirror as an 8.98 mm error. Endpoints
are compared as an unordered pair.
This commit is contained in:
Tommaso Bianchi
+73
-1
@@ -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,
|
||||
|
||||
@@ -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<std::vector<std::vector<Vec2d>>>& regions);
|
||||
void set_sketch_polygon_sides(int n);
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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<SketchEntity> before = m_entities;
|
||||
const size_t cmark = m_constraints.size();
|
||||
std::vector<std::pair<int, SketchEntity>> 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<SketchEntityConstraintDef> cand;
|
||||
const SketchEntity::Type st = before[ti].type;
|
||||
std::vector<std::vector<SketchEntityConstraintDef>> 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();
|
||||
|
||||
Reference in New Issue
Block a user