From 8dae1448e82da5085d643413b7f5c98cbfd6fbc3 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 26 Jul 2026 18:27:02 +0000 Subject: [PATCH 1/3] Fix for() with an empty argument list failing to parse (issue #90) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Parser::parseFor() unconditionally expected an identifier right after '(', so a bare `for()` — valid OpenSCAD, since the argument list is a generic module-call argument list that's allowed to be empty, as seen in upstream's for-tests.scad — took down parsing of the whole file. Now `for()` parses with an empty ForNode::var sentinel (never otherwise possible, since a parsed loop variable is always a non-empty identifier), and CsgEvaluator::evalFor() treats that as zero iterations, matching real OpenSCAD's builtin_for() (checks `!inst->arguments.empty()` and skips the body entirely rather than running it once vacuously). Also resolves the v3.9 oracle-version caveat for linear_extrude's h= and segments= parameters (issue #91) by checking real OpenSCAD's current source directly: h= is a genuine alias for height (ChiselCAD is correct, not ahead of spec), while segments= is genuine but entirely unimplemented in ChiselCAD — filed as a follow-up rather than guessed at blind, documented in docs/roadmap.md. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01Pn9TSaMhpqzA7iSKiQskrr --- docs/roadmap.md | 42 ++++++++++++++++++++++ src/csg/CsgEvaluator.cpp | 9 +++++ src/lang/Parser.cpp | 70 ++++++++++++++++++++---------------- tests/test_csg_evaluator.cpp | 13 +++++++ tests/test_parser.cpp | 15 ++++++++ 5 files changed, 119 insertions(+), 30 deletions(-) diff --git a/docs/roadmap.md b/docs/roadmap.md index 14b9319..2b1e910 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -437,6 +437,48 @@ run through this so far — `2D`, `bugs`, `bugs2D`, `misc`, `issues` are still unexamined. This should become a standing regression suite (rerun after any geometry-affecting change), not a one-time audit. +## v3.10 — Follow-up on the oracle-version caveat (issue #91) + a parser fix (issue #90) + +- [x] **Resolved the v3.9 oracle-version question for `linear_extrude(h=...)` + and `segments=`.** Checked real OpenSCAD's current `master` source + (`src/core/LinearExtrudeNode.cc`) directly rather than the outdated + `apt` 2021.01 binary: `parameters[{"height", "h"}]` confirms `h` genuinely + is a real, current alias for `height` — ChiselCAD's support for it is + correct, not a ChiselCAD invention, and not something to "fix" back to + 2021.01 behavior. `segments=` is also a genuine, distinct current + parameter (`node->segments`, validated separately from `slices`) — but + unlike `h=`, ChiselCAD doesn't implement it **at all** (no `segments` + param is read anywhere in `MeshEvaluator::evalExtrusion`). Per + `src/geometry/linear_extrude.cc`, real OpenSCAD only acts on it when a + twist or non-uniform scale is active: it subdivides each outline edge + (`Discretizer::splitOutline(outline, twist, scale_x, scale_y, num_slices, + segments)`) before extruding, so a twisted/scaled `linear_extrude` gets a + smoother swept surface instead of faceting along the profile's original + edges. This is a real, previously-unidentified gap and likely explains + some fraction of `linear_extrude-tests.scad`'s remaining ~13.6% error — + filed as a follow-up rather than implemented blind here, since matching + `splitOutline`'s exact per-edge subdivision behavior needs bisecting + against a live OpenSCAD build the same way the v3.9 fixes were, not a + guess. +- [x] **`for()` with a completely empty argument list failed to parse** + (part of issue #90's `for-tests.scad`, which opens with exactly this on + line 2: `for();`). `Parser::parseFor()` unconditionally expected an + `Ident '='` right after `(`, so a bare `for()` — valid OpenSCAD, since the + argument list is a generic module-call argument list that's allowed to be + empty — aborted parsing of the entire file. Fixed by skipping + variable/range parsing when `)` immediately follows `(`, leaving + `ForNode::var` empty as a sentinel (never otherwise possible, since a + parsed loop variable is always a non-empty identifier). + `CsgEvaluator::evalFor()` treats that sentinel as zero iterations, + matching real OpenSCAD's `builtin_for()` (`src/core/control.cc`), which + checks `if (!inst->arguments.empty())` and skips the body entirely rather + than running it once vacuously. Regression tests added at both the parser + level (`tests/test_parser.cpp`) and the CSG-evaluator level + (`tests/test_csg_evaluator.cpp`, including a wrapping-transform case). + This resolves one of issue #90's 11 files; the rest (multi-variable + `for`, other transform-argument edge cases) are still open and need the + same per-file bisection. + ## v4 — Tooling & Visual Quality - [ ] VS Code LSP extension (syntax highlighting, error squiggles, completions) diff --git a/src/csg/CsgEvaluator.cpp b/src/csg/CsgEvaluator.cpp index 2ebb623..6b94a8b 100644 --- a/src/csg/CsgEvaluator.cpp +++ b/src/csg/CsgEvaluator.cpp @@ -554,6 +554,15 @@ CsgNodePtr CsgEvaluator::evalIf(const IfNode& node, const glm::mat4& xform, // --------------------------------------------------------------------------- CsgNodePtr CsgEvaluator::evalFor(const ForNode& node, const glm::mat4& xform, const ColorAttr& color) { + // `for()` with no arguments at all (Parser::parseFor leaves node.var + // empty in this case, which is otherwise unreachable since a parsed + // loop variable is always a non-empty identifier). Matches real + // OpenSCAD's builtin_for(), which never instantiates the body when the + // module call's argument list is empty — there's no iteration set for + // zero variables, not a single vacuous pass. + if (node.var.empty()) + return nullptr; + // Build the sequence of iteration values std::vector values; diff --git a/src/lang/Parser.cpp b/src/lang/Parser.cpp index 9d10928..f4e6c96 100644 --- a/src/lang/Parser.cpp +++ b/src/lang/Parser.cpp @@ -591,46 +591,56 @@ AstNodePtr Parser::parseFor() { node.loc = kw.loc; expect(TokenKind::LParen, "expected '(' after 'for'"); - node.var = expect(TokenKind::Ident, "expected loop variable").text; - expect(TokenKind::Equals, "expected '=' after loop variable"); - if (check(TokenKind::LBracket)) { - advance(); // consume '[' + // `for()` with a completely empty argument list is valid OpenSCAD (seen + // e.g. in the upstream test corpus's for-tests.scad) and parses fine — + // it just contributes no geometry, since there's no variable to iterate. + // node.var stays empty, which CsgEvaluator::evalFor treats as its + // zero-arguments/zero-iterations case (matching real OpenSCAD's + // builtin_for(), which skips the body entirely when inst->arguments is + // empty rather than treating it as one vacuous iteration). + if (!check(TokenKind::RParen)) { + node.var = expect(TokenKind::Ident, "expected loop variable").text; + expect(TokenKind::Equals, "expected '=' after loop variable"); - // Parse first expression — determines range vs list form - auto first = parseExpr(); + if (check(TokenKind::LBracket)) { + advance(); // consume '[' + + // Parse first expression — determines range vs list form + auto first = parseExpr(); - if (check(TokenKind::Colon)) { - // Range form: [start : end] or [start : step : end] - advance(); // consume ':' - auto second = parseExpr(); if (check(TokenKind::Colon)) { + // Range form: [start : end] or [start : step : end] advance(); // consume ':' - auto third = parseExpr(); - node.range.isRange = true; - node.range.start = std::move(first); - node.range.step = std::move(second); - node.range.end = std::move(third); + auto second = parseExpr(); + if (check(TokenKind::Colon)) { + advance(); // consume ':' + auto third = parseExpr(); + node.range.isRange = true; + node.range.start = std::move(first); + node.range.step = std::move(second); + node.range.end = std::move(third); + } else { + node.range.isRange = true; + node.range.start = std::move(first); + node.range.end = std::move(second); + } } else { - node.range.isRange = true; - node.range.start = std::move(first); - node.range.end = std::move(second); + // List form: [first, ...] + node.range.isRange = false; + node.range.isBracketedList = true; + node.range.list.push_back(std::move(first)); + while (match(TokenKind::Comma)) { + if (check(TokenKind::RBracket)) break; + node.range.list.push_back(parseExpr()); + } } + expect(TokenKind::RBracket, "expected ']' after range/list"); } else { - // List form: [first, ...] + // Expression form: for (var = expr) — expr must evaluate to a vector node.range.isRange = false; - node.range.isBracketedList = true; - node.range.list.push_back(std::move(first)); - while (match(TokenKind::Comma)) { - if (check(TokenKind::RBracket)) break; - node.range.list.push_back(parseExpr()); - } + node.range.list.push_back(parseExpr()); } - expect(TokenKind::RBracket, "expected ']' after range/list"); - } else { - // Expression form: for (var = expr) — expr must evaluate to a vector - node.range.isRange = false; - node.range.list.push_back(parseExpr()); } expect(TokenKind::RParen, "expected ')' after for header"); diff --git a/tests/test_csg_evaluator.cpp b/tests/test_csg_evaluator.cpp index f5db43c..a32c7b1 100644 --- a/tests/test_csg_evaluator.cpp +++ b/tests/test_csg_evaluator.cpp @@ -706,6 +706,19 @@ TEST_CASE("CsgEval:for empty range yields no geometry", "[csg]") { REQUIRE(s.roots.empty()); } +TEST_CASE("CsgEval:for() with no arguments yields no geometry", "[csg][bugfix]") { + // Matches real OpenSCAD's builtin_for(), which skips the body entirely + // when the for()'s argument list is empty (no variable to iterate means + // no iterations, not one vacuous pass). + auto s = evaluate("for() sphere(r=1);"); + REQUIRE(s.roots.empty()); +} + +TEST_CASE("CsgEval:for() with no arguments under a transform still yields no geometry", "[csg][bugfix]") { + auto s = evaluate("translate([1,0,0]) for() cube(1);"); + REQUIRE(s.roots.empty()); +} + TEST_CASE("CsgEval:for over bracketed point-list literal iterates per-point, not flattened", "[csg]") { // Regression test: a bracketed list literal whose own elements are diff --git a/tests/test_parser.cpp b/tests/test_parser.cpp index 29f881f..4a9a948 100644 --- a/tests/test_parser.cpp +++ b/tests/test_parser.cpp @@ -453,6 +453,21 @@ TEST_CASE("Parser:for with brace body", "[parser]") { REQUIRE(f.children.size() == 2); } +TEST_CASE("Parser:for() with no arguments parses with an empty loop variable", "[parser][bugfix]") { + // Real OpenSCAD accepts a bare for() (seen in its own test corpus, + // for-tests.scad's `for();`) — the argument list is a generic module-call + // argument list, which is legitimately allowed to be empty. Previously + // ChiselCAD's parser unconditionally expected an identifier right after + // '(', so this failed to parse at all and took the whole file down with + // it. node.var stays empty as the sentinel CsgEvaluator::evalFor checks + // for zero-iterations. + auto r = parse("for() sphere(r=1);"); + REQUIRE(r.roots.size() == 1); + auto& f = asFor(r.roots[0]); + REQUIRE(f.var.empty()); + REQUIRE(f.children.size() == 1); +} + // --------------------------------------------------------------------------- // General range-literal expressions (usable outside `for`) // --------------------------------------------------------------------------- From 18f7a549f0b7db864cb4edc8fb3c40765f1da08d Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 26 Jul 2026 20:15:29 +0000 Subject: [PATCH 2/3] Fix all 11 issue #90 parse failures; resolve issue #91's oracle-version question MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Root-caused and fixed every construct in the issue #90 corpus file list that was taking down parsing: - for() with an empty argument list, and multi-variable for(i=..,j=..,k=..) (ForNode restructured to hold a list of clauses, evaluated as nested loops via CsgEvaluator::evalFor so a later clause can reference an earlier one's variable). - rotate(a, v) (angle + arbitrary axis), and rotate()/mirror() with zero arguments — parseTransform previously required exactly one argument for every transform kind. Implemented rotate's angle-axis form via glm::rotate(angle, axis), and gave each transform its own OpenSCAD- documented default for a missing argument instead of one shared {0,0,0}. - scale(2) (a bare scalar) was only scaling the Z axis instead of broadcasting uniformly — a real, silent correctness bug found while fixing the above; confirmed against real OpenSCAD's builtin_scale() and fixed. - union/difference/intersection/hull/minkowski couldn't take any arguments, including minkowski(convexity=...), a real OpenSCAD parameter. - module definitions required a brace-block body; a single-statement body with no braces (a very common one-liner idiom) failed to parse. - a leaf primitive (cube/sphere/.../polygon) followed by a trailing child statement failed to parse. - polygon()'s positional (unnamed) points list was fundamentally broken: parseParamList's generic [x,y,z]-triple decoding silently misrouted a 3-point polygon's points under the wrong keys, and hard failed to parse 4+ points outright (the common case for any real polygon). Also resolves issue #91: checked real OpenSCAD's current source directly and confirmed linear_extrude's h= is a genuine alias (ChiselCAD is already correct) while segments= is genuine but unimplemented — documented as a follow-up in tests/tools/README.md rather than guessed at without a way to verify it. This environment has no live OpenSCAD oracle and no buildable Manifold (network access is scoped to this repo only), so every fix here was instead verified by compiling the GPU/Manifold-free language+CSG subsystem for real against glm/nlohmann-json/zlib/a from-source Catch2, and running the actual test_parser.cpp/test_csg_evaluator.cpp suites (344 cases, 1758 assertions, all passing) plus feeding all 11 corpus files through the real Lexer/Parser/ CsgEvaluator directly. Exact volumetric correctness against a live OpenSCAD binary remains unverified. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01Pn9TSaMhpqzA7iSKiQskrr --- docs/roadmap.md | 151 +++++++++++++++++++++++----- src/csg/CsgEvaluator.cpp | 185 ++++++++++++++++++++++++----------- src/lang/AST.h | 35 ++++++- src/lang/Parser.cpp | 177 +++++++++++++++++++++++++++------ src/lang/Parser.h | 6 ++ tests/test_csg_evaluator.cpp | 156 +++++++++++++++++++++++++++++ tests/test_parser.cpp | 173 +++++++++++++++++++++++++++++--- tests/tools/README.md | 28 ++++++ 8 files changed, 782 insertions(+), 129 deletions(-) diff --git a/docs/roadmap.md b/docs/roadmap.md index 2b1e910..25b5c33 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -437,11 +437,30 @@ run through this so far — `2D`, `bugs`, `bugs2D`, `misc`, `issues` are still unexamined. This should become a standing regression suite (rerun after any geometry-affecting change), not a one-time audit. -## v3.10 — Follow-up on the oracle-version caveat (issue #91) + a parser fix (issue #90) +## v3.10 — All 11 issue #90 parse failures fixed; issue #91's oracle-version question resolved + +Environment note: this pass had no access to a live OpenSCAD oracle or a +buildable Manifold (network egress in this session is scoped to +`particlesector/chiselcad` only, so `vcpkg`/other-repo downloads used by the +usual build are blocked). All of the parser/grammar fixes below were instead +verified for real: the language+CSG subsystem (`src/lang`, `src/csg`, and +the GPU/Manifold-free import loaders — the same set `tests/tools/README.md` +already documents as buildable with plain `g++ -lz`, no vcpkg needed) was +compiled and linked against real `glm`/`nlohmann-json`/`zlib`/a +from-source-built Catch2, and the *actual* `tests/test_parser.cpp` / +`tests/test_csg_evaluator.cpp` suites were run — 344 test cases / 1758 +assertions, all passing, including new regression tests for every fix below. +Each of issue #90's 11 originally-listed files was also fed through the real +`Lexer`/`Parser`/`CsgEvaluator` directly (fetched fresh from +`openscad/openscad`'s current corpus) and confirmed to both parse and +fully evaluate without error. What's still unverified against a live +OpenSCAD binary is exact geometric/volumetric correctness (the v3.9-style +`sym_diff_volume` comparison) — that needs the oracle this environment +doesn't have. - [x] **Resolved the v3.9 oracle-version question for `linear_extrude(h=...)` - and `segments=`.** Checked real OpenSCAD's current `master` source - (`src/core/LinearExtrudeNode.cc`) directly rather than the outdated + and `segments=`** (issue #91). Checked real OpenSCAD's current `master` + source (`src/core/LinearExtrudeNode.cc`) directly rather than the outdated `apt` 2021.01 binary: `parameters[{"height", "h"}]` confirms `h` genuinely is a real, current alias for `height` — ChiselCAD's support for it is correct, not a ChiselCAD invention, and not something to "fix" back to @@ -456,28 +475,112 @@ after any geometry-affecting change), not a one-time audit. smoother swept surface instead of faceting along the profile's original edges. This is a real, previously-unidentified gap and likely explains some fraction of `linear_extrude-tests.scad`'s remaining ~13.6% error — - filed as a follow-up rather than implemented blind here, since matching - `splitOutline`'s exact per-edge subdivision behavior needs bisecting - against a live OpenSCAD build the same way the v3.9 fixes were, not a - guess. + filed as a follow-up rather than implemented blind here (see + `tests/tools/README.md`), since matching `splitOutline`'s exact per-edge + subdivision behavior needs bisecting against a live modern OpenSCAD build, + not a guess with no way to check the result. - [x] **`for()` with a completely empty argument list failed to parse** - (part of issue #90's `for-tests.scad`, which opens with exactly this on - line 2: `for();`). `Parser::parseFor()` unconditionally expected an - `Ident '='` right after `(`, so a bare `for()` — valid OpenSCAD, since the - argument list is a generic module-call argument list that's allowed to be - empty — aborted parsing of the entire file. Fixed by skipping - variable/range parsing when `)` immediately follows `(`, leaving - `ForNode::var` empty as a sentinel (never otherwise possible, since a - parsed loop variable is always a non-empty identifier). - `CsgEvaluator::evalFor()` treats that sentinel as zero iterations, - matching real OpenSCAD's `builtin_for()` (`src/core/control.cc`), which - checks `if (!inst->arguments.empty())` and skips the body entirely rather - than running it once vacuously. Regression tests added at both the parser - level (`tests/test_parser.cpp`) and the CSG-evaluator level - (`tests/test_csg_evaluator.cpp`, including a wrapping-transform case). - This resolves one of issue #90's 11 files; the rest (multi-variable - `for`, other transform-argument edge cases) are still open and need the - same per-file bisection. + (`for-tests.scad`, issue #90 — opens with exactly this on line 2: + `for();`). `Parser::parseFor()` unconditionally expected an `Ident '='` + right after `(`. Real OpenSCAD's `for()` argument list is a generic + module-call argument list, legitimately allowed to be empty. Fixed by + making `ForNode` hold a `std::vector` (see next item) that's + simply empty in this case; `CsgEvaluator::evalFor()` treats an empty + clause list as zero iterations, matching real OpenSCAD's `builtin_for()` + (`src/core/control.cc`'s `if (!inst->arguments.empty())` guard) — it skips + the body entirely rather than running it once vacuously. +- [x] **Multi-variable `for (i=..., j=..., k=...)` wasn't supported at all** + (`for-nested-tests.scad`, `mirror-tests.scad`, `edge-cases.scad` — 3 of + issue #90's 11 files). `ForNode` only ever held one `var`/`range` pair, so + `Parser::parseFor` parsed the first clause then unconditionally expected + `)`, erroring on the comma before a second clause. Restructured `ForNode` + to hold a list of `ForClause`s; `CsgEvaluator::evalFor` now recurses one + clause at a time as nested loops (Cartesian product, outermost = first + clause), evaluating each clause's range expression only once its own turn + comes up — so a later clause can reference an earlier one's already-bound + variable (`for (i=[0:3], j=[0:i])`), matching real OpenSCAD. +- [x] **`rotate(a, v)` — the angle+arbitrary-axis form — wasn't supported at + all**, and neither were `rotate()`/`mirror()` with zero arguments + (`rotate-parameters.scad`, `transform-tests.scad`, + `scale-mirror2D-3D-tests.scad` — 3 of issue #90's 11 files). + `Parser::parseTransform` called `parseExpr()` exactly once for a single + required argument, for all of translate/rotate/scale/mirror/multmatrix — + so `rotate(30, [0,1,0])` (two positional args), `rotate(v=..., a=...)` + (named), and a bare `rotate()`/`mirror()` (zero args) all failed to parse. + Added a dedicated argument-list parser for `rotate` (positional/named `a` + and `v`, matching real OpenSCAD's `Parameters::parse(arguments, {"a", + "v"})`) and made the single-positional-argument case optional for the + rest. `CsgEvaluator::makeMatrix` now implements the angle+axis rotation + itself via `glm::rotate(angle, axis)` (falling back to the Z axis when `v` + isn't given — mathematically identical to the old hard-coded Z-only + scalar case, so existing scalar `rotate(angle)` callers are unaffected), + and gives each transform its own OpenSCAD-documented default for a + missing/non-numeric argument (confirmed against real OpenSCAD's + `TransformNode.cc`: translate → `[0,0,0]`, scale → `[1,1,1]`, mirror → + `[1,0,0]`) instead of the one-size-fits-all `{0,0,0}` (or, for a scalar, + `{0,0,n}`) every transform previously shared. +- [x] **`scale(2)` (a bare scalar) scaled only the Z axis by 2, leaving X/Y + untouched, instead of scaling uniformly** — a real, silent correctness bug + found while fixing the above (the same shared "scalar → Z axis" decoding + rule is correct for `rotate(angle)` but was wrong for `scale`/`translate`/ + `mirror`, which never had their own scalar handling). Confirmed against + real OpenSCAD's `builtin_scale()`: a non-vector numeric argument falls + back to `scalevec.setConstant(num)` — uniform. Fixed for `Scale` + specifically (`transform-tests.scad`'s `scale(0.5) mycyl()` and + `scale3D-tests.scad`'s `scale(2) obj3D()` both exercise this). +- [x] **`union`/`difference`/`intersection`/`hull`/`minkowski` couldn't take + any arguments at all**, even `minkowski(convexity=2)`, a real OpenSCAD + parameter (`minkowski3-difference-test.scad`, issue #90). + `Parser::parseBoolean` called `expect(RParen)` immediately after `(` with + no attempt to parse anything in between. Now parses (and discards) a + generic param list first, same treatment `render()`'s `convexity=` already + got. +- [x] **Module definitions required a brace-block body — a single-statement + body with no braces (e.g. `module obj3D() cylinder(r=1, ...);`, an + extremely common one-liner idiom) failed to parse** (`scale3D-tests.scad`, + `scale-mirror2D-3D-tests.scad`, issue #90). Every *other* body (for/if/ + transform/boolean/...) already accepted both forms via the shared + `parseBody()` helper; module definitions had their own hard-coded + `expect(LBrace)` instead. Now uses `parseBody()` like everything else. +- [x] **A leaf primitive (cube/sphere/.../polygon) followed by a trailing + child statement failed to parse** (`scale-mirror2D-3D-tests.scad`'s + `module obj2D() polygon(...) square(...);` — `square()` is bound as + `polygon()`'s syntactic child, which OpenSCAD's grammar allows for *any* + module instantiation even though a leaf primitive's semantics ignore it). + `Parser::parsePrimitive` never called `parseBody()` at all. Now does, and + discards the result — matching how `union()`/`translate()`/etc. already + tolerate a bare `;` with no child via the very same call. +- [x] **`polygon()`'s positional (unnamed) points list was fundamentally + broken for any point count** — found while root-causing the item above, + not itself one of the original 11 files, but the same corpus file + (`scale-mirror2D-3D-tests.scad`) also exercises it, and it's a + significant, previously-unnoticed correctness bug independent of issue + #90. `parseParamList`'s generic "positional vector" case assumes any + bracketed positional argument is an `[x,y,z]`-shaped triple and + decomposes it into `"x"`/`"y"`/`"z"` keys, capped at 3 elements. For + `polygon([[x,y],...])` this is wrong in two different ways depending on + point count: **exactly 3 points** parsed without error but landed under + `"x"`/`"y"`/`"z"` instead of `"points"` — the key `CsgEvaluator`'s + polygon handling actually reads — so the polygon silently got *zero* + points; **4 or more points** (the overwhelmingly common real case) failed + to parse outright, since the loop's hard-coded 3-element cap left a + dangling `,` where `)`'s matching `]` was expected. Added a dedicated + `Parser::parsePolygonParams` (first positional → `points`, second → + `paths`, matching real OpenSCAD's declared `polygon(points, paths, + convexity)` order; third positional/`convexity` parsed and discarded). + Every unnamed `polygon([...])` call in the wild — likely most real-world + `.scad` files that use `polygon()` at all — was affected by one of these + two failure modes before this fix. + +All of the above have regression tests in `tests/test_parser.cpp` and/or +`tests/test_csg_evaluator.cpp` (tagged `[bugfix]`), verified passing against +the real compiled `Lexer`/`Parser`/`Interpreter`/`CsgEvaluator` (see the +environment note above) — not just read for syntactic plausibility. + +Issue #90's remaining scope: only the `3D/features` corpus subdirectory has +been checked (matching v3.9's existing note); `2D`, `bugs`, `bugs2D`, +`misc`, `issues` are still unexamined and may turn up further parse +failures of their own. ## v4 — Tooling & Visual Quality diff --git a/src/csg/CsgEvaluator.cpp b/src/csg/CsgEvaluator.cpp index 6b94a8b..22e463f 100644 --- a/src/csg/CsgEvaluator.cpp +++ b/src/csg/CsgEvaluator.cpp @@ -16,6 +16,7 @@ #include #include #include +#include #include namespace chisel::csg { @@ -439,7 +440,7 @@ glm::mat4 CsgEvaluator::makeMatrix(const TransformNode& t) const { // Rows are read directly into the matrix; glm is column-major, so // element [row][col] of the OpenSCAD matrix goes to m[col][row]. if (t.kind == TransformNode::Kind::Matrix) { - Value matVal = m_interp->evaluate(*t.vec); + Value matVal = t.vec ? m_interp->evaluate(*t.vec) : Value::undef(); glm::mat4 m{1.0f}; if (matVal.isVector()) { const auto& rows = matVal.asVec(); @@ -456,7 +457,13 @@ glm::mat4 CsgEvaluator::makeMatrix(const TransformNode& t) const { return m; } - Value rotVal = m_interp->evaluate(*t.vec); // evaluate once, share result + // Evaluate once, share result. t.vec is null for a completely empty + // argument list (e.g. bare `mirror()`/`rotate()`), which is distinct + // from evaluating to a non-vector/non-number value (e.g. `scale(undef)` + // or a typo'd string) — both fall through to the same "not vector, not + // number" default below, matching OpenSCAD's own behavior of falling + // back to its pre-conversion default in either case. + Value rotVal = t.vec ? m_interp->evaluate(*t.vec) : Value::undef(); auto vec = [&]() -> std::array { if (rotVal.isVector()) { std::array r = {0.0, 0.0, 0.0}; @@ -465,8 +472,26 @@ glm::mat4 CsgEvaluator::makeMatrix(const TransformNode& t) const { r[i] = rotVal.asVec()[i].asNumber(); return r; } - if (rotVal.isNumber()) - return {0.0, 0.0, rotVal.asNumber()}; // scalar → Z axis + if (rotVal.isNumber()) { + double n = rotVal.asNumber(); + // scale(2) broadcasts uniformly to all three axes — confirmed + // against real OpenSCAD's builtin_scale(), which falls back to + // `scalevec.setConstant(num)` when the argument isn't a vector + // but is a plain number. rotate(angle) still means "around Z" + // here; its scalar case is actually handled by its own branch + // below now, not this shared helper. + if (t.kind == TransformNode::Kind::Scale) + return {n, n, n}; + return {0.0, 0.0, n}; + } + // Argument missing entirely, or present but not a number/vector + // (undef, string, ...) — each transform's own OpenSCAD-documented + // pre-conversion default (see TransformNode.cc upstream): + // translate → [0,0,0] (no-op) + // scale → [1,1,1] (no-op) + // mirror → [1,0,0] + if (t.kind == TransformNode::Kind::Scale) return {1.0, 1.0, 1.0}; + if (t.kind == TransformNode::Kind::Mirror) return {1.0, 0.0, 0.0}; return {0.0, 0.0, 0.0}; }(); const double vx = vec[0], vy = vec[1], vz = vec[2]; @@ -480,13 +505,41 @@ glm::mat4 CsgEvaluator::makeMatrix(const TransformNode& t) const { break; case TransformNode::Kind::Rotate: { - // scalar rotate(angle) → Z-axis; vector → XYZ Euler - float rx = static_cast(vx * kDeg2Rad); - float ry = static_cast(vy * kDeg2Rad); - float rz = static_cast(vz * kDeg2Rad); - m = glm::rotate(m, rx, glm::vec3(1.0f, 0.0f, 0.0f)); - m = glm::rotate(m, ry, glm::vec3(0.0f, 1.0f, 0.0f)); - m = glm::rotate(m, rz, glm::vec3(0.0f, 0.0f, 1.0f)); + if (rotVal.isVector()) { + // rotate([x,y,z]) (or a 1/2-element prefix of it) — XYZ Euler + // angles; 'v' is ignored here, matching real OpenSCAD ("when + // deg_a is an array, the 'v' argument is ignored"). + float rx = static_cast(vx * kDeg2Rad); + float ry = static_cast(vy * kDeg2Rad); + float rz = static_cast(vz * kDeg2Rad); + m = glm::rotate(m, rx, glm::vec3(1.0f, 0.0f, 0.0f)); + m = glm::rotate(m, ry, glm::vec3(0.0f, 1.0f, 0.0f)); + m = glm::rotate(m, rz, glm::vec3(0.0f, 0.0f, 1.0f)); + } else { + // rotate(a) / rotate(a, v) / rotate(a=..., v=...) / rotate() — + // 'a' is a scalar angle in degrees (0 if absent or not a + // number) around axis 'v' (default the Z axis if 'v' is absent + // or degenerate), matching real OpenSCAD's + // angle_axis_degrees(a, v) path. A scalar-only rotate(angle) + // (no 'v') is the same rotation as the Z-axis case this used to + // hard-code, so existing scalar callers are unaffected. + double angle = rotVal.isNumber() ? rotVal.asNumber() : 0.0; + glm::vec3 axis(0.0f, 0.0f, 1.0f); + if (t.axis) { + Value axisVal = m_interp->evaluate(*t.axis); + if (axisVal.isVector()) { + std::array a3 = {0.0, 0.0, 0.0}; + for (std::size_t i = 0; i < 3 && i < axisVal.asVec().size(); ++i) + if (axisVal.asVec()[i].isNumber()) + a3[i] = axisVal.asVec()[i].asNumber(); + glm::vec3 candidate(static_cast(a3[0]), static_cast(a3[1]), + static_cast(a3[2])); + if (glm::dot(candidate, candidate) > 1e-12f) + axis = candidate; + } + } + m = glm::rotate(m, static_cast(angle * kDeg2Rad), glm::normalize(axis)); + } break; } @@ -554,58 +607,74 @@ CsgNodePtr CsgEvaluator::evalIf(const IfNode& node, const glm::mat4& xform, // --------------------------------------------------------------------------- CsgNodePtr CsgEvaluator::evalFor(const ForNode& node, const glm::mat4& xform, const ColorAttr& color) { - // `for()` with no arguments at all (Parser::parseFor leaves node.var - // empty in this case, which is otherwise unreachable since a parsed - // loop variable is always a non-empty identifier). Matches real - // OpenSCAD's builtin_for(), which never instantiates the body when the - // module call's argument list is empty — there's no iteration set for - // zero variables, not a single vacuous pass. - if (node.var.empty()) + // `for()` with no arguments at all (Parser::parseFor leaves node.clauses + // empty in this case). Matches real OpenSCAD's builtin_for(), which + // never instantiates the body when the module call's argument list is + // empty — there's no iteration set for zero variables, not a single + // vacuous pass. + if (node.clauses.empty()) return nullptr; - // Build the sequence of iteration values - std::vector values; - - if (node.range.isRange) { - double start = m_interp->evalNumber(*node.range.start); - double end = m_interp->evalNumber(*node.range.end); - double step = node.range.step ? m_interp->evalNumber(*node.range.step) : 1.0; - values = m_interp->expandRange(start, step, end); - } else if (node.range.isBracketedList) { - // Bracketed list literal `[a, b, c]` — each element is its own loop - // value, even if it evaluates to a vector (e.g. a point list - // `[[1,2,3], [4,5,6]]` must yield two vector iterations, not six - // scalars). - for (const auto& e : node.range.list) - values.push_back(m_interp->evaluate(*e)); - } else { - // Expression form `for (v = expr)` — expr must evaluate to a vector - // or a range (e.g. a variable holding one, or a general range - // literal used directly: `for (i = someRange)`); expand either so - // `for (pt = pts)` iterates over pts' elements. - for (const auto& e : node.range.list) { - Value v = m_interp->evaluate(*e); - for (auto& elem : m_interp->iterationValues(v)) - values.push_back(std::move(elem)); + std::vector all; + + // Multi-variable for (var1 = ..., var2 = ..., ...) iterates the + // Cartesian product of all clauses as nested loops, outermost = first + // clause (matches real OpenSCAD). Recurse one clause at a time so a + // later clause's range expression can reference an earlier clause's + // already-bound variable (e.g. `for (i = [0:3], j = [0:i])`) — each + // clause's values are only computed once its own turn comes up, by + // which point every outer clause's variable is already set in m_env. + std::function runClause = [&](std::size_t idx) { + if (idx == node.clauses.size()) { + for (const auto& child : node.children) + if (auto c = evalNode(*child, xform, color)) + all.push_back(std::move(c)); + return; } - } - // Each iteration gets a fresh copy of the enclosing scope: the loop - // variable and any local assignment inside the body must not leak into - // the next iteration or survive past the loop (matching OpenSCAD's - // per-iteration block scoping), so restore the pre-loop snapshot before - // each iteration rather than just saving/restoring node.var alone. - auto savedEnv = m_interp->snapshotEnv(); - std::vector all; - for (const Value& v : values) { - m_interp->restoreEnv(savedEnv); - m_interp->setVar(node.var, v); - for (const auto& child : node.children) { - if (auto c = evalNode(*child, xform, color)) - all.push_back(std::move(c)); + const ForClause& clause = node.clauses[idx]; + + // Build this clause's sequence of iteration values. + std::vector values; + if (clause.range.isRange) { + double start = m_interp->evalNumber(*clause.range.start); + double end = m_interp->evalNumber(*clause.range.end); + double step = clause.range.step ? m_interp->evalNumber(*clause.range.step) : 1.0; + values = m_interp->expandRange(start, step, end); + } else if (clause.range.isBracketedList) { + // Bracketed list literal `[a, b, c]` — each element is its own + // loop value, even if it evaluates to a vector (e.g. a point + // list `[[1,2,3], [4,5,6]]` must yield two vector iterations, + // not six scalars). + for (const auto& e : clause.range.list) + values.push_back(m_interp->evaluate(*e)); + } else { + // Expression form `for (v = expr)` — expr must evaluate to a + // vector or a range (e.g. a variable holding one, or a general + // range literal used directly: `for (i = someRange)`); expand + // either so `for (pt = pts)` iterates over pts' elements. + for (const auto& e : clause.range.list) { + Value v = m_interp->evaluate(*e); + for (auto& elem : m_interp->iterationValues(v)) + values.push_back(std::move(elem)); + } } - } - m_interp->restoreEnv(std::move(savedEnv)); + + // Each iteration of *this* clause gets a fresh copy of the scope as + // of just before this clause's loop started (i.e. with every outer + // clause's variable already bound, but none of this clause's own + // prior iterations' bindings or body-local assignments still + // hanging around) — matching OpenSCAD's per-iteration block + // scoping. + auto levelSnapshot = m_interp->snapshotEnv(); + for (const Value& v : values) { + m_interp->restoreEnv(levelSnapshot); + m_interp->setVar(clause.var, v); + runClause(idx + 1); + } + m_interp->restoreEnv(std::move(levelSnapshot)); + }; + runClause(0); if (all.empty()) return nullptr; diff --git a/src/lang/AST.h b/src/lang/AST.h index bd758d0..06ff203 100644 --- a/src/lang/AST.h +++ b/src/lang/AST.h @@ -98,9 +98,21 @@ struct TransformNode { Kind kind; // [x, y, z] vector argument — stored as a VectorLit expression so - // that variable references like translate([dx, 0, 0]) work. - // Unused (nullptr) for Kind::Identity. + // that variable references like translate([dx, 0, 0]) work. Also + // rotate()'s angle argument ('a': a scalar degrees value, or a 3-element + // [x,y,z] Euler-angle vector). nullptr when the argument list is + // completely empty (a bare `translate()`/`rotate()`/`mirror()`, all + // valid OpenSCAD — CsgEvaluator::makeMatrix falls back to each + // transform's own OpenSCAD-documented default in that case) or for + // Kind::Identity. ExprPtr vec; + // rotate(a, v) / rotate(a=..., v=...)'s axis-vector argument — only + // meaningful for Kind::Rotate when `vec` ('a') evaluates to a scalar + // rather than the 3-element Euler-angle vector form (matching real + // OpenSCAD: "when deg_a is an array, the 'v' argument is ignored"). + // nullptr when not given, in which case rotate()'s axis defaults to the + // Z axis [0,0,1]. Unused for every other Kind. + ExprPtr axis; std::vector children; SourceLoc loc; uint8_t modifiers = ModNone; @@ -151,9 +163,24 @@ struct ForRange { bool isBracketedList = false; }; +// One `var = range` binding in a for()'s (possibly multi-variable) argument +// list: `for (i = [0:3], j = [0:1])` has two clauses, evaluated as nested +// loops (Cartesian product, outermost = first clause) — matching real +// OpenSCAD, where a later clause's range expression may reference an +// earlier clause's already-bound variable (e.g. `for (i=[0:3], j=[0:i])`). +struct ForClause { + std::string var; + ForRange range; +}; + struct ForNode { - std::string var; - ForRange range; + // Empty means `for()` — a completely empty argument list, which is + // valid OpenSCAD (its argument list is a generic module-call argument + // list, allowed to be empty) but has no variable to iterate. + // CsgEvaluator::evalFor treats this as zero iterations, matching real + // OpenSCAD's builtin_for(), which skips the body entirely rather than + // running it once vacuously. + std::vector clauses; std::vector children; SourceLoc loc; uint8_t modifiers = ModNone; diff --git a/src/lang/Parser.cpp b/src/lang/Parser.cpp index f4e6c96..8a049cf 100644 --- a/src/lang/Parser.cpp +++ b/src/lang/Parser.cpp @@ -435,9 +435,23 @@ AstNodePtr Parser::parsePrimitive(TokenKind k) { } expect(TokenKind::LParen, "expected '(' after primitive name"); - parseParamList(node.params, node.center); + if (node.kind == PrimitiveNode::Kind::Polygon2D) + parsePolygonParams(node.params); + else + parseParamList(node.params, node.center); expect(TokenKind::RParen, "expected ')' after primitive arguments"); + // A leaf primitive can still be followed by a trailing child + // statement/block syntactically — every OpenSCAD module instantiation + // accepts one, even though a leaf primitive's own semantics have no use + // for it (upstream test corpus's scale-mirror2D-3D-tests.scad: + // `module obj2D() polygon(...) square(...);` — square() is bound as + // polygon()'s syntactic child and simply has no effect). Parse and + // discard it so the file keeps parsing instead of erroring out here — + // matches how union()/translate()/etc. already tolerate a bare `;` with + // no child via this same parseBody() call. + parseBody(); + return makePrimitive(std::move(node)); } @@ -458,8 +472,17 @@ AstNodePtr Parser::parseBoolean(TokenKind k) { default: break; } - expect(TokenKind::LParen, "expected '(' after boolean operator"); - expect(TokenKind::RParen, "expected ')' after boolean operator"); + expect(TokenKind::LParen, "expected '(' after boolean operator"); + // union/difference/intersection/hull() take no arguments in real + // OpenSCAD, but minkowski() accepts a `convexity=` hint (upstream test + // corpus: `minkowski(convexity=2) { ... }`) — like render()'s + // convexity, a preview-only concept ChiselCAD always fully evaluates + // past, so parse and discard whatever's given rather than requiring an + // empty argument list outright. + std::unordered_map discardedParams; + bool unusedCenter = false; + parseParamList(discardedParams, unusedCenter); + expect(TokenKind::RParen, "expected ')' after boolean operator"); node.children = parseBody(); return makeBoolean(std::move(node)); @@ -483,7 +506,44 @@ AstNodePtr Parser::parseTransform(TokenKind k) { } expect(TokenKind::LParen, "expected '(' after transform name"); - node.vec = parseExpr(); // [x,y,z] literal, 4x4 matrix literal, or any expression yielding one + + if (node.kind == TransformNode::Kind::Rotate) { + // rotate(a) / rotate(a, v) / rotate(a=..., v=...) / rotate() — 'a' + // (angle: scalar degrees, or a 3-element [x,y,z] Euler-angle vector) + // and 'v' (axis vector, only meaningful when 'a' is a scalar) bind + // like any other named/positional OpenSCAD argument list, matching + // real OpenSCAD's Parameters::parse(arguments, {"a", "v"}). Both are + // optional: a bare rotate() is valid (identity — angle 0). + bool sawPositional = false; + while (!check(TokenKind::RParen) && !atEnd()) { + const size_t prevPos = m_pos; // guard against zero-progress infinite loops + + if (peek(1).kind == TokenKind::Equals && isParamNameToken(peek())) { + std::string name = peek().text; + advance(); // name + advance(); // '=' + if (name == "v") node.axis = parseExpr(); + else node.vec = parseExpr(); // "a" (or any other name — discarded) + } else if (!sawPositional) { + node.vec = parseExpr(); + sawPositional = true; + } else { + node.axis = parseExpr(); + } + + match(TokenKind::Comma); + if (m_pos == prevPos) break; + } + } else if (!check(TokenKind::RParen)) { + // translate([x,y,z]) / scale(...) / mirror([x,y,z]) / multmatrix(m) + // — a single positional argument. An empty argument list (e.g. + // mirror(), matching OpenSCAD) leaves node.vec unset, so + // CsgEvaluator::makeMatrix falls back to each transform's own + // OpenSCAD-documented default (translate: [0,0,0]; scale: [1,1,1]; + // mirror: [1,0,0]). + node.vec = parseExpr(); // [x,y,z] literal, 4x4 matrix literal, or any expression yielding one + } + expect(TokenKind::RParen, "expected ')' after transform argument"); node.children = parseBody(); @@ -583,7 +643,13 @@ AstNodePtr Parser::parseIf() { } // --------------------------------------------------------------------------- -// for — for (var = [start:step:end]) or for (var = [v0, v1, ...]) +// for — for (var = [start:step:end]) or for (var = [v0, v1, ...]), or the +// multi-variable form for (var1 = ..., var2 = ..., ...) — a comma-separated +// list of independent clauses evaluated as nested loops (Cartesian product, +// outermost = first clause; see ForClause in AST.h). `for()` with a +// completely empty argument list is also valid OpenSCAD (seen e.g. in the +// upstream test corpus's for-tests.scad) — node.clauses stays empty in that +// case. // --------------------------------------------------------------------------- AstNodePtr Parser::parseFor() { const Token& kw = advance(); // consume 'for' @@ -592,15 +658,11 @@ AstNodePtr Parser::parseFor() { expect(TokenKind::LParen, "expected '(' after 'for'"); - // `for()` with a completely empty argument list is valid OpenSCAD (seen - // e.g. in the upstream test corpus's for-tests.scad) and parses fine — - // it just contributes no geometry, since there's no variable to iterate. - // node.var stays empty, which CsgEvaluator::evalFor treats as its - // zero-arguments/zero-iterations case (matching real OpenSCAD's - // builtin_for(), which skips the body entirely when inst->arguments is - // empty rather than treating it as one vacuous iteration). - if (!check(TokenKind::RParen)) { - node.var = expect(TokenKind::Ident, "expected loop variable").text; + while (!check(TokenKind::RParen) && !atEnd()) { + const size_t prevPos = m_pos; // guard against zero-progress infinite loops + + ForClause clause; + clause.var = expect(TokenKind::Ident, "expected loop variable").text; expect(TokenKind::Equals, "expected '=' after loop variable"); if (check(TokenKind::LBracket)) { @@ -616,31 +678,36 @@ AstNodePtr Parser::parseFor() { if (check(TokenKind::Colon)) { advance(); // consume ':' auto third = parseExpr(); - node.range.isRange = true; - node.range.start = std::move(first); - node.range.step = std::move(second); - node.range.end = std::move(third); + clause.range.isRange = true; + clause.range.start = std::move(first); + clause.range.step = std::move(second); + clause.range.end = std::move(third); } else { - node.range.isRange = true; - node.range.start = std::move(first); - node.range.end = std::move(second); + clause.range.isRange = true; + clause.range.start = std::move(first); + clause.range.end = std::move(second); } } else { // List form: [first, ...] - node.range.isRange = false; - node.range.isBracketedList = true; - node.range.list.push_back(std::move(first)); + clause.range.isRange = false; + clause.range.isBracketedList = true; + clause.range.list.push_back(std::move(first)); while (match(TokenKind::Comma)) { if (check(TokenKind::RBracket)) break; - node.range.list.push_back(parseExpr()); + clause.range.list.push_back(parseExpr()); } } expect(TokenKind::RBracket, "expected ']' after range/list"); } else { // Expression form: for (var = expr) — expr must evaluate to a vector - node.range.isRange = false; - node.range.list.push_back(parseExpr()); + clause.range.isRange = false; + clause.range.list.push_back(parseExpr()); } + + node.clauses.push_back(std::move(clause)); + + if (m_pos == prevPos) break; // no token consumed — stop to avoid infinite loop + if (!match(TokenKind::Comma)) break; } expect(TokenKind::RParen, "expected ')' after for header"); @@ -718,6 +785,51 @@ void Parser::parseParamList(std::unordered_map& params, } } +// --------------------------------------------------------------------------- +// parsePolygonParams — polygon(points, paths, convexity) +// +// parseParamList's generic "positional vector" case (used by cube/square/ +// etc.) assumes a bracketed positional argument is an [x,y,z]-shaped triple +// and decomposes it into "x"/"y"/"z" keys, capping at 3 elements. polygon()'s +// positional arguments are structurally different — "points" is a whole +// list of [x,y] points (almost always more than 3 of them) and "paths" a +// whole list of index lists — that CsgEvaluator needs intact under the +// "points"/"paths" keys to evaluate as one vector value each. Reusing the +// generic case here silently broke every unnamed polygon() call: a point +// list with exactly 3 points parsed without error but the points ended up +// under "x"/"y"/"z" (never read by CsgEvaluator's polygon handling, which +// only looks at "points"), and 4+ points hit the loop's hardcoded 3-element +// cap and failed to parse at all ("expected ']'") — the latter is what +// upstream's scale-mirror2D-3D-tests.scad (issue #90) tripped over. +// --------------------------------------------------------------------------- +void Parser::parsePolygonParams(std::unordered_map& params) { + static const char* posKeys[] = {"points", "paths"}; + int posIdx = 0; + + while (!check(TokenKind::RParen) && !atEnd()) { + const size_t prevPos = m_pos; // guard against zero-progress infinite loops + + // Named param: any token (Ident or keyword) followed by '=' + if (peek(1).kind == TokenKind::Equals && isParamNameToken(peek())) { + std::string name = advance().text; + advance(); // '=' + params[name] = parseExpr(); + } else if (posIdx < 2) { + // First positional = points, second = paths — matches real + // OpenSCAD's declared polygon(points, paths, convexity) order. + params[posKeys[posIdx++]] = parseExpr(); + } else { + // Third positional (convexity) or beyond — a preview-only hint + // ChiselCAD always fully evaluates past; parse and discard, + // same as render()/minkowski()'s convexity. + parseExpr(); + } + + match(TokenKind::Comma); + if (m_pos == prevPos) break; + } +} + // --------------------------------------------------------------------------- // Expression parser (Pratt / precedence climbing) // --------------------------------------------------------------------------- @@ -1033,10 +1145,13 @@ ModuleDef Parser::parseModuleDefCore() { } expect(TokenKind::RParen, "expected ')' after parameter list"); - // Body must be a brace block for module definitions - expect(TokenKind::LBrace, "expected '{' for module body"); - def.body = parseBraceBlock(); - expect(TokenKind::RBrace, "expected '}' to close module body"); + // Module body: a brace block `{ ... }`, or — matching real OpenSCAD + // (e.g. upstream test corpus's `module obj3D() cylinder(r=1, ...);`) — + // a single statement with no braces at all. Every other body (for/if/ + // transform/boolean/...) already accepts both forms via parseBody(); + // module definitions had been hard-requiring a brace block, which + // rejected this extremely common one-liner module idiom outright. + def.body = parseBody(); return def; } diff --git a/src/lang/Parser.h b/src/lang/Parser.h index 86a75cd..6398692 100644 --- a/src/lang/Parser.h +++ b/src/lang/Parser.h @@ -80,6 +80,12 @@ class Parser { // ---- argument helpers ------------------------------------------------ void parseParamList(std::unordered_map& params, bool& center); + // polygon(points, paths, convexity) — unlike parseParamList's generic + // positional-vector case (which assumes an [x,y,z]-shaped triple and + // decomposes it into "x"/"y"/"z" keys), polygon's positional arguments + // are whole point/path LISTS that CsgEvaluator needs intact under + // "points"/"paths" keys — see parsePrimitive. + void parsePolygonParams(std::unordered_map& params); void parseExtrusionParams(std::unordered_map& params); // Parses `(arg, name=arg, ...)` up to and including the closing ')' — // shared by parsePrimary's `ident(...)` FunctionCall and parsePostfix's diff --git a/tests/test_csg_evaluator.cpp b/tests/test_csg_evaluator.cpp index a32c7b1..cffdc7f 100644 --- a/tests/test_csg_evaluator.cpp +++ b/tests/test_csg_evaluator.cpp @@ -284,6 +284,86 @@ TEST_CASE("CsgEval:mirror [0,0,0] is identity", "[csg]") { REQUIRE(leaf.transform[2][2] == Approx(1.0f)); } +// --------------------------------------------------------------------------- +// scale(scalar) broadcasts uniformly, and empty-argument-list transforms +// (bugfix — issue #90's rotate-parameters.scad / scale-mirror2D-3D-tests.scad) +// --------------------------------------------------------------------------- +TEST_CASE("CsgEval:scale(scalar) broadcasts to all three axes", "[csg][bugfix]") { + // Confirmed against real OpenSCAD's builtin_scale(): when the argument + // isn't a vector but is a plain number, it falls back to + // `scalevec.setConstant(num)` — a uniform scale — not the "scalar means + // Z axis only" rule that's correct for rotate(angle) but was previously + // (incorrectly) shared by scale/translate/mirror's argument decoding too. + auto s = evaluate("scale(2) cube([1,1,1]);"); + const auto& leaf = asLeaf(s.roots[0]); + REQUIRE(leaf.transform[0][0] == Approx(2.0f)); + REQUIRE(leaf.transform[1][1] == Approx(2.0f)); + REQUIRE(leaf.transform[2][2] == Approx(2.0f)); +} + +TEST_CASE("CsgEval:rotate() with no arguments is identity", "[csg][bugfix]") { + auto s = evaluate("rotate() cube([1,1,1]);"); + const auto& leaf = asLeaf(s.roots[0]); + const glm::mat4 I{1.0f}; + for (int c = 0; c < 4; ++c) + for (int r = 0; r < 4; ++r) + REQUIRE(leaf.transform[c][r] == Approx(I[c][r]).margin(1e-5)); +} + +TEST_CASE("CsgEval:mirror() with no arguments defaults to axis [1,0,0]", "[csg][bugfix]") { + // Confirmed against real OpenSCAD's builtin_mirror(): x/y/z are + // pre-initialized to 1.0/0.0/0.0 before attempting to convert the + // (here, absent) 'v' argument, so a totally bare mirror() mirrors + // across the X axis, same as mirror([1,0,0]). + auto s = evaluate("mirror() cube([1,1,1]);"); + const auto& leaf = asLeaf(s.roots[0]); + REQUIRE(leaf.transform[0][0] == Approx(-1.0f)); + REQUIRE(leaf.transform[1][1] == Approx(1.0f)); + REQUIRE(leaf.transform[2][2] == Approx(1.0f)); +} + +TEST_CASE("CsgEval:rotate(a, v) rotates by angle a around an arbitrary axis v", + "[csg][bugfix]") { + // rotate(90, [0,0,1]) — 90-degree rotation around Z, expressed via the + // angle+axis form rather than the [x,y,z] Euler form. Previously + // unparseable at all (parseTransform only accepted one argument); now + // implemented via glm::rotate(angle, axis), matching real OpenSCAD's + // angle_axis_degrees(a, v). + auto s = evaluate("rotate(90, [0,0,1]) cube([1,1,1]);"); + const auto& leaf = asLeaf(s.roots[0]); + REQUIRE(leaf.transform[0][0] == Approx(0.0f).margin(1e-4)); + REQUIRE(leaf.transform[0][1] == Approx(1.0f).margin(1e-4)); + REQUIRE(leaf.transform[1][0] == Approx(-1.0f).margin(1e-4)); + REQUIRE(leaf.transform[1][1] == Approx(0.0f).margin(1e-4)); + REQUIRE(leaf.transform[2][2] == Approx(1.0f).margin(1e-4)); +} + +TEST_CASE("CsgEval:rotate(a=..., v=...) named arguments work in either order", + "[csg][bugfix]") { + auto s1 = evaluate("rotate(a=90, v=[0,0,1]) cube([1,1,1]);"); + auto s2 = evaluate("rotate(v=[0,0,1], a=90) cube([1,1,1]);"); + const auto& l1 = asLeaf(s1.roots[0]); + const auto& l2 = asLeaf(s2.roots[0]); + for (int c = 0; c < 3; ++c) + for (int r = 0; r < 3; ++r) + REQUIRE(l1.transform[c][r] == Approx(l2.transform[c][r]).margin(1e-5)); + REQUIRE(l1.transform[1][0] == Approx(-1.0f).margin(1e-4)); +} + +TEST_CASE("CsgEval:rotate([x,y,z] vector) ignores a 'v' axis argument", "[csg][bugfix]") { + // Matches real OpenSCAD: "when deg_a is an array, the 'v' argument is + // ignored" — a two-positional-argument rotate() where the first + // argument is a 3-element vector should fall back to the Euler form, + // never the angle-axis form. + auto withV = evaluate("rotate([0,0,90], [1,0,0]) cube([1,1,1]);"); + auto withoutV = evaluate("rotate([0,0,90]) cube([1,1,1]);"); + const auto& a = asLeaf(withV.roots[0]); + const auto& b = asLeaf(withoutV.roots[0]); + for (int c = 0; c < 3; ++c) + for (int r = 0; r < 3; ++r) + REQUIRE(a.transform[c][r] == Approx(b.transform[c][r]).margin(1e-5)); +} + // --------------------------------------------------------------------------- // multmatrix() folds the given 4x4 rows straight into the leaf transform // --------------------------------------------------------------------------- @@ -719,6 +799,53 @@ TEST_CASE("CsgEval:for() with no arguments under a transform still yields no geo REQUIRE(s.roots.empty()); } +TEST_CASE("CsgEval:multi-variable for() iterates the Cartesian product of all clauses", + "[csg][bugfix]") { + // for (x=[0:1], y=[0:1], z=[0:1]) — real OpenSCAD's multi-variable + // for-loop form (upstream corpus for-nested-tests.scad/mirror-tests.scad/ + // edge-cases.scad). 2 x 2 x 2 = 8 children, nested outer-to-inner in the + // order the clauses were written (x slowest, z fastest). + auto s = evaluate("for (x=[0:1], y=[0:1], z=[0:1]) translate([x,y,z]) sphere(r=1);"); + REQUIRE(s.roots.size() == 1); + const auto& b = asBool(s.roots[0]); + REQUIRE(b.children.size() == 8); + REQUIRE(asLeaf(b.children[0]).transform[3][2] == Approx(0.0f)); // x=0,y=0,z=0 + REQUIRE(asLeaf(b.children[1]).transform[3][2] == Approx(1.0f)); // x=0,y=0,z=1 + REQUIRE(asLeaf(b.children[2]).transform[3][1] == Approx(1.0f)); // x=0,y=1,z=0 + REQUIRE(asLeaf(b.children[4]).transform[3][0] == Approx(1.0f)); // x=1,y=0,z=0 + REQUIRE(asLeaf(b.children[7]).transform[3][0] == Approx(1.0f)); // x=1,y=1,z=1 + REQUIRE(asLeaf(b.children[7]).transform[3][1] == Approx(1.0f)); + REQUIRE(asLeaf(b.children[7]).transform[3][2] == Approx(1.0f)); +} + +TEST_CASE("CsgEval:multi-variable for() later clause can reference an earlier clause's variable", + "[csg][bugfix]") { + // for (i=[0:2], j=[0:i]) — j's range depends on i, which must already be + // bound by the time j's clause runs (matches real OpenSCAD). i=0 → j + // has 1 value (0); i=1 → 2 values (0,1); i=2 → 3 values (0,1,2). Total + // children: 1 + 2 + 3 = 6. + auto s = evaluate("for (i=[0:2], j=[0:i]) sphere(r=1);"); + REQUIRE(s.roots.size() == 1); + const auto& b = asBool(s.roots[0]); + REQUIRE(b.children.size() == 6); +} + +TEST_CASE("CsgEval:multi-variable for() restores outer variable between outer iterations", + "[csg][bugfix]") { + // A body-local assignment or inner-clause binding must not leak into + // the next outer iteration — same guarantee the single-variable case + // already had, now checked across nesting levels. + auto s = evaluate("for (x=[0:1], y=[10,20]) translate([x,y,0]) sphere(r=1);"); + const auto& b = asBool(s.roots[0]); + REQUIRE(b.children.size() == 4); + REQUIRE(asLeaf(b.children[0]).transform[3][0] == Approx(0.0f)); + REQUIRE(asLeaf(b.children[0]).transform[3][1] == Approx(10.0f)); + REQUIRE(asLeaf(b.children[1]).transform[3][0] == Approx(0.0f)); + REQUIRE(asLeaf(b.children[1]).transform[3][1] == Approx(20.0f)); + REQUIRE(asLeaf(b.children[2]).transform[3][0] == Approx(1.0f)); + REQUIRE(asLeaf(b.children[2]).transform[3][1] == Approx(10.0f)); +} + TEST_CASE("CsgEval:for over bracketed point-list literal iterates per-point, not flattened", "[csg]") { // Regression test: a bracketed list literal whose own elements are @@ -1004,6 +1131,35 @@ TEST_CASE("CsgEval:list comprehension builds polygon() points end-to-end", "[csg REQUIRE(leaf.polyPoints[2].y == Approx(4.0f)); } +TEST_CASE("CsgEval:polygon with a positional (unnamed) 4+ point list", "[csg][bugfix]") { + // Previously misparsed entirely — see the matching Parser-level bugfix + // tests. A positional polygon() point list is the overwhelmingly common + // real-world form (upstream test corpus's scale-mirror2D-3D-tests.scad + // uses exactly this, with 4 points). + auto s = evaluate("polygon([[-0.5,-0.5],[1,-0.5],[1,1],[-0.5,0.5]]);"); + REQUIRE(s.roots.size() == 1); + const auto& leaf = asLeaf(s.roots[0]); + REQUIRE(leaf.kind == CsgLeaf::Kind::Polygon2D); + REQUIRE(leaf.polyPoints.size() == 4); + REQUIRE(leaf.polyPoints[0].x == Approx(-0.5f)); + REQUIRE(leaf.polyPoints[0].y == Approx(-0.5f)); + REQUIRE(leaf.polyPoints[3].x == Approx(-0.5f)); + REQUIRE(leaf.polyPoints[3].y == Approx(0.5f)); +} + +TEST_CASE("CsgEval:polygon with exactly 3 positional points (previous accidental pass-through)", + "[csg][bugfix]") { + // A 3-point positional polygon used to parse without error but land its + // points under "x"/"y"/"z" (parseParamList's generic vector-decompose + // case) instead of "points" — CsgEvaluator's `p.params.count("points")` + // was always false, so it silently produced zero points. + auto s = evaluate("polygon([[0,0],[10,0],[5,8]]);"); + REQUIRE(s.roots.size() == 1); + const auto& leaf = asLeaf(s.roots[0]); + REQUIRE(leaf.kind == CsgLeaf::Kind::Polygon2D); + REQUIRE(leaf.polyPoints.size() == 3); +} + TEST_CASE("CsgEval:for over a variable holding a range literal expands it", "[csg][bugfix]") { auto s = evaluate("r = [0:2];" "for (i = r) cube(i + 1);"); diff --git a/tests/test_parser.cpp b/tests/test_parser.cpp index 4a9a948..b37aba7 100644 --- a/tests/test_parser.cpp +++ b/tests/test_parser.cpp @@ -171,6 +171,34 @@ TEST_CASE("Parser:minkowski", "[parser]") { REQUIRE(b.children.size() == 2); } +TEST_CASE("Parser:minkowski(convexity=...) parses and discards the hint", "[parser][bugfix]") { + // Real OpenSCAD's minkowski() accepts a convexity= preview hint (upstream + // test corpus: `minkowski(convexity=2) { ... }`). Previously + // parseBoolean unconditionally expected ')' immediately after '(' for + // every boolean op, so any argument at all — even minkowski's legitimate + // one — failed to parse. Parsed and discarded, same as render()'s + // convexity, since ChiselCAD always fully evaluates. + auto r = parse("minkowski(convexity=2) { cube(1); sphere(1); }"); + REQUIRE(r.roots.size() == 1); + auto& b = asBool(r.roots[0]); + REQUIRE(b.op == BooleanNode::Op::Minkowski); + REQUIRE(b.children.size() == 2); +} + +TEST_CASE("Parser:a leaf primitive tolerates a trailing child statement", "[parser][bugfix]") { + // Every OpenSCAD module instantiation — including leaf primitives that + // have no real use for children — syntactically accepts a trailing + // child statement (upstream test corpus's scale-mirror2D-3D-tests.scad: + // `module obj2D() polygon(...) square(...);`, where square() is + // polygon()'s syntactic child and has no effect). Previously + // parsePrimitive never called parseBody(), so a primitive followed by + // anything other than ';' or end-of-block took the parse down. + auto r = parse("cube(1) sphere(1);"); + REQUIRE(r.roots.size() == 1); + auto& c = asPrim(r.roots[0]); + REQUIRE(c.kind == PrimitiveNode::Kind::Cube); +} + TEST_CASE("Parser:hull empty", "[parser]") { auto r = parse("hull() {}"); REQUIRE(r.roots.size() == 1); @@ -427,24 +455,25 @@ TEST_CASE("Parser:for range [start:end]", "[parser]") { auto r = parse("for (i = [0:4]) sphere(r=1);"); REQUIRE(r.roots.size() == 1); auto& f = asFor(r.roots[0]); - REQUIRE(f.var == "i"); - REQUIRE(f.range.isRange == true); - REQUIRE(f.range.step == nullptr); // implicit step + REQUIRE(f.clauses.size() == 1); + REQUIRE(f.clauses[0].var == "i"); + REQUIRE(f.clauses[0].range.isRange == true); + REQUIRE(f.clauses[0].range.step == nullptr); // implicit step REQUIRE(f.children.size() == 1); } TEST_CASE("Parser:for range [start:step:end]", "[parser]") { auto r = parse("for (i = [0:2:8]) sphere(r=1);"); auto& f = asFor(r.roots[0]); - REQUIRE(f.range.isRange == true); - REQUIRE(f.range.step != nullptr); + REQUIRE(f.clauses[0].range.isRange == true); + REQUIRE(f.clauses[0].range.step != nullptr); } TEST_CASE("Parser:for list", "[parser]") { auto r = parse("for (v = [1, 3, 7]) sphere(r=1);"); auto& f = asFor(r.roots[0]); - REQUIRE(f.range.isRange == false); - REQUIRE(f.range.list.size() == 3); + REQUIRE(f.clauses[0].range.isRange == false); + REQUIRE(f.clauses[0].range.list.size() == 3); } TEST_CASE("Parser:for with brace body", "[parser]") { @@ -453,21 +482,87 @@ TEST_CASE("Parser:for with brace body", "[parser]") { REQUIRE(f.children.size() == 2); } -TEST_CASE("Parser:for() with no arguments parses with an empty loop variable", "[parser][bugfix]") { +TEST_CASE("Parser:for() with no arguments parses with zero clauses", "[parser][bugfix]") { // Real OpenSCAD accepts a bare for() (seen in its own test corpus, // for-tests.scad's `for();`) — the argument list is a generic module-call // argument list, which is legitimately allowed to be empty. Previously // ChiselCAD's parser unconditionally expected an identifier right after // '(', so this failed to parse at all and took the whole file down with - // it. node.var stays empty as the sentinel CsgEvaluator::evalFor checks - // for zero-iterations. + // it. node.clauses stays empty as the sentinel CsgEvaluator::evalFor + // checks for zero-iterations. auto r = parse("for() sphere(r=1);"); REQUIRE(r.roots.size() == 1); auto& f = asFor(r.roots[0]); - REQUIRE(f.var.empty()); + REQUIRE(f.clauses.empty()); REQUIRE(f.children.size() == 1); } +TEST_CASE("Parser:multi-variable for() parses one clause per comma-separated binding", + "[parser][bugfix]") { + // `for (x = ..., y = ..., z = ...)` — real OpenSCAD's multi-variable + // for-loop form (seen e.g. in the upstream test corpus's + // for-nested-tests.scad/mirror-tests.scad/edge-cases.scad). Previously + // Parser::parseFor only ever parsed a single `var = ...` clause and then + // unconditionally expected ')', so a second comma-separated clause took + // the whole file down with a parse error. + auto r = parse("for (x = [0:3], y = [0:0.5:1], z = [0,2,3]) sphere(1);"); + REQUIRE(r.roots.size() == 1); + auto& f = asFor(r.roots[0]); + REQUIRE(f.clauses.size() == 3); + REQUIRE(f.clauses[0].var == "x"); + REQUIRE(f.clauses[0].range.isRange == true); + REQUIRE(f.clauses[1].var == "y"); + REQUIRE(f.clauses[1].range.isRange == true); + REQUIRE(f.clauses[2].var == "z"); + REQUIRE(f.clauses[2].range.isRange == false); + REQUIRE(f.clauses[2].range.list.size() == 3); +} + +TEST_CASE("Parser:rotate() with no arguments parses", "[parser][bugfix]") { + // Real OpenSCAD accepts a bare rotate() (seen in the upstream test + // corpus's rotate-parameters.scad: "rotate() //same as undef"). + // Previously Parser::parseTransform unconditionally called parseExpr() + // for exactly one required argument, so this failed to parse. + auto r = parse("rotate() cube(1);"); + REQUIRE(r.roots.size() == 1); + auto& t = asTrans(r.roots[0]); + REQUIRE(t.vec == nullptr); +} + +TEST_CASE("Parser:rotate(a, v) two-positional-argument form parses", "[parser][bugfix]") { + // rotate(angle, axis) — real OpenSCAD's arbitrary-axis rotation form + // (upstream corpus rotate-parameters.scad: "rotate(30,[0,1,0])"). + // Previously unparseable: parseTransform's single parseExpr() consumed + // just the angle, then expected ')' but found ',' next. + auto r = parse("rotate(30,[0,1,0]) cube(1);"); + REQUIRE(r.roots.size() == 1); + auto& t = asTrans(r.roots[0]); + REQUIRE(t.vec != nullptr); + REQUIRE(t.axis != nullptr); +} + +TEST_CASE("Parser:rotate(v=..., a=...) named-argument form parses in either order", + "[parser][bugfix]") { + // Upstream corpus transform-tests.scad: "rotate(v=[-1,0,0], a=45)". + auto r1 = parse("rotate(v=[-1,0,0], a=45) cube(1);"); + auto& t1 = asTrans(r1.roots[0]); + REQUIRE(t1.vec != nullptr); + REQUIRE(t1.axis != nullptr); + + auto r2 = parse("rotate(a=45, v=[-1,0,0]) cube(1);"); + auto& t2 = asTrans(r2.roots[0]); + REQUIRE(t2.vec != nullptr); + REQUIRE(t2.axis != nullptr); +} + +TEST_CASE("Parser:mirror() with no arguments parses", "[parser][bugfix]") { + // Upstream corpus scale-mirror2D-3D-tests.scad: "mirror() obj2D();". + auto r = parse("mirror() cube(1);"); + REQUIRE(r.roots.size() == 1); + auto& t = asTrans(r.roots[0]); + REQUIRE(t.vec == nullptr); +} + // --------------------------------------------------------------------------- // General range-literal expressions (usable outside `for`) // --------------------------------------------------------------------------- @@ -641,6 +736,22 @@ TEST_CASE("Parser:module definition stored in moduleDefs", "[parser]") { REQUIRE(r.moduleDefs[0].body.size() == 1); } +TEST_CASE("Parser:module definition with a single-statement body (no braces)", + "[parser][bugfix]") { + // Real OpenSCAD allows a module body to be one statement with no braces + // at all — an extremely common one-liner idiom (upstream test corpus: + // `module obj3D() cylinder(r=1, center=true, $fn=8);`). Previously + // parseModuleDefCore hard-required a '{' for every module body, which + // rejected this outright even though every other body (for/if/ + // transform/boolean/...) already accepted it via parseBody(). + auto r = parse("module obj3D() cylinder(r=1, center=true, $fn=8);"); + REQUIRE(r.moduleDefs.size() == 1); + REQUIRE(r.moduleDefs[0].name == "obj3D"); + REQUIRE(r.moduleDefs[0].body.size() == 1); + auto& c = asPrim(r.moduleDefs[0].body[0]); + REQUIRE(c.kind == PrimitiveNode::Kind::Cylinder); +} + TEST_CASE("Parser:module-local variable assignment is kept, not discarded", "[parser][bugfix]") { auto r = parse("module box(r) { d = r * 2; cube(d); }"); REQUIRE(r.moduleDefs.size() == 1); @@ -774,6 +885,44 @@ TEST_CASE("Parser:polygon with points", "[parser]") { REQUIRE(p.params.count("points") == 1); } +TEST_CASE("Parser:polygon with a positional (unnamed) points list of any length", + "[parser][bugfix]") { + // parseParamList's generic "positional vector [x,y,z]" case (used by + // cube/square/etc.) assumed a bracketed positional argument was a + // <=3-element x/y/z triple and decomposed it into "x"/"y"/"z" keys — + // completely wrong for polygon()'s positional points list, which + // CsgEvaluator needs intact under the "points" key. A 3-point polygon + // silently ended up with no "points" key at all (points landed under + // "x"/"y"/"z" instead, never read); a 4+ point polygon (the overwhelming + // common case — upstream test corpus's scale-mirror2D-3D-tests.scad + // uses a 4-point positional polygon) failed to parse outright, since the + // old loop's hardcoded 3-element cap left a dangling ',' where ']' was + // expected. + auto r3 = parse("polygon([[0,0],[10,0],[5,8]]);"); + const auto& p3 = asPrim(r3.roots[0]); + REQUIRE(p3.params.count("points") == 1); + + auto r4 = parse("polygon([[-0.5,-0.5],[1,-0.5],[1,1],[-0.5,0.5]]);"); + REQUIRE(r4.roots.size() == 1); + const auto& p4 = asPrim(r4.roots[0]); + REQUIRE(p4.kind == PrimitiveNode::Kind::Polygon2D); + REQUIRE(p4.params.count("points") == 1); + Interpreter interp; + Value pts = interp.evaluate(*p4.params.at("points")); + REQUIRE(pts.isVector()); + REQUIRE(pts.asVec().size() == 4); +} + +TEST_CASE("Parser:polygon with positional points and paths", "[parser][bugfix]") { + // polygon(points, paths, convexity) — real OpenSCAD's declared + // positional argument order; the third (convexity) is a preview-only + // hint, parsed and discarded like render()'s. + auto r = parse("polygon([[0,0],[1,0],[1,1],[0,1]], [[0,1,2,3]], 4);"); + const auto& p = asPrim(r.roots[0]); + REQUIRE(p.params.count("points") == 1); + REQUIRE(p.params.count("paths") == 1); +} + TEST_CASE("Parser:linear_extrude with height", "[parser]") { auto r = parse("linear_extrude(height=10) { circle(r=5); }"); REQUIRE(r.roots.size() == 1); @@ -1113,7 +1262,7 @@ TEST_CASE("Parser:for-loop variable named after a builtin primitive", "[parser]" auto r = parse("for (cube = [0:2]) { translate([cube, 0, 0]) sphere(1); }"); REQUIRE(r.roots.size() == 1); const auto& f = asFor(r.roots[0]); - REQUIRE(f.var == "cube"); + REQUIRE(f.clauses[0].var == "cube"); } TEST_CASE("Parser:let binding named after a builtin boolean op", "[parser]") { diff --git a/tests/tools/README.md b/tests/tools/README.md index 1ddb56f..1b3cd78 100644 --- a/tests/tools/README.md +++ b/tests/tools/README.md @@ -194,3 +194,31 @@ fixed/open list, including a note on a **test-tool** bug (not a ChiselCAD bug) in `scad_to_stl` itself that was masking several more files' results — only one corpus subdirectory (`tests/data/scad/3D/features`) has been run through this so far. + +## Known caveat: the installed oracle is older than the corpus (v3.10) + +The only easily-available oracle in this environment is `apt`'s OpenSCAD +2021.01. The corpus above is cloned from `openscad/openscad`'s current +`master` branch, which assumes some features 2021.01 predates — so a +mismatch against the 2021.01 oracle is not automatically a ChiselCAD bug. + +Confirmed case: `linear_extrude(h=10, ...)` — the corpus file's own comment +says `h` is an alias for `height`, but 2021.01 rejects it +(`WARNING: variable h not specified as parameter`). Checking real OpenSCAD's +current source directly (`src/core/LinearExtrudeNode.cc`, +`parameters[{"height", "h"}]`) confirms `h` genuinely is a current alias — +ChiselCAD supporting it is correct, not a bug to "fix" back to 2021.01 +behavior. `segments=` is also a genuine current `linear_extrude` parameter +(`src/geometry/linear_extrude.cc`: it subdivides the profile's outline edges +before extruding, distinct from `slices`, and only takes effect when a twist +or non-uniform scale is active) — but unlike `h=`, ChiselCAD doesn't +implement `segments=` at all yet. That's a real, currently-unfixed gap +(likely explains some of `linear_extrude-tests.scad`'s remaining error), not +implemented here since matching `splitOutline`'s exact per-edge subdivision +behavior needs bisecting against a live modern OpenSCAD build the same way +the v3.9 fixes were, not a guess. + +Before treating any other corpus mismatch as a confirmed ChiselCAD bug, +check whether it could be a similar version-mismatch case first — especially +anything involving a parameter or feature that could plausibly have been +added to OpenSCAD after 2021.01. From fe12c26c8e48691815242805cf2149f8d86b60b9 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 03:03:13 +0000 Subject: [PATCH 3/3] Address review: pin multmatrix() with no arguments as intentional identity Verified against real OpenSCAD's source (TransformNode.cc's builtin_multmatrix()): it also silently defaults to identity when its "m" argument is absent or not a vector, no error or warning. ChiselCAD's existing behavior already matched; added a regression test to make that explicit rather than accidental. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01Pn9TSaMhpqzA7iSKiQskrr --- tests/test_csg_evaluator.cpp | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/tests/test_csg_evaluator.cpp b/tests/test_csg_evaluator.cpp index cffdc7f..0fb01c4 100644 --- a/tests/test_csg_evaluator.cpp +++ b/tests/test_csg_evaluator.cpp @@ -394,6 +394,27 @@ TEST_CASE("CsgEval:multmatrix composes with outer translate", "[csg]") { REQUIRE(leaf.transform[3][0] == Approx(11.0f)); } +TEST_CASE("CsgEval:multmatrix() with no arguments is identity", "[csg][bugfix]") { + // parseTransform's "empty positional argument is optional" case (added + // for translate()/rotate()/scale()/mirror() — see the matching + // bugfix tests) also makes a bare multmatrix() parse, which raised a + // review question of whether that's correct or should instead error. + // Checked real OpenSCAD's source directly (src/core/TransformNode.cc's + // builtin_multmatrix()): it binds a single "m" parameter and only + // applies it `if (parameters["m"].type() == Value::Type::VECTOR)` — + // when "m" is absent (or any other non-vector value), that whole block + // is skipped with no error or warning, and the node's matrix stays at + // its constructor default of Identity(). So a bare multmatrix() being + // silently identity isn't ChiselCAD-only permissiveness — it's exactly + // real OpenSCAD's own documented behavior. + auto s = evaluate("multmatrix() cube([1,1,1]);"); + const auto& leaf = asLeaf(s.roots[0]); + const glm::mat4 I{1.0f}; + for (int c = 0; c < 4; ++c) + for (int r = 0; r < 4; ++r) + REQUIRE(leaf.transform[c][r] == Approx(I[c][r]).margin(1e-5)); +} + // --------------------------------------------------------------------------- // render() groups children under an implicit union, no transform of its own // ---------------------------------------------------------------------------