A reference is a reference whatever feature holds it

Port of snaporca 1bb9825db0. Four defects from an independent 20-agent audit,
each verified in the code first; two further findings from the same report were
verified OUT and are not in this commit.

remove_feature()/move_feature() remapped Extrude::sketch_ref and a Mate's two
connectors and nothing else, leaving seven of the nine index-bearing fields —
sweep_path_ref, loft_profile_refs[], pattern_curve_sketch, rib_sketch_ref, and
sketch_ref on Revolve, Sweep, Rib and the Surface* family — pointing at whatever
slid into the slot. Quiet by construction: the shifted index still names a real
feature, recompute() succeeds, the solid is built from the wrong profile. The
comment above the loop already required "EVERY field holding a feature index"; the
code under it handled two, because a type switch is only correct on the day it is
written. for_each_feature_ref() visits the FIELDS instead, so a feature type added
later is covered the moment it reuses one. plane_base and axis_plane_a/b are
excluded on purpose and documented at the helper — they encode an ordinal into the
datum-plane list, not an index into features[], and are filed separately. The
delete cascade got the same field-based treatment.

The regression test was run against the pre-fix code to prove it bites: all three
sections fail there, and move_feature returns TRUE while leaving sketch_ref == 1
where it must be 0 — success with the wrong answer, which is what makes this class
expensive.

apply_constraint, commit_entity_constraints and delete_constraint mutated the
recipe with no checkpoint() and no sync_recipe_to_model(), alone among seventeen
mutation sites in that file: Ctrl+Z reached past the constraint edit and discarded
unrelated work, and saving persisted the pre-constraint blob. A rejected constraint
now calls abandon_checkpoint() rather than leaving an undo step that does nothing.

MCP: params["generation"].get<uint64_t>() sat outside the try inside a bare
CallAfter lambda, so one malformed string terminated the process through the wx
event loop; it is type-checked now and the lambda lets nothing escape. The socket
bound with no mode of its own in a world-writable directory — umask around bind()
plus chmod, and it refuses to listen rather than listen wide. The reply write is no
longer a bare write(), which could SIGPIPE the app when a client hung up.

Kernel suite on this fork: 190 cases / 2562 assertions, green. GUI target compiles.
The full ladder gate ran on snaporca (ALL LADDERS HELD — gestures 98/98, offer
108/108, corpus and corpus-scale green) and fork-check parity holds at 17 identical
/ 8 diverging as expected, which is what makes that gate transferable here.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MrMzTpAf78U4NG2M8jfvHY
This commit is contained in:
Tommaso Bianchi
2026-08-24 19:00:45 +02:00
co-authored by Claude Opus 5
parent 6e7f6429fa
commit 3fd5c3353a
5 changed files with 188 additions and 23 deletions
+72
View File
@@ -8145,3 +8145,75 @@ TEST_CASE("a body carries its own name, through recompute and the recipe", "[Cad
CHECK(fresh.bodies[0].has_user_name);
CHECK(fresh.bodies[0].user_name == "Bracket");
}
// A feature reference is a reference whatever feature holds it.
//
// remove_feature()/move_feature() used to remap Extrude::sketch_ref and a Mate's two
// connectors, and nothing else — so every later consumer of a Sketch (Revolve, Sweep, Loft,
// Rib, Pattern-on-curve, the Surface* family) kept an index that the erase had just
// invalidated. The failure is quiet by construction: a shifted index still names a real
// feature, recompute() succeeds, and what comes out is built from the wrong profile.
//
// The volume is the assertion. Two different profiles go in; deleting the unused feature in
// front of them must leave the SAME solid behind, which it only can if the reference moved
// with its target.
TEST_CASE("deleting a feature remaps the references of every consumer, not just Extrude",
"[CadDocument]")
{
using namespace Slic3r;
const SketchPlane xy = SketchPlane::XY();
// A rectangle 10x10 centred at v = 15, revolved 360 deg about the plane X axis.
auto rect_at = [&](double v0) {
CadFeature sk;
sk.type = CadFeatureType::Sketch;
sk.plane = xy;
sk.profile.points = { Vec2d(-5, v0 - 5), Vec2d(5, v0 - 5),
Vec2d(5, v0 + 5), Vec2d(-5, v0 + 5) };
sk.profile.closed = true;
return sk;
};
SECTION("Revolve::sketch_ref survives the deletion of a feature in front of it") {
CadDocument doc;
doc.features.push_back(rect_at(40.0)); // f0: a decoy profile, never consumed
doc.features.push_back(rect_at(15.0)); // f1: the real profile
doc.add_revolve(1, 360.0, /*axis=X*/0, false, BooleanMode::New, "Rev");
REQUIRE(doc.recompute());
const double before = double(doc.display_mesh.volume());
REQUIRE(before == Approx(9424.78).epsilon(0.05));
// f0 is consumed by nothing, so this deletes exactly one feature and shifts f1 to 0.
REQUIRE(doc.remove_feature(0));
REQUIRE(doc.features.size() == 2);
REQUIRE(doc.features[1].type == CadFeatureType::Revolve);
REQUIRE(doc.features[1].sketch_ref == 0); // followed its target
REQUIRE(doc.error.empty());
REQUIRE(double(doc.display_mesh.volume()) == Approx(before).epsilon(1e-6));
}
SECTION("move_feature swaps a Revolve's reference too") {
CadDocument doc;
doc.features.push_back(rect_at(40.0)); // f0
doc.features.push_back(rect_at(15.0)); // f1 -> consumed
doc.add_revolve(1, 360.0, 0, false, BooleanMode::New, "Rev");
REQUIRE(doc.recompute());
const double before = double(doc.display_mesh.volume());
REQUIRE(doc.move_feature(0, 1)); // f0 and f1 trade places
REQUIRE(doc.features[2].sketch_ref == 0); // the profile is now at 0
REQUIRE(double(doc.display_mesh.volume()) == Approx(before).epsilon(1e-6));
}
SECTION("deleting a Sketch cascades to a Revolve that consumes it") {
CadDocument doc;
doc.features.push_back(rect_at(15.0));
doc.add_revolve(0, 360.0, 0, false, BooleanMode::New, "Rev");
REQUIRE(doc.recompute());
// The Revolve has no profile without this Sketch, exactly as an Extrude would not:
// it goes with it rather than being left pointing at nothing.
REQUIRE(doc.remove_feature(0));
REQUIRE(doc.features.empty());
}
}