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);",