Connector face drift: warn without crying wolf — and bump the recipe version

kqih option (c). A FaceAndDirection connector stores a global face index, and an
upstream edit can renumber faces so the index silently names a different one. The
DANGLING case already threw; this is the in-range-but-wrong case, which nothing
detected.

Fingerprint the face on first resolve, compare afterwards, and report a mismatch
into mate_conflicts — the channel that already marks the tree row — never as an
error. A drift warning must not abort the recompute, because the alternative
makes a legitimate Draft on a mated face fatal.

WHAT THE FINGERPRINT IS, AND WHAT IT IS NOT. Surface type plus edge count. Not
centroid or area: legitimate parametric edits move and resize faces, which is the
entire point of the model, so either would fire on every dimension change. Not
the normal, which is the tempting one — Draft deliberately tilts a face and
Transform reorients a body, both legitimate. Type and edge count survive rigid
motion, tilting and resizing, and catch the case that actually happens: a planar
index sliding onto a fillet's cylindrical face after a dress-up inserts faces.
The accepted cost is that a slide between two planar 4-edge faces is invisible. A
partial detector that never cries wolf beats a total one that does, because a
false alarm on a valid connector teaches people to ignore the warning.

Connectors with no fingerprint record one on first recompute, so old recipes
self-heal and both writers (DesignPanel, McpControl) get it without changing.

THE VERSION BUMP IS THE IMPORTANT HALF. The task was specified with "do not
change the recipe version" — that was wrong, and the rule is written in the
header three lines above the constant: bump whenever save/load gains a field.
deserialize_recipe() gates on v == VERSION and then reads a FLAT symmetric field
list. A v3 blob under a v3 build that has grown two fields passes the gate and
reads two ints past the end of every connector, into the next feature's bytes.
That is silent corruption of a saved project, which is worse than any load error.
Now v4, and v3 gets the existing clean refusal.

cad_recipe_v3.bin is KEPT, unregenerated, with a test asserting it is refused and
that nothing half-read is left behind. It is the only artefact that can prove the
gate works, because it was written by an older build — regenerating it with
today's code would destroy the evidence, which the test says in as many words.

Suite 163 -> 167 cases, 2248 -> 2277 assertions, green. Fixture v4 34928 bytes.
snaporca-kqih.
This commit is contained in:
Tommaso Bianchi
2026-08-12 23:04:49 +02:00
parent 13c702bed3
commit 9125e0b4f8
4 changed files with 166 additions and 5 deletions
+114 -2
View File
@@ -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<std::streamsize>(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<char>(ifs)), std::istreambuf_iterator<char>());
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<char>(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());
}