Modeling Data - Fix AdvApp2Var Jacobi maxima written to the wrong workspace slot - #1418
Merged
dpasukhi merged 1 commit intoAug 10, 2026
Merged
Conversation
…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.
This was referenced Jul 31, 2026
Contributor
|
@gsdali , this is a regression since OCCT 7.6.0 after patch 3016a39 for
|
dpasukhi
approved these changes
Aug 10, 2026
Member
|
Thank you for the patch! |
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
mma2ce1_partitions one scratch allocation into seven consecutive buffers, of whichipt4holdsXMAXJU(the maxima of the U Jacobi polynomials) andipt5holdsXMAXJV. Bothmma2jmx_calls that fill them targetipt5:So
XMAXJUis never written.mma2ce2_still reads it atipt4, where the allocation left whatever was there — in practice zeros — and passes it tomma2er1_/mma2er2_, whose entire error model isA zero
XMAXJUzeroes every term. Two consequences, both silent:mma2ce2_(if (errmax[nd] > epsapr[nd])) can never fail on the patch interior.AdvApp2Var_ApproxAFunc2Var::MaxErrorthen reflects only the boundary-iso errorsAdvApp2Var_Patch::AddErrorsadds afterwards.mma2er2_, asked for the lowest degree whose truncation error still fits the tolerance, always answersNDMINU— 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, withIsDone()true.The write also overruns:
mma2jmx_writesndjacu + 1 - 2*(IORDRU+1)doubles, and theipt5slot is sized for thendjacvequivalent, so a request withMaxDegUwell aboveMaxDegVruns pastXMAXJVinto theVECERRslot behind it. That part is benign in practice —VECERRis re-zeroed on entry tomma2ce2_and the run stays inside the single allocation — but it is out of bounds for the buffer it was given.Reproducer
GeomConvert_ApproxSurfaceatGeomAbs_C0is where both surface. C0 givesIORDRU = 0, and a full sphere's V-boundary isos degenerate to its two poles, one coefficient each, soNDMINUis 1.Before:
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 as1.07e-4with the tolerance met. After:A bicubic Bezier at C0/C0 collapses to a 2x2 bilinear patch reporting
4.08e-15, unchanged from tolerance1e-1down to1e-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
NDMINUfloor 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 atvDegree = 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 tolerance1e-3, plus C0/C0 across five tolerances — comparing the reportedMaxError()against the real maximum deviation over a 21x21 grid of the source domain:MaxErrorMaxErrorat allThe one row still over the line after the fix is the Bezier at C0/C2, reporting
9.95221e-15against a measured9.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 (
-O0single-TU override-link) shows the mechanism:Scope
AdvApp2Var_Context's own twomma2jmx_calls, the only others in the tree, already write to separate per-direction arrays and are unaffected.GeomConvert_ApproxSurfaceis not a leaf. Live construction sites areGeomFill_Sweep.cxx:296,BRepOffset_Offset.cxx:1626,ShapeCustom_BSplineRestriction.cxx:852,ShapeConstruct.cxx:265,ShapeUpgrade_UnifySameDomain.cxx:3629,GeomLib.cxx:1517andGeomConvert_1.cxx:786/:960;ShapeCustom_ConvertToBSplinereaches it throughShapeConstruct, andGeomPlate_MakeApproxdrivesAdvApp2Var_ApproxAFunc2Vardirectly. (The two remaining mentions of the class,BRepFill_Sweep.cxx:1162andBRepFill_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::ConvertSurfaceToBSplineandShapeCustom_BSplineRestrictionboth loop the requested continuity down to 0 on failure, then decide whether to accept the result withanApprox.MaxError() <= tol, i.e. against the number that could not be exceeded — andShapeCustom_ConvertToBSplinestarts atGeomAbs_C0for 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 betweenmasterand theV8_0_0_p1tag 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