diff --git a/docs/roadmap.md b/docs/roadmap.md index 14b9319..25b5c33 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -437,6 +437,151 @@ 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 — 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=`** (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 + 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 (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** + (`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 - [ ] VS Code LSP extension (syntax highlighting, error squiggles, completions) diff --git a/src/csg/CsgEvaluator.cpp b/src/csg/CsgEvaluator.cpp index 2ebb623..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,49 +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) { - // 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)); - } - } + // `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; - // 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)); + + // 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; } - } - m_interp->restoreEnv(std::move(savedEnv)); + + 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)); + } + } + + // 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 9d10928..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' @@ -591,46 +657,57 @@ 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 '[' + while (!check(TokenKind::RParen) && !atEnd()) { + const size_t prevPos = m_pos; // guard against zero-progress infinite loops - // Parse first expression — determines range vs list form - auto first = parseExpr(); + ForClause clause; + clause.var = expect(TokenKind::Ident, "expected loop variable").text; + expect(TokenKind::Equals, "expected '=' after loop variable"); + + 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(); + clause.range.isRange = true; + clause.range.start = std::move(first); + clause.range.step = std::move(second); + clause.range.end = std::move(third); + } else { + clause.range.isRange = true; + clause.range.start = std::move(first); + clause.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, ...] + 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; + clause.range.list.push_back(parseExpr()); + } } + expect(TokenKind::RBracket, "expected ']' after range/list"); } else { - // 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()); - } + // Expression form: for (var = expr) — expr must evaluate to a vector + clause.range.isRange = false; + 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()); + + 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"); @@ -708,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) // --------------------------------------------------------------------------- @@ -1023,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 f5db43c..0fb01c4 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 // --------------------------------------------------------------------------- @@ -314,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 // --------------------------------------------------------------------------- @@ -706,6 +807,66 @@ 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: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 @@ -991,6 +1152,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 29f881f..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,6 +482,87 @@ TEST_CASE("Parser:for with brace body", "[parser]") { REQUIRE(f.children.size() == 2); } +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.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.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`) // --------------------------------------------------------------------------- @@ -626,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); @@ -759,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); @@ -1098,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.