Skip to content

Extend Op::PushBuiltinWrap to hull/minkowski/render/extrude/offset/roof - #68

Merged
revarbat merged 1 commit into
mainfrom
builtinwrap-hull-minkowski-render-extrude-offset-roof
Aug 1, 2026
Merged

Extend Op::PushBuiltinWrap to hull/minkowski/render/extrude/offset/roof#68
revarbat merged 1 commit into
mainfrom
builtinwrap-hull-minkowski-render-extrude-offset-roof

Conversation

@revarbat

@revarbat revarbat commented Aug 1, 2026

Copy link
Copy Markdown
Member

Summary

  • Every remaining non-leaf builtin whose own children can wrap a recursive call still fell to Op::NativeStatement — one real native C++ reentry per level, capped at kMaxDriveVmNativeDepth=40 (same risk Op::PushBuiltinWrap Close the NativeStatement gap for translate/color/modifier-wrapped recursion #63 and Op::PushCsgWrap Add Op::PushCsgWrap: eliminate union/difference/intersection's native reentry #67 already closed for their own subsets).
  • hull()/minkowski()/render()/linear_extrude()/rotate_extrude()/projection()/offset()/roof() all share resolveTransform's own "resolve args, evaluate children, build params" shape, so they reuse Op::PushBuiltinWrap's existing bracket via 6 new Kind values instead of a new opcode set — no new mechanism, just widening the one that already exists.
  • One behavioral subtlety: roof()'s "Unknown roof method" warning is an observable side effect, so its params are computed at Pop time (after children) instead of Push time like every other kind here, to preserve native's own echo/warn ordering exactly. Verified with a dedicated ordering test.
  • intersection_for remains uncovered — its group_sizes are keyed by runtime loop iteration count, not a static source-statement list, so it needs real loop-compilation machinery neither existing bracket shape provides. Flagged as a separate, larger follow-up (not silently skipped).

Test plan

  • Full 737-test suite green both OSCAD_BYTECODE_VM on (default) and off, up from 727 (10 new tests: one recursion-depth test per builtin, $-arg propagation into a compiled wrap, and the roof warn-ordering test).
  • Caught and fixed a test-design bug during verification: offset(r=1) nested 1500 levels deep is a real, unbounded geometric operation (each level's rounded-join offset grows both size and vertex count) — hung for minutes before being rewritten to offset(delta=0) (a real no-op at every level, same opcode path, none of the compounding cost).
  • Real-world smoke check: BOSL2's examples/fractal_tree.scad renders to a byte-for-byte identical STL before/after this change.
  • Version bumped to 0.13.6.

🤖 Generated with Claude Code

Every remaining non-leaf builtin whose own children can wrap a recursive
call still fell to Op::NativeStatement -- one real native C++ reentry
per level, capped at kMaxDriveVmNativeDepth=40 -- exactly the class of
risk Op::PushBuiltinWrap (translate/color/#/%/!) and Op::PushCsgWrap
(union/difference/intersection) already closed for their own subsets.

hull()/minkowski()/render()/linear_extrude()/rotate_extrude()/
projection()/offset()/roof() all share resolveTransform's own "resolve
args, evaluate children, build params" shape (verified against each of
their existing native resolve functions), so they reuse Op::
PushBuiltinWrap's own bracket via 6 new Kind values instead of a new
opcode set: Passthrough (hull/minkowski/render -- empty params) and
LinearExtrude/RotateExtrude/Projection/Offset compute their params at
Push time, same as Transform/Color already do (verified side-effect-
free); Roof computes its params at Pop time instead, since its "Unknown
roof method" warning is an observable side effect that must stay
ordered after any child's own echo/warn output, matching native
resolveRoof's own evalChildren-then-compute-params order exactly.

Each computeXParams helper is extracted from its resolve function's
existing tail (shared by both the native and compiled path, same
pattern as the pre-existing computeTransformParams/computeColorParams)
-- no behavior change to the interpreter path.

Only intersection_for remains uncovered: its own group_sizes are keyed
by RUNTIME loop iteration count, not a static source-statement list the
way union/difference/intersection's are, so it needs real loop-
compilation machinery neither existing bracket shape provides -- a
separate, larger follow-up.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@revarbat

revarbat commented Aug 1, 2026

Copy link
Copy Markdown
Member Author

Verification follow-up: while working on the intersection_for follow-up (#69), found that my local CLI binary (tools/cli/openscad-cpp-evaluator) hadn't been relinked since the start of the session — a separate build target from the test binary, easy to miss. This meant the fractal_tree.scad byte-identical check in this PR's description ran against a stale binary. That check was weak anyway (that script doesn't exercise hull/minkowski/render/extrude/offset/roof — it's mostly cylinder/attach/rotate/color), so it wasn't invalidating anything, but it also wasn't testing what it claimed to.

The unit test suite itself was never affected (CMake always relinks oscad_eval_tests correctly on --target oscad_eval_tests), so the 737/737 differential result stands. Re-ran the actual depth check with a properly rebuilt CLI to be sure:

hull depth=39: SUCCEEDED   (was capped at 39 before this fix, per the earlier investigation)
hull depth=1500: SUCCEEDED

Confirms the fix is real, not just unit-test-shaped. No code changes needed here — just flagging the gap in case CI's own from-scratch build didn't already make this moot (it should have).

@revarbat
revarbat merged commit f31f2f5 into main Aug 1, 2026
3 checks passed
@revarbat
revarbat deleted the builtinwrap-hull-minkowski-render-extrude-offset-roof branch August 1, 2026 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant