diff --git a/src/libslic3r/CadDocument.cpp b/src/libslic3r/CadDocument.cpp index 34862a1cbe..ec1fcb4ec6 100644 --- a/src/libslic3r/CadDocument.cpp +++ b/src/libslic3r/CadDocument.cpp @@ -3404,6 +3404,37 @@ bool CadDocument::recompute() } } bodies = std::move(built); + + // Face-drift fingerprint for FaceAndDirection CoordSys connectors. This runs AFTER the + // bodies are final and APPENDS to mate_conflicts (detect_mate_conflicts() cleared it at + // the top of recompute() and must not run again here). A mismatch is a WARNING, not an + // error: a legitimate Draft on a mated face renumbers nothing but a real renumber after a + // dress-up silently points the connector at a different face, and that must not abort. + for (int fi = 0; fi < int(features.size()); ++fi) { + CadFeature& f = features[fi]; + if (!f.enabled) continue; + if (f.type != CadFeatureType::CoordSys) continue; + if (f.coordsys_type != CoordSysType::FaceAndDirection) continue; + if (f.coordsys_body < 0 || f.coordsys_body >= int(bodies.size())) continue; + TopoDS_Face face = GeometryEngine::face_by_index(bodies[f.coordsys_body].shape, f.coordsys_face); + if (face.IsNull()) continue; // "face not found" is already reported by datum_frame() + const int kind = int(BRepAdaptor_Surface(face).GetType()); + const int edges = int(GeometryEngine::edges_of_face(face).size()); + if (f.coordsys_face_kind < 0) { + // No fingerprint yet (old recipe, or a connector never resolved): adopt the state + // the user last saw. Self-heals old recipes on first load, and gives both existing + // writers the fingerprint for free. + f.coordsys_face_kind = kind; + f.coordsys_face_edges = edges; + continue; + } + if (f.coordsys_face_kind != kind || f.coordsys_face_edges != edges) { + mate_conflicts.emplace_back(int(fi), + "connector \"" + f.name + "\" may have moved to a different face " + "(an upstream edit renumbered this body's faces)"); + } + } + body = compound_of(bodies); display_mesh = tessellate_bodies(bodies, display_tri_face, display_tri_body, display_body_meshes, diff --git a/src/libslic3r/CadDocument.hpp b/src/libslic3r/CadDocument.hpp index 2016649392..ceab1a2e66 100644 --- a/src/libslic3r/CadDocument.hpp +++ b/src/libslic3r/CadDocument.hpp @@ -251,6 +251,17 @@ struct CadFeature { int coordsys_edge{-1}; Vec3d coordsys_x_hint{1, 0, 0}; + // Fingerprint of the face this connector was bound to, for drift detection. -1 = not yet + // recorded (an old recipe, or a connector that has never resolved). + // + // Surface TYPE and EDGE COUNT specifically, because they survive every legitimate edit: + // Transform moves the body, Draft tilts the face, a dimension change resizes it, and none + // of those change either value. Centroid, area and normal all fail that test — see the + // issue. The cost is that a slide from one planar 4-edge face to another planar 4-edge face + // is invisible; a detector that never cries wolf is worth more here than a total one. + int coordsys_face_kind{-1}; // GeomAbs_SurfaceType as int + int coordsys_face_edges{-1}; // number of edges bounding the face + // Helix curve params (consumed as a sweep path to build springs/coils/augers). // Axis = plane normal through plane origin. pitch = axial rise per full turn. // left_handed flips the winding direction. taper_deg != 0 gives a conical helix. @@ -345,7 +356,8 @@ struct CadFeature { rib_sketch_ref, rib_entity, rib_thickness, rib_depth, pattern_curve_sketch, pattern_curve_entity, expr, - mate_kind, mate_cs_a, mate_cs_b, mate_offset, mate_angle, mate_flip); + mate_kind, mate_cs_a, mate_cs_b, mate_offset, mate_angle, mate_flip, + coordsys_face_kind, coordsys_face_edges); } template void load(Archive& ar) { @@ -383,7 +395,8 @@ struct CadFeature { rib_sketch_ref, rib_entity, rib_thickness, rib_depth, pattern_curve_sketch, pattern_curve_entity, expr, - mate_kind, mate_cs_a, mate_cs_b, mate_offset, mate_angle, mate_flip); + mate_kind, mate_cs_a, mate_cs_b, mate_offset, mate_angle, mate_flip, + coordsys_face_kind, coordsys_face_edges); imported_solid = brep_from_string(brep); } }; @@ -594,7 +607,12 @@ public: // - bump this whenever CadFeature::save/load gains or loses a field // - v1 blobs are deliberately not loadable; there is no migration path by design // - append fields ONLY at the end of save/load, never reorder (golden fixture enforces this) - static constexpr uint32_t SNAPORCA_CAD_RECIPE_VERSION = 3; + // v4: coordsys_face_kind + coordsys_face_edges appended (connector face-drift fingerprint). + // The bump is not optional. deserialize_recipe() gates on v == VERSION and then reads a FLAT + // symmetric field list, so a v3 blob under a v3 build that has grown two fields passes the + // gate and then reads two ints past the end of every connector — straight into the next + // feature's bytes. That is silent corruption of a saved project, not a load error. + static constexpr uint32_t SNAPORCA_CAD_RECIPE_VERSION = 4; std::string serialize_recipe() const; bool deserialize_recipe(const std::string& blob); diff --git a/tests/data/cad_recipe_v4.bin b/tests/data/cad_recipe_v4.bin new file mode 100644 index 0000000000..eb8f8ec730 Binary files /dev/null and b/tests/data/cad_recipe_v4.bin differ diff --git a/tests/libslic3r/test_caddocument.cpp b/tests/libslic3r/test_caddocument.cpp index fe29b28ec7..edf1e26853 100644 --- a/tests/libslic3r/test_caddocument.cpp +++ b/tests/libslic3r/test_caddocument.cpp @@ -3767,7 +3767,7 @@ TEST_CASE("regenerate golden recipe fixture", "[.regen]") auto blob = doc.serialize_recipe(); REQUIRE_FALSE(blob.empty()); - std::string path = std::string(TEST_DATA_DIR) + "/cad_recipe_v3.bin"; + std::string path = std::string(TEST_DATA_DIR) + "/cad_recipe_v4.bin"; std::ofstream ofs(path, std::ios::binary); REQUIRE(ofs.is_open()); ofs.write(blob.data(), static_cast(blob.size())); @@ -3775,13 +3775,37 @@ TEST_CASE("regenerate golden recipe fixture", "[.regen]") SUCCEED("Fixture written to " << path); } +// The previous format's real blob, kept on disk deliberately. It is the only thing that can +// prove the version gate does its job: a v3 recipe carries FEWER fields per feature than this +// build reads, so without the bump to v4 it would have passed the gate and had two ints read +// past the end of every connector — into the next feature's bytes. Silent corruption of a saved +// project, which is far worse than a refusal. This asserts the refusal is clean and says why. +// +// Do NOT regenerate cad_recipe_v3.bin. Its value is entirely that it was written by an older +// build; rewriting it with today's code destroys the only evidence this test rests on. +TEST_CASE("a previous-format recipe is refused, not silently misread", "[CadDocument]") +{ + std::string path = std::string(TEST_DATA_DIR) + "/cad_recipe_v3.bin"; + std::ifstream ifs(path, std::ios::binary); + REQUIRE(ifs.is_open()); + std::string blob((std::istreambuf_iterator(ifs)), std::istreambuf_iterator()); + ifs.close(); + REQUIRE_FALSE(blob.empty()); + + CadDocument doc; + REQUIRE_FALSE(doc.deserialize_recipe(blob)); + REQUIRE_FALSE(doc.error.empty()); + REQUIRE(doc.error.find("older version") != std::string::npos); + REQUIRE(doc.features.empty()); // nothing half-read was left behind +} + TEST_CASE("golden recipe v1 still deserialises", "[CadDocument]") { using Catch::Matchers::WithinRel; using Catch::Matchers::WithinAbs; // Read the golden blob from disk - std::string path = std::string(TEST_DATA_DIR) + "/cad_recipe_v3.bin"; + std::string path = std::string(TEST_DATA_DIR) + "/cad_recipe_v4.bin"; std::ifstream ifs(path, std::ios::binary); REQUIRE(ifs.is_open()); std::string blob((std::istreambuf_iterator(ifs)), @@ -7317,3 +7341,91 @@ TEST_CASE("dressup: four chamfer ids captured up-front drift as earlier chamfers INFO("single-recompute driver path: ok=" << ok << " error=" << (ok ? std::string() : doc2.error)); REQUIRE(ok); } + +// --- Face-drift fingerprint: a FaceAndDirection connector warns when its face index slides --- + +TEST_CASE("a connector records its face fingerprint on first recompute", "[CadDocument][mate]") +{ + CadDocument doc; + int sk = doc.add_sketch(SketchShape::Rectangle, SketchPlane::XY(), 20, 20, 10, "Box"); + doc.add_extrude(sk, 10.0, false, BooleanMode::New, "Extrude"); + REQUIRE(doc.recompute()); + + int top_face = find_face_by_normal(doc, 0, Vec3d(0, 0, 1)); + REQUIRE(top_face >= 0); + + int cs = doc.add_coordsys(CoordSysType::FaceAndDirection, Vec3d(0, 0, 0), "CS"); + doc.features[cs].coordsys_body = 0; + doc.features[cs].coordsys_face = top_face; + REQUIRE(doc.recompute()); + + REQUIRE(doc.features[cs].coordsys_face_kind >= 0); + REQUIRE(doc.features[cs].coordsys_face_edges >= 0); + REQUIRE(doc.mate_conflicts.empty()); +} + +TEST_CASE("a connector whose face index slides onto a different KIND of face is reported", "[CadDocument][mate]") +{ + CadDocument doc; + int sk = doc.add_sketch(SketchShape::Rectangle, SketchPlane::XY(), 20, 20, 10, "Box"); + doc.add_extrude(sk, 10.0, false, BooleanMode::New, "Extrude"); + REQUIRE(doc.recompute()); + + int top_face = find_face_by_normal(doc, 0, Vec3d(0, 0, 1)); + REQUIRE(top_face >= 0); + + int cs = doc.add_coordsys(CoordSysType::FaceAndDirection, Vec3d(0, 0, 0), "CS"); + doc.features[cs].coordsys_body = 0; + doc.features[cs].coordsys_face = top_face; + REQUIRE(doc.recompute()); + REQUIRE(doc.mate_conflicts.empty()); + REQUIRE(doc.features[cs].coordsys_face_kind >= 0); + + // Insert a fillet: the box gains cylindrical faces and the face map is renumbered. + doc.add_fillet(2.0, FaceGroup::All, "Fillet"); + REQUIRE(doc.recompute()); + REQUIRE(doc.error.empty()); + + // Find the cylindrical face the fillet introduced (a different KIND than the recorded + // planar fingerprint), then force the drift by pointing coordsys_face at it. + int cyl_face = -1; + auto faces = GeometryEngine::faces_of(doc.bodies[0].shape); + for (int i = 0; i < int(faces.size()); ++i) { + if (GeometryEngine::cylinder_of_face(faces[i]).ok) { cyl_face = i; break; } + } + REQUIRE(cyl_face >= 0); + + doc.features[cs].coordsys_face = cyl_face; + const bool ok = doc.recompute(); + REQUIRE(ok); // non-fatality is the contract under test + + bool reported = false; + for (const auto& c : doc.mate_conflicts) + if (c.first == cs) { reported = true; break; } + REQUIRE(reported); +} + +TEST_CASE("a resized body does not report drift", "[CadDocument][mate]") +{ + CadDocument doc; + int sk = doc.add_sketch(SketchShape::Rectangle, SketchPlane::XY(), 20, 20, 10, "Box"); + doc.add_extrude(sk, 10.0, false, BooleanMode::New, "Extrude"); + REQUIRE(doc.recompute()); + + int top_face = find_face_by_normal(doc, 0, Vec3d(0, 0, 1)); + REQUIRE(top_face >= 0); + + int cs = doc.add_coordsys(CoordSysType::FaceAndDirection, Vec3d(0, 0, 0), "CS"); + doc.features[cs].coordsys_body = 0; + doc.features[cs].coordsys_face = top_face; + REQUIRE(doc.recompute()); + REQUIRE(doc.mate_conflicts.empty()); + + // Resize upstream: same face, same kind, same edge count — only its dimensions changed. + // This is a legitimate parametric edit and must not raise the drift warning. + for (CadFeature& f : doc.features) + if (f.type == CadFeatureType::Extrude) f.distance = 25.0; + REQUIRE(doc.recompute()); + REQUIRE(doc.error.empty()); + REQUIRE(doc.mate_conflicts.empty()); +}