diff --git a/.gitignore b/.gitignore index f24e881..2f5643f 100644 --- a/.gitignore +++ b/.gitignore @@ -4,3 +4,5 @@ dist/ wheelhouse/ *.egg-info/ __pycache__/ +build-tbb/ +.DS_Store diff --git a/CLAUDE.md b/CLAUDE.md index 2cc5dc2..a2c8d52 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1609,8 +1609,46 @@ implementation of "CSG subtree → `object()`"; both engines call it — the interpreter from `evalRenderExpr`, the VM from `Op::PopBuiltinWrap`'s `Kind::Measure` branch. +**`idToCallChain` is what a picker should actually walk.** `idToNode` names the +node that *produced* the geometry, and for anything a library builds that is a +node inside the library — a plain `cube(10)` maps into BOSL2's `builtins.scad` +the moment BOSL2 is included, because BOSL2 overrides the primitives with its +own modules. Pointing an editor at that span means pointing it at another file, +or (if the consumer forgets to check `origin`) splicing a library's byte offsets +into the user's buffer, which is how BelfrySCAD #450 appended a `translate()` to +the end of a 59-character script. + +`idToCallChain` maps each `originalID` to its **whole call chain**, innermost +frame first. Deliberately unfiltered: a single `cuboid()` is 24 frames, most of +them BOSL2's own `attachable`/`_attach_transform` layers, and someone debugging +the library wants to step into exactly those. Which frames are reachable depends +on what the front end has open, which the evaluator has no business guessing — +so it records all of them and a consumer picks its level (BelfrySCAD defaults to +the last frame in the running script). + +It is stored as a **cactus stack**, not a list per node. Chains nest, so the +distinct chains over a run form a tree: `callChains_` holds `{site, parent, +isModule}` and a `CSGNode` holds one `uint32` into it. Memory tracks distinct +call *paths* — tens on a real model; Dalek's 139 bodies share 13 innermost sites +— rather than CSG nodes, and nothing allocates per node. That is what makes this +affordable where "the full frame list a TRACE would need" (`csg_node.hpp`) was +not. + +`isModule` separates a module frame, which has geometry behind it, from a +function frame like `_find_anchor`, which does not: both are worth showing, only +the former is worth dragging. + +A cache hit restamps to the chain reaching the geometry *now*, not the one that +first produced it: a module called twice genuinely is two call chains. + +No stored script path is needed to tell the user's file from a library: +`callStack_.front()` is by construction the call made from top level. + +Exposed to Python as `node.call_sites` (innermost-first `_CallFrame`s, each a +`_Position` plus `is_module`), with `node.call_site` as the innermost. + `Evaluator::measuring_` is set for the whole generate. It suppresses the four -writes that exist solely to describe *drawn* geometry — `idToNode`/`idToColor` in +writes that exist solely to describe *drawn* geometry — `idToNode`/`idToColor`/`idToCallChain` 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 diff --git a/bindings/module.cpp b/bindings/module.cpp index 2674028..bba7a31 100644 --- a/bindings/module.cpp +++ b/bindings/module.cpp @@ -196,13 +196,37 @@ struct IdSpan { uint32_t id; int start, end, line, column; std::string origin; + // The whole call chain behind this body, innermost frame first. Empty + // for geometry written at top level, where the span above is already + // the user's own source. Library frames included on purpose -- a BOSL2 + // author stepping into `attachable` wants them; a front end picks the + // level it can actually show. See Evaluator::idToCallChain. + struct Frame { + int start, end, line, column; + std::string origin; + bool is_module; + }; + std::vector chain; }; void collectIdSpans(const oscadeval::Evaluator& ev, std::vector& out) { out.reserve(ev.idToNode.size()); for (const auto& [id, node] : ev.idToNode) { const oscad::Position& p = node->position(); - out.push_back({id, p.start_offset, p.end_offset, p.line, p.column, p.origin}); + IdSpan s{id, p.start_offset, p.end_offset, p.line, p.column, p.origin}; + auto found = ev.idToCallChain.find(id); + uint32_t idx = found == ev.idToCallChain.end() + ? oscadeval::Evaluator::kNoCallChain : found->second; + while (idx != oscadeval::Evaluator::kNoCallChain && idx < ev.callChains_.size()) { + const auto& e = ev.callChains_[idx]; + if (e.site) { + s.chain.push_back({e.site->start_offset, e.site->end_offset, + e.site->line, e.site->column, e.site->origin, + e.isModule}); + } + idx = e.parent; + } + out.push_back(std::move(s)); } } @@ -219,8 +243,12 @@ nb::list bodiesToList(std::vector& bodies) { nb::dict idSpansToDict(const std::vector& idSpans) { nb::dict d; - for (const IdSpan& s : idSpans) - d[nb::cast(s.id)] = nb::make_tuple(s.start, s.end, s.line, s.column, s.origin); + for (const IdSpan& s : idSpans) { + nb::list chain; + for (const IdSpan::Frame& f : s.chain) + chain.append(nb::make_tuple(f.start, f.end, f.line, f.column, f.origin, f.is_module)); + d[nb::cast(s.id)] = nb::make_tuple(s.start, s.end, s.line, s.column, s.origin, chain); + } return d; } diff --git a/include/openscad_cpp_evaluator/csg_node.hpp b/include/openscad_cpp_evaluator/csg_node.hpp index 5044aca..c4132a7 100644 --- a/include/openscad_cpp_evaluator/csg_node.hpp +++ b/include/openscad_cpp_evaluator/csg_node.hpp @@ -51,6 +51,13 @@ struct CSGNode { // call sites still share a cache entry. const oscad::Position* warnEntry = nullptr; + // This node's interned call chain (Evaluator::callChains_), captured + // at RESOLVE time for the same reason warnEntry is: a picker runs long + // after the stack has unwound. kNoCallChain at top level. One uint32, + // not a frame list -- see Evaluator's call-chain section for why that + // is affordable where a per-node list was not. + uint32_t callChain = UINT32_MAX; + // 1 + the deepest child's own treeDepth (1 for a leaf) -- set once, // at construction, by whichever csg_resolve.cpp site finalizes this // node's own `children` (buildTreeNode, evalModularCall's non-splice diff --git a/include/openscad_cpp_evaluator/evaluator.hpp b/include/openscad_cpp_evaluator/evaluator.hpp index 4c29bd8..548fed0 100644 --- a/include/openscad_cpp_evaluator/evaluator.hpp +++ b/include/openscad_cpp_evaluator/evaluator.hpp @@ -284,9 +284,21 @@ class Evaluator { // Give cached bodies fresh originalIDs so a second call site reusing // them is not confused with the first. See its definition. void restampCachedIds(std::vector& bodies, const oscad::ASTNode& node, - const oscad::ASTNode* producer); + const oscad::ASTNode* producer, uint32_t callChain); std::unordered_map idToNode; + // originalID -> its interned call chain (innermost frame; walk + // callChains_[i].parent outwards), or kNoCallChain for geometry + // written at top level, where idToNode already is the user's node. + // + // idToNode names the node that PRODUCED the geometry, which for + // anything a library builds is a node inside that library -- including + // a plain `cube(10)` once BOSL2 is included, since BOSL2 overrides the + // primitives with its own modules. A consumer that wants to point an + // editor at a line the user can actually see walks this instead, and + // picks the level it wants: the last frame in the script for an + // ordinary pick, deeper for someone debugging the library itself. + std::unordered_map idToCallChain; // Nodes whose bodies were actually generated this run (not served from // the ManifoldCache). Reset per run; a diagnostic, and the only honest // way for a test to tell a cache hit from a miss now that an inner @@ -327,6 +339,57 @@ class Evaluator { return callStack_.empty() ? nullptr : callStack_.front().callPosition; } + // -- Call chains ----------------------------------------------------- + // + // The whole call chain behind a body, so a picker can step through it. + // Deliberately UNFILTERED: which frames are reachable depends on what + // the front end has open, which the evaluator has no business guessing. + // A BOSL2 author stepping into `attachable` -> `_attach_transform` is + // the case that settles it; a single cuboid() call is seven frames, six + // of them inside the library. + // + // Stored as a cactus stack rather than a list per node. Call chains + // nest, so the distinct chains over a run form a TREE: each entry is + // one frame plus its parent's index, and a CSGNode holds a single + // uint32 into the pool. Memory tracks distinct call PATHS (tens on a + // real model -- Dalek's 139 bodies share 13 innermost sites) rather + // than CSG nodes, and nothing allocates per node. That is what makes + // this affordable where "the full frame list a TRACE would need" + // (csg_node.hpp) was not. + static constexpr uint32_t kNoCallChain = UINT32_MAX; + + struct CallChainEntry { + const oscad::Position* site = nullptr; // non-owning, AST-lifetime-bound + uint32_t parent = kNoCallChain; // next frame OUT, or kNoCallChain + bool isModule = false; // a function frame has no geometry to drag + }; + + // Intern callStack_ as it stands and return its index, or kNoCallChain + // at top level. Outermost-first walk, so a chain sharing a prefix with + // one already interned costs only its own tail. + uint32_t internCurrentCallChain() { + uint32_t parent = kNoCallChain; + for (const CallStackFrame& f : callStack_) { + if (!f.callPosition) continue; + const ChainKey key{parent, f.callPosition}; + auto found = chainIndex_.find(key); + if (found != chainIndex_.end()) { + parent = found->second; + continue; + } + const uint32_t idx = static_cast(callChains_.size()); + callChains_.push_back({f.callPosition, parent, + f.kind == CallStackFrame::Kind::Module}); + chainIndex_.emplace(key, idx); + parent = idx; + } + return parent; // the innermost frame, or kNoCallChain + } + + // The interned pool. Public for the same reason idToNode is: a binding + // walks it after evaluate() to flatten each id's chain. + std::vector callChains_; + // Set by generateTreeImpl() to the CSGNode currently being generated, // so warn() can name the user's own call site during a phase where // callStack_ is necessarily empty. Public for the same reason @@ -335,6 +398,9 @@ class Evaluator { // than anything reentrancy-aware, since generateTreeImpl recurses // depth-first on one thread. const oscad::Position* generateWarnEntry = nullptr; + // The same republish for CSGNode::callChain, read by tagGenerated to + // fill idToCallChain. + uint32_t generateCallChain = kNoCallChain; // Set while generating anything beneath a hull(). An open mesh there is // not a mistake to report: a convex hull needs only points, so BOSL2's @@ -1790,6 +1856,22 @@ class Evaluator { // eval_error.hpp). std::vector callStack_; + // (parent chain index, frame position) -> chain index, the memo that + // makes internCurrentCallChain() share prefixes. Cleared with the pool + // wherever idToNode is. + struct ChainKey { + uint32_t parent; + const oscad::Position* site; + bool operator==(const ChainKey& o) const { return parent == o.parent && site == o.site; } + }; + struct ChainKeyHash { + std::size_t operator()(const ChainKey& k) const { + return std::hash{}(k.parent) ^ + (std::hash{}(k.site) << 1); + } + }; + std::unordered_map chainIndex_; + // Count of callStack_ entries that ACTUALLY cost native C++ stack -- // i.e. pushed with skipDepthGuard=false (enterUserCall's own // interpreted-call sites: evalUserFunctionCore, evalUserModule). diff --git a/pyproject.toml b/pyproject.toml index 76fadda..eaba213 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "scikit_build_core.build" [project] name = "openscad_cpp_evaluator" -version = "1.20.5" +version = "1.21.0" description = "C++ OpenSCAD evaluator with Python bindings" readme = "README.md" requires-python = ">=3.12" diff --git a/python/openscad_cpp_evaluator/__init__.py b/python/openscad_cpp_evaluator/__init__.py index 6087b28..79fe7cc 100644 --- a/python/openscad_cpp_evaluator/__init__.py +++ b/python/openscad_cpp_evaluator/__init__.py @@ -47,11 +47,43 @@ def __init__(self, line, column, origin, start_offset, end_offset): class _DeclNode: - __slots__ = ("name", "position") + __slots__ = ("name", "position", "call_sites") - def __init__(self, name, position): + def __init__(self, name, position, call_sites=()): self.name = name self.position = position + #: For a geometry node: the whole call chain behind it, INNERMOST + #: frame first, each a `_CallFrame`. Empty for geometry written at + #: top level, where `position` is already the user's own source. + #: + #: `position` names the node that PRODUCED the geometry, which for + #: anything a library builds is inside that library -- a plain + #: `cube(10)` lands in BOSL2's builtins.scad once BOSL2 is included. + #: A caller that wants a line to show the user walks this and picks + #: the level it can display: the last frame in the running script + #: for an ordinary pick, a deeper one for someone reading the + #: library itself. Library frames are included deliberately. + self.call_sites = tuple(call_sites) + + @property + def call_site(self): + """The innermost frame, or None. Convenience for a caller that does + not care about the chain.""" + return self.call_sites[0] if self.call_sites else None + + +class _CallFrame(_Position): + """One call in a chain: a position, plus whether it is a MODULE call. + + A function frame (`_find_anchor`, an `assert`) has no geometry of its + own to drag, so a gizmo should decline it even though it is worth + showing. + """ + __slots__ = ("is_module",) + + def __init__(self, line, column, origin, start_offset, end_offset, is_module): + super().__init__(line, column, origin, start_offset, end_offset) + self.is_module = is_module class OscObject: @@ -635,8 +667,13 @@ def evaluate(self, source_path: str, viewport_params: Optional[dict] = None, # originalID -> a node with `.position` (start/end offsets) for WYSIWYG # picking and gizmo write-back. id_to_node = { - oid: _DeclNode(None, _Position(line, column, origin, start, end)) - for oid, (start, end, line, column, origin) in id_spans.items() + oid: _DeclNode( + None, + _Position(line, column, origin, start, end), + [_CallFrame(cl, cc, co, cs, ce, mod) + for (cs, ce, cl, cc, co, mod) in chain], + ) + for oid, (start, end, line, column, origin, chain) in id_spans.items() } return bodies, id_to_node diff --git a/src/bytecode_vm.cpp b/src/bytecode_vm.cpp index 363cdc3..19d216f 100644 --- a/src/bytecode_vm.cpp +++ b/src/bytecode_vm.cpp @@ -1400,6 +1400,7 @@ Value driveVm(Evaluator& ev, size_t floor) { treeNode->node = site.node; treeNode->isBuiltin = true; treeNode->warnEntry = ev.currentWarnEntry(); + treeNode->callChain = ev.internCurrentCallChain(); treeNode->children = std::move(children); treeNode->params = std::move(pending.params); treeNode->uncacheable = uncacheable; @@ -1503,6 +1504,7 @@ Value driveVm(Evaluator& ev, size_t floor) { treeNode->node = site.node; treeNode->isBuiltin = true; treeNode->warnEntry = ev.currentWarnEntry(); + treeNode->callChain = ev.internCurrentCallChain(); treeNode->children = std::move(children); treeNode->params = std::move(params); treeNode->uncacheable = uncacheable; diff --git a/src/csg_generate.cpp b/src/csg_generate.cpp index 5c50ac7..a9c4b1a 100644 --- a/src/csg_generate.cpp +++ b/src/csg_generate.cpp @@ -68,7 +68,8 @@ std::vector Evaluator::generateTreeImpl(const std::vector if (node.node && !measuring_) { auto producer = cacheProducer_.find(*key); restampCachedIds(node.bodies, *node.node, - producer == cacheProducer_.end() ? nullptr : producer->second); + producer == cacheProducer_.end() ? nullptr : producer->second, + node.callChain); } } else { // Recurse into children first (bottom-up) -- populates each @@ -95,9 +96,12 @@ std::vector Evaluator::generateTreeImpl(const std::vector // can name that line. Restored after (rather than left set) // so a sibling generated at top level doesn't inherit it. const oscad::Position* savedWarnEntry = generateWarnEntry; + const uint32_t savedCallChain = generateCallChain; generateWarnEntry = node.warnEntry; + generateCallChain = node.callChain; node.bodies = it->second(*this, node.params, node.children, *node.node); generateWarnEntry = savedWarnEntry; + generateCallChain = savedCallChain; } else { // No registered GenerateFn for this kind (a builtin with // display-only semantics like render(), or a non-builtin @@ -331,7 +335,7 @@ std::shared_ptr> remapParts(const std::vector& bodies, const oscad::ASTNode& node, - const oscad::ASTNode* producer) { + const oscad::ASTNode* producer, uint32_t callChain) { // A cache hit hands back the geometry AND the originalIDs of whichever // call site first produced it. Those IDs are provenance -- "which node // made this" -- not content, so two identical shapes at two call sites @@ -374,6 +378,12 @@ void Evaluator::restampCachedIds(std::vector& bodies, const oscad:: auto old = idToNode.find(id); idToNode[fresh] = (old != idToNode.end() && old->second != producer) ? old->second : &node; + // Unlike idToNode, this is the call site that reached the + // geometry NOW: whichever node the cached copy is being + // reused at is the line the user would be shown, whatever + // produced the original. A module called twice really is + // two different call sites. + idToCallChain[fresh] = callChain; // The colour has to be re-recorded too, or a later merge // cannot tell this body's runs apart from uncoloured ones // (attachTriColors looks runs up here). Across renders @@ -453,6 +463,7 @@ ColoredBody Evaluator::tagGenerated(manifold::Manifold body, const oscad::ASTNod for (uint32_t originalId : mesh.runOriginalID) { idToNode[originalId] = &node; idToColor[originalId] = color; + idToCallChain[originalId] = generateCallChain; } } ColoredBody cb; @@ -474,6 +485,7 @@ ColoredBody Evaluator::tagDisplayOnly(manifold::MeshGL mesh, const oscad::ASTNod if (!measuring_) { idToNode[originalId] = &node; idToColor[originalId] = color; + idToCallChain[originalId] = generateCallChain; } ColoredBody cb; diff --git a/src/csg_resolve.cpp b/src/csg_resolve.cpp index 4e4fcdc..c7c34df 100644 --- a/src/csg_resolve.cpp +++ b/src/csg_resolve.cpp @@ -68,6 +68,7 @@ void Evaluator::buildTreeNode(const std::string& kind, const oscad::ASTNode& nod treeNode->node = &node; treeNode->isBuiltin = true; treeNode->warnEntry = currentWarnEntry(); + treeNode->callChain = internCurrentCallChain(); treeNode->children = std::move(children); treeNode->params = std::move(params); treeNode->uncacheable = uncacheable; @@ -139,6 +140,7 @@ void Evaluator::evalModularCall(const oscad::ModularCall& node, EvalContext& ctx treeNode->node = &node; treeNode->isBuiltin = true; treeNode->warnEntry = currentWarnEntry(); + treeNode->callChain = internCurrentCallChain(); treeNode->uncacheable = uncacheable; treeNode->children = std::move(children); treeNode->params = std::move(params); @@ -164,6 +166,7 @@ void Evaluator::spliceModuleChildren(std::vector> child unionNode->node = &callNode; unionNode->isBuiltin = false; unionNode->warnEntry = currentWarnEntry(); + unionNode->callChain = internCurrentCallChain(); unionNode->uncacheable = std::any_of(children.begin(), children.end(), [](const auto& c) { return c->uncacheable; }); unionNode->children = std::move(children); setTreeDepthOrThrow(*unionNode, callNode); diff --git a/tests/test_python_bindings.py b/tests/test_python_bindings.py index 73e940d..fcbd790 100644 --- a/tests/test_python_bindings.py +++ b/tests/test_python_bindings.py @@ -13,6 +13,7 @@ wheels.yml's CIBW_TEST_COMMAND instead, which already builds+installs the package on every release platform. """ +import os import sys import tempfile from pathlib import Path @@ -876,3 +877,118 @@ def test_keep_minuend_color_paints_cut_faces_with_the_minuend(tmp_path): kept, _ = Evaluator(keep_minuend_color=True).evaluate(str(src), {}) assert kept[0].tri_colors is None # one colour: the minuend's assert all(abs(a - b) < 1e-3 for a, b in zip(kept[0].color[:3], (1.0, 0.647, 0.0))) + + +def _innermost_in(chain, path): + """What a front end does: the deepest frame it can actually show.""" + for f in chain: + if os.path.realpath(f.origin) == os.path.realpath(path): + return f + return None + + +def test_call_sites_is_the_whole_chain_library_frames_included(tmp_path): + """The chain is NOT filtered to the running script. + + Someone debugging the library wants to step into it, so the evaluator + records every frame and leaves the choice of level to the caller. + """ + import openscad_cpp_evaluator as E + + lib = tmp_path / "MYLIB" + lib.mkdir() + (lib / "std.scad").write_text( + "module boxy(s) { cube(s); }\n" + "module wrapped(s) { boxy(s); }\n" + ) + script = tmp_path / "m.scad" + src = 'include \nwrapped(10);\n' + script.write_text(src) + + old = os.environ.get("OPENSCADPATH") + os.environ["OPENSCADPATH"] = str(tmp_path) + try: + ev = E.Evaluator() + _bodies, id_to_node = ev.evaluate(str(script), {}) + finally: + if old is None: + os.environ.pop("OPENSCADPATH", None) + else: + os.environ["OPENSCADPATH"] = old + + assert id_to_node, "the script builds geometry" + for node in id_to_node.values(): + chain = node.call_sites + assert chain, "library-built geometry has a chain" + origins = [os.path.realpath(f.origin) for f in chain] + # Both layers of the library are present... + assert os.path.realpath(str(lib / "std.scad")) in origins + # ...and so is the user's own call, further out. + assert os.path.realpath(str(script)) in origins + # innermost-first: the library frame comes before the script's + assert origins.index(os.path.realpath(str(lib / "std.scad"))) < \ + origins.index(os.path.realpath(str(script))) + # node.call_site is just the innermost, for a caller that does not care + assert node.call_site is chain[0] + + +def test_a_front_end_picks_the_last_frame_in_the_running_script(tmp_path): + """The default a picker wants: the deepest frame the user wrote.""" + import openscad_cpp_evaluator as E + + lib = tmp_path / "MYLIB" + lib.mkdir() + (lib / "std.scad").write_text("module boxy(s) { cube(s); }\n") + script = tmp_path / "n.scad" + src = ("include \n" + "module inner() { boxy(8); }\n" + "module outer() { inner(); }\n" + "outer();\n") + script.write_text(src) + + old = os.environ.get("OPENSCADPATH") + os.environ["OPENSCADPATH"] = str(tmp_path) + try: + ev = E.Evaluator() + _bodies, id_to_node = ev.evaluate(str(script), {}) + finally: + if old is None: + os.environ.pop("OPENSCADPATH", None) + else: + os.environ["OPENSCADPATH"] = old + + assert id_to_node + for node in id_to_node.values(): + frame = _innermost_in(node.call_sites, str(script)) + assert frame is not None + text = src[frame.start_offset:frame.end_offset] + assert text.startswith("boxy(8)"), f"got {text!r}, wanted the innermost user call" + + +def test_module_and_function_frames_are_distinguished(tmp_path): + """A function frame has no geometry of its own, so a gizmo declines it.""" + import openscad_cpp_evaluator as E + + script = tmp_path / "f.scad" + script.write_text("function dbl(x) = x * 2;\n" + "module boxy(s) { cube(dbl(s)); }\n" + "boxy(4);\n") + ev = E.Evaluator() + _bodies, id_to_node = ev.evaluate(str(script), {}) + assert id_to_node + for node in id_to_node.values(): + assert all(isinstance(f.is_module, bool) for f in node.call_sites) + assert any(f.is_module for f in node.call_sites), "boxy() is a module frame" + +def test_top_level_geometry_has_no_call_site(): + """Nothing to redirect to: `position` is already the user's own node.""" + import openscad_cpp_evaluator as E + p = os.path.join(tempfile.gettempdir(), "toplevel_call_site.scad") + with open(p, "w") as f: + f.write("cube(10);\n") + ev = E.Evaluator() + _bodies, id_to_node = ev.evaluate(p, {}) + assert id_to_node + for node in id_to_node.values(): + assert node.call_site is None + assert os.path.realpath(node.position.origin) == os.path.realpath(p)