diff --git a/src/libslic3r/CadDocument.cpp b/src/libslic3r/CadDocument.cpp index 8897d1f3a0..6a31cf04d0 100644 --- a/src/libslic3r/CadDocument.cpp +++ b/src/libslic3r/CadDocument.cpp @@ -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 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); diff --git a/tests/libslic3r/test_caddocument.cpp b/tests/libslic3r/test_caddocument.cpp index 742c20e30a..de888f2bb6 100644 --- a/tests/libslic3r/test_caddocument.cpp +++ b/tests/libslic3r/test_caddocument.cpp @@ -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]") {