Skip to content

Modeling Data - Fix AdvApp2Var Jacobi maxima written to the wrong workspace slot - #1418

Merged
dpasukhi merged 1 commit into
Open-Cascade-SAS:IRfrom
gsdali:fix/522-advapp2var-jacobi-max-slot
Aug 10, 2026
Merged

Modeling Data - Fix AdvApp2Var Jacobi maxima written to the wrong workspace slot#1418
dpasukhi merged 1 commit into
Open-Cascade-SAS:IRfrom
gsdali:fix/522-advapp2var-jacobi-max-slot

Conversation

@gsdali

@gsdali gsdali commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

mma2ce1_ partitions one scratch allocation into seven consecutive buffers, of which ipt4 holds XMAXJU (the maxima of the U Jacobi polynomials) and ipt5 holds XMAXJV. Both mma2jmx_ calls that fill them target ipt5:

AdvApp2Var_ApproxF2var::mma2jmx_(ndjacu, iordru, &wrkar_off[ipt5]);
AdvApp2Var_ApproxF2var::mma2jmx_(ndjacv, iordrv, &wrkar_off[ipt5]);

So XMAXJU is never written. mma2ce2_ still reads it at ipt4, where the allocation left whatever was there — in practice zeros — and passes it to mma2er1_/mma2er2_, whose entire error model is

error += |PATJAC(i,j)| * XMAXJU(i - 2*(IORDRU+1)) * XMAXJV(j - 2*(IORDRV+1))

A zero XMAXJU zeroes every term. Two consequences, both silent:

  1. The approximation error of a patch is reported as exactly 0 no matter what the discarded Jacobi coefficients are, so the tolerance test in mma2ce2_ (if (errmax[nd] > epsapr[nd])) can never fail on the patch interior. AdvApp2Var_ApproxAFunc2Var::MaxError then reflects only the boundary-iso errors AdvApp2Var_Patch::AddErrors adds afterwards.
  2. mma2er2_, asked for the lowest degree whose truncation error still fits the tolerance, always answers NDMINU — the floor derived from the constraint order and the neighbouring isos — because every candidate scores 0. Where that floor is low the fit collapses onto it and the caller is handed a surface nowhere near its input, with IsDone() true.

The write also overruns: mma2jmx_ writes ndjacu + 1 - 2*(IORDRU+1) doubles, and the ipt5 slot is sized for the ndjacv equivalent, so a request with MaxDegU well above MaxDegV runs past XMAXJV into the VECERR slot behind it. That part is benign in practice — VECERR is re-zeroed on entry to mma2ce2_ and the run stays inside the single allocation — but it is out of bounds for the buffer it was given.

Reproducer

GeomConvert_ApproxSurface at GeomAbs_C0 is where both surface. C0 gives IORDRU = 0, and a full sphere's V-boundary isos degenerate to its two poles, one coefficient each, so NDMINU is 1.

gp_Ax3 ax3(gp_Pnt(0, 0, 0), gp_Dir(0, 0, 1));
Handle(Geom_SphericalSurface) sphere = new Geom_SphericalSurface(ax3, 10.0);
GeomConvert_ApproxSurface a(sphere, 1e-3, GeomAbs_C0, GeomAbs_C0, 8, 8, 100, 0);
Handle(Geom_BSplineSurface) bs = a.Surface();

Before:

isDone=1 maxError=0.000106971  uDeg=1 vDeg=7 uPoles=2 vPoles=8
real max deviation over the source domain: 19.9999  (at u=3.14159 v=0)

Two poles at degree 1 across the sphere's full [0, 2*pi] of longitude is a straight line through the sphere, deviating by its own diameter of 20, reported as 1.07e-4 with the tolerance met. After:

isDone=1 maxError=0.000261108  uDeg=7 vDeg=7 uPoles=15 vPoles=8
real max deviation over the source domain: 0.000120909  (at u=1.5708 v=0)

A bicubic Bezier at C0/C0 collapses to a 2x2 bilinear patch reporting 4.08e-15, unchanged from tolerance 1e-1 down to 1e-7, because the requested tolerance is compared against a number that is always zero. After the fix it is reproduced exactly at degree 3x3 at every one of those tolerances.

C1 and C2 hide the collapse — their NDMINU floor is already 8 — but not the misreported error, which was never specific to C0. Degree collapse per se is not the defect either: a cylinder trimmed in V legitimately fits at vDegree = 1, and does so before and after.

Measurements

Sweep of 98 requests — 7 surface families (sphere, V-trimmed sphere, torus, trimmed cylinder, trimmed cone, surface of revolution, 4x4 Bezier) x all 9 (uContinuity, vContinuity) combinations of C0/C1/C2 at tolerance 1e-3, plus C0/C0 across five tolerances — comparing the reported MaxError() against the real maximum deviation over a 21x21 grid of the source domain:

