From 8a0746ad16fb096600b37326fd094993ac1fe6e5 Mon Sep 17 00:00:00 2001 From: Revar Desmera Date: Thu, 20 Aug 2026 21:04:17 -0700 Subject: [PATCH 1/2] Do not weld coincident vertices when it would fuse touching shells Reported as a magenta patch in the viewport -- the renderer's inverted-normal warning -- on an XOR chain routed through render() and polyhedron(). The solid also silently lost 500 units of volume. Welding coincident vertices is a REPAIR, for meshes whose seams and poles carry duplicates; BOSL2 VNFs routinely do, and without it they arrive NotManifold. Applied unconditionally it destroys a mesh that was already sound. A solid whose shells TOUCH -- the two halves of an XOR meeting along a shared surface, a rod's stubs meeting the block they pass through -- has genuinely distinct vertices at identical positions. Merging those fuses the shells into edges with four faces. Measured on the reported model: 248 vertices, 168 distinct positions. Welding produced a watertight but NON-manifold mesh with 76 non-manifold edges, and 3304.65 where the same geometry built inline gives 3804.65. So weld only when it provably does no harm: build the welded candidate, check it, and keep it only if it is still manifold. checkMesh (mesh_check.hpp) is a cheap combinatorial pass with no Manifold construction, and its own doc comment already names this exact hazard -- "two boxes fused along a face are watertight but have edges with four faces". Both sides had the same flaw and both are gated: - measure_geometry.cpp, exporting a Manifold body to VNF - resolvePolyhedron, importing VNF back Fixing only one would not have helped: a faithful 248-vertex export was re-fused to 168 by polyhedron's own 1e-6 weld. The weld is emphatically NOT abandoned. Manifold splits property-vertices, so a plain cube arrives as 24 vertices and a script reading obj.vertices should still see 8; a BOSL2 spheroid still welds to watertight with zero unwelded vertices. Verified all three cases: tidy where safe, intact where not. resolvePolyhedron now triangulates against raw indices and derives the welded triangles by remapping. Welding only renames vertices, never moves them, so the ear clipping is identical either way -- no second triangulation. Four regression tests, including the reduced XOR chain from the report and a rod-through-block case where welding measurably turns 104 manifold vertices into 72 with 32 non-manifold edges. 957 tests pass under both engines. User confirmed the viewport visually. Co-Authored-By: Claude Opus 5 (1M context) --- src/builtins/primitives_3d.cpp | 57 +++++++++++++++++--- src/measure_geometry.cpp | 98 ++++++++++++++++++++++++++-------- tests/test_render_expr.cpp | 75 ++++++++++++++++++++++++++ 3 files changed, 200 insertions(+), 30 deletions(-) diff --git a/src/builtins/primitives_3d.cpp b/src/builtins/primitives_3d.cpp index 67f7e2f..0124308 100644 --- a/src/builtins/primitives_3d.cpp +++ b/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 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 rawTris; for (const Value& faceVal : (*facesList)->items) { const ListPtr* faceList = std::get_if(&faceVal); if (!faceList || !*faceList) continue; - std::vector remapped; - remapped.reserve((*faceList)->items.size()); + std::vector face; + face.reserve((*faceList)->items.size()); for (const Value& idxVal : (*faceList)->items) { const size_t idx = static_cast(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 weldedTris; + weldedTris.reserve(rawTris.size()); + for (uint32_t t : rawTris) weldedTris.push_back(static_cast(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>& outVerts = useWelded ? uniqueVerts : rawVerts; + const std::vector& tris = useWelded ? weldedTris : rawTris; + std::vector 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]}); diff --git a/src/measure_geometry.cpp b/src/measure_geometry.cpp index 137af5f..1e34fcb 100644 --- a/src/measure_geometry.cpp +++ b/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 @@ -69,35 +70,86 @@ Value objectOfPairs(std::vector> items) { // someone actually wants prettier output. template void meshToVertsAndFaces(const MeshT& mesh, Value& vertsOut, Value& facesOut) { - std::map, int> vertMap; + const size_t numProp = mesh.numProp == 0 ? 3 : static_cast(mesh.numProp); + const size_t numVert = numProp == 0 ? 0 : mesh.vertProperties.size() / numProp; + + std::vector> raw; + raw.reserve(numVert); + for (size_t v = 0; v < numVert; ++v) { + const size_t base = v * numProp; + raw.push_back({static_cast(mesh.vertProperties[base + 0]), + static_cast(mesh.vertProperties[base + 1]), + static_cast(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, uint32_t> seen; + std::vector remap(raw.size()); + std::vector> 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(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>& outVerts = useWelded ? welded : raw; std::vector verts; - std::vector 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(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; + const auto index = [&](uint32_t v) -> double { + const uint32_t i = useWelded ? (v < remap.size() ? remap[v] : 0) : v; + return static_cast(i); }; + std::vector 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(a)}, Value{static_cast(c)}, - Value{static_cast(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)); diff --git a/tests/test_render_expr.cpp b/tests/test_render_expr.cpp index 4b96714..b71e42c 100644 --- a/tests/test_render_expr.cpp +++ b/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"); +} From f495e4b42ba9ea9e08669bf8320970faba63cadd Mon Sep 17 00:00:00 2001 From: Revar Desmera Date: Thu, 20 Aug 2026 21:04:24 -0700 Subject: [PATCH 2/2] Bump to 0.38.1 for the touching-shells weld fix Patch, not minor: no new surface, and the previous behaviour was simply wrong -- 0.38.0 silently produced a non-manifold solid with the wrong volume for any mesh whose shells touch. Co-Authored-By: Claude Opus 5 (1M context) --- pyproject.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index 6d152cf..428f0df 100644 --- a/pyproject.toml +++ b/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"