From 96f9261425f970d0cbcd7bcf6a54f5fef9ed8e64 Mon Sep 17 00:00:00 2001 From: Tommaso Bianchi Date: Sun, 23 Aug 2026 06:14:48 +0200 Subject: [PATCH] The offer table is generated again, and its last verb was unreachable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit snaporca-ziam said gen_offer_table.py would silently delete the model-mode "Constrain sketch" row, because that row lived in the generated header and not in tool_atlas.json. Running it found more than that: FOUR rows existed only in the header — constrain, rename, and the three typed- value rows sk_length / sk_radius / sk_angdist — and sk_delete's action had drifted, pointing the sketch row at btn:delete, the FEATURE delete. All five are now in the atlas, so the header regenerates byte-identically from it. Verbs may carry a `note`, emitted as a C++ comment above the row: a rationale written into a generated file is deleted by the next regeneration, which is how this started. snaporca-z8rs (P1), found by making that true: after the atlas held all 92 verbs, the regenerated header differed from the checked-in one by EXACTLY ONE LINE — kOfferVerbCount, 91 against 92. Every consumer loops i < kOfferVerbCount, so the last row of the table was invisible: never listed by show_offer_menu, never findable by mcp_run_verb. The verb that fell off the end is sk_angdist, "Angle / distance…" — the typed-value row for a two-entity selection. On the one selection where you would ask for the angle between two lines, the row that types it was not in the menu. It survived because nothing compared the Sk2Ent menu against the table: sk_angdist accepts Sk2Ent and nothing else, so an off-by-one that dropped the LAST verb was invisible from every other selection. The vocabulary rung now covers Sk2Ent too, and picking the pair taught it one more rig fact — shift-clicking a circle at its +X point grabs the RADIUS GRIP, which replaces the selection with that one entity, so the pair silently collapsed to one and the offer answered SkLine. Correctly, for the selection that actually existed. gen_offer_table.py --check proves header == atlas and changes nothing; it is now the first step of scripts/ladder-all.sh, and the only one that needs no rig. Offer ladder 107/107, gesture ladder 93/93, both on the rig. snaporca-ziam snaporca-z8rs Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01MrMzTpAf78U4NG2M8jfvHY --- docs/ux/mockups/gen_offer_table.py | 28 +++++++- docs/ux/tool_atlas.json | 106 ++++++++++++++++++++++++++++- scripts/ladder-all.sh | 6 ++ scripts/offer-ladder.py | 21 +++++- src/slic3r/GUI/CAD/DesignOffer.hpp | 2 +- 5 files changed, 157 insertions(+), 6 deletions(-) diff --git a/docs/ux/mockups/gen_offer_table.py b/docs/ux/mockups/gen_offer_table.py index 4dc557b627..e56c626a3e 100644 --- a/docs/ux/mockups/gen_offer_table.py +++ b/docs/ux/mockups/gen_offer_table.py @@ -12,6 +12,7 @@ Output: src/slic3r/GUI/CAD/DesignOffer.hpp, checked in and never hand-edited. import json import os +import sys HERE = os.path.dirname(os.path.abspath(__file__)) UX = os.path.dirname(HERE) @@ -112,6 +113,13 @@ def main(): blind = [v["id"] for v in A["verbs"] if v.get("action") and not v.get("hint")] assert not blind, f"wired verbs with no hint: {blind}" for v in A["verbs"]: + # A verb may carry a NOTE: the reason it exists, emitted as a C++ comment above its row. + # Without somewhere to put it, a rationale written into the generated header is deleted by + # the next regeneration — which is how the model-mode "Constrain sketch" row came to exist + # in the header and not in the atlas at all (snaporca-ziam). The map exists once; so does + # the explanation. + for ln in ([v["note"]] if isinstance(v.get("note"), str) else v.get("note") or []): + lines.append(f" // {ln}") mask = 0 for a in v["accepts"]: mask |= 1 << sels.index(a) @@ -133,12 +141,28 @@ def main(): "#endif // slic3r_GUI_DesignOffer_hpp_", "", ] + text = "\n".join(lines) + # --check: prove the checked-in header IS what this generator produces, and change nothing. + # The header calls itself GENERATED and was hand-edited anyway; a claim like that is only + # worth having if something enforces it, so ladder-all.sh runs this on every gate. + if "--check" in sys.argv: + have = open(OUT, encoding="utf-8").read() if os.path.exists(OUT) else "" + if have == text: + print(f"{os.path.relpath(OUT, REPO)} matches tool_atlas.json") + return 0 + import difflib + d = list(difflib.unified_diff(have.splitlines(), text.splitlines(), + "checked-in", "generated", lineterm="", n=1)) + print(f"{os.path.relpath(OUT, REPO)} DIFFERS from tool_atlas.json:") + print("\n".join(d[:60])) + return 1 with open(OUT, "w", encoding="utf-8") as f: - f.write("\n".join(lines)) + f.write(text) wired = sum(1 for v in A["verbs"] if v.get("action")) print(f"wrote {os.path.relpath(OUT, REPO)}: {len(A['verbs'])} verbs, " f"{len(slots)} rows, {wired} wired to existing actions") + return 0 if __name__ == "__main__": - main() + sys.exit(main()) diff --git a/docs/ux/tool_atlas.json b/docs/ux/tool_atlas.json index 177775b336..aaba37097f 100644 --- a/docs/ux/tool_atlas.json +++ b/docs/ux/tool_atlas.json @@ -1040,6 +1040,24 @@ "icon": "design_edit", "hint": "Reopen the selected feature to change what it was made from" }, + { + "id": "rename", + "name": "Rename…", + "slot": "modify", + "key": "F2", + "feature": "Tree", + "mcp": null, + "mode": "model", + "accepts": [ + "sk_loop" + ], + "needs": {}, + "refusal": "Select a feature to rename it", + "gui": true, + "action": "btn:rename", + "icon": null, + "hint": "Give this feature a name you will recognise in the tree" + }, { "id": "delete_face", "name": "Delete Face", @@ -2026,6 +2044,31 @@ "icon": "design_constrain", "hint": "Constrain the selected sketch entities to each other" }, + { + "id": "constrain", + "name": "Constrain sketch", + "slot": "modify", + "key": null, + "feature": "Sketch", + "mcp": null, + "mode": "model", + "accepts": [ + "sk_loop" + ], + "needs": { + "sketches": 1 + }, + "refusal": "Select a sketch to constrain it", + "gui": true, + "action": "btn:constrain", + "icon": "design_constrain", + "hint": "Add dimensions and relations (coincident, tangent, parallel...) to the selected sketch", + "note": [ + "Same verb, model-mode vocabulary: offered when a SKETCH is selected (bit 14, SkLoop), the", + "state a user is in right after finishing one. Without this row the only way in was the", + "toolbar icon, and constraints read as absent — see the Onshape-comparison report." + ] + }, { "id": "sk_construct", "name": "Construction", @@ -2084,9 +2127,70 @@ "needs": {}, "refusal": null, "gui": true, - "action": "btn:delete", + "action": "btn:sk_delete", "icon": "design_delete", "hint": "Delete the selected sketch entities" + }, + { + "id": "sk_length", + "name": "Length…", + "slot": "modify", + "key": "V", + "feature": "Sketch", + "mcp": null, + "mode": "sketch", + "accepts": [ + "sk_line" + ], + "needs": {}, + "refusal": null, + "gui": true, + "action": "key:V", + "icon": "design_dimension", + "hint": "Type the length of this line", + "note": [ + "Typing the defining number of the element you pointed at. Three rows rather than one so", + "each names the quantity in the drawing-office word for THAT element; all three land on", + "the same handler, because dimension_kind() already resolves the quantity from the", + "selection. Without these, an element's own numbers were reachable only by arming the", + "Dimension tool and re-picking geometry that was already selected." + ] + }, + { + "id": "sk_radius", + "name": "Radius / diameter…", + "slot": "modify", + "key": "V", + "feature": "Sketch", + "mcp": null, + "mode": "sketch", + "accepts": [ + "sk_arc" + ], + "needs": {}, + "refusal": null, + "gui": true, + "action": "key:V", + "icon": "design_dimension", + "hint": "Type the radius of this arc, or the diameter of this circle" + }, + { + "id": "sk_angdist", + "name": "Angle / distance…", + "slot": "modify", + "key": "V", + "feature": "Sketch", + "mcp": null, + "mode": "sketch", + "accepts": [ + "sk_2ent" + ], + "needs": {}, + "refusal": null, + "gui": true, + "action": "key:V", + "icon": "design_dimension", + "hint": "Type the angle between two lines, or the distance between the two picks" } ], "chrome_only": { diff --git a/scripts/ladder-all.sh b/scripts/ladder-all.sh index cb9c78ee63..ccd39ae592 100755 --- a/scripts/ladder-all.sh +++ b/scripts/ladder-all.sh @@ -34,6 +34,12 @@ run_in_rig() { # copy the script in fresh, then run it ther docker exec "$C" python3 "$@" } +# FIRST, and it needs no rig: the offer table the menu is compiled from must be what the atlas +# says. The header calls itself GENERATED and had been hand-edited anyway — which cost four rows +# that existed only in the header, one row wired to the wrong action, and a count of 91 for a +# 92-row array, so the last verb was unreachable (snaporca-z8rs, snaporca-ziam). +step "offer table matches the atlas" python3 docs/ux/mockups/gen_offer_table.py --check + step "kernel suite" scripts/kernel-test.sh --vol "${KVOL:-snaporca_kerneltest}" if [ -z "${SKIP_GUI:-}" ]; then diff --git a/scripts/offer-ladder.py b/scripts/offer-ladder.py index 0c6a767498..8ba6eee04e 100644 --- a/scripts/offer-ladder.py +++ b/scripts/offer-ladder.py @@ -399,9 +399,26 @@ def rung_vocabulary(): where = [("SkNone", cx, y1 - (y1 - y0) * 0.12), ("SkLine", (ax + bx) / 2.0, ay), ("SkArc", circ["center"][0] + circ["radius"], circ["center"][1]), - ("SkPoint", pxx, pyy)] + ("SkPoint", pxx, pyy), + ("Sk2Ent", None, None)] seen = {} for name, X, Y in where: + if name == "Sk2Ent": + # The two-entity vocabulary was the one selection nothing compared against the table, + # and it is where the missing row hid: sk_angdist accepts Sk2Ent and nothing else, so + # an off-by-one that dropped the LAST verb was invisible from every other selection. + G.key("Escape", 0.5) + G.clickmm((ax + bx) / 2.0, ay) + G.xdo("keydown shift") + # The TOP of the circle, not its +X point: the radius grip lives there, and a click on + # a grip arms a handle drag which REPLACES the selection with that one entity. The + # pick then silently collapses to one and the offer answers SkLine — right, for the + # selection that actually existed. + G.clickmm(circ["center"][0], circ["center"][1] + circ["radius"]) + G.xdo("keyup shift") + picked = len(G.describe()["selection"]) + G.check("OFFER", picked == 2, f"two entities picked for the pair vocabulary: {picked}") + X, Y = (ax + bx) / 2.0, ay o = open_offer(X, Y) want = sorted(predicted(o.kind, o.sketching)) got = sorted(o.verbs) @@ -414,7 +431,7 @@ def rung_vocabulary(): # And the sets are genuinely DIFFERENT — an offer that adapts is not one that always shows # the same rows. Without this, four identical menus would have passed four checks. G.check("OFFER", len(set(map(frozenset, seen.values()))) == len(seen), - "all four selections offer a different set: " + "every selection offers a different set: " + ", ".join(f"{k}={len(v)}" for k, v in seen.items())) G.check("OFFER", seen["SkLine"] - seen["SkNone"], f"a picked line adds {len(seen['SkLine'] - seen['SkNone'])} verbs an empty pick has not: " diff --git a/src/slic3r/GUI/CAD/DesignOffer.hpp b/src/slic3r/GUI/CAD/DesignOffer.hpp index 82ef068291..bafd7f24cc 100644 --- a/src/slic3r/GUI/CAD/DesignOffer.hpp +++ b/src/slic3r/GUI/CAD/DesignOffer.hpp @@ -182,7 +182,7 @@ static const OfferVerb kOfferVerbs[] = { {"sk_radius", "Radius / diameter…", 7, "V", "key:V", nullptr, 0x00020000u, 0, 0, false, true, nullptr, "design_dimension", "Type the radius of this arc, or the diameter of this circle"}, {"sk_angdist", "Angle / distance…", 7, "V", "key:V", nullptr, 0x00080000u, 0, 0, false, true, nullptr, "design_dimension", "Type the angle between two lines, or the distance between the two picks"}, }; -static const int kOfferVerbCount = 91; +static const int kOfferVerbCount = 92; }} // namespace Slic3r::GUI