Fix rotate_extrude() volume mismatches (issue #87)

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,138 @@ 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 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)
+  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)

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);
         }

src/csg/MeshEvaluator.cpp

@@ -1,6 +1,7 @@
 #include "MeshEvaluator.h"
 #include <glm/glm.hpp>
 #include <algorithm>
+#include <cmath>
 #include <cstdio>
 #include <functional>
 #include <optional>
@@ -442,40 +443,122 @@ manifold::Manifold MeshEvaluator::evalExtrusion(const CsgExtrusion& e,
             result = result.Translate({0.0f, 0.0f,
                                        -static_cast<float>(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.
+        // 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;
-        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<double>(pt.x));
+                maxX = std::max(maxX, static_cast<double>(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 (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
+        // 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<int>(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<float>(angle));
+            static_cast<float>(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

tests/fixtures/headless/rotate_extrude_angle_zero.scad

@@ -0,0 +1 @@
+rotate_extrude(angle=0) translate([10,0]) square([10,10]);

tests/fixtures/headless/rotate_extrude_negative_angle.scad

@@ -0,0 +1 @@
+rotate_extrude(angle=-90, $fn=360) translate([10,0]) square([10,10]);

tests/fixtures/headless/rotate_extrude_negative_x.scad

@@ -0,0 +1 @@
+rotate_extrude($fn=360) translate([-20,0]) square([10,10]);

tests/fixtures/headless/rotate_extrude_radius_far.scad

@@ -0,0 +1 @@
+rotate_extrude($fs=1) translate([3,0]) square([1,1]);

tests/fixtures/headless/rotate_extrude_radius_near.scad

@@ -0,0 +1 @@
+rotate_extrude($fs=1) translate([1,0]) square([1,1]);