From bb6a1810f6e20bf58a17e1c61d9eb8d0383068e7 Mon Sep 17 00:00:00 2001 From: Tommaso Bianchi Date: Wed, 2 Sep 2026 14:58:05 +0200 Subject: [PATCH] A stray click must not break a model that looks perfect on screen MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Revolve failed on a sketch whose profile was closed. Decoding the reported 3mf: four entities forming a proper closed loop (joints open by 4.44e-06 mm, well inside tolerance) plus one stray 1.82 mm Line at (-24.2, 80.3), inside the shaded region, touching nothing. The viewport's region_loops discards open chains ON PURPOSE — it exists to find EXTRUDABLE regions — so the user saw one clean closed region. entities_to_wires kept the stray as its own one-edge loop, so it returned two wires, and Revolve goes through entities_to_wire which demands exactly one. Extrude would have failed one step later in wires_to_face, because a one-edge open wire bounds no face. Same class as the tolerance split fixed in 8b568b7b: the viewport and the kernel disagreeing about the sketch — this time about what BELONGS to the profile. entities_to_wires/entities_to_wire/build_sketch_wire take closed_only. It is not a blanket rule: a SurfaceExtrude builds a sheet FROM an open profile and a Sweep PATH is normally open, so all ten call sites are classified individually — true for the face fallback, Extrude-taper, Revolve, the Sweep PROFILE and Loft profiles; false for SurfaceExtrude/Revolve/Loft/Fill and the Sweep path. A component counts as open when some welded node has DEGREE 1. The first attempt used "the traversal did not return to its starting node", which regressed the bridged C profile: a closed loop that also carries a second edge across the same two nodes has no free endpoint, but its Eulerian walk ends elsewhere. Degree-1 is the property that actually distinguishes a stray segment from a closed profile; the suite caught the difference. Behaviour change decided by Tommaso: a stray is IGNORED, not refused. The test that required refusal dates from when ignoring meant falling through to a default rectangle — geometry nobody drew. That fallback is gone, so ignoring now builds the circle the user actually drew. Its assertion is updated with the reason. The bridge round-trip test extruded an ENTIRELY open chain and "worked" only because OCCT will make a face from an open wire. It gets a genuinely closed profile: the test is about serialization, and deserialize_recipe recomputes, so the document has to be one that legitimately builds. Failures now say WHERE. sketch_open_ends reports free endpoints under the same weld tolerance the wire build uses, and open_loop_message is shared by both throws, because Extrude fails through build_sketch_face and Revolve through build_sketch_wire — enriching only one would have left the commoner path the less informative one. Kernel 66115 assertions / 608 cases green. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_011FbJKJAJxxkhDTs9XdZzKA --- src/libslic3r/CAD/CadDocument.cpp | 52 +++++++++++++---- src/libslic3r/CAD/CadDocument.hpp | 2 +- src/libslic3r/CAD/SketchEngine.cpp | 84 ++++++++++++++++++++++++++-- src/libslic3r/CAD/SketchEngine.hpp | 17 ++++-- tests/libslic3r/test_caddocument.cpp | 32 ++++++++--- tests/libslic3r/test_sketchedit.cpp | 76 +++++++++++++++++++++++++ 6 files changed, 231 insertions(+), 32 deletions(-) diff --git a/src/libslic3r/CAD/CadDocument.cpp b/src/libslic3r/CAD/CadDocument.cpp index 7030c6082a..fad2c6b827 100644 --- a/src/libslic3r/CAD/CadDocument.cpp +++ b/src/libslic3r/CAD/CadDocument.cpp @@ -68,6 +68,7 @@ #include #include #include +#include #include #include #include @@ -2109,10 +2110,33 @@ bool CadDocument::replace_sketch_extrude(int sketch_idx, int extrude_idx, return commit_or_rollback(*this, snapshot); } -TopoDS_Wire CadDocument::build_sketch_wire(const CadFeature& sketch) const +// "the sketch is open" is not actionable; WHERE it is open is. Shared by both throws so the +// two ways of reaching an open profile (Revolve via build_sketch_wire, Extrude via +// build_sketch_face) report it identically. +static std::string open_loop_message(const CadFeature& sketch, + const char* head = "sketch entities do not form a closed loop") +{ + std::string msg = head; + const std::vector open = Slic3r::sketch_open_ends(sketch.entities, sketch.plane); + if (!open.empty()) { + const size_t shown = std::min(open.size(), 3); + char buf[96]; + for (size_t i = 0; i < shown; ++i) { + std::snprintf(buf, sizeof buf, "\n Open at (%.3f, %.3f)", open[i].x(), open[i].y()); + msg += buf; + } + if (open.size() > shown) { + std::snprintf(buf, sizeof buf, "\n ...and %zu more open end(s)", open.size() - shown); + msg += buf; + } + } + return msg; +} + +TopoDS_Wire CadDocument::build_sketch_wire(const CadFeature& sketch, bool closed_only) const { if (!sketch.entities.empty()) { - TopoDS_Wire w = SketchEngine::entities_to_wire(sketch.entities, sketch.plane); + TopoDS_Wire w = SketchEngine::entities_to_wire(sketch.entities, sketch.plane, closed_only); if (!w.IsNull()) return w; // An entity sketch that yields no wire is an ERROR, not a cue to fall through. The // legacy tail of this function ends in a default rectangle built from width/height, @@ -2123,9 +2147,10 @@ TopoDS_Wire CadDocument::build_sketch_wire(const CadFeature& sketch) const // looked deliberate. The legacy profile/shape paths below are still reached by sketches // that legitimately carry no entities at all. throw std::runtime_error( - "sketch has entities but they do not form a single closed wire — a closed entity " - "(circle/ellipse) combined with other entities, or several closed entities, is not " - "supported yet"); + open_loop_message(sketch, + "sketch has entities but they do not form a single closed wire — a closed " + "entity (circle/ellipse) combined with other entities, or several closed " + "entities, is not supported yet")); } if (!sketch.profile.points.empty()) { SketchProfile prof = sketch.profile; @@ -2157,12 +2182,15 @@ TopoDS_Wire CadDocument::build_sketch_wire(const CadFeature& sketch) const TopoDS_Face CadDocument::build_sketch_face(const CadFeature& sketch) const { if (!sketch.entities.empty()) { - const std::vector loops = SketchEngine::entities_to_wires(sketch.entities, sketch.plane); + const std::vector loops = SketchEngine::entities_to_wires(sketch.entities, sketch.plane, true); if (loops.empty()) - throw std::runtime_error("sketch entities do not form a closed loop"); + // Same courtesy as build_sketch_wire: name WHERE the sketch is open. Extrude + // reaches its failure through here, not through build_sketch_wire, so without + // this the most common way to hit an open profile is also the least informative. + throw std::runtime_error(open_loop_message(sketch)); return SketchEngine::wires_to_face(loops, sketch.plane); } - return BRepBuilderAPI_MakeFace(build_sketch_wire(sketch)).Face(); + return BRepBuilderAPI_MakeFace(build_sketch_wire(sketch, true)).Face(); } void CadDocument::apply_feature(TopoDS_Shape& result, bool& have_body, @@ -2232,7 +2260,7 @@ void CadDocument::apply_feature(TopoDS_Shape& result, bool& have_body, for (TopExp_Explorer ex(profile, TopAbs_WIRE); ex.More(); ex.Next()) ++nloops; if (nloops > 1) throw std::runtime_error("tapered extrude of a sketch with holes is not supported yet"); - return SketchEngine::make_extrude_taper(build_sketch_wire(sk), sk.plane, L, f.taper_deg); + return SketchEngine::make_extrude_taper(build_sketch_wire(sk, true), sk.plane, L, f.taper_deg); }; TopoDS_Shape t; switch (f.extrude_end) { @@ -2294,7 +2322,7 @@ void CadDocument::apply_feature(TopoDS_Shape& result, bool& have_body, && (features[f.sketch_ref].type == CadFeatureType::Sketch || features[f.sketch_ref].type == CadFeatureType::Project)) ? features[f.sketch_ref] : f; - TopoDS_Wire wire = build_sketch_wire(sk); + TopoDS_Wire wire = build_sketch_wire(sk, true); const double ang = f.flip ? -f.revolve_angle : f.revolve_angle; TopoDS_Shape tool = SketchEngine::make_revolve(wire, sk.plane, ang, f.revolve_axis); if (!have_body || f.mode == BooleanMode::New) { @@ -2404,7 +2432,7 @@ void CadDocument::apply_feature(TopoDS_Shape& result, bool& have_body, } else { throw std::runtime_error("sweep path must be a sketch or helix"); } - TopoDS_Wire profile = build_sketch_wire(sk); + TopoDS_Wire profile = build_sketch_wire(sk, true); TopoDS_Shape tool = SketchEngine::make_sweep(profile, path); if (!have_body || f.mode == BooleanMode::New) { result = tool; @@ -2433,7 +2461,7 @@ void CadDocument::apply_feature(TopoDS_Shape& result, bool& have_body, || (features[ref].type != CadFeatureType::Sketch && features[ref].type != CadFeatureType::Project)) continue; - profiles.push_back(build_sketch_wire(features[ref])); + profiles.push_back(build_sketch_wire(features[ref], true)); } if (profiles.size() < 2) throw std::runtime_error("loft needs 2+ valid profile sketches"); diff --git a/src/libslic3r/CAD/CadDocument.hpp b/src/libslic3r/CAD/CadDocument.hpp index b41ccac20b..f62536384a 100644 --- a/src/libslic3r/CAD/CadDocument.hpp +++ b/src/libslic3r/CAD/CadDocument.hpp @@ -722,7 +722,7 @@ public: std::vector& out_body_meshes, std::string& err) const; private: - TopoDS_Wire build_sketch_wire(const CadFeature& sketch) const; + TopoDS_Wire build_sketch_wire(const CadFeature& sketch, bool closed_only = false) const; // The planar region an Extrude sweeps: the sketch's outer loop with its inner loops as // holes. Falls back to a face over build_sketch_wire() for the legacy profile/shape paths, // which have no concept of a second loop. diff --git a/src/libslic3r/CAD/SketchEngine.cpp b/src/libslic3r/CAD/SketchEngine.cpp index 213bbab3e7..5cc36ce648 100644 --- a/src/libslic3r/CAD/SketchEngine.cpp +++ b/src/libslic3r/CAD/SketchEngine.cpp @@ -542,7 +542,8 @@ TriangleMesh SketchEngine::tessellate(const TopoDS_Shape& shape, } std::vector SketchEngine::entities_to_wires(const std::vector& entities, - const SketchPlane& plane) + const SketchPlane& plane, + bool closed_only) { // 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 @@ -733,9 +734,10 @@ std::vector SketchEngine::entities_to_wires(const std::vector SketchEngine::entities_to_wires(const std::vector SketchEngine::entities_to_wires(const std::vector& entities, - const SketchPlane& plane) + const SketchPlane& plane, + bool closed_only) { - const std::vector w = entities_to_wires(entities, plane); + const std::vector w = entities_to_wires(entities, plane, closed_only); return w.size() == 1 ? w[0] : TopoDS_Wire{}; } +std::vector sketch_open_ends(const std::vector& entities, + const SketchPlane& /*plane*/) +{ + const double tol = sketch_join_tol(); + + auto is_chain = [](const SketchEntity& e) { + return e.type == SketchEntity::Type::Line || e.type == SketchEntity::Type::Arc || + e.type == SketchEntity::Type::EllipseArc || e.type == SketchEntity::Type::BSpline; + }; + auto endpoints = [](const SketchEntity& e, Vec2d& a, Vec2d& b) -> bool { + if (e.type == SketchEntity::Type::BSpline) { + if (e.ctrl.size() < 2) return false; + a = e.ctrl.front(); b = e.ctrl.back(); + return true; + } + a = e.p0; b = e.p1; + return true; + }; + + // Weld every chain endpoint into a shared node under the SAME tolerance the wire + // build uses, then report the degree-1 nodes: they are where a chain fails to close. + // Circle/Ellipse are always closed and contribute no endpoint. + std::vector node_pt; + std::vector node_deg; + auto node_id = [&](const Vec2d& p) -> int { + for (size_t i = 0; i < node_pt.size(); ++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; + }; + + for (const SketchEntity& e : entities) { + if (e.construction) continue; + if (!is_chain(e)) continue; + Vec2d a, b; + if (!endpoints(e, a, b)) continue; + int ia = node_id(a), ib = node_id(b); + node_deg[ia]++; + node_deg[ib]++; + } + + std::vector out; + for (size_t i = 0; i < node_pt.size(); ++i) + if (node_deg[i] == 1) out.push_back(node_pt[i]); + return out; +} + TopoDS_Face SketchEngine::wires_to_face(const std::vector& wires, const SketchPlane& plane) { diff --git a/src/libslic3r/CAD/SketchEngine.hpp b/src/libslic3r/CAD/SketchEngine.hpp index 41ec5123f5..f2166e96a6 100644 --- a/src/libslic3r/CAD/SketchEngine.hpp +++ b/src/libslic3r/CAD/SketchEngine.hpp @@ -288,15 +288,18 @@ public: double angular_deflection = 0.5); static TopoDS_Wire entities_to_wire(const std::vector& entities, - const SketchPlane& plane); + const SketchPlane& plane, + bool closed_only = false); - // Every closed loop the sketch holds, in the order each loop's FIRST entity appears in + // Every loop the sketch holds, in the order each loop's FIRST entity appears in // `entities`. A Circle or Ellipse is a loop on its own; Line/Arc/EllipseArc/BSpline // entities are grouped into loops by shared endpoints. An OPEN chain is returned too — - // a sweep path is legitimately open, so open-ness is not an error here. - // Empty vector = nothing usable; the caller decides whether that is an error. + // a sweep path is legitimately open, so open-ness is not an error here — unless + // `closed_only` is true, in which case an open chain is DISCARDED (skipped, not an + // error). Empty vector = nothing usable; the caller decides whether that is an error. static std::vector entities_to_wires(const std::vector& entities, - const SketchPlane& plane); + const SketchPlane& plane, + bool closed_only = false); // A planar face from a set of coplanar loops: the largest-area loop is the outer boundary // and every other loop is a hole in it. Throws std::runtime_error with a message naming the @@ -361,6 +364,10 @@ public: const SketchEntity& b, int b_end); }; +// Free endpoints of a sketch: the sketch-space points where a chain fails to close. +// Same weld tolerance as the wire build, so it can never contradict it. +std::vector sketch_open_ends(const std::vector&, const SketchPlane&); + } // namespace Slic3r #endif // slic3r_SketchEngine_hpp_ diff --git a/tests/libslic3r/test_caddocument.cpp b/tests/libslic3r/test_caddocument.cpp index ce93baccf1..1b8f3949e0 100644 --- a/tests/libslic3r/test_caddocument.cpp +++ b/tests/libslic3r/test_caddocument.cpp @@ -4703,14 +4703,23 @@ TEST_CASE("bridge round-trip serialization", "[CadDocument][bridge]") using Catch::Matchers::WithinAbs; CadDocument doc; + // The two collinear stubs plus the bridge span (0,0)->(30,0); the remaining three lines + // return to the origin so the profile is genuinely CLOSED. It used to be just the two + // stubs and the bridge, an entirely open chain that "extruded" only because OCCT will + // make a face out of an open wire — the silently-wrong geometry this area exists to stop. + // This test is about the bridge's serialization round-trip, and deserialize_recipe + // recomputes, so the document has to be one that legitimately builds. std::vector ents = { - {SketchEntity::Type::Line, Vec2d(0,0), Vec2d(10,0)}, - {SketchEntity::Type::Line, Vec2d(20,0), Vec2d(30,0)}, + {SketchEntity::Type::Line, Vec2d(0,0), Vec2d(10,0)}, + {SketchEntity::Type::Line, Vec2d(20,0), Vec2d(30,0)}, + {SketchEntity::Type::Line, Vec2d(30,0), Vec2d(30,10)}, + {SketchEntity::Type::Line, Vec2d(30,10), Vec2d(0,10)}, + {SketchEntity::Type::Line, Vec2d(0,10), Vec2d(0,0)}, }; int sk = doc.add_sketch_entities(ents, SketchPlane::XY(), "S"); REQUIRE(sk == 0); int bi = doc.add_bridge(sk, 0, 1, 1, 0, "Bridge"); - REQUIRE(bi == 2); + REQUIRE(bi == 5); doc.add_extrude(sk, 5.0, false, BooleanMode::New, "Extrude"); REQUIRE(doc.recompute()); @@ -4721,7 +4730,7 @@ TEST_CASE("bridge round-trip serialization", "[CadDocument][bridge]") REQUIRE(doc2.deserialize_recipe(blob)); REQUIRE(doc2.features.size() == 2); - const auto& br = doc2.features[0].entities[2]; + const auto& br = doc2.features[0].entities[5]; REQUIRE(br.type == SketchEntity::Type::BSpline); REQUIRE(br.ctrl.size() == 4); REQUIRE_THAT(br.ctrl.front().x(), WithinAbs(10.0, 1e-9)); @@ -7484,14 +7493,21 @@ TEST_CASE("An entity sketch that forms no wire fails instead of extruding a defa Catch::Matchers::WithinRel(M_PI * 100.0 * 5.0, 0.01)); } - SECTION("circle + stray line is rejected, not silently turned into a box") { + // Behaviour CHANGED 2026-09-02 by Tommaso's decision. This used to require a refusal, + // because back then ignoring the stray meant falling through to a default rectangle — a + // box the user never drew. That fallback is gone: the closed profile is now built and the + // stray open chain is discarded, exactly as the viewport's region_loops already discards + // open chains when it decides what is extrudable. A stray click must not break a model + // that looks perfect on screen. + SECTION("circle + stray line builds the circle and ignores the stray") { CadDocument doc; int sk = doc.add_sketch_entities({ circle({0, 0}, 10.0), line({40, 40}, {60, 40}) }, SketchPlane::XY(), "Sketch"); doc.add_extrude(sk, 5.0, false, BooleanMode::New, "Extrude"); - CHECK_FALSE(doc.recompute()); - CHECK(doc.error.find("does not bound a face") != std::string::npos); - CHECK(doc.bodies.empty()); + REQUIRE(doc.recompute()); + CHECK(doc.error.empty()); + CHECK_THAT(doc.body_mass_properties(0).volume, + Catch::Matchers::WithinRel(M_PI * 100.0 * 5.0, 0.01)); } SECTION("two disjoint circles are rejected too") { diff --git a/tests/libslic3r/test_sketchedit.cpp b/tests/libslic3r/test_sketchedit.cpp index 973785e812..634e743b99 100644 --- a/tests/libslic3r/test_sketchedit.cpp +++ b/tests/libslic3r/test_sketchedit.cpp @@ -1,6 +1,7 @@ #include // mainline OrcaSlicer ships Catch2 v3 (v2 was catch2/catch.hpp) #include "libslic3r/CAD/SketchEngine.hpp" #include +#include #include #include @@ -652,3 +653,78 @@ TEST_CASE("auto-close off makes the kernel demand an exact joint", "[SketchEngin // Restore the default so test order cannot leak OFF into the other cases. Slic3r::set_sketch_auto_close(true); } + +// A stray open segment touching nothing must not break a closed profile: the viewport +// discards open chains when it shades a region extrudable, so with closed_only the kernel +// must discard them too — otherwise Revolve/Extrude fail on a sketch that looks perfect. +TEST_CASE("a stray open segment does not break a closed profile", "[SketchEngine]") +{ + auto line = [](double x0, double y0, double x1, double y1) { + SketchEntity e; + e.type = SketchEntity::Type::Line; + e.p0 = Vec2d(x0, y0); + e.p1 = Vec2d(x1, y1); + return e; + }; + + std::vector ents; + ents.push_back(line(0, 0, 20, 0)); // 20x10 quad + ents.push_back(line(20, 0, 20, 10)); + ents.push_back(line(20, 10, 0, 10)); + ents.push_back(line(0, 10, 0, 0)); + ents.push_back(line(5, 5, 6, 5.2)); // stray, touches nothing + + auto edge_count = [](const TopoDS_Wire& w) { + int n = 0; + for (TopExp_Explorer ex(w, TopAbs_EDGE); ex.More(); ex.Next()) ++n; + return n; + }; + + // Unchanged behaviour: the stray line is its own open wire. + auto wires_all = SketchEngine::entities_to_wires(ents, SketchPlane::XY(), /*closed_only=*/false); + REQUIRE(wires_all.size() == 2); + + // closed_only drops the open chain: one closed quad survives. + auto wires_closed = SketchEngine::entities_to_wires(ents, SketchPlane::XY(), /*closed_only=*/true); + REQUIRE(wires_closed.size() == 1); + REQUIRE(edge_count(wires_closed[0]) == 4); + REQUIRE(wires_closed[0].Closed()); + + // The Revolve path (entities_to_wire) finds the single closed loop. + TopoDS_Wire w = SketchEngine::entities_to_wire(ents, SketchPlane::XY(), /*closed_only=*/true); + REQUIRE_FALSE(w.IsNull()); + REQUIRE(edge_count(w) == 4); +} + +// sketch_open_ends names the two free endpoints of an open chain, so the "does not form a +// single closed wire" failure can say WHERE the sketch is open. +TEST_CASE("sketch_open_ends names where a chain fails to close", "[SketchEngine]") +{ + auto line = [](double x0, double y0, double x1, double y1) { + SketchEntity e; + e.type = SketchEntity::Type::Line; + e.p0 = Vec2d(x0, y0); + e.p1 = Vec2d(x1, y1); + return e; + }; + + // Open C shape: three lines, free endpoints at (0,0) and (0,10). + std::vector ents; + ents.push_back(line(0, 0, 10, 0)); + ents.push_back(line(10, 0, 10, 10)); + ents.push_back(line(10, 10, 0, 10)); + + auto got = sketch_open_ends(ents, SketchPlane::XY()); + REQUIRE(got.size() == 2); + + std::sort(got.begin(), got.end(), [](const Vec2d& a, const Vec2d& b) { + if (a.x() < b.x()) return true; + if (a.x() > b.x()) return false; + return a.y() < b.y(); + }); + + REQUIRE_THAT(got[0].x(), WithinAbs(0.0, 1e-9)); + REQUIRE_THAT(got[0].y(), WithinAbs(0.0, 1e-9)); + REQUIRE_THAT(got[1].x(), WithinAbs(0.0, 1e-9)); + REQUIRE_THAT(got[1].y(), WithinAbs(10.0, 1e-9)); +}