From b9d6b59f901b29789dd75d29722a38cddd2834b7 Mon Sep 17 00:00:00 2001 From: Tommaso Bianchi Date: Wed, 12 Aug 2026 20:57:35 +0200 Subject: [PATCH] A feature that destroys a body must say so, not ship a phantom MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Driving the control socket: hexagon prism, six vertical fillets, four chamfers on the already-filleted rim, an M8 hole. Afterwards describe_scene reported bodies=3 and error='' — entirely healthy — while body 2's TopoDS_Shape was null. Only mass_properties on that one body revealed anything was wrong. So a feature destroyed a body, recompute() returned true, and the document went on advertising it. Any downstream consumer — slicing, STEP export, a mass properties report — met a null shape with no warning. That is the silent corruption class, which is the one class this project does not tolerate. recompute() now scans the freshly built bodies for a null shape, names the body and the feature that destroyed it, and returns false. Returning false rather than just setting error is the point: it hands the caller its normal rollback path, so the operation that destroyed the body is undone instead of committed. The message says "an unidentified feature" when source_feature is -1. "feature 0" would be a lie, and a message that exists to tell you where to look has to be trusted. TEST IS A POSITIVE CONTRACT, AND THE REASON MATTERS. The reported order was driven headlessly first, as the better test: it does NOT reproduce. The dress-up step throws "fillet radius too large", which is an already-loud already-caught path, so recompute fails honestly and never nulls a body. No public-API sequence found so far reaches the guard's branch without a GUI, and faking a null into `bodies` after the fact would not exercise it — the guard runs on `built`, before the swap. So the test asserts what can be asserted: a box + fillet recomputes true, error is empty, and no body is null. The guard's own branch is defensive and currently unexercised; that is stated here rather than implied by a green suite. Kernel suite: 2217 assertions in 161 test cases, all passing. No existing test relied on a null body surviving a recompute, so hardening this broke nothing. snaporca-5425 (part a). Part b — why the chamfer chain degenerates on an already-filleted rim — is untouched and stays open. --- src/libslic3r/CadDocument.cpp | 20 +++++++++++++++ tests/libslic3r/test_caddocument.cpp | 37 ++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+) diff --git a/src/libslic3r/CadDocument.cpp b/src/libslic3r/CadDocument.cpp index e601c16d8f..34862a1cbe 100644 --- a/src/libslic3r/CadDocument.cpp +++ b/src/libslic3r/CadDocument.cpp @@ -3373,6 +3373,26 @@ bool CadDocument::recompute() } if (built.empty()) { error = "no solid-producing features"; return false; } + // A feature that leaves a body with a null shape must fail loudly. Until this existed, + // recompute() returned true and the document kept advertising the body: describe_scene + // counted it, error was empty, and only mass_properties on that specific body revealed + // anything was wrong. Returning false hands the caller its normal rollback path, so the + // operation that destroyed the body is undone rather than committed. + for (size_t i = 0; i < built.size(); ++i) { + if (!built[i].shape.IsNull()) continue; + const int src = built[i].source_feature; + // source_feature is -1 for a body no feature claims. "feature 0" would be a lie, and + // this message exists precisely to be trusted about which feature to look at. + const std::string fname = + (src < 0 || src >= int(features.size())) + ? std::string("an unidentified feature") + : (features[src].name.empty() ? ("feature " + std::to_string(src + 1)) + : features[src].name); + error = "body " + std::to_string(i + 1) + " was destroyed by " + fname + + " (the operation produced an empty shape)"; + return false; + } + // recompute() replaces the bodies vector wholesale, which would drop any per-body // colour override (Color tool). Body indices are stable across a rebuild (bodies are // appended in feature order), so carry the override forward by index — same indexing diff --git a/tests/libslic3r/test_caddocument.cpp b/tests/libslic3r/test_caddocument.cpp index eb45aa791f..2389ca7324 100644 --- a/tests/libslic3r/test_caddocument.cpp +++ b/tests/libslic3r/test_caddocument.cpp @@ -7101,3 +7101,40 @@ TEST_CASE("plane_of_face gives a sketchable plane for a planar face only", "[Cad CHECK(refused > 0); } } + +// snaporca-5425 — POSITIVE-CONTRACT variant. A feature that left a body with a null +// TopoDS_Shape used to be tolerated: recompute() returned true and the document kept +// advertising the body. The new guard makes that a hard failure. This test asserts the +// contract the guard preserves on the healthy side: a normal box + fillet document +// recomputes true, reports an empty error, and NO resulting body has a null shape. +// +// Why not assert the negative branch (a dress-up chain nulling a body -> recompute false)? +// Reproducing the original silent-null degeneracy (prism -> 6 vertical fillets -> chamfer on +// the already-filleted rim -> hole) is a separate open question. Driving that order headlessly +// makes the dress-up step THROW (OCCT "fillet radius too large" — an already-loud, already- +// caught path) rather than silently null a body, so no public-API sequence reliably reaches +// the guard's null branch in a headless test. Faking one (writing a null into `bodies` after +// recompute) would not exercise the guard — it runs on the freshly-built vector — and a test +// that asserts a state the code cannot reach is exactly what the contract forbids. +TEST_CASE("recompute on a healthy box + fillet leaves no body null and no error (positive contract)", "[CadDocument]") +{ + using namespace Slic3r; + CadDocument doc; + SketchProfile sp; + sp.points.push_back(Vec2d(0, 0)); + sp.points.push_back(Vec2d(20, 0)); + sp.points.push_back(Vec2d(20, 20)); + sp.points.push_back(Vec2d(0, 20)); + sp.closed = true; + const int sk = doc.add_sketch_profile(sp, SketchPlane::XY(), "Box"); + doc.add_extrude(sk, 10.0, false, BooleanMode::New, "Extrude"); + doc.add_fillet(2.0, FaceGroup::All, "Fillet"); + + REQUIRE(doc.recompute()); + REQUIRE(doc.error.empty()); + REQUIRE_FALSE(doc.bodies.empty()); + for (size_t i = 0; i < doc.bodies.size(); ++i) { + INFO("body " << i << " has a null shape"); + REQUIRE_FALSE(doc.bodies[i].shape.IsNull()); + } +}