From 347b83d8872f44e2fec293bfc7c4ab9c90520db2 Mon Sep 17 00:00:00 2001 From: Tommaso Bianchi Date: Sun, 26 Jul 2026 23:33:50 +0200 Subject: [PATCH] Sketch fillet: never write a failed solve's geometry back, and commit the op MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two P0s in the same gesture. Filleting a corner of a parametric rectangle produced either nothing at all or a sharp corner with a stray arc floating above it. Trigger (snaporca-cq2): the only routes that ever reached confirm_op were finishing the whole sketch and an unsignposted click on empty space. set_tool() dropped a ready op, so typing a radius or dragging the arrow and then touching any other tool threw the value away. Commit a ready op on tool change (before m_mode is reassigned — op_ready() and confirm_op() both switch on it), commit on Enter in the radius editor, and drop the pending op before Esc's tool downgrade so Esc still cancels rather than applies. Substitution (snaporca-pl5): libslvs writes its last Newton iterate into the params whether or not it converged, and SketchSolver read them back unconditionally, so every REJECTED solve deformed the sketch. The fillet ladder tries a deliberately over-constrained rung first (a tangent on each leg, against the legs' own H/V); it is correctly rejected, but its wreckage then failed rungs 2 and 3, which solve cleanly on their own. The arc ended up with no constraints at all, the rigid loop won, and the corner snapped shut. Measured: from pristine geometry rung 1 gives result=INCONSISTENT with 3 bad constraints, rung 2 gives dof=6 with the arc's radius intact. Read the geometry back only on success. try_add_constraints then needs no "restore" re-solve — the entities still hold the prior solved state. Kernel suite 151 cases / 2072 assertions green on both forks; the GUI check ran on the Snapmaker fork (9d72c4377a). Ported from the Snapmaker fork. snaporca-pl5 snaporca-cq2 --- src/libslic3r/SketchSolver.cpp | 9 ++++ src/slic3r/GUI/DesignSketchTool.cpp | 32 ++++++++++-- tests/libslic3r/test_caddocument.cpp | 77 ++++++++++++++++++++++++++++ 3 files changed, 114 insertions(+), 4 deletions(-) diff --git a/src/libslic3r/SketchSolver.cpp b/src/libslic3r/SketchSolver.cpp index f13f02b371..861709ea56 100644 --- a/src/libslic3r/SketchSolver.cpp +++ b/src/libslic3r/SketchSolver.cpp @@ -333,6 +333,15 @@ static SketchSolveResult solve_impl(std::vector& entities, } // ---- Read solved geometry back -------------------------------------------------- + // ONLY on success. A failed solve leaves libslvs' params holding its last Newton + // iterate — geometry that satisfies nothing and is usually wildly deformed. Writing + // that back made every rejected attempt destructive: the caller rolls the constraints + // back, but the sketch it rolls back to is already wreckage, so the next attempt starts + // from the corpse. The fillet degrade ladder hit this on every corner — rung 1 (a + // tangent on each leg) is legitimately over-constrained against the legs' own H/V, and + // its wreckage then failed rungs 2 and 3, which solve cleanly on their own. The arc + // ended up with no constraints at all and the solver snapped the corner shut. snaporca-pl5. + if (!out.ok) return out; for (size_t i = 0; i < entities.size(); ++i) { SketchEntity& e = entities[i]; const Slots& s = slot[i]; diff --git a/src/slic3r/GUI/DesignSketchTool.cpp b/src/slic3r/GUI/DesignSketchTool.cpp index 05596f0874..d25c204172 100644 --- a/src/slic3r/GUI/DesignSketchTool.cpp +++ b/src/slic3r/GUI/DesignSketchTool.cpp @@ -205,6 +205,16 @@ void DesignSketchTool::rebuild_features_from_entities() void DesignSketchTool::set_tool(Mode mode) { + // A READY edit-op carries the user's typed or dragged value, so switching tools commits it + // rather than dropping it — the same rule Tab follows in the dimension editor. Discarding it + // here is most of why Fillet looked like it simply did not work: every documented route (type + // the radius, or drag the arrow) left the op ready-but-pending, and the next tool click threw + // the value away and reverted the corner to sharp, with nothing on screen saying so. + // This MUST run before m_mode is reassigned: op_ready() and confirm_op() both switch on + // m_mode, so after the assignment they would test the tool being switched TO. That read + // op_ready()==0 with a=0 b=3 val=28.205 sitting right there — picked, valued, and dropped. + if (op_ready()) confirm_op(); + // Switch the active drawing tool without dropping accumulated entities. m_mode = mode; m_points.clear(); @@ -212,7 +222,12 @@ void DesignSketchTool::set_tool(Mode mode) m_awaiting_length = false; m_autoedit_seen = int(m_entities.size()); // resync baseline so a switch never fires m_autoedit_pending = false; - reset_op(); // drop any in-progress edit-op gizmo + // A READY edit-op carries the user's typed or dragged value, so switching tools commits it + // rather than dropping it — the same rule Tab follows in the dimension editor. Discarding it + // here is most of why Fillet looked like it simply did not work: every documented route (type + // the radius, or drag the arrow) left the op ready-but-pending, and the next tool click threw + // the value away and reverted the corner to sharp, with nothing on screen saying so. + reset_op(); // drop any in-progress (not yet ready) edit-op gizmo reset_tf(); // drop any in-progress transform gizmo m_selection.clear(); if (on_selection_changed) on_selection_changed(0); @@ -250,7 +265,9 @@ void DesignSketchTool::cancel() void DesignSketchTool::request_exit() { if (!m_points.empty()) { m_points.clear(); m_has_cursor = false; return; } - if (m_mode != Mode::Select) { set_tool(Mode::Select); return; } + // Drop any pending edit-op BEFORE the downgrade: set_tool commits a ready one, and Esc must + // cancel it, never apply it. Right-click already discards it through its own branch. + if (m_mode != Mode::Select) { reset_op(); set_tool(Mode::Select); return; } if (on_exit) on_exit(); else cancel(); } @@ -2096,7 +2113,9 @@ bool DesignSketchTool::try_add_constraints(const std::vector ents = { + line(Vec2d(-89.32, 72.05), Vec2d( 52.00, 72.05)), // 0 top (H) + line(Vec2d( 52.00, 72.05), Vec2d( 52.00, -68.97)), // 1 right (V) + line(Vec2d( 52.00, -68.97), Vec2d(-89.32, -68.97)), // 2 bottom (H) + line(Vec2d(-89.32, -68.97), Vec2d(-89.32, 72.05)), // 3 left (V) + }; + std::vector cs = { + coinc(0, R::P1, 1, R::P0), coinc(0, R::P0, 3, R::P1), + coinc(1, R::P1, 2, R::P0), coinc(2, R::P1, 3, R::P0), + axis(CT::Horizontal, 0), axis(CT::Vertical, 1), + axis(CT::Horizontal, 2), axis(CT::Vertical, 3), + }; + REQUIRE(solve_sketch_entities(ents, cs)); + + // Fillet the top-left corner: trim both legs, drop the stale corner Coincident. + const int a = 0, b = 3; + SketchEntity a_out, b_out, arc; + REQUIRE(SketchEngine::fillet_lines(ents[a], ents[b], 28.205, a_out, b_out, arc)); + ents[a] = a_out; ents[b] = b_out; + const int xi = int(ents.size()); + ents.push_back(arc); + cs.erase(std::remove_if(cs.begin(), cs.end(), [&](const SketchEntityConstraintDef& d) { + return d.type == CT::Coincident && d.ea == a && d.ra == R::P0 && d.eb == b && d.rb == R::P1; + }), cs.end()); + const std::vector trimmed = ents; + + auto coin = [&](R xr, int ln, R lr) { return coinc(xi, xr, ln, lr); }; + auto tang = [&](int ln) { + SketchEntityConstraintDef d; d.type = CT::Tangent; d.ea = xi; d.eb = ln; return d; }; + + // Rung 1 of the ladder: a tangent on each leg. Over-constrained against the legs' + // own H/V, so it must be rejected -- and must not move a single point. + { + std::vector pc = cs; + for (const auto& c : { coin(R::P0, a, R::P0), coin(R::P1, b, R::P1), tang(a), tang(b) }) + pc.push_back(c); + std::vector e = ents; + REQUIRE_FALSE(solve_sketch_entities(e, pc)); + for (size_t i = 0; i < e.size(); ++i) { + CHECK((e[i].p0 - trimmed[i].p0).norm() == Approx(0.0).margin(1e-9)); + CHECK((e[i].p1 - trimmed[i].p1).norm() == Approx(0.0).margin(1e-9)); + CHECK((e[i].center - trimmed[i].center).norm() == Approx(0.0).margin(1e-9)); + } + } + + // Rung 2 (one tangent) solves, and the arc keeps the radius the fillet gave it. + { + std::vector pc = cs; + for (const auto& c : { coin(R::P0, a, R::P0), coin(R::P1, b, R::P1), tang(a) }) + pc.push_back(c); + REQUIRE(solve_sketch_entities(ents, pc)); + CHECK(ents[xi].radius == Approx(28.205).margin(1e-6)); + // Corner stays open: the legs end on the arc, they do not meet each other. + CHECK((ents[a].p0 - ents[b].p1).norm() > 1.0); + CHECK((ents[a].p0 - ents[xi].p0).norm() == Approx(0.0).margin(1e-6)); + CHECK((ents[b].p1 - ents[xi].p1).norm() == Approx(0.0).margin(1e-6)); + } +}