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" 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"); +}