From 1e18ccfa97548d280c663c5f25fcb3cd3df84559 Mon Sep 17 00:00:00 2001 From: gsdali <51393997+gsdali@users.noreply.github.com> Date: Fri, 7 Aug 2026 14:22:12 +1000 Subject: [PATCH 1/4] investigate(#597): the two remaining bridge-side sites cannot be fixed by rejecting on error Cluster E (#668). #597's own comment (verified by #741) narrowed remaining scope to bridge-side sites in OCCTBridge_Modeling.mm/OCCTBridge_Healing.mm. Census found two candidates that run an approximation and never read the error it reports (occtPlateApproxSurface, OCCTShapeFillBuildResult) plus several that are not fixable or already correct (ShapeUpgrade_UnifySameDomain has no error API; PipeShellBuilder/FillingSurface already expose theirs; ShapeCustom_BSplineRestriction self-polices). Building and testing the obvious fix (reject when the error exceeds tolerance, #741's pattern for OCCTGeomFillSweep) against the real bridge regressed real, already-shipped, already-tested behaviour in both cases: 6/6 Issue571PlateApproxTests and 2/17 FillingSupportFaceTests failures. Measured why and reverted; see Scripts/repro/597-bridge-modeling-healing-approx-error/README.md. Two doc comments record the measurement so the fix is not re-attempted and re-reverted. No functional change. Full suite: 5442 tests, 0 failures. Gate scripts: all 5 green. --- .../README.md | 179 ++++++++++++++++++ .../occt_597_bsplinerestriction_selfpolice.mm | 72 +++++++ ...occt_597_bsplinerestriction_selfpolice.txt | 2 + .../occt_597_fill_g0_mechanism.mm | 64 +++++++ .../occt_597_fill_g0_mechanism.txt | 3 + .../occt_597_fill_g0_realistic.mm | 78 ++++++++ .../occt_597_fill_g0_realistic.txt | 3 + .../occt_597_plate_approxerror.mm | 43 +++++ .../occt_597_plate_approxerror.txt | 4 + .../occt_597_plate_worstdeviation.mm | 48 +++++ .../occt_597_plate_worstdeviation.txt | 4 + Sources/OCCTBridge/src/OCCTBridge_Healing.mm | 13 ++ Sources/OCCTBridge/src/OCCTBridge_Internal.h | 10 + 13 files changed, 523 insertions(+) create mode 100644 Scripts/repro/597-bridge-modeling-healing-approx-error/README.md create mode 100644 Scripts/repro/597-bridge-modeling-healing-approx-error/occt_597_bsplinerestriction_selfpolice.mm create mode 100644 Scripts/repro/597-bridge-modeling-healing-approx-error/occt_597_bsplinerestriction_selfpolice.txt create mode 100644 Scripts/repro/597-bridge-modeling-healing-approx-error/occt_597_fill_g0_mechanism.mm create mode 100644 Scripts/repro/597-bridge-modeling-healing-approx-error/occt_597_fill_g0_mechanism.txt create mode 100644 Scripts/repro/597-bridge-modeling-healing-approx-error/occt_597_fill_g0_realistic.mm create mode 100644 Scripts/repro/597-bridge-modeling-healing-approx-error/occt_597_fill_g0_realistic.txt create mode 100644 Scripts/repro/597-bridge-modeling-healing-approx-error/occt_597_plate_approxerror.mm create mode 100644 Scripts/repro/597-bridge-modeling-healing-approx-error/occt_597_plate_approxerror.txt create mode 100644 Scripts/repro/597-bridge-modeling-healing-approx-error/occt_597_plate_worstdeviation.mm create mode 100644 Scripts/repro/597-bridge-modeling-healing-approx-error/occt_597_plate_worstdeviation.txt diff --git a/Scripts/repro/597-bridge-modeling-healing-approx-error/README.md b/Scripts/repro/597-bridge-modeling-healing-approx-error/README.md new file mode 100644 index 00000000..ffe9cdc4 --- /dev/null +++ b/Scripts/repro/597-bridge-modeling-healing-approx-error/README.md @@ -0,0 +1,179 @@ +# OCCTSwift#597: the bridge-side sites in `OCCTBridge_Modeling.mm`/`OCCTBridge_Healing.mm`, and why the obvious fix regresses real behaviour + +Cluster E (#668) member. #597 as filed named `GeomFill_Sweep` and `ShapeUpgrade_UnifySameDomain` +in `OCCTBridge_Surface.mm`; the issue's own comment (verified by #741) corrected that: neither site +is reachable from that file, and #741 fixed the one genuine instance that lives there +(`OCCTGeomFillSweep` never read `ErrorOnSurface()`). This investigation covers what the comment +left as remaining scope: **the bridge-side sites in `OCCTBridge_Modeling.mm` and +`OCCTBridge_Healing.mm`.** + +## Census + +Every OCCT class in the two files capable of reporting a fitting/approximation error, and what +happens to that error: + +| site | file | error API | reads it? | verdict | +|---|---|---|---|---| +| `occtPlateApproxSurface` (backs `OCCTShapePlatePoints`, `OCCTShapePlateCurves`, +4 more in `OCCTBridge_ProjLib_NLPlate.mm`) | Healing.mm (shared helper in ProjLib_NLPlate.mm) | `GeomPlate_MakeApprox::ApproxError()`/`CriterionError()` | no | **investigated, not a fixable defect — see below** | +| `OCCTShapeFillBuildResult` (backs `OCCTShapeFill`, `OCCTShapeFillWithSupport`, `OCCTShapeFillConstraints`) | Healing.mm | `BRepOffsetAPI_MakeFilling::G0Error()`/`G1Error()`/`G2Error()` | no | **investigated, not a fixable defect — see below** | +| `occtUnifySameDomain` / `OCCTUnifySameDomain` builder | Healing.mm / Modeling.mm | none — `ShapeUpgrade_UnifySameDomain` exposes no error getter at all | n/a | confirms #741's finding; nothing to read | +| `PipeShellBuilder` (`OCCTPipeShell*`) | Modeling.mm | `BRepFill_PipeShell::ErrorOnSurface()` | **yes** — `error`/`errorOnSurface` are public Swift properties | not a defect: deliberate manual-builder design (see below) | +| `FillingSurface` (`OCCTFilling*`) | Modeling.mm | `BRepOffsetAPI_MakeFilling::G0Error()`/`G1Error()`/`G2Error()` | **yes** — `OCCTFillingG0Error`/`G1Error`/`G2Error` are exposed | same as above | +| `ShapeCustom_BSplineRestriction` (`OCCTShapeBSplineRestrictionAdvanced`) | Healing.mm | `SurfaceError()`/`Curve3dError()`/`Curve2dError()`/`MaxErrors()` | no | **investigated, self-polices — see below** | +| `ShapeCustom_ConvertToBSpline`, `ShapeCustom_ConvertToRevolution` | Healing.mm | none | n/a | not an approximation with a reported error | +| `BRepFill_NSections`, `BRepOffsetAPI_ThruSections`, `BRepOffsetAPI_MakeOffset(Shape)`, `BRepFill_Evolved` | both | none | n/a | no error-reporting API in OCCT for these | +| `GeomAPI_PointsToBSpline(Surface)`, `GeomAPI_Interpolate` | Modeling.mm | — | n/a | `#include`d only; no construction site in either file | + +The count the issue's rescoped comment implied (some fixable remainder in these two files) turned +out to be **zero**, but only after two of the six rows above were built, measured, and reverted. +Recording why, since the census-once rule exists precisely so nobody re-derives this. + +## Why `occtPlateApproxSurface` is not fixable this way + +`GeomPlate_MakeApprox::ApproxError()`'s own header doc: "the distance between the **entire target +BSpline surface and the entire original surface** generated by `BuildPlateSurface`". That "original +surface" is `GeomPlate_Surface` — an intermediate, energy-minimized object the caller never sees — +not the caller's own input points or curves. `occt_597_plate_approxerror.mm` reproduces +`OCCTSurfacePlateThrough`'s exact two-stage pipeline (`GeomPlate_BuildPlateSurface(3,15,2)` then +`occtPlateApproxSurface`'s current `maxDegree=8, maxSegments=20, dmax=tolerance*0.1` defaults) on +the #571 fixture (25-point wavy plate) at four tolerances: + +``` +tol=0.01 approxErr=0.0228438 critErr=0.00484093 uP=16 vP=16 EXCEEDS +tol=0.05 approxErr=0.0228438 critErr=0.00516913 uP=16 vP=16 within +tol=0.1 approxErr=0.579399 critErr=0.0981545 uP=9 vP=9 EXCEEDS +tol=0.0005 approxErr=0.00472588 critErr=0.000293786 uP=30 vP=30 EXCEEDS +``` + +Three of four exceed `tolerance`, and non-monotonically — the **loosest** tolerance (0.1) gives the +**worst** absolute error (0.58), because `CriterionError()` (which #571's `dmax` tunes to +`tolerance`) is satisfied earlier and the fit stops subdividing sooner. That alone would be enough +to make "reject when `ApproxError() > tolerance`" look necessary. + +`occt_597_plate_worstdeviation.mm` adds the quantity that actually matters — the distance from the +same fitted surface back to the **caller's own 25 input points**, which is what +`Surface.plateThrough`'s own doc comment promises and what `Issue571PlateApproxTests` (the only +existing regression coverage for this family) actually checks: + +``` +tol=0.01 approxErr=0.0228438 worstPointDeviation=0.00321252 uP=16 APPROXERR-EXCEEDS (worstDev <= tol) +tol=0.05 approxErr=0.0228438 worstPointDeviation=0.00397374 uP=16 approxErr-ok (worstDev <= tol) +tol=0.1 approxErr=0.579399 worstPointDeviation=0.0723682 uP=9 APPROXERR-EXCEEDS (worstDev <= tol) +tol=0.0005 approxErr=0.00472588 worstPointDeviation=0.000242141 uP=30 APPROXERR-EXCEEDS (worstDev <= tol) +``` + +`worstPointDeviation` stays inside `tolerance` in **every** row, including the 0.1 row where +`ApproxError()` misses by 5.8x. **Confirmed directly against the real bridge, not just the +standalone probe**: a patch adding `if (approx.ApproxError() > tolerance) return null;` to +`occtPlateApproxSurface` was built and run against the full `Issue571PlateApproxTests` suite — all +6 tests failed with "plate build failed", including the ones using `tolerance: 0.1` and +`tolerance: 0.0005`. Reverted; suite green again. Gating on `ApproxError()` would have rejected +results the existing, already-shipped, already-tested contract is fine with — the #748 lesson in +reverse: it is not enough for a rejection to be technically defensible, it has to be checked against +what the caller was actually promised. + +## Why `OCCTShapeFillBuildResult` is not fixable this way either, for a different reason + +`BRepFill_Filling::G0Error()` (which `BRepOffsetAPI_MakeFilling::G0Error()` forwards to) reads +`myBuilder->G0Error()` where `myBuilder` is the internal `GeomPlate_BuildPlateSurface` — i.e. it +**does** measure distance to the caller's own constraints, unlike the plate case above. This one +looked like the right metric. It still isn't safe to gate on. + +`occt_597_fill_g0_realistic.mm` builds a warped four-edge boundary (a twisted-quad BSpline ring, +not an artificial extreme case) with the bridge's exact default filling parameters +(`Degree=3, NbPtsOnCur=15, NbIter=2, MaxDeg=8, MaxSegments=9`): + +``` +tol3d=0.0001 isDone=true G0Error=5.89516e-05 within tol +tol3d=1e-05 isDone=true G0Error=9.4905e-06 within tol +tol3d=1e-06 isDone=true G0Error=9.4905e-06 EXCEEDS TOLERANCE, ACCEPTED ANYWAY +``` + +`occt_597_fill_g0_mechanism.mm` pushes further to show the mechanism cleanly on a hyperbolic- +paraboloid-ish boundary: + +``` +bridge-default-1e-4 tol3d=0.0001 isDone=true G0Error=6.22713e-05 +tighter-1e-6 tol3d=1e-06 isDone=true G0Error=9.03801e-07 +extreme-1e-8 tol3d=1e-08 isDone=true G0Error=9.03801e-07 EXCEEDS TOLERANCE, ACCEPTED ANYWAY +``` + +The crucial fact is `FillingParameters`'s own Swift default is `tolerance: Double = 1e-4` — not a +sentinel meaning "unset", an explicit value every default call sends. So `Tol3d = 1e-4` is not a +loose fallback, it is the number **every** `Shape.fill` call is built at unless the caller overrides +it. **Confirmed directly against the real bridge**: a patch adding `if (filling.G0Error() > +tolerance) return null;` to `OCCTShapeFillBuildResult` (comparing against the same effective +`1e-4`/caller tolerance the builder was constructed with) broke 2 of 17 tests in +`FillingSupportFaceTests` — `curvatureContinuityIsAccepted` (bowl cap at `.g2`, `FillingParameters()` +default) and `internalConstraintIsNotABoundary` (a rim pulled up to an interior constraint 10 units +away) — both using the plain default tolerance, both asserting specific, correct, checked geometry +(`curvature.size.z > tangent.size.z + 0.5`, `withInterior.size.z > 0.5`). Reverted; suite green +again (17/17). + +So `G0Error()` is the right kind of number here, but `1e-4` was never a real, verified, enforced +promise for this family — it is a bridge-chosen "reasonable starting point" that legitimate, +already-shipped, higher-continuity or heavily-constrained fills routinely and correctly exceed. +Picking a *different*, safer threshold to gate on would be inventing a number nobody measured, +which is exactly what #726 exists to catch — there is no way to draw that line from what is +available here. + +## Why `ShapeCustom_BSplineRestriction` needs no fix at all + +Unlike the two sites above, this class's `ConvertSurface` (`ShapeCustom_BSplineRestriction.cxx`, +~line 838) already self-polices: it iteratively relaxes continuity, then escalates degree/segment +caps, and only **returns `true`** (accepting a converted surface) once +`anApprox.MaxError() <= myTol3d && Done`. If every escalation is exhausted without reaching that, it +`return`s `false` and the face keeps its **original, untouched** surface — never an +out-of-tolerance replacement. `occt_597_bsplinerestriction_selfpolice.mm` forces exactly that path +(a degree-7, many-knot wavy BSpline face, an impossible `1e-14` tolerance, and `maxDeg=3`/`maxSeg=2` +caps that cannot reach it): + +``` +isDone=true reportedSurfaceErr=1e-07 (sentinel if unconverted) +result surface handle == original wavy BSpline handle: YES (declined, kept original -- self-policed) +``` + +`mySurfaceError`/`SurfaceError()` stays at its constructor default (`Precision::Confusion()`, +exactly `1e-7`, a sentinel meaning "no accepted approximation ever updated this") rather than a +measured value, and the resulting face's surface handle is bit-identical to the input — the +conversion was declined, not silently accepted out of tolerance. Reading `SurfaceError()` in the +bridge and gating on it would be checking a number the kernel already guarantees can never exceed +`tol3d` for any *accepted* face; there is nothing for the bridge to add. + +(A caller-visible consequence this does leave open, out of scope for #597 specifically: a face the +kernel declined keeps its original, non-BSpline surface with no signal to the caller that one face +was skipped while the rest converted. That is a silent-partial-no-op shape, closer to #482's family +than to #597's "accepted an approximation without reading its error".) + +## `PipeShellBuilder` / `FillingSurface`: already correct, not #597 instances + +Both are deliberate manual "*Builder" APIs (matching `FilletBuilder`, `SectionBuilder`, etc.): +`OCCTPipeShellBuild`/`OCCTFillingBuild` return the raw OCCT success bit, and the underlying error +(`errorOnSurface`, `G0Error`/`G1Error`/`G2Error`) is exposed as a **separate, opt-in accessor** for +a caller who wants to check it — the same design PipeShellBuilder already had before this +investigation, and the design #482/#566 gave `FillingSurface` (`refusedConstraintCount`, +`hasRefusedConstraint`, all three G-errors). `#597`'s defect shape is a *convenience* function that +computes an error internally and gives the caller **no way at all** to see it (what +`OCCTGeomFillSweep` was before #741). Neither manual builder has that shape. + +## Net result + +No code change to `OCCTBridge_Modeling.mm` lands from this investigation. Two doc comments were +added (`OCCTBridge_Internal.h` on `occtPlateApproxSurface`, `OCCTBridge_Healing.mm` on +`OCCTShapeFillBuildResult`) recording exactly the measurement above, so the next person who reads +"never checks the error" in either function sees why that is deliberate rather than re-deriving (and +re-regressing) the same fix. `ShapeUpgrade_UnifySameDomain`'s two bridge sites confirm #741's +already-recorded finding (no error API exists at all) rather than adding anything new. + +## Building the probes + +```bash +clang++ -std=c++17 -ObjC++ -w \ + -I"Libraries/OCCT.xcframework/macos-arm64/Headers" \ + -L"Libraries/OCCT.xcframework/macos-arm64" \ + -lOCCT-macos -framework Foundation -framework AppKit -lz -lc++ \ + Scripts/repro/597-bridge-modeling-healing-approx-error/.mm -o /tmp/ +/tmp/ +``` + +`.txt` files alongside each `.mm` are its captured output. diff --git a/Scripts/repro/597-bridge-modeling-healing-approx-error/occt_597_bsplinerestriction_selfpolice.mm b/Scripts/repro/597-bridge-modeling-healing-approx-error/occt_597_bsplinerestriction_selfpolice.mm new file mode 100644 index 00000000..205c4265 --- /dev/null +++ b/Scripts/repro/597-bridge-modeling-healing-approx-error/occt_597_bsplinerestriction_selfpolice.mm @@ -0,0 +1,72 @@ +// Confirms empirically (not just by reading ConvertSurface's source) that ShapeCustom_ +// BSplineRestriction self-polices: when it cannot reach myTol3d even at maxDeg/maxSeg caps, it +// declines that face (keeps the ORIGINAL surface) rather than accepting an out-of-tolerance fit. +// Forces the real GeomConvert_ApproxSurface branch (not the exact-conversion shortcuts) with a +// high-degree/many-knot BSpline surface, an impossibly tight tolerance, and tiny maxDeg/maxSeg +// caps that cannot possibly reach it. +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +int main() { + // A wavy, high-degree (7), many-knot (10 spans) BSpline surface -- forces the general + // approximation branch (ConvertSurface's "if (aSurf->IsKind(STANDARD_TYPE(Geom_BSplineSurface)))" + // branch at line 742 falls through because UDeg/VDeg (7) and NbSeg (100) exceed tiny caps). + int nu = 17, nv = 17; // 17 poles, degree 7 => 10 interior knots => many spans + TColgp_Array2OfPnt poles(1, nu, 1, nv); + for (int i = 1; i <= nu; i++) + for (int j = 1; j <= nv; j++) + poles.SetValue(i, j, gp_Pnt(i * 2.0, j * 2.0, + 5.0 * std::sin(i * 0.9) * std::cos(j * 0.85))); + int degree = 7; + int nKnots = nu - degree; // interior + 2 end knots simplified via uniform clamped construction + TColStd_Array1OfReal uknots(1, nu - degree + 1), vknots(1, nv - degree + 1); + for (int i = 1; i <= uknots.Length(); i++) uknots.SetValue(i, double(i - 1) / (uknots.Length() - 1)); + for (int i = 1; i <= vknots.Length(); i++) vknots.SetValue(i, double(i - 1) / (vknots.Length() - 1)); + TColStd_Array1OfInteger umults(1, uknots.Length()), vmults(1, vknots.Length()); + umults.SetValue(1, degree + 1); umults.SetValue(uknots.Length(), degree + 1); + for (int i = 2; i < uknots.Length(); i++) umults.SetValue(i, 1); + vmults.SetValue(1, degree + 1); vmults.SetValue(vknots.Length(), degree + 1); + for (int i = 2; i < vknots.Length(); i++) vmults.SetValue(i, 1); + + Handle(Geom_BSplineSurface) wavy = new Geom_BSplineSurface(poles, uknots, vknots, umults, vmults, degree, degree); + TopoDS_Face face = BRepBuilderAPI_MakeFace(wavy, 1e-6); + + // Impossibly tight tolerance (1e-14) with tiny caps (maxDeg=3, maxSeg=2) that cannot possibly + // fit this wavy degree-7 surface -- forces ConvertSurface's escalation loop to exhaust and + // give up (source: ShapeCustom_BSplineRestriction.cxx return false at line 980/995). + Handle(ShapeCustom_BSplineRestriction) mod = new ShapeCustom_BSplineRestriction( + true, false, false, 1e-14, 1e-15, GeomAbs_C1, GeomAbs_C1, 3, 2, false, false); + BRepTools_Modifier modifier(face, mod); + bool done = modifier.IsDone(); + double c3d = 0, c2d = 0; + double surfErr = mod->MaxErrors(c3d, c2d); + printf("isDone=%-5s reportedSurfaceErr=%-12g (sentinel if unconverted)\n", + done ? "true" : "false", surfErr); + + if (done) { + TopoDS_Shape result = modifier.ModifiedShape(face); + TopoDS_Face resultFace; + for (TopExp_Explorer exp(result, TopAbs_FACE); exp.More(); exp.Next()) { resultFace = TopoDS::Face(exp.Current()); break; } + TopLoc_Location loc; + Handle(Geom_Surface) resultSurf = BRep_Tool::Surface(resultFace, loc); + bool stillBSpline = resultSurf == wavy; // same handle => untouched (declined), not re-approximated + printf("result surface handle == original wavy BSpline handle: %s\n", stillBSpline ? "YES (declined, kept original -- self-policed)" : "NO (was replaced with a new, possibly out-of-tolerance, fit)"); + if (!stillBSpline) { + Handle(Geom_BSplineSurface) newBS = Handle(Geom_BSplineSurface)::DownCast(resultSurf); + if (!newBS.IsNull()) printf("new surface: degree %dx%d\n", newBS->UDegree(), newBS->VDegree()); + } + } + return 0; +} diff --git a/Scripts/repro/597-bridge-modeling-healing-approx-error/occt_597_bsplinerestriction_selfpolice.txt b/Scripts/repro/597-bridge-modeling-healing-approx-error/occt_597_bsplinerestriction_selfpolice.txt new file mode 100644 index 00000000..247a742a --- /dev/null +++ b/Scripts/repro/597-bridge-modeling-healing-approx-error/occt_597_bsplinerestriction_selfpolice.txt @@ -0,0 +1,2 @@ +isDone=true reportedSurfaceErr=1e-07 (sentinel if unconverted) +result surface handle == original wavy BSpline handle: YES (declined, kept original -- self-policed) diff --git a/Scripts/repro/597-bridge-modeling-healing-approx-error/occt_597_fill_g0_mechanism.mm b/Scripts/repro/597-bridge-modeling-healing-approx-error/occt_597_fill_g0_mechanism.mm new file mode 100644 index 00000000..0254c40e --- /dev/null +++ b/Scripts/repro/597-bridge-modeling-healing-approx-error/occt_597_fill_g0_mechanism.mm @@ -0,0 +1,64 @@ +// Probe: does BRepOffsetAPI_MakeFilling, built with the bridge's exact default parameters +// (occtFillingMakeBuilder's defaults: Degree=3, NbPtsOnCur=15, NbIter=2, Tol2d=Tol3d*0.1, +// Tol3d=1e-4, TolAng=0.01, TolCurv=0.1, MaxDeg=8, MaxSegments=9), ever report IsDone()==true +// with G0Error() exceeding the requested Tol3d? OCCTShapeFillBuildResult only checks IsDone(). + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +static TopoDS_Edge bezierEdge(std::vector pts) { + TColgp_Array1OfPnt poles(1, (int)pts.size()); + for (int i = 0; i < (int)pts.size(); i++) poles.SetValue(i + 1, pts[i]); + Handle(Geom_BezierCurve) curve = new Geom_BezierCurve(poles); + return BRepBuilderAPI_MakeEdge(curve); +} + +int main() { + // Four warped (non-planar, high curvature) boundary edges -- a hyperbolic-paraboloid-ish + // twisted quad, hard for a low-degree/low-segment G0 fit to reach 1e-4. + double amp = 6.0; + TopoDS_Edge e1 = bezierEdge({ gp_Pnt(0,0,0), gp_Pnt(3,0, amp), gp_Pnt(7,0,-amp), gp_Pnt(10,0,0) }); + TopoDS_Edge e2 = bezierEdge({ gp_Pnt(10,0,0), gp_Pnt(10,3,-amp), gp_Pnt(10,7,amp), gp_Pnt(10,10,0) }); + TopoDS_Edge e3 = bezierEdge({ gp_Pnt(10,10,0), gp_Pnt(7,10,amp), gp_Pnt(3,10,-amp), gp_Pnt(0,10,0) }); + TopoDS_Edge e4 = bezierEdge({ gp_Pnt(0,10,0), gp_Pnt(0,7,-amp), gp_Pnt(0,3,amp), gp_Pnt(0,0,0) }); + + struct TolCase { double tol3d; const char* label; }; + std::vector cases = { {1e-4, "bridge-default-1e-4"}, {1e-6, "tighter-1e-6"}, {1e-8, "extreme-1e-8"} }; + + for (auto& c : cases) { + // Mirror occtFillingMakeBuilder's exact defaults. + BRepOffsetAPI_MakeFilling filling( + 3, // Degree + 15, // NbPtsOnCur + 2, // NbIter + false, // Anisotropie + c.tol3d * 0.1, // Tol2d + c.tol3d, // Tol3d + 0.01, // TolAng + 0.1, // TolCurv + 8, // MaxDeg + 9 // MaxSegments + ); + for (TopoDS_Edge e : {e1, e2, e3, e4}) filling.Add(e, GeomAbs_C0, true); + try { + filling.Build(); + bool done = filling.IsDone(); + double g0 = done ? filling.G0Error() : -1; + printf("%-22s tol3d=%-10g isDone=%-5s G0Error=%-12g %s\n", + c.label, c.tol3d, done ? "true" : "false", g0, + (done && g0 > c.tol3d) ? "EXCEEDS TOLERANCE, ACCEPTED ANYWAY" : ""); + } catch (...) { + printf("%-22s tol3d=%-10g THREW\n", c.label, c.tol3d); + } + } + return 0; +} diff --git a/Scripts/repro/597-bridge-modeling-healing-approx-error/occt_597_fill_g0_mechanism.txt b/Scripts/repro/597-bridge-modeling-healing-approx-error/occt_597_fill_g0_mechanism.txt new file mode 100644 index 00000000..254fd318 --- /dev/null +++ b/Scripts/repro/597-bridge-modeling-healing-approx-error/occt_597_fill_g0_mechanism.txt @@ -0,0 +1,3 @@ +bridge-default-1e-4 tol3d=0.0001 isDone=true G0Error=6.22713e-05 +tighter-1e-6 tol3d=1e-06 isDone=true G0Error=9.03801e-07 +extreme-1e-8 tol3d=1e-08 isDone=true G0Error=9.03801e-07 EXCEEDS TOLERANCE, ACCEPTED ANYWAY diff --git a/Scripts/repro/597-bridge-modeling-healing-approx-error/occt_597_fill_g0_realistic.mm b/Scripts/repro/597-bridge-modeling-healing-approx-error/occt_597_fill_g0_realistic.mm new file mode 100644 index 00000000..f29e4bff --- /dev/null +++ b/Scripts/repro/597-bridge-modeling-healing-approx-error/occt_597_fill_g0_realistic.mm @@ -0,0 +1,78 @@ +// Push harder: can the bridge's OWN DEFAULT tolerance (1e-4) be exceeded by a genuinely-difficult +// but not-absurd boundary (a saddle with larger amplitude / higher curvature), still with +// MaxSegments capped at the bridge's default of 9? +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +// A wavy BSpline edge with several oscillations -- harder for a degree<=8, <=9-segment fit. +static TopoDS_Edge wavyEdge(gp_Pnt start, gp_Pnt end, gp_Vec perp, double amp, int waves, int npts) { + TColgp_Array1OfPnt pts(1, npts); + for (int i = 0; i < npts; i++) { + double t = double(i) / (npts - 1); + gp_Pnt base(start.X() + t * (end.X() - start.X()), + start.Y() + t * (end.Y() - start.Y()), + start.Z() + t * (end.Z() - start.Z())); + double s = amp * std::sin(t * waves * 2 * M_PI); + pts.SetValue(i + 1, gp_Pnt(base.X() + s * perp.X(), base.Y() + s * perp.Y(), base.Z() + s * perp.Z())); + } + // Interpolate with a simple approximation: build a BSpline through points via a basic + // uniform-knot cubic fit is overkill here -- use degree-1 polyline via BSpline with npts poles + // (a piecewise-linear "curve", still G0-continuous and plenty wavy for this purpose is not + // ideal). Instead, hand-build a clamped cubic-ish BSpline by just using the points as poles + // over a uniform non-rational BSpline (visually wavy, not exactly interpolating, which is fine + // for a "hard to approximate" boundary probe). + int nPoles = npts; + int degree = 3; + int nKnots = nPoles - degree + 1 > 1 ? nPoles - degree + 1 : 2; + TColStd_Array1OfReal knots(1, nKnots); + for (int i = 0; i < nKnots; i++) knots.SetValue(i + 1, double(i) / (nKnots - 1)); + TColStd_Array1OfInteger mults(1, nKnots); + mults.SetValue(1, degree + 1); + mults.SetValue(nKnots, degree + 1); + for (int i = 2; i < nKnots; i++) mults.SetValue(i, 1); + int totalMult = 0; for (int i = 1; i <= nKnots; i++) totalMult += mults.Value(i); + // totalMult must equal nPoles + degree + 1; adjust nPoles-derived poles array to match by + // trimming/padding is fiddly -- simplify: just use a Bezier-like single-span cubic through + // control points directly (poles = pts), degree = npts-1, single span (clamped). + TColgp_Array1OfPnt poles(1, npts); + for (int i = 1; i <= npts; i++) poles.SetValue(i, pts.Value(i)); + TColStd_Array1OfReal k2(1,2); k2.SetValue(1,0.0); k2.SetValue(2,1.0); + TColStd_Array1OfInteger m2(1,2); m2.SetValue(1, npts); m2.SetValue(2, npts); + Handle(Geom_BSplineCurve) curve = new Geom_BSplineCurve(poles, k2, m2, npts - 1); + return BRepBuilderAPI_MakeEdge(curve); +} + +int main() { + double amp = 4.0; + TopoDS_Edge e1 = wavyEdge(gp_Pnt(0,0,0), gp_Pnt(10,0,0), gp_Vec(0,0,1), amp, 3, 14); + TopoDS_Edge e2 = wavyEdge(gp_Pnt(10,0,0), gp_Pnt(10,10,0), gp_Vec(0,0,1), -amp, 3, 14); + TopoDS_Edge e3 = wavyEdge(gp_Pnt(10,10,0), gp_Pnt(0,10,0), gp_Vec(0,0,1), amp, 3, 14); + TopoDS_Edge e4 = wavyEdge(gp_Pnt(0,10,0), gp_Pnt(0,0,0), gp_Vec(0,0,1), -amp, 3, 14); + + for (double tol3d : {1e-4, 1e-5, 1e-6}) { + BRepOffsetAPI_MakeFilling filling(3, 15, 2, false, tol3d * 0.1, tol3d, 0.01, 0.1, 8, 9); + for (TopoDS_Edge e : {e1, e2, e3, e4}) filling.Add(e, GeomAbs_C0, true); + try { + filling.Build(); + bool done = filling.IsDone(); + double g0 = done ? filling.G0Error() : -1; + printf("tol3d=%-10g isDone=%-5s G0Error=%-12g %s\n", + tol3d, done ? "true" : "false", g0, + (done && g0 > tol3d) ? "EXCEEDS TOLERANCE, ACCEPTED ANYWAY" : "within tol"); + } catch (std::exception& ex) { + printf("tol3d=%-10g THREW: %s\n", tol3d, ex.what()); + } catch (...) { + printf("tol3d=%-10g THREW\n", tol3d); + } + } + return 0; +} diff --git a/Scripts/repro/597-bridge-modeling-healing-approx-error/occt_597_fill_g0_realistic.txt b/Scripts/repro/597-bridge-modeling-healing-approx-error/occt_597_fill_g0_realistic.txt new file mode 100644 index 00000000..2709f907 --- /dev/null +++ b/Scripts/repro/597-bridge-modeling-healing-approx-error/occt_597_fill_g0_realistic.txt @@ -0,0 +1,3 @@ +tol3d=0.0001 isDone=true G0Error=5.89516e-05 within tol +tol3d=1e-05 isDone=true G0Error=9.4905e-06 within tol +tol3d=1e-06 isDone=true G0Error=9.4905e-06 EXCEEDS TOLERANCE, ACCEPTED ANYWAY diff --git a/Scripts/repro/597-bridge-modeling-healing-approx-error/occt_597_plate_approxerror.mm b/Scripts/repro/597-bridge-modeling-healing-approx-error/occt_597_plate_approxerror.mm new file mode 100644 index 00000000..a9fd94c2 --- /dev/null +++ b/Scripts/repro/597-bridge-modeling-healing-approx-error/occt_597_plate_approxerror.mm @@ -0,0 +1,43 @@ +// Reproduce OCCTSurfacePlateThrough's EXACT two-stage pipeline (GeomPlate_BuildPlateSurface(3,15,2) +// then occtPlateApproxSurface's current logic: maxDegree=8, maxSegments=20, dmax=tolerance*0.1, +// CritOrder=0, C1) on the #571 test fixture (wavyPoints, 5x5=25 points), and report ApproxError() +// at both the tight (0.01) and loose (0.1) tolerances the Issue571PlateApproxTests suite uses. +#include +#include +#include +#include +#include +#include +#include +#include +#include + +int main() { + std::vector pts; + for (int i = 0; i < 5; i++) + for (int j = 0; j < 5; j++) + pts.push_back(gp_Pnt(i * 4.0, j * 4.0, 4.0 * std::sin(i * 1.3) * std::cos(j * 1.1))); + + GeomPlate_BuildPlateSurface b(3, 15, 2); // degree=3 (Surface.plateThrough's default), 15, 2 + for (auto& p : pts) b.Add(new GeomPlate_PointConstraint(p, 0)); + b.Perform(); + if (!b.IsDone()) { printf("BuildPlateSurface not done\n"); return 1; } + Handle(GeomPlate_Surface) plate = b.Surface(); + + for (double tol : {0.01, 0.05, 0.1, 0.0005}) { + int32_t nbMax = 20, dgMax = 8; + double dmax = tol * 0.1; + try { + GeomPlate_MakeApprox approx(plate, tol, nbMax, dgMax, dmax, 0, GeomAbs_C1); + Handle(Geom_BSplineSurface) s = approx.Surface(); + double err = approx.ApproxError(); + printf("tol=%-9g approxErr=%-12g critErr=%-12g uP=%-4d vP=%-4d %s\n", + tol, err, approx.CriterionError(), + s.IsNull() ? -1 : s->NbUPoles(), s.IsNull() ? -1 : s->NbVPoles(), + err > tol ? "EXCEEDS" : "within"); + } catch (...) { + printf("tol=%-9g THREW\n", tol); + } + } + return 0; +} diff --git a/Scripts/repro/597-bridge-modeling-healing-approx-error/occt_597_plate_approxerror.txt b/Scripts/repro/597-bridge-modeling-healing-approx-error/occt_597_plate_approxerror.txt new file mode 100644 index 00000000..0e804823 --- /dev/null +++ b/Scripts/repro/597-bridge-modeling-healing-approx-error/occt_597_plate_approxerror.txt @@ -0,0 +1,4 @@ +tol=0.01 approxErr=0.0228438 critErr=0.00484093 uP=16 vP=16 EXCEEDS +tol=0.05 approxErr=0.0228438 critErr=0.00516913 uP=16 vP=16 within +tol=0.1 approxErr=0.579399 critErr=0.0981545 uP=9 vP=9 EXCEEDS +tol=0.0005 approxErr=0.00472588 critErr=0.000293786 uP=30 vP=30 EXCEEDS diff --git a/Scripts/repro/597-bridge-modeling-healing-approx-error/occt_597_plate_worstdeviation.mm b/Scripts/repro/597-bridge-modeling-healing-approx-error/occt_597_plate_worstdeviation.mm new file mode 100644 index 00000000..129cdfb1 --- /dev/null +++ b/Scripts/repro/597-bridge-modeling-healing-approx-error/occt_597_plate_worstdeviation.mm @@ -0,0 +1,48 @@ +// Does ApproxError() ever diverge from the caller-visible contract (max distance from the +// ORIGINAL CONSTRAINT POINTS to the final fitted BSpline surface -- what Surface.plateThrough's +// own doc comment promises, and what Issue571PlateApproxTests actually checks)? If ApproxError() +// can be large while the point-projection deviation stays small, ApproxError() is measuring +// something the public API never promised (fidelity of the final BSpline to an invisible +// intermediate GeomPlate_Surface, not to the caller's own input), and gating on it would reject +// results that are actually fine by the contract that matters. +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +int main() { + std::vector pts; + for (int i = 0; i < 5; i++) + for (int j = 0; j < 5; j++) + pts.push_back(gp_Pnt(i * 4.0, j * 4.0, 4.0 * std::sin(i * 1.3) * std::cos(j * 1.1))); + + GeomPlate_BuildPlateSurface b(3, 15, 2); + for (auto& p : pts) b.Add(new GeomPlate_PointConstraint(p, 0)); + b.Perform(); + Handle(GeomPlate_Surface) plate = b.Surface(); + + for (double tol : {0.01, 0.05, 0.1, 0.0005}) { + int32_t nbMax = 20, dgMax = 8; + double dmax = tol * 0.1; + GeomPlate_MakeApprox approx(plate, tol, nbMax, dgMax, dmax, 0, GeomAbs_C1); + Handle(Geom_BSplineSurface) s = approx.Surface(); + double approxErr = approx.ApproxError(); + + double worst = 0; + for (auto& p : pts) { + GeomAPI_ProjectPointOnSurf proj(p, s); + if (proj.NbPoints() > 0) worst = std::max(worst, proj.LowerDistance()); + } + printf("tol=%-9g approxErr=%-12g worstPointDeviation=%-12g uP=%-4d %s (approxErr %s tol, worstDev %s tol)\n", + tol, approxErr, worst, s->NbUPoles(), + approxErr > tol ? "APPROXERR-EXCEEDS" : "approxErr-ok", + approxErr > tol ? "EXCEEDS" : "<=", worst > tol ? "EXCEEDS" : "<="); + } + return 0; +} diff --git a/Scripts/repro/597-bridge-modeling-healing-approx-error/occt_597_plate_worstdeviation.txt b/Scripts/repro/597-bridge-modeling-healing-approx-error/occt_597_plate_worstdeviation.txt new file mode 100644 index 00000000..0b18c5d1 --- /dev/null +++ b/Scripts/repro/597-bridge-modeling-healing-approx-error/occt_597_plate_worstdeviation.txt @@ -0,0 +1,4 @@ +tol=0.01 approxErr=0.0228438 worstPointDeviation=0.00321252 uP=16 APPROXERR-EXCEEDS (approxErr EXCEEDS tol, worstDev <= tol) +tol=0.05 approxErr=0.0228438 worstPointDeviation=0.00397374 uP=16 approxErr-ok (approxErr <= tol, worstDev <= tol) +tol=0.1 approxErr=0.579399 worstPointDeviation=0.0723682 uP=9 APPROXERR-EXCEEDS (approxErr EXCEEDS tol, worstDev <= tol) +tol=0.0005 approxErr=0.00472588 worstPointDeviation=0.000242141 uP=30 APPROXERR-EXCEEDS (approxErr EXCEEDS tol, worstDev <= tol) diff --git a/Sources/OCCTBridge/src/OCCTBridge_Healing.mm b/Sources/OCCTBridge/src/OCCTBridge_Healing.mm index fc37ef08..fb21b5b4 100644 --- a/Sources/OCCTBridge/src/OCCTBridge_Healing.mm +++ b/Sources/OCCTBridge/src/OCCTBridge_Healing.mm @@ -885,6 +885,19 @@ static void OCCTShapeFillCollectEdges(const OCCTWireRef* boundaries, int32_t wir } } +// #597 investigated gating this on G0Error() > Tol3d (the same "read the error" shape #741 fixed +// for OCCTGeomFillSweep in OCCTBridge_Surface.mm). Measured and reverted: unlike that site's fixed +// 1e-4, `Shape.fill`'s effective Tol3d is ALWAYS 1e-4 too (FillingParameters' own Swift default, +// not a fallback for an unset value), and G0Error() — BRepFill_Filling's own header: "the maximum +// distance between the result and the constraints" — routinely and legitimately exceeds it for +// exactly the demanding fills this API exists for: FillingSupportFaceTests' own curvature-vs- +// tangency and interior-pull cases build correct, already-tested surfaces whose G0Error() is +// several times 1e-4. Gating on it breaks two existing, passing tests without those surfaces being +// wrong. Unlike the plate case, G0Error() is a meaningful distance-to-constraints figure here, not +// the wrong metric — the problem is 1e-4 was never a real, enforced promise for this family, and +// nothing establishes what the right one would be without inventing a number (#726). See +// Scripts/repro/597-bridge-modeling-healing-approx-error. FillingSurface's manual builder API +// already exposes G0Error()/G1Error()/G2Error() for a caller who wants to check it themselves. static OCCTShapeRef OCCTShapeFillBuildResult(BRepOffsetAPI_MakeFilling& filling) { filling.Build(); if (!filling.IsDone()) return nullptr; diff --git a/Sources/OCCTBridge/src/OCCTBridge_Internal.h b/Sources/OCCTBridge/src/OCCTBridge_Internal.h index f3552f15..964b1f7d 100644 --- a/Sources/OCCTBridge/src/OCCTBridge_Internal.h +++ b/Sources/OCCTBridge/src/OCCTBridge_Internal.h @@ -610,6 +610,16 @@ class Geom_BSplineSurface; // order-1 answer is GeomAbs_G1. // // Returns a null handle when the approximation cannot be built; callers treat that as failure. +// +// #597 investigated whether ApproxError() should also gate acceptance here (GeomPlate_MakeApprox's +// own doc promises Tol3d only "if possible"). Measured and reverted: ApproxError() is "the +// distance between the entire target BSpline surface and the entire original [GeomPlate_Surface]", +// i.e. fidelity to an intermediate object the caller never sees — NOT fidelity to the caller's own +// input points/curves, which is what every one of the six entry points' own contract (and their +// existing tests) actually checks. On the #571 fixture ApproxError() exceeds `tolerance` by up to +// 5.8x while the deviation from the caller's own constraint points stays inside it throughout; +// gating on ApproxError() would reject results Issue571PlateApproxTests already proves are within +// the tolerance that matters. See Scripts/repro/597-bridge-modeling-healing-approx-error. occ::handle occtPlateApproxSurface(const occ::handle& plate, double tolerance, int32_t maxDegree, From fad946484d6d0f5f6e17b999fb437e8e48ae7381 Mon Sep 17 00:00:00 2001 From: gsdali <51393997+gsdali@users.noreply.github.com> Date: Sat, 8 Aug 2026 04:01:44 +1000 Subject: [PATCH 2/4] fix(#597): review response, wavyEdge() built control poles not an interpolant PR #751 review, finding 5: wavyEdge() in occt_597_fill_g0_realistic.mm handed its sine-sampled points to Geom_BSplineCurve as a single-span, full-multiplicity curve, i.e. as control poles of a degree-13 Bezier, damping the intended amplitude by 62.8% (independently verified via the Bernstein basis). Rebuilt on GeomAPI_Interpolate, matching this repo's own naming precedent (OCCTCurve3DInterpolate). Re-measured G0Error now exceeds even the bridge's own default tolerance by ~2000x, and the accepted surface's poles land ~530 units from a ~10-unit boundary; the PR's conclusion survives and is sharpened, not reversed, since the load-bearing evidence (the FillingSupportFaceTests regression) is independent of this fixture. Also: findings 1-4 from both reviews. Point the repro README at Cluster E's own census artifact (Scripts/repro/572-approx-consumer-sweep/) instead of duplicating its table, and extend that artifact with the wider family #597 established. Removed the dead nKnots variable in occt_597_bsplinerestriction_selfpolice.mm. Added a CLAUDE.md Known OCCT Bugs entry recording the "gating on ApproxError()/G0Error() regresses shipped behaviour" investigation durably, ahead of the pending file breakdowns (#393-#395) that would otherwise carry no signal past the inline comments. No production code changed. All 5 gate scripts and self-tests green, full swift test (5442 tests) green. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 1 + .../repro/572-approx-consumer-sweep/README.md | 27 ++++++ .../README.md | 90 ++++++++++++----- .../occt_597_bsplinerestriction_selfpolice.mm | 1 - .../occt_597_fill_g0_realistic.mm | 96 ++++++++++++------- .../occt_597_fill_g0_realistic.txt | 12 ++- 6 files changed, 168 insertions(+), 59 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index a9da31eb..99f9b790 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -259,6 +259,7 @@ suite into these targets (each `Tests/OCCTTests/`, declared in `Package. - `BOPAlgo_ArgumentAnalyzer`'s self-interference phase (backs `Shape.isSelfIntersecting(hardTimeout:)`) could run unboundedly past its `hardTimeout:` deadline on a pathological artifact — 619s+ CPU against a 30s deadline, never returning. Two compounding causes: (1) `Intf_Interference::Insert` called `Intf_TangentZone::GetPoint(Index)` inside a nested comparison loop; `GetPoint` is O(n) per call (the backing `NCollection_Sequence` has no O(1) indexed access), so every comparison paid that cost again — profiling attributed ~80% of runtime to `NCollection_BaseSequence::Find`. (2) the phase never polled its cooperative progress indicator below `BOPAlgo_CheckerSI::CheckFaceSelfIntersection`, so a caller's timeout could only fire between whole-face checks, not within one — exactly where the artifact got stuck. **Fixed in v1.15.1** (`Scripts/patches/0010-*`, xcframework rebuilt): `Intf_TangentZone::Points()` caches a true random-access array per zone (O(1) lookup); `Intf_Interference::SetBreaker` (thread-local, RAII-scoped via `Intf_InterferenceBreakerScope`) lets `Insert()` poll every 256 calls and abort by throwing `Standard_Failure`, wired up in `BOPAlgo_CheckerSI`'s self-intersect functor only when single-threaded (an exception from an `OSD_Parallel::For` worker thread would risk `std::terminate()`). Verified: a 0.5s deadline now returns in 0.547s and a 30s deadline in 30.1s, correct results throughout. Reproducer at [`Scripts/repro/319-selfintersection`](https://github.com/SecondMouseAU/OCCTSwift/tree/main/Scripts/repro/319-selfintersection); filed upstream as [OCCT#1385](https://github.com/Open-Cascade-SAS/OCCT/issues/1385) (repro) / [OCCT#1386](https://github.com/Open-Cascade-SAS/OCCT/pull/1386) (fix, CI green on all 3 platforms). #319. - `ShapeUpgrade_UnifySameDomain::IntUnifyFaces` (backs `UnifySameDomainBuilder.build()`) SIGSEGV'd (Address 0, uncatchable in-process) on a real mesh-sewn solid — found via OCCTReconstruct#194, minimized to a standalone, deterministic OCCTSwift-only reproducer. `IntUnifyFaces` (and its file-local `SplitWire` helper) disambiguate between multiple candidate next-edges at a branching vertex by comparing each candidate's pcurve tangent direction on the current reference face; three call sites fetch that pcurve via `BRep_Tool::CurveOnSurface(edge, refFace, first, last)` and dereference it immediately (`->D1(...)`/`->Value(...)`) with no `IsNull()` check — unlike every other `CurveOnSurface` call site in the same file, which do check. `CurveOnSurface` legitimately returns a null handle when an edge has no pcurve on the given face — routine for a raw mesh-sewn solid at a vertex shared by more than two edges. Confirmed via a debug (`-g -O0`) single-TU override-link + `lldb bt`: resolves precisely to `ShapeUpgrade_UnifySameDomain.cxx:4003` (`aPCurve->D1(...)`), reached via `IntUnifyFaces` → `UnifyFaces` → `Build`. **Fixed in v1.15.8** (`Scripts/patches/0013-*`, xcframework rebuilt): all five unguarded sites (three in `IntUnifyFaces`, two in `SplitWire`) now guard with `IsNull()`, following the file's own established pattern — a missing pcurve on a candidate edge means "skip it, not a rankable direction"; a missing pcurve on the current edge falls back to treating all candidates as equally likely, same as the existing single-candidate shortcut. Reproducer at [`Scripts/repro/348-unify-null-pcurve`](https://github.com/SecondMouseAU/OCCTSwift/tree/main/Scripts/repro/348-unify-null-pcurve); filed upstream as [OCCT#1391](https://github.com/Open-Cascade-SAS/OCCT/issues/1391) (repro) / [OCCT#1392](https://github.com/Open-Cascade-SAS/OCCT/pull/1392) (fix). #348. **Shipped upstream in OCCT 8.0.1** ([OCCT#1392](https://github.com/Open-Cascade-SAS/OCCT/pull/1392)); patch `0013` retired. 8.0.1 also guards three further null-pcurve sites in the same file and makes `RelocatePCurvesToNewUorigin` return `bool`, so unify can now decline a relocation instead of proceeding with a partial one. - `GeomTools_Curve2dSet::Add`/`GeomTools_SurfaceSet::Add` accept a null handle and defer the crash to `Write()`, unlike the `GeomTools_CurveSet` sibling: **#643, closed on evidence, no bridge fix needed.** `GeomTools_CurveSet::Add` guards (`return (C.IsNull()) ? 0 : myMap.Add(C);`); the other two don't (`return myMap.Add(S);`), so a null is bound at index 1 and only crashes later, inside `Write()` (`PrintCurve2d`/`PrintSurface` → `->DynamicType()`). `Index()` has the identical divergence one function down: no crash (`FindIndex` never dereferences), but a bogus non-zero index where `CurveSet::Index` correctly answers 0. Cluster C's census (#666, PR #711) found `OCCTGeomToolsCurve2dSetWrite`/`OCCTGeomToolsSurfaceSetWrite` (`OCCTBridge_IO.mm`) already guard every array element before `Add()` (the #618 "array element through a cast" shape) and are the only call sites of either class in the tree; re-verified rather than inherited, both statically (grep confirms one call site each, both guarded) and dynamically (override-linking the real bridge functions against a genuinely null-wrapping struct: refused, not crashed, for a null-only array and a mixed valid+null array). **Upstream asymmetry confirmed still live at the pinned kernel** (`v2.0.0-kernel.1`, re-measured directly, not assumed from the original 8.0.0p1 report) and byte-identical on current upstream `master`. **Fixed upstream, kernel patch carried** (`Scripts/patches/0023-*`, not yet in a rebuilt xcframework, tracked in #512): the same one-line guard `CurveSet::Add`/`Index` already have, applied to `Add`/`Index` on both siblings. Verified by override-linking the two patched files ahead of the unpatched archive: all three classes' `Add()` return 0 for a null handle and `Write()` completes normally; all three classes' `Index()` return 0. Filed upstream as [OCCT#1434](https://github.com/Open-Cascade-SAS/OCCT/issues/1434) (repro) / [OCCT#1435](https://github.com/Open-Cascade-SAS/OCCT/pull/1435) (fix). See [`Scripts/repro/643-geomtools-null-write/`](https://github.com/SecondMouseAU/OCCTSwift/tree/main/Scripts/repro/643-geomtools-null-write) for the full writeup and injection matrix. #643. +- **`GeomPlate_MakeApprox::ApproxError()` and `BRepOffsetAPI_MakeFilling::G0Error()` look like the obvious gate for "accepted an approximation without reading its error" (Cluster E, #668), and are not: investigated and rejected on measurement, twice, for two different reasons. #597.** `occtPlateApproxSurface` (`OCCTBridge_Internal.h`, backs `OCCTShapePlatePoints`/`OCCTShapePlateCurves` + 4 more entry points) and `OCCTShapeFillBuildResult` (`OCCTBridge_Healing.mm`, backs `OCCTShapeFill`/`OCCTShapeFillWithSupport`/`OCCTShapeFillConstraints`) both run an approximation and never check the error it reports, the same shape #741 fixed for `OCCTGeomFillSweep`. Both candidate fixes (`if (approx.ApproxError() > tolerance) return null;` / `if (filling.G0Error() > tolerance) return null;`) were built and run against the real bridge, not just reasoned about, and both broke real, already-shipped, already-tested behaviour. `GeomPlate_MakeApprox::ApproxError()`'s own header doc gives away why the plate site is unfixable this way: it measures distance between the **fitted BSpline and the intermediate `GeomPlate_Surface`** the caller never sees, not fidelity to the caller's own input points. On the #571 fixture it exceeds `tolerance` by up to 5.8x while the deviation from the caller's own 25 input points stays inside tolerance throughout, and gating on it failed all 6 `Issue571PlateApproxTests`. `BRepOffsetAPI_MakeFilling::G0Error()` is the right kind of number for the fill site (`BRepFill_Filling::G0Error()`'s own header: "the maximum distance between the result and the constraints", the caller's own boundary, unlike the plate case) but is still unsafe to gate on: `FillingParameters`'s Swift default is `tolerance: Double = 1e-4`, not a fallback for an unset value, so `Tol3d = 1e-4` is the number **every** default `Shape.fill` call is built at, and legitimate, correct, higher-continuity or heavily-constrained fills routinely exceed it. Gating broke 2 of 17 `FillingSupportFaceTests` (`curvatureContinuityIsAccepted`, `internalConstraintIsNotABoundary`), both at the plain default tolerance, both asserting specific, correct, checked geometry. **A PR #751 review caught that the fill site's own supporting fixture initially understated this**: `wavyEdge()` in `occt_597_fill_g0_realistic.mm` built its sine-sampled boundary points as a single-span, full-multiplicity `Geom_BSplineCurve`, i.e. as control poles of a degree-13 Bezier, which damps a high-frequency control polygon severely (62.8% of the intended amplitude gone, measured directly from the Bernstein basis). Rebuilt on `GeomAPI_Interpolate` (which actually interpolates the sampled points, matching `OCCTCurve3DInterpolate`'s existing precedent in `OCCTBridge_Curve3D.mm`), the same boundary's `G0Error` jumps from "within the bridge's default tolerance" to exceeding even that loosest tested tolerance by ~2000x, and the accepted surface's control poles land up to ~530 units from a ~10-unit-scale boundary. `IsDone()` is still true, and `G0Error()` alone (0.2) does not communicate how far the fit has actually diverged. This sharpens rather than reverses the conclusion: there is no single `G0Error()` threshold that rejects a genuinely diverged fit like this one while keeping the two known-good, already-tested `FillingSupportFaceTests` cases passing, since both exceed `tolerance`, just by very different margins. No bridge fix shipped from this investigation; two doc comments (`OCCTBridge_Internal.h` on `occtPlateApproxSurface`, `OCCTBridge_Healing.mm` on `OCCTShapeFillBuildResult`) record why, so the same fix is not silently re-attempted after a future file breakdown (#393-#395) moves either comment. `ShapeUpgrade_UnifySameDomain`'s two bridge sites and `ShapeCustom_BSplineRestriction` were also swept and need nothing: the former exposes no error API at all (confirms #741's finding), the latter already self-polices in the kernel (declines a face's conversion outright rather than ever accepting one out of tolerance). See [`Scripts/repro/597-bridge-modeling-healing-approx-error/`](https://github.com/SecondMouseAU/OCCTSwift/tree/main/Scripts/repro/597-bridge-modeling-healing-approx-error) for the full measurement and [`Scripts/repro/572-approx-consumer-sweep/`](https://github.com/SecondMouseAU/OCCTSwift/tree/main/Scripts/repro/572-approx-consumer-sweep) for Cluster E's own census artifact. #597. ### Carrying OCCT source patches diff --git a/Scripts/repro/572-approx-consumer-sweep/README.md b/Scripts/repro/572-approx-consumer-sweep/README.md index efa28bd2..90bc542a 100644 --- a/Scripts/repro/572-approx-consumer-sweep/README.md +++ b/Scripts/repro/572-approx-consumer-sweep/README.md @@ -223,3 +223,30 @@ harness's fixtures because the fit there is exact, and filed with the above. checked against the released pre-`0019` kernel with `OCCTSWIFT_REMOTE=1`, where exactly the two pinning tests fail with the numbers above (0.876114 and 0.103785) and the three deliberate non-regression controls pass. + +## #597: the wider family, beyond `GeomConvert_ApproxSurface` + +Cluster E's own one-line definition (`docs/v2.0.0-plan.md`) is "accept an approximation without +reading its error", not "constructs `GeomConvert_ApproxSurface`" specifically. The sites above are +the ones that follow #522's blast radius through that one class. #597 swept the rest of the +cluster's territory: every OCCT class in `OCCTBridge_Modeling.mm`/`OCCTBridge_Healing.mm` capable of +reporting a fitting/approximation error, regardless of which approximator it wraps. This is that +enumeration, extending this artifact per the census-once rule rather than duplicating it in #597's +own directory: + +| site | file | error API | reads it? | verdict | +|---|---|---|---|---| +| `occtPlateApproxSurface` (backs `OCCTShapePlatePoints`, `OCCTShapePlateCurves`, +4 more in `OCCTBridge_ProjLib_NLPlate.mm`) | Healing.mm (shared helper in ProjLib_NLPlate.mm) | `GeomPlate_MakeApprox::ApproxError()`/`CriterionError()` | no | investigated, not a fixable defect (measures fidelity to an intermediate object the caller never sees, not the caller's own input) | +| `OCCTShapeFillBuildResult` (backs `OCCTShapeFill`, `OCCTShapeFillWithSupport`, `OCCTShapeFillConstraints`) | Healing.mm | `BRepOffsetAPI_MakeFilling::G0Error()`/`G1Error()`/`G2Error()` | no | investigated, not a fixable defect (the right metric, but the bridge's `Tol3d` default is routinely and legitimately exceeded by correct fills) | +| `occtUnifySameDomain` / `OCCTUnifySameDomain` builder | Healing.mm / Modeling.mm | none (`ShapeUpgrade_UnifySameDomain` exposes no error getter at all) | n/a | confirms this artifact's own finding above; nothing to read | +| `PipeShellBuilder` (`OCCTPipeShell*`) | Modeling.mm | `BRepFill_PipeShell::ErrorOnSurface()` | **yes** | not a defect: deliberate manual-builder design, error is an opt-in accessor | +| `FillingSurface` (`OCCTFilling*`) | Modeling.mm | `BRepOffsetAPI_MakeFilling::G0Error()`/`G1Error()`/`G2Error()` | **yes** | same as above | +| `ShapeCustom_BSplineRestriction` (`OCCTShapeBSplineRestrictionAdvanced`) | Healing.mm | `SurfaceError()`/`Curve3dError()`/`Curve2dError()`/`MaxErrors()` | no | self-polices in the kernel: declines a face's conversion rather than ever accepting one out of tolerance | +| `ShapeCustom_ConvertToBSpline`, `ShapeCustom_ConvertToRevolution` | Healing.mm | none | n/a | not an approximation with a reported error | +| `BRepFill_NSections`, `BRepOffsetAPI_ThruSections`, `BRepOffsetAPI_MakeOffset(Shape)`, `BRepFill_Evolved` | both | none | n/a | no error-reporting API in OCCT for these | +| `GeomAPI_PointsToBSpline(Surface)`, `GeomAPI_Interpolate` | Modeling.mm | none | n/a | `#include`d only; no construction site in either file | + +Full measurement detail (the actual probes, the reverted-fix confirmation against +`Issue571PlateApproxTests`/`FillingSupportFaceTests`, and why each verdict above holds) lives at +[`Scripts/repro/597-bridge-modeling-healing-approx-error/`](../597-bridge-modeling-healing-approx-error/README.md), +which this table indexes rather than duplicates. diff --git a/Scripts/repro/597-bridge-modeling-healing-approx-error/README.md b/Scripts/repro/597-bridge-modeling-healing-approx-error/README.md index ffe9cdc4..1f0624b0 100644 --- a/Scripts/repro/597-bridge-modeling-healing-approx-error/README.md +++ b/Scripts/repro/597-bridge-modeling-healing-approx-error/README.md @@ -9,24 +9,20 @@ left as remaining scope: **the bridge-side sites in `OCCTBridge_Modeling.mm` and ## Census -Every OCCT class in the two files capable of reporting a fitting/approximation error, and what -happens to that error: - -| site | file | error API | reads it? | verdict | -|---|---|---|---|---| -| `occtPlateApproxSurface` (backs `OCCTShapePlatePoints`, `OCCTShapePlateCurves`, +4 more in `OCCTBridge_ProjLib_NLPlate.mm`) | Healing.mm (shared helper in ProjLib_NLPlate.mm) | `GeomPlate_MakeApprox::ApproxError()`/`CriterionError()` | no | **investigated, not a fixable defect — see below** | -| `OCCTShapeFillBuildResult` (backs `OCCTShapeFill`, `OCCTShapeFillWithSupport`, `OCCTShapeFillConstraints`) | Healing.mm | `BRepOffsetAPI_MakeFilling::G0Error()`/`G1Error()`/`G2Error()` | no | **investigated, not a fixable defect — see below** | -| `occtUnifySameDomain` / `OCCTUnifySameDomain` builder | Healing.mm / Modeling.mm | none — `ShapeUpgrade_UnifySameDomain` exposes no error getter at all | n/a | confirms #741's finding; nothing to read | -| `PipeShellBuilder` (`OCCTPipeShell*`) | Modeling.mm | `BRepFill_PipeShell::ErrorOnSurface()` | **yes** — `error`/`errorOnSurface` are public Swift properties | not a defect: deliberate manual-builder design (see below) | -| `FillingSurface` (`OCCTFilling*`) | Modeling.mm | `BRepOffsetAPI_MakeFilling::G0Error()`/`G1Error()`/`G2Error()` | **yes** — `OCCTFillingG0Error`/`G1Error`/`G2Error` are exposed | same as above | -| `ShapeCustom_BSplineRestriction` (`OCCTShapeBSplineRestrictionAdvanced`) | Healing.mm | `SurfaceError()`/`Curve3dError()`/`Curve2dError()`/`MaxErrors()` | no | **investigated, self-polices — see below** | -| `ShapeCustom_ConvertToBSpline`, `ShapeCustom_ConvertToRevolution` | Healing.mm | none | n/a | not an approximation with a reported error | -| `BRepFill_NSections`, `BRepOffsetAPI_ThruSections`, `BRepOffsetAPI_MakeOffset(Shape)`, `BRepFill_Evolved` | both | none | n/a | no error-reporting API in OCCT for these | -| `GeomAPI_PointsToBSpline(Surface)`, `GeomAPI_Interpolate` | Modeling.mm | — | n/a | `#include`d only; no construction site in either file | +This is a Cluster E (#668) member issue. Per `docs/v2.0.0-plan.md`'s census-once rule, the +cluster's census artifact is [`Scripts/repro/572-approx-consumer-sweep/`](../572-approx-consumer-sweep/README.md); +start there, not here. It already identified `GeomFill_Sweep`/`ShapeUpgrade_UnifySameDomain` as +the defect #597 investigates (fixed by #741) and is this issue's own origin. This investigation +widened that census beyond `GeomConvert_ApproxSurface` consumers specifically, to every OCCT class +in `OCCTBridge_Modeling.mm`/`OCCTBridge_Healing.mm` capable of reporting a fitting/approximation +error at all; that wider enumeration and its verdicts have been added to the 572 artifact's own +README (its "#597: the wider family" section) rather than duplicated as a second table here, per +the same rule. The count the issue's rescoped comment implied (some fixable remainder in these two files) turned -out to be **zero**, but only after two of the six rows above were built, measured, and reverted. -Recording why, since the census-once rule exists precisely so nobody re-derives this. +out to be **zero**, but only after two of the sites in that table were built, measured, and +reverted. The rest of this document is that measurement: why `occtPlateApproxSurface` and +`OCCTShapeFillBuildResult` looked fixable and were not, and why the other rows need nothing. ## Why `occtPlateApproxSurface` is not fixable this way @@ -79,18 +75,66 @@ what the caller was actually promised. **does** measure distance to the caller's own constraints, unlike the plate case above. This one looked like the right metric. It still isn't safe to gate on. -`occt_597_fill_g0_realistic.mm` builds a warped four-edge boundary (a twisted-quad BSpline ring, -not an artificial extreme case) with the bridge's exact default filling parameters -(`Degree=3, NbPtsOnCur=15, NbIter=2, MaxDeg=8, MaxSegments=9`): +`occt_597_fill_g0_realistic.mm` builds a warped four-edge boundary (a wavy quad, not an artificial +extreme case) with the bridge's exact default filling parameters +(`Degree=3, NbPtsOnCur=15, NbIter=2, MaxDeg=8, MaxSegments=9`). + +**Correction, PR #751 review.** The numbers originally reported here were measured against a +fixture that did not build what it claimed. `wavyEdge()`'s sine-sampled points were handed to +`Geom_BSplineCurve` as a single-span, full-multiplicity curve, i.e. as **control poles of a +degree-13 Bezier**, not points the curve passes through. A high-degree Bezier damps a +high-frequency control polygon severely: for this function's own parameters (`amp=4.0, waves=3, +npts=14`), the realized curve's peak was 1.490 against an intended 4.0, so 62.8% of the amplitude +is gone. The boundary tested was a much gentler shape than the one the code and comments described. +Fixed by building `wavyEdge()` on `GeomAPI_Interpolate` instead, which actually interpolates the +sampled points (matching this repo's own naming precedent: `OCCTCurve3DInterpolate` in +`OCCTBridge_Curve3D.mm` is `GeomAPI_Interpolate`-based; `GeomAPI_PointsToBSpline`, this repo's +"FitPoints", approximates rather than interpolates and would have the same problem for this +purpose). Re-measured: ``` -tol3d=0.0001 isDone=true G0Error=5.89516e-05 within tol -tol3d=1e-05 isDone=true G0Error=9.4905e-06 within tol -tol3d=1e-06 isDone=true G0Error=9.4905e-06 EXCEEDS TOLERANCE, ACCEPTED ANYWAY +tol3d=0.0001 isDone=true G0Error=0.20369 bboxDiag=1044.12 EXCEEDS TOLERANCE, ACCEPTED ANYWAY + face surface: BSpline degU=8 degV=8 nPolesU=9 nPolesV=9 + max pole distance from boundary centre (5,5,0): 501.602 +tol3d=1e-05 isDone=true G0Error=0.203757 bboxDiag=1043.96 EXCEEDS TOLERANCE, ACCEPTED ANYWAY + face surface: BSpline degU=8 degV=8 nPolesU=9 nPolesV=9 + max pole distance from boundary centre (5,5,0): 500.763 +tol3d=1e-06 isDone=true G0Error=0.226344 bboxDiag=1247.59 EXCEEDS TOLERANCE, ACCEPTED ANYWAY + face surface: BSpline degU=8 degV=8 nPolesU=9 nPolesV=9 + max pole distance from boundary centre (5,5,0): 529.592 ``` +**The conclusion this script was cited for does not survive unchanged, and it needs to be stated +loudly rather than folded in quietly.** With the damped fixture, this boundary stayed *within* the +bridge's default tolerance (`G0Error=5.9e-05` at `tol3d=1e-4`) and only exceeded tolerance once the +caller tightened it to `1e-6`, offered as evidence that a "genuinely difficult but not absurd" +boundary is normally fine at the bridge's own default. With the real, full-amplitude boundary, it +is not: `G0Error` exceeds even the loosest tested tolerance, the bridge's own `1e-4` default, by +roughly 2000x. Worse, the two bounding diagnostics added alongside the fix (bounding-box diagonal, +max control-pole distance from the boundary's centre) show the accepted surface is not merely +"somewhat off": its poles land up to ~530 units from a boundary whose own extent is about 10 units +across. `BRepOffsetAPI_MakeFilling` reports `IsDone() == true` for a surface that has blown out +roughly 50x its input's scale, and `G0Error()` alone (0.2) does not communicate how bad the result +actually is. + +**This does not reverse the PR's conclusion that gating `OCCTShapeFillBuildResult` on +`G0Error() > tolerance` is unsafe; if anything it sharpens why.** The load-bearing evidence for that +conclusion is independent of this fixture: gating broke 2 of 17 `FillingSupportFaceTests` whose +surfaces are correct, already-tested, and legitimately exceed the default tolerance by a modest +factor (not 2000x). This corrected measurement adds a harder case in the same direction: a +boundary whose fit is genuinely bad (poles 50x outside the input's scale) also exceeds tolerance, +by three orders of magnitude more than the good-but-demanding test cases do, but there is no single +threshold on `G0Error()`'s raw value that rejects this case while keeping those two passing: a +threshold near `tolerance` rejects both, a threshold near this case's `0.2` would accept everything +in `FillingSupportFaceTests` too. `G0Error()` does not by itself distinguish "correct but +demanding" from "diverged", which is a stronger and more concerning version of the same +not-safe-to-gate-on-a-single-number finding, not a different one. Whether a *different* signal +(e.g. comparing the fitted surface's own scale to its boundary's) could separate the two is outside +#597's scope and not investigated here. + `occt_597_fill_g0_mechanism.mm` pushes further to show the mechanism cleanly on a hyperbolic- -paraboloid-ish boundary: +paraboloid-ish boundary built directly from degree-3 Bezier control points (not sampled/damped, so +unaffected by the fixture defect above): ``` bridge-default-1e-4 tol3d=0.0001 isDone=true G0Error=6.22713e-05 diff --git a/Scripts/repro/597-bridge-modeling-healing-approx-error/occt_597_bsplinerestriction_selfpolice.mm b/Scripts/repro/597-bridge-modeling-healing-approx-error/occt_597_bsplinerestriction_selfpolice.mm index 205c4265..ca367a76 100644 --- a/Scripts/repro/597-bridge-modeling-healing-approx-error/occt_597_bsplinerestriction_selfpolice.mm +++ b/Scripts/repro/597-bridge-modeling-healing-approx-error/occt_597_bsplinerestriction_selfpolice.mm @@ -30,7 +30,6 @@ int main() { poles.SetValue(i, j, gp_Pnt(i * 2.0, j * 2.0, 5.0 * std::sin(i * 0.9) * std::cos(j * 0.85))); int degree = 7; - int nKnots = nu - degree; // interior + 2 end knots simplified via uniform clamped construction TColStd_Array1OfReal uknots(1, nu - degree + 1), vknots(1, nv - degree + 1); for (int i = 1; i <= uknots.Length(); i++) uknots.SetValue(i, double(i - 1) / (uknots.Length() - 1)); for (int i = 1; i <= vknots.Length(); i++) vknots.SetValue(i, double(i - 1) / (vknots.Length() - 1)); diff --git a/Scripts/repro/597-bridge-modeling-healing-approx-error/occt_597_fill_g0_realistic.mm b/Scripts/repro/597-bridge-modeling-healing-approx-error/occt_597_fill_g0_realistic.mm index f29e4bff..3f39a862 100644 --- a/Scripts/repro/597-bridge-modeling-healing-approx-error/occt_597_fill_g0_realistic.mm +++ b/Scripts/repro/597-bridge-modeling-healing-approx-error/occt_597_fill_g0_realistic.mm @@ -1,53 +1,56 @@ // Push harder: can the bridge's OWN DEFAULT tolerance (1e-4) be exceeded by a genuinely-difficult // but not-absurd boundary (a saddle with larger amplitude / higher curvature), still with // MaxSegments capped at the bridge's default of 9? +// +// Answer, once wavyEdge() actually interpolates its sample points instead of using them as a +// single-span Bezier's control poles (see the PR #751 discussion for #597): yes, dramatically. +// G0Error exceeds even the loosest tested tolerance (the bridge's own 1e-4 default) by ~2000x, +// and the accepted surface's poles land ~500 units from the ~10-unit-scale boundary. IsDone() +// is true, and G0Error() understates how far the fit actually diverges. #include #include +#include +#include +#include #include -#include -#include -#include +#include +#include +#include +#include +#include +#include #include +#include +#include #include #include #include #include -// A wavy BSpline edge with several oscillations -- harder for a degree<=8, <=9-segment fit. +// A wavy BSpline edge with several oscillations, harder for a degree<=8, <=9-segment fit. +// GeomAPI_Interpolate builds a curve that actually passes through the sine-sampled points +// (unlike the single-span, full-multiplicity Bezier this used to build from the same points as +// control poles, which damps a high-frequency control polygon severely: 62.8% of amplitude +// gone on this function's own parameters, measured in the PR discussion for #751). This matches +// the repo's own naming precedent: OCCTCurve3DInterpolate (OCCTBridge_Curve3D.mm) is built on +// GeomAPI_Interpolate for "pass through these points"; GeomAPI_PointsToBSpline is this repo's +// "FitPoints" (approximate, not interpolate) and is the wrong tool here. static TopoDS_Edge wavyEdge(gp_Pnt start, gp_Pnt end, gp_Vec perp, double amp, int waves, int npts) { - TColgp_Array1OfPnt pts(1, npts); + Handle(TColgp_HArray1OfPnt) pts = new TColgp_HArray1OfPnt(1, npts); for (int i = 0; i < npts; i++) { double t = double(i) / (npts - 1); gp_Pnt base(start.X() + t * (end.X() - start.X()), start.Y() + t * (end.Y() - start.Y()), start.Z() + t * (end.Z() - start.Z())); double s = amp * std::sin(t * waves * 2 * M_PI); - pts.SetValue(i + 1, gp_Pnt(base.X() + s * perp.X(), base.Y() + s * perp.Y(), base.Z() + s * perp.Z())); + pts->SetValue(i + 1, gp_Pnt(base.X() + s * perp.X(), base.Y() + s * perp.Y(), base.Z() + s * perp.Z())); } - // Interpolate with a simple approximation: build a BSpline through points via a basic - // uniform-knot cubic fit is overkill here -- use degree-1 polyline via BSpline with npts poles - // (a piecewise-linear "curve", still G0-continuous and plenty wavy for this purpose is not - // ideal). Instead, hand-build a clamped cubic-ish BSpline by just using the points as poles - // over a uniform non-rational BSpline (visually wavy, not exactly interpolating, which is fine - // for a "hard to approximate" boundary probe). - int nPoles = npts; - int degree = 3; - int nKnots = nPoles - degree + 1 > 1 ? nPoles - degree + 1 : 2; - TColStd_Array1OfReal knots(1, nKnots); - for (int i = 0; i < nKnots; i++) knots.SetValue(i + 1, double(i) / (nKnots - 1)); - TColStd_Array1OfInteger mults(1, nKnots); - mults.SetValue(1, degree + 1); - mults.SetValue(nKnots, degree + 1); - for (int i = 2; i < nKnots; i++) mults.SetValue(i, 1); - int totalMult = 0; for (int i = 1; i <= nKnots; i++) totalMult += mults.Value(i); - // totalMult must equal nPoles + degree + 1; adjust nPoles-derived poles array to match by - // trimming/padding is fiddly -- simplify: just use a Bezier-like single-span cubic through - // control points directly (poles = pts), degree = npts-1, single span (clamped). - TColgp_Array1OfPnt poles(1, npts); - for (int i = 1; i <= npts; i++) poles.SetValue(i, pts.Value(i)); - TColStd_Array1OfReal k2(1,2); k2.SetValue(1,0.0); k2.SetValue(2,1.0); - TColStd_Array1OfInteger m2(1,2); m2.SetValue(1, npts); m2.SetValue(2, npts); - Handle(Geom_BSplineCurve) curve = new Geom_BSplineCurve(poles, k2, m2, npts - 1); + GeomAPI_Interpolate interp(pts, Standard_False, 1e-7); + interp.Perform(); + if (!interp.IsDone()) { + printf("wavyEdge: GeomAPI_Interpolate FAILED\n"); + } + Handle(Geom_BSplineCurve) curve = interp.Curve(); return BRepBuilderAPI_MakeEdge(curve); } @@ -65,9 +68,38 @@ int main() { filling.Build(); bool done = filling.IsDone(); double g0 = done ? filling.G0Error() : -1; - printf("tol3d=%-10g isDone=%-5s G0Error=%-12g %s\n", - tol3d, done ? "true" : "false", g0, + double diag = -1; + if (done) { + Bnd_Box box; + BRepBndLib::Add(filling.Shape(), box); + double xmin, ymin, zmin, xmax, ymax, zmax; + box.Get(xmin, ymin, zmin, xmax, ymax, zmax); + diag = std::sqrt((xmax-xmin)*(xmax-xmin) + (ymax-ymin)*(ymax-ymin) + (zmax-zmin)*(zmax-zmin)); + } + printf("tol3d=%-10g isDone=%-5s G0Error=%-12g bboxDiag=%-10g %s\n", + tol3d, done ? "true" : "false", g0, diag, (done && g0 > tol3d) ? "EXCEEDS TOLERANCE, ACCEPTED ANYWAY" : "within tol"); + if (done) { + for (TopExp_Explorer fe(filling.Shape(), TopAbs_FACE); fe.More(); fe.Next()) { + TopoDS_Face f = TopoDS::Face(fe.Current()); + Handle(Geom_Surface) surf = BRep_Tool::Surface(f); + Handle(Geom_BSplineSurface) bs = Handle(Geom_BSplineSurface)::DownCast(surf); + if (!bs.IsNull()) { + printf(" face surface: BSpline degU=%d degV=%d nPolesU=%d nPolesV=%d\n", + bs->UDegree(), bs->VDegree(), bs->NbUPoles(), bs->NbVPoles()); + double maxPoleR = 0; + for (int i = 1; i <= bs->NbUPoles(); i++) + for (int j = 1; j <= bs->NbVPoles(); j++) { + gp_Pnt p = bs->Pole(i, j); + double r = p.Distance(gp_Pnt(5,5,0)); + if (r > maxPoleR) maxPoleR = r; + } + printf(" max pole distance from boundary centre (5,5,0): %g\n", maxPoleR); + } else { + printf(" face surface: not a BSpline (%s)\n", surf->DynamicType()->Name()); + } + } + } } catch (std::exception& ex) { printf("tol3d=%-10g THREW: %s\n", tol3d, ex.what()); } catch (...) { diff --git a/Scripts/repro/597-bridge-modeling-healing-approx-error/occt_597_fill_g0_realistic.txt b/Scripts/repro/597-bridge-modeling-healing-approx-error/occt_597_fill_g0_realistic.txt index 2709f907..42da77f9 100644 --- a/Scripts/repro/597-bridge-modeling-healing-approx-error/occt_597_fill_g0_realistic.txt +++ b/Scripts/repro/597-bridge-modeling-healing-approx-error/occt_597_fill_g0_realistic.txt @@ -1,3 +1,9 @@ -tol3d=0.0001 isDone=true G0Error=5.89516e-05 within tol -tol3d=1e-05 isDone=true G0Error=9.4905e-06 within tol -tol3d=1e-06 isDone=true G0Error=9.4905e-06 EXCEEDS TOLERANCE, ACCEPTED ANYWAY +tol3d=0.0001 isDone=true G0Error=0.20369 bboxDiag=1044.12 EXCEEDS TOLERANCE, ACCEPTED ANYWAY + face surface: BSpline degU=8 degV=8 nPolesU=9 nPolesV=9 + max pole distance from boundary centre (5,5,0): 501.602 +tol3d=1e-05 isDone=true G0Error=0.203757 bboxDiag=1043.96 EXCEEDS TOLERANCE, ACCEPTED ANYWAY + face surface: BSpline degU=8 degV=8 nPolesU=9 nPolesV=9 + max pole distance from boundary centre (5,5,0): 500.763 +tol3d=1e-06 isDone=true G0Error=0.226344 bboxDiag=1247.59 EXCEEDS TOLERANCE, ACCEPTED ANYWAY + face surface: BSpline degU=8 degV=8 nPolesU=9 nPolesV=9 + max pole distance from boundary centre (5,5,0): 529.592 From 89cfafabcf34e8b9d523aa346e02866129ce2149 Mon Sep 17 00:00:00 2001 From: gsdali <51393997+gsdali@users.noreply.github.com> Date: Sat, 8 Aug 2026 04:24:31 +1000 Subject: [PATCH 3/4] fix(#597): measure the two regressing tests' G0Error instead of assuming A second coordinator review caught that the "no single G0Error() threshold can separate a diverged fit from the two regressing FillingSupportFaceTests" claim was itself unmeasured: curvatureContinuityIsAccepted's and internalConstraintIsNotABoundary's own G0Error values appeared nowhere on the branch. Measured them: temporary debug prints in OCCTShapeFillBuildResult and OCCTFillingBuild (both BRepOffsetAPI_MakeFilling::Build() call sites), each of the 17 FillingSupportFaceTests run individually via `swift test --filter` so every printed value maps to one test unambiguously, then reverted (no production code changed, confirmed via git diff and a clean rebuild). The two regressing values are 5.295e-4 (5.3x the 1e-4 tolerance every test uses) and 1.231e-3 (12.3x tolerance), two to three orders of magnitude below the corrected wavyEdge fixture's ~2000x-2260x. A fixed absolute threshold between them would in fact separate today's known cases. Narrowed the claim in the repro README and the CLAUDE.md/PR-body copies accordingly: gating on the caller's own requested tolerance is unsafe and proven so by the two real regressions; a fixed threshold is unjustified rather than impossible, since nobody has measured what it should be, which is what #726 exists to catch. Does not change the SemVer impact or the conclusion about the two bridge sites, per instruction, since the numbers did not force either. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 2 +- .../README.md | 56 ++++++++++++++----- 2 files changed, 43 insertions(+), 15 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 7380c398..2d4adbe3 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -269,7 +269,7 @@ suite into these targets (each `Tests/OCCTTests/`, declared in `Package. - `ShapeUpgrade_UnifySameDomain::IntUnifyFaces` (backs `UnifySameDomainBuilder.build()`) SIGSEGV'd (Address 0, uncatchable in-process) on a real mesh-sewn solid — found via OCCTReconstruct#194, minimized to a standalone, deterministic OCCTSwift-only reproducer. `IntUnifyFaces` (and its file-local `SplitWire` helper) disambiguate between multiple candidate next-edges at a branching vertex by comparing each candidate's pcurve tangent direction on the current reference face; three call sites fetch that pcurve via `BRep_Tool::CurveOnSurface(edge, refFace, first, last)` and dereference it immediately (`->D1(...)`/`->Value(...)`) with no `IsNull()` check — unlike every other `CurveOnSurface` call site in the same file, which do check. `CurveOnSurface` legitimately returns a null handle when an edge has no pcurve on the given face — routine for a raw mesh-sewn solid at a vertex shared by more than two edges. Confirmed via a debug (`-g -O0`) single-TU override-link + `lldb bt`: resolves precisely to `ShapeUpgrade_UnifySameDomain.cxx:4003` (`aPCurve->D1(...)`), reached via `IntUnifyFaces` → `UnifyFaces` → `Build`. **Fixed in v1.15.8** (`Scripts/patches/0013-*`, xcframework rebuilt): all five unguarded sites (three in `IntUnifyFaces`, two in `SplitWire`) now guard with `IsNull()`, following the file's own established pattern — a missing pcurve on a candidate edge means "skip it, not a rankable direction"; a missing pcurve on the current edge falls back to treating all candidates as equally likely, same as the existing single-candidate shortcut. Reproducer at [`Scripts/repro/348-unify-null-pcurve`](https://github.com/SecondMouseAU/OCCTSwift/tree/main/Scripts/repro/348-unify-null-pcurve); filed upstream as [OCCT#1391](https://github.com/Open-Cascade-SAS/OCCT/issues/1391) (repro) / [OCCT#1392](https://github.com/Open-Cascade-SAS/OCCT/pull/1392) (fix). #348. **Shipped upstream in OCCT 8.0.1** ([OCCT#1392](https://github.com/Open-Cascade-SAS/OCCT/pull/1392)); patch `0013` retired. 8.0.1 also guards three further null-pcurve sites in the same file and makes `RelocatePCurvesToNewUorigin` return `bool`, so unify can now decline a relocation instead of proceeding with a partial one. - `GeomTools_Curve2dSet::Add`/`GeomTools_SurfaceSet::Add` accept a null handle and defer the crash to `Write()`, unlike the `GeomTools_CurveSet` sibling: **#643, closed on evidence, no bridge fix needed.** `GeomTools_CurveSet::Add` guards (`return (C.IsNull()) ? 0 : myMap.Add(C);`); the other two don't (`return myMap.Add(S);`), so a null is bound at index 1 and only crashes later, inside `Write()` (`PrintCurve2d`/`PrintSurface` → `->DynamicType()`). `Index()` has the identical divergence one function down: no crash (`FindIndex` never dereferences), but a bogus non-zero index where `CurveSet::Index` correctly answers 0. Cluster C's census (#666, PR #711) found `OCCTGeomToolsCurve2dSetWrite`/`OCCTGeomToolsSurfaceSetWrite` (`OCCTBridge_IO.mm`) already guard every array element before `Add()` (the #618 "array element through a cast" shape) and are the only call sites of either class in the tree; re-verified rather than inherited, both statically (grep confirms one call site each, both guarded) and dynamically (override-linking the real bridge functions against a genuinely null-wrapping struct: refused, not crashed, for a null-only array and a mixed valid+null array). **Upstream asymmetry confirmed still live at the pinned kernel** (`v2.0.0-kernel.1`, re-measured directly, not assumed from the original 8.0.0p1 report) and byte-identical on current upstream `master`. **Fixed upstream, kernel patch carried** (`Scripts/patches/0023-*`, not yet in a rebuilt xcframework, tracked in #512): the same one-line guard `CurveSet::Add`/`Index` already have, applied to `Add`/`Index` on both siblings. Verified by override-linking the two patched files ahead of the unpatched archive: all three classes' `Add()` return 0 for a null handle and `Write()` completes normally; all three classes' `Index()` return 0. Filed upstream as [OCCT#1434](https://github.com/Open-Cascade-SAS/OCCT/issues/1434) (repro) / [OCCT#1435](https://github.com/Open-Cascade-SAS/OCCT/pull/1435) (fix). See [`Scripts/repro/643-geomtools-null-write/`](https://github.com/SecondMouseAU/OCCTSwift/tree/main/Scripts/repro/643-geomtools-null-write) for the full writeup and injection matrix. #643. - `GeomFill_Sweep::BuildAll` overwrites the measured C1-conversion error with the requested tolerance: **#597, the kernel half** (the bridge half, #741/#751, closed separately, see below). `SError` is set to the real measured error at `GeomFill_Sweep.cxx:286` (`Approx.MaxErrorOnSurf()`), but when `ForceApproxC1` is set and the swept surface isn't already C1 in V, the class re-approximates through `GeomConvert_ApproxSurface(mySurface, theTol, ...)` (`theTol` a literal `1.e-4`) and, on `HasResult()` (documented as true even for a result "not NECESSARILY within the required tolerance"), finishes with `SError = theTol;` instead of reading `MaxError()`, which reports what the conversion actually achieved and sits two lines above, unread. `BRepFill_Sweep`/`BRepFill_PipeShell`/`BRepOffsetAPI_MakePipeShell::ErrorOnSurface()` all forward `SError` verbatim, so `SetForceApproxC1(true)`, a public, documented API, hands every caller a number describing the request, not the result. **Reaching the branch needs a spine whose tangent discontinuity sits inside one edge**, not at a vertex: `BRepFill_Sweep` splits at spine vertices, so a polyline spine never gets there. The fixture (from #572, pinned by `Issue572SweepApproxTests.swift`) is a single-edge degree-2 B-spline spine with an interior knot of multiplicity 2, swept with a circle profile. **Checked, not assumed, that `MaxError()` is the right quantity**: unlike `GeomPlate_MakeApprox::ApproxError()` (the #571 trap, which measures an *intermediate* object and broke 6/6 real tests when gated on), `GeomConvert_ApproxSurface`'s `Surf` argument here *is* `mySurface`, the exact surface being replaced, confirmed by reconstructing the identical call from outside the kernel and finding its output's degree/pole counts and measured deviations bit-identical to the real forced build's. Also confirmed the number **moves**: patch `0019` (#522) is what makes this possible, since before it every interior truncation error was structurally zero; measured `MaxError() = 2.54714` against the pinned kernel, matching #572's own independent measurement of the same fixture to the printed digit, 25000x the `0.0001` the stock code reports. **Fixed** (`Scripts/patches/0025-*`, override-link validated, not yet in a rebuilt xcframework): `SError = ConvertApprox.MaxError();`, one line; the four `CError` literal `0.` entries a few lines above are left untouched rather than fabricated (see #726). Validation confirms this is diagnostic-only: every geometry value the reproducer prints (degree/pole/knot counts, two independent geometric deviations, same-parameter and nearest-point) is byte-identical before and after, since `mySurface` is already `ConvertApprox.Surface()` two statements earlier; no bridge site gates on this number today (`PipeShellBuilder.errorOnSurface` is info-only, and `OCCTGeomFillSweep`'s own error gate from #741 never sets `ForceApproxC1` so never reaches this branch), so `swift test` is unaffected. See [`Scripts/repro/597-geomfill-sweep-error-overwrite/`](https://github.com/SecondMouseAU/OCCTSwift/tree/main/Scripts/repro/597-geomfill-sweep-error-overwrite) for the full writeup. Upstream PR drafted but not sent (`draft-pr.md` in that directory), per `okf/policies/upstream-occt-style.md`. #597. -- **`GeomPlate_MakeApprox::ApproxError()` and `BRepOffsetAPI_MakeFilling::G0Error()` look like the obvious gate for "accepted an approximation without reading its error" (Cluster E, #668), and are not: investigated and rejected on measurement, twice, for two different reasons. This is the bridge half of #597** (the kernel half is immediately above). `occtPlateApproxSurface` (`OCCTBridge_Internal.h`, backs `OCCTShapePlatePoints`/`OCCTShapePlateCurves` + 4 more entry points) and `OCCTShapeFillBuildResult` (`OCCTBridge_Healing.mm`, backs `OCCTShapeFill`/`OCCTShapeFillWithSupport`/`OCCTShapeFillConstraints`) both run an approximation and never check the error it reports, the same shape #741 fixed for `OCCTGeomFillSweep`. Both candidate fixes (`if (approx.ApproxError() > tolerance) return null;` / `if (filling.G0Error() > tolerance) return null;`) were built and run against the real bridge, not just reasoned about, and both broke real, already-shipped, already-tested behaviour. `GeomPlate_MakeApprox::ApproxError()`'s own header doc gives away why the plate site is unfixable this way: it measures distance between the **fitted BSpline and the intermediate `GeomPlate_Surface`** the caller never sees, not fidelity to the caller's own input points. On the #571 fixture it exceeds `tolerance` by up to 5.8x while the deviation from the caller's own 25 input points stays inside tolerance throughout, and gating on it failed all 6 `Issue571PlateApproxTests`. `BRepOffsetAPI_MakeFilling::G0Error()` is the right kind of number for the fill site (`BRepFill_Filling::G0Error()`'s own header: "the maximum distance between the result and the constraints", the caller's own boundary, unlike the plate case) but is still unsafe to gate on: `FillingParameters`'s Swift default is `tolerance: Double = 1e-4`, not a fallback for an unset value, so `Tol3d = 1e-4` is the number **every** default `Shape.fill` call is built at, and legitimate, correct, higher-continuity or heavily-constrained fills routinely exceed it. Gating broke 2 of 17 `FillingSupportFaceTests` (`curvatureContinuityIsAccepted`, `internalConstraintIsNotABoundary`), both at the plain default tolerance, both asserting specific, correct, checked geometry. **A PR #751 review caught that the fill site's own supporting fixture initially understated this**: `wavyEdge()` in `occt_597_fill_g0_realistic.mm` built its sine-sampled boundary points as a single-span, full-multiplicity `Geom_BSplineCurve`, i.e. as control poles of a degree-13 Bezier, which damps a high-frequency control polygon severely (62.8% of the intended amplitude gone, measured directly from the Bernstein basis). Rebuilt on `GeomAPI_Interpolate` (which actually interpolates the sampled points, matching `OCCTCurve3DInterpolate`'s existing precedent in `OCCTBridge_Curve3D.mm`), the same boundary's `G0Error` jumps from "within the bridge's default tolerance" to exceeding even that loosest tested tolerance by ~2000x, and the accepted surface's control poles land up to ~530 units from a ~10-unit-scale boundary. `IsDone()` is still true, and `G0Error()` alone (0.2) does not communicate how far the fit has actually diverged. This sharpens rather than reverses the conclusion: there is no single `G0Error()` threshold that rejects a genuinely diverged fit like this one while keeping the two known-good, already-tested `FillingSupportFaceTests` cases passing, since both exceed `tolerance`, just by very different margins. No bridge fix shipped from this investigation; two doc comments (`OCCTBridge_Internal.h` on `occtPlateApproxSurface`, `OCCTBridge_Healing.mm` on `OCCTShapeFillBuildResult`) record why, so the same fix is not silently re-attempted after a future file breakdown (#393-#395) moves either comment. `ShapeUpgrade_UnifySameDomain`'s two bridge sites and `ShapeCustom_BSplineRestriction` were also swept and need nothing: the former exposes no error API at all (confirms #741's finding), the latter already self-polices in the kernel (declines a face's conversion outright rather than ever accepting one out of tolerance). See [`Scripts/repro/597-bridge-modeling-healing-approx-error/`](https://github.com/SecondMouseAU/OCCTSwift/tree/main/Scripts/repro/597-bridge-modeling-healing-approx-error) for the full measurement and [`Scripts/repro/572-approx-consumer-sweep/`](https://github.com/SecondMouseAU/OCCTSwift/tree/main/Scripts/repro/572-approx-consumer-sweep) for Cluster E's own census artifact. #597. +- **`GeomPlate_MakeApprox::ApproxError()` and `BRepOffsetAPI_MakeFilling::G0Error()` look like the obvious gate for "accepted an approximation without reading its error" (Cluster E, #668), and are not: investigated and rejected on measurement, twice, for two different reasons. This is the bridge half of #597** (the kernel half is immediately above). `occtPlateApproxSurface` (`OCCTBridge_Internal.h`, backs `OCCTShapePlatePoints`/`OCCTShapePlateCurves` + 4 more entry points) and `OCCTShapeFillBuildResult` (`OCCTBridge_Healing.mm`, backs `OCCTShapeFill`/`OCCTShapeFillWithSupport`/`OCCTShapeFillConstraints`) both run an approximation and never check the error it reports, the same shape #741 fixed for `OCCTGeomFillSweep`. Both candidate fixes (`if (approx.ApproxError() > tolerance) return null;` / `if (filling.G0Error() > tolerance) return null;`) were built and run against the real bridge, not just reasoned about, and both broke real, already-shipped, already-tested behaviour. `GeomPlate_MakeApprox::ApproxError()`'s own header doc gives away why the plate site is unfixable this way: it measures distance between the **fitted BSpline and the intermediate `GeomPlate_Surface`** the caller never sees, not fidelity to the caller's own input points. On the #571 fixture it exceeds `tolerance` by up to 5.8x while the deviation from the caller's own 25 input points stays inside tolerance throughout, and gating on it failed all 6 `Issue571PlateApproxTests`. `BRepOffsetAPI_MakeFilling::G0Error()` is the right kind of number for the fill site (`BRepFill_Filling::G0Error()`'s own header: "the maximum distance between the result and the constraints", the caller's own boundary, unlike the plate case) but is still unsafe to gate on: `FillingParameters`'s Swift default is `tolerance: Double = 1e-4`, not a fallback for an unset value, so `Tol3d = 1e-4` is the number **every** default `Shape.fill` call is built at, and legitimate, correct, higher-continuity or heavily-constrained fills routinely exceed it. Gating broke 2 of 17 `FillingSupportFaceTests` (`curvatureContinuityIsAccepted`, `internalConstraintIsNotABoundary`), both at the plain default tolerance, both asserting specific, correct, checked geometry. **A PR #751 review caught that the fill site's own supporting fixture initially understated this**: `wavyEdge()` in `occt_597_fill_g0_realistic.mm` built its sine-sampled boundary points as a single-span, full-multiplicity `Geom_BSplineCurve`, i.e. as control poles of a degree-13 Bezier, which damps a high-frequency control polygon severely (62.8% of the intended amplitude gone, measured directly from the Bernstein basis). Rebuilt on `GeomAPI_Interpolate` (which actually interpolates the sampled points, matching `OCCTCurve3DInterpolate`'s existing precedent in `OCCTBridge_Curve3D.mm`), the same boundary's `G0Error` jumps from "within the bridge's default tolerance" to exceeding even that loosest tested tolerance by ~2000x, and the accepted surface's control poles land up to ~530 units from a ~10-unit-scale boundary. `IsDone()` is still true, and `G0Error()` alone (0.2) does not communicate how far the fit has actually diverged. A draft of this entry then claimed no single `G0Error()` threshold could separate that diverged fit from the two regressing tests, without measuring what those two tests' own `G0Error()` actually were; measured directly (temporary debug prints in both `BRepOffsetAPI_MakeFilling::Build()` call sites, reverted after), they are 5.295e-4 (5.3x tolerance) and 1.231e-3 (12.3x tolerance), two to three orders of magnitude below the corrected fixture's ~2000x. So a fixed absolute threshold placed between those values and the diverged case (e.g. 0.01) would in fact separate them; the honest conclusion is narrower, not reversed: gating on the *caller's own requested tolerance* is unsafe and proven so by the two real regressions, while a *fixed* threshold is unjustified rather than impossible, since nobody has measured what it should be and picking one now would be inventing a number with as little basis as the `1e-4` this investigation already discredited, exactly the failure mode #726 exists to catch. No bridge fix shipped from this investigation; two doc comments (`OCCTBridge_Internal.h` on `occtPlateApproxSurface`, `OCCTBridge_Healing.mm` on `OCCTShapeFillBuildResult`) record why, so the same fix is not silently re-attempted after a future file breakdown (#393-#395) moves either comment. `ShapeUpgrade_UnifySameDomain`'s two bridge sites and `ShapeCustom_BSplineRestriction` were also swept and need nothing: the former exposes no error API at all (confirms #741's finding), the latter already self-polices in the kernel (declines a face's conversion outright rather than ever accepting one out of tolerance). See [`Scripts/repro/597-bridge-modeling-healing-approx-error/`](https://github.com/SecondMouseAU/OCCTSwift/tree/main/Scripts/repro/597-bridge-modeling-healing-approx-error) for the full measurement and [`Scripts/repro/572-approx-consumer-sweep/`](https://github.com/SecondMouseAU/OCCTSwift/tree/main/Scripts/repro/572-approx-consumer-sweep) for Cluster E's own census artifact. #597. ### Carrying OCCT source patches diff --git a/Scripts/repro/597-bridge-modeling-healing-approx-error/README.md b/Scripts/repro/597-bridge-modeling-healing-approx-error/README.md index 1f0624b0..dccd76c3 100644 --- a/Scripts/repro/597-bridge-modeling-healing-approx-error/README.md +++ b/Scripts/repro/597-bridge-modeling-healing-approx-error/README.md @@ -117,20 +117,48 @@ across. `BRepOffsetAPI_MakeFilling` reports `IsDone() == true` for a surface tha roughly 50x its input's scale, and `G0Error()` alone (0.2) does not communicate how bad the result actually is. -**This does not reverse the PR's conclusion that gating `OCCTShapeFillBuildResult` on -`G0Error() > tolerance` is unsafe; if anything it sharpens why.** The load-bearing evidence for that -conclusion is independent of this fixture: gating broke 2 of 17 `FillingSupportFaceTests` whose -surfaces are correct, already-tested, and legitimately exceed the default tolerance by a modest -factor (not 2000x). This corrected measurement adds a harder case in the same direction: a -boundary whose fit is genuinely bad (poles 50x outside the input's scale) also exceeds tolerance, -by three orders of magnitude more than the good-but-demanding test cases do, but there is no single -threshold on `G0Error()`'s raw value that rejects this case while keeping those two passing: a -threshold near `tolerance` rejects both, a threshold near this case's `0.2` would accept everything -in `FillingSupportFaceTests` too. `G0Error()` does not by itself distinguish "correct but -demanding" from "diverged", which is a stronger and more concerning version of the same -not-safe-to-gate-on-a-single-number finding, not a different one. Whether a *different* signal -(e.g. comparing the fitted surface's own scale to its boundary's) could separate the two is outside -#597's scope and not investigated here. +**This does not reverse the PR's conclusion that gating `OCCTShapeFillBuildResult` on the caller's +own requested tolerance is unsafe; that conclusion is unchanged. It does narrow a follow-on claim +that had not itself been measured.** A draft of this document asserted that no single `G0Error()` +threshold could separate a genuinely diverged fit like this corrected one from the two +`FillingSupportFaceTests` that regress when gating on `tolerance`, without ever having measured +those two tests' own `G0Error()` values. That was an assumption standing in for a measurement, the +exact failure mode this investigation exists to catch elsewhere. Measured it directly: instrumented +`OCCTShapeFillBuildResult` and `OCCTFillingBuild` (the two `BRepOffsetAPI_MakeFilling::Build()` call +sites) with a temporary debug print of `G0Error()`, ran each of the 17 `FillingSupportFaceTests` +individually (`swift test --filter `) so each printed value maps unambiguously to its test, +then reverted the instrumentation (no production code changed by this measurement). + +Effective tolerance is `1e-4` for all 17 (`FillingParameters`/`FillingSurface`'s shared default; no +test in the suite overrides it). Every `Build()` call across the suite lands on one of five distinct +values, since the fixture geometry repeats: + +| `G0Error()` | as a multiple of `tolerance` | which calls | +|---|---|---| +| `1.332268e-15` | ~0 | the one purely positional (`.g0`) fill | +| `1.431843e-05` | 0.14x | every plain tangent (`.g1`) fill, 9 calls across 9 tests | +| `2.768313e-05` | 0.28x | every flat/boundary-only (`.g0`) fill next to a tangent sibling, 4 calls | +| `5.295031e-04` | **5.3x, exceeds** | the `.g2` (curvature) branch, in both `curvatureContinuityIsAccepted` (the regressing test) and `fillingSurfaceContinuityMappingIsCorrect` (not counted as regressing, because the original gating patch touched only `OCCTShapeFillBuildResult`, not `OCCTFillingBuild`, so this `FillingSurface`-based call was never actually gated in that experiment despite exceeding tolerance by the same margin) | +| `1.231041e-03` | **12.3x, exceeds** | `internalConstraintIsNotABoundary`'s `withInterior` branch (the other regressing test) | + +Six calls produce no `Build()` at all (empty constraint list, a refused nomination, or a refusal +that poisons the whole `FillingSurface` before it reaches the builder), so they carry no `G0Error()` +to compare. + +**The two regressing values, 5.3x and 12.3x tolerance, sit two to three orders of magnitude below +the corrected fixture's ~2000x-2260x, not close to it.** This is outcome 2, not outcome 1: a fixed +absolute threshold placed anywhere in the wide gap between `1.231041e-03` and `0.20369` (for +example `0.01`) would in fact separate today's two known-good, demanding fills from the one +genuinely diverged fixture. The honest conclusion is narrower than "no threshold can work": gating +on the *caller's own requested tolerance* is unsafe and remains proven so by the two real test +regressions above; a *fixed* absolute threshold is not proven impossible, it is unjustified, since +nobody has measured what that number should be, over what range of boundaries, at what continuity, +and picking one now would be inventing a figure with exactly as little basis as the `1e-4` this +investigation already showed is not a real, enforced promise. That is precisely the failure mode +[#726](https://github.com/SecondMouseAU/OCCTSwift/issues/726) exists to catch: a fabricated number +standing in for a measured one. Whether a *different*, principled signal (a fixed threshold at some +justified value, a check relative to the boundary's own scale, or something else) could safely gate +this family is a real, open question this PR does not answer. `occt_597_fill_g0_mechanism.mm` pushes further to show the mechanism cleanly on a hyperbolic- paraboloid-ish boundary built directly from degree-3 Bezier control points (not sampled/damped, so From 328632abecb01a531052bad037f64a2e92019222 Mon Sep 17 00:00:00 2001 From: gsdali <51393997+gsdali@users.noreply.github.com> Date: Sat, 8 Aug 2026 04:26:52 +1000 Subject: [PATCH 4/4] docs(#597): the 2-of-17 regression count is a floor, and drop the em-dashes Two corrections to the repro README, both surfaced by measuring the threshold claim. **The count.** The gating experiment patched `OCCTShapeFillBuildResult` only, but there are two `Build()` call sites: `OCCTFillingBuild` is the `FillingSurface` entry point #434 converged onto the same builder, and it went ungated. `fillingSurfaceContinuityMappingIsCorrect` reaches it and exceeds tolerance by the same 5.3x, so a complete gate breaks three tests, not two. The measured table already recorded this in a row; the paragraph a reader actually lands on did not. It strengthens the conclusion rather than weakening it, which is presumably why it read as a detail. An undercount in your favour is still an undercount, and #434's whole point was that these two entry points had been reaching for the same fixes independently. **The em-dashes.** Fourteen, in a file that does not exist on the base branch, so all new work under a standard that forbids them. The one remaining `--` is inside a captured program transcript, where editing the output would be worse than the inconsistency. --- .../README.md | 33 ++++++++++--------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/Scripts/repro/597-bridge-modeling-healing-approx-error/README.md b/Scripts/repro/597-bridge-modeling-healing-approx-error/README.md index dccd76c3..fb36f7f8 100644 --- a/Scripts/repro/597-bridge-modeling-healing-approx-error/README.md +++ b/Scripts/repro/597-bridge-modeling-healing-approx-error/README.md @@ -28,8 +28,7 @@ reverted. The rest of this document is that measurement: why `occtPlateApproxSur `GeomPlate_MakeApprox::ApproxError()`'s own header doc: "the distance between the **entire target BSpline surface and the entire original surface** generated by `BuildPlateSurface`". That "original -surface" is `GeomPlate_Surface` — an intermediate, energy-minimized object the caller never sees — -not the caller's own input points or curves. `occt_597_plate_approxerror.mm` reproduces +surface" is `GeomPlate_Surface`, an intermediate, energy-minimized object the caller never sees, not the caller's own input points or curves. `occt_597_plate_approxerror.mm` reproduces `OCCTSurfacePlateThrough`'s exact two-stage pipeline (`GeomPlate_BuildPlateSurface(3,15,2)` then `occtPlateApproxSurface`'s current `maxDegree=8, maxSegments=20, dmax=tolerance*0.1` defaults) on the #571 fixture (25-point wavy plate) at four tolerances: @@ -41,12 +40,12 @@ tol=0.1 approxErr=0.579399 critErr=0.0981545 uP=9 vP=9 EXCEED tol=0.0005 approxErr=0.00472588 critErr=0.000293786 uP=30 vP=30 EXCEEDS ``` -Three of four exceed `tolerance`, and non-monotonically — the **loosest** tolerance (0.1) gives the +Three of four exceed `tolerance`, and non-monotonically, the **loosest** tolerance (0.1) gives the **worst** absolute error (0.58), because `CriterionError()` (which #571's `dmax` tunes to `tolerance`) is satisfied earlier and the fit stops subdividing sooner. That alone would be enough to make "reject when `ApproxError() > tolerance`" look necessary. -`occt_597_plate_worstdeviation.mm` adds the quantity that actually matters — the distance from the +`occt_597_plate_worstdeviation.mm` adds the quantity that actually matters, the distance from the same fitted surface back to the **caller's own 25 input points**, which is what `Surface.plateThrough`'s own doc comment promises and what `Issue571PlateApproxTests` (the only existing regression coverage for this family) actually checks: @@ -61,17 +60,17 @@ tol=0.0005 approxErr=0.00472588 worstPointDeviation=0.000242141 uP=30 AP `worstPointDeviation` stays inside `tolerance` in **every** row, including the 0.1 row where `ApproxError()` misses by 5.8x. **Confirmed directly against the real bridge, not just the standalone probe**: a patch adding `if (approx.ApproxError() > tolerance) return null;` to -`occtPlateApproxSurface` was built and run against the full `Issue571PlateApproxTests` suite — all +`occtPlateApproxSurface` was built and run against the full `Issue571PlateApproxTests` suite, all 6 tests failed with "plate build failed", including the ones using `tolerance: 0.1` and `tolerance: 0.0005`. Reverted; suite green again. Gating on `ApproxError()` would have rejected -results the existing, already-shipped, already-tested contract is fine with — the #748 lesson in +results the existing, already-shipped, already-tested contract is fine with, the #748 lesson in reverse: it is not enough for a rejection to be technically defensible, it has to be checked against what the caller was actually promised. ## Why `OCCTShapeFillBuildResult` is not fixable this way either, for a different reason `BRepFill_Filling::G0Error()` (which `BRepOffsetAPI_MakeFilling::G0Error()` forwards to) reads -`myBuilder->G0Error()` where `myBuilder` is the internal `GeomPlate_BuildPlateSurface` — i.e. it +`myBuilder->G0Error()` where `myBuilder` is the internal `GeomPlate_BuildPlateSurface`, i.e. it **does** measure distance to the caller's own constraints, unlike the plate case above. This one looked like the right metric. It still isn't safe to gate on. @@ -170,23 +169,27 @@ tighter-1e-6 tol3d=1e-06 isDone=true G0Error=9.03801e-07 extreme-1e-8 tol3d=1e-08 isDone=true G0Error=9.03801e-07 EXCEEDS TOLERANCE, ACCEPTED ANYWAY ``` -The crucial fact is `FillingParameters`'s own Swift default is `tolerance: Double = 1e-4` — not a +The crucial fact is `FillingParameters`'s own Swift default is `tolerance: Double = 1e-4`, not a sentinel meaning "unset", an explicit value every default call sends. So `Tol3d = 1e-4` is not a loose fallback, it is the number **every** `Shape.fill` call is built at unless the caller overrides it. **Confirmed directly against the real bridge**: a patch adding `if (filling.G0Error() > tolerance) return null;` to `OCCTShapeFillBuildResult` (comparing against the same effective `1e-4`/caller tolerance the builder was constructed with) broke 2 of 17 tests in -`FillingSupportFaceTests` — `curvatureContinuityIsAccepted` (bowl cap at `.g2`, `FillingParameters()` +`FillingSupportFaceTests`. **Read that 2 as a floor.** The patch gated only +`OCCTShapeFillBuildResult`, and there are two `Build()` call sites: `OCCTFillingBuild` +(`OCCTBridge_Modeling.mm:4476`) is the `FillingSurface` entry point #434 converged onto the same +builder, and it was left ungated. `fillingSurfaceContinuityMappingIsCorrect` reaches it and exceeds +tolerance by the same 5.3x, so a complete gate breaks **3** of 17, not 2. The two named below are `curvatureContinuityIsAccepted` (bowl cap at `.g2`, `FillingParameters()` default) and `internalConstraintIsNotABoundary` (a rim pulled up to an interior constraint 10 units -away) — both using the plain default tolerance, both asserting specific, correct, checked geometry +away), both using the plain default tolerance, both asserting specific, correct, checked geometry (`curvature.size.z > tangent.size.z + 0.5`, `withInterior.size.z > 0.5`). Reverted; suite green again (17/17). So `G0Error()` is the right kind of number here, but `1e-4` was never a real, verified, enforced -promise for this family — it is a bridge-chosen "reasonable starting point" that legitimate, +promise for this family, it is a bridge-chosen "reasonable starting point" that legitimate, already-shipped, higher-continuity or heavily-constrained fills routinely and correctly exceed. Picking a *different*, safer threshold to gate on would be inventing a number nobody measured, -which is exactly what #726 exists to catch — there is no way to draw that line from what is +which is exactly what #726 exists to catch, there is no way to draw that line from what is available here. ## Why `ShapeCustom_BSplineRestriction` needs no fix at all @@ -195,7 +198,7 @@ Unlike the two sites above, this class's `ConvertSurface` (`ShapeCustom_BSplineR ~line 838) already self-polices: it iteratively relaxes continuity, then escalates degree/segment caps, and only **returns `true`** (accepting a converted surface) once `anApprox.MaxError() <= myTol3d && Done`. If every escalation is exhausted without reaching that, it -`return`s `false` and the face keeps its **original, untouched** surface — never an +`return`s `false` and the face keeps its **original, untouched** surface, never an out-of-tolerance replacement. `occt_597_bsplinerestriction_selfpolice.mm` forces exactly that path (a degree-7, many-knot wavy BSpline face, an impossible `1e-14` tolerance, and `maxDeg=3`/`maxSeg=2` caps that cannot reach it): @@ -207,7 +210,7 @@ result surface handle == original wavy BSpline handle: YES (declined, kept origi `mySurfaceError`/`SurfaceError()` stays at its constructor default (`Precision::Confusion()`, exactly `1e-7`, a sentinel meaning "no accepted approximation ever updated this") rather than a -measured value, and the resulting face's surface handle is bit-identical to the input — the +measured value, and the resulting face's surface handle is bit-identical to the input, the conversion was declined, not silently accepted out of tolerance. Reading `SurfaceError()` in the bridge and gating on it would be checking a number the kernel already guarantees can never exceed `tol3d` for any *accepted* face; there is nothing for the bridge to add. @@ -222,7 +225,7 @@ than to #597's "accepted an approximation without reading its error".) Both are deliberate manual "*Builder" APIs (matching `FilletBuilder`, `SectionBuilder`, etc.): `OCCTPipeShellBuild`/`OCCTFillingBuild` return the raw OCCT success bit, and the underlying error (`errorOnSurface`, `G0Error`/`G1Error`/`G2Error`) is exposed as a **separate, opt-in accessor** for -a caller who wants to check it — the same design PipeShellBuilder already had before this +a caller who wants to check it, the same design PipeShellBuilder already had before this investigation, and the design #482/#566 gave `FillingSurface` (`refusedConstraintCount`, `hasRefusedConstraint`, all three G-errors). `#597`'s defect shape is a *convenience* function that computes an error internally and gives the caller **no way at all** to see it (what