From 64fe79e91f9766794082303c376a5e45b3d24587 Mon Sep 17 00:00:00 2001 From: Revar Desmera Date: Sun, 9 Aug 2026 19:05:02 -0700 Subject: [PATCH] Give cached bodies fresh originalIDs per call site Two identical shapes at two call sites came back sharing one originalID, so a UI keying off it treated them as one thing: selecting either picked both. Reported against two identical cylinders; it affects any array of identical parts, which in CAD is most of them. A cache hit handed back the geometry AND the IDs of whichever call site first produced it. Those IDs are provenance -- which node made this -- not content, so they cannot be cached along with the shape. Each distinct run now gets a fresh ID on a hit, one per run rather than one per body. That matters: Manifold's own AsOriginal() collapses a multi-run body to a single ID, which would have made a cached subtree of several parts selectable only as a whole. Each new ID inherits the old one's node and colour, so parts keep pointing at the AST nodes that actually produced them. Measured on 200 identical spheres, where every one after the first is a hit: 170 ms cold, 128 ms warm. The re-stamp rebuilds each cached body from its mesh, and the cache still pays for itself comfortably. Co-Authored-By: Claude Opus 5 (1M context) --- include/openscad_cpp_evaluator/evaluator.hpp | 4 ++ pyproject.toml | 2 +- src/csg_generate.cpp | 32 ++++++++++++ tests/test_manifold_cache.cpp | 51 ++++++++++++++++++++ 4 files changed, 88 insertions(+), 1 deletion(-) diff --git a/include/openscad_cpp_evaluator/evaluator.hpp b/include/openscad_cpp_evaluator/evaluator.hpp index 272ac31..20bc53c 100644 --- a/include/openscad_cpp_evaluator/evaluator.hpp +++ b/include/openscad_cpp_evaluator/evaluator.hpp @@ -223,6 +223,10 @@ class Evaluator { // evaluate()/resolveTree()+generateTree() complete. Mirrors the // reference's Evaluator.id_to_node/id_to_color (also plain public // attributes there). + // 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); + std::unordered_map idToNode; std::unordered_map>> idToColor; diff --git a/pyproject.toml b/pyproject.toml index 804b924..87f3b5c 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "scikit_build_core.build" [project] name = "openscad_cpp_evaluator" -version = "0.26.1" +version = "0.27.0" description = "C++ OpenSCAD evaluator with Python bindings" readme = "README.md" requires-python = ">=3.12" diff --git a/src/csg_generate.cpp b/src/csg_generate.cpp index 92b1a69..9b26807 100644 --- a/src/csg_generate.cpp +++ b/src/csg_generate.cpp @@ -48,6 +48,7 @@ std::vector Evaluator::generateTreeImpl(const std::vector std::optional> cached = key ? manifoldCache_->get(*key) : std::nullopt; if (cached) { node.bodies = std::move(*cached); + restampCachedIds(node.bodies); } else { // Recurse into children first (bottom-up) -- populates each // child's own .bodies in place so flattenCsgTree/a GenerateFn @@ -104,6 +105,37 @@ std::vector Evaluator::generatePartialTree() { return generateTreeImpl(flat); } +void Evaluator::restampCachedIds(std::vector& bodies) { + // 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 + // came back sharing one identity and selecting either picked both. + // + // Each distinct run gets a fresh ID, one per run rather than one per + // body, so a cached subtree made of several parts stays selectable + // part by part. Each new ID inherits the old one's node and color, so + // the parts keep pointing at the AST nodes that really produced them. + for (ColoredBody& cb : bodies) { + if (!cb.body || cb.body->IsEmpty()) continue; + manifold::MeshGL mesh = cb.body->GetMeshGL(); + if (mesh.runOriginalID.empty()) continue; + std::unordered_map remap; + for (uint32_t& id : mesh.runOriginalID) { + auto found = remap.find(id); + if (found == remap.end()) { + const uint32_t fresh = manifold::Manifold::ReserveIDs(1); + auto node = idToNode.find(id); + if (node != idToNode.end()) idToNode[fresh] = node->second; + auto color = idToColor.find(id); + if (color != idToColor.end()) idToColor[fresh] = color->second; + found = remap.emplace(id, fresh).first; + } + id = found->second; + } + cb.body = manifold::Manifold(mesh); + } +} + ColoredBody Evaluator::tagGenerated(manifold::Manifold body, const oscad::ASTNode& node, const Value& colorValue) { manifold::MeshGL mesh = body.GetMeshGL(); std::optional> color = valueToColor(colorValue); diff --git a/tests/test_manifold_cache.cpp b/tests/test_manifold_cache.cpp index e949627..b9baf73 100644 --- a/tests/test_manifold_cache.cpp +++ b/tests/test_manifold_cache.cpp @@ -5,7 +5,10 @@ #include "test_helpers.hpp" #include +#include +#include #include +#include using namespace oscadeval; using namespace oscadeval::test; @@ -156,6 +159,54 @@ TEST(ManifoldCache, UncacheableNodeAlwaysRegeneratesNeverHitsOrPopulatesCache) { EXPECT_FALSE(second.ev.idToNode.empty()); } +// A cache hit must not hand back the originalIDs of whichever call site +// first produced the geometry. Those IDs are provenance, and sharing them +// made two identical shapes one thing to a UI: selecting either picked +// both. Common in CAD, where arrays of identical parts are the norm. +TEST(ManifoldCache, IdenticalShapesAtDifferentCallSitesGetDistinctOriginalIds) { + auto cache = std::make_shared(); + Evaluated e = evalSrcWithCache( + "translate([0,0,0]) cylinder(h=20,r=4,$fn=32);\n" + "translate([25,0,0]) cylinder(h=20,r=4,$fn=32);\n", cache); + ASSERT_EQ(e.bodies.size(), 2u); + + // Held in locals: GetMeshGL() returns by value, so taking begin() and + // end() from two separate calls walks between unrelated buffers. + const manifold::MeshGL meshA = e.bodies[0].body->GetMeshGL(); + const manifold::MeshGL meshB = e.bodies[1].body->GetMeshGL(); + const std::set first(meshA.runOriginalID.begin(), meshA.runOriginalID.end()); + const std::set second(meshB.runOriginalID.begin(), meshB.runOriginalID.end()); + ASSERT_FALSE(first.empty()); + ASSERT_FALSE(second.empty()); + std::vector shared; + std::set_intersection(first.begin(), first.end(), second.begin(), second.end(), + std::back_inserter(shared)); + EXPECT_TRUE(shared.empty()) << "the two cylinders share an originalID"; + + // Both still resolve to a node, so re-stamping does not lose the + // mapping selection needs. + for (uint32_t id : first) EXPECT_EQ(e.ev.idToNode.count(id), 1u); + for (uint32_t id : second) EXPECT_EQ(e.ev.idToNode.count(id), 1u); +} + +// Re-stamping is per run, not per body, so a cached subtree made of +// several parts stays selectable part by part rather than collapsing into +// one -- which is what Manifold's own AsOriginal() would have done. +TEST(ManifoldCache, ACachedMultiPartSubtreeKeepsOneIdPerPart) { + auto cache = std::make_shared(); + Evaluated e = evalSrcWithCache( + "module pair() { cube(2); translate([5,0,0]) sphere(1,$fn=8); }\n" + "pair();\n" + "translate([0,20,0]) pair();\n", cache); + ASSERT_EQ(e.bodies.size(), 4u); + std::set all; + for (const ColoredBody& b : e.bodies) { + const manifold::MeshGL mesh = b.body->GetMeshGL(); + for (uint32_t id : mesh.runOriginalID) all.insert(id); + } + EXPECT_EQ(all.size(), 4u) << "parts of the reused subtree share IDs"; +} + TEST(ManifoldCache, OutputIsIdenticalCacheOnVsCacheOffAcrossBuiltinCategories) { const std::vector scripts = { "cube(3);",