Classify Boolean edges where a tangent fillet surface folds onto the shell. Fixes #1291.

[BoykoNeov]

Fixes the remaining Boolean failures from #1291: the models from the issue comments where a fillet surface of the tool is tangent to the faces it rounds off — PrismFilletMinimalTestCase.slvs, PrismFilletMinimalTestCase_OnlyOneFailingEdge.slvs, PrismChamferMinimalTestCase.slvs (@ruevs), BoolBreakTangent.slvs (@drwho495), 1291_cube_cut.slvs — @phkahler's relocated original model with a true spline-face tangency — and @ruevs' further relocation of that model, which turned out to hit a false positive in the naked-edge checker rather than a Boolean defect (the fifth item below). (The cube_cut.slvs from the opening post is already fixed by #1729; as @phkahler pointed out, it was not saved in the failing position — the relocated versions needed two more fixes, the fourth and fifth defects below.)

Stacked on #1729 — the first three commits here are that PR's commits (rebased onto current master, which already has the AddExactIntersectionCurve() race fix). Only the last eight commits are new; I'll rebase this branch the moment #1729 merges so the diff shrinks to just them. Reviewing commit-by-commit works today.

What was still failing, and why

These models hit five independent defects:

1. SShell::ClassifyEdge() misclassified tangent folds (src/srf/raycast.cpp). Along the tangency line, the tool's flat cap and its fillet cylinder meet edge-on-edge, and both lie in the tangent plane of the surface being trimmed — both direction cosines are ~0. The old code assumed this meant "a flat face split in two pieces" and returned SURF_COINC_SAME/OPP for both sides of the edge. That's only right when both faces really are flat; here one of them curves away, so one side of the edge is genuinely inside or outside the shell. First-order data (normals at the edge) fundamentally cannot distinguish these — exactly @phkahler's "coincidence + curvature" hypothesis in the issue.

   The fix probes each shell face's actual surface a small distance in from the edge (scaled by the face's own control-net size), giving each face's deviation from the tangent plane. Each side of the edge is then classified by the nearest face extending to that side:

   • it stays in the tangent plane → coincident, using that face's normal (the old code used face 0's normal for both sides — wrong when a checkerboard-coincident split puts oppositely-oriented faces on the two sides);
   • it curves away from the tangent plane → inside or outside, by which way it curves relative to our outward normal;
   • no face extends to that side, i.e. both faces fold back onto the other side (a zero-angle wedge) → outside if the wedge between them is material, inside if it's a void slot, decided by which of the two faces is nearer the tangent plane.

   For the ordinary flat split face the new code reproduces the old answers exactly.

2. Tangent intersection curves got an arbitrary edge direction (src/srf/boolean.cpp, MakeCopyTrimAgainst). New intersection edges are oriented by the triple product (tn × d) · sn, which is ~0 when the two surfaces are tangent along the curve — the sign was numerical noise. Since KeepEdge() keeps only correctly-oriented edges, even correctly classified tangent edges were then discarded, leaving holes. Degenerate cases now add the edge in both directions; classification keeps the right one and CullExtraneousEdges() drops the leftover parallel duplicate.

3. The edge-classification probe distance wasn't local (src/srf/boolean.cpp, EdgeNormalsWithinSurface). The probe point is offset from the edge midpoint by SS.ChordTolMm() — 1 mm by default, which on these ~0.5 mm-radius models is far outside the edge's neighborhood. Evaluating the fillet's arc patch that far past its end curves back (rational quadratics do), misclassifying a perfectly transversal edge — but only at export/regenerate chord tolerances, which is why OnlyOneFailingEdge failed on export while looking fine on screen. The probe is now clamped to a tenth of the edge's length.

