From a3db1cef890645c8b57992d96c4322391f4839a5 Mon Sep 17 00:00:00 2001 From: Revar Desmera Date: Thu, 20 Aug 2026 12:07:11 -0700 Subject: [PATCH 1/4] Measure geometry from an expression: render() returns an object() obj = render() { difference() { cube(100); sphere(20); } }; echo(obj.volume, obj.genus, obj.boundingbox); polyhedron(obj); OpenSCAD could not previously measure its own geometry: the numbers only exist after the geometry is built, and the language had no way to reach them. This adds the evaluator half (the parser half is the submodule bump in this same commit). It measures and DISCARDS -- nothing is drawn. That is what makes the semantics uniform: no side effects, so top level, module bodies, function bodies, list comprehensions and ternaries all behave identically, and function purity survives. `function fits(w) = render() { cube(w); }.volume < 500;` works with no exception carved out for it. The two-pass problem needed no new pipeline. generatePartialTree() already generates mid-resolve for the debugger's live render, and documents why it is safe (the resolve pass never reads CSGNode::bodies). measureCsgSubtree does the same thing for one subtree, on its own treeStack_ frame, which is popped and discarded so it can never reach the drawn tree. Discarding costs one guard. measuring_ suppresses the four writes that exist solely to describe DRAWN geometry -- idToNode/idToColor in tagGenerated and tagDisplayOnly, the restampCachedIds call on a cache hit, and cacheProducer_ -- because those tables are cleared once per pass, so a leak is permanent and surfaces much later as wrong click-to-source. The geometry cache itself deliberately stays ON: cacheKey is content-addressed, so an entry a measurement stores is genuinely reusable by the real render. The same flag suppresses checkDebug, which would otherwise inject stops at the paused statement's own callStack_ depth and corrupt lastStmtByDepth_'s duplicate-collapse state. Mesh output is VNF-shaped -- [x,y,z] vertices, 0-based faces, CLOCKWISE seen from outside -- so it feeds polyhedron() and BOSL2 unchanged. Two details there are load-bearing and silent when wrong: the winding is reversed on the way out (Manifold's triVerts is CCW), and vertices are welded by exact position (Manifold splits property-vertices, so without the weld the round-tripped polyhedron is an OPEN mesh). Both are pinned by a round-trip test that asserts a POSITIVE volume -- verified empirically that a reversed mesh comes back as volume -1 with Status()==NoError, so the sign is the whole signal and abs() would hide it. polyhedron() and polygon() now accept the object directly, so the round trip is one call rather than two arguments. Works for any object with the keys, not just one render() produced. Measurements come from Manifold (Volume/SurfaceArea/Genus/BoundingBox), never hand-computed. An empty result reports dim=0 with boundingbox=undef rather than Manifold's empty Box, which is {+inf,-inf} and would poison downstream arithmetic. An open surface still returns its mesh, with volume and genus unavailable. Not yet done: the bytecode VM currently falls back to the interpreter for a declaration containing a render expression, via compileExpr's existing NotCompilable safety net. A real Kind::Measure bracket follows. Co-Authored-By: Claude Opus 5 (1M context) --- external/openscad_cpp_parser | 2 +- include/openscad_cpp_evaluator/evaluator.hpp | 57 ++++ src/CMakeLists.txt | 1 + src/builtins/builtins.hpp | 18 ++ src/builtins/primitives_2d.cpp | 17 + src/builtins/primitives_3d.cpp | 19 ++ src/csg_generate.cpp | 27 +- src/debug_profile.cpp | 8 +- src/expr_eval.cpp | 3 + src/measure_geometry.cpp | 319 +++++++++++++++++++ tests/CMakeLists.txt | 1 + tests/test_render_expr.cpp | 267 ++++++++++++++++ 12 files changed, 730 insertions(+), 9 deletions(-) create mode 100644 src/measure_geometry.cpp create mode 100644 tests/test_render_expr.cpp diff --git a/external/openscad_cpp_parser b/external/openscad_cpp_parser index b621014..49e83e7 160000 --- a/external/openscad_cpp_parser +++ b/external/openscad_cpp_parser @@ -1 +1 @@ -Subproject commit b6210145687e73b1034dca2687d018d25fa6fdc9 +Subproject commit 49e83e74e6713973a3c3b64007854da3cd7df87f diff --git a/include/openscad_cpp_evaluator/evaluator.hpp b/include/openscad_cpp_evaluator/evaluator.hpp index 256aad6..e67f8fc 100644 --- a/include/openscad_cpp_evaluator/evaluator.hpp +++ b/include/openscad_cpp_evaluator/evaluator.hpp @@ -217,6 +217,26 @@ class Evaluator { // `_generate_partial_render`. std::vector generatePartialTree(); + // Consumes a resolved CSG subtree: generates it, applies the top-level + // dimension rules, folds the surviving bodies into one, and returns the + // render()-expression measurement object(). The subtree is destroyed on + // return -- NOTHING is drawn. + // + // Callers must already have measuring_ set (see its doc comment): the + // generate inside would otherwise write provenance for geometry that is + // about to be discarded. The flag is the caller's to own because the VM + // holds it across two separate op handlers. + // + // THE only implementation of "CSG subtree -> object()". Both engines call + // exactly this -- the interpreter from evalRenderExpr, the VM from + // Op::PopBuiltinWrap's Kind::Measure branch. + Value measureCsgSubtree(std::vector> sub, const oscad::ASTNode& node); + + // evalExpr's arm for `render() { ... }` in expression position. Pushes + // its own treeStack_ frame, resolves the children into it, then hands + // the frame to measureCsgSubtree and discards it. + Value evalRenderExpr(const oscad::RenderExpression& node, EvalContext& ctx); + // Provenance tables populated by tagGenerated() during generate -- // originalID -> the AST node that produced it / that node's own // resolved color. Public, read by a caller (CLI, WYSIWYG picking) after @@ -1084,6 +1104,43 @@ class Evaluator { // whether evalExprMaybeCompiled's cache is safe to touch right now. bool inResolvePass_ = false; + // True only while evalRenderExpr (or the VM's Kind::Measure bracket) is + // building geometry for a render() EXPRESSION -- geometry that is + // measured and then thrown away, never drawn. + // + // It suppresses everything that exists solely to describe DRAWN + // geometry, and nothing else: + // - idToNode/idToColor writes in tagGenerated/tagDisplayOnly, and the + // restampCachedIds call on a cache hit (which writes them too). + // Those maps are cleared once per pass, at resolveTreeImpl entry, so + // without this a discarded measurement would leave permanent + // provenance entries for triangles no render ever draws. + // - cacheProducer_ writes, which decide click-to-source attribution + // for a LATER cache hit. A discarded node must never become the + // recorded producer of geometry the real render then reuses. + // - checkDebug, so resolving a measured subtree does not inject + // debug stops at the paused statement's own callStack_ depth and + // corrupt lastStmtByDepth_'s duplicate-collapse state. + // + // The ManifoldCache itself deliberately stays ON: cacheKey is a pure + // function of kind/params/children, so an entry stored by a measurement + // is genuinely reusable by the real render (and vice versa) -- that is + // the point. Only the producer attribution above needed suppressing. + // + // Save/restore rather than a plain set/clear, so nesting is correct for + // free. The interpreter uses an RAII guard; the VM stores the saved + // value in its bracket (PendingBuiltinWrap::savedMeasuring). + bool measuring_ = false; + +public: + // Test-only accessors for the invariants above -- a leaked measuring_ or + // an unbalanced treeStack_ is otherwise silent until something much + // later goes mysteriously wrong. + bool measuringForTesting() const { return measuring_; } + size_t treeStackDepthForTesting() const { return treeStack_.size(); } + +private: + public: // lookupOrCompileChunk/lookupCompiledLiteralChunk: public (not just // private helpers of evalUserFunction*/evalFunctionLiteral* anymore) diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 6976a67..eeebde4 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -11,6 +11,7 @@ add_library(openscad_cpp_evaluator STATIC css_colors.cpp csg_resolve.cpp csg_generate.cpp + measure_geometry.cpp manifold_cache.cpp debug_profile.cpp eval_use.cpp diff --git a/src/builtins/builtins.hpp b/src/builtins/builtins.hpp index 1698ee7..96f82be 100644 --- a/src/builtins/builtins.hpp +++ b/src/builtins/builtins.hpp @@ -90,6 +90,24 @@ struct RoleSplit { }; RoleSplit splitByRole(const std::vector& bodies); +// polyhedron(obj) / polygon(obj): a render()-expression result -- or any +// object() carrying the same keys -- may stand in for the two separate list +// arguments, so the round trip is `polyhedron(render() { ... })` rather than +// `polyhedron(obj.vertices, obj.faces)`. Returns nullptr when `v` is not an +// object or has no such key. +inline const Value* objectFieldOrNull(const Value& v, const std::string& key) { + const ObjectPtr* o = std::get_if(&v); + if (!o || !*o) return nullptr; + for (const std::pair& kv : (*o)->items) { + if (kv.first == key) return &kv.second; + } + return nullptr; +} +inline bool isObject(const Value& v) { + const ObjectPtr* o = std::get_if(&v); + return o && *o; +} + // manifold::ToString(Manifold::Error) only exists under MANIFOLD_DEBUG -- // not enabled in this build -- so this mirrors it. Shared by import.cpp and // primitives_3d.cpp, both of which report why a mesh wouldn't build. diff --git a/src/builtins/primitives_2d.cpp b/src/builtins/primitives_2d.cpp index 741f331..782282d 100644 --- a/src/builtins/primitives_2d.cpp +++ b/src/builtins/primitives_2d.cpp @@ -52,6 +52,23 @@ CSGParams resolve2d(Evaluator& ev, const oscad::ModularCall& node, EvalContext& // polygon Value pointsArg = getArg(args, 0, "points", Value{}); Value pathsArg = getArg(args, 1, "paths", Value{}); + + // polygon(obj) -- the 2D counterpart of polyhedron(obj): an object() + // with `vertices` (and optionally `paths`) stands in for the two lists. + if (isObject(pointsArg)) { + const Value* verts = objectFieldOrNull(pointsArg, "vertices"); + if (!verts) verts = objectFieldOrNull(pointsArg, "points"); + if (!verts) ev.error("polygon: object has no 'vertices' (or 'points') key", node); + const Value* paths = objectFieldOrNull(pointsArg, "paths"); + // Copy before assigning -- both borrow into pointsArg's ObjectPtr. + Value newPoints = *verts; + Value newPaths = paths ? *paths : Value{}; + pointsArg = std::move(newPoints); + // A missing `paths` stays undef, which already means "pts is one + // single contour" below -- no error, unlike polyhedron's `faces`. + pathsArg = std::move(newPaths); + } + const ListPtr* pointsList = std::get_if(&pointsArg); if (!pointsList || !*pointsList) { ev.error("polygon: 'points' is required", node); diff --git a/src/builtins/primitives_3d.cpp b/src/builtins/primitives_3d.cpp index a70dc80..67f7e2f 100644 --- a/src/builtins/primitives_3d.cpp +++ b/src/builtins/primitives_3d.cpp @@ -610,6 +610,25 @@ CSGParams resolvePolyhedron(Evaluator& ev, const oscad::ModularCall& node, EvalC Value facesArg = getArg(args, 1, "faces", Value{}); if (isUndef(facesArg)) facesArg = getArg(args, 1, "triangles", Value{}); // legacy alias + // polyhedron(obj) -- an object() with `vertices` and `faces` stands in + // for the two lists, so a render() expression round-trips in one call. + // Works for ANY such object, not just one this evaluator produced, so a + // script can build or transform its own. `points` is accepted as an + // alias for `vertices` since that is this argument's own name. + if (isObject(pointsArg)) { + const Value* verts = objectFieldOrNull(pointsArg, "vertices"); + if (!verts) verts = objectFieldOrNull(pointsArg, "points"); + const Value* faces = objectFieldOrNull(pointsArg, "faces"); + if (!verts) ev.error("polyhedron: object has no 'vertices' (or 'points') key", node); + if (!faces) ev.error("polyhedron: object has no 'faces' key", node); + // Copy BEFORE assigning: both pointers borrow into pointsArg's own + // ObjectPtr, which the first assignment would release. + Value newPoints = *verts; + Value newFaces = *faces; + pointsArg = std::move(newPoints); + facesArg = std::move(newFaces); + } + const ListPtr* pointsList = std::get_if(&pointsArg); const ListPtr* facesList = std::get_if(&facesArg); if (!pointsList || !*pointsList || !facesList || !*facesList) { diff --git a/src/csg_generate.cpp b/src/csg_generate.cpp index 24bc4ad..35b4254 100644 --- a/src/csg_generate.cpp +++ b/src/csg_generate.cpp @@ -48,7 +48,11 @@ std::vector Evaluator::generateTreeImpl(const std::vector std::optional> cached = key ? manifoldCache_->get(*key) : std::nullopt; if (cached) { node.bodies = std::move(*cached); - if (node.node) { + // Skipped entirely while measuring: restampCachedIds writes + // idToNode/idToColor, and geometry that is about to be discarded + // has no click-to-source identity worth recording. See + // Evaluator::measuring_. + if (node.node && !measuring_) { auto producer = cacheProducer_.find(*key); restampCachedIds(node.bodies, *node.node, producer == cacheProducer_.end() ? nullptr : producer->second); @@ -83,8 +87,13 @@ std::vector Evaluator::generateTreeImpl(const std::vector node.bodies = flattenCsgTree(node.children); } if (key) { + // The geometry itself is still cached while measuring -- + // cacheKey is content-addressed, so the real render can + // legitimately reuse it. Only the PRODUCER attribution is + // suppressed: a node that gets discarded must never be + // recorded as the origin of geometry a later render draws. manifoldCache_->put(*key, node.bodies); - cacheProducer_[*key] = node.node; + if (!measuring_) cacheProducer_[*key] = node.node; } } for (const ColoredBody& b : node.bodies) topLevelBodies.push_back(b); @@ -271,9 +280,11 @@ void Evaluator::restampCachedIds(std::vector& bodies, const oscad:: ColoredBody Evaluator::tagGenerated(manifold::Manifold body, const oscad::ASTNode& node, const Value& colorValue) { manifold::MeshGL mesh = body.GetMeshGL(); std::optional> color = valueToColor(colorValue); - for (uint32_t originalId : mesh.runOriginalID) { - idToNode[originalId] = &node; - idToColor[originalId] = color; + if (!measuring_) { + for (uint32_t originalId : mesh.runOriginalID) { + idToNode[originalId] = &node; + idToColor[originalId] = color; + } } ColoredBody cb; cb.body = std::move(body); @@ -291,8 +302,10 @@ ColoredBody Evaluator::tagDisplayOnly(manifold::MeshGL mesh, const oscad::ASTNod const uint32_t originalId = manifold::Manifold::ReserveIDs(1); mesh.runOriginalID = {originalId}; mesh.runIndex = {0, static_cast(mesh.triVerts.size())}; - idToNode[originalId] = &node; - idToColor[originalId] = color; + if (!measuring_) { + idToNode[originalId] = &node; + idToColor[originalId] = color; + } ColoredBody cb; // Left set-but-empty rather than nullopt: consumers dereference diff --git a/src/debug_profile.cpp b/src/debug_profile.cpp index ebf8624..807c8ac 100644 --- a/src/debug_profile.cpp +++ b/src/debug_profile.cpp @@ -98,7 +98,13 @@ std::vector Evaluator::buildDebugFrames(const EvalContext* ctx) cons void Evaluator::checkDebug(const oscad::ASTNode& node, EvalContext& ctx, bool forced, bool exprLevel, bool callSite) { - if (!debugHooks_.debugHook) return; + // measuring_: a render() EXPRESSION resolves its children mid-expression, + // so every statement inside it would otherwise fire a checkpoint at the + // SAME callStack_ depth as the statement the debugger is paused on -- + // injecting stops the user never wrote and corrupting lastStmtByDepth_'s + // duplicate-collapse state below. Rides the existing null-hook + // short-circuit so it costs nothing on the normal path. + if (measuring_ || !debugHooks_.debugHook) return; const oscad::Position& pos = node.position(); const size_t stmtDepth = callStack_.size(); if (lastStmtByDepth_.size() <= stmtDepth) lastStmtByDepth_.resize(stmtDepth + 1, {-1, std::string{}}); diff --git a/src/expr_eval.cpp b/src/expr_eval.cpp index 1cdbd06..e191d96 100644 --- a/src/expr_eval.cpp +++ b/src/expr_eval.cpp @@ -541,6 +541,9 @@ Value Evaluator::evalExpr(const oscad::Expression& node, EvalContext& ctx) { n.position()); } + case NodeKind::RenderExpression: + return evalRenderExpr(static_cast(node), ctx); + default: throw std::logic_error(std::string("Evaluator::evalExpr: NodeKind '") + oscad::nodeKindName(node.kind()) + "' not yet implemented (later phase)"); diff --git a/src/measure_geometry.cpp b/src/measure_geometry.cpp new file mode 100644 index 0000000..137af5f --- /dev/null +++ b/src/measure_geometry.cpp @@ -0,0 +1,319 @@ +// `render()` in EXPRESSION position: builds geometry, measures it, and +// throws the geometry away. +// +// obj = render() { difference() { cube(100); sphere(20); } }; +// echo(obj.volume, obj.genus); +// polyhedron(obj); +// +// This is the ONLY implementation of "resolved CSG subtree -> object()". +// Both engines call measureCsgSubtree: the interpreter from evalRenderExpr +// (expr_eval.cpp), the VM from Op::PopBuiltinWrap's Kind::Measure branch. +// Keeping it in one place is what stops the two engines drifting. +// +// Nothing here draws. The caller has already popped the subtree off +// treeStack_, so it never reaches the drawn tree, and Evaluator::measuring_ +// is set for the whole generate below -- see its doc comment for the four +// provenance writes that suppresses and why the geometry cache stays on. + +#include "openscad_cpp_evaluator/evaluator.hpp" + +#include "builtins/builtins.hpp" + +#include + +#include +#include +#include +#include + +namespace oscadeval { + +namespace { + +Value listOf(std::vector items) { + return Value{std::make_shared(ValueList{std::move(items)})}; +} + +Value pointOf(double x, double y, double z) { return listOf({Value{x}, Value{y}, Value{z}}); } +Value pointOf(double x, double y) { return listOf({Value{x}, Value{y}}); } + +Value objectOfPairs(std::vector> items) { + return Value{std::make_shared(ValueObject{std::move(items)})}; +} + +// Manifold mesh -> VNF halves. +// +// Two things here are load-bearing and both are silent when wrong: +// +// 1. WINDING IS REVERSED. Manifold's triVerts is counter-clockwise seen +// from outside (mesh.h); VNF and polyhedron() are clockwise +// (BOSL2/vnf.scad). This is the exact inverse of resolvePolyhedron's +// intake, which reverses on the way in (primitives_3d.cpp). Getting it +// wrong does NOT fail a round-trip -- Manifold happily rebuilds a +// reversed-but-closed mesh -- it just yields inside-out normals that +// only a slicer or BOSL2's vnf_validate notices. +// +// 2. THE VERTEX WELD IS NOT COSMETIC. Manifold splits property-vertices, so +// the raw vertex list has duplicates at every seam. Without deduping by +// exact position the round-tripped polyhedron() is an OPEN mesh and +// lands in isDrawableFailure. Same approach as import.cpp's meshToVnf. +// +// Not reusing meshToVnf itself: it is file-local to import.cpp, takes a +// LoadedMesh rather than a Manifold, has no winding-reversal option, and +// returns the wrapped [[verts],[faces]] 2-list when what is needed here is +// the two halves as separate object keys. Adapting it would cost more than +// this does. +// ponytail: emits triangles, not merged N-gons. VNF and polyhedron() both +// accept triangles. MeshGL64::faceID would allow merging coplanar triangles +// back into quads, but that needs boundary-loop extraction -- do it only if +// someone actually wants prettier output. +template +void meshToVertsAndFaces(const MeshT& mesh, Value& vertsOut, Value& facesOut) { + std::map, int> vertMap; + std::vector verts; + std::vector faces; + + const size_t numProp = mesh.numProp == 0 ? 3 : static_cast(mesh.numProp); + const size_t numVert = mesh.vertProperties.size() / (numProp == 0 ? 1 : numProp); + + const auto indexOf = [&](uint32_t v) -> int { + if (static_cast(v) >= numVert) return -1; + const size_t base = static_cast(v) * numProp; + const std::array pos = {static_cast(mesh.vertProperties[base + 0]), + static_cast(mesh.vertProperties[base + 1]), + static_cast(mesh.vertProperties[base + 2])}; + auto it = vertMap.find(pos); + if (it != vertMap.end()) return it->second; + const int idx = static_cast(verts.size()); + vertMap.emplace(pos, idx); + verts.push_back(pointOf(pos[0], pos[1], pos[2])); + return idx; + }; + + for (size_t t = 0; t + 2 < mesh.triVerts.size(); t += 3) { + const int a = indexOf(mesh.triVerts[t + 0]); + const int b = indexOf(mesh.triVerts[t + 1]); + const int c = indexOf(mesh.triVerts[t + 2]); + if (a < 0 || b < 0 || c < 0) continue; + // a, c, b -- see (1) above. + faces.push_back(listOf({Value{static_cast(a)}, Value{static_cast(c)}, + Value{static_cast(b)}})); + } + + vertsOut = listOf(std::move(verts)); + facesOut = listOf(std::move(faces)); +} + +// 2D: CrossSection -> flat `vertices` plus an index `paths` list, matching +// polygon(points=, paths=)'s own argument shape. Deliberately not +// import.cpp's contoursToValue, which emits nested point lists with no +// index split -- the wrong shape for a `paths` key. +void sectionToVertsAndPaths(const manifold::CrossSection& cs, Value& vertsOut, Value& pathsOut, double& perimeter) { + std::vector verts; + std::vector paths; + perimeter = 0.0; + for (const auto& poly : cs.ToPolygons()) { + std::vector path; + path.reserve(poly.size()); + for (size_t i = 0; i < poly.size(); ++i) { + path.push_back(Value{static_cast(verts.size())}); + verts.push_back(pointOf(poly[i].x, poly[i].y)); + // Closed loop: the last vertex's edge runs back to the first. + const auto& next = poly[(i + 1) % poly.size()]; + perimeter += std::hypot(next.x - poly[i].x, next.y - poly[i].y); + } + paths.push_back(listOf(std::move(path))); + } + vertsOut = listOf(std::move(verts)); + pathsOut = listOf(std::move(paths)); +} + +Value boxValue3d(const manifold::Box& b) { + return listOf({pointOf(b.min.x, b.min.y, b.min.z), pointOf(b.max.x, b.max.y, b.max.z)}); +} + +// dim = 0: no geometry at all. Every 3D key is still present so the object's +// shape is stable and `obj.volume` never errors -- but boundingbox is undef, +// NOT Manifold's empty Box, which is {+inf, -inf} and would poison any +// arithmetic a script does with it. +Value emptyMeasurement() { + return objectOfPairs({ + {"vertices", listOf({})}, + {"faces", listOf({})}, + {"volume", Value{0.0}}, + {"area", Value{0.0}}, + {"genus", Value{0.0}}, + {"boundingbox", Value{}}, + {"dim", Value{0.0}}, + {"vnf", listOf({listOf({}), listOf({})})}, + }); +} + +} // namespace + +// The interpreter half. Resolves the children into their OWN treeStack_ +// frame, generates that frame, measures it, and discards it -- the VM's +// Kind::Measure bracket does the same three steps as two opcodes. +Value Evaluator::evalRenderExpr(const oscad::RenderExpression& node, EvalContext& ctx) { + // evalExpr is also reachable outside a resolve pass (the debug REPL), + // where there is no frame to push onto and no geometry to speak of. + if (!inResolvePass_ || treeStack_.empty()) { + warn("render(): geometry expression is only valid during evaluation", &node.position()); + return Value{}; + } + + // Resolve arguments purely for the $-propagation side effect -- exactly + // what resolveRender does for the statement form. `convexity` is + // accepted and ignored; `$fn` and friends reach the children via effCtx. + auto [args, effCtx] = resolveCallArgs(*this, node.arguments, ctx); + (void)args; + + // Save/restore rather than set/clear: nesting (a render() inside a + // render()'s children) then works for free rather than needing an error. + const bool savedMeasuring = measuring_; + measuring_ = true; + + treeStack_.emplace_back(); + std::vector> sub; + try { + evalChildren(node.children, effCtx); + sub = std::move(treeStack_.back()); + treeStack_.pop_back(); + } catch (...) { + // buildTreeNode's own shape: the frame must come off on the throw + // path too, or every later resolve accumulates into a dead level. + treeStack_.pop_back(); + measuring_ = savedMeasuring; + throw; + } + + Value result; + try { + // measuring_ must still be TRUE here -- it is what suppresses the + // provenance writes inside the generate. + result = measureCsgSubtree(std::move(sub), node); + } catch (...) { + measuring_ = savedMeasuring; + throw; + } + measuring_ = savedMeasuring; + return result; +} + +Value Evaluator::measureCsgSubtree(std::vector> sub, const oscad::ASTNode& node) { + // Callers must already have measuring_ set -- the generate below writes + // provenance otherwise. Asserting rather than setting it here because + // the VM's bracket owns the flag's lifetime across two op handlers. + std::vector ptrs; + ptrs.reserve(sub.size()); + for (const std::unique_ptr& n : sub) ptrs.push_back(n.get()); + + generateTreeImpl(ptrs); + // Same two lines generateTree() runs over the real top level: a render() + // block is an implicit union, so it gets the same Group dimension + // treatment -- which is also what warns about (and drops) mixed 2D/3D. + // 0 is DimRule::Group; the enum itself is file-local to csg_generate.cpp, + // which is why applyDimensionRulesTo takes an int (see its declaration). + applyDimensionRulesTo(ptrs, /*DimRule::Group=*/0); + + std::vector bodies; + for (CSGNode* n : ptrs) { + for (const ColoredBody& b : n->bodies) bodies.push_back(b); + } + + const RoleSplit split = splitByRole(bodies); + + // An open surface (polyhedron() with boundary edges) has no Manifold to + // measure, but the script should still get its mesh back. The producing + // builtin has already warned with the boundary-edge count, so don't + // repeat that detail here. + if (split.foreground.empty() && !split.displayOnly.empty()) { + const ColoredBody& cb = split.displayOnly.front(); + Value verts, faces; + meshToVertsAndFaces(*cb.rawMesh, verts, faces); + warn("render(): result is not a closed solid; volume and genus are unavailable", &node.position()); + // Bounding box by hand: there is no Manifold to ask. + double lo[3] = {0, 0, 0}, hi[3] = {0, 0, 0}; + bool any = false; + const size_t numProp = cb.rawMesh->numProp == 0 ? 3 : static_cast(cb.rawMesh->numProp); + for (size_t i = 0; i + numProp <= cb.rawMesh->vertProperties.size(); i += numProp) { + for (int k = 0; k < 3; ++k) { + const double v = static_cast(cb.rawMesh->vertProperties[i + static_cast(k)]); + if (!any || v < lo[k]) lo[k] = v; + if (!any || v > hi[k]) hi[k] = v; + } + any = true; + } + return objectOfPairs({ + {"vertices", verts}, + {"faces", faces}, + {"volume", Value{0.0}}, + {"area", Value{0.0}}, + {"genus", Value{}}, + {"boundingbox", any ? listOf({pointOf(lo[0], lo[1], lo[2]), pointOf(hi[0], hi[1], hi[2])}) : Value{}}, + {"dim", Value{3.0}}, + {"vnf", listOf({verts, faces})}, + }); + } + + if (split.foreground.empty()) return emptyMeasurement(); + + // Drop operands Manifold considers broken before any union: the same + // rule generateCsg enforces -- a non-NoError body must never reach + // operator+, or it silently zeroes the whole result. + std::vector usable; + manifold::Manifold::Error firstError = manifold::Manifold::Error::NoError; + for (const ColoredBody& b : split.foreground) { + if (b.body && b.body->Status() != manifold::Manifold::Error::NoError) { + if (firstError == manifold::Manifold::Error::NoError) firstError = b.body->Status(); + continue; + } + usable.push_back(b); + } + if (usable.empty()) { + if (firstError != manifold::Manifold::Error::NoError) { + warn("render(): " + manifoldErrorName(firstError) + "; nothing to measure", &node.position()); + } + return emptyMeasurement(); + } + + const ColoredBody merged = combineBodies(usable); + + if (merged.section) { + Value verts, paths; + double perimeter = 0.0; + sectionToVertsAndPaths(*merged.section, verts, paths, perimeter); + const manifold::Rect r = merged.section->Bounds(); + const bool empty = merged.section->IsEmpty(); + return objectOfPairs({ + {"vertices", verts}, + {"paths", paths}, + {"area", Value{merged.section->Area()}}, + {"perimeter", Value{perimeter}}, + {"boundingbox", empty ? Value{} : listOf({pointOf(r.min.x, r.min.y), pointOf(r.max.x, r.max.y)})}, + {"dim", Value{2.0}}, + }); + } + + if (!merged.body || merged.body->IsEmpty()) return emptyMeasurement(); + + // GetMeshGL64, not GetMeshGL: MeshGL is MeshGLP, so a 100mm cube's + // vertices would come back float-rounded rather than exact. + Value verts, faces; + meshToVertsAndFaces(merged.body->GetMeshGL64(), verts, faces); + + return objectOfPairs({ + {"vertices", verts}, + {"faces", faces}, + {"volume", Value{merged.body->Volume()}}, + {"area", Value{merged.body->SurfaceArea()}}, + {"genus", Value{static_cast(merged.body->Genus())}}, + {"boundingbox", boxValue3d(merged.body->BoundingBox())}, + {"dim", Value{3.0}}, + // Every BOSL2 function takes the 2-list, not two arguments. Shares + // the same two children, so it costs one ValueList. + {"vnf", listOf({verts, faces})}, + }); +} + +} // namespace oscadeval diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 03b90dc..8e43227 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -18,6 +18,7 @@ add_executable(oscad_eval_tests test_export.cpp test_primitives.cpp test_transforms.cpp + test_render_expr.cpp test_booleans.cpp test_control_flow.cpp test_tail_calls.cpp diff --git a/tests/test_render_expr.cpp b/tests/test_render_expr.cpp new file mode 100644 index 0000000..6682285 --- /dev/null +++ b/tests/test_render_expr.cpp @@ -0,0 +1,267 @@ +// `render()` in EXPRESSION position -- `obj = render() { cube(10); };` +// +// It measures and returns; it never draws. Most of these tests therefore +// assert on TWO things at once: the object's contents, and that the drawn +// body count is unchanged. + +#include "test_helpers.hpp" + +#include + +#include +#include + +using namespace oscadeval; +using namespace oscadeval::test; + +namespace { + +// Captures echo output; every measurement assertion reads it back, since +// the object only exists inside the script. +struct Measured { + Evaluated e; + std::vector echoes; +}; + +Measured runScript(const std::string& code) { + std::vector echoes; + // The lambda outlives this call inside the Evaluator, so it captures a + // pointer to storage the caller keeps. + auto captured = std::make_shared>(); + Evaluated e = evalSrc(code, [captured](const std::string& m) { captured->push_back(m); }); + return Measured{std::move(e), *captured}; +} + +size_t drawnBodies(const Evaluated& e) { + size_t n = 0; + for (const ColoredBody& b : e.bodies) { + if ((b.body && !b.body->IsEmpty()) || b.section || b.isDisplayOnly()) ++n; + } + return n; +} + +} // namespace + +// -- Measurements against known solids ------------------------------------ + +TEST(RenderExpr, MeasuresACube) { + Measured r = runScript("o = render() { cube(10); };\n" + "echo(o.volume, o.area, o.genus, o.dim);\n" + "echo(o.boundingbox);\n" + "echo(len(o.vertices), len(o.faces));"); + ASSERT_EQ(r.echoes.size(), 3u); + EXPECT_EQ(r.echoes[0], "ECHO: 1000, 600, 0, 3"); + EXPECT_EQ(r.echoes[1], "ECHO: [[0, 0, 0], [10, 10, 10]]"); + // 8 and 12, not 24 and 12: the exact-position weld ran. Without it the + // round-trip below would build an OPEN mesh. + EXPECT_EQ(r.echoes[2], "ECHO: 8, 12"); +} + +TEST(RenderExpr, MeasuresABooleanResult) { + Measured r = runScript("o = render() { difference() { cube(10); cube(5); } };\necho(o.volume, o.genus);"); + ASSERT_EQ(r.echoes.size(), 1u); + EXPECT_EQ(r.echoes[0], "ECHO: 875, 0"); +} + +TEST(RenderExpr, ReportsGenusOfATorus) { + // genus is the one measurement no other code path in this repo produces, + // so it gets its own case. A torus is genus 1 however coarsely faceted. + Measured r = runScript("o = render() { rotate_extrude($fn=24) translate([5,0]) circle(1,$fn=12); };\necho(o.genus, o.dim);"); + ASSERT_EQ(r.echoes.size(), 1u); + EXPECT_EQ(r.echoes[0], "ECHO: 1, 3"); +} + +TEST(RenderExpr, HonoursDollarVariableArguments) { + Measured coarse = runScript("o = render($fn=8) { sphere(10); };\necho(len(o.faces));"); + Measured fine = runScript("o = render($fn=64) { sphere(10); };\necho(len(o.faces));"); + ASSERT_EQ(coarse.echoes.size(), 1u); + ASSERT_EQ(fine.echoes.size(), 1u); + EXPECT_NE(coarse.echoes[0], fine.echoes[0]) << "$fn did not reach the children"; +} + +TEST(RenderExpr, AcceptsAndIgnoresConvexity) { + Measured r = runScript("o = render(convexity=4) { cube(2); };\necho(o.volume);"); + ASSERT_EQ(r.echoes.size(), 1u); + EXPECT_EQ(r.echoes[0], "ECHO: 8"); +} + +// -- The mesh round-trips ------------------------------------------------- + +TEST(RenderExpr, VnfRoundTripsThroughPolyhedron) { + // THE winding test. A reversed mesh still builds -- Manifold reports + // Status()==NoError -- but its volume comes back NEGATIVE. So assert a + // POSITIVE volume, never abs(): the sign is the whole signal. + Measured r = runScript("o = render() { sphere(10, $fn=16); };\npolyhedron(o.vertices, o.faces);"); + ASSERT_EQ(drawnBodies(r.e), 1u); + const ColoredBody& b = r.e.bodies.front(); + ASSERT_TRUE(b.body.has_value()); + EXPECT_EQ(b.body->Status(), manifold::Manifold::Error::NoError) << "open mesh -- the vertex weld regressed"; + EXPECT_GT(b.body->Volume(), 0.0) << "NEGATIVE volume -- face winding is reversed"; + + // Fidelity: against the SAME sphere drawn directly, not against echo + // output (which rounds to 6 significant digits). + Evaluated direct = evalSrc("sphere(10, $fn=16);"); + ASSERT_EQ(drawnBodies(direct), 1u); + EXPECT_NEAR(b.body->Volume(), direct.bodies.front().body->Volume(), 1e-9); + EXPECT_EQ(b.body->NumTri(), direct.bodies.front().body->NumTri()); +} + +TEST(RenderExpr, PolyhedronAcceptsTheObjectDirectly) { + Measured r = runScript("o = render() { difference() { cube(10); cube(5); } };\npolyhedron(o);"); + ASSERT_EQ(drawnBodies(r.e), 1u); + const ColoredBody& b = r.e.bodies.front(); + ASSERT_TRUE(b.body.has_value()); + EXPECT_EQ(b.body->Status(), manifold::Manifold::Error::NoError); + EXPECT_NEAR(b.body->Volume(), 875.0, 1e-6); +} + +TEST(RenderExpr, PolyhedronAcceptsAnyObjectWithTheKeys) { + // Nothing about this is render()-specific -- a script can build its own. + Measured r = runScript("v = [[0,0,0],[1,0,0],[1,1,0],[0,1,0],[0,0,1],[1,0,1],[1,1,1],[0,1,1]];\n" + "f = [[0,1,2,3],[7,6,5,4],[0,4,5,1],[1,5,6,2],[2,6,7,3],[3,7,4,0]];\n" + "polyhedron(object(vertices=v, faces=f));"); + ASSERT_EQ(drawnBodies(r.e), 1u); + EXPECT_NEAR(r.e.bodies.front().body->Volume(), 1.0, 1e-9); +} + +TEST(RenderExpr, PolyhedronNamesTheMissingKey) { + EXPECT_THROW(evalSrc("polyhedron(object(vertices=[[0,0,0]]));"), std::exception); + EXPECT_THROW(evalSrc("polyhedron(object(faces=[[0,1,2]]));"), std::exception); +} + +TEST(RenderExpr, ExposesVnfAsTheTwoList) { + // BOSL2 functions take the 2-list, not two arguments. + Measured r = runScript("o = render() { cube(3); };\necho(len(o.vnf), o.vnf[0] == o.vertices, o.vnf[1] == o.faces);"); + ASSERT_EQ(r.echoes.size(), 1u); + EXPECT_EQ(r.echoes[0], "ECHO: 2, true, true"); +} + +// -- 2D ------------------------------------------------------------------- + +TEST(RenderExpr, MeasuresA2dShape) { + Measured r = runScript("o = render() { square([4,3]); };\n" + "echo(o.dim, o.area, o.perimeter);\n" + "echo(o.boundingbox);\n" + "echo(len(o.vertices), len(o.paths));"); + ASSERT_EQ(r.echoes.size(), 3u); + EXPECT_EQ(r.echoes[0], "ECHO: 2, 12, 14"); + EXPECT_EQ(r.echoes[1], "ECHO: [[0, 0], [4, 3]]"); + EXPECT_EQ(r.echoes[2], "ECHO: 4, 1"); +} + +TEST(RenderExpr, TwoDShapeWithAHoleHasTwoPaths) { + // The inner square must be strictly INSIDE: at the origin it would cut a + // corner notch, which is still a single contour. + Measured r = runScript("o = render() { difference() { square(10); translate([4,4]) square(2); } };\n" + "echo(o.area, len(o.paths));"); + ASSERT_EQ(r.echoes.size(), 1u); + EXPECT_EQ(r.echoes[0], "ECHO: 96, 2"); +} + +TEST(RenderExpr, PolygonAcceptsTheObjectDirectly) { + Measured r = runScript("o = render() { difference() { square(10); translate([4,4]) square(2); } };\npolygon(o);"); + ASSERT_EQ(drawnBodies(r.e), 1u); + ASSERT_TRUE(r.e.bodies.front().section.has_value()); + EXPECT_NEAR(r.e.bodies.front().section->Area(), 96.0, 1e-9); +} + +// -- Degenerate input ----------------------------------------------------- + +TEST(RenderExpr, EmptyGeometryYieldsDimZeroAndUndefBounds) { + // boundingbox must be undef, NOT Manifold's empty Box -- that is + // {+inf, -inf} and would poison any arithmetic downstream. + for (const char* src : {"o = render() { };", "o = render() { if (false) cube(1); };"}) { + Measured r = runScript(std::string(src) + "\necho(o.dim, o.volume, o.boundingbox, len(o.vertices));"); + ASSERT_EQ(r.echoes.size(), 1u) << src; + EXPECT_EQ(r.echoes[0], "ECHO: 0, 0, undef, 0") << src; + } +} + +// -- The headline property: nothing is drawn ------------------------------ + +TEST(RenderExpr, DrawsNothing) { + Measured r = runScript("o = render() { cube(100); };\ncube(1);"); + ASSERT_EQ(drawnBodies(r.e), 1u) << "the measured cube(100) leaked into the model"; + EXPECT_NEAR(r.e.bodies.front().body->Volume(), 1.0, 1e-9); +} + +TEST(RenderExpr, DrawsNothingFromAnyContext) { + // Same rule everywhere -- no special case for functions, loops, or + // branches, precisely because there are no side effects to sequence. + const char* cases[] = { + "module m() { o = render() { cube(100); }; } m();", + "function f() = render() { cube(100); }.volume; x = f();", + "v = [for (i = [0:2]) render() { cube(i + 1); }];", + "x = true ? render() { cube(100); }.volume : 0;", + "o = render() { cube(100); };", + }; + for (const char* src : cases) { + Measured r = runScript(std::string(src) + "\ncube(1);"); + EXPECT_EQ(drawnBodies(r.e), 1u) << src; + } +} + +TEST(RenderExpr, EvaluatesOncePerEvaluationInAComprehension) { + Measured r = runScript("v = [for (i = [0:2]) render() { cube(i + 1); }];\necho(len(v), v[0].volume, v[2].volume);"); + ASSERT_EQ(r.echoes.size(), 1u); + EXPECT_EQ(r.echoes[0], "ECHO: 3, 1, 27"); +} + +TEST(RenderExpr, WorksInsideAFunctionBody) { + // Function purity is preserved because nothing is drawn -- this is the + // use case that a draw-and-measure design would have had to forbid. + Measured r = runScript("function fits(w) = render() { cube(w); }.volume < 500;\necho(fits(5), fits(10));"); + ASSERT_EQ(r.echoes.size(), 1u); + EXPECT_EQ(r.echoes[0], "ECHO: true, false"); +} + +// -- Provenance is not polluted ------------------------------------------- + +TEST(RenderExpr, LeavesNoProvenanceEntriesBehind) { + // Discarded geometry must not register originalID -> AST node entries: + // those tables are cleared once per pass, so a leak here is permanent + // and shows up much later as wrong click-to-source. This fails if ANY + // of the four guards in csg_generate.cpp is missed. + Evaluated without = evalSrc("sphere(5);"); + Evaluated with = evalSrc("o = render() { cube(10); };\nsphere(5);"); + EXPECT_EQ(with.ev.idToNode.size(), without.ev.idToNode.size()); + EXPECT_EQ(with.ev.idToColor.size(), without.ev.idToColor.size()); +} + +TEST(RenderExpr, RestoresItsInternalStateAfterAThrow) { + // A throw inside the measured children must unwind the pushed + // treeStack_ frame AND clear measuring_, or every later resolve + // accumulates into a dead level and provenance stays suppressed. + Evaluated e{parseSrc("cube(1);"), nullptr, Evaluator(), {}, {}}; + e.scope = oscad::buildScopes(e.ast); + const size_t depthBefore = e.ev.treeStackDepthForTesting(); + EXPECT_FALSE(e.ev.measuringForTesting()); + + EXPECT_THROW(evalSrc("o = render() { assert(false); cube(1); };"), std::exception); + EXPECT_THROW(evalSrc("translate([5,0,0]) { o = render() { assert(false); }; }"), std::exception); + + // And the machine still works afterwards. + Measured after = runScript("o = render() { cube(3); };\necho(o.volume);"); + EXPECT_EQ(after.echoes.size(), 1u); + EXPECT_EQ(after.echoes[0], "ECHO: 27"); + EXPECT_EQ(after.e.ev.treeStackDepthForTesting(), depthBefore); + EXPECT_FALSE(after.e.ev.measuringForTesting()); +} + +// -- The statement form is untouched -------------------------------------- + +TEST(RenderExpr, StatementFormStillDraws) { + Evaluated e = evalSrc("render() cube(2);"); + ASSERT_EQ(drawnBodies(e), 1u); + EXPECT_NEAR(e.bodies.front().body->Volume(), 8.0, 1e-9); +} + +TEST(RenderExpr, MeasuredSubtreeUsesItsOwnCoordinates) { + // An enclosing transform applies to what is DRAWN, not to what a + // render() expression measures -- the expression resolves its own + // children in its own frame. + Measured r = runScript("translate([5,0,0]) { o = render() { cube(2); }; cube(o.volume); }\n" + "echo(o.boundingbox);"); + ASSERT_EQ(r.echoes.size(), 1u); + EXPECT_EQ(r.echoes[0], "ECHO: [[0, 0, 0], [2, 2, 2]]"); +} From fe238ef5f2f27a5a36c0d62b62851600e8363ac0 Mon Sep 17 00:00:00 2001 From: Revar Desmera Date: Thu, 20 Aug 2026 12:19:47 -0700 Subject: [PATCH 2/4] Compile render() expressions to bytecode instead of bailing A declaration containing a render() expression previously fell back to the interpreter wholesale, via compileExpr's NotCompilable safety net. The interpreter is many times slower than the VM, so a single measurement anywhere in a function dragged that whole function onto the slow path. No new opcode. A new BuiltinWrapSite::Kind::Measure reuses the existing Op::PushBuiltinWrap/PopBuiltinWrap pair, which is what lets it inherit the entire bracket lifecycle -- push/pop counting, ctxChain discipline, exception teardown -- for free. A new op pair would have needed a fourth parallel per-frame stack, and the teardown's pop ordering would then have had to satisfy both nesting orders at once. That ordering turned out to be a non-problem, and the comments claiming otherwise are corrected here: the three loops in teardownVmCallStackDownTo are COUNTS, not targeted pops. Popping N off the back of treeStack_ removes the top N whichever group counted them, so the grouping is bookkeeping, not sequencing. Only measuring_ needed restoring, which is one line reading front().savedMeasuring -- recorded by every kind so no kind-inspection is needed. The Pop branch's RAII guard deliberately wraps measureCsgSubtree rather than preceding it: measuring_ must stay true across the generate, since that flag is what suppresses the four provenance writes. Restoring early would leave them inert on the VM path only -- no crash, no wrong geometry, just silently wrong click-to-source. ProvenanceStaysCleanUnderTheVm is the only thing that catches it. One real bug this surfaced, and it is why the interpreter landed first: statement opcodes now run inside FUNCTION chunks for the first time, and a compiled function keeps its parameters and lets in frame SLOTS that no EvalContext can see. The children of a render expression resolve names through the context, so `function f(w) = render() { cube(w); }.volume;` found no `w` at all and silently measured an undef-sized cube -- returning volume 0 rather than erroring. Kind::Measure now captures (name, slot) for every visible local at compile time and republishes them into the children's context at Push. Caught by the test suite, not by inspection. Op::PopBuiltinWrap asserts both the operand-stack and treeStack_ depths it recorded at Push. Every compileOneStatement case is operand-stack-neutral, but nothing structurally enforces that, and this is the first bracket to run statements with a non-empty operand stack beneath it. Tests: 12 scripts asserted identical under both engines, a positive check that the chunks actually compile (the regression guard against reintroducing the bail), captured-locals, provenance under the VM, and five throw paths including one originating deep inside a nested module frame so the multi-frame teardown loop runs. 953 tests pass under both OSCAD_BYTECODE_VM=0 and =1, and clean under ASan. Co-Authored-By: Claude Opus 5 (1M context) --- include/openscad_cpp_evaluator/bytecode.hpp | 15 +++ .../openscad_cpp_evaluator/bytecode_vm.hpp | 12 ++ include/openscad_cpp_evaluator/evaluator.hpp | 6 +- src/bytecode_compiler.cpp | 55 +++++++- src/bytecode_vm.cpp | 85 ++++++++++++- tests/test_render_expr.cpp | 119 ++++++++++++++++++ 6 files changed, 286 insertions(+), 6 deletions(-) diff --git a/include/openscad_cpp_evaluator/bytecode.hpp b/include/openscad_cpp_evaluator/bytecode.hpp index 9c59df3..6b96644 100644 --- a/include/openscad_cpp_evaluator/bytecode.hpp +++ b/include/openscad_cpp_evaluator/bytecode.hpp @@ -892,6 +892,13 @@ struct CompiledChunk { Color, Modifier, Passthrough, // hull()/minkowski()/render() -- empty params, no compute function needed + // render() in EXPRESSION position. The only Kind whose Pop + // does NOT append a CSGNode to the parent's accumulator: it + // discards the subtree and pushes the measurement object() onto + // the operand stack instead. Its `node` is a RenderExpression, + // not a ModularCall -- warnUnexpectedBuiltinArgs already + // self-guards on that (registry.cpp). + Measure, LinearExtrude, RotateExtrude, Projection, @@ -901,6 +908,14 @@ struct CompiledChunk { Kind kind; std::string tagName; // "translate"/"rotate"/.../"color"/"highlight"/"background"/"show_only"/"hull"/... const oscad::ASTNode* node = nullptr; + // Kind::Measure only. (name, slot) for every local visible at this + // site, so Op::PushBuiltinWrap can publish them into the children's + // EvalContext. A render() expression's children are STATEMENT + // opcodes and resolve names through the context, but a compiled + // FUNCTION keeps its parameters and lets in frame slots instead -- + // without this, `function f(w) = render() { cube(w); }.volume;` + // cannot see `w` and silently builds nothing. + std::vector> capturedLocals; }; // One Op::PushCsgWrap/PopCsgWrap site pair -- see those ops' own doc diff --git a/include/openscad_cpp_evaluator/bytecode_vm.hpp b/include/openscad_cpp_evaluator/bytecode_vm.hpp index 256e632..07e6632 100644 --- a/include/openscad_cpp_evaluator/bytecode_vm.hpp +++ b/include/openscad_cpp_evaluator/bytecode_vm.hpp @@ -38,6 +38,18 @@ struct PendingBuiltinWrap { // other kind computes its params at Push time and leaves this default- // empty. CallArgs deferredArgs; + + // ev.measuring_ as it stood immediately BEFORE this bracket opened. + // Recorded for EVERY kind, not just Measure, so the exception teardown + // can restore from front() without inspecting kinds -- a non-Measure + // entry simply records and restores the same value. + bool savedMeasuring = false; + // f.stack.size() and ev.treeStack_.size() at Push time. Kind::Measure's + // Pop asserts both: it is the first bracket that runs STATEMENT opcodes + // with a non-empty operand stack beneath it, and nothing structurally + // enforces that those statements are operand-stack-neutral. + size_t stackDepthAtPush = 0; + size_t treeStackDepthAtPush = 0; }; // One still-open Op::PushCsgWrap bracket's own state -- see that op's own diff --git a/include/openscad_cpp_evaluator/evaluator.hpp b/include/openscad_cpp_evaluator/evaluator.hpp index e67f8fc..513a568 100644 --- a/include/openscad_cpp_evaluator/evaluator.hpp +++ b/include/openscad_cpp_evaluator/evaluator.hpp @@ -1130,9 +1130,13 @@ class Evaluator { // Save/restore rather than a plain set/clear, so nesting is correct for // free. The interpreter uses an RAII guard; the VM stores the saved // value in its bracket (PendingBuiltinWrap::savedMeasuring). +public: + // Public for the same reason treeStack_ is: bytecode_vm.cpp is a separate + // translation unit and its Kind::Measure bracket owns this flag's + // lifetime across two separate op handlers (Push sets it, Pop restores + // it), which no RAII guard can span. bool measuring_ = false; -public: // Test-only accessors for the invariants above -- a leaked measuring_ or // an unbalanced treeStack_ is otherwise silent until something much // later goes mysteriously wrong. diff --git a/src/bytecode_compiler.cpp b/src/bytecode_compiler.cpp index 278998d..7a444dd 100644 --- a/src/bytecode_compiler.cpp +++ b/src/bytecode_compiler.cpp @@ -169,6 +169,22 @@ class CompileScope { return std::nullopt; } + // Every local visible right now, as (name, slot), OUTERMOST frame first + // so that applying them in order lets an inner binding shadow an outer + // one. Used only by Kind::Measure (see compileExpr's RenderExpression + // case): a render() expression's children are STATEMENT opcodes, which + // resolve names through the EvalContext, but a compiled function keeps + // its parameters and lets in frame SLOTS that no EvalContext can see. + // Capturing the mapping here is what lets `function f(w) = render() { + // cube(w); }.volume;` find `w` at all. + std::vector> flatten() const { + std::vector> out; + for (const auto& frame : frames_) { + for (const auto& [name, slot] : frame) out.emplace_back(name, slot); + } + return out; + } + private: std::vector> frames_; }; @@ -858,6 +874,32 @@ class Compiler { OSCAD_COMPILE_BINARY(BitwiseShiftRightOp) #undef OSCAD_COMPILE_BINARY + case NodeKind::RenderExpression: { + // The ONLY expression that compiles to STATEMENT opcodes. + // Emitted as an ordinary PushBuiltinWrap/PopBuiltinWrap + // bracket (Kind::Measure) rather than a new opcode pair, + // which is what lets it inherit the whole bracket lifecycle + // -- push/pop counting, ctxChain discipline, exception + // teardown -- for free. + // + // emitBuiltinWrap, NOT tryCompileChildrenList: the latter + // builds a SEPARATE chunk run in a separate frame, so its + // Pop could not push onto THIS frame's operand stack. These + // children are statically known, so they compile inline. + // + // The children are operand-stack-neutral (every + // compileOneStatement case is), so the Value the Pop pushes + // lands exactly where this expression's own operand belongs. + // Op::PopBuiltinWrap asserts that rather than trusting it. + auto& n = static_cast(node); + std::vector kids; + kids.reserve(n.children.size()); + for (const auto& c : n.children) kids.push_back(c.get()); + emitBuiltinWrap(CompiledChunk::BuiltinWrapSite::Kind::Measure, "render", n, kids, out, + /*emitCheckDebug=*/false, scope.flatten()); + return; + } + default: // Safety net for any Expression NodeKind without its own // case above -- falls back to the interpreter for the @@ -1194,14 +1236,23 @@ class Compiler { // for real: DebugHooks.FastContinueNotHookSkippableStillFiresEvery- // Checkpoint (a translate()-wrapped script) caught the miscount when // this was first omitted by analogy to Op::CallModule. + // emitCheckDebug=false only for Kind::Measure: a render() EXPRESSION is + // part of a statement whose own checkpoint has already fired, and a + // CheckDebugStatement emitted here would run BEFORE Push sets + // measuring_ -- breaking parity with the interpreter's evalRenderExpr, + // which fires no checkpoint of its own either. void emitBuiltinWrap(CompiledChunk::BuiltinWrapSite::Kind kind, const std::string& tagName, const oscad::ASTNode& wrapperNode, const std::vector& children, - std::vector& out) { - out.push_back({Op::CheckDebugStatement, internNativeStatement(&wrapperNode), 0, nullptr}); + std::vector& out, bool emitCheckDebug = true, + std::vector> capturedLocals = {}) { + if (emitCheckDebug) { + out.push_back({Op::CheckDebugStatement, internNativeStatement(&wrapperNode), 0, nullptr}); + } CompiledChunk::BuiltinWrapSite site; site.kind = kind; site.tagName = tagName; site.node = &wrapperNode; + site.capturedLocals = std::move(capturedLocals); chunk_.builtinWrapSites.push_back(std::move(site)); const int idx = static_cast(chunk_.builtinWrapSites.size()) - 1; out.push_back({Op::PushBuiltinWrap, idx, 0, &wrapperNode.position()}); diff --git a/src/bytecode_vm.cpp b/src/bytecode_vm.cpp index dc1acec..55c282e 100644 --- a/src/bytecode_vm.cpp +++ b/src/bytecode_vm.cpp @@ -1,3 +1,4 @@ +#include #include "openscad_cpp_evaluator/bytecode_vm.hpp" #include "openscad_cpp_evaluator/call_args.hpp" @@ -405,13 +406,24 @@ void teardownVmCallStackDownTo(Evaluator& ev, size_t floor) { // accumulator (Op::PushBuiltinWrap's own runtime handler, above) // that would normally be popped by its matching Op::PopBuiltinWrap; // on the exception path that never runs. These are the MOST - // RECENTLY pushed treeStack_ entries relative to this frame's own - // (below) -- pop them first to respect treeStack_'s own LIFO order. + // (below). Note these three loops are COUNTS, not targeted pops: + // popping N off the back of treeStack_ removes the top N whichever + // group counted them, so the order between the groups is + // unobservable and adding a fourth kind cannot break it. + // ownsModuleSplice reads last only because it is the deepest. // See Op::PopBuiltinWrap's own doc comment (bytecode.hpp) for why // this is a real counter, not the single-bool shape // ownsModuleSplice, below, gets away with (N of these can be open // at once; at most one module-call splice ever can). for (size_t i = 0; i < frame->builtinWrapStack.size(); ++i) ev.treeStack_.pop_back(); + // A Kind::Measure bracket also set ev.measuring_ on the way in, and + // its matching Op::PopBuiltinWrap -- which would have restored it -- + // never ran. front(), not back(): savedMeasuring is recorded by + // EVERY kind, so this frame's OUTERMOST still-open bracket holds the + // value that was live before any of them opened. Nothing else in a + // frame can change the flag (a nested interpreter evalRenderExpr is + // scoped; a nested VM frame restores in its own turn of this loop). + if (!frame->builtinWrapStack.empty()) ev.measuring_ = frame->builtinWrapStack.front().savedMeasuring; frame->builtinWrapStack.clear(); // Same reasoning, same LIFO-order requirement, for any still-open // Op::PushCsgWrap bracket(s) -- see PendingCsgWrap's/Op::PushCsgWrap's @@ -1102,6 +1114,34 @@ Value driveVm(Evaluator& ev, size_t floor) { f.ctxChain.push_back(std::move(effCtx)); break; } + case CompiledChunk::BuiltinWrapSite::Kind::Measure: { + // Arguments resolve purely for the + // $-propagation-into-children side effect, as + // Passthrough does -- differing only in the + // concrete node type they hang off. + auto [args, effCtx] = resolveCallArgs( + ev, static_cast(*site.node).arguments, ctx); + (void)args; + // Publish this frame's slot locals into the + // children's context. The children are STATEMENT + // opcodes and resolve names through the + // EvalContext, but a compiled function keeps its + // parameters and lets in slots, which no context + // can see -- so without this, + // `function f(w) = render() { cube(w); }.volume;` + // resolves `w` to undef and silently measures + // nothing. Applied in outermost-first order so an + // inner binding shadows an outer one, and only + // into effCtx's own fresh trail level, so nothing + // leaks back to the caller. + for (const auto& [name, slot] : site.capturedLocals) { + if (static_cast(slot) < f.slots.size()) { + effCtx.let_->set(name, f.slots[static_cast(slot)]); + } + } + f.ctxChain.push_back(std::move(effCtx)); + break; + } case CompiledChunk::BuiltinWrapSite::Kind::LinearExtrude: { BuiltinWrapParams result = computeLinearExtrudeParams( ev, static_cast(*site.node), ctx); @@ -1146,7 +1186,11 @@ Value driveVm(Evaluator& ev, size_t floor) { } } ev.treeStack_.emplace_back(); - f.builtinWrapStack.push_back({std::move(params), randsBefore, ins.a, std::move(deferredArgs)}); + f.builtinWrapStack.push_back({std::move(params), randsBefore, ins.a, std::move(deferredArgs), + ev.measuring_, f.stack.size(), ev.treeStack_.size() - 1}); + // AFTER the push, so a throw from the push itself leaves + // the flag untouched rather than stuck on. + if (site.kind == CompiledChunk::BuiltinWrapSite::Kind::Measure) ev.measuring_ = true; ++f.pc; break; } @@ -1161,6 +1205,41 @@ Value driveVm(Evaluator& ev, size_t floor) { f.builtinWrapStack.pop_back(); const CompiledChunk::BuiltinWrapSite& site = f.chunk->builtinWrapSites[static_cast(pending.siteIdx)]; + if (site.kind == CompiledChunk::BuiltinWrapSite::Kind::Measure) { + // Bookkeeping is already popped above, matching this + // op's own "pop first, THEN do anything that can + // throw" rule: measureCsgSubtree generates real + // Manifold geometry and can throw, and if it does, + // teardownVmCallStackDownTo must not see a bracket + // that is no longer open. + // + // measuring_ must stay TRUE across measureCsgSubtree + // -- that flag is what suppresses the four + // provenance writes in csg_generate.cpp -- so it is + // restored by a guard scoped AROUND the call, not + // before it. Restoring early would leave those + // guards inert on the VM path only: no crash, no + // wrong geometry, just quietly wrong click-to-source. + struct RestoreMeasuring { + Evaluator& ev; + bool prev; + ~RestoreMeasuring() { ev.measuring_ = prev; } + } restoreMeasuring{ev, pending.savedMeasuring}; + + f.ctxChain.pop_back(); // the effCtx pushed at Push + std::vector> sub = std::move(ev.treeStack_.back()); + ev.treeStack_.pop_back(); + assert(ev.treeStack_.size() == pending.treeStackDepthAtPush); + // The children are statement opcodes running mid- + // expression; they must be operand-stack-neutral or + // this expression's own result lands in the wrong + // slot. Every compileOneStatement case is, but + // nothing enforces it -- hence the assert. + assert(f.stack.size() == pending.stackDepthAtPush); + f.stack.push_back(ev.measureCsgSubtree(std::move(sub), *site.node)); + ++f.pc; + break; + } // Roof's params computation is deferred to here (see // this Kind's own doc comment, bytecode.hpp) -- ctx is // still on top of f.ctxChain, not yet popped below, so diff --git a/tests/test_render_expr.cpp b/tests/test_render_expr.cpp index 6682285..4b96714 100644 --- a/tests/test_render_expr.cpp +++ b/tests/test_render_expr.cpp @@ -265,3 +265,122 @@ TEST(RenderExpr, MeasuredSubtreeUsesItsOwnCoordinates) { ASSERT_EQ(r.echoes.size(), 1u); EXPECT_EQ(r.echoes[0], "ECHO: [[0, 0, 0], [2, 2, 2]]"); } + +// -- Both engines, same answers ------------------------------------------- +// +// The VM compiles a render expression to a real Kind::Measure bracket +// (Op::PushBuiltinWrap/PopBuiltinWrap) rather than declining to compile and +// letting the interpreter take over -- the whole containing declaration +// would otherwise run interpreted, which is many times slower. + +namespace { + +class ScopedVm { +public: + explicit ScopedVm(bool enabled) { Evaluator::setBytecodeVmEnabledForTesting(enabled); } + ~ScopedVm() { Evaluator::setBytecodeVmEnabledForTesting(std::nullopt); } +}; + +std::vector echoesUnder(bool vm, const std::string& code) { + ScopedVm guard(vm); + return runScript(code).echoes; +} + +} // namespace + +TEST(RenderExprEngines, InterpreterAndVmAgree) { + const char* cases[] = { + "o = render() { cube(10); };\necho(o.volume, o.area, o.genus, o.dim, o.boundingbox);", + "o = render() { difference() { cube(10); cube(5); } };\necho(o.volume, len(o.vertices), len(o.faces));", + "o = render($fn=16) { sphere(10); };\necho(o.volume, len(o.faces));", + "o = render() { square([4,3]); };\necho(o.dim, o.area, o.perimeter, o.boundingbox);", + "o = render() { };\necho(o.dim, o.boundingbox);", + "function f(w) = render() { cube(w); }.volume;\necho(f(2), f(3));", + "module m() { o = render() { cube(4); }; echo(o.volume); } m();", + "v = [for (i = [1:3]) render() { cube(i); }.volume];\necho(v);", + "x = true ? render() { cube(2); }.volume : 0;\necho(x);", + "g = function(a) a + render() { cube(a); }.volume;\necho(g(2));", + "o = render() { translate([5,0,0]) cube(2); };\necho(o.boundingbox);", + "o = render() { render() { cube(3); } };\necho(o.volume);", + }; + for (const char* src : cases) { + EXPECT_EQ(echoesUnder(false, src), echoesUnder(true, src)) << "engines diverge for:\n" << src; + } +} + +TEST(RenderExprEngines, DrawsNothingUnderEitherEngine) { + for (bool vm : {false, true}) { + ScopedVm guard(vm); + Evaluated e = evalSrc("function f(w) = render() { cube(w); }.volume;\nx = f(9);\ncube(1);"); + ASSERT_EQ(drawnBodies(e), 1u) << "vm=" << vm; + EXPECT_NEAR(e.bodies.front().body->Volume(), 1.0, 1e-9) << "vm=" << vm; + } +} + +TEST(RenderExprEngines, TheDeclarationActuallyCompiles) { + // The regression guard for "someone reintroduced a NotCompilable bail". + // If compileExpr ever declines a RenderExpression again, these chunks + // come back null and the containing declaration runs interpreted. + ScopedVm guard(true); + Evaluated e = evalSrc("function f(w) = render() { cube(w); }.volume;\n" + "module m() { o = render() { cube(2); }; }\n" + "x = f(3);\nm();"); + const oscad::FunctionDeclaration* fn = nullptr; + const oscad::ModuleDeclaration* mod = nullptr; + for (const auto& n : e.ast) { + if (auto* d = dynamic_cast(n.get())) fn = d; + if (auto* d = dynamic_cast(n.get())) mod = d; + } + ASSERT_NE(fn, nullptr); + ASSERT_NE(mod, nullptr); + EXPECT_NE(e.ev.lookupOrCompileChunk(*fn), nullptr) << "function containing render() fell back to the interpreter"; + EXPECT_NE(e.ev.lookupOrCompileModuleChunk(*mod), nullptr) << "module containing render() fell back to the interpreter"; +} + +TEST(RenderExprEngines, CapturedLocalsReachTheChildren) { + // A compiled function keeps parameters and lets in frame SLOTS, which + // the children's EvalContext cannot see -- Kind::Measure republishes + // them. Without that, every one of these measures an undef-sized shape. + ScopedVm guard(true); + Measured r = runScript("function f(w) = render() { cube(w); }.volume;\n" + "function g(a, b) = let (s = a * b) render() { cube(s); }.volume;\n" + "echo(f(2), f(3), g(2, 2));"); + ASSERT_EQ(r.echoes.size(), 1u); + EXPECT_EQ(r.echoes[0], "ECHO: 8, 27, 64"); +} + +TEST(RenderExprEngines, UnwindsCleanlyUnderTheVm) { + // Both nesting orders, plus a throw originating DEEP inside a nested + // module frame so teardownVmCallStackDownTo's multi-frame loop runs -- + // that is the path where a mismatched pop would corrupt treeStack_. + ScopedVm guard(true); + const char* throwing[] = { + "o = render() { assert(false); cube(1); };", + "o = render() { translate([1,0,0]) { assert(false); } };", + "translate([5,0,0]) { o = render() { assert(false); }; }", + "module deep(n) { if (n > 0) deep(n - 1); else assert(false); }\no = render() { deep(20); };", + "function f(w) = render() { assert(false); cube(w); }.volume;\nx = f(2);", + }; + for (const char* src : throwing) { + EXPECT_THROW(evalSrc(src), std::exception) << src; + } + // The machine still works, and both invariants are back where they started. + Measured after = runScript("o = render() { cube(3); };\necho(o.volume);"); + ASSERT_EQ(after.echoes.size(), 1u); + EXPECT_EQ(after.echoes[0], "ECHO: 27"); + EXPECT_EQ(after.e.ev.treeStackDepthForTesting(), 0u); + EXPECT_FALSE(after.e.ev.measuringForTesting()); +} + +TEST(RenderExprEngines, ProvenanceStaysCleanUnderTheVm) { + // The RestoreMeasuring guard in Op::PopBuiltinWrap must wrap the + // generate, not precede it. Restoring measuring_ early leaves the + // provenance guards inert on the VM path ONLY -- no crash, no wrong + // geometry, just silently wrong click-to-source. This is the only test + // that catches that. + ScopedVm guard(true); + Evaluated without = evalSrc("sphere(5);"); + Evaluated with = evalSrc("function f(w) = render() { cube(w); }.volume;\nx = f(9);\nsphere(5);"); + EXPECT_EQ(with.ev.idToNode.size(), without.ev.idToNode.size()); + EXPECT_EQ(with.ev.idToColor.size(), without.ev.idToColor.size()); +} From b317cb0ef698d3df3194367687bfc184cccadc94 Mon Sep 17 00:00:00 2001 From: Revar Desmera Date: Thu, 20 Aug 2026 12:22:42 -0700 Subject: [PATCH 3/4] Bump to 0.38.0; document render() in expression position Minor, not patch: a new language construct plus a newly reserved word. The CLAUDE.md section leads with the two things that will actually bite someone -- `render` can no longer be an identifier, and `obj = render() cube(1);` does not parse (the child_statement swallows the semicolon) -- then the non-obvious invariants: winding is reversed on the way out, vertices are welded by exact position, a reversed mesh reports NEGATIVE volume so tests must never use abs(), and genus is Manifold's for the whole result so a sealed internal cavity reports -1. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++ pyproject.toml | 2 +- 2 files changed, 82 insertions(+), 1 deletion(-) diff --git a/CLAUDE.md b/CLAUDE.md index 4ae9842..b334a1c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1094,3 +1094,84 @@ than the current pause point). Ported identically (same qualifier syntax, same e profiling/debugging's own bookkeeping resets happen at the right point too. - `tools/cli/main.cpp` — a 3-line wrapper: builds `args` from `argv`, calls `runCli(args)` with the real `std::cin`/`std::cout`/`std::cerr`, returns its exit code. + +## `render()` in expression position + +`render()` has two jobs, decided by where it is written: + +```openscad +render() cube(1); // STATEMENT -- draws, as always +obj = render() { cube(1); }; // EXPRESSION -- measures, draws NOTHING +``` + +The expression form builds its children's geometry, measures it, extracts the +mesh, and **discards the geometry**. It is the only way OpenSCAD can inspect its +own geometry — volume, surface area, genus, bounding box, and the mesh itself. + +```openscad +obj = render() difference() { cube(100); sphere(20); }; +echo(obj.volume, obj.genus, obj.boundingbox); +polyhedron(obj); // straight back in +``` + +Keys — ordered, and the order is part of the API (`ValueObject` is an ordered +vector and `oscEqual` is order-sensitive): + +| dim | keys | +|---|---| +| 3 | `vertices`, `faces`, `volume`, `area`, `genus`, `boundingbox`, `dim`, `vnf` | +| 2 | `vertices`, `paths`, `area`, `perimeter`, `boundingbox`, `dim` | +| 0 | the 3D key set, all zero, `boundingbox = undef` | + +`vertices`/`faces` are VNF-shaped — `[x,y,z]` points, 0-based N-gon index lists, +**clockwise seen from outside** — so they feed `polyhedron()` and BOSL2 unchanged. +`vnf` is the `[vertices, faces]` 2-list BOSL2 functions actually take. +`polyhedron()` and `polygon()` also accept the object directly, and will accept +*any* object carrying the right keys, not just one `render()` produced. + +### Things that will bite + +- **`render` is a reserved keyword.** It cannot be a variable, module, function, + argument, or member name. Required: LALR(1) cannot otherwise tell `render(` in + expression position from a function call, and bison's shift-over-reduce would + silently turn every function call in an expression into a module instantiation. +- **`obj = render() cube(1);` does not parse.** A bare call's `child_statement` + swallows the `;`, leaving the assignment unterminated. Use + `render() { cube(1); }` or a form ending in `}`. Inherent to OpenSCAD's grammar. +- **`genus` is Manifold's, for the whole result.** A cube with a sealed internal + cavity reports `-1`, not `0` — its boundary has two components. Correct, but + surprising. +- **Nothing is drawn from any context** — top level, module body, function body, + list comprehension, ternary. That is what preserves function purity, and why no + context needs a special case. + +### Implementation notes + +`Evaluator::measureCsgSubtree` (`src/measure_geometry.cpp`) is the *only* +implementation of "CSG subtree → `object()`"; both engines call it — the +interpreter from `evalRenderExpr`, the VM from `Op::PopBuiltinWrap`'s +`Kind::Measure` branch. + +`Evaluator::measuring_` is set for the whole generate. It suppresses the four +writes that exist solely to describe *drawn* geometry — `idToNode`/`idToColor` in +`tagGenerated` and `tagDisplayOnly`, the `restampCachedIds` call on a cache hit, +and `cacheProducer_` — because those tables are cleared once per pass, so a leak +is permanent and surfaces later as wrong click-to-source. It also suppresses +`checkDebug`, which would otherwise inject stops at the paused statement's own +`callStack_` depth. The **geometry cache stays on**: `cacheKey` is +content-addressed, so an entry a measurement stores is genuinely reusable by the +real render. + +Two details in the mesh conversion are load-bearing and silent when wrong: the +winding is **reversed** on the way out (Manifold's `triVerts` is CCW), and +vertices are welded by exact position (Manifold splits property-vertices, so +without the weld the round-tripped `polyhedron()` is an *open* mesh). A reversed +mesh still builds — `Status() == NoError` — but comes back with **negative +volume**, so tests assert a positive volume and must never use `abs()`. + +On the VM side, `Kind::Measure` captures `(name, slot)` for every visible local +at compile time and republishes them into the children's `EvalContext` at Push. +A compiled function keeps its parameters in frame slots, which no `EvalContext` +can see, and the children are statement opcodes that resolve names through the +context — without this, `function f(w) = render() { cube(w); }.volume;` finds no +`w` and silently measures nothing. diff --git a/pyproject.toml b/pyproject.toml index 6e579fc..6d152cf 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "scikit_build_core.build" [project] name = "openscad_cpp_evaluator" -version = "0.37.0" +version = "0.38.0" description = "C++ OpenSCAD evaluator with Python bindings" readme = "README.md" requires-python = ">=3.12" From ebfd8197731db4fc5f62cb9730936c398b1eed5d Mon Sep 17 00:00:00 2001 From: Revar Desmera Date: Thu, 20 Aug 2026 12:27:33 -0700 Subject: [PATCH 4/4] Point the parser submodule at merged main BelfrySCAD/openscad_cpp_parser#5 (render() in expression position) is in. Co-Authored-By: Claude Opus 5 (1M context) --- external/openscad_cpp_parser | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/external/openscad_cpp_parser b/external/openscad_cpp_parser index 49e83e7..8a651c3 160000 --- a/external/openscad_cpp_parser +++ b/external/openscad_cpp_parser @@ -1 +1 @@ -Subproject commit 49e83e74e6713973a3c3b64007854da3cd7df87f +Subproject commit 8a651c31c5433d96fe98fa0e160754e9687fac2f