Modeling Data - Fix GCPnts arc-length sampler point counts

[gsdali]

Description

Two defects in the GCPnts arc-length samplers, both about the requested point count.

NbPoints() is not bounded by the requested count

GCPnts_UniformAbscissa::initialize sizes myParams at theNbPoints + 5 and the walk fills it
until it reaches the end parameter or runs out of room, setting myNbPoints to whatever it reached.
A caller that sizes its own buffer from the requested count rather than from NbPoints() is handed
more points than it asked for. GCPnts_QuasiUniformAbscissa inherits this for every curve that is
neither Bezier nor BSpline, since it forwards to GCPnts_UniformAbscissa for those, so the same
class returns exactly theNbPoints on a Bezier and possibly more on an ellipse.

The cause is a tolerance mismatch rather than an off-by-one. Perform terminates on

if (std::abs(aUi - aUU2) <= theEPSILON)

where theEPSILON is theC.Resolution(theTol), a parametric tolerance derived from a 3D one using
the curve's largest derivative. On an ellipse with major radius 1e6 and minor radius 1e-3 that is
about 1e-13, while the local derivative at the end of that curve is 1e-3, so the parametric tolerance
that actually corresponds to 1e-7 in 3D there is about 1e-4. The test is around nine orders of
magnitude too tight at that end of the curve. The walk stops 1.557e-08 short, does not treat that as
done, takes one more step and snaps it to the end.

The appended point is a duplicate: 1.175e-10 from its neighbour in 3D. On that curve, 22 of the
counts from 2 to 60 return one point more than requested, for both classes.

Perform now also accepts a point that coincides with the end within the caller's 3D tolerance, not
only one close in parameter. The tolerance is passed in alongside the parametric one, the end point
is evaluated once before the walk, and the distance test is gated behind aUU2 - aUi < aDelta so it
runs on the final step rather than on every step.

Clamping myNbPoints to theNbPoints was the other option and is worse: the surplus point is the
one carrying the exact end parameter, so clamping leaves the distribution stopping short of the
curve.

A point count below 2 stores out of bounds

Both classes document theNbPoints >= 2 and enforce it with Standard_ConstructionError_Raise_if,
which compiles to nothing when No_Exception is defined, as it is for Release builds with
BUILD_RELEASE_DISABLE_EXCEPTIONS (the default). GCPnts_QuasiUniformAbscissa::initialize then
allocates NCollection_HArray1<double>(1, theNbPoints), an empty range for such a count, and the
next statement is an unconditional myParams->SetValue(1, theU1). SetValue's own bounds check is a
Raise_if too, so the store lands out of bounds.

Reproduced with theNbPoints = 0 and with a negative count, on a 4-pole Bezier and an 8-point
BSpline fit. GCPnts_UniformAbscissa has the same missing precondition without the out-of-bounds
store: it answers a request for zero points with five.

Both classes now leave the object not done for a count below 2. The Raise_if calls stay, so a build
with exceptions enabled throws exactly as before; this only stops the undefined behaviour in builds
where the check is compiled out.

No public API signature changes.

Type of change

• Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

A harness fingerprints both classes across 17 curve types (a line, circles of radius 1e-6 to 1e7, a
1e6 x 1e-3 ellipse, a 5 x 2 ellipse, a hyperbola, a parabola, a 2-pole and a 4-pole Bezier, an
8-point and a 40-point BSpline, an offset circle, an offset BSpline, a trimmed circle, and a
half-period slice of the pathological ellipse) and counts 2 to 200, recording the point count, the
first and last parameter and a digest of the whole parameter list, so a stock run and a patched run
can be diffed result by result.

• 6766 configurations measured. 232 results change, and they are exactly the 232 that were
  returning more points than requested. Every other result is identical parameter for parameter,
  and on the changed ones the last parameter is still exactly the end.
• Requests that over-returned: 22 counts to 0.
• Degenerate counts (0, 1, negative) on every curve now return IsDone() == false for both classes,
  in place of an out-of-bounds store, a five-point answer or a one-point answer depending on which
  curve and which class was called.
• clang-format clean on both touched files.

Also built into a downstream project's kernel and run against its full suite (4842 tests): clean.

Note for anyone reproducing the second defect: build with -DNo_Exception (or a Release build with
BUILD_RELEASE_DISABLE_EXCEPTIONS=ON). Without it the Raise_if is live and throws, hiding the
out-of-bounds store that a shipped Release build performs.

Reproducers and the full write-up:
https://github.com/SecondMouseAU/OCCTSwift/tree/main/Scripts/repro/555-gcpnts-count-contract

Checklist:

• My code follows the code style of this project (clang-format clean on every touched file)
• My change requires a change to the documentation

[gkv311]

@gsdali ,

  Standard_ConstructionError_Raise_if(
    theNbPoints <= 1,
    "GCPnts_QuasiUniformAbscissa::Initialize(), number of points should be >= 2");
+  if (theNbPoints <= 1)
+  {
+    // The check above is compiled out in a build defining No_Exception. Without this the code
+    // below stores into myParams, which is allocated empty for such a count.
+    myDone     = false;
+    myNbPoints = 0;
+    return;
+  }

Macros like _Raise_if protected with No_Exception were designed to designate developers errors like asserts - e.g. they are disabled by macros No_Exception to avoid overhead in Release builds. But places like GCPnts_UniformAbscissa are doubtfully make sense to optimize. I would say either - to replace Standard_ConstructionError_Raise_if with if (...) throw Standard_ConstructionError(...); or to remove this duplicating check and keep only myDone=false; in your patch.

      if (std::abs(aUi - aUU2) <= theEPSILON
          || (aUU2 - aUi < aDelta && theC.Value(aUi).Distance(aPEnd) <= theTol3d))

Normally, OCCT tries to avoid an overhead of calling std::sqrt() in loop and use SquareDistance() instead of Distance() and compare with const double aTol2 = theTol*theTol calculated outside of the loop.

theTol3d name might look misleading as this template function is also used for Adaptor2d_Curve2d, so probably it would be better renaming it to theTol.

[dpasukhi]

conflict with the previous checks of Standard_ConstructionError_Raise_if

[dpasukhi]

Too much text which is unreadable without PR context, please concentrate the description in shorter form

[dpasukhi]

Too much text which is unreadable without PR context, please concentrate the description in shorter form