before after
real deviation > 10x reported MaxError 12 of 98 0 of 98
real deviation > reported MaxError at all 17 of 98 1 of 98
worst ratio of real deviation to reported error 3.4e13 1.0018

The one row still over the line after the fix is the Bezier at C0/C2, reporting 9.95221e-15 against a measured 9.96978e-15 — a surface reproduced exactly, disagreeing at the last bit.

Reported errors rise slightly everywhere, which is the interior contribution being counted for the first time. Degrees rise only where the collapse was happening.

Dumping the buffer directly (-O0 single-TU override-link) shows the mechanism:

before:  xmaxju[8] = 0 0 0 0 0 0 0 0
after:   xmaxju[8] = 0.9682 0.986 1.078 1.173 1.265 1.352 1.434 1.513

Scope

AdvApp2Var_Context's own two mma2jmx_ calls, the only others in the tree, already write to separate per-direction arrays and are unaffected.

GeomConvert_ApproxSurface is not a leaf. Live construction sites are GeomFill_Sweep.cxx:296, BRepOffset_Offset.cxx:1626, ShapeCustom_BSplineRestriction.cxx:852, ShapeConstruct.cxx:265, ShapeUpgrade_UnifySameDomain.cxx:3629, GeomLib.cxx:1517 and GeomConvert_1.cxx:786/:960; ShapeCustom_ConvertToBSpline reaches it through ShapeConstruct, and GeomPlate_MakeApprox drives AdvApp2Var_ApproxAFunc2Var directly. (The two remaining mentions of the class, BRepFill_Sweep.cxx:1162 and BRepFill_Filling.cxx:712, are both inside comment blocks.)

Most pass C1 or C2, where the collapse cannot happen, but the always-zero interior error affected all of them. The healing paths reach C0 on purpose: ShapeConstruct::ConvertSurfaceToBSpline and ShapeCustom_BSplineRestriction both loop the requested continuity down to 0 on failure, then decide whether to accept the result with anApprox.MaxError() <= tol, i.e. against the number that could not be exceeded — and ShapeCustom_ConvertToBSpline starts at GeomAbs_C0 for any offset surface (ShapeCustom_ConvertToBSpline.cxx:148, a 1999 workaround for a hang) before handing off to the first of those.

Found while building surface-approximation parity tests in the OCCTSwift wrapper. Carried there as patch 0019; the file is byte-identical between master and the V8_0_0_p1 tag that project pins, so this is the same change on both. Full writeup, both reproducers and the before/after sweep transcripts: https://github.com/SecondMouseAU/OCCTSwift/tree/main/Scripts/repro/522-approx-c0-collapse

…kspace slot

mma2ce1_ partitions one scratch allocation into seven buffers, ipt4 holding XMAXJU
and ipt5 holding XMAXJV, but both mma2jmx_ calls target ipt5. XMAXJU is therefore
never written, and mma2ce2_ reads uninitialized memory at ipt4.

mma2er1_ and mma2er2_ scale every coefficient by XMAXJU(i) * XMAXJV(j), so a zeroed
XMAXJU makes each patch report zero approximation error. The tolerance test in
mma2ce2_ then never fails on the patch interior, and the degree-reduction search
always returns NDMINU because every candidate degree scores zero.

GeomConvert_ApproxSurface at GeomAbs_C0 shows both. A sphere of radius 10 at
tolerance 1e-3 is returned as a degree-1, 2-pole-in-U B-spline spanning the whole
2*pi of longitude, deviating by 20, with IsDone() true and MaxError() 1.07e-4.

The write is also out of bounds: mma2jmx_ writes ndjacu + 1 - 2*(IORDRU+1) doubles
into a slot sized for the ndjacv equivalent.
@gkv311

gkv311 commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

@gsdali , this is a regression since OCCT 7.6.0 after patch 3016a39 for

0032495: Coding rules - eliminate CLang UndefinedBehaviorSanitizer warnings
pload MODELING

sphere s 10
approxsurf r s 1.e-3 0 0 8 8 100
dump r
donly r
axo
fit

#set d [dumpjson r]
#regexp {"udeg": ([0-9]*)} $d full udeg
#regexp {"vdeg": ([0-9]*)} $d full vdeg
#if { $udeg != 7 } { puts "Error: wrong U degree" }
#if { $vdeg != 7 } { puts "Error: wrong V degree" }

@github-project-automation github-project-automation Bot moved this from Todo to Integration in Maintenance Aug 10, 2026
@dpasukhi
dpasukhi merged commit 07e6d8d into Open-Cascade-SAS:IR Aug 10, 2026
18 checks passed
@github-project-automation github-project-automation Bot moved this from Integration to Done in Maintenance Aug 10, 2026
@dpasukhi

Copy link
Copy Markdown
Member

Thank you for the patch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants