Do not weld coincident vertices when it would fuse touching shells

pyproject.toml

@@ -4,7 +4,7 @@ build-backend = "scikit_build_core.build"
 
 [project]
 name = "openscad_cpp_evaluator"
-version = "0.38.0"
+version = "0.38.1"
 description = "C++ OpenSCAD evaluator with Python bindings"
 readme = "README.md"
 requires-python = ">=3.12"

src/builtins/primitives_3d.cpp

@@ -1,3 +1,4 @@
+#include "openscad_cpp_evaluator/mesh_check.hpp"
 #include "builtins.hpp"
 
 #include "openscad_cpp_evaluator/call_args.hpp"
@@ -666,22 +667,64 @@ CSGParams resolvePolyhedron(Evaluator& ev, const oscad::ModularCall& node, EvalC
         }
     }
 
-    std::vector<uint32_t> tris;
+    // Triangulate against the RAW indices first. Welding only renames
+    // vertices, never moves them, so the ear clipping is identical either
+    // way -- which means the welded triangles can be derived afterwards by
+    // remapping, rather than triangulating twice.
+    std::vector<uint32_t> rawTris;
     for (const Value& faceVal : (*facesList)->items) {
         const ListPtr* faceList = std::get_if<ListPtr>(&faceVal);
         if (!faceList || !*faceList) continue;
-        std::vector<size_t> remapped;
-        remapped.reserve((*faceList)->items.size());
+        std::vector<size_t> face;
+        face.reserve((*faceList)->items.size());
         for (const Value& idxVal : (*faceList)->items) {
             const size_t idx = static_cast<size_t>(toDoubleLenient(idxVal));
-            remapped.push_back(idx < remap.size() ? remap[idx] : 0);
+            face.push_back(idx < rawVerts.size() ? idx : 0);
         }
-        triangulateFace(uniqueVerts, remapped, tris);
+        triangulateFace(rawVerts, face, rawTris);
+    }
+
+    std::vector<uint32_t> weldedTris;
+    weldedTris.reserve(rawTris.size());
+    for (uint32_t t : rawTris) weldedTris.push_back(static_cast<uint32_t>(remap[t]));
+
+    // Welding is a repair for meshes whose seams and poles carry duplicate
+    // vertices -- BOSL2 VNFs routinely do, and without it they come out as
+    // NotManifold. But it is only ever a repair, and applied blindly it
+    // BREAKS a mesh that was already sound: a solid with two shells that
+    // touch (the two halves of an XOR meeting along a shared surface) has
+    // genuinely coincident vertices belonging to different shells, and
+    // merging those fuses the shells into edges with four faces. Measured
+    // on exactly such a case: 248 raw vertices, 168 distinct positions,
+    // welding turned a watertight manifold mesh into one with 76
+    // non-manifold edges and silently lost 500 units of volume.
+    //
+    // So: weld only when the raw mesh actually needs it. checkMesh is a
+    // cheap combinatorial pass (no Manifold construction), and its own doc
+    // comment names this exact hazard -- "two boxes fused along a face are
+    // watertight but have edges with four faces".
+    const bool weldChangesAnything = uniqueVerts.size() != rawVerts.size();
+    bool useWelded = weldChangesAnything;
+    if (weldChangesAnything) {
+        manifold::MeshGL64 probe;
+        probe.numProp = 3;
+        probe.vertProperties.reserve(rawVerts.size() * 3);
+        for (const auto& v : rawVerts) {
+            probe.vertProperties.push_back(v[0]);
+            probe.vertProperties.push_back(v[1]);
+            probe.vertProperties.push_back(v[2]);
+        }
+        probe.triVerts.assign(rawTris.begin(), rawTris.end());
+        // Already sound without the repair -> leave it alone.
+        if (checkMesh(probe).manifold()) useWelded = false;
     }
 
+    const std::vector<std::array<double, 3>>& outVerts = useWelded ? uniqueVerts : rawVerts;
+    const std::vector<uint32_t>& tris = useWelded ? weldedTris : rawTris;
+
     std::vector<Value> vertsValues;