4. Tangent plane–extrusion intersection lines were never generated (src/srf/surfinter.cpp, IntersectAgainst). This is what @phkahler's relocated 1291_cube_cut.slvs still hit after the three fixes above, and it sits upstream of them: fixes 1–2 classify and orient tangent intersection edges correctly, but only once the intersection curve exists. The plane-vs-parallel-extrusion branch finds the intersection lines by casting a probe line within the plane against the surface numerically, and PointIntersectingLine() cannot converge on a grazing (tangent) intersection — the plane-line step degenerates exactly at the solution. A cylinder never gets there (closed-form intersection with an explicit tangency case), which is why the arc-profile fillets above worked while the relocated model's cubic-spline profile failed: its tangent line silently didn't exist, and the cube face's trim polygon was left open.

   Such a tangency happens where the extruded profile ends on the plane, so the missing line is exactly the trim curve the extruded surface shares with its neighbour there — and that curve lies entirely in the plane. The fix adds those trim curves as exact intersection curves (the same way the general branch already handles exact curves lying in a plane, from d72eba8) and skips numerical probe hits that duplicate a curve added exactly. Downstream, fixes 1–2 then keep exactly the right directed edge with no further changes.

   Known limitation: a tangency in the interior of the extruded profile (a plane grazing the middle of a spline, where no trim curve exists) would still be missed; that needs a root finder that is robust at grazing intersections. Fillet-style tangencies are at profile endpoints by construction, and no known model hits the interior case.

5. The self-intersection checker flagged exact edge-on-face grazes (src/mesh.cpp, SKdNode::FindEdgeOn). This is what @ruevs' second relocated 1291_cube_cut hit, and it is not a Boolean defect at all. That drag moved the profile's free corner inside the cube, so the tool touches the cube's side face only along the sweep line through the other corner — which is constrained onto the cube's edge, so the line lies exactly in the face. The face is correctly left uncut: the grazing line dangles into the face's interior at the tool's cap, so no valid trim polygon exists to split it with, and the Boolean output is right — its volume matches the mesh-Boolean (forceToMesh=1) ground truth to ~1e-10 at every position along the whole drag path. But the strict mode of FindEdgeOn (used by Analyze → Show Naked Edges and by every mesh export) reported a self-intersection whenever a face-triangulation edge happened to cross a tool triangle's plane exactly on that triangle's grazing edge. Whether any of the face's triangulation diagonals cross within the grazing edge's span is per-position, per-machine floating-point luck — which is why the model fails on one machine and passes on another, independent of chord tolerance and of everything upstream.

   The relaxed mode of the checker already forgave intersections landing on a triangle's edge; the fix extends that forgiveness to both modes and removes the now-meaningless coplanarIsInter flag. No real intersection can be lost: an edge that actually passes into the mesh at a triangle's edge also passes through the interior of the triangles on the far side of that edge, and is reported there; and a coplanar overlap always puts a crossing point strictly inside some triangle. Grazes of this kind are inherent to correct Boolean output whenever a tool face ends exactly on a shell face, so the tangency models in this PR were all exposed to this false positive.

Regression tests

• test/group/boolean_tangent_fillet/ — @ruevs' PrismFilletMinimalTestCase.slvs verbatim, checking the display mesh is watertight (no naked or self-intersecting edges) and the volume matches. Fails with naked edges without fixes 1–3, passes after. Volume rather than image checks, as before.
• test/group/boolean_tangent_spline/ — @phkahler's relocated 1291_cube_cut.slvs verbatim, same watertight + volume checks. Fails with naked edges without fix 4, passes after.
• test/group/boolean_grazing_edge/ — @ruevs' second relocated model (canonically re-saved), same watertight + volume checks. Fails "self-intersecting" without fix 5 on machines where the triangulation lands unluckily.
• test/core/mesh/ — a deterministic unit pair for fix 5, independent of triangulation luck: a triangle edge lying in another triangle's interior (a graze) must not be reported as a self-intersection, while an edge passing through another triangle's interior still must be.

Verification

• All eight models from the issue thread (cube_cut, both relocated 1291_cube_cuts, PrismFillet, OnlyOneFailingEdge, PrismChamfer, BoolBreakTangent, plus the working BoolWorks control) export watertight, with volumes matching the mesh-Boolean ground truth of the same models (forceToMesh=1) — the relocated models to the last digit.
• For fix 5, the relocated corner was additionally swept along its whole drag path (201 positions) and the dragged control point over a surrounding grid: the Boolean volume matches the mesh ground truth everywhere, and the naked-edge/self-intersection check now passes at every position (before the fix it failed at a machine-dependent band of positions).
• Full test suite passes in Debug and Release (264 cases / 928 checks), including the NURBS failure when ClassifyEdge fails (was: Difference fail depending on compilation options (was: Difference fail with OpenMP)) #1619/NURBS failure with countersunk holes #1091 regression tests from Fix NURBS boolean failures from unhandled edge-on-edge classification (fixes #1619, part of #1091) #1729.
• Repeated runs with OpenMP produce identical triangle sets.

🤖 Generated with Claude Code

[ruevs]

One easily gets used to good things - two clean and small commits (plus the test case in a separate one) - and the bug is fixed:

@jwesthues please take a look at these if you have time. I don't want to merge them without your blessing.

@phkahler since this is on top of #1729; once we decide to start merging the correct sequence now is #1729, #1731 (this) and then #1730

[phkahler]

@ruevs doesn't this include the commits from #1729? Could we just merge this one and drop that one?

I'm eager to merge these and have Claude solve one more long standing NURBS issue before time or tokens expire.

[phkahler]

Unfortunately this doesn't fix the original problem in #1291. Over in that issue I reattached the original problem file but moved into the position that causes NURBS failure. The original was to play with the issue, but I suppose I should have uploaded it in the failing position.

I would still accept this PR since it fixes some related scenarios and moves closer to a complete solution.

[ruevs]

@ruevs doesn't this include the commits from #1729? Could we just merge this one and drop that one?

Yes we can; and since this is already rebased on master I can (locally) --ff-only merge it - when it is time. #1729 will appear "closed with unmerged commits" but that is not a problem at all.

I'm eager to merge these and have Claude solve one more long standing NURBS issue before time or tokens expire.

Well; I see that @BoykoNeov just pushed two more commits working on these: 1, 2, 3. I'll try it now.

[BoykoNeov]

Unfortunately this doesn't fix the original problem in #1291. Over in that issue I reattached the original problem file but moved into the position that causes NURBS failure. The original was to play with the issue, but I suppose I should have uploaded it in the failing position.

I would still accept this PR since it fixes some related scenarios and moves closer to a complete solution.

The machines may work for us, but now I work for it (as his messenger) this doesn't go somewhere nice ;)

That is its reply anyway:

@phkahler Thanks — confirmed, and fixed now (d5e8167d + regression test bbb55707).

Your relocated 1291_cube_cut.slvs was failing one level upstream of everything this PR fixed so far: the tangent intersection line between the cube face and the spline surface was never generated at all, so the classification and edge-orientation fixes never got a chance to act. The plane-vs-extrusion intersection (extrusion parallel to the plane) finds its lines by casting a probe line within the plane against the surface numerically, and PointIntersectingLine() cannot converge on a grazing intersection — the plane–line step degenerates exactly at the solution (that's the parallel (surface intersecting line) spam on this model). A cylinder never takes that path: it's intersected in closed form with an explicit tangency case. That's why the arc-profile fillet models in this PR worked while the relocated model's cubic-spline profile failed.

The fix: such a tangency happens where the extruded profile ends on the plane, so the missing line is exactly the trim curve the extruded surface shares with its neighbour there — and that curve lies entirely in the plane. The parallel-extrusion branch now adds those trim curves as exact intersection curves (the same thing the general branch has done since d72eba8), and skips numerical probe hits that duplicate a curve added exactly. Downstream, the tangent-edge machinery from this PR keeps exactly the right directed edge with no further changes. Your model now exports watertight, matching its own forceToMesh=1 ground truth to the last digit (203667.6903 mm³), and is added verbatim as test/group/boolean_tangent_spline/ — it fails with naked edges without the fix.

One known limitation, deliberately left out of this PR: a tangency in the interior of the profile — the plane grazing the middle of a spline, where no trim curve exists. It's not unsolvable; with the extrusion parallel to the plane the problem is one-dimensional: f(t) = n·B(t) − d along the profile, and a tangency is a grazing double root (f = f′ = 0), which a root finder on f′ locates robustly where Newton on f alone degenerates (for a non-rational cubic profile it's even closed-form). I left it out because no known model exercises it to validate against — fillet-style tangencies are at profile endpoints by construction. If such a model turns up, that's the shape of the fix.

[ruevs]

The machines may work for us, but now I work for it (as his messenger) this doesn't go somewhere nice ;)

Indeed :-/ This is the philosophically/existentially most important comment in this PR :-)

[ruevs]

he machines may work for us, but now I work for it (as his messenger) this doesn't go somewhere nice ;)

That is its reply anyway:

@phkahler Thanks — confirmed, and fixed now (d5e8167d + regression test bbb55707).

And I work as it's "double checker" (not even tester, because it already tested it and added a test case :-)

And it works:

This PR is very, very, very impressive.

It is still possible to cause a failure with the bezier by very slowly and carefully moving the point I marked with and X:

Since this is actually hard to reproduce I am attaching and updated test case:
1291_cube_cut_StillFailsAfter_1731_With_bbb55707c46b297dd24afe4d2e23d30e58c15221.zip

My only advantage to the LLM here is that I can move the model in real time and visually inspect the result. But I see that it also "iteratively animated" a .slvs file and exported it with the CLI while working on this PR, so even this is not much of an advantage...

By the way I'm pretty sure this last failure goes pretty deep in the "numerical weeds" but it does not depend on the chord tolerance.

[BoykoNeov]

@ruevs , the game continues:

@ruevs Thanks — reproduced, diagnosed, and fixed; pushed as d7bff4c + 6a943f4. The interesting part: on this model the Boolean was never wrong. What you were seeing is a false positive in the naked-edge/self-intersection checker, and whether it fires is floating-point luck — which is exactly why it fails on your machine, passed on mine at your saved position, and doesn't respond to chord tolerance.

What your drag changed. Moving the marked control point pulled the profile's free corner inside the cube (it used to poke out through the side face). The other corner of the profile is constrained onto the cube's edge, so after the drag the tool touches the cube's side face only along the sweep line through that corner — a line lying exactly in the face, with the rest of the tool strictly inside. The face is correctly left uncut: that grazing line dangles into the face's interior at the tool's cap, so there is no closed trim polygon that could split the face there. The result is a legitimate T-junction — two tool surfaces ending edge-on in the interior of an uncut face. The mesh Boolean produces the same touch, and the NURBS volume matches the forceToMesh=1 ground truth to ~1e-10 at every point along your whole drag path (I swept it at 201 positions).

Why it "fails" anyway, sometimes. SKdNode::FindEdgeOn in its strict mode (used by Analyze → Show Naked Edges and by every mesh export) flagged any triangle edge that crosses another triangle's plane exactly on that triangle's edge. At the graze, the cube face's triangulation diagonals cross the tool triangles' planes precisely on the grazing edge — if a diagonal happens to land within that edge's span. Whether it does depends on how the face gets triangulated, which varies with the exact coordinates and the machine. On my machine your saved position passes but a nearby band of the same drag fails; on yours it's the reverse. Classic razor edge.

The fix. The checker's relaxed mode already forgave intersections landing on a triangle's edge; d7bff4c extends that to both modes (and removes the then-meaningless coplanarIsInter flag). No real self-intersection can be lost by this: an edge that actually passes into the mesh at a triangle's edge also passes through the interior of the triangles on the far side of that edge, and is caught there; a coplanar overlap always puts a crossing strictly inside some triangle. Since this kind of graze is inherent to correct Boolean output whenever a tool face ends exactly on a shell face, all the tangency models in this PR were exposed to the same false positive — so this also makes the PR's other regression tests robust across machines.

Tests (6a943f4): your model as test/group/boolean_grazing_edge/ (watertight + volume), plus a deterministic unit pair in test/core/mesh/ that doesn't depend on triangulation luck — an edge lying in another triangle's interior must not be reported, an edge passing through one still must be.

Verified: your file and the 201-position drag sweep all clean (volume matching mesh ground truth everywhere), a grid of positions of the dragged control point clean, all earlier models from this thread and #1619/#1091 unchanged, full suite 264 cases / 928 checks in Debug and Release.

[phkahler]

I don't think I like the solution to 1291. It's adding a special case to code that intersects an extrusion and a plane. It's not wrong, but my issue is that the extruded fillet already has that edge. A more correct solution would include that edge because its in-plane. In fact I added that feature some time ago to fix another issue. Either that edge isn't being included or it's getting improperly discarded.

I once posted a model showing this person how to make a filleted hole work:
#738 (comment)

Not sure where it went, but I still have the workaround model and if fails these days. But the above fix will not handle that model because the in-plane curve is not part of an extrusion.

