A feature that destroys a body must say so, not ship a phantom

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.
This commit is contained in:
Tommaso Bianchi
2026-08-12 20:57:35 +02:00
parent e8306a6e9a
commit b9d6b59f90
2 changed files with 57 additions and 0 deletions
+20
View File
@@ -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