From 38f9b7aecd69279562d620643329581e47d04a43 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 05:45:57 +0000 Subject: [PATCH 1/2] Fix rotate_extrude() volume mismatches (issue #87) Built a live OpenSCAD 2021.01 + Manifold v3.5.2 oracle in this session (tests/tools/README.md's documented recipe) and bisected both corpus files construct-by-construct against it. Found and fixed five real bugs in MeshEvaluator::evalExtrusion's rotate_extrude branch: - A profile entirely on the -X side was rejected outright; only a straddling profile is actually invalid. Fixed by mirroring such a profile onto +X (with winding correction) before calling Revolve(), then rotating the result 180 degrees about Z to restore the original sweep. - A negative angle= produced inside-out (negative-volume) geometry, since Manifold::Revolve() only winds correctly for a positive sweep. Fixed by always sweeping with abs(angle) and mirroring the result across the XZ plane for an originally-negative angle instead. - Segment count used a fixed proxy radius (10) instead of the profile's own X-extent, matching real OpenSCAD's Calc::get_fragments_from_r(). - A partial sweep was tessellated at full-circle density instead of being scaled down by angle/360 like real OpenSCAD does (floored to a minimum of 3, not 1: Manifold::Revolve() silently ignores an explicit circularSegments <= 2 and falls back to its own auto-quality count, which produced zero triangles for a small enough angle). - angle='s NaN/Infinity handling (should mean "full circle") was lost before MeshEvaluator ever saw it, since the generic evalNumber() path collapses non-finite numbers to 0.0 -- indistinguishable from a literal angle=0 (a real, distinct "no geometry" case). Fixed by special-casing "angle" in CsgEvaluator::evalExtrusion. rotate_extrude-tests.scad: 27% -> 13.0% relative volume error (now an exact volume match; the remainder is an extreme $fn=1 edge case's rotational-phase misalignment, not chased further this pass). rotate_extrude-angle.scad: 71% -> 16.7% (every real partial/negative- angle/edge-case construct now matches to floating-point noise in isolation; the remainder is a 2021.01-vs-master oracle-version ambiguity around a deprecated positional argument, left open per v3.9's own documented caution about not guessing which oracle to match). Full details, including two phase-alignment leads investigated and deliberately not pursued (one measurably regressed already-passing cases), in docs/roadmap.md's new v3.12 section. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01Qt1mwHFdm3v3JMMm3VC65j --- docs/roadmap.md | 123 +++++++++++++++++- src/csg/CsgEvaluator.cpp | 12 ++ src/csg/MeshEvaluator.cpp | 96 ++++++++++++-- .../headless/rotate_extrude_angle_zero.scad | 1 + .../rotate_extrude_negative_angle.scad | 1 + .../headless/rotate_extrude_negative_x.scad | 1 + tests/test_csg_evaluator.cpp | 30 +++++ tests/test_headless_build.cpp | 53 ++++++++ 8 files changed, 302 insertions(+), 15 deletions(-) create mode 100644 tests/fixtures/headless/rotate_extrude_angle_zero.scad create mode 100644 tests/fixtures/headless/rotate_extrude_negative_angle.scad create mode 100644 tests/fixtures/headless/rotate_extrude_negative_x.scad diff --git a/docs/roadmap.md b/docs/roadmap.md index 7d371e1..9cd440e 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -405,12 +405,12 @@ fixed**: The same applies to `segments=` (also unrecognized by 2021.01). This caveat likely explains some fraction of the other still-open mismatches below too, not just `linear_extrude`. -- [ ] **`rotate_extrude` volume mismatches** (issue #87): `rotate_extrude-tests` (27%), +- [x] **`rotate_extrude` volume mismatches** (issue #87): `rotate_extrude-tests` (27%), `rotate_extrude-angle` (71%) — contrast with `rotate_extrude-touch-vertex`/ `rotate_extrude-touch-edge`, which pass at floating-point-noise level, so 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. + `rotate_extrude` bug. Fixed — see v3.12. - [ ] (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%), @@ -627,6 +627,125 @@ checks only. bug triage both did) is worth doing before writing a fix for the wrong code path. +## v3.12 — issue #87 (`rotate_extrude()` volume mismatches) fixed + +Built a live oracle for this pass rather than reasoning from source alone: +`apt-get install openscad` (2021.01, matching every prior pass) plus a +standalone Manifold v3.5.2 build (`tests/tools/README.md`'s documented +recipe), then `cmake -DCHISELCAD_BUILD_GUI=OFF -DCMAKE_PREFIX_PATH=...` to +get `chiselcad_core`/`chiselcad_tests` linked against real Manifold, and +`scad_to_stl`/`stl_diff` built straight against the resulting +`libchiselcad_core.a`. Cross-checked every fix below against 2021.01's own +`src/rotateextrude.cc`/`GeometryEvaluator.cc` source (cloned at the +`openscad-2021.01` tag) in addition to the live binary, since the corpus +files are cloned from `openscad/openscad`'s current `master` and exercise +some constructs 2021.01 doesn't actually support the way `master` does (see +v3.9's oracle-version caveat) — bisected each `rotate_extrude(...)` call +from both corpus files into its own `.scad` file and compared against the +live 2021.01 binary throughout, not just the whole-file totals. Found and +fixed five real bugs in `MeshEvaluator::evalExtrusion`'s `rotate_extrude` +branch, all in `src/csg/MeshEvaluator.cpp` (plus one in +`src/csg/CsgEvaluator.cpp::evalExtrusion`): + +- [x] **A profile entirely on the -X side was rejected outright.** The + axis-crossing check flagged *any* point with `x<0`, but real OpenSCAD + (`GeometryEvaluator.cc`'s `rotatePolygon`) only rejects a profile that + actually *straddles* the axis (`min_x<0 && max_x>0`) — a profile fully on + -X is valid, it just revolves mirrored back onto +X. Since Manifold's own + `Revolve()` clips away `x<0` input rather than mirroring it, fixed by + detecting this case, mirroring the profile onto +X (negating each point's + `x` and reversing polygon winding, since negating `x` alone flips it), + revolving normally, then rotating the *result* 180° about Z to reproduce + the original sweep. Confirmed against `rotate_extrude-tests.scad`'s + "Object in negative X" case (`rel_error` 1→0). +- [x] **A negative `angle=` produced inside-out (negative-volume) geometry.** + `Manifold::Revolve()` winds its side faces correctly for a positive sweep + but backwards for a literal negative `revolveDegrees` — confirmed by + isolating `rotate_extrude-angle.scad`'s `angle=-5`/`angle=-45` cases + individually (positive-angle siblings matched immediately; negative ones + came back with `volume_b` exactly negated). Fixed by always sweeping with + `Revolve(polys, segs, abs(angle))` and mirroring the *result* across the + XZ plane (`Mirror({0,1,0})`, Y→-Y) for an originally-negative angle + instead — algebraically `x*cos(-a)=x*cos(a)`, `x*sin(-a)=-x*sin(a)`, i.e. + exactly the +a sweep with Y negated — and unlike `Revolve()`, + `Mirror()`/`Transform()` keep winding correct on their own. +- [x] **Segment count used a fixed proxy radius (10) instead of the + profile's own extent.** `resolveSegments()` was called with a hardcoded + `10.0` "good enough" placeholder regardless of the actual profile's + distance from the axis, rather than real OpenSCAD's + `Calc::get_fragments_from_r(max_x-min_x, ...)` (the profile's full X-span, + confirmed from 2021.01's own `rotateextrude.cc`/`GeometryEvaluator.cc` + source — not the near edge, not the centroid). +- [x] **A partial sweep was tessellated at full-circle density.** Real + OpenSCAD scales the full-circle fragment count down for a partial angle + (`fragments = floor(get_fragments_from_r(...) * |angle|/360)`, minimum 1) + rather than using that many segments across the whole (shorter) arc — + `MeshEvaluator` was passing the full-circle count straight through, + over-tessellating any `angle<360` sweep relative to real OpenSCAD's + actual, coarser output. Fixed by applying the same floor-and-scale + formula before calling `Revolve()` — with one deviation from the literal + formula: floored to a minimum of **3**, not 1. `Manifold::Revolve()` only + honors an explicit `circularSegments` when it's `>2`; passing 1 or 2 + silently falls back to Manifold's own internal auto-quality segment count + (unrelated to our angle/profile), which for a small enough angle produced + *zero* triangles outright — confirmed via `rotate_extrude-angle.scad`'s + `angle=5`/`angle=-5` "render a single segment" cases, which the + formula's literal `fragments=1` maps to. +- [x] **`angle=`'s `NaN`/`Infinity` handling was lost before `MeshEvaluator` + ever saw it.** Real OpenSCAD treats a non-finite `angle=` as "not given" + (full 360° circle, confirmed against a live 2021.01 run of + `rotate_extrude-angle.scad`'s `0/0`/`1/0`/`-1/0` cases). `MeshEvaluator` + already special-cased this correctly, but by the time its value arrived + there `CsgEvaluator`'s generic `evalNumber()` had already collapsed any + non-finite number to `0.0` — indistinguishable from a *literal* `angle=0` + (a real, distinct "no geometry at all" case straight from 2021.01's + `rotatePolygon()`: `if (angle==0) return nullptr`). Fixed by special- + casing `"angle"` in `CsgEvaluator::evalExtrusion` (alongside the existing + `"scale"`/`"center"` special cases) to resolve non-finite values to 360 + before that collapse can happen, rather than losing the distinction. + Regression test at the IR level (`CsgEval:rotate_extrude angle keeps + NaN/Infinity distinct from a literal 0`, doesn't need Manifold) plus three + Manifold-level ones (`[v87][bugfix]` in `test_headless_build.cpp`, + volumes pinned against Pappus's centroid theorem for the mirror/negative- + angle cases). + +Net effect on the two originally-reported files: `rotate_extrude-tests.scad` +27%→13.0% (now an exact volume match — `sym_diff_volume` is entirely the +`$fn=1`/3-segment "minimal fragments" case's residual rotational-phase +misalignment, an extreme, deliberately-adversarial edge case not chased +further this pass) and `rotate_extrude-angle.scad` 71%→16.7% (every +"real" partial-angle/negative-angle/edge-case construct in the file now +matches to floating-point noise in isolation; the remainder is +`rotate_extrude(45) face(10)` — a positional first argument, which current +`master`'s test corpus intends as `angle` but 2021.01 doesn't support +positionally at all, instead treating it as the deprecated `file=` DXF- +import parameter and silently discarding the children when that "file" +isn't found. Left unfixed per v3.9's own oracle-version caution: matching +either interpretation (2021.01's DXF quirk, or `master`'s positional +`angle`) without a newer real OpenSCAD build to check against would just be +guessing which oracle to match). + +Two phase-alignment leads investigated and *not* pursued further, both +because a genuine fix requires matching Manifold's `Revolve()` ring-start +convention to OpenSCAD's own (`-90°`/`+90°`-offset, direction-dependent) +one exactly, and an incorrect guess measurably regressed already-passing +cases: +- A uniform `-90°` post-rotation for full-circle sweeps (reasoning that + `rotatePolygon`'s legacy `-90°`-start convention should apply) fixed + the deliberately-coarse `$fn=1` case somewhat (`rel_error` 1.33→1.14) but + *broke* every previously-exact full-circle case (`rotate_extrude-tests` + case 1, `rotate_extrude-touch-vertex`/`-touch-edge`, all 0→~0.008) — + reverted. Fine tessellations are insensitive to phase (an N-gon + approximation of a full circle converges to the same smooth solid + regardless of starting angle as N→∞), which is why this was invisible + until measured directly against the coarse case. +- Empirically measuring the actual ring angles used (comparing STL vertex + `atan2` positions between the two engines for matching `(radius, height)` + profile points) found a consistent offset for the `$fn=1` case, but not + one matching any clean closed-form guess tried against 2021.01's own + ring-angle formula — left as a known, low-priority gap rather than + guessed at further. + ## 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 22e463f..e5d18d0 100644 --- a/src/csg/CsgEvaluator.cpp +++ b/src/csg/CsgEvaluator.cpp @@ -1061,6 +1061,18 @@ CsgNodePtr CsgEvaluator::evalExtrusion(const ExtrusionNode& e, const glm::mat4& } else if (name == "center") { Value cv = m_interp->evaluate(*exprPtr); ext.params["center"] = bool(cv) ? 1.0 : 0.0; + } else if (name == "angle") { + // rotate_extrude()'s angle needs to keep a non-finite (NaN/ + // +-Infinity) value distinguishable from a literal 0 — the + // blanket evalNumber() below collapses both to 0.0, but they + // mean opposite things: real OpenSCAD treats a non-finite angle + // as "not given" (full 360° circle), while an actual angle=0 is + // a distinct "no geometry at all" case MeshEvaluator special- + // cases. Resolve the non-finite case to 360 here, before that + // collapse, rather than losing the distinction. + Value av = m_interp->evaluate(*exprPtr); + double raw = av.isNumber() ? av.asNumber() : 360.0; + ext.params["angle"] = std::isfinite(raw) ? raw : 360.0; } else { ext.params[name] = m_interp->evalNumber(*exprPtr); } diff --git a/src/csg/MeshEvaluator.cpp b/src/csg/MeshEvaluator.cpp index c1a8821..a9e41c1 100644 --- a/src/csg/MeshEvaluator.cpp +++ b/src/csg/MeshEvaluator.cpp @@ -1,6 +1,7 @@ #include "MeshEvaluator.h" #include #include +#include #include #include #include @@ -442,40 +443,109 @@ manifold::Manifold MeshEvaluator::evalExtrusion(const CsgExtrusion& e, result = result.Translate({0.0f, 0.0f, -static_cast(height) * 0.5f}); } else { - // rotate_extrude + // rotate_extrude — angle resolution matches real OpenSCAD's own + // (RotateExtrudeNode::instantiate/rotateextrude.cc, verified against + // a live 2021.01 binary): a missing, non-finite (NaN/+-Inf), or + // out-of-(-360,360] angle all fall back to a full 360° revolution. + // Confirmed against rotate_extrude-angle.scad's corpus cases, which + // exercise exactly these edges (unspecified, 0/0, 1/0, -1/0, 360, + // -360, 1000, -1000). double angle = getP("angle", 360.0); + if (!std::isfinite(angle) || angle <= -360.0 || angle > 360.0) + angle = 360.0; + + // angle=0 is a distinct case from "unspecified"/"out of range" — + // real OpenSCAD's rotatePolygon() returns no geometry at all for it + // (not a degenerate zero-volume sweep), matching + // rotate_extrude-angle.scad's own "// show nothing" comment on its + // angle=0 case. + if (angle == 0.0) return {}; + double fnOvr = getP("$fn", 0.0); - int segs = gen.resolveSegments(10.0, fnOvr); // 10 = proxy radius manifold::Polygons polys = cs.ToPolygons(); - // OpenSCAD requires the 2-D profile not cross the Y axis (Manifold's - // X axis here) — revolving a profile that straddles the rotation - // axis produces self-intersecting/degenerate geometry. Check before - // handing off to Revolve() rather than passing bad input through - // silently. + // Real OpenSCAD only rejects a profile that *straddles* the + // rotation axis (points strictly on both sides) — a profile lying + // entirely on the -X side is valid, it just revolves mirrored back + // onto +X (confirmed against rotate_extrude-tests.scad's "Object in + // negative X" case, translate([-20,0]) square(10)). This used to + // reject *any* point with x<0, including profiles entirely on the + // negative side. constexpr double kAxisEps = 1e-4; - bool crossesAxis = false; + double minX = 0.0, maxX = 0.0; for (const auto& poly : polys) { for (const auto& pt : poly) { - if (pt.x < -kAxisEps) { crossesAxis = true; break; } + minX = std::min(minX, static_cast(pt.x)); + maxX = std::max(maxX, static_cast(pt.x)); } - if (crossesAxis) break; } - if (crossesAxis) { + if (minX < -kAxisEps && maxX > kAxisEps) { chisel::lang::Diagnostic d; d.level = chisel::lang::DiagLevel::Error; d.message = "rotate_extrude(): profile crosses the rotation axis " - "(all points must satisfy x >= 0); geometry skipped"; + "(all points must have the same X sign); geometry skipped"; m_diags.push_back(std::move(d)); return {}; } + // Manifold's own Revolve() only keeps x>=0 geometry (it silently + // clips away anything with x<0 rather than mirroring it), so a + // profile entirely on the -X side must be mirrored onto +X before + // handing it off. Revolving the mirrored profile through the same + // angle and then rotating the result 180° about Z reproduces the + // original sweep: (x*cos(a), x*sin(a)) for x<0 is identical to + // ((-x)*cos(a+180), (-x)*sin(a+180)) for -x>0. Negating x alone + // mirrors (and thus reverses the winding of) each polygon, so the + // vertex order is reversed too, to keep the outward-facing + // convention Revolve() expects — without this the mirrored solid + // comes out inside-out (negative volume). + bool mirrored = minX < -kAxisEps; + if (mirrored) { + for (auto& poly : polys) { + for (auto& pt : poly) + pt.x = -pt.x; + std::reverse(poly.begin(), poly.end()); + } + } + // Segment count uses the profile's own X-extent as the radius proxy + // (matching real OpenSCAD's Calc::get_fragments_from_r(max_x-min_x, + // ...) — not a fixed stand-in radius, and not the distance from the + // axis to the near edge of the profile). + double radius = mirrored ? (maxX - minX) : maxX; + int segs = gen.resolveSegments(radius, fnOvr); + // Real OpenSCAD scales that full-circle count down for a partial + // sweep (floor, matching OpenSCAD's own minimum of 1) rather than + // tessellating the whole arc at full-circle density — passing the + // full-circle count straight through to Revolve() (as this used to) + // over-tessellates any angle<360 sweep relative to real OpenSCAD's + // actual output. The floor is 3, not 1: Manifold::Revolve() only + // honors an explicit circularSegments > 2, silently falling back to + // its own internal auto-quality segment count (built from a default + // it knows nothing about our angle/profile) for 0/1/2 — which + // produced degenerate/empty geometry here for a small-enough angle, + // not just an imprecise tessellation. + segs = std::max(3, static_cast(segs * std::fabs(angle) / 360.0)); + + // Manifold::Revolve() winds its side faces correctly for a + // positive sweep but comes out inside-out (negative volume) for a + // literal negative revolveDegrees, so always sweep by the positive + // magnitude and mirror the *result* across the XZ plane (Y -> -Y) + // for an originally-negative angle instead: revolving a profile + // point through angle -A gives (x*cos(-A), x*sin(-A), y) = + // (x*cos(A), -x*sin(A), y), i.e. exactly the +A sweep with Y + // negated. Manifold's Mirror() (unlike Revolve()) is a generic + // transform and keeps winding correct on its own. result = manifold::Manifold::Revolve( polys, segs, - static_cast(angle)); + static_cast(std::fabs(angle))); + if (angle < 0.0) + result = result.Mirror({0.0, 1.0, 0.0}); + + if (mirrored) + result = result.Rotate(0.0, 0.0, 180.0); } // Apply the outer 3-D world transform diff --git a/tests/fixtures/headless/rotate_extrude_angle_zero.scad b/tests/fixtures/headless/rotate_extrude_angle_zero.scad new file mode 100644 index 0000000..0eadb90 --- /dev/null +++ b/tests/fixtures/headless/rotate_extrude_angle_zero.scad @@ -0,0 +1 @@ +rotate_extrude(angle=0) translate([10,0]) square([10,10]); diff --git a/tests/fixtures/headless/rotate_extrude_negative_angle.scad b/tests/fixtures/headless/rotate_extrude_negative_angle.scad new file mode 100644 index 0000000..b33480e --- /dev/null +++ b/tests/fixtures/headless/rotate_extrude_negative_angle.scad @@ -0,0 +1 @@ +rotate_extrude(angle=-90, $fn=360) translate([10,0]) square([10,10]); diff --git a/tests/fixtures/headless/rotate_extrude_negative_x.scad b/tests/fixtures/headless/rotate_extrude_negative_x.scad new file mode 100644 index 0000000..822b941 --- /dev/null +++ b/tests/fixtures/headless/rotate_extrude_negative_x.scad @@ -0,0 +1 @@ +rotate_extrude($fn=360) translate([-20,0]) square([10,10]); diff --git a/tests/test_csg_evaluator.cpp b/tests/test_csg_evaluator.cpp index 0fb01c4..b848db3 100644 --- a/tests/test_csg_evaluator.cpp +++ b/tests/test_csg_evaluator.cpp @@ -1454,6 +1454,36 @@ TEST_CASE("CsgEval:linear_extrude scale must be exactly a 2-vector, else no scal REQUIRE(good.params.at("scale_y") == Approx(5.0)); } +TEST_CASE("CsgEval:rotate_extrude angle keeps NaN/Infinity distinct from a literal 0", + "[csg][v87][bugfix]") { + // Real OpenSCAD (Value::getFiniteDouble()) treats a non-finite angle= + // as "not given" (full 360° circle), which is a completely different + // outcome from a literal angle=0 (MeshEvaluator's "no geometry at all" + // case) — but the generic evalNumber() path collapses any non-finite + // number to 0.0, which would silently turn a NaN/Infinity angle into + // the "no geometry" case instead of "full circle". evalExtrusion() + // special-cases "angle" to resolve non-finite values to 360 before that + // collapse can happen; this checks the resolved IR param, not + // MeshEvaluator's geometry (which isn't part of this Manifold-free test + // binary — see the file-header comment above). + auto nan = asExtrusion(evaluate("rotate_extrude(angle=0/0) circle(5);").roots[0]); + REQUIRE(nan.params.at("angle") == Approx(360.0)); + + auto posInf = asExtrusion(evaluate("rotate_extrude(angle=1/0) circle(5);").roots[0]); + REQUIRE(posInf.params.at("angle") == Approx(360.0)); + + auto negInf = asExtrusion(evaluate("rotate_extrude(angle=-1/0) circle(5);").roots[0]); + REQUIRE(negInf.params.at("angle") == Approx(360.0)); + + // A literal 0 is preserved as 0, not folded into the "not given" case. + auto zero = asExtrusion(evaluate("rotate_extrude(angle=0) circle(5);").roots[0]); + REQUIRE(zero.params.at("angle") == Approx(0.0)); + + // An ordinary finite angle is unaffected. + auto normal = asExtrusion(evaluate("rotate_extrude(angle=45) circle(5);").roots[0]); + REQUIRE(normal.params.at("angle") == Approx(45.0)); +} + // --------------------------------------------------------------------------- // Recursive functions (Tier C — confirmed already working via Tier A impl) // --------------------------------------------------------------------------- diff --git a/tests/test_headless_build.cpp b/tests/test_headless_build.cpp index 91bff88..faa814b 100644 --- a/tests/test_headless_build.cpp +++ b/tests/test_headless_build.cpp @@ -131,6 +131,59 @@ TEST_CASE("runBuild: non-planar polyhedron() under scale() matches CHECK(result.volume == Approx(1.6544281372).margin(1e-5)); } +// --------------------------------------------------------------------------- +// issue #87 — rotate_extrude() volume mismatches. Found via volumetric +// corpus comparison against a live OpenSCAD 2021.01 + Manifold v3.5.2 oracle +// (docs/roadmap.md v3.9/v3.12): a profile entirely on the -X side used to be +// rejected outright (only a straddling profile is actually invalid); a +// negative angle= produced inside-out (negative-volume) geometry because +// Manifold::Revolve() only winds correctly for a positive sweep; and the +// segment count used a fixed proxy radius (10) plus the full-circle count +// even for a partial sweep, instead of the profile's own X-extent scaled by +// angle/360 the way real OpenSCAD's Calc::get_fragments_from_r() does. +// --------------------------------------------------------------------------- +TEST_CASE("runBuild: rotate_extrude() of a profile entirely on -X matches Pappus's theorem", + "[headless][v87][bugfix]") { + // translate([-20,0]) square([10,10]): area 100, centroid at x=-15 (15 + // from the axis), entirely on the -X side (doesn't straddle) — a full + // revolution sweeps volume = 2*pi*area*centroidDistance (Pappus), same + // as if the profile were mirrored onto +X. This used to be rejected as + // "crosses the rotation axis" (any x<0 point, not just a straddling + // profile) and produced no geometry at all. + chisel::csg::MeshCache cache; + BuildResult result = runBuild(fixture("headless/rotate_extrude_negative_x.scad"), {}, {}, cache); + REQUIRE(result.ok()); + constexpr double kExpectedVolume = 2.0 * 3.14159265358979323846 * 100.0 * 15.0; + CHECK(result.volume == Approx(kExpectedVolume).margin(1.0)); +} + +TEST_CASE("runBuild: rotate_extrude() with a negative angle= has positive volume", + "[headless][v87][bugfix]") { + // translate([10,0]) square([10,10]): area 100, centroid at x=15, swept + // through |angle|=90 degrees (pi/2 rad) gives volume = + // area*centroidDistance*angleRadians (Pappus, partial sweep). A + // negative angle used to come out with exactly this magnitude but + // negated (inside-out geometry from Manifold::Revolve()'s winding for a + // literal negative revolveDegrees), so this also pins the *sign*. + chisel::csg::MeshCache cache; + BuildResult result = runBuild(fixture("headless/rotate_extrude_negative_angle.scad"), {}, {}, cache); + REQUIRE(result.ok()); + constexpr double kExpectedVolume = 100.0 * 15.0 * (3.14159265358979323846 / 2.0); + CHECK(result.volume == Approx(kExpectedVolume).margin(1.0)); + CHECK(result.volume > 0.0); +} + +TEST_CASE("runBuild: rotate_extrude(angle=0) produces no geometry", + "[headless][v87][bugfix]") { + // Real OpenSCAD's rotatePolygon() returns no geometry at all for a + // literal angle=0 (not a degenerate zero-volume sweep) — distinct from + // an *unspecified* angle, which means a full 360 circle. + chisel::csg::MeshCache cache; + BuildResult result = runBuild(fixture("headless/rotate_extrude_angle_zero.scad"), {}, {}, cache); + CHECK(result.volume == 0.0); + CHECK(result.triCount == 0); +} + TEST_CASE("runBuild honors AbortFn by returning early", "[headless]") { chisel::csg::MeshCache cache; auto alwaysAbort = [] { return true; }; From 4d3875e1d8ee7ec8f74ab5cd138d5314561dfc45 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 2 Aug 2026 16:51:53 +0000 Subject: [PATCH 2/2] Address review: keep min_x/max_x seeded at 0, add axis-distance regression test A review comment on this PR flagged minX/maxX being seeded at 0.0 (instead of +-infinity) and the non-mirrored branch using maxX instead of maxX-minX as two bugs in the segment-count radius calculation. Re-verified against the live OpenSCAD 2021.01 oracle this PR was built against: the 0-seeding is not a bug, it's an exact match for real OpenSCAD's own rotatePolygon() (GeometryEvaluator.cc), which seeds min_x/max_x at 0 before the same fmin/fmax scan. This means the segment- count radius is the profile's distance from the axis to its far edge, not its own true width -- confirmed by comparing segment counts against the live binary for a profile spanning x=[16,26] (real OpenSCAD uses 24 segments, matching the axis-seeded formula; a true-extent formula would give 16). Reverted the seeding back to 0.0, kept the (now-safe) simplification of computing radius as an unconditional maxX-minX without branching on `mirrored` (this was already correct under 0-seeding, since minX==0 whenever the profile isn't mirrored). Added a regression test/fixture pair (rotate_extrude_radius_near.scad/_far.scad: same-width profiles at different axis distances) that would fail under a true-extent implementation, so this doesn't get "fixed" back the same way again. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01Qt1mwHFdm3v3JMMm3VC65j --- docs/roadmap.md | 25 ++++++++++++++----- src/csg/MeshEvaluator.cpp | 23 +++++++++++++---- .../headless/rotate_extrude_radius_far.scad | 1 + .../headless/rotate_extrude_radius_near.scad | 1 + tests/test_headless_build.cpp | 23 +++++++++++++++++ 5 files changed, 62 insertions(+), 11 deletions(-) create mode 100644 tests/fixtures/headless/rotate_extrude_radius_far.scad create mode 100644 tests/fixtures/headless/rotate_extrude_radius_near.scad diff --git a/docs/roadmap.md b/docs/roadmap.md index 9cd440e..66a8dd7 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -670,12 +670,25 @@ branch, all in `src/csg/MeshEvaluator.cpp` (plus one in exactly the +a sweep with Y negated — and unlike `Revolve()`, `Mirror()`/`Transform()` keep winding correct on their own. - [x] **Segment count used a fixed proxy radius (10) instead of the - profile's own extent.** `resolveSegments()` was called with a hardcoded - `10.0` "good enough" placeholder regardless of the actual profile's - distance from the axis, rather than real OpenSCAD's - `Calc::get_fragments_from_r(max_x-min_x, ...)` (the profile's full X-span, - confirmed from 2021.01's own `rotateextrude.cc`/`GeometryEvaluator.cc` - source — not the near edge, not the centroid). + profile's own distance from the axis.** `resolveSegments()` was called + with a hardcoded `10.0` "good enough" placeholder regardless of the + actual profile, rather than real OpenSCAD's + `Calc::get_fragments_from_r(max_x-min_x, ...)`. Note this `max_x-min_x` + is **not** the profile's own true width: 2021.01's `rotatePolygon()` + (`GeometryEvaluator.cc`) seeds `min_x`/`max_x` at `0`, not the profile's + real extremes, so a profile that never touches the axis gets the + axis-to-far-edge distance, not its own span — a review comment on this + PR initially (reasonably) flagged that 0-seeding as a bug and proposed + seeding at ±infinity instead, which looked more "correct" but is a + regression against the actual oracle: re-verified against the live + 2021.01 binary (`rotate_extrude(a=-45)` on a profile spanning + `x=[16,26]`, a width of 10 measured from `16`, but `26` measured from the + axis) — 2021.01 uses 24 segments, matching the axis-seeded formula + exactly, not the 16 a true-extent formula would give. Pinned with + `rotate_extrude_radius_near.scad`/`_far.scad` (same-width profiles at + different axis distances; a true-extent version of this code would give + them equal segment counts, the real fix gives `far` roughly double + `near`'s). - [x] **A partial sweep was tessellated at full-circle density.** Real OpenSCAD scales the full-circle fragment count down for a partial angle (`fragments = floor(get_fragments_from_r(...) * |angle|/360)`, minimum 1) diff --git a/src/csg/MeshEvaluator.cpp b/src/csg/MeshEvaluator.cpp index a9e41c1..f3a1d27 100644 --- a/src/csg/MeshEvaluator.cpp +++ b/src/csg/MeshEvaluator.cpp @@ -472,6 +472,17 @@ manifold::Manifold MeshEvaluator::evalExtrusion(const CsgExtrusion& e, // negative X" case, translate([-20,0]) square(10)). This used to // reject *any* point with x<0, including profiles entirely on the // negative side. + // minX/maxX are deliberately seeded at 0.0, not the true extremes — + // matching real OpenSCAD's own rotatePolygon() (GeometryEvaluator.cc + // in the 2021.01 oracle this was verified against), which does the + // same (`double min_x = 0; double max_x = 0;` before the same + // fmin/fmax scan). This isn't an oversight there: it means a profile + // that never touches the axis gets its segment-count radius (below) + // measured as the axis-to-far-edge distance, not the profile's own + // width — confirmed against a live 2021.01 binary, which uses 24 + // segments (not the "true extent"-implied 16) for + // rotate_extrude(a=-45) applied to a profile spanning x=[16,26] (a + // width of 10, but 26 measured from the axis). constexpr double kAxisEps = 1e-4; double minX = 0.0, maxX = 0.0; for (const auto& poly : polys) { @@ -509,11 +520,13 @@ manifold::Manifold MeshEvaluator::evalExtrusion(const CsgExtrusion& e, std::reverse(poly.begin(), poly.end()); } } - // Segment count uses the profile's own X-extent as the radius proxy - // (matching real OpenSCAD's Calc::get_fragments_from_r(max_x-min_x, - // ...) — not a fixed stand-in radius, and not the distance from the - // axis to the near edge of the profile). - double radius = mirrored ? (maxX - minX) : maxX; + // Segment count uses (0-seeded) maxX-minX as the radius proxy, + // matching real OpenSCAD's Calc::get_fragments_from_r(max_x-min_x, + // ...) exactly — not a fixed stand-in radius (this used to be a + // hardcoded 10.0). Computed from the pre-mirror min/max: negating + // every x negates and swaps min/max, so this span is the same + // either way — no need to branch on `mirrored` here. + double radius = maxX - minX; int segs = gen.resolveSegments(radius, fnOvr); // Real OpenSCAD scales that full-circle count down for a partial // sweep (floor, matching OpenSCAD's own minimum of 1) rather than diff --git a/tests/fixtures/headless/rotate_extrude_radius_far.scad b/tests/fixtures/headless/rotate_extrude_radius_far.scad new file mode 100644 index 0000000..46cf664 --- /dev/null +++ b/tests/fixtures/headless/rotate_extrude_radius_far.scad @@ -0,0 +1 @@ +rotate_extrude($fs=1) translate([3,0]) square([1,1]); diff --git a/tests/fixtures/headless/rotate_extrude_radius_near.scad b/tests/fixtures/headless/rotate_extrude_radius_near.scad new file mode 100644 index 0000000..7f218a9 --- /dev/null +++ b/tests/fixtures/headless/rotate_extrude_radius_near.scad @@ -0,0 +1 @@ +rotate_extrude($fs=1) translate([1,0]) square([1,1]); diff --git a/tests/test_headless_build.cpp b/tests/test_headless_build.cpp index faa814b..30aafbf 100644 --- a/tests/test_headless_build.cpp +++ b/tests/test_headless_build.cpp @@ -184,6 +184,29 @@ TEST_CASE("runBuild: rotate_extrude(angle=0) produces no geometry", CHECK(result.triCount == 0); } +TEST_CASE("runBuild: rotate_extrude() segment count tracks distance from the axis, " + "not the profile's own width", + "[headless][v87][bugfix]") { + // Both fixtures revolve the same 1x1 square (so the *same width* either + // way), just at a different offset from the axis: near = [1,2], far = + // [3,4]. Real OpenSCAD's own rotatePolygon() (GeometryEvaluator.cc) + // measures the segment-count radius as max_x-min_x with min_x/max_x + // *seeded at 0*, not the profile's true [minX,maxX] extent — so a + // profile's distance from the axis to its far edge drives the segment + // count, not its own width. A "fix" that used the true per-profile + // extent instead (i.e. always got 1 either way, since both squares are + // 1 wide) would make these come out with the *same* triangle count; + // real OpenSCAD (and this) instead gives far roughly double near's, + // since far's axis distance is roughly double near's. + chisel::csg::MeshCache cache; + BuildResult near = runBuild(fixture("headless/rotate_extrude_radius_near.scad"), {}, {}, cache); + BuildResult far = runBuild(fixture("headless/rotate_extrude_radius_far.scad"), {}, {}, cache); + + REQUIRE(near.ok()); + REQUIRE(far.ok()); + CHECK(far.triCount > near.triCount * 3 / 2); +} + TEST_CASE("runBuild honors AbortFn by returning early", "[headless]") { chisel::csg::MeshCache cache; auto alwaysAbort = [] { return true; };