Anyway, I'd take this fix because it's small and solves some common problems (assuming it doesn't cause regressions) but I'd flag it as potentially redundant solution that might interfere when other similar issues are fixed.

Also, does the existing fix work in you make the corner cut double sided? In other words, will it clip the curve against the top of the cube?

[ruevs]

Unfortunately d7bff4c did not fully fix the "dragging problem". However Claude Fable 5 is probably on to something, since re-opening the broken looking model "fixes" the problem. Or even switching to the previous group (g0004-sketch-fillet) and then switching back to g0005-cut "fixes" it.

Her is a video:

1731_Drag_x264.mp4

So we should probably not merge the last two commits. But even so this is very, very impressive. Thank you @BoykoNeov

[phkahler]

@ruevs yes, those blinking red failures are a long standing problem with a lot of different boolean operations. Sometimes they'll even show up in a fully constrained model, and then trying unsuccessfully to drag a point makes them go away. They've always seemed like numeric rather than algorithmic problems to me.

I think we should look at those two commits and decide if they are good or not based on the implementation. There could be any number of causes for the red flashing and maybe these commits actually reduce the likelihood?

[phkahler]

@ruevs Yeah, I think rejecting those last two is a good idea. It looks like those tests are to see if a mesh intersects another mesh, but in this case there should only be one mesh assuming the NURBS boolean operation completed correctly. So it's confusing me more than making sense. (for now anyway).

[jwesthues]

I'm also quite impressed by Claude's performance here. The issues had good context and it incorporated that, but my incomplete review finds significant new and apparently correct geometric reasoning. I wonder if it's applying patterns it saw in other geometric code (elsewhere in SolveSpace, OpenCASCADE, what else does this?), or whether we're benefiting from the training they do for research mathematics, or something else.

In general the changes seem to be mostly to cases that were previously unhandled since I'd made no consideration for tangency, handling dot < 0 and dot > 0 but not dot = 0 etc. So I don't think they're likely to break anything that deliberately worked before. My initial human authorship of these routines was a combination of imperfect theoretical understanding and experiment, so I don't think we're losing much by letting a machine do the same, especially when the change is small like 2534a5a or 010d0de. I feel more nervous when the changes are larger, since that makes it easier to overfit your way to passing tests (regardless of whether it's a human or machine iterating). For stuff like 4ce92bb and 1615453 it would be nice to either get some human understanding or greatly increase the test coverage.

As to LLM use more broadly, I wonder if we should apply for Anthropic's open source program. The project isn't huge, but perhaps they'd appreciate the mathematical novelty of the application. I believe that most NURBS operations are easier to test programmatically than to implement, to the extent they have simple implicit representations. For example, as long as we trust our point-in-shell test, we can test a Boolean union by testing many points against the input shells and output shell to confirm they satisfy inOut = inOp1 | inOp2. I think unimplemented operations like rounding or offsetting can also be put in this form. This would let the agent grind autonomously until the operation is correct (or the token budget runs out).

[phkahler]

@ruevs of anyone really. How do we merge the first 9 commits in this PR but not the last two?

After that we'll go merge #1730.

This PR includes commits from #1729 so we can close that unmerged.

[ruevs]

@ruevs of anyone really. How do we merge the first 9 commits in this PR but not the last two?

@phkahler

git checkout master
git merge --ff-only bbb55707c
git push origin  // or whatever your upstream remote is called

I've done it on my fork. Do you want me to push it upstream now?

[phkahler]

I've done it on my fork. Do you want me to push it upstream now?

Yes please! Assuming you got the first 9 that way.

[ruevs]

I've done it on my fork. Do you want me to push it upstream now?

Yes please! Assuming you got the first 9 that way.

@phkahler done. The fist nine are in master - with the exact same hashes as here - I did a fast forward merge. Because of this this PR remains clean and GitHub still considers it unmerged.

#1730 must now be rebased before it can be merged. I'll do it now, because I'll disappear offline for the next five days.

[ruevs]

#1730 must now be rebased before it can be merged. I'll do it now, because I'll disappear offline for the next five days.

Done #1730 is now merged. The last two commits from here remain unmerged - I think this branch should stay in case we want to refer to them.