Repository navigation
fix: cut/intersect unify their result, and a blend never returns an invalid solid (#88) - #92
Merged
Merged
Conversation
…nd 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 <noreply@anthropic.com>
NiJingzhe
added a commit
that referenced
this pull request
Nov 11, 2026
… result, ProductInput) The committed API pages must equal what tools/auto_docs_gen.py writes (test_auto_docs_gen); the new parameter type of export_product_package_to_step/stl/obj/mjcf was not in them, which failed Python 3.10-3.13 in CI. The cut and intersect pages the generator also touched belong to #92 (they come from the native module installed here, which has that branch's signatures) and are left out. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…ilders, 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 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #88. The decisions, the rejected alternatives and what was not done are in the commit message.
fillet_rsolid/chamfer_rsolidnow raise when OCCT returns a solid thatBRepCheck_Analyzercalls invalid while the input was valid. Case A used to return one silently (volume 28305.59 instead of 29604.46).cut_rsolid,cut_rsolid_list,intersect_rsolidunify the same-domain faces and edges of their result (the history follows, so tags and lineage survive), falling back to the raw result if the unified one is invalid.clean=Falsekeeps the raw result. Recorded asparams.clean.cleanexisted is rejected like any missing parameter and has to be recorded again (the owner's rule: no old formats kept alive).Reproduced on the issue's two cases; both now give the volumes CadQuery gives (29604.46 and 29811.31). Python suite: 2674 passed; the 22 failures and 9 errors are environmental (Windows paths, locks, OCP absent, a math-library digit), none in booleans.
Watch in CI: the C++ unit tests were not run locally, and
CutOptions::cleandefaults to true.🤖 Generated with Claude Code