-    vertsValues.reserve(uniqueVerts.size() * 3);
-    for (const auto& v : uniqueVerts) {
+    vertsValues.reserve(outVerts.size() * 3);
+    for (const auto& v : outVerts) {
         vertsValues.push_back(Value{v[0]});
         vertsValues.push_back(Value{v[1]});
         vertsValues.push_back(Value{v[2]});

src/measure_geometry.cpp

@@ -18,6 +18,7 @@
 #include "openscad_cpp_evaluator/evaluator.hpp"
 
 #include "builtins/builtins.hpp"
+#include "openscad_cpp_evaluator/mesh_check.hpp"
 
 #include <manifold/manifold.h>
 
@@ -69,35 +70,86 @@ Value objectOfPairs(std::vector<std::pair<std::string, Value>> items) {
 // someone actually wants prettier output.
 template <typename MeshT>
 void meshToVertsAndFaces(const MeshT& mesh, Value& vertsOut, Value& facesOut) {
-    std::map<std::array<double, 3>, int> vertMap;
+    const size_t numProp = mesh.numProp == 0 ? 3 : static_cast<size_t>(mesh.numProp);
+    const size_t numVert = numProp == 0 ? 0 : mesh.vertProperties.size() / numProp;
+
+    std::vector<std::array<double, 3>> raw;
+    raw.reserve(numVert);
+    for (size_t v = 0; v < numVert; ++v) {
+        const size_t base = v * numProp;
+        raw.push_back({static_cast<double>(mesh.vertProperties[base + 0]),
+                       static_cast<double>(mesh.vertProperties[base + 1]),
+                       static_cast<double>(mesh.vertProperties[base + 2])});
+    }
+
+    // Weld by exact position, purely for tidiness: Manifold splits
+    // property-vertices, so a plain cube arrives as 24 vertices rather than
+    // 8, and a script reading obj.vertices should not have to see that.
+    std::map<std::array<double, 3>, uint32_t> seen;
+    std::vector<uint32_t> remap(raw.size());
+    std::vector<std::array<double, 3>> welded;
+    for (size_t i = 0; i < raw.size(); ++i) {
+        auto it = seen.find(raw[i]);
+        if (it != seen.end()) {
+            remap[i] = it->second;
+        } else {
+            const uint32_t idx = static_cast<uint32_t>(welded.size());
+            welded.push_back(raw[i]);
+            seen.emplace(raw[i], idx);
+            remap[i] = idx;
+        }
+    }
+
+    // ...but tidiness must never cost correctness. Coincident vertices are
+    // not always redundant: a solid with two shells that TOUCH (the two
+    // halves of an XOR meeting along a shared surface) has genuinely
+    // distinct vertices at identical positions, and merging those fuses the
+    // shells into edges with four faces. Measured on exactly that case: 248
+    // vertices, 168 distinct positions, and welding turned a watertight
+    // manifold mesh into one with 76 non-manifold edges -- which then lost
+    // 500 units of volume on the way back through polyhedron().
+    //
+    // Manifold's own indexing is manifold by construction, so the raw mesh
+    // is always the safe answer; the weld is kept only when it provably
+    // does no harm. checkMesh is a cheap combinatorial pass.
+    bool useWelded = welded.size() != raw.size();
+    if (useWelded) {
+        manifold::MeshGL64 probe;
+        probe.numProp = 3;
+        probe.vertProperties.reserve(welded.size() * 3);
+        for (const auto& v : welded) {
+            probe.vertProperties.push_back(v[0]);
+            probe.vertProperties.push_back(v[1]);
+            probe.vertProperties.push_back(v[2]);
+        }
+        probe.triVerts.reserve(mesh.triVerts.size());
+        for (uint32_t t : mesh.triVerts) {
+            probe.triVerts.push_back(t < remap.size() ? remap[t] : 0);
+        }
+        if (!checkMesh(probe).manifold()) useWelded = false;
+    }
+
+    const std::vector<std::array<double, 3>>& outVerts = useWelded ? welded : raw;
     std::vector<Value> verts;
-    std::vector<Value> faces;
+    verts.reserve(outVerts.size());
+    for (const auto& v : outVerts) verts.push_back(pointOf(v[0], v[1], v[2]));
 
-    const size_t numProp = mesh.numProp == 0 ? 3 : static_cast<size_t>(mesh.numProp);
-    const size_t numVert = mesh.vertProperties.size() / (numProp == 0 ? 1 : numProp);
-
-    const auto indexOf = [&](uint32_t v) -> int {
-        if (static_cast<size_t>(v) >= numVert) return -1;
-        const size_t base = static_cast<size_t>(v) * numProp;
-        const std::array<double, 3> pos = {static_cast<double>(mesh.vertProperties[base + 0]),
-                                           static_cast<double>(mesh.vertProperties[base + 1]),
-                                           static_cast<double>(mesh.vertProperties[base + 2])};
-        auto it = vertMap.find(pos);
-        if (it != vertMap.end()) return it->second;
-        const int idx = static_cast<int>(verts.size());
-        vertMap.emplace(pos, idx);
-        verts.push_back(pointOf(pos[0], pos[1], pos[2]));
-        return idx;
+    const auto index = [&](uint32_t v) -> double {
+        const uint32_t i = useWelded ? (v < remap.size() ? remap[v] : 0) : v;
+        return static_cast<double>(i);
     };
 
+    std::vector<Value> faces;
+    faces.reserve(mesh.triVerts.size() / 3);
     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;
+        if (mesh.triVerts[t] >= numVert || mesh.triVerts[t + 1] >= numVert ||
+            mesh.triVerts[t + 2] >= numVert) {
+            continue;
+        }
         // a, c, b -- see (1) above.
-        faces.push_back(listOf({Value{static_cast<double>(a)}, Value{static_cast<double>(c)},
-                                Value{static_cast<double>(b)}}));
+        faces.push_back(listOf({Value{index(mesh.triVerts[t + 0])},
+                                Value{index(mesh.triVerts[t + 2])},
+                                Value{index(mesh.triVerts[t + 1])}}));
     }
 
     vertsOut = listOf(std::move(verts));

tests/test_render_expr.cpp

@@ -384,3 +384,78 @@ TEST(RenderExprEngines, ProvenanceStaysCleanUnderTheVm) {
     EXPECT_EQ(with.ev.idToNode.size(), without.ev.idToNode.size());
     EXPECT_EQ(with.ev.idToColor.size(), without.ev.idToColor.size());
 }
+
+// -- Touching shells must not be welded together ---------------------------
+//
+// Regression for a silent, geometry-destroying bug. Welding coincident
+// vertices is a REPAIR for meshes whose seams carry duplicates (BOSL2 VNFs
+// routinely do). Applied blindly it destroys a mesh that was already sound:
+// a solid whose shells TOUCH has genuinely distinct vertices at identical
+// positions, and merging those fuses the shells into edges with four faces.
+//
+// Measured on the case below: welding turned a watertight manifold mesh of
+// 104 vertices into a 72-vertex one with 32 non-manifold edges. Both the
+// exporter and polyhedron() now weld only when it provably does no harm.
+
+namespace {
+
+// A block with a rod through it, XORed: the rod's protruding stubs touch the
+// block's faces exactly, so the shells share vertex positions.
+constexpr const char* kTouchingShells =
+    "union() {\n"
+    "  difference(){ cube([20,20,20],center=true); cylinder(d=8,h=60,center=true,$fn=16); }\n"
+    "  difference(){ cylinder(d=8,h=60,center=true,$fn=16); cube([20,20,20],center=true); }\n"
+    "}\n";
+
+} // namespace
+
+TEST(RenderExpr, DoesNotWeldTouchingShellsTogether) {
+    Measured r = runScript(std::string("o = render() { ") + kTouchingShells + " };\n"
+                            "echo(o.volume, o.genus, len(o.vertices));");
+    ASSERT_EQ(r.echoes.size(), 1u);
+    // 104, not 72: the 32 coincident-but-distinct vertices are kept apart.
+    // A negative genus is the signal that this solid has several shells.
+    EXPECT_EQ(r.echoes[0], "ECHO: 8979.67, -1, 104");
+}
+
+TEST(RenderExpr, TouchingShellsSurviveThePolyhedronRoundTrip) {
+    // The end-to-end symptom: volume silently dropped on the way back
+    // through polyhedron(), and the viewport showed backfaces where the
+    // fused shells had inverted.
+    Measured r = runScript(std::string("o = render() { ") + kTouchingShells + " };\n"
+                            "rt = render() { polyhedron(o); };\n"
+                            "echo(o.volume == rt.volume, o.genus == rt.genus,\n"
+                            "     len(o.vertices) == len(rt.vertices));");
+    ASSERT_EQ(r.echoes.size(), 1u);
+    EXPECT_EQ(r.echoes[0], "ECHO: true, true, true");
+}
+
+TEST(RenderExpr, WeldingStillHappensWhenItIsSafe) {
+    // The weld must not be abandoned wholesale -- Manifold splits
+    // property-vertices, so a plain cube arrives as 24 vertices and a script
+    // reading obj.vertices should still see 8.
+    Measured r = runScript("o = render() { cube(10); };\necho(len(o.vertices), len(o.faces));");
+    ASSERT_EQ(r.echoes.size(), 1u);
+    EXPECT_EQ(r.echoes[0], "ECHO: 8, 12");
+}
+
+TEST(RenderExpr, XorChainMatchesTheSameGeometryBuiltDirectly) {
+    // The reported failure, reduced: an XOR chain routed through render()
+    // and polyhedron() must produce the same solid as building it inline.
+    const std::string shapes =
+        "$fn=36;\n"
+        "module geometry(o) { if (o.dim == 3) polyhedron(o); }\n"
+        "A = [30,10,10]; B = [10,30,10];\n";
+    const std::string inlineXor =
+        shapes +
+        "x = render() { union() {\n"
+        "  difference(){ cube(A,center=true); cube(B,center=true); }\n"
+        "  difference(){ cube(B,center=true); cube(A,center=true); } } };\n"
+        "y = render() { union() {\n"
+        "  difference(){ polyhedron(x); rotate([90,0,0]) cylinder(d=5,h=30,center=true); }\n"
+        "  difference(){ rotate([90,0,0]) cylinder(d=5,h=30,center=true); polyhedron(x); } } };\n"
+        "echo(y.volume);";
+    Measured r = runScript(inlineXor);
+    ASSERT_EQ(r.echoes.size(), 1u);
+    EXPECT_EQ(r.echoes[0], "ECHO: 3804.65");
+}