Classify a tangent face that meets our surface edge-on beside a crossing face. Fixes the tangent-outside case from #1291 - #5
Conversation
…ing face. SShell::ClassifyEdge()'s edge-on-edge case, where exactly two faces of the shell meet along the edge being classified, treats a face whose normal is parallel to our surface's normal at p as coincident with our surface. But such a face may instead be merely tangent to our surface at the shell's edge, curving away from it past that edge; the normals at p cannot tell those apart. The both-parallel branch already probes each face's geometry a little way in from the shell's edge to distinguish them, but the mixed branches--one face parallel, the other crossing--did not, and reported the side of our edge towards the tangent face as coincident regardless. That side is then neither kept as material nor cut away, so the edge is dropped from the trim and the whole face fails to assemble. It happens where the spline face of a cut ends tangent to the face it cuts through while the cut's neighbouring face crosses that same face at the tangency, so that the cut's material fills the region behind the tangent face (issue solvespace#1291): the cube face there is lost, leaving six naked edges. Probe the tangent face the same way the both-parallel branch does, and if it curves away rather than lying flat, classify that side as inside or outside the shell accordingly. Share the probe between the two branches. Fixes the tangent-outside case from solvespace#1291. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ing face. The model is ruevs' cube_cut with the profile spline made tangent to the cube's face on the other side, its corner left outside the cube so that the line continuing the profile from the tangency crosses that face. Without the classification fix the cube's face fails to assemble and the shell leaks; the test checks that the result is watertight and has the volume of the exact Boolean. Like the other Boolean cases, it deliberately has no CHECK_SAVE or CHECK_RENDER: the surface cache holds last-ulp floats that PrepareSavefile does not normalize. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
I have added a new issue in the upstream solvespace repository here: This offers another failing variant of the 1291 model along with my proposed explanation for why it fails, and another model that I believe suffers from the same failure mode. |
|
The fix works: solvespace#1291 (comment) |
|
Here solvespace#1743 (comment) is a simplified reproduction model for "@phkahler's two-sided-extrusion case." It is interesting that if the |
|
Written by Claude Opus 5 — both this text and the code it describes; posted by @BoykoNeov. @ruevs Thanks for confirming, and @phkahler thanks for solvespace#1743 — I've read it along with the simplified model. I agree it is a different defect, and that it sits upstream of everything this PR touches. What this PR changes is which edges survive classification; solvespace#1743 is about an edge that is never created in the first place. Your description matches that: the box edge is tangent to both fillet faces where the fillet's sharp edge lands on it, so no surface-crossing is found there, and the vertex that So this PR stands on its own: it fixes the tangent-outside case (which was a generated-then-discarded curve), and it neither fixes nor obstructs the split failure. Worth noting that your two models make a good pair for whoever picks solvespace#1743 up — The difference-vs-intersection asymmetry @ruevs noticed is interesting and I would not want to explain it from the armchair — the two operations keep opposite sides, so it may only mean the missing split lands on the discarded side in one of them, or it may be pointing at something sharper. |
|
Written by Claude Opus 5 — both this text and the code it describes; posted by @BoykoNeov. Correction to my comment above. I wrote that this PR "neither fixes nor obstructs" the split failure in solvespace#1743. The second half holds, the first is too flat: @ruevs' simplified model does move on this branch — from 10 naked edges down to 6, deterministically, on master and on this branch alike. A maintainer re-running it would see the difference and reasonably wonder what I had measured. What actually happens is that model needs two fixes, one of which is this one:
With both applied the simplified model is clean: 28 triangles, 0 naked edges, volume matching the mesh-Boolean value. They are independent, which I checked rather than assumed. My original point survives: the two defects are in different places — this one is about which edges survive classification, #6 is about a curve that was never split. But "changes nothing" was the wrong summary for the simplified model, and I would rather say so than have someone find it by running it. |
|
@BoykoNeov can you make a pull request for this to the upstream solvespace repository? This PR want to merge to the master on your fork rather than the master upstream. |
|
@BoykoNeov never mind. I merged this to solvespace/master. It might be good to close this one. |
3 participants
Fixes the tangent-outside case from solvespace#1291 — @ruevs'
1291_cube_cut_tangent_outside_still_fails.slvs, which still failed after the nine commits of solvespace#1731 that are now in master.This continues that work and sits directly on current master. It does not include the two commits from solvespace#1731 that you decided against (the
SKdNode::FindEdgeOnrelaxation and its tests) — this fix is independent of them, and I rebased it off them so there is nothing to re-litigate here.Both of the obvious explanations are wrong
I had two candidates going in, and I want to record that they're dead so nobody spends time on them:
ctrl[2] = (30, −12.2621)andctrl[3] = (30, 18.7884), both atx = 30, so the tangency is at the bezier's endpoint, and the exact-in-plane path from that PR fires correctly:EXACT-IN-PLANE (30, 18.7884, 30) → (30, 18.7884, 0).The curve is generated correctly and then thrown away by misclassification. It reaches
MakeCopyTrimAgainstas three chains, all withKEEP = 0(ins = COINC_OPP, outs = OUTSIDEin one direction, reversed in the other). The cube'sx = +30face is left with five edges,AssemblePolygonfails withfailed: I=5, avoid=4, and the face disappears from the output.Root cause
In
SShell::ClassifyEdge()'s edge-on-edge case (src/srf/raycast.cpp), theedge_inters == 2mixed branches — where one face's direction cosine is ~0 and the other's is not — assume that a face whose normal is parallel to ours is coincident with our surface. It may instead be merely tangent at the shell's edge and curving away immediately. First-order data at the point of contact cannot tell those apart.This is the same class of defect as the both-tangent branch fixed in solvespace#1731, in the branches that PR left alone. I flagged it there as out of scope for lack of a model that reached it; @ruevs' file is that model.
The fix
Extract the both-tangent branch's existing geometry probe as
ProbeTangentFace()and apply it to the mixed branches too: probe the parallel-normal face a short distance into itself (scaled by that face's own control-net size) and see whether it stays in the tangent plane or leaves it. If it curves away, that side is classifiedSURF_INSIDE/SURF_OUTSIDEby the sign ofdev * surf_n·inter_surf_nrather than being called coincident.Faces that really are flat take the old path unchanged, so nothing that worked before changes answer.
Regression test
test/group/boolean_tangent_crossing/— @ruevs' file verbatim, checking that the display mesh is watertight (no naked or self-intersecting edges) and that the volume matches. Fails with(leaks) = truewithout the fix. Volume rather than image checks, consistent with the other Boolean cases.Verification
forceToMesh = 1) to the last digit: 38 triangles / volume 204432.7149 / 0 naked edges, against 54 triangles / 204432.7149 / 0 naked edges.Not addressed
@phkahler's two-sided-extrusion case. There is no model attached to the issue for it yet. Flipping a working model's group subtype 7000 → 7001 does produce failures, but that is not his geometry — it doubles the tool length so it pokes out of the top of the cube, and the tangency still involves one tangent and one crossing face rather than being tangent to both fillet faces. Those failures are pre-existing and unchanged by this PR. I'd rather wait for the real model than claim a fix I can't demonstrate.
Please fetch and fast-forward this branch rather than using the merge button on my fork — that keeps it a clean fast-forward for upstream.
🤖 Generated with Claude Code