juli/OrcaSlicer
1
1
Fork 0
mirror of https://github.com/OrcaSlicer/OrcaSlicer.git synced 2026-09-23 17:02:39 +00:00
Code Issues Packages Projects Releases Wiki Activity

CadDocument: reindex mate connectors on feature delete and reorder

Browse Source 
mate_cs_a/mate_cs_b are feature indices. remove_feature() remapped sketch_ref
through the deletion but not the mate connectors, and move_feature() swapped
sketch_ref but not the mate connectors. Deleting or reordering any feature
ahead of a connector slid both references onto whatever features landed on
those slots.

Nothing reported it. recompute() only rejects out-of-range and non-CoordSys
targets, and a shifted index normally lands on the assembly's other CoordSys —
an assembly carries at least two by construction. So the mate resolved against
the wrong frames and moved the wrong body, silently.

Extracted a remap lambda in remove_feature() and a swap_ref lambda in
move_feature(), applied to sketch_ref and both mate connectors.

Two tests, both confirmed red before the fix. [mate] tags green here:
395 assertions / 29 cases — the first end-to-end kernel compile of this fork.

Ported from snaporca; CadDocument.cpp is byte-identical across forks again.

Refs: snaporca-kqih

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Tommaso Bianchi
2026-08-12 13:14:10 +02:00
co-authored by Claude Opus 5
parent 0a2faedc32
commit 8ff7ba440d
2 changed files with 139 additions and 12 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/libslic3r/CadDocument.cpp
+33 -12
View File
@@ -1856,18 +1856,31 @@ bool CadDocument::remove_feature(int index)
for (auto it = remove.rbegin(); it != remove.rend(); ++it)
features.erase(features.begin() + *it);
// Remap surviving sketch_ref through the deletions: subtract the count of
// Remap surviving feature references through the deletions: subtract the count of
// removed indices that sat before it; orphaned refs (target removed) -> -1.
for (auto& f : features) {
if (f.type != CadFeatureType::Extrude || f.sketch_ref < 0)
continue;
if (std::binary_search(remove.begin(), remove.end(), f.sketch_ref)) {
f.sketch_ref = -1;
//
// This must cover EVERY field holding a feature index, not just sketch_ref. A mate's two
// connectors are such indices, and leaving them behind slid them onto whatever features
// landed on those slots — silently, because recompute() only rejects out-of-range and
// non-CoordSys targets, and a shifted index usually lands on the assembly's other CoordSys.
auto remap = [&remove](int& ref) {
if (ref < 0)
return;
if (std::binary_search(remove.begin(), remove.end(), ref)) {
ref = -1;
} else {
int shift = 0;
for (int r : remove)
if (r < f.sketch_ref) ++shift;
f.sketch_ref -= shift;
if (r < ref) ++shift;
ref -= shift;
}
};
for (auto& f : features) {
if (f.type == CadFeatureType::Extrude) {
remap(f.sketch_ref);
} else if (f.type == CadFeatureType::Mate) {
remap(f.mate_cs_a);
remap(f.mate_cs_b);
}
}
@@ -1885,11 +1898,19 @@ bool CadDocument::move_feature(int index, int delta)
std::vector<CadFeature> snapshot = features;
std::swap(features[index], features[target]);
// The two slots traded places: fix any sketch_ref that pointed at either.
// The two slots traded places: fix every feature reference that pointed at either —
// a mate's connectors as well as sketch_ref, for the reason given in remove_feature().
auto swap_ref = [index, target](int& ref) {
if (ref == index) ref = target;
else if (ref == target) ref = index;
};
for (auto& f : features) {
if (f.type != CadFeatureType::Extrude) continue;
if (f.sketch_ref == index) f.sketch_ref = target;
else if (f.sketch_ref == target) f.sketch_ref = index;
if (f.type == CadFeatureType::Extrude) {
swap_ref(f.sketch_ref);
} else if (f.type == CadFeatureType::Mate) {
swap_ref(f.mate_cs_a);
swap_ref(f.mate_cs_b);
}
}
return commit_or_rollback(*this, snapshot);
tests/libslic3r/test_caddocument.cpp
+106
View File
@@ -5433,6 +5433,112 @@ TEST_CASE("mate error: out of range connectors", "[CadDocument][mate]")
doc.features.pop_back(); doc.error.clear();
}
// A mate stores its two connectors as FEATURE INDICES. remove_feature() erases a slot and remaps
// every surviving sketch_ref through the deletion, but it did not remap mate_cs_a / mate_cs_b —
// so deleting anything ahead of a connector slid both references down onto whatever features
// happened to occupy those indices. The validation in recompute() only catches out-of-range and
// non-CoordSys targets; when the landing slots are themselves CoordSys features — the normal
// case, since an assembly carries at least two — nothing reports anything. The mate silently
// resolves against the wrong frames and moves the wrong body.
//
// The third connector below (CS_Spare) is what makes the corruption silent rather than loud:
// without it the one-slot shift would push mate_cs_b onto the Mate feature itself and trip the
// "not a valid CoordSys" guard, hiding the real defect behind an error that looks handled.
TEST_CASE("mate connectors survive deletion of an earlier feature", "[CadDocument][mate]")
{
using Catch::Matchers::WithinAbs;
CadDocument doc;
int sk_box = doc.add_sketch(SketchShape::Rectangle, SketchPlane::XY(), 10, 10, 0, "Box");
doc.add_extrude(sk_box, 5.0, false, BooleanMode::New, "BoxExt");
REQUIRE(doc.recompute());
REQUIRE(doc.error.empty());
// The victim: an unrelated connector sitting AHEAD of the pair the mate will use.
int cs_decoy = doc.add_coordsys(CoordSysType::PointWorld, Vec3d(1, 1, 1), "CS_Decoy");
doc.features[cs_decoy].coordsys_body = 0;
REQUIRE(doc.recompute());
int sk_cyl = doc.add_sketch(SketchShape::Circle, SketchPlane::XY(), 0, 0, 3, "Cyl");
doc.add_extrude(sk_cyl, 10.0, false, BooleanMode::New, "CylExt");
REQUIRE(doc.recompute());
REQUIRE(doc.bodies.size() == 2);
int cs_fixed = doc.add_coordsys(CoordSysType::PointWorld, Vec3d(5, 5, 5), "CS_Fixed");
doc.features[cs_fixed].coordsys_body = 0;
int cs_moving = doc.add_coordsys(CoordSysType::PointWorld, Vec3d(0, 0, 5), "CS_Moving");
doc.features[cs_moving].coordsys_body = 1;
int cs_spare = doc.add_coordsys(CoordSysType::PointWorld, Vec3d(9, 9, 9), "CS_Spare");
doc.features[cs_spare].coordsys_body = 0;
REQUIRE(doc.recompute());
REQUIRE(doc.error.empty());
doc.add_mate(0, cs_fixed, cs_moving, 0.0, 0.0, false, "Mate");
REQUIRE(doc.recompute());
REQUIRE(doc.error.empty());
GProp_GProps props;
BRepGProp::VolumeProperties(doc.bodies[1].shape, props);
REQUIRE_THAT(double(props.CentreOfMass().X()), WithinAbs(5.0, 1e-4));
REQUIRE(doc.remove_feature(cs_decoy));
REQUIRE(doc.recompute());
REQUIRE(doc.error.empty());
// The mate must still name the same two connectors it was built with.
const CadFeature& mate = doc.features.back();
REQUIRE(mate.type == CadFeatureType::Mate);
REQUIRE(mate.mate_cs_a >= 0);
REQUIRE(mate.mate_cs_b >= 0);
REQUIRE(doc.features[mate.mate_cs_a].name == "CS_Fixed");
REQUIRE(doc.features[mate.mate_cs_b].name == "CS_Moving");
// ...and therefore still assemble the same way: the cylinder on the fixed connector.
BRepGProp::VolumeProperties(doc.bodies[1].shape, props);
REQUIRE_THAT(double(props.CentreOfMass().X()), WithinAbs(5.0, 1e-4));
REQUIRE_THAT(double(props.CentreOfMass().Y()), WithinAbs(5.0, 1e-4));
REQUIRE_THAT(double(props.CentreOfMass().Z()), WithinAbs(5.0, 1e-4));
}
// move_feature() has the same blind spot from the other direction: it swaps two slots and
// rewrites sketch_ref for both, but leaves mate_cs_a / mate_cs_b pointing at the old positions.
TEST_CASE("mate connectors survive reordering of an earlier feature", "[CadDocument][mate]")
{
CadDocument doc;
int sk_box = doc.add_sketch(SketchShape::Rectangle, SketchPlane::XY(), 10, 10, 0, "Box");
doc.add_extrude(sk_box, 5.0, false, BooleanMode::New, "BoxExt");
REQUIRE(doc.recompute());
int sk_cyl = doc.add_sketch(SketchShape::Circle, SketchPlane::XY(), 0, 0, 3, "Cyl");
doc.add_extrude(sk_cyl, 10.0, false, BooleanMode::New, "CylExt");
REQUIRE(doc.recompute());
REQUIRE(doc.bodies.size() == 2);
int cs_fixed = doc.add_coordsys(CoordSysType::PointWorld, Vec3d(5, 5, 5), "CS_Fixed");
doc.features[cs_fixed].coordsys_body = 0;
int cs_moving = doc.add_coordsys(CoordSysType::PointWorld, Vec3d(0, 0, 5), "CS_Moving");
doc.features[cs_moving].coordsys_body = 1;
REQUIRE(doc.recompute());
REQUIRE(doc.error.empty());
doc.add_mate(0, cs_fixed, cs_moving, 0.0, 0.0, false, "Mate");
REQUIRE(doc.recompute());
REQUIRE(doc.error.empty());
// Swap the two connectors' slots. The mate's references must follow the features,
// not stay behind on the indices.
REQUIRE(doc.move_feature(cs_fixed, 1));
REQUIRE(doc.recompute());
REQUIRE(doc.error.empty());
const CadFeature& mate = doc.features.back();
REQUIRE(mate.type == CadFeatureType::Mate);
REQUIRE(mate.mate_cs_a >= 0);
REQUIRE(mate.mate_cs_b >= 0);
REQUIRE(doc.features[mate.mate_cs_a].name == "CS_Fixed");
REQUIRE(doc.features[mate.mate_cs_b].name == "CS_Moving");
}
TEST_CASE("mate error: no associated body", "[CadDocument][mate]")
{