From faf4406f89238e3c024db02b06e7ca2de8284764 Mon Sep 17 00:00:00 2001 From: Tommaso Bianchi Date: Wed, 2 Sep 2026 13:15:51 +0200 Subject: [PATCH] A wire that lost two edges still called itself done MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An extrude built a solid the user never drew: three sides of the handle plus the arc that bulges outside the outline, with the bowl's second arc and the left edge missing. Two faults met. entities_to_wires added edges in ENTITY-CREATION order, so a partial wire rejects the next edge even when the sketch closes perfectly; and one joint of the reported sketch is open by 2.28e-5 mm, wider than OCCT's 1e-7 vertex tolerance and wider than this function's own EPS of 1e-6, so that edge was refused on geometry too. Neither showed up, because BRepLib_MakeWire::Add DROPS a disconnected edge (BRepLib_DisconnectedWire + NotDone) while every successful Add ends with BRepLib_WireDone + Done() — overwriting the failure. `if (!wm.IsDone()) return {}` was therefore asking only whether the LAST edge connected. Six edges in, four out, IsDone() true. Endpoints now weld into shared nodes at one tolerance (kSketchWeldTol) used by BOTH the union-find grouping and the wire build — they disagreed before, which is how a joint gets united into a loop and then refused by the builder. Each node becomes ONE TopoDS_Vertex, so the builder matches on identity instead of proximity, with the vertex tolerance widened because BRepLib_MakeEdge::Init projects a vertex onto the curve within that tolerance and a welded node sits up to the weld gap off its neighbour's curve. Members are then walked in traversal order. Finally the result is counted: IsDone() alone is not evidence, edge_count == members.size() is. Arc geometry is untouched — the midpoint from (start_angle+end_angle)/2 and the solver's angle reflow both measured correct and were never part of this. The regression case carries the reported sketch verbatim, open joint included. It fails 4 == 6 without the fix, which was measured, not assumed. Kernel 66092 assertions / 604 cases green. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_011FbJKJAJxxkhDTs9XdZzKA --- src/libslic3r/CAD/SketchEngine.cpp | 99 +++++++++++++++++++++++++---- tests/libslic3r/test_sketchedit.cpp | 56 ++++++++++++++++ 2 files changed, 143 insertions(+), 12 deletions(-) diff --git a/src/libslic3r/CAD/SketchEngine.cpp b/src/libslic3r/CAD/SketchEngine.cpp index 199139d632..835bb7a5fc 100644 --- a/src/libslic3r/CAD/SketchEngine.cpp +++ b/src/libslic3r/CAD/SketchEngine.cpp @@ -5,6 +5,7 @@ #include #include +#include #include #include #include @@ -599,8 +600,12 @@ std::vector SketchEngine::entities_to_wires(const std::vector parent(valid.size()); @@ -679,19 +684,81 @@ std::vector SketchEngine::entities_to_wires(const std::vector node_pt; // welded sketch point per node + 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); + node_pt.push_back(p); + node_deg.push_back(0); + return int(node_pt.size()) - 1; + }; + std::vector ms; + ms.reserve(loop.members.size()); for (size_t m : loop.members) { - const SketchEntity* e = valid[m].e; + Vec2d p0, p1; + if (!endpoints(*valid[m].e, p0, p1)) return {}; + int a = node_id(p0), b = node_id(p1); + node_deg[a]++; node_deg[b]++; + ms.push_back({m, a, b}); + } + + // Traverse: begin at a degree-1 node for an open chain, else at any member, then + // repeatedly take the unused member sharing the current open node. + std::vector order; + order.reserve(ms.size()); + std::vector used(ms.size(), 0); + size_t start = 0; + for (size_t i = 0; i < ms.size(); ++i) + if (node_deg[ms[i].a] == 1 || node_deg[ms[i].b] == 1) { start = i; break; } + int open = (node_deg[ms[start].a] == 1) ? ms[start].a + : (node_deg[ms[start].b] == 1) ? ms[start].b + : ms[start].a; + for (;;) { + size_t next = ms.size(); + for (size_t k = 0; k < ms.size(); ++k) { + if (used[k]) continue; + if (ms[k].a == open || ms[k].b == open) { next = k; break; } + } + if (next == ms.size()) break; + order.push_back(next); + used[next] = 1; + open = (ms[next].a == open) ? ms[next].b : ms[next].a; + } + if (order.size() != ms.size()) return {}; + + // One TopoDS_Vertex per node at the welded world point. Tolerance widened to + // kSketchWeldTol because MakeEdge(curve, va, vb) projects each vertex onto the + // curve within the vertex tolerance (BRepLib_MakeEdge::Init), and a welded node + // can be up to the weld gap off another entity's curve. + 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); + } + + for (size_t o : order) { + const Member& mm = ms[o]; + const SketchEntity* e = valid[mm.v].e; + const TopoDS_Vertex& va = verts[mm.a]; + const TopoDS_Vertex& vb = verts[mm.b]; if (e->type == SketchEntity::Type::Line) { - Vec3d p0 = plane.to_world(e->p0); - Vec3d p1 = plane.to_world(e->p1); - gp_Pnt pa(p0.x(), p0.y(), p0.z()); - gp_Pnt pb(p1.x(), p1.y(), p1.z()); - wm.Add(BRepBuilderAPI_MakeEdge(pa, pb).Edge()); + wm.Add(BRepBuilderAPI_MakeEdge(va, vb).Edge()); } else if (e->type == SketchEntity::Type::EllipseArc) { if (e->radius <= 1e-9 || e->rminor <= 1e-9) return {}; GC_MakeArcOfEllipse arc_maker(make_elips(*e), e->start_angle, e->end_angle, Standard_True); if (!arc_maker.IsDone()) return {}; - wm.Add(BRepBuilderAPI_MakeEdge(arc_maker.Value()).Edge()); + wm.Add(BRepBuilderAPI_MakeEdge(arc_maker.Value(), va, vb).Edge()); } else if (e->type == SketchEntity::Type::Arc) { Vec3d p0 = plane.to_world(e->p0); Vec3d p1 = plane.to_world(e->p1); @@ -705,17 +772,25 @@ std::vector SketchEngine::entities_to_wires(const std::vectortype == SketchEntity::Type::BSpline) { Handle(Geom_BSplineCurve) crv = make_bspline(*e); if (crv.IsNull()) return {}; - wm.Add(BRepBuilderAPI_MakeEdge(crv).Edge()); + wm.Add(BRepBuilderAPI_MakeEdge(crv, va, vb).Edge()); } } } wm.Build(); + // IsDone() reflects only the LAST Add: BRepLib_MakeWire::Add sets BRepLib_DisconnectedWire + // + NotDone() and returns on a disconnected edge (dropping it), but every successful Add + // finishes with BRepLib_WireDone + Done(), overwriting that failure (BRepLib_MakeWire.cxx). + // So dropped edges go unnoticed — count the edges actually in the wire instead. if (!wm.IsDone()) return {}; - out.push_back(wm.Wire()); + const TopoDS_Wire wire = wm.Wire(); + size_t edge_count = 0; + for (TopExp_Explorer ex(wire, TopAbs_EDGE); ex.More(); ex.Next()) ++edge_count; + if (edge_count != (loop.closed_single ? 1 : loop.members.size())) return {}; + out.push_back(wire); } return out; } diff --git a/tests/libslic3r/test_sketchedit.cpp b/tests/libslic3r/test_sketchedit.cpp index 5af9705d03..022aaa12e0 100644 --- a/tests/libslic3r/test_sketchedit.cpp +++ b/tests/libslic3r/test_sketchedit.cpp @@ -1,6 +1,8 @@ #include // mainline OrcaSlicer ships Catch2 v3 (v2 was catch2/catch.hpp) #include "libslic3r/CAD/SketchEngine.hpp" #include +#include +#include using namespace Slic3r; @@ -499,3 +501,57 @@ TEST_CASE("Trim arc with no crossing returns false", "[SketchEdit]") REQUIRE_FALSE(SketchEngine::trim_entity(e, {cut}, Vec2d(5 * std::cos(M_PI/4), 5 * std::sin(M_PI/4)))); } + +// Regression guard: BEFORE the weld fix this test failed with 4 edges instead of 6. +// BRepLib_MakeWire::Add silently DROPS a disconnected edge (BRepLib_DisconnectedWire + NotDone) +// yet every successful Add ends with BRepLib_WireDone + Done(), so IsDone() reported only whether +// the LAST edge connected. This sketch is a real user loop (2 arcs + 4 lines) given in +// creation order, which is NOT traversal order, and its joint between the 3rd and 4th entity +// below is open by 2.28e-5 mm — larger than OCCT's default vertex tolerance. +TEST_CASE("entities_to_wires keeps every edge of a loop drawn out of order", "[SketchEngine]") +{ + std::vector ents(6); + + ents[0].type = SketchEntity::Type::Line; + ents[0].p0 = Vec2d(-0.537697713190522, -0.0009077462579133498); + ents[0].p1 = Vec2d(99.46230228680926, -0.0009141694814321626); + + ents[1].type = SketchEntity::Type::Arc; + ents[1].p0 = Vec2d(-0.537697713190522, -0.0009077462579133498); + ents[1].p1 = Vec2d(-100.14602636660666, -0.27673132181233495); + ents[1].center = Vec2d(-50.313868583115394, -10.248112903179617); + ents[1].radius = 50.81999999999999; + ents[1].start_angle = 0.20302922018398933; + ents[1].end_angle = 2.944101582158999; + + ents[2].type = SketchEntity::Type::Line; + ents[2].p0 = Vec2d(99.46228668626469, -39.22091416947833); + ents[2].p1 = Vec2d(-0.537864366432629, -39.22420589376945); + + ents[3].type = SketchEntity::Type::Arc; + ents[3].p0 = Vec2d(-100.14602636694521, -38.94673132181234); + ents[3].p1 = Vec2d(-0.5378420354900413, -39.22421049164698); + ents[3].center = Vec2d(-50.31377078666179, -28.975499083790503); + ents[3].radius = 50.82006659345552; + ents[3].start_angle = -2.944104841286737; + ents[3].end_angle = -0.20305921095748136; + + ents[4].type = SketchEntity::Type::Line; + ents[4].p0 = Vec2d(99.46228668626469, -39.22091416947833); + ents[4].p1 = Vec2d(99.46230228680926, -0.0009141694814321626); + + ents[5].type = SketchEntity::Type::Line; + ents[5].p0 = Vec2d(-100.14602636694521, -38.94673132181234); + ents[5].p1 = Vec2d(-100.14602636660666, -0.27673132181233495); + + 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 == 6); + + REQUIRE(wires[0].Closed()); +}