From 44fde20fdcdbfe0950262f616a80a6402d8186b3 Mon Sep 17 00:00:00 2001 From: ylzha Date: Sun, 31 May 2026 01:12:16 -0400 Subject: [PATCH] Add geometry-loop + Move advisories to validation Two advisory-only validation checks (never affect pass/fail), closing gaps surfaced building the Moravian workbench: - Geometry-loop advisory (helpers/sp/deps.py): AST-scans the tracked build script for for/while loops that create geometry (sketches/extrudes/combines/ cyl_/box_) and nudges toward a Rectangular Pattern or Mirror. This is the source-level complement to validate_design's body-congruence replication advisory, which can't see a loop of identical CUTs (e.g. dog-hole arrays remove voids, not add bodies). Resolves the script path up-front so it runs even when an explicit metadata_path is passed. - Move advisory (addin/tools/validate_design.py): flags Move/rotate features that likely orient a part to an angled reference; prefer building in place (or a two-section loft for compound splay). Opt out a legit roll / measured alignment with a `_norep` substring in the feature name. - docs/angled-construction.md: documents the parametric two-section Loft as the Move-free way to do compound splay (and why a Loft, not an off-axis extrude), plus the "check_interference after a Move" reflex. - tests/test_deps_loops.py: offline unit test for the loop detector. Co-Authored-By: Claude Opus 4.8 (1M context) --- addin/tools/validate_design.py | 62 +++++++++++++++++++++++- docs/angled-construction.md | 18 ++++++- helpers/sp/deps.py | 86 +++++++++++++++++++++++++++++++--- tests/test_deps_loops.py | 85 +++++++++++++++++++++++++++++++++ 4 files changed, 242 insertions(+), 9 deletions(-) create mode 100644 tests/test_deps_loops.py diff --git a/addin/tools/validate_design.py b/addin/tools/validate_design.py index 7626646..95c94f6 100644 --- a/addin/tools/validate_design.py +++ b/addin/tools/validate_design.py @@ -335,6 +335,37 @@ def _check_replication(design, bodies, exclude_prefixes, min_group=2): return {"advisory": True, "groups": findings} +def _check_moves(design): + """ADVISORY (never affects pass/fail): Move/rotate features in the timeline. + + A Move that *orients a part to an angled reference* (the common case — splay, + tilt) bakes a non-parametric transform and can mask sizing bugs that only show + once the part is in place. Prefer building the part in position: sketch its + outline against the reference, or — for a compound angle — loft between two + offset cross-sections (parametric, no Move). Legit uses remain (a cosmetic + roll about a part's own axis, aligning to a measured orientation); suppress + those by putting `_norep` in the Move feature's name. + """ + moves = [] + try: + tl = design.timeline + for i in range(tl.count): + try: + feat = tl.item(i).entity + ot = getattr(feat, "objectType", "") or "" + if "MoveFeature" not in ot: + continue + nm = getattr(feat, "name", "") or "" + if "_norep" in nm: + continue + moves.append(nm or "(unnamed Move)") + except Exception: + pass + except Exception: + pass + return {"advisory": True, "count": len(moves), "names": moves} + + def handler(exclude_prefixes: list = None, min_contact_cm2: float = None) -> dict: """Run all structural validation checks on the current design.""" @@ -389,6 +420,15 @@ def handler(exclude_prefixes: list = None, min_contact_cm2: float = None) -> dic except Exception as re: replication = {"advisory": True, "error": str(re), "groups": []} + # Move advisory — Move/rotate features that likely orient a part to an + # angled reference. ADVISORY ONLY: never changes `passed`. Prefer building + # in place (or loft for compound angles); suppress legit rolls with _norep. + moves = None + try: + moves = _check_moves(design) + except Exception as me: + moves = {"advisory": True, "error": str(me), "names": []} + import json result = { "passed": passed, @@ -399,6 +439,8 @@ def handler(exclude_prefixes: list = None, min_contact_cm2: float = None) -> dic result["deps"] = deps_result if replication is not None and replication.get("groups"): result["replication"] = replication + if moves is not None and moves.get("names"): + result["moves"] = moves parts = [] if connectivity["connected"] and not connectivity["weakConnections"]: @@ -427,6 +469,13 @@ def handler(exclude_prefixes: list = None, min_contact_cm2: float = None) -> dic f"replication ADVISORY ({len(g)} group(s) built independently — " f"{ex}) [does not affect pass/fail]") + if moves is not None and moves.get("names"): + parts.append( + f"move ADVISORY ({moves['count']} Move feature(s): " + f"{', '.join(moves['names'][:4])} — if orienting to an angled " + f"reference, prefer build-in-place/loft; name a legit roll " + f"`_norep`) [does not affect pass/fail]") + status = "PASSED" if passed else "FAILED" msg = f"{status}: {', '.join(parts)}" @@ -470,9 +519,20 @@ def handler(exclude_prefixes: list = None, min_contact_cm2: float = None) -> dic a redo; the recommended response is to refactor the named group to one template + Mirror/Pattern (a sub-agent can do this so the main build's context stays lean). +5. **Move advisory** (ADVISORY — never affects pass/fail): Move/rotate features + in the timeline. A Move that orients a part to an angled reference bakes a + non-parametric transform and can hide sizing bugs until the part is in place — + prefer building the part in position (sketch its outline against the reference; + for a compound angle, loft between two offset cross-sections). Legit uses + (a cosmetic roll about a part's own axis, aligning to a measured face) are + suppressed by putting `_norep` in the Move feature's name. + +Note: the geometry-loop advisory (a Python loop that creates geometry instead of +a Rectangular Pattern/Mirror feature) is emitted by the dependency-tree check and +appears in the deps output. Returns a single pass/fail result. Fails if disconnected, has weak -connections, or has interference (NOT for the replication advisory). +connections, or has interference (NOT for the replication/move/loop advisories). Run after EVERY phase.""" tool = Tool.create_simple( diff --git a/docs/angled-construction.md b/docs/angled-construction.md index ccf0431..a9db5f0 100644 --- a/docs/angled-construction.md +++ b/docs/angled-construction.md @@ -36,7 +36,21 @@ Build compound splay in two steps: 2. **Secondary splay (along width)** — Apply a Move feature that rotates the leg body around the X axis by `splay_w`. This avoids compound-angle sketch math and keeps each splay axis independent. -**Why not a single compound-angle sketch?** The sketch would need trigonometric expressions for foreshortened dimensions, and the extrude direction wouldn't align with any principal axis. Two-step splay is simpler, more readable, and each angle is independently adjustable. +**Why not a single compound-angle sketch?** A *single* compound-angle sketch would need trigonometric expressions for foreshortened dimensions, and the extrude direction wouldn't align with any principal axis. + +### Parametric Alternative: Loft Between Two Sections (no Move) + +The trapezoid + Move above bakes the second angle as a non-parametric transform. To keep compound splay **fully parametric and Move-free**, use **two sketches + a Loft** instead: + +1. Sketch the leg's cross-section (a plain, true-size rectangle) on an XY plane at the **top** (seat) height. +2. Sketch the same rectangle on an XY plane at the **bottom** (floor), its center offset by `splay_shift` in X **and** `splay_shift_w` in Y. +3. **Loft** between the two profiles. + +Both sections are axis-aligned rectangles at true size — **no foreshortening trig** — and the loft produces the doubly-leaning leg (an oblique prism with planar faces and horizontal top/bottom). The offsets are ordinary `tan()`-based parameter expressions, so the leg recomputes when a splay angle changes. + +**Why a Loft and not an Extrude?** Fusion's Extrude only goes *normal to the profile plane* (the taper angle adds draft, it does not redirect the extrusion axis) — there is no off-axis extrude. Two separated profiles can therefore only be bridged by a Loft (profile→profile) or a Sweep along an angled path. A Loft between world-aligned rectangles also gives the geometry you actually want: **horizontal top/bottom faces** (flat on the floor and into the seat) with the sides leaning — whereas tilting a sketch plane for a normal extrude would yield faces *perpendicular to the leg axis* (angled cuts, wrong for floor/seat contact). + +Note that **single-axis splay needs neither** — the trapezoid/parallelogram sketch (lean *in* the sketch) extruded normally in the perpendicular direction already captures it. Reach for Loft (or, as the legacy option, the Move below) only for the *second* axis. `validate_design` emits a (non-failing) **move advisory** when a Move is present, to nudge toward this in-place alternative; if a Move is genuinely the right tool (a cosmetic roll about a part's own axis, or aligning to a measured orientation), name the feature with a `_norep` substring to suppress it. ### Trapezoid Sketch (Primary Splay) @@ -464,7 +478,7 @@ matrix = [ The Move feature's matrix is baked at script time — it doesn't update when parameters change. If `splay_w` changes in Change Parameters, the Move angle stays the same. -**Mitigation:** For furniture models, splay angles are rarely adjusted after the initial design. If parametric splay is required, use a component-level rotation (via occurrence transform) instead of a Move feature, but this adds complexity to cross-component CUT operations. +**Mitigation:** For furniture models, splay angles are rarely adjusted after the initial design. If parametric splay IS required, prefer building the part in place — for compound splay, the **two-section Loft** above (parametric, no Move); see "Parametric Alternative: Loft Between Two Sections." (A component-level rotation via occurrence transform also stays parametric but complicates cross-component CUTs.) A Move also *hides* sizing bugs until the part lands in position, so after any Move, run `check_interference` and sanity-check the part's dimensions in its final spot. ### Re-Find Bodies After Move diff --git a/helpers/sp/deps.py b/helpers/sp/deps.py index 30674c9..6b4e9ea 100644 --- a/helpers/sp/deps.py +++ b/helpers/sp/deps.py @@ -218,6 +218,68 @@ def _check_sketch_anchoring(comp, comp_name, is_root_comp, issues): # else: only spline interiors are free → legitimate sculpted profile. +def _geometry_loop_findings(script_path): + """ADVISORY: scan the tracked build script for Python ``for``/``while`` loops + that CREATE geometry (sketches, extrudes, combines, cyl_/box_ helpers) — the + non-parametric replication anti-pattern. The right way is a Rectangular + Pattern or Mirror feature (one parametric feature that recomputes when the + count changes), not N independent features from a loop. + + This is the script-source complement to validate_design's body-congruence + replication advisory: that one only sees congruent BODIES, so a loop of + identical CUTS (dog holes, mortise/hole arrays — which remove voids, not add + bodies) is invisible to it. The AST sees both. + + Returns a list of {line, kind, calls}. Best-effort; never raises. A loop that + builds a list and then patterns/mirrors it (calls a *pattern*/*mirror* helper) + is NOT flagged. + """ + import ast + import os + try: + if not script_path or not os.path.exists(script_path): + return [] + with open(script_path, "r") as f: + tree = ast.parse(f.read()) + except Exception: + return [] + + # call-name fragments (lower-cased, dotted) that create per-instance geometry + GEO = ("sketches.add", "extrudefeatures", "revolvefeatures", "loftfeatures", + "sweepfeatures", "ext_new", "ext_op", "ext_new_sym", "combine", + "cyl_", "box_", "addbytwopoints", "addbycenterradius", "sketchcircles", + "sketchlines", "sketcharcs") + # if the loop itself patterns/mirrors, it's already the right approach + OK = ("pattern", "mirror") + + def _dotted(fn): + if isinstance(fn, ast.Attribute): + base = _dotted(fn.value) + return (base + "." + fn.attr) if base else fn.attr + if isinstance(fn, ast.Name): + return fn.id + return None + + findings = [] + for node in ast.walk(tree): + if not isinstance(node, (ast.For, ast.While)): + continue + calls = [] + for n in ast.walk(node): + if isinstance(n, ast.Call): + d = _dotted(n.func) + if d: + calls.append(d.lower()) + if any(o in c for c in calls for o in OK): + continue + hits = sorted({g for c in calls for g in GEO if g in c}) + if hits: + findings.append({"line": node.lineno, + "kind": "for" if isinstance(node, ast.For) else "while", + "calls": hits}) + return findings + + def validate_deps(ctx, metadata_path=None): """Validate dependency tree from model.json. @@ -235,13 +297,25 @@ def validate_deps(ctx, metadata_path=None): import os import re + # Resolve the tracked build script up-front — used both to locate model.json + # (when no metadata_path was passed) AND for the geometry-loop advisory below, + # which must run even when an explicit metadata_path was given. + script_path = None + try: + from server.document_tracker import DocumentTracker + script_path = DocumentTracker._script_path + except Exception: + pass + + # Geometry-loop advisory (NOTE — never affects pass/fail): a Python loop that + # creates geometry should be a Rectangular Pattern or Mirror feature instead. + for fnd in _geometry_loop_findings(script_path): + print(f" NOTE geometry built in a `{fnd['kind']}` loop at line " + f"{fnd['line']} ({', '.join(fnd['calls'])}) — use a Rectangular " + f"Pattern or Mirror feature so the count stays parametric, not a " + f"Python loop. (advisory; does not affect pass/fail)") + if metadata_path is None: - script_path = None - try: - from server.document_tracker import DocumentTracker - script_path = DocumentTracker._script_path - except Exception: - pass if script_path: script_dir = os.path.dirname(script_path) stem = os.path.splitext(os.path.basename(script_path))[0] diff --git a/tests/test_deps_loops.py b/tests/test_deps_loops.py new file mode 100644 index 0000000..03b3eaa --- /dev/null +++ b/tests/test_deps_loops.py @@ -0,0 +1,85 @@ +"""Offline test of deps.py geometry-loop advisory. Pure Python — no Fusion. + +Run: python3 tests/test_deps_loops.py (also pytest-discoverable) + +Exercises _geometry_loop_findings: a Python for/while loop that CREATES geometry +should be flagged (use a Rectangular Pattern / Mirror instead); a loop that +patterns/mirrors, or does no geometry, should not be. +""" +import os, sys, types, tempfile, importlib.util + +for m in ("adsk", "adsk.core", "adsk.fusion"): + sys.modules[m] = types.ModuleType(m) + +_DEPS = os.path.normpath(os.path.join( + os.path.dirname(os.path.abspath(__file__)), "..", "helpers", "sp", "deps.py")) +spec = importlib.util.spec_from_file_location("deps", _DEPS) +deps = importlib.util.module_from_spec(spec) +spec.loader.exec_module(deps) + + +def _find(src): + with tempfile.NamedTemporaryFile("w", suffix=".py", delete=False) as f: + f.write(src); path = f.name + try: + return deps._geometry_loop_findings(path) + finally: + os.unlink(path) + + +CASES = [ + # (name, source, expect_flagged) + ("for-loop of CUTs (dog holes)", """ +def run(ctx): + for j in range(6): + sp.ext_op(top_c, prof, "t", CUT, top_front, "Dog_%d" % j, flip=False) +""", True), + ("while-loop creating sketches", """ +def run(ctx): + j = 0 + while j < n: + sk = top_c.sketches.add(pl) + j += 1 +""", True), + ("for-loop of cyl_ helper", """ +def run(ctx): + for k in range(4): + cyl_y(cx, z, ya, dia, ylen, "Hole_%d" % k) +""", True), + ("loop that patterns — NOT flagged", """ +def run(ctx): + bodies = [] + for k in range(n): + bodies.append(make_template(k)) + sp.body_pattern(comp, seed, axis, "n", "sp", "Pat") +""", False), + ("loop with no geometry — NOT flagged", """ +def run(ctx): + for b in comp.bRepBodies: + b.name = classify(b) +""", False), + ("syntax error / missing — no crash", "def run(ctx): this is not python", False), +] + + +def test_geometry_loop_findings(): + ok = True + for name, src, expect in CASES: + flagged = len(_find(src)) > 0 + passed = (flagged == expect) + ok = ok and passed + print(f" {'PASS' if passed else 'FAIL'} {name} " + f"(flagged={flagged}, expect={expect})") + assert deps._geometry_loop_findings(None) == [] # None path → no crash + assert deps._geometry_loop_findings("/no/such.py") == [] + assert ok, "geometry-loop detector cases failed" + + +if __name__ == "__main__": + try: + test_geometry_loop_findings() + print("\nall geometry-loop cases passed") + sys.exit(0) + except AssertionError as e: + print(f"\nFAILED: {e}") + sys.exit(1)