From 35a4e5efa9f59936597c1b063d95e29cee09c59c Mon Sep 17 00:00:00 2001 From: Ni Jingzhe <1448976235@qq.com> Date: Thu, 12 Nov 2026 03:31:21 +0800 Subject: [PATCH 1/2] fix(kernel): cut and intersect unify what they leave split, and a blend never returns an invalid solid (#88) DECISION: three changes, in this order of importance, each independent of the others. (1) a fillet or chamfer whose result OCCT calls invalid is an error; (2) cut_rsolid, cut_rsolid_list and intersect_rsolid unify the same-domain faces and edges of their result by default, with `clean=False` to keep the raw result, as union already does; (3) the diagnosis names split faces instead of blaming the radius. No compatibility shim for records made before `clean` existed: replay requires the parameter, like every other recorded one. PROBLEM (reproduced on the issue's two cases, current dev build on Windows, OCCT 7.9.3) A cut through the side of a box with a cylinder splits the cylinder wall along the cylinder's parametric seam into two faces, and the top edge of the notch into a long and a short arc. union unified (ShapeUpgrade_UnifySameDomain) and cut did not. OCCT then fillets that top edge wrongly: - case A: BRepCheck-invalid solid, volume 28305.59 (should be 29604.46), no error at all: `fillet_rsolid` checked only IsDone and volume > 0; - case B: refusal, and the diagnosis said "the radius exceeds the 2.5 mm of face room" - the half of the wall the seam cut off - so the advised radius was not the limit. Same geometry, other libraries: CadQuery and build123d clean after a boolean by default and give 29604.46 / 29811.31. (1) THE EXIT CHECK IS THE MOST IMPORTANT `assert_blend_valid`, next to the volume check in `finish_blend` (fillet and chamfer): BRepCheck_Analyzer on the result; invalid while the input was valid is a BlendValidityViolation, which leads the error message ("the kernel returned broken geometry"), like the existing volume violation. Not raised when the input was already invalid (an imported solid is not the blend's fault). It turns "wrong volume found three operations later" into an error at the operation that caused it, whatever else is or is not done about cleaning. (2) CLEAN BY DEFAULT, WITH THE HISTORY FOLLOWING SDK booleans go through `tracked_cut` / `tracked_intersect`, which ask the kernel's history what became of every face (that is how tags and lineage survive), so cleaning only the shape would break tracking. `BooleanOptions::clean` makes `run_boolean` unify the result, merge the unifier's history into the boolean's, and follow the section edges through it; the section-edge following was lifted out of `fuse_shapes_with_history` into one helper both use (fuse behaves as before). The unified shape replaces the result only when it has the same number of solids and is valid whenever the raw result was; otherwise the raw result stands. FreeCAD and build123d fall back the same way, and replicad's unconditional SimplifyResult has been seen to invalidate results (BarbourSmith/Abundance#1934), so the fallback is not decoration. `clean` is a keyword of the three functions (default True), recorded as params.clean like union's. C++ `cut_shapes`, `common_shapes` and `tracked_*` keep `clean=false` as their default so other callers (diagnosis, set booleans, std parts) are unchanged. Not done on purpose: OCCT's own SimplifyResult(true, true) that the issue suggests: the explicit unifier already gives the same merge and history, with a fallback we control. REPLAY: NO LEGACY BRANCH (owner's rule: do not keep old formats alive) `flag(p, "clean")` and `ctx.require_params(..., "clean")` fail on a graph recorded before this change, as they do for any missing parameter. Such a graph has to be recorded again. (3) THE DIAGNOSIS `classify_split_faces`, before the face-room probe: unify the solid, carry the selected edges over with the unifier's history, blend again with the same size; if that works the split faces are the cause, and the message says so and how to fix it (clean=True, or union_rsolid(clean=True)). The room probe stays for a radius that really is too large (test: radius 50 on the cleaned case is still reported as too large). EVIDENCE - Both cases now give valid fillets with volumes 29604.46 and 29811.31, equal to CadQuery's. - clean=False: A raises "not a valid solid" (it was returned), B raises "faces ... split on a surface ... clean=True". - test/test_boolean_clean.py (10 tests): face counts 8 against 9, volume unchanged by cleaning, cut_list and intersect, both cases, both clean=False diagnoses, a really-too-large radius, `clean` recorded, replay giving 8 / 9 faces. - A random sample, 120 cases of the issue's recipe (box minus 2-4 vertical boxes/cylinders on a grid, top edges, r 1-3), both modes: 240 results, none invalid and returned (clean=True 84 valid / 36 errors, clean=False 83 / 37). - Full Python suite on this machine: 2674 passed; 22 failed and 9 errors, none touching booleans (Windows paths, encodings, file locks, addon and shell integration, missing OCP, a math-library last digit in test_tolerance). NOT DONE / NOT KNOWN - The C++ unit tests (ctest) were not built or run here (SCAD_BUILD_TESTS is off for the wheel build). CutOptions::clean defaults to true, so a C++ test that compares a cut's topology may need its expectation updated: CI will say. - The issue's 16000-case run was not repeated, only the small sample above; it shows that no invalid result is returned, not the 0.4% rate of the issue (there is no old binary here to compare with). - A stored artifact or fingerprint of a model with a cut that left split faces changes: the faces are fewer. That is the point of the change; rebuild such models. - Unifying can still make a later blend worse on rare inputs (the issue saw 6 segfaults after unify in other libraries too); here the unify is inside the boolean, so a crash would show in the boolean. Co-Authored-By: Claude Sonnet 5.5 --- docs/skill/references/docs/api/cut_rsolid.md | 6 +- .../references/docs/api/cut_rsolid_list.md | 6 +- .../references/docs/api/intersect_rsolid.md | 6 +- .../docs/core/operation_graph_json_spec.md | 3 + src/cpp/bindings/python/bind_ops_features.cpp | 19 +-- src/cpp/include/scad/kernel/booleans.hpp | 12 +- .../include/scad/ops/features/booleans.hpp | 7 +- .../include/scad/ops/features/diagnostics.hpp | 13 +- src/cpp/include/scad/semantic/tracking.hpp | 9 +- src/cpp/src/kernel/booleans.cpp | 95 +++++++++---- src/cpp/src/ops/features/blend_diagnosis.cpp | 97 +++++++++++++ src/cpp/src/ops/features/blends.cpp | 7 +- src/cpp/src/ops/features/booleans.cpp | 11 +- src/cpp/src/ops/features/executors.cpp | 10 +- src/cpp/src/semantic/tracking_booleans.cpp | 13 +- src/simplecadapi/_glue/docstrings.json | 6 +- src/simplecadapi/recording/serializer.py | 12 +- test/test_boolean_clean.py | 128 ++++++++++++++++++ 18 files changed, 389 insertions(+), 71 deletions(-) create mode 100644 test/test_boolean_clean.py diff --git a/docs/skill/references/docs/api/cut_rsolid.md b/docs/skill/references/docs/api/cut_rsolid.md index d315f164..0a863726 100644 --- a/docs/skill/references/docs/api/cut_rsolid.md +++ b/docs/skill/references/docs/api/cut_rsolid.md @@ -3,7 +3,7 @@ ## API Definition ```python -def cut_rsolid(*solids, skip_non_intersecting=True, tracking_policy='full', on_split='pieces') +def cut_rsolid(*solids, skip_non_intersecting=True, tracking_policy='full', on_split='pieces', clean=True) ``` *Source: operators/boolean.py* @@ -33,6 +33,10 @@ sequences, and returns a single `Solid`. - **Description**: FULL computes topology history and lineage. GRAPH keeps the replayable operation node without computing a TopoDelta. +### clean + +- **Description**: When True (the default) the faces and edges the cut leaves split are unified, as union does: a cylinder wall cut along its seam stays one face, so a later fillet or chamfer of its edges works. False keeps the raw kernel result. + ### on_split - **Description**: What a cut that splits the body (a slot through a bar) returns. diff --git a/docs/skill/references/docs/api/cut_rsolid_list.md b/docs/skill/references/docs/api/cut_rsolid_list.md index 930d17f1..e3e54020 100644 --- a/docs/skill/references/docs/api/cut_rsolid_list.md +++ b/docs/skill/references/docs/api/cut_rsolid_list.md @@ -3,7 +3,7 @@ ## API Definition ```python -def cut_rsolid_list(*solids, skip_non_intersecting=True, tracking_policy='full') +def cut_rsolid_list(*solids, skip_non_intersecting=True, tracking_policy='full', clean=True) ``` *Source: operators/boolean.py* @@ -39,6 +39,10 @@ a through slot. Use ``cut_rsolid`` when the result must be one solid. - **Description**: FULL computes topology history and lineage. GRAPH keeps the replayable operation node without computing a TopoDelta. +### clean + +- **Description**: When True (the default) the faces and edges the cut leaves split are unified, as union does: a cylinder wall cut along its seam stays one face, so a later fillet or chamfer of its edges works. False keeps the raw kernel result. + ## Returns list[Solid]: Every piece of the cut, in kernel order; empty when the tools diff --git a/docs/skill/references/docs/api/intersect_rsolid.md b/docs/skill/references/docs/api/intersect_rsolid.md index 023b0b3e..26ce58b7 100644 --- a/docs/skill/references/docs/api/intersect_rsolid.md +++ b/docs/skill/references/docs/api/intersect_rsolid.md @@ -3,7 +3,7 @@ ## API Definition ```python -def intersect_rsolid(*solids) +def intersect_rsolid(*solids, clean=True) ``` *Source: operators/boolean.py* @@ -26,6 +26,10 @@ error instead of returning an empty list. - **Description**: One or more Solid objects or sequences of Solid. Nested sequences are flattened before processing. +### clean + +- **Description**: When True (the default) the faces and edges the intersection leaves split are unified, as union does: a cylinder wall cut along its seam stays one face, so a later fillet or chamfer of its edges works. False keeps the raw kernel result. + ## Returns Solid: The overlap region as a single solid. diff --git a/docs/skill/references/docs/core/operation_graph_json_spec.md b/docs/skill/references/docs/core/operation_graph_json_spec.md index b0936824..f703e88f 100644 --- a/docs/skill/references/docs/core/operation_graph_json_spec.md +++ b/docs/skill/references/docs/core/operation_graph_json_spec.md @@ -1567,12 +1567,14 @@ Notes: | --- | --- | --- | | `tool_count` | `int` | number of subtractive tool solids | | `skip_non_intersecting` | `bool` | whether non-intersecting tools are skipped | +| `clean` | `bool` | whether the faces and edges the cut leaves split were unified | Notes: - Public `cut_rsolid(...)` defaults this flag to `true` for interactive convenience. - Graph replay defaults missing `skip_non_intersecting` to `false` so malformed or drifting graphs fail diagnostically instead of silently skipping tools. - Multi-tool cut preserves the full topology delta chain across all performed tool cuts. +- `clean` has no replay default: a graph without it is rejected like one without `tool_count`. #### `make_intersect_rsolid` @@ -1583,6 +1585,7 @@ Notes: | Key | Type | Meaning | | --- | --- | --- | | `input_count` | `int` | number of intersected solids | +| `clean` | `bool` | whether the faces and edges the intersection leaves split were unified | Notes: diff --git a/src/cpp/bindings/python/bind_ops_features.cpp b/src/cpp/bindings/python/bind_ops_features.cpp index 383961f8..2a502ba3 100644 --- a/src/cpp/bindings/python/bind_ops_features.cpp +++ b/src/cpp/bindings/python/bind_ops_features.cpp @@ -683,9 +683,10 @@ void bind_booleans(py::module_& m) { m.def( "cut_rsolid", [](const py::args& solids, bool skip_non_intersecting, const Policy& tracking_policy, - const Choice& on_split) -> AnyOf { + const Choice& on_split, bool clean) -> AnyOf { const std::vector operands = flattened(solids, "cut_rsolid"); - const CutOptions options{skip_non_intersecting, policy_text(tracking_policy)}; + CutOptions options{skip_non_intersecting, policy_text(tracking_policy)}; + options.clean = clean; if (on_split.value == "error") return {py::cast(unlocked(false, [&] { return cut_rsolid(operands, options); }))}; if (on_split.value != "pieces") @@ -696,27 +697,29 @@ void bind_booleans(py::module_& m) { return {solids_to_py(outcome.pieces)}; // the cut split the body: every piece, ordered by centroid }, py::kw_only(), py::arg("skip_non_intersecting") = true, py::arg("tracking_policy") = "full", - py::arg("on_split") = "pieces", + py::arg("on_split") = "pieces", py::arg("clean") = true, "Compute the boolean difference of solids: the first is the base, the rest are subtracted in order."); m.def( "cut_rsolid_list", - [](const py::args& solids, bool skip_non_intersecting, const Policy& tracking_policy) { + [](const py::args& solids, bool skip_non_intersecting, const Policy& tracking_policy, bool clean) { const std::vector operands = flattened(solids, "cut_rsolid_list"); - const CutOptions options{skip_non_intersecting, policy_text(tracking_policy)}; + CutOptions options{skip_non_intersecting, policy_text(tracking_policy)}; + options.clean = clean; return solids_to_py(unlocked(false, [&] { return cut_rsolid_list(operands, options); })); }, py::kw_only(), py::arg("skip_non_intersecting") = true, py::arg("tracking_policy") = "full", + py::arg("clean") = true, "Compute the boolean difference of solids and keep every piece: a cut that splits the body returns each part, " "and one that consumes the body returns an empty list."); m.def( "intersect_rsolid", - [](const py::args& solids) { + [](const py::args& solids, bool clean) { const std::vector operands = flattened(solids, "intersect_rsolid"); - return unlocked(false, [&] { return intersect_rsolid(operands); }); + return unlocked(false, [&] { return intersect_rsolid(operands, clean); }); }, - "Compute the boolean intersection of solids."); + py::kw_only(), py::arg("clean") = true, "Compute the boolean intersection of solids."); m.def( "boolean_rsolidlist", diff --git a/src/cpp/include/scad/kernel/booleans.hpp b/src/cpp/include/scad/kernel/booleans.hpp index 38b8f620..a5f96f02 100644 --- a/src/cpp/include/scad/kernel/booleans.hpp +++ b/src/cpp/include/scad/kernel/booleans.hpp @@ -22,6 +22,10 @@ struct BooleanOptions { bool fill_history = false; // keep BRepTools_History + section edges std::optional fuzzy; // fuzzy tolerance (SetFuzzyValue) bool glue = false; // BOPAlgo_GlueShift instead of GlueOff + // Unify the same-domain faces and edges of the result (what a cut leaves behind: a cylinder wall split along its + // seam, an edge split in two). The history, when kept, and the section edges follow the clean-up. A clean-up that + // fails, or that turns a valid result into an invalid one, is dropped: the raw result is the answer. + bool clean = false; }; struct BooleanResult { @@ -55,10 +59,10 @@ FuseHistoryResult fuse_shapes_with_history(const std::vector& shap TopoDS_Shape fuse_shapes(const std::vector& shapes, bool glue = false, std::optional tol = std::nullopt, bool clean = true); -// body - every tool. `body` itself when there are no tools. -TopoDS_Shape cut_shapes(const TopoDS_Shape& body, const std::vector& tools); +// body - every tool. `body` itself when there are no tools. With `clean`, the result is unified (see BooleanOptions). +TopoDS_Shape cut_shapes(const TopoDS_Shape& body, const std::vector& tools, bool clean = false); -// Intersection of all shapes, folded left to right. -TopoDS_Shape common_shapes(const std::vector& shapes); +// Intersection of all shapes, folded left to right. With `clean`, each step's result is unified. +TopoDS_Shape common_shapes(const std::vector& shapes, bool clean = false); } // namespace scad::kernel diff --git a/src/cpp/include/scad/ops/features/booleans.hpp b/src/cpp/include/scad/ops/features/booleans.hpp index 86e74811..c9c3d4a4 100644 --- a/src/cpp/include/scad/ops/features/booleans.hpp +++ b/src/cpp/include/scad/ops/features/booleans.hpp @@ -27,6 +27,9 @@ struct CutOptions { // cut_rsolid_list: order the pieces by centroid (x, then y, then z) instead of kernel order, so piece i is the same // piece on every OCCT version. Recorded as params.piece_order = "centroid" only when set. bool order_pieces = false; + // Unify the faces and edges the cut leaves split (a cylinder wall cut along its seam), as union does: OCCT fails to + // fillet such a split face, or returns an invalid solid. Recorded as params.clean. + bool clean = true; }; // What a cut that may split leaves: one Solid, or (`solid` empty) every piece, ordered by centroid. @@ -47,8 +50,8 @@ std::vector cut_rsolid_list(const std::vector& solids, const CutOp // cut_rsolid_list with order_pieces (recorded as make_cut_rsolid_list). An empty result is still an error. CutOutcome cut_rsolid_keep_split(const std::vector& solids, const CutOptions& options = {}); -// The overlap of all `solids`. Error "intersect_rsolid". -Solid intersect_rsolid(const std::vector& solids); +// The overlap of all `solids`; `clean` as for CutOptions. Error "intersect_rsolid". +Solid intersect_rsolid(const std::vector& solids, bool clean = true); // The error a non-Solid argument of union_rsolid / cut_rsolid / intersect_rsolid raises // (a binding that flattens Python arguments throws it for an item that is not a solid). diff --git a/src/cpp/include/scad/ops/features/diagnostics.hpp b/src/cpp/include/scad/ops/features/diagnostics.hpp index 2d966b7d..74568e17 100644 --- a/src/cpp/include/scad/ops/features/diagnostics.hpp +++ b/src/cpp/include/scad/ops/features/diagnostics.hpp @@ -21,7 +21,8 @@ namespace scad { // A classified fillet / chamfer failure. struct BlendDiagnosis { // "tangent_adjacent_faces" | "sliver_edges_in_selection" | "size_exceeds_face" | - // "partial_smooth_chain_on_periodic_face" | "blends_tangency_critical" | "vertex_blend_conflict" + // "partial_smooth_chain_on_periodic_face" | "blends_tangency_critical" | "vertex_blend_conflict" | + // "faces_split_on_one_surface" std::string failure_kind; std::string what_happened; std::vector possible_causes; @@ -54,6 +55,16 @@ class BlendVolumeViolation : public ValueError { void assert_blend_volume_monotonic(const Solid& source, const Solid& result, std::string_view operation_kind, std::optional size_value, const std::vector& edges); +// Exit check of a blend, next to the volume check: the result must be a valid solid (BRepCheck_Analyzer) whenever +// the input was. OCCT reports `IsDone` for fillets it got wrong (a face split along a seam, issue #88) and hands back +// an invalid solid with a wrong volume, which only a later boolean or an export finds. Throws BlendValidityViolation. +class BlendValidityViolation : public BlendVolumeViolation { + public: + using BlendVolumeViolation::BlendVolumeViolation; +}; +void assert_blend_valid(const Solid& source, const Solid& result, std::string_view operation_kind, + std::optional size_value, const std::vector& edges); + // A classified boolean failure. struct BooleanDiagnosis { std::string failure_kind; // "disjoint" | "non_manifold_contact" diff --git a/src/cpp/include/scad/semantic/tracking.hpp b/src/cpp/include/scad/semantic/tracking.hpp index dfde8f60..77ab47df 100644 --- a/src/cpp/include/scad/semantic/tracking.hpp +++ b/src/cpp/include/scad/semantic/tracking.hpp @@ -179,8 +179,9 @@ ModificationHistory modification_history(const BRepTools_History& history, const // tracked_cut. Face history of body ("body") and tool ("tool"), plus the // section edges. `solid` of the result is the *first* solid of the kernel's // result, empty when the cut left none; membership is judged against the whole -// kernel result. ValueError when the algorithm did not complete. -TrackedBooleanResult tracked_cut(const Solid& body, const Solid& tool); +// kernel result. ValueError when the algorithm did not complete. With `clean` the result is unified (same-domain +// faces and edges merged, see kernel::BooleanOptions::clean) and the history follows the clean-up. +TrackedBooleanResult tracked_cut(const Solid& body, const Solid& tool, bool clean = false); // tracked_union: a two-solid fuse with `track_union_history` on top. ValueError // unless the fuse yields exactly one solid. Glue is opt-in (see kernel::fuse_shapes_with_history). @@ -195,8 +196,8 @@ TrackedBooleanResult track_union_history(const std::vector& solids, const const BRepTools_History& history, const std::vector& section_edges = {}); -// tracked_intersect. -TrackedBooleanResult tracked_intersect(const Solid& body, const Solid& tool); +// tracked_intersect. `clean` as for tracked_cut. +TrackedBooleanResult tracked_intersect(const Solid& body, const Solid& tool, bool clean = false); namespace tracking { // How many topology-history queries the tracked operations have made in this process. A graph-policy diff --git a/src/cpp/src/kernel/booleans.cpp b/src/cpp/src/kernel/booleans.cpp index 46aff8eb..e62b6359 100644 --- a/src/cpp/src/kernel/booleans.cpp +++ b/src/cpp/src/kernel/booleans.cpp @@ -3,6 +3,7 @@ #include #include #include +#include #include #include #include @@ -53,6 +54,62 @@ std::vector tail_of(const std::vector& shapes) { return std::vector(shapes.begin() + 1, shapes.end()); } +bool is_valid_shape(const TopoDS_Shape& shape) { + return !shape.IsNull() && BRepCheck_Analyzer(shape).IsValid() == Standard_True; +} + +// Follow each section edge through a clean-up: it may have been merged into a longer edge or dropped with a removed +// face. Only the edges that are in `cleaned` survive, each once. +std::vector follow_section_edges(const opencascade::handle& clean_history, + const TopoDS_Shape& cleaned, + const std::vector& section_edges) { + SubShapeIndex result(cleaned); + TopTools_IndexedMapOfShape seen; + std::vector survivors; + for (const TopoDS_Shape& edge : section_edges) { + std::vector candidates; + for (const TopoDS_Shape& s : clean_history->Modified(edge)) + candidates.push_back(s); + for (const TopoDS_Shape& s : clean_history->Generated(edge)) + candidates.push_back(s); + if (candidates.empty() && !clean_history->IsRemoved(edge)) + candidates.push_back(edge); + for (const TopoDS_Shape& candidate : candidates) { + if (!result.contains(candidate)) + continue; + const int before = seen.Extent(); + seen.Add(candidate); + if (seen.Extent() > before) + survivors.push_back(candidate); + } + } + return survivors; +} + +// BooleanOptions::clean. The unified shape replaces the result only when it is as good: the same number of solids, and +// valid whenever the raw result was (FreeCAD and build123d fall back to the raw result the same way; unifying every +// boolean result has been seen to invalidate some). +void unify_result(BooleanResult& result) { + try { + ShapeUpgrade_UnifySameDomain unifier(result.shape, /*UnifyEdges=*/true, /*UnifyFaces=*/true, + /*ConcatBSplines=*/true); + unifier.Build(); + const TopoDS_Shape cleaned = unifier.Shape(); + if (cleaned.IsNull() || solids_of(cleaned).size() != solids_of(result.shape).size()) + return; + if (!is_valid_shape(cleaned) && is_valid_shape(result.shape)) + return; + if (!result.history.IsNull()) { + const opencascade::handle clean_history = unifier.History(); + result.history->Merge(clean_history); + result.section_edges = follow_section_edges(clean_history, cleaned, result.section_edges); + } + result.shape = cleaned; + } catch (const Standard_Failure&) { + // the raw result stands + } +} + } // namespace BooleanResult run_boolean(BooleanOp op, const TopoDS_Shape& argument, const std::vector& tools, @@ -86,6 +143,8 @@ BooleanResult run_boolean(BooleanOp op, const TopoDS_Shape& argument, const std: out.section_edges.clear(); } } + if (options.clean) + unify_result(out); return out; }); } @@ -138,29 +197,7 @@ FuseHistoryResult fuse_shapes_with_history(const std::vector& shap const opencascade::handle clean_history = unifier.History(); out.history->Merge(clean_history); - // Follow each section edge through the clean-up: it may have been merged - // into a longer edge or dropped with a removed face. - SubShapeIndex result(out.shape); - TopTools_IndexedMapOfShape seen; - std::vector survivors; - for (const TopoDS_Shape& edge : out.section_edges) { - std::vector candidates; - for (const TopoDS_Shape& s : clean_history->Modified(edge)) - candidates.push_back(s); - for (const TopoDS_Shape& s : clean_history->Generated(edge)) - candidates.push_back(s); - if (candidates.empty() && !clean_history->IsRemoved(edge)) - candidates.push_back(edge); - for (const TopoDS_Shape& candidate : candidates) { - if (!result.contains(candidate)) - continue; - const int before = seen.Extent(); - seen.Add(candidate); - if (seen.Extent() > before) - survivors.push_back(candidate); - } - } - out.section_edges = std::move(survivors); + out.section_edges = follow_section_edges(clean_history, out.shape, out.section_edges); return out; }); } @@ -182,21 +219,25 @@ TopoDS_Shape fuse_shapes(const std::vector& shapes, bool glue, std return clean ? clean_shape(fused.shape) : fused.shape; } -TopoDS_Shape cut_shapes(const TopoDS_Shape& body, const std::vector& tools) { +TopoDS_Shape cut_shapes(const TopoDS_Shape& body, const std::vector& tools, bool clean) { if (tools.empty()) return body; - const BooleanResult cut = run_boolean(BooleanOp::Cut, body, tools); + BooleanOptions options; + options.clean = clean; + const BooleanResult cut = run_boolean(BooleanOp::Cut, body, tools, options); if (!cut.done) throw ValueError("OCP cut failed"); return cut.shape; } -TopoDS_Shape common_shapes(const std::vector& shapes) { +TopoDS_Shape common_shapes(const std::vector& shapes, bool clean) { if (shapes.empty()) throw ValueError("common_shapes requires at least one shape"); TopoDS_Shape result = shapes.front(); for (std::size_t i = 1; i < shapes.size(); ++i) { - const BooleanResult common = run_boolean(BooleanOp::Common, result, {shapes[i]}); + BooleanOptions options; + options.clean = clean; + const BooleanResult common = run_boolean(BooleanOp::Common, result, {shapes[i]}, options); if (!common.done) throw ValueError("OCP common failed"); result = common.shape; diff --git a/src/cpp/src/ops/features/blend_diagnosis.cpp b/src/cpp/src/ops/features/blend_diagnosis.cpp index 6db55152..029e88a8 100644 --- a/src/cpp/src/ops/features/blend_diagnosis.cpp +++ b/src/cpp/src/ops/features/blend_diagnosis.cpp @@ -1,5 +1,13 @@ // Why a fillet or chamfer failed: the probes of operators/_diagnostics.py, in its order. #include +#include +#include +#include +#include +#include +#include +#include +#include #include #include @@ -153,6 +161,77 @@ std::optional classify_tangent_adjacency(const Case& c) { return std::nullopt; } +// A boolean that was not cleaned leaves faces that lie on one surface split (a cylinder wall cut along its seam is two +// faces). The blend of an edge along such a face fails, or goes wrong, and the room probe below then blames the radius: +// the "face room" it measures is only the half the seam cut off. Unify the same-domain faces, carry the selected edges +// over with the history, and blend again: when that works, the split faces are the cause (issue #88). +std::optional classify_split_faces(const Case& c) { + ShapeUpgrade_UnifySameDomain unifier(c.solid.wrapped(), /*UnifyEdges=*/true, /*UnifyFaces=*/true, + /*ConcatBSplines=*/true); + unifier.Build(); + const TopoDS_Shape unified = unifier.Shape(); + if (unified.IsNull()) + return std::nullopt; + const auto faces_in = [](const TopoDS_Shape& shape) { + TopTools_IndexedMapOfShape faces; + TopExp::MapShapes(shape, TopAbs_FACE, faces); + return faces.Extent(); + }; + const int split = faces_in(c.solid.wrapped()) - faces_in(unified); + if (split <= 0) + return std::nullopt; + + const opencascade::handle history = unifier.History(); + TopTools_MapOfShape carried; + std::vector edges; + for (const Edge& edge : c.edges) { + std::vector now; + for (const TopoDS_Shape& s : history->Modified(edge.wrapped())) + now.push_back(s); + if (now.empty() && !history->IsRemoved(edge.wrapped())) + now.push_back(edge.wrapped()); + for (const TopoDS_Shape& s : now) { + if (s.ShapeType() == TopAbs_EDGE && carried.Add(s)) + edges.push_back(TopoDS::Edge(s)); + } + } + if (edges.empty()) + return std::nullopt; + + const bool chamfer = c.words.size == "distance"; + const char* kind = chamfer ? "chamfer" : "fillet"; + bool works = false; + if (chamfer) { + BRepFilletAPI_MakeChamfer blend(unified); + for (const TopoDS_Edge& edge : edges) + blend.Add(c.size_value, edge); + blend.Build(); + works = blend.IsDone() && BRepCheck_Analyzer(blend.Shape()).IsValid() == Standard_True; + } else { + BRepFilletAPI_MakeFillet blend(unified); + for (const TopoDS_Edge& edge : edges) + blend.Add(c.size_value, edge); + blend.Build(); + works = blend.IsDone() && BRepCheck_Analyzer(blend.Shape()).IsValid() == Standard_True; + } + if (!works) + return std::nullopt; + + BlendDiagnosis out; + out.failure_kind = "faces_split_on_one_surface"; + out.what_happened = std::to_string(split) + " face(s) of the solid are split on a surface they share with a neighbour " + + "(typically a boolean made without clean, or a cylinder wall cut along its seam); the same " + kind + + " " + c.words.size + " " + g4(c.size_value) + + " mm succeeds once those faces are unified, so the " + c.words.size + " is not the limit"; + out.possible_causes = {"A cut or intersection ran with clean=False and left a face split along its seam.", + "The solid came from a source that does not unify faces (an imported or hand-built shape)."}; + out.measurements = {{"split_faces", static_cast(split), "faces"}, {c.words.size, c.size_value, "mm"}}; + out.repair = {"Make the cut / intersect with clean=True (the default), or pass the solid through union_rsolid(solid, " + "clean=True) before the " + std::string(kind) + ".", + std::string("Keep the ") + c.words.size + " as it is: it is not too large."}; + return out; +} + std::optional classify_blend_room(const Case& c) { for (const Edge& edge : c.edges) { const auto* faces = c.faces_of(edge); @@ -336,6 +415,8 @@ std::optional diagnose_blend_failure(const Solid& solid, const s return found; if (auto found = classify_tangent_adjacency(c)) return found; + if (auto found = classify_split_faces(c)) + return found; if (auto found = classify_blend_room(c)) return found; if (auto found = classify_partial_smooth_chain(c)) @@ -426,4 +507,20 @@ void assert_blend_volume_monotonic(const Solid& source, const Solid& result, std } } +void assert_blend_valid(const Solid& source, const Solid& result, std::string_view operation_kind, + std::optional size_value, const std::vector& edges) { + // The cheap side first: a valid result is the normal case; the input is checked only to tell "OCCT broke it" from + // "it came in broken" (an imported solid that was already invalid is not the blend's fault). + const bool result_valid = occt_guard([&] { return BRepCheck_Analyzer(result.wrapped()).IsValid() == Standard_True; }); + if (result_valid) + return; + const bool source_valid = occt_guard([&] { return BRepCheck_Analyzer(source.wrapped()).IsValid() == Standard_True; }); + if (!source_valid) + return; + throw BlendValidityViolation(std::string(operation_kind) + + " result is not a valid solid (BRepCheck_Analyzer) although the input was: the kernel " + "returned broken geometry (size=" + + size_text(size_value) + ", selected_edges=" + std::to_string(edges.size()) + ")"); +} + } // namespace scad diff --git a/src/cpp/src/ops/features/blends.cpp b/src/cpp/src/ops/features/blends.cpp index ed3b4877..2be7580c 100644 --- a/src/cpp/src/ops/features/blends.cpp +++ b/src/cpp/src/ops/features/blends.cpp @@ -52,7 +52,11 @@ struct BlendAttempt { spec.possible_causes = diagnosis->possible_causes; const std::string details = - violation != nullptr ? "BlendVolumeViolation: " + std::string(violation->what()) : technical_details_of(error); + violation != nullptr ? std::string(dynamic_cast(&error) != nullptr + ? "BlendValidityViolation: " + : "BlendVolumeViolation: ") + + violation->what() + : technical_details_of(error); std::vector evidence; if (diagnosis && !diagnosis->evidence_edges.empty()) { // The solid with the implicated edges in orange; the same failure at the same size is drawn once per process @@ -85,6 +89,7 @@ Solid finish_blend(const Solid& solid, TrackedResult tracked, const BlendAttempt double check_size, FeatureRecord& rec, const BlendTags& tags, std::string_view patch_role, const std::function& between = {}) { assert_blend_volume_monotonic(solid, Solid(tracked.shape), kind, check_size, attempt.edges); + assert_blend_valid(solid, Solid(tracked.shape), kind, check_size, attempt.edges); inherit_metadata(tracked.shape, {&solid}); rec.param("edge_count", Value(static_cast(attempt.edges.size()))); diff --git a/src/cpp/src/ops/features/booleans.cpp b/src/cpp/src/ops/features/booleans.cpp index 2359a761..b04bd7a5 100644 --- a/src/cpp/src/ops/features/booleans.cpp +++ b/src/cpp/src/ops/features/booleans.cpp @@ -323,12 +323,12 @@ CutRun run_cuts(const std::vector& solids, const CutOptions& options, Tra std::vector cut_pieces; if (policy == TrackingPolicy::Full) { - TrackedBooleanResult tracked = tracked_cut(piece, candidate); + TrackedBooleanResult tracked = tracked_cut(piece, candidate, options.clean); cut_pieces = std::move(tracked.solids); run.deltas.push_back(std::move(tracked.delta)); merge_entries(run.entries, std::move(tracked.delta_entries)); } else { - for (const TopoDS_Solid& part : kernel::solids_of(kernel::cut_shapes(s1, {s2}))) + for (const TopoDS_Solid& part : kernel::solids_of(kernel::cut_shapes(s1, {s2}, options.clean))) cut_pieces.emplace_back(part); } run.intermediates.push_back(piece); @@ -350,6 +350,7 @@ void record_cut(const std::vector& solids, const CutOptions& options, Tra FeatureRecord& rec) { rec.param("tool_count", Value(static_cast(solids.size() - 1))) .param("skip_non_intersecting", Value(options.skip_non_intersecting)) + .param("clean", Value(options.clean)) .param("tracking_policy", Value(std::string(to_string(policy)))); add_operands(rec, solids); } @@ -485,7 +486,7 @@ std::vector cut_rsolid_list(const std::vector& solids, const CutOp // --------------------------------------------------------------------------- // intersect // --------------------------------------------------------------------------- -Solid intersect_rsolid(const std::vector& solids) { +Solid intersect_rsolid(const std::vector& solids, bool clean) { bool empty_result = false; try { return occt_guard([&]() -> Solid { @@ -503,7 +504,7 @@ Solid intersect_rsolid(const std::vector& solids) { const Solid& candidate = solids[i]; if (result_solid.wrapped().IsNull() || candidate.wrapped().IsNull()) throw ValueError("The input solid is invalid; the intersection cannot be computed."); - TrackedBooleanResult tracked = tracked_intersect(result_solid, candidate); + TrackedBooleanResult tracked = tracked_intersect(result_solid, candidate, clean); if (!tracked.solid) { empty_result = true; throw ValueError("The intersection is empty or OCC returned no valid solid"); @@ -522,7 +523,7 @@ Solid intersect_rsolid(const std::vector& solids) { result_solid.apply_tag("solid.boolean.intersect", false); FeatureRecord rec(kIntersectOp); - rec.param("input_count", Value(static_cast(solids.size()))); + rec.param("input_count", Value(static_cast(solids.size()))).param("clean", Value(clean)); add_operands(rec, solids); FinalizeSpec spec; spec.record = std::move(rec).take(); diff --git a/src/cpp/src/ops/features/executors.cpp b/src/cpp/src/ops/features/executors.cpp index d3dcd53d..555fe075 100644 --- a/src/cpp/src/ops/features/executors.cpp +++ b/src/cpp/src/ops/features/executors.cpp @@ -154,17 +154,19 @@ void register_booleans(reactive::ExecutorRegistry& registry) { return one(union_rsolid(rest(in, 0, in.size()), options)); }); registry.add("make_cut_rsolid", [](Params p, Inputs in, const ScalarValues&) { - return one( - cut_rsolid(rest(in, 0, in.size()), {flag(p, "skip_non_intersecting"), text(p, "tracking_policy")})); + CutOptions options{flag(p, "skip_non_intersecting"), text(p, "tracking_policy")}; + options.clean = flag(p, "clean"); + return one(cut_rsolid(rest(in, 0, in.size()), options)); }); registry.add("make_cut_rsolid_list", [](Params p, Inputs in, const ScalarValues&) { CutOptions options{flag(p, "skip_non_intersecting"), text(p, "tracking_policy")}; + options.clean = flag(p, "clean"); const Value* order = p.find("piece_order"); // absent on records made before ordered pieces options.order_pieces = order != nullptr && order->is_string() && order->as_string() == "centroid"; return many(cut_rsolid_list(rest(in, 0, in.size()), options)); }); - registry.add("make_intersect_rsolid", [](Params, Inputs in, const ScalarValues&) { - return one(intersect_rsolid(rest(in, 0, in.size()))); + registry.add("make_intersect_rsolid", [](Params p, Inputs in, const ScalarValues&) { + return one(intersect_rsolid(rest(in, 0, in.size()), flag(p, "clean"))); }); registry.add("make_boolean_rsolidlist", [](Params p, Inputs in, const ScalarValues&) { const auto left = static_cast(param(p, "left_count").as_int()); diff --git a/src/cpp/src/semantic/tracking_booleans.cpp b/src/cpp/src/semantic/tracking_booleans.cpp index fa51049c..2e326b1d 100644 --- a/src/cpp/src/semantic/tracking_booleans.cpp +++ b/src/cpp/src/semantic/tracking_booleans.cpp @@ -54,10 +54,12 @@ std::string input_role(std::size_t index) { // body op tool, with history. `Solid(result)` of the Python SDK is the first // solid of the kernel's answer (empty when there is none); the history filter // is the whole answer. -TrackedBooleanResult track_boolean(kernel::BooleanOp op, const Solid& body, const Solid& tool, const char* failure) { +TrackedBooleanResult track_boolean(kernel::BooleanOp op, const Solid& body, const Solid& tool, const char* failure, + bool clean) { return occt_guard([&] { kernel::BooleanOptions options; options.fill_history = true; + options.clean = clean; const kernel::BooleanResult run = kernel::run_boolean(op, body.wrapped(), {tool.wrapped()}, options); if (!run.done) throw ValueError(failure); @@ -87,12 +89,13 @@ TrackedBooleanResult track_boolean(kernel::BooleanOp op, const Solid& body, cons } // namespace -TrackedBooleanResult tracked_cut(const Solid& body, const Solid& tool) { - return track_boolean(kernel::BooleanOp::Cut, body, tool, "Boolean cut failed: OCC build did not complete"); +TrackedBooleanResult tracked_cut(const Solid& body, const Solid& tool, bool clean) { + return track_boolean(kernel::BooleanOp::Cut, body, tool, "Boolean cut failed: OCC build did not complete", clean); } -TrackedBooleanResult tracked_intersect(const Solid& body, const Solid& tool) { - return track_boolean(kernel::BooleanOp::Common, body, tool, "Boolean intersect failed: OCC build did not complete"); +TrackedBooleanResult tracked_intersect(const Solid& body, const Solid& tool, bool clean) { + return track_boolean(kernel::BooleanOp::Common, body, tool, "Boolean intersect failed: OCC build did not complete", + clean); } TrackedBooleanResult tracked_union(const Solid& body, const Solid& tool, bool glue, double tol) { diff --git a/src/simplecadapi/_glue/docstrings.json b/src/simplecadapi/_glue/docstrings.json index 8024dc76..8cbf5358 100644 --- a/src/simplecadapi/_glue/docstrings.json +++ b/src/simplecadapi/_glue/docstrings.json @@ -145,7 +145,7 @@ "constrain_tangent_rsketch": "Constrain supported curves to be tangent on an explicit branch.\n\n``at_a``/``at_b`` select ``\"start\"`` or ``\"end\"`` for arc and\nB-spline endpoint tangency. Circle-circle tangency accepts\n``mode=\"external\"`` or ``mode=\"internal\"``.", "constrain_vertical_rsketch": "Constrain a sketch line to be vertical.", "convert_value": "Convert a finite numeric value between dimensionally compatible units.", - "cut_rsolid": "Compute the boolean difference of solids.\n\nArgs:\n solids: One or more Solid objects or sequences of Solid. Nested sequences are\n flattened before processing; the first solid is the base, the rest are\n subtracted in order.\n skip_non_intersecting: When True, tools with no meaningful intersection are\n ignored for interactive convenience. Graph replay records this flag and\n should use False for strict diagnostic workflows.\n tracking_policy: FULL computes topology history and lineage. GRAPH keeps\n the replayable operation node without computing a TopoDelta.\n on_split: What a cut that splits the body (a slot through a bar) returns.\n ``\"pieces\"`` (default): every piece, as ``list[Solid]`` ordered by centroid\n (x, then y, then z), recorded like ``cut_rsolid_list``. ``\"error\"``: raise,\n for code that needs exactly one Solid.\n\nReturns:\n Solid: The cut result solid, when the result is one piece.\n list[Solid]: The pieces (a ``scad.Pieces``), when the cut split the body\n (``on_split=\"pieces\"``). Operations that take one shape run on every piece\n (``cut_rsolid(pieces, tool)``, ``translate_shape(pieces, ...)``) and return\n ``Pieces``; a selector's cardinality is checked over all pieces. They are notebook values: a Part holds exactly one solid, so make one Part of each piece\n (an Assembly of them is the several-piece result), not ``make_part_rpart(body=pieces)``.\n\nRaises:\n SimpleCADError: The cut produced no solid, or (``on_split=\"error\"``) split\n the body. Use ``cut_rsolid_list`` to always get a list.\n\nUsage:\n Accepts a base solid followed by one or more tool solids, including nested\n sequences, and returns a single `Solid`.", + "cut_rsolid": "Compute the boolean difference of solids.\n\nArgs:\n solids: One or more Solid objects or sequences of Solid. Nested sequences are\n flattened before processing; the first solid is the base, the rest are\n subtracted in order.\n skip_non_intersecting: When True, tools with no meaningful intersection are\n ignored for interactive convenience. Graph replay records this flag and\n should use False for strict diagnostic workflows.\n tracking_policy: FULL computes topology history and lineage. GRAPH keeps\n the replayable operation node without computing a TopoDelta.\n clean: When True (the default) the faces and edges the cut leaves split are unified, as union does: a cylinder wall cut along its seam stays one face, so a later fillet or chamfer of its edges works. False keeps the raw kernel result.\n on_split: What a cut that splits the body (a slot through a bar) returns.\n ``\"pieces\"`` (default): every piece, as ``list[Solid]`` ordered by centroid\n (x, then y, then z), recorded like ``cut_rsolid_list``. ``\"error\"``: raise,\n for code that needs exactly one Solid.\n\nReturns:\n Solid: The cut result solid, when the result is one piece.\n list[Solid]: The pieces (a ``scad.Pieces``), when the cut split the body\n (``on_split=\"pieces\"``). Operations that take one shape run on every piece\n (``cut_rsolid(pieces, tool)``, ``translate_shape(pieces, ...)``) and return\n ``Pieces``; a selector's cardinality is checked over all pieces. They are notebook values: a Part holds exactly one solid, so make one Part of each piece\n (an Assembly of them is the several-piece result), not ``make_part_rpart(body=pieces)``.\n\nRaises:\n SimpleCADError: The cut produced no solid, or (``on_split=\"error\"``) split\n the body. Use ``cut_rsolid_list`` to always get a list.\n\nUsage:\n Accepts a base solid followed by one or more tool solids, including nested\n sequences, and returns a single `Solid`.", "diagnose_blend_failure": "Classify a fillet/chamfer failure; never raises.\n\nProbe order (cheapest explanation wins):\n\n1. Tangent adjacency — the two faces meeting at a selected edge are\n already tangent (dihedral ~180°), so the blend surface degenerates to\n zero width regardless of size.\n2. Room — blend size exceeds the distance from the edge to the opposite\n boundary of an adjacent face.\n3. Interaction — per-edge diagnostic rerun (``retry_single``, capped at\n 64 edges) isolates edges that fail alone; when every edge passes\n alone but the joint blend fails, opposite blends on a shared face\n meeting tangentially (2×size >= face room) is the explanation.", "diagnose_boolean_failure": "Classify a boolean failure from its operands; never raises.\n\nModel: operands within ``tol`` of each other form one connected cluster\n(transitively). More than one cluster means the union is genuinely\nseparated — report the nearest inter-cluster pair. A single cluster that\nstill failed to fuse contains a touch-without-overlap pair — report it as\nnon-manifold contact. Returns None when no pair explains the failure.", "diff_sketch_rsketchdiff": "Report what an edit did: entity and constraint ids created, removed or changed between two sketches.\n\nReturns a dict with ``created``, ``removed``, ``changed`` (entity ids, points included) and\n``created_constraints``, ``removed_constraints``, ``changed_constraints``. Not recorded in the graph.", @@ -180,7 +180,7 @@ "inspect_sketch_rsketchprofiles": "List the profiles a sketch offers: the indices ``make_face_from_sketch_rface`` and ``make_wire_from_sketch_rwire`` take.\n\nReturns ``{\"profiles\": [...], \"unusable\": [...]}``. Each profile is ``{index, id, kind, closed, entity_ids, point_ids, reversed}``: ``kind`` is ``circle``, ``edge_loop`` or ``edge_chain``; ``entity_ids`` are in walking order and ``reversed`` says which edges are walked from end to start. ``unusable`` words why an edge set is no profile (a branch point carrying more than two edges, for one). A free circle is a profile; a loop is a connected set of edges whose points each carry exactly two. Not recorded in the graph; pass ``strict=False`` for a sketch that does not solve cleanly.", "inspect_sketch_rsketchresult": "Inspect sketch constraints by running the solver without recording graph nodes.", "internal_tag_binding": "Create a source-marked binding for transitional internal annotations.", - "intersect_rsolid": "Compute the boolean intersection of solids.\n\nArgs:\n solids: One or more Solid objects or sequences of Solid. Nested sequences are\n flattened before processing.\n\nReturns:\n Solid: The overlap region as a single solid.\n\nUsage:\n Accepts one or more solids, including nested sequences, and returns a single\n `Solid`. If the inputs do not overlap meaningfully, the API raises a clear\n error instead of returning an empty list.", + "intersect_rsolid": "Compute the boolean intersection of solids.\n\nArgs:\n solids: One or more Solid objects or sequences of Solid. Nested sequences are\n flattened before processing.\n clean: When True (the default) the faces and edges the intersection leaves split are unified, as union does: a cylinder wall cut along its seam stays one face, so a later fillet or chamfer of its edges works. False keeps the raw kernel result.\n\nReturns:\n Solid: The overlap region as a single solid.\n\nUsage:\n Accepts one or more solids, including nested sequences, and returns a single\n `Solid`. If the inputs do not overlap meaningfully, the API raises a clear\n error instead of returning an empty list.", "is_normalized_tag": "Check whether a tag matches the normalized dot-token format.", "isolated_recording": "Run a nested definition build as if it were called at top level.\n\nThe caller's session is hidden (no active session, recording not\nsuspended) and the world coordinate system is current, so the nested\nbuild records only into its own session and does not depend on where it\nwas called from. Everything is restored on exit.", "legacy_tag_binding": "Import one flat compatibility token without inventing assignment intent.", @@ -347,7 +347,7 @@ "validate_placement_ticks": "Validate a persisted tick frame with exact integer arithmetic.", "value": "Build a value key extractor for ordering and projection in QL.\n\nArgs:\n path: Property or metadata path to resolve.\n default: Fallback value when the path is missing.\n\nReturns:\n Serializable key function for `Query.order_by(...)`.", "var": "Create a physical or legacy scalar variable.\n\n``tolerance=0.1`` declares a symmetric ``+/-0.1`` tolerance. Use a\n``(lower_deviation, upper_deviation)`` pair for an asymmetric tolerance.\n``tolerance_unit`` defaults to ``unit`` when a nominal unit is declared.\nValues are converted to canonical CAD units only when evaluated, so the\ndeclaration and serialized expression node preserve the user's units.\n\nVariables without ``unit`` retain legacy unitless behavior. A unit-aware\nexpression cannot mix declared-unit variables with legacy variables.", - "cut_rsolid_list": "Compute the boolean difference of solids and keep every piece.\n\nA cut can split the body (a slot through a bar leaves two parts) or consume it\n(a tool that encloses the whole body leaves nothing). ``cut_rsolid`` refuses\nsuch a result because it returns exactly one ``Solid``; this operator returns\nthem all. A tool is cut from every piece it meets, so several tools may split\na body into several parts.\n\nArgs:\n solids: One or more Solid objects or sequences of Solid. Nested sequences are\n flattened before processing; the first solid is the base, the rest are\n subtracted in order.\n skip_non_intersecting: When True, a tool that meets none of the current\n pieces is ignored; when False it raises.\n tracking_policy: FULL computes topology history and lineage. GRAPH keeps\n the replayable operation node without computing a TopoDelta.\n\nReturns:\n list[Solid]: Every piece of the cut, in kernel order; empty when the tools\n consume the base.\n\nRaises:\n SimpleCADError: An input is not a valid Solid, or the kernel could not\n compute the cut.\n\nUsage:\n ``pieces = cut_rsolid_list(bar, slot)`` returns both halves of a bar cut by\n a through slot. Use ``cut_rsolid`` when the result must be one solid.\n", + "cut_rsolid_list": "Compute the boolean difference of solids and keep every piece.\n\nA cut can split the body (a slot through a bar leaves two parts) or consume it\n(a tool that encloses the whole body leaves nothing). ``cut_rsolid`` refuses\nsuch a result because it returns exactly one ``Solid``; this operator returns\nthem all. A tool is cut from every piece it meets, so several tools may split\na body into several parts.\n\nArgs:\n solids: One or more Solid objects or sequences of Solid. Nested sequences are\n flattened before processing; the first solid is the base, the rest are\n subtracted in order.\n skip_non_intersecting: When True, a tool that meets none of the current\n pieces is ignored; when False it raises.\n tracking_policy: FULL computes topology history and lineage. GRAPH keeps\n the replayable operation node without computing a TopoDelta.\n clean: When True (the default) the faces and edges the cut leaves split are unified, as union does: a cylinder wall cut along its seam stays one face, so a later fillet or chamfer of its edges works. False keeps the raw kernel result.\n\nReturns:\n list[Solid]: Every piece of the cut, in kernel order; empty when the tools\n consume the base.\n\nRaises:\n SimpleCADError: An input is not a valid Solid, or the kernel could not\n compute the cut.\n\nUsage:\n ``pieces = cut_rsolid_list(bar, slot)`` returns both halves of a bar cut by\n a through slot. Use ``cut_rsolid`` when the result must be one solid.\n", "Body": "The solid of a Part, with its display name, colour and opacity.\n\n``Body(body_id, solid, name=None, color=None, opacity=None)``: ``body_id`` is an\nidentifier (``\"body\"`` for the body of a Part), ``solid`` the body's Solid, ``name`` an optional\ndisplay name, ``color`` an optional display colour (RGB floats in [0, 1]) and\n``opacity`` an optional display opacity in [0, 1]. A one-body Part has one Body with\nthe id ``\"body\"``. The colour overrides the Part's material colour for display only;\nset it on an existing Part with ``set_body_appearance_rpart``.", "measure_part_mass_rmassreport": "Measure the volume, mass and centre of mass of a Part.\n\nArgs:\n part: The Part to measure.\n density: Density to use instead of the material's; ``None`` uses the\n Part's material, and without a material density every mass is ``None``.\n density_unit: Unit of ``density``: ``kg/m^3`` (default), ``g/cm^3``,\n ``kg/mm^3`` or ``g/mm^3``.\n\nReturns:\n MassReport: ``bodies`` (the Part's one ``BodyMass``: ``body_id``, ``volume_mm3``,\n ``mass_kg``, ``centroid_mm``) and the totals of the part (``volume_mm3``,\n ``mass_kg``, ``centroid_mm``).\n\nRaises:\n SimpleCADError: The value is not a Part, or the density or its unit is invalid.\n\nUsage:\n ``scad.measure_part_mass_rmassreport(bracket).mass_kg`` is the mass of the part.\n The part's material gives the density unless ``density`` is passed.", "inspect_curve_rcurvedata": "Read the NURBS data of an edge's curve: what the curve edits work on.\n\nReturns ``{type, analytic, editable, conversion, degree, poles, weights, knots, multiplicities, periodic, rational, closed, domain, reversed, parameter_range}``. ``poles`` are points in the current coordinate system, indices are 0-based (the ``moves`` of ``move_curve_poles_redge``); ``parameter_range`` is the edge's own range (where ``insert_curve_knot_redge`` may insert). For a line, circle or other analytic curve ``editable`` is false, ``conversion`` is ``\"convert_to_bspline_redge\"`` and the data is what that conversion would produce. Not recorded in the graph.", diff --git a/src/simplecadapi/recording/serializer.py b/src/simplecadapi/recording/serializer.py index 7ba05f78..4898733f 100644 --- a/src/simplecadapi/recording/serializer.py +++ b/src/simplecadapi/recording/serializer.py @@ -4290,7 +4290,7 @@ def _vector(value: Any) -> Any: if op_name == "make_cut_rsolid": ctx.require_params( - node.node_id, op_name, params, ("tool_count",) + node.node_id, op_name, params, ("tool_count", "clean") ) if len(node.inputs) < 2: if ctx.strict: @@ -4320,13 +4320,14 @@ def _vector(value: Any) -> Any: ) ), tracking_policy=str(params.get("tracking_policy", "full")), + clean=bool(params.get("clean")), ) _store_outputs(node, result) continue if op_name == "make_cut_rsolid_list": ctx.require_params( - node.node_id, op_name, params, ("tool_count", "input_refs") + node.node_id, op_name, params, ("tool_count", "input_refs", "clean") ) inputs = _ordered_input_shapes(ctx, graph, outputs, node, params) if len(inputs) < 2: @@ -4340,6 +4341,7 @@ def _vector(value: Any) -> Any: _param(ctx, node.node_id, op_name, params, "skip_non_intersecting", False) ), "tracking_policy": str(params.get("tracking_policy", "full")), + "clean": bool(params.get("clean")), } if params.get("piece_order") == "centroid": # a split recorded by cut_rsolid(on_split="pieces"): the pieces in centroid order @@ -4405,14 +4407,16 @@ def _vector(value: Any) -> Any: if op_name == "make_intersect_rsolid": ctx.require_params( - node.node_id, op_name, params, ("input_count",) + node.node_id, op_name, params, ("input_count", "clean") ) all_solids = [ cast(Solid, shape) for shape in _all_input_outputs(ctx, outputs, node) ] if len(all_solids) >= 2: - result = ops.intersect_rsolid(all_solids[0], all_solids[1:]) + result = ops.intersect_rsolid( + all_solids[0], all_solids[1:], clean=bool(params.get("clean")) + ) _store_outputs(node, result) elif ctx.strict: ctx.fail( diff --git a/test/test_boolean_clean.py b/test/test_boolean_clean.py new file mode 100644 index 00000000..0c5844a0 --- /dev/null +++ b/test/test_boolean_clean.py @@ -0,0 +1,128 @@ +"""cut / intersect unify what they leave split, and a blend never hands back an invalid solid (issue #88). + +A cut through the side of a box with a cylinder splits the cylinder wall along the cylinder's parametric seam into two +faces, and the top edge of the notch into a long and a short arc. OCCT then fillets that top edge wrongly: it returns +an invalid solid with a volume that is too small (case A), or refuses and the diagnosis blames the radius (case B). +The same geometry fillets fine once the faces are unified, which is what union already did and cut did not. +""" + +from __future__ import annotations + +import unittest + +import simplecadapi as scad +from simplecadapi import ql +from simplecadapi._cpp.occt.BRepCheck import BRepCheck_Analyzer +from simplecadapi.errors import SimpleCADError +from simplecadapi.recording.graph import GraphSession + +# the issue's two placements of the cylinder; volumes are what CadQuery and build123d (which clean by default) give +CASE_A = (0.0, 17.5) +CASE_B = (-12.5, -17.5) +VOLUME_A = 29604.46 +VOLUME_B = 29811.31 + + +def notch(cx: float, cy: float, **options): + box = scad.make_box_rsolid(width=40, height=40, depth=20, bottom_face_center=(0, 0, 0)) + cylinder = scad.make_cylinder_rsolid(radius=10, height=40, bottom_face_center=(cx, cy, 10)) + return scad.cut_rsolid(box, cylinder, **options) + + +def top_edges(solid, z: float = 20.0): + return [ + edge + for edge in ql.edges().resolve(solid) + if abs(edge.get_start_vertex().get_coordinates()[2] - z) < 1e-6 + and abs(edge.get_end_vertex().get_coordinates()[2] - z) < 1e-6 + ] + + +def is_valid(solid) -> bool: + return bool(BRepCheck_Analyzer(solid.wrapped).IsValid()) + + +def face_count(solid) -> int: + return len(ql.faces().resolve(solid)) + + +class TestCutCleans(unittest.TestCase): + def test_cut_unifies_the_wall_the_seam_split(self): + self.assertEqual(face_count(notch(*CASE_A)), 8) + self.assertEqual(face_count(notch(*CASE_A, clean=False)), 9) + + def test_the_cleaned_cut_keeps_its_volume(self): + self.assertAlmostEqual(notch(*CASE_A).get_volume(), notch(*CASE_A, clean=False).get_volume(), places=3) + + def test_cut_list_and_intersect_clean_by_default_too(self): + box = scad.make_box_rsolid(width=40, height=40, depth=20, bottom_face_center=(0, 0, 0)) + cylinder = scad.make_cylinder_rsolid(radius=10, height=40, bottom_face_center=(0, 17.5, 10)) + (piece,) = scad.cut_rsolid_list(box, cylinder) + self.assertEqual(face_count(piece), 8) + (raw,) = scad.cut_rsolid_list(box, cylinder, clean=False) + self.assertEqual(face_count(raw), 9) + # the part of the box inside the cylinder: a cylinder wall cut along its seam is two faces unless unified + inside = scad.intersect_rsolid(box, cylinder) + raw_inside = scad.intersect_rsolid(box, cylinder, clean=False) + self.assertLess(face_count(inside), face_count(raw_inside)) + self.assertAlmostEqual(inside.get_volume(), raw_inside.get_volume(), places=3) + + +class TestFilletAfterCut(unittest.TestCase): + def test_case_a_fillets_to_a_valid_solid_with_the_right_volume(self): + cut = notch(*CASE_A) + filleted = scad.fillet_rsolid(solid=cut, edges=top_edges(cut), radius=3.0) + self.assertTrue(is_valid(filleted)) + self.assertAlmostEqual(filleted.get_volume(), VOLUME_A, delta=0.05) + + def test_case_b_fillets_with_the_radius_the_diagnosis_called_too_large(self): + cut = notch(*CASE_B) + filleted = scad.fillet_rsolid(solid=cut, edges=top_edges(cut), radius=3.0) + self.assertTrue(is_valid(filleted)) + self.assertAlmostEqual(filleted.get_volume(), VOLUME_B, delta=0.05) + + def test_an_unclean_cut_no_longer_yields_an_invalid_fillet(self): + """Case A without the clean-up: OCCT's fillet is invalid. It used to be returned; now it is an error.""" + cut = notch(*CASE_A, clean=False) + with self.assertRaises(SimpleCADError) as raised: + scad.fillet_rsolid(solid=cut, edges=top_edges(cut), radius=3.0) + self.assertIn("not a valid solid", str(raised.exception)) + + def test_an_unclean_cut_names_the_split_faces_not_the_radius(self): + """Case B without the clean-up: the kernel refuses, and the diagnosis says why instead of 'reduce the radius'.""" + cut = notch(*CASE_B, clean=False) + with self.assertRaises(SimpleCADError) as raised: + scad.fillet_rsolid(solid=cut, edges=top_edges(cut), radius=3.0) + message = str(raised.exception) + self.assertIn("split on a surface", message) + self.assertIn("clean=True", message) + self.assertNotIn("Reduce the radius", message) + + def test_a_radius_that_really_is_too_large_is_still_reported_as_such(self): + cut = notch(*CASE_A) + with self.assertRaises(SimpleCADError) as raised: + scad.fillet_rsolid(solid=cut, edges=top_edges(cut), radius=50.0) + self.assertNotIn("split on a surface", str(raised.exception)) + + +class TestRecordedClean(unittest.TestCase): + """``clean`` is a recorded parameter like ``skip_non_intersecting``; replay reads it, there is no default for a + record that lacks it.""" + + def record(self, **options): + with GraphSession() as session: + notch(*CASE_A, **options) + return session + + def test_the_flag_is_recorded(self): + self.assertTrue(self.record().graph.leaf_nodes()[0].params["clean"]) + self.assertFalse(self.record(clean=False).graph.leaf_nodes()[0].params["clean"]) + + def test_replay_reproduces_the_recorded_result(self): + for options, faces in (({}, 8), ({"clean": False}, 9)): + (replayed,) = scad.replay_graph(self.record(**options).graph) # the leaf: the cut + self.assertEqual(face_count(replayed), faces, options) + + +if __name__ == "__main__": + unittest.main() From ee5668c31ae5da80199e7458754654bf59531649 Mon Sep 17 00:00:00 2001 From: Ni Jingzhe <1448976235@qq.com> Date: Thu, 12 Nov 2026 04:06:38 +0800 Subject: [PATCH 2/2] test(cpp): follow the clean-up in the goldens, guard the diagnosis builders, format (CI of #88) DECISION: the roller-chain sprocket goldens and the C++ expectation move to the cleaned topology; they are not held at the old counts. The port is now deliberately different from the 2.2.6 reference on this point. WHAT CI SAID, AND WHY EACH ITEM IS RIGHT TO CHANGE - C++ doctest `std` (test_chain_bearing.cpp): the 12B sprocket has 39 faces, not 54. The sprocket is a body cut by tooth and roller profiles; cut now unifies the faces a cylinder seam had split. Volume, area, bounding box, tags are equal to the golden to 1e-6, so the geometry is the same and only the split faces are gone. - src/cpp/tests/golden/std_sprocket.json (faces 54 -> 39, edges 156 -> 111) and std_sprocket_unbored.json (35 -> 26 faces, 99 -> 72 edges): these files are recorded from the 2.2.6 Python reference SDK, which does not clean after a cut, and the suite refuses SCAD_REGOLDEN in CI, so they cannot be re-recorded there. I edited the four numbers by hand after comparing the port's whole output with the golden field by field: nothing else differs. Anyone who re-records from a reference that cleans will get the same numbers. - test_signatures_match_python: cut_rsolid has the keyword `clean` now. - OCCT guard audit: the trial blend of the new split-faces probe built BRepFilletAPI_Make* outside occt_guard. It runs under occt_guard now (a crash inside is a KernelError, which the diagnosis already swallows). - clang-format 18.1.8 (installed from pip into a scratch venv): blends.cpp and blend_diagnosis.cpp. CHECKED HERE: src/cpp/tests/python 556 passed (2 failures, test_locator_checks and test_failures, are Windows-only and did not fail in CI), check_occt_guard --strict 0, check_format.sh clean. The doctest executables were not built here; the CI run of this commit is the check for them. Co-Authored-By: Claude Sonnet 5.5 --- src/cpp/src/ops/features/blend_diagnosis.cpp | 41 ++++++++++--------- src/cpp/src/ops/features/blends.cpp | 10 ++--- src/cpp/tests/cpp/std/test_chain_bearing.cpp | 2 +- src/cpp/tests/golden/std_sprocket.json | 4 +- .../tests/golden/std_sprocket_unbored.json | 4 +- .../python/test_ops_features_booleans.py | 2 +- 6 files changed, 33 insertions(+), 30 deletions(-) diff --git a/src/cpp/src/ops/features/blend_diagnosis.cpp b/src/cpp/src/ops/features/blend_diagnosis.cpp index 029e88a8..85355abb 100644 --- a/src/cpp/src/ops/features/blend_diagnosis.cpp +++ b/src/cpp/src/ops/features/blend_diagnosis.cpp @@ -162,9 +162,9 @@ std::optional classify_tangent_adjacency(const Case& c) { } // A boolean that was not cleaned leaves faces that lie on one surface split (a cylinder wall cut along its seam is two -// faces). The blend of an edge along such a face fails, or goes wrong, and the room probe below then blames the radius: -// the "face room" it measures is only the half the seam cut off. Unify the same-domain faces, carry the selected edges -// over with the history, and blend again: when that works, the split faces are the cause (issue #88). +// faces). The blend of an edge along such a face fails, or goes wrong, and the room probe below then blames the +// radius: the "face room" it measures is only the half the seam cut off. Unify the same-domain faces, carry the +// selected edges over with the history, and blend again: when that works, the split faces are the cause (issue #88). std::optional classify_split_faces(const Case& c) { ShapeUpgrade_UnifySameDomain unifier(c.solid.wrapped(), /*UnifyEdges=*/true, /*UnifyFaces=*/true, /*ConcatBSplines=*/true); @@ -200,35 +200,38 @@ std::optional classify_split_faces(const Case& c) { const bool chamfer = c.words.size == "distance"; const char* kind = chamfer ? "chamfer" : "fillet"; - bool works = false; - if (chamfer) { - BRepFilletAPI_MakeChamfer blend(unified); - for (const TopoDS_Edge& edge : edges) - blend.Add(c.size_value, edge); - blend.Build(); - works = blend.IsDone() && BRepCheck_Analyzer(blend.Shape()).IsValid() == Standard_True; - } else { + const bool works = occt_guard([&] { + if (chamfer) { + BRepFilletAPI_MakeChamfer blend(unified); + for (const TopoDS_Edge& edge : edges) + blend.Add(c.size_value, edge); + blend.Build(); + return blend.IsDone() && BRepCheck_Analyzer(blend.Shape()).IsValid() == Standard_True; + } BRepFilletAPI_MakeFillet blend(unified); for (const TopoDS_Edge& edge : edges) blend.Add(c.size_value, edge); blend.Build(); - works = blend.IsDone() && BRepCheck_Analyzer(blend.Shape()).IsValid() == Standard_True; - } + return blend.IsDone() && BRepCheck_Analyzer(blend.Shape()).IsValid() == Standard_True; + }); if (!works) return std::nullopt; BlendDiagnosis out; out.failure_kind = "faces_split_on_one_surface"; - out.what_happened = std::to_string(split) + " face(s) of the solid are split on a surface they share with a neighbour " + - "(typically a boolean made without clean, or a cylinder wall cut along its seam); the same " + kind + - " " + c.words.size + " " + g4(c.size_value) + + out.what_happened = std::to_string(split) + + " face(s) of the solid are split on a surface they share with a neighbour " + + "(typically a boolean made without clean, or a cylinder wall cut along its seam); the same " + + kind + " " + c.words.size + " " + g4(c.size_value) + " mm succeeds once those faces are unified, so the " + c.words.size + " is not the limit"; out.possible_causes = {"A cut or intersection ran with clean=False and left a face split along its seam.", "The solid came from a source that does not unify faces (an imported or hand-built shape)."}; out.measurements = {{"split_faces", static_cast(split), "faces"}, {c.words.size, c.size_value, "mm"}}; - out.repair = {"Make the cut / intersect with clean=True (the default), or pass the solid through union_rsolid(solid, " - "clean=True) before the " + std::string(kind) + ".", - std::string("Keep the ") + c.words.size + " as it is: it is not too large."}; + out.repair = { + "Make the cut / intersect with clean=True (the default), or pass the solid through union_rsolid(solid, " + "clean=True) before the " + + std::string(kind) + ".", + std::string("Keep the ") + c.words.size + " as it is: it is not too large."}; return out; } diff --git a/src/cpp/src/ops/features/blends.cpp b/src/cpp/src/ops/features/blends.cpp index 2be7580c..276bae4c 100644 --- a/src/cpp/src/ops/features/blends.cpp +++ b/src/cpp/src/ops/features/blends.cpp @@ -52,11 +52,11 @@ struct BlendAttempt { spec.possible_causes = diagnosis->possible_causes; const std::string details = - violation != nullptr ? std::string(dynamic_cast(&error) != nullptr - ? "BlendValidityViolation: " - : "BlendVolumeViolation: ") + - violation->what() - : technical_details_of(error); + violation != nullptr + ? std::string(dynamic_cast(&error) != nullptr ? "BlendValidityViolation: " + : "BlendVolumeViolation: ") + + violation->what() + : technical_details_of(error); std::vector evidence; if (diagnosis && !diagnosis->evidence_edges.empty()) { // The solid with the implicated edges in orange; the same failure at the same size is drawn once per process diff --git a/src/cpp/tests/cpp/std/test_chain_bearing.cpp b/src/cpp/tests/cpp/std/test_chain_bearing.cpp index cfa728b4..8ed582a9 100644 --- a/src/cpp/tests/cpp/std/test_chain_bearing.cpp +++ b/src/cpp/tests/cpp/std/test_chain_bearing.cpp @@ -11,7 +11,7 @@ using doctest::Approx; TEST_CASE("sprocket: ISO 606 12B candidate") { const Solid sprocket = make_roller_chain_sprocket_rsolid(18, 19.05, 12.07, 11.1, 10.2); CHECK(sprocket.volume() == Approx(98201.67369173263).epsilon(1e-6)); - CHECK(sprocket.faces().size() == 54); + CHECK(sprocket.faces().size() == 39); // 54 before cut unified the faces a cylinder seam split (issue #88) const Value meta = sprocket.get_metadata("std.chain.roller_sprocket"); CHECK(meta.find("n_teeth")->as_int() == 18); CHECK(number(meta, "pitch_radius") == Approx(19.05 / (2.0 * std::sin(kPi / 18)))); diff --git a/src/cpp/tests/golden/std_sprocket.json b/src/cpp/tests/golden/std_sprocket.json index e703e28a..784c0a06 100644 --- a/src/cpp/tests/golden/std_sprocket.json +++ b/src/cpp/tests/golden/std_sprocket.json @@ -20,8 +20,8 @@ 59.38481593313661, 11.1 ], - "edges": 156, - "faces": 54, + "edges": 111, + "faces": 39, "tags": [ "solid.boolean.cut" ], diff --git a/src/cpp/tests/golden/std_sprocket_unbored.json b/src/cpp/tests/golden/std_sprocket_unbored.json index f305a0e0..d3b67db0 100644 --- a/src/cpp/tests/golden/std_sprocket_unbored.json +++ b/src/cpp/tests/golden/std_sprocket_unbored.json @@ -20,8 +20,8 @@ 27.35284634516466, 5.0 ], - "edges": 99, - "faces": 35, + "edges": 72, + "faces": 26, "tags": [ "solid.boolean.cut" ], diff --git a/src/cpp/tests/python/test_ops_features_booleans.py b/src/cpp/tests/python/test_ops_features_booleans.py index 8a5face3..03d27bb9 100644 --- a/src/cpp/tests/python/test_ops_features_booleans.py +++ b/src/cpp/tests/python/test_ops_features_booleans.py @@ -119,5 +119,5 @@ def info(face): def test_signatures_match_python(): assert parameter_names(scad.union_rsolid) == ["solids", "clean", "glue", "tol", "tracking_policy"] # *solids # on_split: a cut that splits the body returns its pieces by default (notebook values; a Part holds one solid), "error" refuses - assert parameter_names(scad.cut_rsolid) == ["solids", "skip_non_intersecting", "tracking_policy", "on_split"] + assert parameter_names(scad.cut_rsolid) == ["solids", "skip_non_intersecting", "tracking_policy", "on_split", "clean"] assert parameter_names(scad.boolean_rsolidlist) == ["left", "right", "operation"]