From 7f0a8c85ee05fd3e16d5e549bfdb110297d8e0df Mon Sep 17 00:00:00 2001 From: Tommaso Bianchi Date: Thu, 30 Jul 2026 09:56:18 +0200 Subject: [PATCH] An entity sketch that forms no wire fails, instead of extruding a default box entities_to_wire handles exactly two shapes: one lone closed entity (Circle/Ellipse), or a chain of open ones (Line/Arc/EllipseArc/BSpline). Anything else -- a circle coexisting with a line, two circles -- returns a null wire. build_sketch_wire answered that by falling through to its legacy tail, which ends in a rectangle built from width/height. For an entity sketch those fields are whatever they were initialised to, so the extrude produced a box the user never drew, silently and with ok:true. That is how the ellipse+stray-arc case in the P2 Tier-B.1 verification turned into a default-rectangle solid. Throw there instead. The legacy profile/shape paths below are still reached by sketches that legitimately carry no entities at all, so the enum and profile constructors are untouched -- only the case where entities exist and cannot be turned into a wire now fails, which is exactly the case that was fabricating geometry. This does NOT implement the multi-loop support the issue asks for. Doing that properly means deciding containment -- a circle inside a rectangle is a hole, a circle beside it is a second region -- and make_extrude_regions cannot be reused because it takes flattened Vec2d contours for imported Text/SVG art and would discard the analytic circle. Guessing containment would trade a visible failure for a wrong solid, which is the opposite of the point. Left scoped on snaporca-88v. Also converts the three float comparisons in the two test cases added this session from Approx to WithinAbs/WithinRel, per tests/CLAUDE.md, which rules Approx out for being asymmetric and double-only. The rest of the file's pre-existing Approx uses are left alone. 153 cases / 2090 assertions green on both forks; no existing test depended on the default-rectangle fallback. snaporca-88v (partial: the silent-fallback half). --- src/libslic3r/CadDocument.cpp | 13 ++++++- tests/libslic3r/test_caddocument.cpp | 52 ++++++++++++++++++++++++++-- 2 files changed, 61 insertions(+), 4 deletions(-) diff --git a/src/libslic3r/CadDocument.cpp b/src/libslic3r/CadDocument.cpp index caf46280ce..de50cc1da1 100644 --- a/src/libslic3r/CadDocument.cpp +++ b/src/libslic3r/CadDocument.cpp @@ -1930,7 +1930,18 @@ TopoDS_Wire CadDocument::build_sketch_wire(const CadFeature& sketch) const if (!sketch.entities.empty()) { TopoDS_Wire w = SketchEngine::entities_to_wire(sketch.entities, sketch.plane); if (!w.IsNull()) return w; - // fall through to legacy paths if entities produced nothing + // 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, + // which for an entity sketch are whatever they happened to be initialised to — so a + // sketch entities_to_wire cannot handle (a circle coexisting with a line, two circles: + // snaporca-88v) used to extrude into a box the user never drew, silently. Failing here + // costs the caller an error message; falling through cost them wrong geometry that + // 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"); } if (!sketch.profile.points.empty()) { SketchProfile prof = sketch.profile; diff --git a/tests/libslic3r/test_caddocument.cpp b/tests/libslic3r/test_caddocument.cpp index e90f3d2013..e35204566a 100644 --- a/tests/libslic3r/test_caddocument.cpp +++ b/tests/libslic3r/test_caddocument.cpp @@ -6726,7 +6726,8 @@ TEST_CASE("A cut that removes no material is an error, not a silent success", "[ // And the body is left as it was, not half-applied. CadDocument again = box(); REQUIRE(again.recompute()); - CHECK(again.body_mass_properties(0).volume == Approx(solid).margin(1e-6)); + CHECK_THAT(again.body_mass_properties(0).volume, + Catch::Matchers::WithinAbs(solid, 1e-6)); } SECTION("the same hole on the body still works") { @@ -6735,8 +6736,8 @@ TEST_CASE("A cut that removes no material is an error, not a silent success", "[ const double solid = doc.body_mass_properties(0).volume; doc.add_hole(8.0, 20.0, true, 0.0, 0.0, SketchPlane::XY(), "Hole"); REQUIRE(doc.recompute()); - CHECK(doc.body_mass_properties(0).volume - == Approx(solid - M_PI * 16.0 * 20.0).epsilon(0.01)); + CHECK_THAT(doc.body_mass_properties(0).volume, + Catch::Matchers::WithinRel(solid - M_PI * 16.0 * 20.0, 0.01)); } SECTION("a cut-mode extrude that misses is rejected too") { @@ -6752,3 +6753,48 @@ TEST_CASE("A cut that removes no material is an error, not a silent success", "[ CHECK(doc.error.find("removed no material") != std::string::npos); } } + +// entities_to_wire handles exactly two shapes: one lone closed entity, or a chain of open ones. +// Anything else returns a null wire, and build_sketch_wire used to answer that by falling through +// to its legacy tail — which ends in a rectangle built from width/height. For an entity sketch +// those are whatever they were initialised to, so the extrude produced a box nobody drew. +// snaporca-88v. +TEST_CASE("An entity sketch that forms no wire fails instead of extruding a default box", "[CadDocument]") +{ + auto circle = [](Vec2d c, double r) { + SketchEntity e; e.type = SketchEntity::Type::Circle; e.center = c; e.p0 = c; e.radius = r; + return e; + }; + auto line = [](Vec2d a, Vec2d b) { + SketchEntity e; e.type = SketchEntity::Type::Line; e.p0 = a; e.p1 = b; + return e; + }; + + SECTION("a lone circle still works — the supported case is untouched") { + CadDocument doc; + int sk = doc.add_sketch_entities({ circle({0, 0}, 10.0) }, SketchPlane::XY(), "Sketch"); + doc.add_extrude(sk, 5.0, false, BooleanMode::New, "Extrude"); + REQUIRE(doc.recompute()); + CHECK_THAT(doc.body_mass_properties(0).volume, + Catch::Matchers::WithinRel(M_PI * 100.0 * 5.0, 0.01)); + } + + SECTION("circle + stray line is rejected, not silently turned into a box") { + 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("do not form a single closed wire") != std::string::npos); + CHECK(doc.bodies.empty()); + } + + SECTION("two circles are rejected too") { + CadDocument doc; + int sk = doc.add_sketch_entities({ circle({-20, 0}, 8.0), circle({20, 0}, 8.0) }, + SketchPlane::XY(), "Sketch"); + doc.add_extrude(sk, 5.0, false, BooleanMode::New, "Extrude"); + CHECK_FALSE(doc.recompute()); + CHECK(doc.error.find("do not form a single closed wire") != std::string::npos); + } +}