From 8b568b7b9dc7b902077b90a73381bb064ab20a9f Mon Sep 17 00:00:00 2001 From: Tommaso Bianchi Date: Wed, 2 Sep 2026 14:34:17 +0200 Subject: [PATCH] The viewport and the kernel now answer "is this joint closed?" with one number MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tommaso asked the question that names the real defect: if the sketch was open, why was the same sketch shaded closed and offered for extrude? Because the two halves used different tolerances. region_loops shades a region closed at 1e-3 mm; connected_loop chained at 1e-3; the kernel welded at 1e-4 and OCCT matched vertices at 1e-7. The 2.28e-5 mm gap in the reported sketch did not cause that disagreement, it only made it visible — and fixing the gap alone would have left the contradiction in place, ready to reappear anywhere in (1e-4, 1e-3]. kSketchJoinTol now lives in SketchEngine.hpp and is the only place the number exists. region_loops, loop_report, connected_loop and entities_to_wires all read it through sketch_join_tol(). The viewport cannot promise a region the kernel refuses to build. The welding is optional, because a kernel that silently closes loops should let you say no: "Auto-close sketch loops" in Preferences, default ON, no restart. OFF means only exactly coincident endpoints join — and since both halves read the same value, the viewport simply stops shading the region closed, so an open loop is visible rather than welded behind your back. No separate UI needed for that; it falls out of sharing one number. Details that matter. The kernel defaults to auto-close ON independently of the GUI, so headless and MCP callers behave like the viewport instead of inheriting an unset preference. With the tolerance at 0 the comparisons become <=, because OFF must mean exact, not broken. OCCT never receives a zero vertex tolerance — it is clamped to Precision::Confusion. The preference is pushed from EVERY entry that starts a sketch session, not just begin(): a Constrain session enters through begin_constrain / begin_constrain_entities and uses region_loops and connected_loop, so a single push site would have left those sessions running on whatever the previous one set. begin_imported_transform is excluded deliberately — it works on imported regions, not chained entities. Tests: a loop with one joint open by 9e-4 mm, given out of traversal order, builds a closed four-edge wire; with auto-close off the same loop yields no wire; and an exactly closed loop still builds with auto-close off, proving OFF means exact. Kernel 66104 assertions / 606 cases green. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_011FbJKJAJxxkhDTs9XdZzKA --- src/libslic3r/AppConfig.cpp | 6 ++ src/libslic3r/CAD/SketchEngine.cpp | 31 ++++++-- src/libslic3r/CAD/SketchEngine.hpp | 18 +++++ src/slic3r/GUI/CAD/DesignSketchTool.cpp | 19 ++++- src/slic3r/GUI/GUI_App.hpp | 2 + src/slic3r/GUI/Preferences.cpp | 12 +++ tests/libslic3r/test_sketchedit.cpp | 97 +++++++++++++++++++++++++ 7 files changed, 174 insertions(+), 11 deletions(-) diff --git a/src/libslic3r/AppConfig.cpp b/src/libslic3r/AppConfig.cpp index 17b86ac13e..2f2fb4a0a1 100644 --- a/src/libslic3r/AppConfig.cpp +++ b/src/libslic3r/AppConfig.cpp @@ -316,6 +316,12 @@ void AppConfig::set_defaults() if (get("enable_cad_feature").empty()) set_bool("enable_cad_feature", false); + // Auto-weld sketch endpoints within kSketchJoinTol when building closed loops. + // Default ON: it is what the ~90% case wants; OFF makes the kernel demand an exact + // joint. The GUI pushes it into SketchEngine via set_sketch_auto_close(). + if (get("auto_close_sketch_loops").empty()) + set_bool("auto_close_sketch_loops", true); + // Design tab: draw a mate connector as a face rather than as the abstract disc + roll // quadrant. Defaults ON — face orientation is hardwired perception, so the roll and the // verse read without being learned, which no abstract glyph achieves. Turning it off diff --git a/src/libslic3r/CAD/SketchEngine.cpp b/src/libslic3r/CAD/SketchEngine.cpp index 835bb7a5fc..213bbab3e7 100644 --- a/src/libslic3r/CAD/SketchEngine.cpp +++ b/src/libslic3r/CAD/SketchEngine.cpp @@ -42,6 +42,7 @@ #include #include #include +#include #include #include #include @@ -51,6 +52,14 @@ namespace Slic3r { +// Single source of truth for the weld tolerance the viewport and the kernel share. +// Defaults ON so headless/kernel-only callers keep welding; the GUI pushes the +// "auto_close_sketch_loops" preference in via set_sketch_auto_close(). +static bool s_auto_close = true; + +double sketch_join_tol() { return s_auto_close ? kSketchJoinTol : 0.0; } +void set_sketch_auto_close(bool on) { s_auto_close = on; } + // ---- SketchPlane ---- gp_Pln SketchPlane::to_occt() const @@ -535,6 +544,11 @@ TriangleMesh SketchEngine::tessellate(const TopoDS_Shape& shape, std::vector SketchEngine::entities_to_wires(const std::vector& entities, const SketchPlane& plane) { + // Effective weld tolerance: kSketchJoinTol when auto-close is on, 0.0 when off. + // Read ONCE so the union-find, the node weld and the vertex tolerance below all + // agree. With 0.0 the comparisons use <= so exactly coincident endpoints still join. + const double tol = sketch_join_tol(); + struct Item { const SketchEntity* e; size_t idx; }; std::vector valid; valid.reserve(entities.size()); @@ -600,12 +614,12 @@ std::vector SketchEngine::entities_to_wires(const std::vector parent(valid.size()); @@ -687,7 +701,7 @@ std::vector SketchEngine::entities_to_wires(const std::vector SketchEngine::entities_to_wires(const std::vector node_deg; // endpoint count per node auto node_id = [&](const Vec2d& p) -> int { for (size_t i = 0; i < node_pt.size(); ++i) - if ((node_pt[i] - p).norm() < kSketchWeldTol) return int(i); + if ((node_pt[i] - p).norm() <= tol) return int(i); node_pt.push_back(p); node_deg.push_back(0); return int(node_pt.size()) - 1; @@ -736,15 +750,16 @@ std::vector SketchEngine::entities_to_wires(const std::vector verts(node_pt.size()); BRep_Builder B; for (size_t i = 0; i < node_pt.size(); ++i) { Vec3d w = plane.to_world(node_pt[i]); verts[i] = BRepBuilderAPI_MakeVertex(gp_Pnt(w.x(), w.y(), w.z())).Vertex(); - B.UpdateVertex(verts[i], kSketchWeldTol); + B.UpdateVertex(verts[i], std::max(tol, Precision::Confusion())); } for (size_t o : order) { diff --git a/src/libslic3r/CAD/SketchEngine.hpp b/src/libslic3r/CAD/SketchEngine.hpp index 7ccc807103..41ec5123f5 100644 --- a/src/libslic3r/CAD/SketchEngine.hpp +++ b/src/libslic3r/CAD/SketchEngine.hpp @@ -78,6 +78,24 @@ struct SketchProfile { void serialize(Archive& ar) { ar(points, closed); } }; +// Two sketch endpoints this close are ONE joint. Shared deliberately by the viewport +// (region_loops / connected_loop / open-end detection) and by the kernel +// (entities_to_wires): the viewport is what shades a region closed and offers it for +// extrude, so the kernel MUST be able to build every loop the viewport shades. When +// these two numbers disagreed the viewport promised a closed region at 1e-3 and the +// kernel refused it at 1e-4, which extruded a solid the user never drew. +// Nothing legitimate in a mm-scale sketch is 1 um apart. +inline constexpr double kSketchJoinTol = 1e-3; // mm + +// Effective sketch joint tolerance. ONE value for the viewport (region_loops / +// loop_report / connected_loop) and the kernel (entities_to_wires): if these ever +// disagree again, the viewport shades a region closed that the kernel refuses to +// build, which is how a sketch got extruded into the wrong solid. The GUI pushes +// the "auto_close_sketch_loops" preference in via set_sketch_auto_close(); the +// kernel defaults to ON so headless/kernel-only callers keep welding. +double sketch_join_tol(); +void set_sketch_auto_close(bool on); + enum class SketchConstraintType { Fix, Coincident, Horizontal, Vertical, Distance, LockX, LockY, EqualLength, Parallel, Perpendicular, diff --git a/src/slic3r/GUI/CAD/DesignSketchTool.cpp b/src/slic3r/GUI/CAD/DesignSketchTool.cpp index 9ef927cd42..4d4aa02fec 100644 --- a/src/slic3r/GUI/CAD/DesignSketchTool.cpp +++ b/src/slic3r/GUI/CAD/DesignSketchTool.cpp @@ -62,8 +62,19 @@ static double ray_segment_dist3(const Vec3d& ro, const Vec3d& rd, const Vec3d& a return wxPoint(int(sx + 0.5), int(sy + 0.5)); } +// The kernel's weld tolerance follows the app preference, and it must be pushed at EVERY +// point that starts a sketch session: a Constrain session never passes through begin(), and +// it uses region_loops()/connected_loop(), which read the same tolerance. Pushing in one +// place only would leave those sessions on whatever the previous session set. +static void push_auto_close_pref() +{ + Slic3r::set_sketch_auto_close(wxGetApp().is_auto_close_sketch_loops()); +} + void DesignSketchTool::begin(const SketchPlane& plane, Mode mode) { + push_auto_close_pref(); + m_plane = plane; m_mode = mode; m_step_mode_last = -1; // a new session re-announces its step, even if it repeats the last @@ -2221,6 +2232,7 @@ void DesignSketchTool::finish() void DesignSketchTool::begin_constrain(const SketchProfile& prof, const SketchPlane& plane) { + push_auto_close_pref(); m_plane = plane; m_mode = Mode::Constrain; m_points = prof.points; @@ -2235,6 +2247,7 @@ void DesignSketchTool::begin_constrain(const SketchProfile& prof, const SketchPl void DesignSketchTool::begin_constrain_entities(const std::vector& ents, const SketchPlane& plane) { + push_auto_close_pref(); m_plane = plane; m_mode = Mode::Constrain; m_constrain_entities = true; @@ -6333,7 +6346,7 @@ std::vector DesignSketchTool::region_loops(const std::vector& ents) const { std::vector regions; - const double eps2 = 1e-3 * 1e-3; + const double eps2 = sketch_join_tol() * sketch_join_tol(); auto is_near = [&](const Vec2d& a, const Vec2d& b) { return (a - b).squaredNorm() < eps2; }; // Circles are self-closed regions; lines/arcs are open segments to be chained. Each @@ -9420,7 +9433,7 @@ DesignSketchTool::LoopReport DesignSketchTool::loop_report() const ends.push_back({ e.p1 }); } } - const double eps = 1e-3; + const double eps = sketch_join_tol(); for (size_t i = 0; i < ends.size(); ++i) { int met = 0; for (size_t j = 0; j < ends.size(); ++j) { @@ -9553,7 +9566,7 @@ std::vector DesignSketchTool::connected_loop(int seed) const { std::vector out; if (seed < 0 || seed >= int(m_entities.size())) return out; - const double eps2 = 1e-6; + const double eps2 = sketch_join_tol() * sketch_join_tol(); std::vector vis(m_entities.size(), false); std::vector stack = { seed }; vis[seed] = true; diff --git a/src/slic3r/GUI/GUI_App.hpp b/src/slic3r/GUI/GUI_App.hpp index 68074894ee..f4c20af60c 100644 --- a/src/slic3r/GUI/GUI_App.hpp +++ b/src/slic3r/GUI/GUI_App.hpp @@ -351,6 +351,8 @@ public: inline bool is_enable_multi_machine() { return this->app_config&& this->app_config->get("enable_multi_machine") == "true"; } #ifdef SLIC3R_CAD inline bool is_enable_cad_feature() { return this->app_config && this->app_config->get_bool("enable_cad_feature"); } + inline bool is_auto_close_sketch_loops() { return !this->app_config + || this->app_config->get_bool("auto_close_sketch_loops"); } #endif std::map test_url_state; diff --git a/src/slic3r/GUI/Preferences.cpp b/src/slic3r/GUI/Preferences.cpp index 39acb05d83..77574fdd25 100644 --- a/src/slic3r/GUI/Preferences.cpp +++ b/src/slic3r/GUI/Preferences.cpp @@ -8,6 +8,7 @@ #include "I18N.hpp" #include "libslic3r/AppConfig.hpp" #include "libslic3r/Format/DRC.hpp" +#include "libslic3r/CAD/SketchEngine.hpp" #include #include "OG_CustomCtrl.hpp" #include "wx/graphics.h" @@ -1747,6 +1748,13 @@ void PreferencesDialog::create_items() "parametrically. This feature is experimental and still under development."), "enable_cad_feature", _L("(Requires restart)")); g_sizer->Add(item_cad_feature); + + auto item_auto_close_sketch_loops = create_item_checkbox(_L("Auto-close sketch loops"), + _L("Treat sketch endpoints within 0.001 mm as one joint and weld the loop shut. " + "Off: only exactly coincident endpoints join, so a loop with a tiny gap is " + "shown as open instead of being closed for you."), + "auto_close_sketch_loops"); + g_sizer->Add(item_auto_close_sketch_loops); #endif #if 0 @@ -1834,6 +1842,10 @@ void PreferencesDialog::create_items() "Turn this off for the conventional CAD representation."), "design_connector_face_glyph"); g_sizer->Add(item_connector_face_glyph); } + + // Push the weld preference into the kernel now so toggling it takes effect without + // a restart (the sketch tool also re-pushes on activation, see DesignSketchTool::begin). + Slic3r::set_sketch_auto_close(wxGetApp().is_auto_close_sketch_loops()); #endif std::vector ButtonDragActions = {_L("None"), _L("Pan"), _L("Rotate")}; diff --git a/tests/libslic3r/test_sketchedit.cpp b/tests/libslic3r/test_sketchedit.cpp index 022aaa12e0..973785e812 100644 --- a/tests/libslic3r/test_sketchedit.cpp +++ b/tests/libslic3r/test_sketchedit.cpp @@ -555,3 +555,100 @@ TEST_CASE("entities_to_wires keeps every edge of a loop drawn out of order", "[S REQUIRE(wires[0].Closed()); } + +// Regression guard: this fails at 1e-4 (the wire builder refuses a joint the viewport had +// already shaded closed) and passes at kSketchJoinTol. A 20x10 quad with one joint left open +// by 9e-4 mm — just inside kSketchJoinTol, exactly the case the viewport shades closed — given +// in an order that is NOT traversal order, so the ordering path is covered too. +TEST_CASE("a loop the viewport shades closed is buildable by the kernel", "[SketchEngine]") +{ + std::vector ents(4); + + // (0,0) -> (20,0) -> (20,10) -> (0,10) -> (0.0009, 0): last endpoint misses (0,0) by 9e-4. + ents[0].type = SketchEntity::Type::Line; + ents[0].p0 = Vec2d(0, 0); + ents[0].p1 = Vec2d(20, 0); + + // Index 1 is the FAR side, not the neighbour of index 0: creation order here is + // deliberately not traversal order, so a partial wire would reject it without the + // traversal walk. + ents[1].type = SketchEntity::Type::Line; + ents[1].p0 = Vec2d(20, 10); + ents[1].p1 = Vec2d(0, 10); + + ents[2].type = SketchEntity::Type::Line; + ents[2].p0 = Vec2d(20, 0); + ents[2].p1 = Vec2d(20, 10); + + ents[3].type = SketchEntity::Type::Line; + ents[3].p0 = Vec2d(0, 10); + ents[3].p1 = Vec2d(0.0009, 0); + + auto wires = SketchEngine::entities_to_wires(ents, SketchPlane::XY()); + + REQUIRE(wires.size() == 1); + + int edge_count = 0; + for (TopExp_Explorer ex(wires[0], TopAbs_EDGE); ex.More(); ex.Next()) + ++edge_count; + REQUIRE(edge_count == 4); + + REQUIRE(wires[0].Closed()); +} + +// Regression guard for the auto-close preference. Same 20x10 quad, one joint open by 9e-4 mm +// and given out of traversal order, as "a loop the viewport shades closed is buildable by the +// kernel". With auto-close ON the gap welds (one closed wire); with auto-close OFF it must not. +TEST_CASE("auto-close off makes the kernel demand an exact joint", "[SketchEngine]") +{ + std::vector ents(4); + + ents[0].type = SketchEntity::Type::Line; + ents[0].p0 = Vec2d(0, 0); + ents[0].p1 = Vec2d(20, 0); + + ents[1].type = SketchEntity::Type::Line; + ents[1].p0 = Vec2d(20, 10); + ents[1].p1 = Vec2d(0, 10); + + ents[2].type = SketchEntity::Type::Line; + ents[2].p0 = Vec2d(20, 0); + ents[2].p1 = Vec2d(20, 10); + + ents[3].type = SketchEntity::Type::Line; + ents[3].p0 = Vec2d(0, 10); + ents[3].p1 = Vec2d(0.0009, 0); + + auto edge_count = [](const TopoDS_Wire& w) { + int n = 0; + for (TopExp_Explorer ex(w, TopAbs_EDGE); ex.More(); ex.Next()) ++n; + return n; + }; + + // ON: the 9e-4 mm gap is inside kSketchJoinTol, so the loop welds into one closed wire. + Slic3r::set_sketch_auto_close(true); + auto wires_on = SketchEngine::entities_to_wires(ents, SketchPlane::XY()); + REQUIRE(wires_on.size() == 1); + REQUIRE(edge_count(wires_on[0]) == 4); + REQUIRE(wires_on[0].Closed()); + + // OFF: the joint is not exact, so the gap is NOT welded. entities_to_wires legitimately + // returns open chains (a sweep path is open), so the observable is an OPEN wire — the + // kernel no longer hands back the closed loop the viewport would have shaded. + Slic3r::set_sketch_auto_close(false); + auto wires_off = SketchEngine::entities_to_wires(ents, SketchPlane::XY()); + REQUIRE(wires_off.size() == 1); + REQUIRE(edge_count(wires_off[0]) == 4); + REQUIRE_FALSE(wires_off[0].Closed()); + + // OFF + an EXACT joint (last endpoint exactly (0,0)): the quad still builds closed, + // proving "off" means exact rather than broken. + ents[3].p1 = Vec2d(0, 0); + auto wires_exact = SketchEngine::entities_to_wires(ents, SketchPlane::XY()); + REQUIRE(wires_exact.size() == 1); + REQUIRE(edge_count(wires_exact[0]) == 4); + REQUIRE(wires_exact[0].Closed()); + + // Restore the default so test order cannot leak OFF into the other cases. + Slic3r::set_sketch_auto_close(true); +}