From 1de720b642e7ea5bc15c2c3309dd53ab92bc7d3d Mon Sep 17 00:00:00 2001 From: Revar Desmera Date: Sat, 22 Aug 2026 23:00:19 -0700 Subject: [PATCH] children(separate=true): forward children as separate CSG operands difference() children() returns the union, not a difference. The children arrive as ONE operand, so there is nothing to subtract from. Upstream OpenSCAD does the same, and has no per-call way out of it. module frame() { difference() children(separate=true); } frame() { cube(50,center=true); sphere(30); cylinder(h=99,r=8); } intersection() is the case with no workaround at all: for difference you could already write difference() { children(0); children([1:$children-1]); } since A-(B|C) == A-B-C, but A&(B|C) is not A&B&C. The obvious implementation does not work. The wrapper children() builds is already display-only (isBuiltin=false, so generateTreeImpl falls through to plain concatenation) -- removing it changes nothing, because difference() groups its operands by AST CHILD STATEMENT, not by node. children() is one statement however many nodes it splices, so the group stays one group. That per-statement grouping is deliberate and load-bearing (BOSL2's attachable() returns parent + attachments as several bodies and must count as one operand), so the feature is instead: let one statement contribute SEVERAL groups. A CSGNode::separateOperand flag, set on the forwarded nodes, does that. It rides on the nodes rather than on the Evaluator, which buys three things free: several splices in one statement work; nesting degrades correctly (under translate() the nodes land in translate's frame, so difference() still sees one operand); and there is no state to restore on the throw path. group_sizes was being built twice, once per engine. Both now go through one shared Evaluator::appendGroupSizes, landed first as a behaviour-neutral commit-sized step with the suite green -- including the size-ZERO group a statement that produced nothing contributes, which generateCsg relies on to reset intersection() and bail difference(). Also closes a real gap this made testable: Op::CallChildren never called warnUnexpectedBuiltinArgs, so a children() typo warned under the interpreter and was silent under the VM. Verified by mutation: breaking the two VM marking sites fails 7 of the 16 new tests, so the VM coverage is genuine rather than an interpreter fallback. 1005 tests pass under both engines. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 5 +- .../openscad_cpp_evaluator/bytecode_vm.hpp | 4 + include/openscad_cpp_evaluator/csg_node.hpp | 18 ++ include/openscad_cpp_evaluator/evaluator.hpp | 29 ++ pyproject.toml | 2 +- src/builtins/booleans.cpp | 10 +- src/builtins/control.cpp | 3 +- src/builtins/registry.cpp | 2 +- src/bytecode_vm.cpp | 18 +- src/csg_resolve.cpp | 7 +- src/user_calls.cpp | 12 +- tests/CMakeLists.txt | 1 + tests/test_children_separate.cpp | 278 ++++++++++++++++++ 13 files changed, 374 insertions(+), 15 deletions(-) create mode 100644 tests/test_children_separate.cpp diff --git a/CLAUDE.md b/CLAUDE.md index 9c48283..8da524c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -481,7 +481,10 @@ grep for `ponytail:`. lexically nested inside an already-active call," picking `childCtx()` vs `callCtx()` accordingly; read its doc comment before touching it, it's the second-trickiest mechanism in this codebase after the CSG tree stack), `evalUserModule`/`evalUserFunction`/`evalFunctionLiteral`, - and `builtinChildren` (children()/children(N), deferred evaluation). `evalFunctionCall`'s + and `builtinChildren` (children()/children(N), deferred evaluation; `children(separate=true)` + additionally marks each forwarded node `separateOperand` so the enclosing + union/difference/intersection treats them as separate operands rather than one grouped one -- + a BelfrySCAD extension, see `Evaluator::appendGroupSizes`). `evalFunctionCall`'s precedence order (checked in this exact sequence): `import` (special-cased, its own module/expression-context split) → `isBuiltinFunctionName` (function_builtins.hpp — always wins if true) → user function lookup → function-literal *value* probe → "unknown function" warning. diff --git a/include/openscad_cpp_evaluator/bytecode_vm.hpp b/include/openscad_cpp_evaluator/bytecode_vm.hpp index 07e6632..c81d7e1 100644 --- a/include/openscad_cpp_evaluator/bytecode_vm.hpp +++ b/include/openscad_cpp_evaluator/bytecode_vm.hpp @@ -219,6 +219,10 @@ struct VmFrame { // mirrors evalModularCall's own `&node` (csg_resolve.cpp). Only // meaningful when ownsModuleSplice is true. const oscad::ASTNode* moduleSpliceCallNode = nullptr; + // children(separate=true) on the call this frame is forwarding for. + // Only meaningful while ownsModuleSplice is true, same as the two + // fields above; reset in releaseVmFrame for the same pooling reason. + bool separateChildren = false; // Whether callStack_.back() (at push time) genuinely IS this frame's // own logical call, and therefore safe for a tail hop inside this diff --git a/include/openscad_cpp_evaluator/csg_node.hpp b/include/openscad_cpp_evaluator/csg_node.hpp index 4f00785..6472724 100644 --- a/include/openscad_cpp_evaluator/csg_node.hpp +++ b/include/openscad_cpp_evaluator/csg_node.hpp @@ -34,6 +34,16 @@ struct CSGNode { CSGParams params; // resolve step's plain-data output bool uncacheable = false; // set by a later phase (ManifoldCache, Phase 8) + // Set only by children(separate=true): this node begins its OWN operand + // group in an enclosing union/difference/intersection/intersection_for, + // instead of merging into the single group its enclosing statement + // would otherwise form. See Evaluator::appendGroupSizes. + // + // Deliberately absent from cacheKey()'s allowlist (manifold_cache.cpp): + // it changes the PARENT's hashed "group_sizes" param, never this node's + // own geometry, so the parent already re-keys and this node must not. + bool separateOperand = false; + // The call site that entered this node's call chain from the top // level, captured at RESOLVE time -- non-owning, AST-lifetime-bound // like `node`. nullptr for a node resolved at top level. @@ -73,4 +83,12 @@ struct CSGNode { std::optional cachedKey; }; +// Marks every node from `from` onward as starting its own operand group -- +// what children(separate=true) does to the geometry it just forwarded. +// A free function, not a member, so all three splice sites (user_calls.cpp +// and bytecode_vm.cpp's two) can reach it from this header alone. +inline void markSeparateOperands(std::vector>& nodes, size_t from) { + for (size_t i = from; i < nodes.size(); ++i) nodes[i]->separateOperand = true; +} + } // namespace oscadeval diff --git a/include/openscad_cpp_evaluator/evaluator.hpp b/include/openscad_cpp_evaluator/evaluator.hpp index 19decb4..7c05765 100644 --- a/include/openscad_cpp_evaluator/evaluator.hpp +++ b/include/openscad_cpp_evaluator/evaluator.hpp @@ -190,6 +190,30 @@ class Evaluator { // statement. size_t currentTreeFrameSize() const { return treeStack_.back().size(); } + // Records the group(s) one child statement contributed, given the frame + // size captured before it ran. Normally that is exactly one group, of + // however many nodes the statement pushed -- including a group of size + // ZERO for a statement that produced no geometry (a disabled `*cube()`), + // which generateCsg relies on to reset intersection() and to bail + // difference(). Only children(separate=true) produces more than one: + // each node it marked starts a fresh group, so its forwarded children + // reach the enclosing operator as separate operands. + // + // Shared by all four group builders -- resolveCsg, resolveIntersectionFor + // and the VM's Op::CsgGroupEnd (which serves both) -- because + // "group_sizes" is otherwise built twice, once per engine, and would + // drift. + void appendGroupSizes(std::vector& groupSizes, size_t before) const { + const std::vector>& frame = treeStack_.back(); + size_t start = before; + for (size_t i = before + 1; i < frame.size(); ++i) { + if (!frame[i]->separateOperand) continue; + groupSizes.push_back(Value{static_cast(i - start)}); + start = i; + } + groupSizes.push_back(Value{static_cast(frame.size() - start)}); + } + // Generate pass: walks `tree` bottom-up (children before their own // node), calling each node's registered GenerateFn (falling back to // concatenating children's bodies for a kind with none registered). @@ -342,6 +366,11 @@ class Evaluator { struct ChildrenForward { EvalContext evalCtx; std::vector nodes; + // children(separate=true): hand these to the enclosing union/ + // difference/intersection as SEPARATE operands rather than as one + // grouped operand. Acted on after the nodes are evaluated, by + // marking the CSGNodes they produced -- see markSeparateOperands. + bool separate = false; }; std::optional prepareChildrenForward(const CallArgs& args, EvalContext& ctx); diff --git a/pyproject.toml b/pyproject.toml index eabf514..912e6dd 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "scikit_build_core.build" [project] name = "openscad_cpp_evaluator" -version = "0.41.0" +version = "0.42.0" description = "C++ OpenSCAD evaluator with Python bindings" readme = "README.md" requires-python = ">=3.12" diff --git a/src/builtins/booleans.cpp b/src/builtins/booleans.cpp index baa8703..e1420f1 100644 --- a/src/builtins/booleans.cpp +++ b/src/builtins/booleans.cpp @@ -68,7 +68,12 @@ std::optional toCrossSection(const std::vector{geoNode}, effCtx); - const size_t after = ev.currentTreeFrameSize(); - groupSizes.push_back(Value{static_cast(after - before)}); + ev.appendGroupSizes(groupSizes, before); } CSGParams params; diff --git a/src/builtins/control.cpp b/src/builtins/control.cpp index 2bcdea4..bb010e7 100644 --- a/src/builtins/control.cpp +++ b/src/builtins/control.cpp @@ -119,8 +119,7 @@ CSGParams resolveIntersectionFor(Evaluator& ev, const oscad::ModularIntersection if (!bodyNodes.empty()) ev.checkDebug(*bodyNodes.front(), parentCtx, /*forced=*/false, /*exprLevel=*/true); const size_t before = ev.currentTreeFrameSize(); ev.evalChildren(bodyNodes, parentCtx); - const size_t after = ev.currentTreeFrameSize(); - groupSizes.push_back(Value{static_cast(after - before)}); + ev.appendGroupSizes(groupSizes, before); return; } const auto& assign = node.assignments[depth]; diff --git a/src/builtins/registry.cpp b/src/builtins/registry.cpp index b25b5d5..6424e7d 100644 --- a/src/builtins/registry.cpp +++ b/src/builtins/registry.cpp @@ -112,7 +112,7 @@ const std::vector* builtinParamNames(const std::string& name) { {"intersection", {}}, {"hull", {}}, {"minkowski", {"convexity"}}, - {"children", {"index"}}, + {"children", {"index", "separate"}}, {"render", {"convexity"}}, // "repair" is this port's own addition, not an upstream parameter. {"import", diff --git a/src/bytecode_vm.cpp b/src/bytecode_vm.cpp index 961f2aa..b92e4d9 100644 --- a/src/bytecode_vm.cpp +++ b/src/bytecode_vm.cpp @@ -360,7 +360,7 @@ void pushBracketedModuleFrame(Evaluator& ev, const CompiledChunk& chunk, const o // because module chunks never contain tail-call opcodes); don't inherit // a pooled function frame's stale true here either. void pushChildrenForwardFrame(Evaluator& ev, const CompiledChunk& chunk, EvalContext evalCtx, - std::uint64_t randsBefore, const oscad::ASTNode& callNode) { + std::uint64_t randsBefore, const oscad::ASTNode& callNode, bool separate) { if (ev.vmCallStack_.size() >= Evaluator::kMaxVmCallStackDepth) { ev.error("Recursion too deep while forwarding children()", callNode); } @@ -381,6 +381,7 @@ void pushChildrenForwardFrame(Evaluator& ev, const CompiledChunk& chunk, EvalCon frame->ownsModuleSplice = true; frame->moduleRandsBefore = randsBefore; frame->moduleSpliceCallNode = &callNode; + frame->separateChildren = separate; ev.vmCallStack_.push_back(std::move(frame)); ev.vmCallBrackets_.emplace_back(std::nullopt); } @@ -505,6 +506,7 @@ Value driveVm(Evaluator& ev, size_t floor) { const bool ownsModuleSplice = finished->ownsModuleSplice; const std::uint64_t moduleRandsBefore = finished->moduleRandsBefore; const oscad::ASTNode* moduleSpliceCallNode = finished->moduleSpliceCallNode; + const bool separateChildren = finished->separateChildren; while (!finished->ctxChain.empty()) finished->ctxChain.pop_back(); // Module frames never fire returnHook (native evalUserModule // never did either -- a module call has no "return value" @@ -514,6 +516,7 @@ Value driveVm(Evaluator& ev, size_t floor) { if (isModule && ownsModuleSplice) { std::vector> children = std::move(ev.treeStack_.back()); ev.treeStack_.pop_back(); + if (separateChildren) markSeparateOperands(children, 0); ev.spliceModuleChildren(std::move(children), moduleRandsBefore, *moduleSpliceCallNode); } if (isFloorFrame) { @@ -1023,6 +1026,11 @@ Value driveVm(Evaluator& ev, size_t floor) { static_cast(f.chunk->nativeStatements[static_cast(ins.a)]); EvalContext scopedCtx = ctx.withScope(callNode->scope() ? callNode->scope() : ctx.scope); ev.checkDebug(*callNode, scopedCtx); + // Same order as evalModularCall's own (csg_resolve.cpp): + // warn, then resolve. Without this a children() typo + // warns only under the interpreter, so no test for that + // warning could run under both engines. + warnUnexpectedBuiltinArgs(ev, *callNode); const std::uint64_t randsBefore = ev.randsCallCount(); auto [args, effCtx] = resolveCallArgs(ev, callNode->arguments, scopedCtx); std::optional fwd = ev.prepareChildrenForward(args, effCtx); @@ -1039,9 +1047,11 @@ Value driveVm(Evaluator& ev, size_t floor) { const CompiledChunk* chunk = (ev.useBytecodeVm() && ev.inResolvePass()) ? ev.lookupOrCompileChildrenListChunk(fwd->nodes) : nullptr; + // Read before fwd->evalCtx is moved from. + const bool separate = fwd->separate; if (chunk) { ev.treeStack_.emplace_back(); - pushChildrenForwardFrame(ev, *chunk, std::move(fwd->evalCtx), randsBefore, *callNode); + pushChildrenForwardFrame(ev, *chunk, std::move(fwd->evalCtx), randsBefore, *callNode, separate); // f.pc deliberately NOT advanced -- resumes when // the pushed frame completes; driveVm's completion // branch runs the splice (isModule && @@ -1065,6 +1075,7 @@ Value driveVm(Evaluator& ev, size_t floor) { } std::vector> children = std::move(ev.treeStack_.back()); ev.treeStack_.pop_back(); + if (separate) markSeparateOperands(children, 0); ev.spliceModuleChildren(std::move(children), randsBefore, *callNode); ++f.pc; } @@ -1304,8 +1315,7 @@ Value driveVm(Evaluator& ev, size_t floor) { } case Op::CsgGroupEnd: { PendingCsgWrap& pending = f.csgWrapStack.back(); - const size_t after = ev.treeStack_.back().size(); - pending.groupSizes.push_back(Value{static_cast(after - pending.groupStartSize)}); + ev.appendGroupSizes(pending.groupSizes, pending.groupStartSize); ++f.pc; break; } diff --git a/src/csg_resolve.cpp b/src/csg_resolve.cpp index 06861bd..f3e73c4 100644 --- a/src/csg_resolve.cpp +++ b/src/csg_resolve.cpp @@ -158,7 +158,12 @@ void Evaluator::spliceModuleChildren(std::vector> child // landed on. for (auto& c : children) c->uncacheable = true; } - if (children.size() > 1) { + // children(separate=true) marked these to start their own operand + // groups, and the group walk only inspects the enclosing frame's top + // level -- so the wrapper would hide the marks. Splice instead. + const bool anySeparate = + std::any_of(children.begin(), children.end(), [](const auto& c) { return c->separateOperand; }); + if (children.size() > 1 && !anySeparate) { auto unionNode = std::make_unique(); unionNode->kind = "union"; unionNode->node = &callNode; diff --git a/src/user_calls.cpp b/src/user_calls.cpp index 261e25b..d4917af 100644 --- a/src/user_calls.cpp +++ b/src/user_calls.cpp @@ -179,6 +179,7 @@ void Evaluator::releaseVmFrame(std::unique_ptr frame) { frame->ownsModuleSplice = false; frame->moduleRandsBefore = 0; frame->moduleSpliceCallNode = nullptr; + frame->separateChildren = false; vmFramePool_.push_back(std::move(frame)); } @@ -932,6 +933,11 @@ Value Evaluator::parentModuleName(int idx) const { std::optional Evaluator::prepareChildrenForward(const CallArgs& args, EvalContext& ctx) { Value idxArg = getArg(args, 0, "index", Value{}); + // Positional slot 1 is accepted as well as the name: adding "separate" + // to the builtin's parameter list already suppresses the "Too many + // unnamed arguments" warning for children(0, true), so reading it + // named-only would silently ignore an argument the author wrote. + const bool separate = truthy(getArg(args, 1, "separate", Value{false})); if (!ctx.childrenNodes || ctx.childrenNodes->empty()) return std::nullopt; const EvalContext* callerCtx = ctx.childrenCallerCtx; if (!callerCtx) return std::nullopt; @@ -977,7 +983,7 @@ std::optional Evaluator::prepareChildrenForward(cons } if (std::holds_alternative(idxArg)) { - return ChildrenForward{std::move(evalCtx), *ctx.childrenNodes}; + return ChildrenForward{std::move(evalCtx), *ctx.childrenNodes, separate}; } // children(N) indexes child *statements*, not output bodies -- a @@ -1041,13 +1047,15 @@ std::optional Evaluator::prepareChildrenForward(cons picked.push_back(geoNodes[static_cast(idx)]); } if (picked.empty()) return std::nullopt; - return ChildrenForward{std::move(evalCtx), std::move(picked)}; + return ChildrenForward{std::move(evalCtx), std::move(picked), separate}; } void Evaluator::builtinChildren(const CallArgs& args, EvalContext& ctx) { std::optional fwd = prepareChildrenForward(args, ctx); if (!fwd) return; + const size_t before = currentTreeFrameSize(); evalChildren(fwd->nodes, fwd->evalCtx); + if (fwd->separate) markSeparateOperands(treeStack_.back(), before); } } // namespace oscadeval diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 8e43227..94b304d 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -20,6 +20,7 @@ add_executable(oscad_eval_tests test_transforms.cpp test_render_expr.cpp test_booleans.cpp + test_children_separate.cpp test_control_flow.cpp test_tail_calls.cpp test_function_builtins.cpp diff --git a/tests/test_children_separate.cpp b/tests/test_children_separate.cpp new file mode 100644 index 0000000..939d20f --- /dev/null +++ b/tests/test_children_separate.cpp @@ -0,0 +1,278 @@ +// children(separate=true) -- hand the forwarded children to the enclosing +// union/difference/intersection as SEPARATE operands instead of as the one +// grouped operand a statement normally contributes. +// +// Every test runs under BOTH engines. That is the standing rule in this +// repo, and here it is load-bearing rather than ceremonial: "group_sizes" +// is built in completely separate code per engine (resolveCsg in +// booleans.cpp vs Op::CsgGroupEnd in bytecode_vm.cpp), and the splice +// itself has three call sites across the two. + +#include "openscad_cpp_evaluator/evaluator.hpp" + +#include "test_helpers.hpp" + +#include + +using namespace oscadeval; +using namespace oscadeval::test; + +namespace { + +// Same reasoning as test_csg_tree.cpp's own copy: the OSCAD_BYTECODE_VM env +// var is cached after its first read, so a test that must exercise a +// specific engine has to force it. +class ScopedVm { +public: + explicit ScopedVm(bool enabled) { Evaluator::setBytecodeVmEnabledForTesting(enabled); } + ~ScopedVm() { Evaluator::setBytecodeVmEnabledForTesting(std::nullopt); } +}; + +double totalVolume(const std::vector& bodies) { + double v = 0.0; + for (const ColoredBody& b : bodies) { + if (b.body) v += b.body->Volume(); + } + return v; +} + +// One big cube with two small cubes fully INSIDE it, disjoint from each +// other. Deliberate: the two subtrahends contribute nothing to a union, so +// the union answer (125000) and each partially-wrong grouping (124000) are +// all numerically distinct from the right one (123000). Overlapping shapes +// would have made several wrong answers land on the same number. +constexpr const char* kThreeChildren = + "{ cube(50, center=true);" + " translate([-15,0,0]) cube(10, center=true);" + " translate([ 15,0,0]) cube(10, center=true); }"; + +constexpr double kCube = 125000.0; // 50^3 +constexpr double kSmall = 1000.0; // 10^3 + +} // namespace + +// -- The feature ---------------------------------------------------------- + +TEST(ChildrenSeparate, DifferenceSubtractsEachForwardedChild) { + for (bool vm : {false, true}) { + ScopedVm guard(vm); + Evaluated e = evalSrc(std::string("module frame() { difference() children(separate=true); }\n" + "frame() ") + kThreeChildren); + EXPECT_NEAR(totalVolume(e.bodies), kCube - 2 * kSmall, 1e-6) << "vm=" << vm; + } +} + +TEST(ChildrenSeparate, MatchesTheHandWrittenDifference) { + // The oracle: the same shape spelled out. These must agree exactly -- + // that is the whole claim of the feature. + for (bool vm : {false, true}) { + ScopedVm guard(vm); + Evaluated sep = evalSrc(std::string("module frame() { difference() children(separate=true); }\n" + "frame() ") + kThreeChildren); + Evaluated hand = evalSrc(std::string("difference() ") + kThreeChildren); + EXPECT_NEAR(totalVolume(sep.bodies), totalVolume(hand.bodies), 1e-6) << "vm=" << vm; + } +} + +TEST(ChildrenSeparate, WithoutTheFlagNothingChanges) { + // Pins the default. A regression here means every existing script that + // forwards children through a difference() silently changed meaning. + for (bool vm : {false, true}) { + ScopedVm guard(vm); + Evaluated e = evalSrc(std::string("module frame() { difference() children(); }\n" + "frame() ") + kThreeChildren); + EXPECT_NEAR(totalVolume(e.bodies), kCube, 1e-6) << "vm=" << vm; + } +} + +TEST(ChildrenSeparate, IntersectionIntersectsEachForwardedChild) { + // The case with no workaround at all: for difference, A-(B|C) == A-B-C, + // so a union of the subtrahends happens to be right. For intersection + // it is not -- A&(B|C) != A&B&C. + for (bool vm : {false, true}) { + ScopedVm guard(vm); + const char* kids = "{ cube([10,10,10]); translate([5,0,0]) cube([10,10,10]); }"; + Evaluated sep = evalSrc(std::string("module m() { intersection() children(separate=true); }\nm() ") + kids); + Evaluated off = evalSrc(std::string("module m() { intersection() children(); }\nm() ") + kids); + EXPECT_NEAR(totalVolume(sep.bodies), 500.0, 1e-6) << "vm=" << vm; // the overlap + EXPECT_NEAR(totalVolume(off.bodies), 1500.0, 1e-6) << "vm=" << vm; // the union + } +} + +TEST(ChildrenSeparate, HonorsAnIndexSelection) { + // The 4th child would change the volume visibly if the selection leaked. + for (bool vm : {false, true}) { + ScopedVm guard(vm); + Evaluated e = evalSrc("module frame() { difference() children([0:2], separate=true); }\n" + "frame() { cube(50, center=true);" + " translate([-15,0,0]) cube(10, center=true);" + " translate([ 15,0,0]) cube(10, center=true);" + " translate([ 0,0,0]) cube(20, center=true); }"); + EXPECT_NEAR(totalVolume(e.bodies), kCube - 2 * kSmall, 1e-6) << "vm=" << vm; + } +} + +// -- Tree shape: the mechanism itself ------------------------------------- + +TEST(ChildrenSeparate, SplicesInsteadOfWrappingAndSplitsGroupSizes) { + for (bool vm : {false, true}) { + ScopedVm guard(vm); + Evaluated e = evalSrc(std::string("module frame() { difference() children(separate=true); }\n" + "frame() ") + kThreeChildren); + ASSERT_EQ(e.tree.size(), 1u) << "vm=" << vm; + const CSGNode& diff = *e.tree[0]; + EXPECT_EQ(diff.kind, "difference") << "vm=" << vm; + // The grouping wrapper would hide the marks from the group walk, so + // it must not be there: three children, three groups of one. + ASSERT_EQ(diff.children.size(), 3u) << "vm=" << vm; + for (const auto& c : diff.children) { + EXPECT_NE(c->kind, "union") << "vm=" << vm; + } + const ListPtr* sizes = std::get_if(&diff.params.at("group_sizes")); + ASSERT_NE(sizes, nullptr) << "vm=" << vm; + ASSERT_EQ((*sizes)->items.size(), 3u) << "vm=" << vm; + for (const Value& s : (*sizes)->items) EXPECT_EQ(std::get(s), 1.0) << "vm=" << vm; + } +} + +TEST(ChildrenSeparate, WithoutTheFlagStillWrapsIntoOneGroup) { + for (bool vm : {false, true}) { + ScopedVm guard(vm); + Evaluated e = evalSrc(std::string("module frame() { difference() children(); }\n" + "frame() ") + kThreeChildren); + ASSERT_EQ(e.tree.size(), 1u) << "vm=" << vm; + const CSGNode& diff = *e.tree[0]; + ASSERT_EQ(diff.children.size(), 1u) << "vm=" << vm; + EXPECT_EQ(diff.children[0]->kind, "union") << "vm=" << vm; + EXPECT_FALSE(diff.children[0]->isBuiltin) << "vm=" << vm; + const ListPtr* sizes = std::get_if(&diff.params.at("group_sizes")); + ASSERT_NE(sizes, nullptr) << "vm=" << vm; + ASSERT_EQ((*sizes)->items.size(), 1u) << "vm=" << vm; + EXPECT_EQ(std::get((*sizes)->items[0]), 1.0) << "vm=" << vm; + } +} + +// -- Degradation: it must not leak ---------------------------------------- + +TEST(ChildrenSeparate, UnderATransformDegradesToOneOperand) { + // The children land in translate()'s own frame, so difference() sees the + // single translate node. Nothing to separate -- the union is correct. + for (bool vm : {false, true}) { + ScopedVm guard(vm); + Evaluated e = evalSrc(std::string("module frame() { difference() translate([0,0,0]) children(separate=true); }\n" + "frame() ") + kThreeChildren); + EXPECT_NEAR(totalVolume(e.bodies), kCube, 1e-6) << "vm=" << vm; + } +} + +TEST(ChildrenSeparate, DoesNotAffectALaterUnrelatedDifference) { + // A separated splice under a non-CSG parent must not colour the grouping + // of a difference() elsewhere in the same evaluation -- including one + // whose first statement happens to produce the same number of nodes. + for (bool vm : {false, true}) { + ScopedVm guard(vm); + Evaluated e = evalSrc("module m() { translate([200,0,0]) children(separate=true); }\n" + "m() { cube(1); cube(2); cube(3); }\n" + "difference() { cube(50, center=true); translate([-15,0,0]) cube(10, center=true); }"); + // 1 + 8 + 27 from the spliced group, plus a normal difference. + EXPECT_NEAR(totalVolume(e.bodies), 36.0 + kCube - kSmall, 1e-6) << "vm=" << vm; + } +} + +TEST(ChildrenSeparate, PropagatesThroughAUserModuleWrapper) { + // A judgement call, pinned so it cannot drift silently: the mark rides + // on the nodes, so a module whose whole body is children(separate=true) + // passes the separateness outward to whatever encloses the call. + // Retreating to "stop at the module boundary" is any_of -> all_of in + // spliceModuleChildren. + for (bool vm : {false, true}) { + ScopedVm guard(vm); + Evaluated e = evalSrc(std::string("module pass() { children(separate=true); }\n" + "difference() pass() ") + kThreeChildren); + EXPECT_NEAR(totalVolume(e.bodies), kCube - 2 * kSmall, 1e-6) << "vm=" << vm; + } +} + +// -- Degenerate cases ----------------------------------------------------- + +TEST(ChildrenSeparate, OneChildIsASilentNoOp) { + for (bool vm : {false, true}) { + ScopedVm guard(vm); + std::vector warnings; + Evaluated e = evalSrc("module frame() { difference() children(separate=true); }\n" + "frame() { cube(3); }", + [&](const std::string& m) { warnings.push_back(m); }); + EXPECT_NEAR(totalVolume(e.bodies), 27.0, 1e-6) << "vm=" << vm; + EXPECT_TRUE(warnings.empty()) << "vm=" << vm; + } +} + +TEST(ChildrenSeparate, NoChildrenIsASilentNoOp) { + for (bool vm : {false, true}) { + ScopedVm guard(vm); + std::vector warnings; + Evaluated e = evalSrc("module frame() { difference() children(separate=true); }\nframe();", + [&](const std::string& m) { warnings.push_back(m); }); + EXPECT_TRUE(e.bodies.empty()) << "vm=" << vm; + EXPECT_TRUE(warnings.empty()) << "vm=" << vm; + } +} + +TEST(ChildrenSeparate, LeavesUnionAndHullUnchanged) { + // union() gives the same geometry either way, and hull()/minkowski() + // read bodies rather than groups -- the wrapper they used to receive was + // plain concatenation, so they already saw the children separately. + // This is the executable form of that claim. + for (bool vm : {false, true}) { + ScopedVm guard(vm); + for (const char* op : {"union", "hull"}) { + const std::string on = std::string("module m() { ") + op + "() children(separate=true); }\nm() "; + const std::string off = std::string("module m() { ") + op + "() children(); }\nm() "; + Evaluated a = evalSrc(on + kThreeChildren); + Evaluated b = evalSrc(off + kThreeChildren); + EXPECT_NEAR(totalVolume(a.bodies), totalVolume(b.bodies), 1e-6) << "vm=" << vm << " op=" << op; + } + } +} + +// -- The argument itself -------------------------------------------------- + +TEST(ChildrenSeparate, TheArgumentIsAccepted) { + for (bool vm : {false, true}) { + ScopedVm guard(vm); + std::vector warnings; + evalSrc("module m() { children(separate=true); }\nm() { cube(1); cube(2); }", + [&](const std::string& m) { warnings.push_back(m); }); + EXPECT_TRUE(warnings.empty()) << "vm=" << vm; + } +} + +TEST(ChildrenSeparate, AMisspelledArgumentStillWarnsUnderBothEngines) { + // Op::CallChildren did not call warnUnexpectedBuiltinArgs, so a typo was + // silent on the compiled path only. That gap is why this test can exist. + for (bool vm : {false, true}) { + ScopedVm guard(vm); + std::vector warnings; + evalSrc("module m() { children(seperate=true); }\nm() { cube(1); cube(2); }", + [&](const std::string& m) { warnings.push_back(m); }); + bool found = false; + for (const std::string& w : warnings) { + if (w.find("seperate") != std::string::npos) found = true; + } + EXPECT_TRUE(found) << "vm=" << vm << ", warnings=" << warnings.size(); + } +} + +TEST(ChildrenSeparate, AcceptsThePositionalSecondArgument) { + // Adding "separate" to the builtin's parameter list already silences the + // "Too many unnamed arguments" warning for this shape, so it must be + // read rather than silently ignored. + for (bool vm : {false, true}) { + ScopedVm guard(vm); + Evaluated e = evalSrc("module frame() { difference() children([0:2], true); }\n" + "frame() { cube(50, center=true);" + " translate([-15,0,0]) cube(10, center=true);" + " translate([ 15,0,0]) cube(10, center=true); }"); + EXPECT_NEAR(totalVolume(e.bodies), kCube - 2 * kSmall, 1e-6) << "vm=" << vm; + } +}