From 5ad802220850243b617a72411ba8937883e6335d Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 12 Aug 2026 03:18:55 +0000 Subject: [PATCH] Fix issue #88: seven volumetric corpus mismatches, root-caused individually Rebuilt the v3.9 volumetric-comparison toolchain (real OpenSCAD oracle, Manifold, headless chiselcad_core) and bisected each of the seven files issue #88 flagged. All seven are real, distinct bugs, now fixed: - ifelse-tests.scad: cube()/square()'s positional `center` argument (`cube(2, true)`) was silently dropped by the parser. - primitive-inf-tests.scad: non-finite cube/sphere/cylinder dimensions were folded to 0 instead of rejected, sometimes producing a valid but wrong shape instead of empty geometry; a non-finite $fn also escaped its minimum-segment clamp via undefined int-cast behavior. - surface-simple.scad: surface()'s base was clamped to 0 instead of real OpenSCAD's `minHeight - 1`, `center` incorrectly re-centered Z, and the row-to-Y mapping was flipped relative to real OpenSCAD. - module-recursion.scad: rotate([x,y,z]) composed its three axis rotations in the reverse order from real OpenSCAD's X,Y,Z convention. - resize-tests.scad: resize()'s negative-newsize and auto= handling didn't match real OpenSCAD's actual algorithm. - intersection-tests.scad: an empty-geometry operand was silently dropped instead of nullifying the whole intersection(); excluded non-geometric statements (echo/assert) and '%'-backgrounded children from that rule so they're still just skipped. - cylinder-tests.scad: when both r and r1/r2 were given, r unconditionally overwrote both instead of only filling the unspecified slot. Re-verified against the live OpenSCAD oracle across the entire tests/data/scad/3D/features corpus (not just these seven files), which caught and fixed a regression the intersection() fix introduced in background-modifier2.scad. Filed issue #105 for a pre-existing, unrelated crash found during that sweep (linear_extrude-parameter-tests.scad). 9 new regression tests added; full chiselcad_tests suite passes (3679 assertions, 664 test cases). Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01PTiTkTcWUPE4ucANory5WA --- docs/roadmap.md | 165 +++++++++++++++++- src/csg/CsgEvaluator.cpp | 81 ++++++++- src/csg/MeshEvaluator.cpp | 80 +++++---- src/csg/PrimitiveGen.cpp | 49 +++++- src/import/SurfaceLoader.cpp | 70 ++++---- src/lang/Interpreter.cpp | 7 + src/lang/Interpreter.h | 12 ++ src/lang/Parser.cpp | 15 ++ .../headless/cylinder_ambiguous_r_r1.scad | 1 + .../headless/cylinder_fn_infinite.scad | 1 + .../headless/cylinder_infinite_r2.scad | 1 + .../fixtures/headless/cylinder_r1_0_r2_5.scad | 1 + .../headless/resize_negative_newsize.scad | 1 + .../resize_negative_newsize_auto.scad | 1 + tests/test_csg_evaluator.cpp | 63 ++++++- tests/test_headless_build.cpp | 99 +++++++++++ tests/test_parser.cpp | 16 ++ tests/test_surface_loader.cpp | 17 +- 18 files changed, 595 insertions(+), 85 deletions(-) create mode 100644 tests/fixtures/headless/cylinder_ambiguous_r_r1.scad create mode 100644 tests/fixtures/headless/cylinder_fn_infinite.scad create mode 100644 tests/fixtures/headless/cylinder_infinite_r2.scad create mode 100644 tests/fixtures/headless/cylinder_r1_0_r2_5.scad create mode 100644 tests/fixtures/headless/resize_negative_newsize.scad create mode 100644 tests/fixtures/headless/resize_negative_newsize_auto.scad diff --git a/docs/roadmap.md b/docs/roadmap.md index 927ec0b..e5d2a25 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -411,11 +411,12 @@ fixed**: this is parameter-specific (likely the `angle=` partial-revolution case given `-angle` is the worse of the two) rather than a blanket `rotate_extrude` bug. Fixed — see v3.12. -- [ ] (issue #88) `intersection-tests` (6.4%), `cylinder-tests` (13%, improved from +- [x] (issue #88) `intersection-tests` (6.4%), `cylinder-tests` (13%, improved from totally-blocked but still a real remaining gap after the harness fix above), `primitive-inf-tests` (83%), `ifelse-tests` (175%), `module-recursion`, `resize-tests` (1.5%), `surface-simple` — real - mismatches, not yet individually triaged. + mismatches, not yet individually triaged. All seven individually + root-caused and fixed — see v3.14. - [x] `assign-tests` and `intersection_for-tests` (issue #89) still produce zero valid geometry even after the harness fix — unlike the files that fix unblocked, these appear to genuinely fail in `MeshEvaluator`/`PrimitiveGen` itself @@ -820,6 +821,166 @@ available — but the root cause (both constructs being completely unrecognized, not a `MeshEvaluator`/`PrimitiveGen` tessellation bug) is confirmed, closing the specific question issue #89 asked. +## v3.14 — issue #88 (seven untriaged corpus mismatches) individually root-caused and fixed + +Rebuilt the full v3.9 volumetric-comparison toolchain (real OpenSCAD 2021.01 +oracle via `apt`, Manifold v3.5.2 from source, `chiselcad_core` headless via +`cmake -DCHISELCAD_BUILD_GUI=OFF`, `scad_to_stl`/`stl_diff` linked against +the real compiled mesh pipeline — see `tests/tools/README.md`) and bisected +each of issue #88's seven files individually, the same technique the +already-fixed `sphere-tests.scad`/`linear_extrude-tests.scad`/ +`rotate_extrude-*` cases used. All seven are now real bugs found and fixed, +not tessellation noise — each file's `sym_diff_volume` is now at +floating-point-noise level against the live oracle: + +- [x] **`ifelse-tests.scad` (175% → exact):** not an `if`/`else` bug at all + — `cube(size, center)`'s *positional* `center` argument (`cube(2, true)`) + was silently dropped. `Parser::parseParamList()` only recognized + `center=true`/`center=false` in *named* form; a bare positional `true`/ + `false` token fell into the generic `_posN` bucket, which no + `CsgEvaluator` case reads, so the cube came out uncentered. Every branch + in the corpus file happened to use this exact positional form, which is + why the volumes still summed to the same total (12 same-sized cubes) while + the symmetric difference was large (each one shifted). Fixed by checking + for a bare `true`/`false` token before falling through to the positional- + expression case. +- [x] **`primitive-inf-tests.scad` (83% → exact):** `Interpreter:: + evalNumber()`'s blanket "fold non-finite (inf/nan) to 0.0" — added + defensively, per its own comment, because "those aren't safe to hand to + Manifold" — silently turned `cylinder(r2=1/0)` into a *valid* `r2=0` cone + instead of the empty geometry real OpenSCAD renders for any non-finite + primitive dimension. Fixed at the correct layer instead of patching around + it: added a new `evalNumberPreserveNonFinite()` for cube/sphere/cylinder's + parameter resolution specifically, paired with explicit `std::isfinite()` + checks in `PrimitiveGen::generate()` that return an empty `Manifold` for a + non-finite dimension — matching real OpenSCAD's own behavior instead of + relying on Manifold's inconsistent handling of infinite inputs (some + silently "succeeded" with garbage, as `r2=inf` did; others correctly + flagged `InvalidConstruction`, which `MeshEvaluator::checkStatus()` logged + but still passed the garbage mesh through unchanged — also fixed, it now + returns an empty `Manifold` on any error status). A related, separate bug + in the same file: a non-finite `$fn` (`cylinder($fn=1/0)`) reached + `PrimitiveGen::resolveSegments()`, whose `static_cast(std::round(fn))` + is undefined behavior for an out-of-range double — empirically `INT_MAX` + on this platform, not the small/negative value that would make an + unguarded `std::max(3, ...)` clamp harmlessly — inflating the segment + count instead of clamping to real OpenSCAD's own minimum of 3. Fixed with + an explicit `!std::isfinite(fn)` check ahead of the cast. +- [x] **`surface-simple.scad` (82% → exact):** two compounding bugs in + `SurfaceLoader.cpp`, both confirmed against a live OpenSCAD export byte- + for-byte: (1) the solid's base was clamped to `min(0.0, minHeight)` + instead of real OpenSCAD's actual rule, unconditionally `minHeight - 1` + (`SurfaceNode::createGeometry()`: `min_val = data.min_value() - 1`) — a + surface with an all-non-negative heightmap (like this file's `[0,3]` + span) got a flat base at `z=0` instead of `z=-1`, losing exactly the + "one unit of skirt" real OpenSCAD always adds; (2) `center=` was also + re-centering Z, but real OpenSCAD's `center` only ever offsets X/Y + (`ox`/`oy` in the same function) — Z is left as the raw height data + regardless. A third, latent bug surfaced while fixing the first two: the + row-to-Y mapping was flipped (row 0 was treated as *max* Y) relative to + real OpenSCAD's actual convention (row 0 -> *min* Y, confirmed via an + asymmetric test grid) — harmless for this file's specific 2x2 grid + contents by coincidence of the volume check, but a real orientation bug + fixed alongside the other two (with a matching flip added to the PNG + loader, which had been relying on the wrong row mapping to *accidentally* + land in the right place before this fix, and needed its own explicit + top-row-is-max-Y flip to stay correct afterward). Triangle winding for the + top/bottom faces and the boundary-wall traversal direction both needed + re-deriving from scratch to stay outward-facing under the corrected Y + mapping (the pre-fix code was exporting a negative-volume mesh once the + row flip alone was corrected). +- [x] **`module-recursion.scad` (31% → exact):** `rotate([x,y,z])` + composed its three axis rotations in the wrong order. Real OpenSCAD + rotates about X, then Y, then Z (a point transforms as `Rz*(Ry*(Rx*p))`, + X innermost) — confirmed by isolating `rotate([40,0,0])` and + `rotate([0,0,180])` (each matched real OpenSCAD exactly alone) against + `rotate([40,0,180])` (badly mismatched combined), which only a + composition-order bug explains. `CsgEvaluator::makeMatrix()`'s three + `glm::rotate(m, angle, axis)` post-multiply calls were issued in + `rx, ry, rz` source order, which builds `Rx*Ry*Rz` (Z innermost) — the + reverse of OpenSCAD's own order, and of this same function's own + pre-existing doc comment ("Rotation order: Z first, then Y, then X + (OpenSCAD convention)"), which the code had never actually matched. + Fixed by issuing the calls in the reverse order (`rz, ry, rx`) so the + composed matrix comes out as `Rz*Ry*Rx`, matching both real OpenSCAD and + the function's own documented intent. +- [x] **`resize-tests.scad` (1.5% → exact):** `MeshEvaluator::evalResize()` + had generalized real OpenSCAD's `GeometryUtils::getResizeTransform()` + instead of porting it verbatim, and the generalization was wrong in two + ways: (1) an axis was "explicitly resized" whenever its `newsize` was + non-zero, not — as real OpenSCAD requires — *strictly positive*, so + `resize([-5,0,0])` scaled that axis by `|-5|` instead of leaving it + unresized (real OpenSCAD: any negative `newsize` component makes the + *whole* `resize()` call a no-op, confirmed live); (2) `auto=`'s broadcast + scale factor was computed as the largest *scale factor* among the + explicitly-resized axes, not — as real OpenSCAD computes it — the scale + of whichever axis has the largest *raw* `newsize` value (which may itself + default to 1.0 if that axis isn't itself `>0`); the two only coincide + when extents happen to be uniform across axes. Rewrote the scale + computation as a direct, verbatim port of the real algorithm instead of a + reimplementation. +- [x] **`intersection-tests.scad` (6.4% → exact):** a child statement that + itself produces no geometry (a bare `render();`/`linear_extrude();`/etc. + with no body of its own) evaluates to `nullptr` in `CsgEvaluator`; that + `nullptr` was silently dropped from a boolean op's child list — correct + for `union()`/`difference()`/`hull()` (dropping an empty operand changes + nothing), but wrong for `intersection()` specifically, where "intersected + with nothing" must make the *whole* intersection empty (real OpenSCAD: + `intersection() { cube(4, center=true); render(); }` renders nothing, not + the bare cube). Fixed by tracking whether any `intersection()` child came + back empty and returning `nullptr` for the whole node if so — but only + for genuinely empty *geometry* children, not statements that were never + geometry to begin with (`echo()`/`assert()`, local `x=expr;` assignments, + local module/function defs, or a `*`-disabled child all also evaluate to + `nullptr`, and the corpus file's own comment confirms real OpenSCAD + simply ignores a non-geometry `echo()` mixed into an `intersection()` + rather than nullifying it) — a new `isNonGeometricStatement()` helper + distinguishes the two cases by inspecting the child AST node directly. A + `%`-backgrounded child (excluded from the main CSG tree on purpose, also + a `nullptr` return) needed the same carve-out — caught by re-running the + *entire* `3D/features` corpus (not just this issue's seven files) after + this specific fix, which turned up a real regression this fix introduced + in `background-modifier2.scad` (`intersection() { %sphere(10); cube(15, + center=true); } ` must still intersect down to the cube alone, not become + empty just because the backgrounded sphere isn't part of the main tree) + — fixed by exempting `ModBackground` alongside `ModDisable` in the same + check. +- [x] **`cylinder-tests.scad` (13% → exact):** when both `r` and `r1` (or + `r` and `r2`) were given — real OpenSCAD warns "Cylinder parameters + ambiguous" but still renders a frustum, not nothing — `PrimitiveGen` let + `r` unconditionally overwrite *both* `r1` and `r2` whenever `r` was given, + ignoring an explicitly-given `r1`/`r2` entirely. Real OpenSCAD instead + lets the explicit `r1`/`r2` win for its own slot and only uses `r` to + fill in the *other*, unspecified slot (confirmed live: `cylinder(h=5, + r=5, r1=0, center=true)` renders the same frustum as `cylinder(h=5, r1=5, + r2=0)`, i.e. `r1` stays 0 — not a uniform `r=5` cylinder). Fixed by + checking for an explicit `r1`/`r2` key in the leaf's params before + falling back to `r`. + +All fixes verified against the real `chiselcad_tests` suite (3679 +assertions, 664 test cases, all passing, including 9 new regression tests: +a parser-level test for positional `cube`/`square` centering, two +mesh-level tests for non-finite cylinder dimensions and `$fn`, a CsgEvaluator-level +test for `rotate([x,y,z])`'s composition order, two mesh-level tests for +`resize()`'s negative-`newsize` no-op behavior, two CsgEvaluator-level tests +for `intersection()`'s empty-operand-vs-echo() distinction, and a mesh-level +test for the ambiguous `r`/`r1` precedence) and re-verified against the live +OpenSCAD oracle across the *entire* `3D/features` corpus subdirectory (72 +of its 73 files — the remaining one, `linear_extrude-parameter-tests.scad`, +crashes ChiselCAD's mesh pipeline with a heap-corruption `free(): invalid +next size (fast)` before and after this pass alike, confirmed via a clean +worktree build of the pre-this-pass commit; a real bug, but pre-existing +and unrelated to issue #88 — filed as issue #105 for separate triage), not +just the seven files this issue named — this is what caught the +`%`-modifier regression above before it landed. Every file that already matched real OpenSCAD +before this pass still does; the small number that already had a real, +different remaining gap (`child-modifier`, `for-nested-tests`, `for-tests`, +`linear_extrude-scale-zero-tests`, `linear_extrude_invisible-tests`, +`minkowski3-erosion`, `rotate_extrude-angle`, `rotate_extrude-tests`, +`scale3D-tests` — none named by issue #88) are unchanged or, for two +(`rotate-parameters`, `transform-tests` — also not issue #88's), improved +by the `rotate([x,y,z])` composition-order fix above; none regressed. + ## 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 d9b1467..2fcc799 100644 --- a/src/csg/CsgEvaluator.cpp +++ b/src/csg/CsgEvaluator.cpp @@ -282,10 +282,15 @@ CsgNodePtr CsgEvaluator::evalPrimitive(const PrimitiveNode& p, const glm::mat4& leaf.kind = CsgLeaf::Kind::Cube; // Resolve scalar params ($fn, etc.) — skip "size"/"_pos0", which may // be vectors, so they aren't coerced to 0 by the blanket evalNumber. + // Uses evalNumberPreserveNonFinite() (not evalNumber()) so a + // non-finite dimension (e.g. `cube(1/0)`) reaches PrimitiveGen as + // inf/nan rather than being silently folded to a *different*, finite + // shape (0) — PrimitiveGen explicitly checks std::isfinite() on + // these before handing them to Manifold (issue #88). for (const auto& [name, exprPtr] : p.params) { if (name == "size" || name == "_pos0") continue; - leaf.params[name] = m_interp->evalNumber(*exprPtr); + leaf.params[name] = m_interp->evalNumberPreserveNonFinite(*exprPtr); } // Named "size=" takes priority; otherwise a bare positional arg // (cube(5) or cube(v) where v is a vector variable) is "size". @@ -312,8 +317,9 @@ CsgNodePtr CsgEvaluator::evalPrimitive(const PrimitiveNode& p, const glm::mat4& // ---- sphere(r) / sphere(d) --------------------------------------------- case PrimitiveNode::Kind::Sphere: leaf.kind = CsgLeaf::Kind::Sphere; + // See the Cube case above for why evalNumberPreserveNonFinite(). for (const auto& [name, exprPtr] : p.params) - leaf.params[name] = m_interp->evalNumber(*exprPtr); + leaf.params[name] = m_interp->evalNumberPreserveNonFinite(*exprPtr); // diameter → radius: d always wins over r when both are given // (order-independent — confirmed against real OpenSCAD, which // resolves d before r regardless of argument order), not just when @@ -325,8 +331,9 @@ CsgNodePtr CsgEvaluator::evalPrimitive(const PrimitiveNode& p, const glm::mat4& // ---- cylinder(h, r) / cylinder(h=, r=/r1=/r2=/d=/d1=/d2=) -------------- case PrimitiveNode::Kind::Cylinder: leaf.kind = CsgLeaf::Kind::Cylinder; + // See the Cube case above for why evalNumberPreserveNonFinite(). for (const auto& [name, exprPtr] : p.params) - leaf.params[name] = m_interp->evalNumber(*exprPtr); + leaf.params[name] = m_interp->evalNumberPreserveNonFinite(*exprPtr); // Bare positional args: cylinder(h) / cylinder(h, r) if (!leaf.params.count("h") && leaf.params.count("_pos0")) leaf.params["h"] = leaf.params["_pos0"]; @@ -423,6 +430,25 @@ CsgNodePtr CsgEvaluator::evalPrimitive(const PrimitiveNode& p, const glm::mat4& return makeLeaf(std::move(leaf)); } +// A child statement that isn't actually a geometry statement at all — +// echo()/assert(), a local `x = expr;` assignment, or a local module/ +// function definition — always evaluates to nullptr, same as a genuine +// empty-geometry result (e.g. a bare `render();`/`minkowski();` with no +// body). intersection() (see evalBoolean below) needs to tell these apart: +// "intersected with nothing" must make the whole intersection empty, but a +// non-geometric statement mixed into an intersection() block (upstream's +// own corpus test: "Non-geometry (echo) statement as first child should be +// ignored") must simply be skipped, not treated as an empty operand. +static bool isNonGeometricStatement(const AstNode& node) { + if (std::holds_alternative(node) || + std::holds_alternative(node) || + std::holds_alternative(node)) + return true; + if (const auto* call = std::get_if(&node)) + return call->name == "echo" || call->name == "assert"; + return false; +} + // --------------------------------------------------------------------------- // Boolean — preserve the tree structure, pass xform down to children // --------------------------------------------------------------------------- @@ -458,11 +484,40 @@ CsgNodePtr CsgEvaluator::evalBoolean(const BooleanNode& b, const glm::mat4& xfor // scoped to this block: save/restore around the whole child list so they // don't leak into whatever follows this node in the enclosing scope. auto savedEnv = m_interp->snapshotEnv(); + // A child statement that itself produces no geometry (e.g. a bare + // `render();`/`linear_extrude();` with no body of its own) evaluates to + // nullptr here — for union()/difference()/hull() that's correctly a + // no-op contribution (dropping it changes nothing), but for + // intersection() specifically, "intersected with nothing" must make the + // *whole* intersection empty (confirmed against real OpenSCAD — issue + // #88's intersection-tests.scad corpus mismatch: `intersection() { + // cube(4, center=true); render(); }` renders nothing, not the bare + // cube). Silently dropping the nullptr child instead left the other + // operand un-intersected. Still evaluate every child (not short- + // circuited) so side effects like nested `assign()`s and diagnostics + // aren't skipped. + bool anyChildEmpty = false; for (const auto& child : b.children) { - if (auto c = evalNode(*child, childXform, color)) + if (auto c = evalNode(*child, childXform, color)) { bnode.children.push_back(std::move(c)); + } else if (bnode.op == CsgBoolean::Op::Intersection && + !isNonGeometricStatement(*child) && + !(astModifiers(*child) & (ModDisable | ModBackground))) { + // A '*'-disabled or '%'-backgrounded child also evaluates to + // nullptr here, same as genuinely empty geometry, but for an + // unrelated reason: it was deliberately excluded from the main + // CSG tree, not "intersected with nothing" (background- + // modifier2.scad's own corpus mismatch after the fix above + // first landed: `intersection() { %sphere(10); cube(15, + // center=true); }` must still intersect down to the cube + // alone, not become empty because the backgrounded sphere + // isn't part of the main tree). + anyChildEmpty = true; + } } m_interp->restoreEnv(std::move(savedEnv)); + if (anyChildEmpty && bnode.op == CsgBoolean::Op::Intersection) + return nullptr; return makeBoolean(std::move(bnode)); } @@ -581,9 +636,23 @@ glm::mat4 CsgEvaluator::makeMatrix(const TransformNode& t) const { 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)); + // Real OpenSCAD's rotate([x,y,z]) rotates about X, then Y, then + // Z, in that order — i.e. a point is transformed as + // Rz*(Ry*(Rx*p)), X innermost/first. glm::rotate(m, angle, axis) + // post-multiplies (m*R), so to build up that composed matrix + // (Rz*Ry*Rx) via successive post-multiplies from identity, the + // *calls* must happen in the reverse order (Z, then Y, then X) — + // confirmed against real OpenSCAD (docs/roadmap.md, issue #88's + // module-recursion.scad corpus mismatch): `rotate([40,0,0])` and + // `rotate([0,0,180])` alone each matched real OpenSCAD exactly, + // but `rotate([40,0,180])` (both axes at once) didn't — only a + // composition-order bug explains one-axis-at-a-time working + // while the combination doesn't. Applying the calls in this + // source order (rx, ry, rz) had been building Rx*Ry*Rz instead + // (Z innermost) — the reverse of OpenSCAD's own order. m = glm::rotate(m, rz, glm::vec3(0.0f, 0.0f, 1.0f)); + m = glm::rotate(m, ry, glm::vec3(0.0f, 1.0f, 0.0f)); + m = glm::rotate(m, rx, glm::vec3(1.0f, 0.0f, 0.0f)); } else { // rotate(a) / rotate(a, v) / rotate(a=..., v=...) / rotate() — // 'a' is a scalar angle in degrees (0 if absent or not a diff --git a/src/csg/MeshEvaluator.cpp b/src/csg/MeshEvaluator.cpp index f3a1d27..5a1bad2 100644 --- a/src/csg/MeshEvaluator.cpp +++ b/src/csg/MeshEvaluator.cpp @@ -4,7 +4,6 @@ #include #include #include -#include #include #include #include @@ -208,8 +207,17 @@ manifold::Manifold MeshEvaluator::checkStatus(manifold::Manifold m, const std::s chisel::lang::Diagnostic d; d.level = chisel::lang::DiagLevel::Error; d.message = context + ": invalid geometry (" + manifoldErrorName(status) + - "); result may be empty or degenerate"; + "); rendered as empty"; m_diags.push_back(std::move(d)); + // Discard the Manifold returned alongside an error status + // rather than passing it through: it isn't a best-effort partial + // result, it's whatever partially-constructed/garbage state Manifold + // was in when the operation failed (e.g. a non-finite cylinder + // radius previously came back as a finite-looking but wrong cone + // collapsed to a point — a real, silent volume bug, not just a + // cosmetic one). Real OpenSCAD renders no geometry at all for an + // invalid primitive/op, which {} matches (issue #88). + return {}; } return m; } @@ -582,41 +590,41 @@ manifold::Manifold MeshEvaluator::evalResize(const CsgResize& r, const Primitive result = result + evalNode(*r.children[i], gen); manifold::Box box = result.BoundingBox(); - const double extentX = static_cast(box.max.x - box.min.x); - const double extentY = static_cast(box.max.y - box.min.y); - const double extentZ = static_cast(box.max.z - box.min.z); - - // An axis is "explicitly resized" when newsize for it is non-zero and - // the current extent is non-degenerate; anything else defaults to an - // unchanged (1.0) scale unless auto= kicks in below. - auto explicitScale = [](double newsize, double extent) -> std::optional { - if (newsize == 0.0 || extent <= 1e-9) return std::nullopt; - return newsize / extent; + const double extent[3] = { + static_cast(box.max.x - box.min.x), + static_cast(box.max.y - box.min.y), + static_cast(box.max.z - box.min.z), }; - std::optional sxExplicit = explicitScale(r.newX, extentX); - std::optional syExplicit = explicitScale(r.newY, extentY); - std::optional szExplicit = explicitScale(r.newZ, extentZ); - - // OpenSCAD's auto=: an axis with newsize 0 and auto=true scales by the - // largest explicit scale factor among the OTHER axes, so the shape - // grows/shrinks proportionally along that axis instead of staying put; - // falls back to 1 (unchanged) if no axis was explicitly resized. - double maxExplicit = 1.0; - bool haveExplicit = false; - for (auto s : {sxExplicit, syExplicit, szExplicit}) { - if (!s) continue; - maxExplicit = haveExplicit ? std::max(maxExplicit, *s) : *s; - haveExplicit = true; - } - - auto resolveAxis = [&](std::optional explicitS, bool autoFlag) -> double { - if (explicitS) return *explicitS; - return autoFlag ? maxExplicit : 1.0; - }; - - const double sx = resolveAxis(sxExplicit, r.autoX); - const double sy = resolveAxis(syExplicit, r.autoY); - const double sz = resolveAxis(szExplicit, r.autoZ); + const double newsize[3] = {r.newX, r.newY, r.newZ}; + const bool autosize[3] = {r.autoX, r.autoY, r.autoZ}; + + // Matches real OpenSCAD's GeometryUtils::getResizeTransform() exactly + // (confirmed against a live export — docs/roadmap.md, issue #88's + // resize-tests.scad corpus mismatch: `resize([-5,0,0])` leaves the + // shape completely unresized in real OpenSCAD, not scaled by |-5|). + // Two things this codebase previously got wrong by generalizing instead + // of porting verbatim: (1) an axis is only "explicitly resized" when + // newsize is *strictly positive* (`newsize[i] > 0`), not merely + // non-zero — a negative newsize is silently ignored, not scaled by its + // magnitude; (2) `auto=`'s scale factor is the scale of whichever axis + // has the largest *raw* newsize value (which may itself default to 1.0 + // if that axis isn't itself >0), not the largest *scale factor* among + // the explicitly-resized axes — those two only coincide when extents + // happen to be uniform across axes. + int maxdim = 0; + for (int i = 1; i < 3; ++i) + if (newsize[i] > newsize[maxdim]) maxdim = i; + + double scale[3] = {1.0, 1.0, 1.0}; + for (int i = 0; i < 3; ++i) + if (newsize[i] > 0.0 && extent[i] > 1e-9) scale[i] = newsize[i] / extent[i]; + + const double autoscale = scale[maxdim]; + double newscale[3]; + for (int i = 0; i < 3; ++i) + newscale[i] = (!autosize[i] || newsize[i] > 0.0) ? scale[i] : autoscale; + + const double sx = newscale[0], sy = newscale[1], sz = newscale[2]; result = result.Scale({static_cast(sx), static_cast(sy), static_cast(sz)}); diff --git a/src/csg/PrimitiveGen.cpp b/src/csg/PrimitiveGen.cpp index 290bd5e..f03f1ca 100644 --- a/src/csg/PrimitiveGen.cpp +++ b/src/csg/PrimitiveGen.cpp @@ -121,8 +121,19 @@ double PrimitiveGen::getParam(const std::unordered_map& p, int PrimitiveGen::resolveSegments(double r, double fnOverride, double faOverride, double fsOverride) const { double fn = (fnOverride > 0.0) ? fnOverride : globalFn; - if (fn > 0.0) + if (fn > 0.0) { + // A non-finite $fn (e.g. `$fn = 1/0`) can't be meaningfully rounded + // to a segment count: casting an out-of-range double to int is + // undefined behavior, and empirically this platform's cast yields + // INT_MAX (not INT_MIN, as `std::max(3, ...)` alone would need to + // safely clamp), silently building a huge/degenerate mesh instead + // of erroring. Real OpenSCAD's own equivalent clamps to the minimum + // of 3 sides here — confirmed against a live export: + // `cylinder($fn=1/0)` renders as a 3-sided prism (5 facets total). + if (!std::isfinite(fn)) + return 3; return std::max(3, static_cast(std::round(fn))); + } double fa = (faOverride > 0.0) ? faOverride : globalFa; double fs = (fsOverride > 0.0) ? fsOverride : globalFs; @@ -146,6 +157,14 @@ manifold::Manifold PrimitiveGen::generate(const CsgLeaf& leaf) const { double x = getParam(p, "x", 1.0); double y = getParam(p, "y", 1.0); double z = getParam(p, "z", 1.0); + // A non-finite dimension (e.g. `cube(1/0)`) isn't a shape Manifold + // can construct — real OpenSCAD rejects it outright and renders no + // geometry at all (confirmed live: `cube(1/0)` exports an empty + // STL, no warning). Without this check, an infinite param sometimes + // reaches Manifold::Cube() and comes back with a non-empty but + // garbage/degenerate mesh instead (docs/roadmap.md, issue #88). + if (!std::isfinite(x) || !std::isfinite(y) || !std::isfinite(z)) + return {}; return manifold::Manifold::Cube( {static_cast(x), static_cast(y), static_cast(z)}, leaf.center); @@ -156,6 +175,10 @@ manifold::Manifold PrimitiveGen::generate(const CsgLeaf& leaf) const { // ------------------------------------------------------------------ case CsgLeaf::Kind::Sphere: { double r = getParam(p, "r", getParam(p, "_pos0", 1.0)); + // See the Cube case above: a non-finite radius (`sphere(1/0)`) + // renders as nothing in real OpenSCAD, not a degenerate mesh. + if (!std::isfinite(r)) + return {}; double fnOvr = getParam(p, "$fn", 0.0); double faOvr = getParam(p, "$fa", 0.0); double fsOvr = getParam(p, "$fs", 0.0); @@ -175,8 +198,28 @@ manifold::Manifold PrimitiveGen::generate(const CsgLeaf& leaf) const { // default to 1.0 — r2 does NOT mirror r1 (confirmed against real // OpenSCAD: cylinder(h=5, r1=5) tapers from r1=5 down to r2=1, not a // uniform r=5 cylinder — see docs/roadmap.md v3.9). - double r1 = (r >= 0.0) ? r : getParam(p, "r1", 1.0); - double r2 = (r >= 0.0) ? r : getParam(p, "r2", 1.0); + // + // When *both* r and r1 (or r and r2) are given — real OpenSCAD warns + // "Cylinder parameters ambiguous" but still renders something, not + // nothing — an explicitly-given r1/r2 wins for its own slot and `r` + // only fills in the *other*, unspecified slot; `r` does not blanket- + // override an explicit r1/r2 the way this used to unconditionally do + // (confirmed against real OpenSCAD: `cylinder(h=5, r=5, r1=0, + // center=true)` renders the same frustum volume as `cylinder(h=5, + // r1=5, r2=0)`, i.e. r1 stays 0 — not a uniform r=5 cylinder, which + // is what checking only `r >= 0.0` here used to produce regardless + // of an explicit r1/r2 — issue #88's cylinder-tests.scad corpus + // mismatch). + double r1 = p.count("r1") ? p.at("r1") : ((r >= 0.0) ? r : 1.0); + double r2 = p.count("r2") ? p.at("r2") : ((r >= 0.0) ? r : 1.0); + // See the Cube case above: a non-finite height/radius (e.g. + // `cylinder(h=10, r1=1, r2=1/0)`) renders as nothing in real + // OpenSCAD. Without this check, an infinite r2 in particular used + // to reach Manifold::Cylinder() and silently come back as a + // degenerate cone collapsed to a point instead of empty/erroring — + // a real (not just cosmetic) volume bug (issue #88). + if (!std::isfinite(h) || !std::isfinite(r1) || !std::isfinite(r2)) + return {}; double fnOvr = getParam(p, "$fn", 0.0); double faOvr = getParam(p, "$fa", 0.0); double fsOvr = getParam(p, "$fs", 0.0); diff --git a/src/import/SurfaceLoader.cpp b/src/import/SurfaceLoader.cpp index de467d4..dd56057 100644 --- a/src/import/SurfaceLoader.cpp +++ b/src/import/SurfaceLoader.cpp @@ -86,11 +86,13 @@ HeightGrid parseGrid(std::ifstream& f) { // forced to RGB by requesting STBI_rgb regardless of the source PNG's actual // channel count, so grayscale/palette/RGBA inputs all take the same path), // linearly scaled from [0, 255] to [0, 100] — i.e. black = 0, white = 100. -// Alpha, if present, is ignored (not requested). Image row 0 (the top row of -// pixels) maps to grid row 0, matching the .dat format's own "first line is -// the far/max-Y edge" convention (see loadSurfaceMesh's row/col-to-XY -// mapping below) so a PNG and an equivalent hand-written .dat produce the -// same orientation. +// Alpha, if present, is ignored (not requested). loadSurfaceMesh's row index +// maps directly to Y (row 0 -> min Y), matching real OpenSCAD's own .dat +// convention (confirmed against a live export — see the comment there), so +// to keep a PNG heightmap the same way up as the equivalent hand-written +// .dat, image row 0 (the top row of pixels) must land at *max* Y: flip +// during storage into grid row (height-1-r), the same +// `data[x + width*(height-1-y)]` flip SurfaceNode::convert_image() does. HeightGrid pngToGrid(const std::filesystem::path& path) { HeightGrid grid; @@ -126,7 +128,7 @@ HeightGrid pngToGrid(const std::filesystem::path& path) { grid.rows.resize(static_cast(height)); for (int r = 0; r < height; ++r) { - std::vector& row = grid.rows[static_cast(r)]; + std::vector& row = grid.rows[static_cast(height - 1 - r)]; row.resize(static_cast(width)); for (int c = 0; c < width; ++c) { const unsigned char* px = pixels + (static_cast(r) * width + c) * 3; @@ -174,25 +176,31 @@ RawSurfaceMesh loadSurfaceMesh(const std::filesystem::path& path, bool center, b // point that was the original maximum) — only the non-inverted case can // have a negative effective minimum. const double effMinH = invert ? 0.0 : minH; - const double effMaxH = invert ? (maxH - minH) : maxH; - // Base at 0 in the common case (all-non-negative heights); if the data - // dips below 0, drop the base to match so the solid never folds back - // through its own top surface. - const double baseZ = std::min(0.0, effMinH); + // Real OpenSCAD's base is always exactly one unit below the data's own + // minimum (SurfaceNode::createGeometry(): `min_val = data.min_value() - + // 1`), never clamped to 0 and never affected by `center` — confirmed + // against a live OpenSCAD export: surface-simple.dat's heights span + // [0,3], and the exported STL's flat bottom face sits at z=-1, not z=0. + // `center` only ever offsets X/Y (`ox`/`oy` in that same function); Z is + // left exactly as the raw data (plus this base), never centered either. + const double bottomZ = effMinH - 1.0; const double xOff = center ? -static_cast(numCols - 1) / 2.0 : 0.0; const double yOff = center ? -static_cast(numRows - 1) / 2.0 : 0.0; - const double zOff = center ? -(baseZ + effMaxH) / 2.0 : 0.0; - const double bottomZ = baseZ + zOff; auto heightAt = [&](std::size_t r, std::size_t c) { double h = grid.rows[r][c]; - return (invert ? (maxH - h) : h) + zOff; + return invert ? (maxH - h) : h; }; auto xAt = [&](std::size_t c) { return static_cast(c) + xOff; }; auto yAt = [&](std::size_t r) { - return static_cast(numRows - 1 - r) + yOff; + // Real OpenSCAD maps the data's row index directly to Y (row 0 -> + // min Y, last row -> max Y) — confirmed against a live OpenSCAD + // export of an asymmetric grid. Not flipped, despite this file's + // PNG loader comment above claiming a "first row = far/max-Y edge" + // convention for .dat too; that claim was itself wrong. + return static_cast(r) + yOff; }; const std::size_t gridN = numRows * numCols; @@ -230,33 +238,35 @@ RawSurfaceMesh loadSurfaceMesh(const std::filesystem::path& path, bool center, b // Top surface (+Z-facing) and bottom (-Z-facing — reversed winding), // one quad (2 triangles) per grid cell. Winding derived so that, since - // grid row r maps to y = numRows-1-r (row 0 = max Y), [topIdx(r,c), - // topIdx(r+1,c), topIdx(r,c+1)] has an outward (+Z) normal. + // grid row r now maps directly to y = r (row 0 = min Y — see yAt() + // above), [topIdx(r,c), topIdx(r,c+1), topIdx(r+1,c)] has an outward + // (+Z) normal: (C-A)x(B-A) with A=(c,r), B=(c,r+1), C=(c+1,r) in (x,y) + // is (1,0)x(0,1) = +1 (CCW from +Z). for (std::size_t r = 0; r + 1 < numRows; ++r) { for (std::size_t c = 0; c + 1 < numCols; ++c) { - pushTri(topIdx(r, c), topIdx(r + 1, c), topIdx(r, c + 1)); - pushTri(topIdx(r + 1, c), topIdx(r + 1, c + 1), topIdx(r, c + 1)); + pushTri(topIdx(r, c), topIdx(r, c + 1), topIdx(r + 1, c)); + pushTri(topIdx(r + 1, c), topIdx(r, c + 1), topIdx(r + 1, c + 1)); - pushTri(bottomIdx(r, c), bottomIdx(r, c + 1), bottomIdx(r + 1, c)); - pushTri(bottomIdx(r + 1, c), bottomIdx(r, c + 1), bottomIdx(r + 1, c + 1)); + pushTri(bottomIdx(r, c), bottomIdx(r + 1, c), bottomIdx(r, c + 1)); + pushTri(bottomIdx(r + 1, c), bottomIdx(r + 1, c + 1), bottomIdx(r, c + 1)); } } // Side walls: walk the grid's outer boundary counter-clockwise (as seen - // from +Z) — front edge (r=numRows-1) toward +X, right edge (c=numCols-1) - // toward +Y, back edge (r=0) toward -X, left edge (c=0) toward -Y — and - // connect each edge's top/bottom vertices with outward-facing winding. - // Each corner is added exactly once (by whichever segment reaches it - // first); the loop closes via `(i+1) % boundary.size()`. + // from +Z) — near edge (r=0) toward +X, right edge (c=numCols-1) + // toward +Y, far edge (r=numRows-1) toward -X, left edge (c=0) toward -Y + // — and connect each edge's top/bottom vertices with outward-facing + // winding. Each corner is added exactly once (by whichever segment + // reaches it first); the loop closes via `(i+1) % boundary.size()`. std::vector> boundary; boundary.reserve(numBoundaryEdges); for (std::size_t c = 0; c < numCols; ++c) - boundary.emplace_back(numRows - 1, c); - for (std::size_t r = numRows - 1; r-- > 0;) + boundary.emplace_back(0, c); + for (std::size_t r = 1; r < numRows; ++r) boundary.emplace_back(r, numCols - 1); for (std::size_t c = numCols - 1; c-- > 0;) - boundary.emplace_back(0, c); - for (std::size_t r = 1; r + 1 < numRows; ++r) + boundary.emplace_back(numRows - 1, c); + for (std::size_t r = numRows - 1; r-- > 1;) boundary.emplace_back(r, 0); for (std::size_t i = 0; i < boundary.size(); ++i) { diff --git a/src/lang/Interpreter.cpp b/src/lang/Interpreter.cpp index 9d640b1..da2179b 100644 --- a/src/lang/Interpreter.cpp +++ b/src/lang/Interpreter.cpp @@ -668,6 +668,13 @@ double Interpreter::evalNumber(const ExprNode& expr) { return 0.0; } +double Interpreter::evalNumberPreserveNonFinite(const ExprNode& expr) { + Value v = evaluate(expr); + if (v.isNumber()) return v.asNumber(); + if (v.isBool()) return v.asBool() ? 1.0 : 0.0; + return 0.0; +} + // --------------------------------------------------------------------------- // evalVec3 — evaluate and return first three elements as doubles // --------------------------------------------------------------------------- diff --git a/src/lang/Interpreter.h b/src/lang/Interpreter.h index ba0c185..983ea4e 100644 --- a/src/lang/Interpreter.h +++ b/src/lang/Interpreter.h @@ -83,6 +83,18 @@ class Interpreter { // Convenience: evaluate and coerce to double (undef → 0.0). double evalNumber(const ExprNode& expr); + // Like evalNumber(), but preserves inf/nan instead of folding them to + // 0.0 — matching real OpenSCAD, which lets arithmetic on primitive + // dimensions (e.g. `cylinder(r2=1/0)`) propagate inf/nan through to the + // geometry generator, which then rejects the whole primitive as invalid + // (renders nothing) rather than silently substituting a *different*, + // finite shape (e.g. 1/0 folded to 0 turns an infinite radius into a + // valid zero radius). Only safe for consumers that explicitly check + // std::isfinite() before handing the value to Manifold — currently + // PrimitiveGen's cube/sphere/cylinder parameter resolution in + // CsgEvaluator (see docs/roadmap.md, issue #88). + double evalNumberPreserveNonFinite(const ExprNode& expr); + // Evaluate a VectorLit and return the first three elements as doubles. std::array evalVec3(const ExprNode& expr); diff --git a/src/lang/Parser.cpp b/src/lang/Parser.cpp index 84d861d..d812855 100644 --- a/src/lang/Parser.cpp +++ b/src/lang/Parser.cpp @@ -787,6 +787,21 @@ void Parser::parseParamList(std::unordered_map& params, continue; } + // Positional `true`/`false` — every one of this function's callers + // (cube/square's `center`) only ever takes a bare boolean literal + // positionally for the `center` flag, so a literal `true`/`false` + // token here is unambiguously that argument (mirrors the named + // `center=true`/`center=false` handling above), not a numbered + // positional param. Without this, `cube(2, true)` silently dropped + // the `true` into `_pos1` — never read by any CsgEvaluator case — + // so the cube came out uncentered. + if (check(TokenKind::True) || check(TokenKind::False)) { + center = (peek().kind == TokenKind::True); + advance(); + match(TokenKind::Comma); + continue; + } + // Positional number/expression — indexed _pos0, _pos1, ... so that // multiple positional args don't collide into a single key. if (!check(TokenKind::RParen)) { diff --git a/tests/fixtures/headless/cylinder_ambiguous_r_r1.scad b/tests/fixtures/headless/cylinder_ambiguous_r_r1.scad new file mode 100644 index 0000000..a0cd4c0 --- /dev/null +++ b/tests/fixtures/headless/cylinder_ambiguous_r_r1.scad @@ -0,0 +1 @@ +cylinder(h=5, r=5, r1=0, center=true); diff --git a/tests/fixtures/headless/cylinder_fn_infinite.scad b/tests/fixtures/headless/cylinder_fn_infinite.scad new file mode 100644 index 0000000..c127a0c --- /dev/null +++ b/tests/fixtures/headless/cylinder_fn_infinite.scad @@ -0,0 +1 @@ +cylinder($fn=1/0); diff --git a/tests/fixtures/headless/cylinder_infinite_r2.scad b/tests/fixtures/headless/cylinder_infinite_r2.scad new file mode 100644 index 0000000..939f651 --- /dev/null +++ b/tests/fixtures/headless/cylinder_infinite_r2.scad @@ -0,0 +1 @@ +cylinder(h=10, r1=1, r2=1/0); diff --git a/tests/fixtures/headless/cylinder_r1_0_r2_5.scad b/tests/fixtures/headless/cylinder_r1_0_r2_5.scad new file mode 100644 index 0000000..d98a472 --- /dev/null +++ b/tests/fixtures/headless/cylinder_r1_0_r2_5.scad @@ -0,0 +1 @@ +cylinder(h=5, r1=0, r2=5, center=true); diff --git a/tests/fixtures/headless/resize_negative_newsize.scad b/tests/fixtures/headless/resize_negative_newsize.scad new file mode 100644 index 0000000..2d76104 --- /dev/null +++ b/tests/fixtures/headless/resize_negative_newsize.scad @@ -0,0 +1 @@ +resize([-5,0,0]) cube(); diff --git a/tests/fixtures/headless/resize_negative_newsize_auto.scad b/tests/fixtures/headless/resize_negative_newsize_auto.scad new file mode 100644 index 0000000..ad5b606 --- /dev/null +++ b/tests/fixtures/headless/resize_negative_newsize_auto.scad @@ -0,0 +1 @@ +resize([-5,0,0],auto=3) cube(); diff --git a/tests/test_csg_evaluator.cpp b/tests/test_csg_evaluator.cpp index 6944b79..e84f622 100644 --- a/tests/test_csg_evaluator.cpp +++ b/tests/test_csg_evaluator.cpp @@ -5,6 +5,7 @@ #include #include +#include #include using namespace chisel::lang; @@ -364,6 +365,33 @@ TEST_CASE("CsgEval:rotate([x,y,z] vector) ignores a 'v' axis argument", "[csg][b REQUIRE(a.transform[c][r] == Approx(b.transform[c][r]).margin(1e-5)); } +TEST_CASE("CsgEval:rotate([x,y,z]) composes X, then Y, then Z, matching real OpenSCAD", + "[csg][bugfix]") { + // Confirmed against real OpenSCAD (docs/roadmap.md, issue #88's + // module-recursion.scad corpus mismatch): rotate([x,y,z]) rotates a + // point about X first, then Y, then Z — i.e. Rz*(Ry*(Rx*p)). A single + // non-zero axis alone can't distinguish composition order (nothing to + // compose), so this uses two: rotate([40,0,180]) applied to (0,1,0) + // must equal Rz(180) applied to (Rx(40) applied to (0,1,0)), not the + // reverse (Rx applied to Rz's result) — the bug this regresses had the + // three glm::rotate() calls composing in the opposite (Z-first) order. + auto s = evaluate("rotate([40,0,180]) cube([1,1,1]);"); + const auto& leaf = asLeaf(s.roots[0]); + glm::vec4 p = leaf.transform * glm::vec4(0.0f, 1.0f, 0.0f, 0.0f); + + constexpr double kPi = 3.14159265358979323846; + const double rx = 40.0 * kPi / 180.0; + const double cosRx = std::cos(rx), sinRx = std::sin(rx); + // Rx(40) * (0,1,0) = (0, cos40, sin40); Rz(180) then negates x and y. + const float expectedX = 0.0f; + const float expectedY = static_cast(-cosRx); + const float expectedZ = static_cast(sinRx); + + REQUIRE(p.x == Approx(expectedX).margin(1e-4)); + REQUIRE(p.y == Approx(expectedY).margin(1e-4)); + REQUIRE(p.z == Approx(expectedZ).margin(1e-4)); +} + // --------------------------------------------------------------------------- // multmatrix() folds the given 4x4 rows straight into the leaf transform // --------------------------------------------------------------------------- @@ -634,6 +662,34 @@ TEST_CASE("CsgEval:empty union has no children", "[csg]") { REQUIRE(b.children.empty()); } +TEST_CASE("CsgEval:intersection with a no-geometry operand (render()) yields nothing", + "[csg][bugfix]") { + // Confirmed against real OpenSCAD (issue #88's intersection-tests.scad + // corpus mismatch): `intersection() { cube(4, center=true); render(); }` + // renders no geometry at all, not the bare cube. A childless render() + // evaluates to nullptr (no geometry of its own) — before this fix, that + // nullptr child was silently dropped from the intersection's child + // list instead of making the whole intersection empty, so it fell back + // to returning the sole remaining cube operand unintersected. + auto s = evaluate("intersection() { cube(4, center=true); render(); }"); + REQUIRE(s.roots.empty()); +} + +TEST_CASE("CsgEval:intersection ignores a non-geometry echo() statement, not the whole result", + "[csg][bugfix]") { + // Confirmed against real OpenSCAD (the same corpus file's own comment: + // "Non-geometry (echo) statement as first child should be ignored"). + // echo() also evaluates to nullptr, like render() above, but for the + // opposite reason — it was never a geometry statement to begin with — + // so it must be skipped rather than nullifying the intersection. + auto s = evaluate("intersection() { echo(\"hi\"); cube([5,5,5], center=true); " + "cylinder(r=2, h=20, center=true); }"); + REQUIRE(s.roots.size() == 1); + const auto& b = asBool(s.roots[0]); + REQUIRE(b.op == CsgBoolean::Op::Intersection); + REQUIRE(b.children.size() == 2); // echo() itself contributes no child +} + // --------------------------------------------------------------------------- // Transform wrapping a boolean passes the matrix to all leaf descendants // --------------------------------------------------------------------------- @@ -1726,9 +1782,10 @@ TEST_CASE("CsgEval:surface() with file=, center=, and invert= named arguments", REQUIRE(s.roots.size() == 1); const auto& leaf = asLeaf(s.roots[0]); REQUIRE(leaf.kind == CsgLeaf::Kind::Mesh); - // Center cell (grid index 4): was the peak (z=5), inverted -> 0, then - // centered around the [0,5] span -> shifted by -2.5. - REQUIRE(leaf.meshPositions[4].z == Approx(-2.5)); + // Center cell (grid index 4): was the peak (z=5), inverted -> 0. `center` + // only ever offsets X/Y in real OpenSCAD, never Z (see SurfaceLoader.cpp), + // so the inverted low point stays at z=0. + REQUIRE(leaf.meshPositions[4].z == Approx(0.0)); } TEST_CASE("CsgEval:surface() honors an outer transform and color", "[csg][tier-e]") { diff --git a/tests/test_headless_build.cpp b/tests/test_headless_build.cpp index 30aafbf..98c1824 100644 --- a/tests/test_headless_build.cpp +++ b/tests/test_headless_build.cpp @@ -97,6 +97,105 @@ TEST_CASE("runBuild: cylinder r2 defaults to 1.0 independently of r1, not mirror CHECK(result.volume == Approx(kExpectedFrustumVolume).margin(1.0)); } +TEST_CASE("runBuild: a non-finite cylinder radius renders as empty, not a garbage cone", + "[headless][v39][bugfix]") { + // cylinder(h=10, r1=1, r2=1/0) — confirmed against real OpenSCAD (which + // renders no geometry at all for any non-finite primitive dimension, + // matching its own general "invalid parameter -> no geometry" behavior + // for cube/cylinder/sphere — see docs/roadmap.md, issue #88's + // primitive-inf-tests.scad corpus mismatch). Before this fix, `1/0` was + // folded to 0.0 well before reaching PrimitiveGen (Interpreter:: + // evalNumber()'s blanket non-finite -> 0.0 coercion), turning an + // infinite r2 into a *valid* r2=0 cone with real, nonzero volume instead + // of empty geometry — a silent, wrong-shape bug, not just a missing + // error. + chisel::csg::MeshCache cache; + BuildResult result = runBuild(fixture("headless/cylinder_infinite_r2.scad"), {}, {}, cache); + CHECK(result.volume == Approx(0.0).margin(1e-9)); + CHECK(result.triCount == 0); +} + +TEST_CASE("runBuild: a non-finite $fn clamps to the minimum 3 segments", + "[headless][v39][bugfix]") { + // cylinder($fn=1/0) — confirmed against real OpenSCAD: renders as a + // 3-sided prism (Facets: 5 in its own stats output), volume + // 1.5*sin(120 deg) ~= 1.299038 for the default r=1,h=1. Before this fix, + // two compounding bugs inflated this: (1) Interpreter::evalNumber() + // folded the non-finite $fn to 0.0, which PrimitiveGen::resolveSegments + // treats as "no override" and falls back to the *global* auto-resolution + // segment count instead of clamping to 3; (2) even with the raw inf + // value reaching resolveSegments, `static_cast(std::round(inf))` is + // undefined behavior that empirically yields INT_MAX on this platform + // (not a small/negative value some other platform's UB might produce), + // so an unguarded std::max(3, ...) wouldn't have clamped it either. + chisel::csg::MeshCache cache; + BuildResult result = runBuild(fixture("headless/cylinder_fn_infinite.scad"), {}, {}, cache); + REQUIRE(result.ok()); + constexpr double kExpectedVolume = 1.5 * 0.86602540378; // 1.5*sin(120 deg) + CHECK(result.volume == Approx(kExpectedVolume).margin(1e-6)); + CHECK(result.triCount == 8); // 3 sides * 2 tris + 2 triangular caps +} + +TEST_CASE("runBuild: resize() with a negative newsize component leaves that axis unchanged", + "[headless][v39][bugfix]") { + // resize([-5,0,0]) cube() — confirmed against real OpenSCAD + // (GeometryUtils::getResizeTransform(): `if (newsize[i] > 0) scale[i] = + // newsize[i]/bbox.sizes()[i]` — a *strictly positive* check, so a + // negative newsize leaves that axis's scale at the default 1.0, same as + // an axis omitted entirely) — the whole resize() here is a no-op, + // volume stays 1 (the unit cube unchanged). Before this fix, + // MeshEvaluator::evalResize() only excluded exactly-zero newsize values + // from being "explicit", so -5 was treated as a real (negative) scale + // factor, giving a wrong, nonzero-magnitude resize (issue #88). + chisel::csg::MeshCache cache; + BuildResult result = runBuild(fixture("headless/resize_negative_newsize.scad"), {}, {}, cache); + REQUIRE(result.ok()); + CHECK(result.volume == Approx(1.0).margin(1e-6)); +} + +TEST_CASE("runBuild: resize() auto= with an all-negative-or-zero newsize is also a no-op", + "[headless][v39][bugfix]") { + // resize([-5,0,0],auto=3) cube() — confirmed against real OpenSCAD: + // still a complete no-op (volume 1), not the 5^3=125 the pre-fix + // `maxExplicit` logic gave by treating auto='s scale factor as the + // largest *scale* among explicitly-resized axes (which included the + // negative -5/1 = -5 factor) rather than real's actual rule: the scale + // of whichever axis has the largest *raw* newsize value, which here is + // one of the two 0-valued axes (0 > -5), and 0 isn't > 0 either, so its + // own scale — and therefore the auto-scale broadcast to every axis — is + // the default 1.0. + chisel::csg::MeshCache cache; + BuildResult result = + runBuild(fixture("headless/resize_negative_newsize_auto.scad"), {}, {}, cache); + REQUIRE(result.ok()); + CHECK(result.volume == Approx(1.0).margin(1e-6)); +} + +TEST_CASE("runBuild: an explicit r1 wins over ambiguous r for its own slot, not vice versa", + "[headless][v39][bugfix]") { + // cylinder(h=5, r=5, r1=0, center=true) — confirmed against real + // OpenSCAD (which warns "Cylinder parameters ambiguous" but still + // renders a frustum, not nothing): r1 stays 0 and r2 takes r's value + // (5), the same frustum as cylinder(h=5, r1=5, r2=0) (frustum volume is + // symmetric in r1/r2, so the two are numerically indistinguishable — + // both give the same expected volume below). Before this fix, + // PrimitiveGen unconditionally set *both* r1 and r2 to r's value + // whenever r was given (r >= 0.0), ignoring the explicit r1=0 entirely + // and rendering a uniform r=5 cylinder instead of a frustum — a much + // larger, wrong volume (issue #88). + chisel::csg::MeshCache cache; + BuildResult result = runBuild(fixture("headless/cylinder_ambiguous_r_r1.scad"), {}, {}, cache); + REQUIRE(result.ok()); + // Compare against the equivalent unambiguous cylinder(h=5, r1=0, r2=5) — + // same tessellation, so this is an exact match rather than an idealized + // continuous-frustum formula subject to its own tessellation-loss + // margin. A uniform r=5 cylinder (the pre-fix bug) gives pi*25*5 ≈ + // 392.7 — clearly distinct from either. + BuildResult unambiguous = runBuild(fixture("headless/cylinder_r1_0_r2_5.scad"), {}, {}, cache); + REQUIRE(unambiguous.ok()); + CHECK(result.volume == Approx(unambiguous.volume).margin(1e-6)); +} + TEST_CASE("runBuild: linear_extrude()'s default height is 100, not 1", "[headless][v39][bugfix]") { // Confirmed against real OpenSCAD's STL output for diff --git a/tests/test_parser.cpp b/tests/test_parser.cpp index f0a1520..96d6055 100644 --- a/tests/test_parser.cpp +++ b/tests/test_parser.cpp @@ -93,6 +93,22 @@ TEST_CASE("Parser:cube centered", "[parser]") { REQUIRE(asPrim(r.roots[0]).center == true); } +TEST_CASE("Parser:cube centered via positional bool", "[parser]") { + // cube(size, center) — OpenSCAD's positional form (issue #88's + // ifelse-tests.scad corpus mismatch used this exact shape). A bare + // `true`/`false` positional arg used to fall into the generic + // `_posN` bucket, which no CsgEvaluator case reads, so the cube came + // out uncentered. + auto r1 = parse("cube(2, true);"); + REQUIRE(asPrim(r1.roots[0]).center == true); + + auto r2 = parse("cube(2, false);"); + REQUIRE(asPrim(r2.roots[0]).center == false); + + auto r3 = parse("square(2, true);"); + REQUIRE(asPrim(r3.roots[0]).center == true); +} + TEST_CASE("Parser:sphere with r", "[parser]") { auto r = parse("sphere(r = 5);"); auto& p = asPrim(r.roots[0]); diff --git a/tests/test_surface_loader.cpp b/tests/test_surface_loader.cpp index 230b05d..847fb05 100644 --- a/tests/test_surface_loader.cpp +++ b/tests/test_surface_loader.cpp @@ -19,8 +19,10 @@ static std::filesystem::path fixture(const std::string& relPath) { // 0 0 0 // 0 5 0 // 0 0 0 -// Row 0 -> y=2 (far edge), row 2 -> y=0 (near edge); the peak sits at -// grid(r=1,c=1) -> (x=1, y=1, z=5). +// Row 0 -> y=0 (near edge), row 2 -> y=2 (far edge) — real OpenSCAD maps a +// data row directly to Y (SurfaceNode::createGeometry(), confirmed against +// a live export of an asymmetric grid); the peak sits at grid(r=1,c=1) -> +// (x=1, y=1, z=5) either way since this fixture is row-symmetric. TEST_CASE("SurfaceLoader:loads a valid grid and builds a closed solid", "[surface-loader][tier-e]") { @@ -40,7 +42,7 @@ TEST_CASE("SurfaceLoader:loads a valid grid and builds a closed solid", REQUIRE(peak.z == Approx(5.0)); } -TEST_CASE("SurfaceLoader:center shifts the footprint and Z extent onto the origin", +TEST_CASE("SurfaceLoader:center shifts the X/Y footprint but never Z", "[surface-loader][tier-e]") { auto mesh = loadSurfaceMesh(fixture("surface/simple.dat"), /*center=*/true, /*invert=*/false); REQUIRE(mesh.error.empty()); @@ -48,7 +50,12 @@ TEST_CASE("SurfaceLoader:center shifts the footprint and Z extent onto the origi const auto& peak = mesh.positions[4]; // grid(1,1), the center cell REQUIRE(peak.x == Approx(0.0)); REQUIRE(peak.y == Approx(0.0)); - REQUIRE(peak.z == Approx(2.5)); // (0 + 5) span, centered -> [-2.5, 2.5] + // Real OpenSCAD's `center` only ever offsets X/Y (SurfaceNode:: + // createGeometry()'s `ox`/`oy`) — Z is left as the raw data value + // regardless of `center`, confirmed against a live OpenSCAD export + // (surface-simple.scad's centered call still has its top surface at + // the raw heights, not re-based around 0). + REQUIRE(peak.z == Approx(5.0)); } TEST_CASE("SurfaceLoader:invert flips heights about the grid's own maximum", @@ -113,7 +120,7 @@ TEST_CASE("SurfaceLoader:PNG heightmap honors center and invert like .dat", auto centered = loadSurfaceMesh(fixture("surface/simple.png"), /*center=*/true, /*invert=*/false); REQUIRE(centered.error.empty()); - REQUIRE(centered.positions[4].z == Approx(50.0)); // [0,100] span, centered -> peak at +50 + REQUIRE(centered.positions[4].z == Approx(100.0)); // center never affects Z, only X/Y auto inverted = loadSurfaceMesh(fixture("surface/simple.png"), /*center=*/false, /*invert=*/true);