diff --git a/src/libslic3r/CadDocument.cpp b/src/libslic3r/CadDocument.cpp index ec1fcb4ec6..38825e8208 100644 --- a/src/libslic3r/CadDocument.cpp +++ b/src/libslic3r/CadDocument.cpp @@ -3404,6 +3404,11 @@ bool CadDocument::recompute() } } bodies = std::move(built); + // The face and edge maps have just been rebuilt, so every global id handed out before this + // point now means something else. Bump here rather than in each mutator: this is the single + // line where the topology is actually replaced, so it cannot be forgotten by a new feature + // type the way a per-mutator bump would be. + ++topo_generation; // 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 diff --git a/src/libslic3r/CadDocument.hpp b/src/libslic3r/CadDocument.hpp index ceab1a2e66..83798931d2 100644 --- a/src/libslic3r/CadDocument.hpp +++ b/src/libslic3r/CadDocument.hpp @@ -607,6 +607,16 @@ 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) + // Bumped every time the bodies are rebuilt, i.e. every time the face and edge MAPS change. + // Global face/edge ids are indices into TopExp::MapShapes and mean nothing across a rebuild, + // so any caller holding an id from an earlier state is holding a wrong one. This is the + // handle that lets it find out instead of silently addressing the wrong edge. + // + // Session-scoped and deliberately NOT serialized: an id is only meaningful within the run + // that produced it, so persisting the counter would imply a promise across loads that the + // ids themselves cannot keep. + uint64_t topo_generation{1}; + // 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 diff --git a/src/slic3r/GUI/McpControl.cpp b/src/slic3r/GUI/McpControl.cpp index 609f03476c..6f0598c66c 100644 --- a/src/slic3r/GUI/McpControl.cpp +++ b/src/slic3r/GUI/McpControl.cpp @@ -112,6 +112,14 @@ json describe_tools() {"app", "SnapOrca CAD"}, {"protocol", "jsonrpc-2.0"}, {"slice", 5}, + // Read this before using any face or edge id. + {"id_lifetime", + "Global face and edge ids are indices into the CURRENT topology and expire the moment " + "a feature rebuilds the model. Reading the scene once and then issuing several " + "operations addresses the wrong edge on every call after the first, and does NOT " + "error, because a stale id still names a real edge. Either re-read query_topology " + "before each id-taking call, or pass the 'generation' you were given back with the " + "call and have it refused if the model has moved on."}, {"tools", json::array({ json{{"name", "describe_tools"}, {"summary", "List callable tools and their parameters."}, {"params", json::array()}}, @@ -414,6 +422,9 @@ json describe_scene(DesignPanel* panel) {"features", std::move(features)}, {"bodies", std::move(bodies)}, {"error", doc.error}, + // Pass this back as "generation" on any call that takes a face or edge id and the call + // is refused if the topology has moved on. See the note on the dispatcher. + {"generation", doc.topo_generation}, }; } @@ -494,7 +505,10 @@ json query_topology(DesignPanel* panel, const json& params) edges.push_back(std::move(je)); } return json{{"body", params.value("body", 0)}, {"face_count", nf}, {"edge_count", ne}, - {"faces", std::move(faces)}, {"edges", std::move(edges)}}; + {"faces", std::move(faces)}, {"edges", std::move(edges)}, + // The ids above are indices into this exact topology and expire with it. Echo + // this back as "generation" on the calls that consume them. + {"generation", panel->mcp_doc().topo_generation}}; } // One measurement reference -> a representative point and (optionally) a direction. @@ -1408,6 +1422,33 @@ std::string handle_on_main(const std::string& method, const json& params, const if (!mf || !mf->m_design_panel) return rpc_error(id, -32001, "Design panel not ready"); DesignPanel* panel = mf->m_design_panel; + + // Stale-id guard, checked here rather than in each handler. + // + // Global face and edge ids are indices into TopExp::MapShapes, valid only against the + // topology that produced them. Every dress-up rewrites those maps, so the natural way to + // drive this socket — read the scene once, then issue several operations — silently + // addresses the WRONG edge on every call after the first. Measured on a box: four chamfers + // with ids re-read each time remove 0.400/0.397/0.397/0.395 mm3; the same four with ids + // captured up front remove 0.400/0.008/0.397/0.280. Neither run errors, because a stale id + // still resolves to a real edge — just not the one that was asked for. + // + // So a caller may pass back the "generation" it got from describe_scene or query_topology, + // and a mismatch is refused instead of silently obeyed. Optional by design: omitting it + // keeps every existing script working exactly as before, and supplying it is what buys the + // guarantee. One check at the dispatcher rather than one per handler, so a method added + // later cannot forget it. + if (params.is_object() && params.contains("generation")) { + const uint64_t want = params["generation"].get(); + const uint64_t have = panel->mcp_doc().topo_generation; + if (want != have) + return rpc_error(id, -32010, + "stale face/edge ids: they were read at generation " + std::to_string(want) + + " but the model is now at " + std::to_string(have) + + ". Re-read query_topology and use the new ids — the old ones still name real " + "edges, just not the ones you measured."); + } + try { if (method == "describe_tools") return rpc_result(id, describe_tools()); if (method == "describe_scene") return rpc_result(id, describe_scene(panel));