Bspline optim

[dpasukhi]

No description provided.

[dpasukhi]

@coderabbitai full review

[coderabbitai]

✅ Actions performed

Full review triggered.

[coderabbitai]

Walkthrough

This pull request refactors the internal data structures of B-spline and Bezier curve and surface classes across 2D and 3D geometry libraries. The primary changes involve replacing legacy handle-based storage (HArray1/HArray2) with direct member arrays (NCollection_Array1/Array2) prefixed with "my". The public API is modernized by adding const-reference accessors for knots, poles, weights, and flat knots, while marking old output-parameter methods as deprecated. Enhanced NCollection_Array2 adds resizing control via ResizeWithTrim and internal helper methods. Geometry grid evaluation code is simplified to use the new internal accessors. Extensive new unit tests validate curve and surface operations across evaluation, manipulation, and geometric transformations.

🚥 Pre-merge checks | ❌ 3

❌ Failed checks (1 warning, 2 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 59.27% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check	❓ Inconclusive	The title 'Bspline optim' is vague and generic, failing to convey meaningful information about the changeset despite the PR implementing extensive refactors and optimizations.	Provide a more descriptive title that captures the main intent, e.g., 'Refactor B-spline/Bezier classes to use internal data structures and const-reference accessors' or 'Optimize B-spline/Bezier implementations with data structure refactors and new tests'.
Description check	❓ Inconclusive	No pull request description was provided by the author, making it impossible to evaluate whether documentation adequately explains the changes.	Add a comprehensive pull request description explaining the objectives, key changes (refactors, new tests, API updates), and impact of the modifications.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

src/ModelingData/TKG2d/Geom2d/Geom2d_BSplineCurve_1.cxx (1)

493-517: ⚠️ Potential issue | 🟡 Minor

Pre-existing copy-paste error in LocalDN error message.

Line 498: The Standard_DomainError_Raise_if message says "Geom2d_BSplineCurve::LocalD3" but this is the LocalDN method. This is a pre-existing issue (the same error exists in the 3D counterpart), not introduced by this refactor, but worth fixing if you're already touching this file.

Proposed fix

-  Standard_DomainError_Raise_if(FromK1 == ToK2, "Geom2d_BSplineCurve::LocalD3");
+  Standard_DomainError_Raise_if(FromK1 == ToK2, "Geom2d_BSplineCurve::LocalDN");

src/ModelingData/TKG3d/Geom/Geom_BezierSurface.cxx (1)

1192-1230: ⚠️ Potential issue | 🟡 Minor

SetWeightRow validation is weaker than SetWeightCol — allows partial row updates.

SetWeightCol (line 1156) enforces CPoleWeights.Length() != myPoles.ColLength() — an exact length match. In contrast, SetWeightRow (lines 1197–1198) only checks that bounds fall within range, so a caller can supply a subset of weights for the row. The remaining weights retain stale or default values, then Rational() is called on the full matrix, potentially yielding incorrect results.

Consider aligning the validation to also enforce a full-row length match, as done in SetWeightCol:

Proposed fix

 void Geom_BezierSurface::SetWeightRow(const int                         UIndex,
                                       const NCollection_Array1<double>& CPoleWeights)
 {
   if (UIndex < 1 || UIndex > myPoles.ColLength())
     throw Standard_OutOfRange("Geom_BezierSurface::SetWeightRow");
-  if (CPoleWeights.Lower() < 1 || CPoleWeights.Lower() > myPoles.RowLength()
-      || CPoleWeights.Upper() < 1 || CPoleWeights.Upper() > myPoles.RowLength())
+  if (CPoleWeights.Length() != myPoles.RowLength())
   {
     throw Standard_ConstructionError("Geom_BezierSurface::SetWeightRow");
   }

🤖 Fix all issues with AI agents

In `@src/FoundationClasses/TKernel/NCollection/NCollection_Array2.hxx`:
- Around line 388-432: resizeImpl currently performs a 2D min(rows)*min(cols)
copy for the linear path, which drops trailing elements when total sizes differ;
modify resizeImpl (template parameter thePreserve2D) so that when thePreserve2D
== false you compute totalOld = mySizeRow * mySizeCol and totalNew = aNewNbRows
* aNewNbCols and copy count = std::min(totalOld, totalNew) elements in 1D order
from anOldPointer[i] into NCollection_Array1<TheItemType>::at(i) (i.e., a single
loop copying anOldPointer[i] -> at(i)); leave the existing 2D-preserve behavior
(and aOldStride) unchanged for thePreserve2D == true.

In `@src/ModelingData/TKG3d/Geom/Geom_BezierSurface.cxx`:
- Line 608: The call Init(myPoles, (myURational || myVRational) ? &myWeights :
nullptr) passes member objects into Init which then mutates them (via
myPoles.Resize), causing self-reference/data-loss risk; instead, after calling
Increase(...) do not call Init with member references—either update rationality
flags and directly clear or allocate myWeights as needed (modify myWeights
in-place based on myURational/myVRational) or pass a safe temporary/copy (e.g.,
a local const copy of myWeights or nullptr) into Init; locate the usage in
Geom_BezierSurface where Increase and Init are called and change it so Init
never receives pointers/references to members that Init itself may resize (use
direct post-Increase handling of myWeights or pass a copy).

In `@src/ModelingData/TKG3d/Geom/Geom_BSplineCurve_1.cxx`:
- Around line 731-771: The knot-comparison loop reuses the pole loop index so
knots with indices <= myPoles.Length() are never checked; in the method IsEqual
(compare uses myPoles, myKnots, myMults and theOther->Knot), reset or use a
fresh loop index for the knot comparison—e.g. start a new loop with i = 1 (or
use a new variable j) and iterate up to myKnots.Length(), comparing
fabs(myKnots(i) - theOther->Knot(i)) with Precision::Parametric(thePreci) so all
knots are validated.

🧹 Nitpick comments (6)

src/ModelingData/TKG3d/GeomGridEval/GeomGridEval_BezierSurface.cxx (1)

208-261: Consider using IsURational()/IsVRational() instead of aWeights != nullptr for the rationality flags.

Lines 239–240 pass isRational (derived from aWeights != nullptr) for both URational and VRational parameters of BSplSLib::D3. While this produces correct results (uniform weights yield a no-op division), Geom_BezierSurface tracks U/V rationality separately. Using the geometry's own IsURational()/IsVRational() would be more precise and could avoid unnecessary rational arithmetic when weights are uniform in one direction.

The same applies to EvaluateGridDN at lines 328–329.

Proposed refinement

-  const bool                         isRational  = (aWeights != nullptr);
+  const bool                         isURational = myGeom->IsURational();
+  const bool                         isVRational = myGeom->IsVRational();

Then use isURational and isVRational in the BSplSLib::D3 and BSplSLib::DN calls.

src/ModelingData/TKG3d/Geom/Geom_BezierSurface.cxx (5)

363-372: Copy constructor copies derivative cache but marks it invalid.

myUMaxDerivInv and myVMaxDerivInv are copied from theOther, yet myMaxDerivInvOk is forced to false, making those copied values dead until recomputed. If the source object had a valid cache, consider propagating theOther.myMaxDerivInvOk to avoid an unnecessary future recomputation in Resolution().

Proposed fix

 Geom_BezierSurface::Geom_BezierSurface(const Geom_BezierSurface& theOther)
     : myPoles(theOther.myPoles),
       myWeights(theOther.myWeights),
       myURational(theOther.myURational),
       myVRational(theOther.myVRational),
       myUMaxDerivInv(theOther.myUMaxDerivInv),
       myVMaxDerivInv(theOther.myVMaxDerivInv),
-      myMaxDerivInvOk(false)
+      myMaxDerivInvOk(theOther.myMaxDerivInvOk)
 {
 }

---

422-448: Rationality check duplicates the Rational() static helper.

Lines 422–448 manually iterate over weights to set myURational/myVRational, replicating the Rational() function defined at lines 57–85. Consider replacing this block with a call to Rational(PoleWeights, myURational, myVRational) to reduce duplication and keep the logic in one place.

Proposed simplification

-  int I, J;
-  myURational = false;
-  myVRational = false;
-  J         = PoleWeights.LowerCol();
-  while (!myVRational && J <= PoleWeights.UpperCol())
-  {
-    I = PoleWeights.LowerRow();
-    while (!myVRational && I <= PoleWeights.UpperRow() - 1)
-    {
-      myVRational = (std::abs(PoleWeights(I, J) - PoleWeights(I + 1, J))
-                   > Epsilon(std::abs(PoleWeights(I, J))));
-      I++;
-    }
-    J++;
-  }
-  I = PoleWeights.LowerRow();
-  while (!myURational && I <= PoleWeights.UpperRow())
-  {
-    J = PoleWeights.LowerCol();
-    while (!myURational && J <= PoleWeights.UpperCol() - 1)
-    {
-      myURational = (std::abs(PoleWeights(I, J) - PoleWeights(I, J + 1))
-                   > Epsilon(std::abs(PoleWeights(I, J))));
-      J++;
-    }
-    I++;
-  }
+  Rational(PoleWeights, myURational, myVRational);

---

1997-2031: BezierUMults and BezierVMults share identical static tables.

Both methods define the same THE_DATA[26][2] array and the same THE_MULTS lambda. Consider extracting them into a single file-level static (or a shared private helper) and indexing it by ColLength()-1 / RowLength()-1 respectively.

---

1997-2012: Hardcoded table size (26) is coupled to BSplCLib::MaxDegree() without a compile-time or runtime guard.

THE_DATA and THE_MULTS have exactly 26 entries (indices 0–25), matching BSplCLib::MaxDegree() == 25. If MaxDegree() is ever increased, THE_MULTS[myPoles.ColLength() - 1] silently goes out of bounds. Consider adding a defensive assertion:

Proposed guard

 const NCollection_Array1<int>& Geom_BezierSurface::BezierUMults() const
 {
   Standard_ProgramError_Raise_if(myPoles.IsEmpty(), "Geom_BezierSurface: empty poles");
+  Standard_ProgramError_Raise_if(myPoles.ColLength() - 1 >= 26,
+                                 "Geom_BezierSurface: degree exceeds precomputed table size");
   static const int THE_DATA[26][2] = {

---

2035-2059: Consolidate duplicate static table initialization in BezierUFlatKnots and BezierVFlatKnots.

Both methods create identical THE_FKNOTS static tables. A single shared static would halve the footprint and improve maintainability. The underlying data from BSplCLib::FlatBezierKnots() is a static const array guaranteed to persist, so the non-owning NCollection_Array1 views are safe.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Linear resize path can drop data on shape changes.

resizeImpl<false> only copies min(rows)*min(cols) elements. For reshapes with equal total size (e.g., 4×6 → 6×4), this loses trailing elements even though the API contract says “linear (row‑major) order.” Consider copying min(oldSize, newSize) elements in 1D order for the linear path.

💡 Suggested fix (1D copy for linear path)

-    const size_t aNbRowsToCopy = (std::min)(mySizeRow, aNewNbRows);
-    const size_t aNbColsToCopy = (std::min)(mySizeCol, aNewNbCols);
-    const size_t aOldStride    = thePreserve2D ? mySizeCol : aNbColsToCopy;
+    const size_t aOldNbRows    = mySizeRow;
+    const size_t aOldNbCols    = mySizeCol;
+    const size_t aNbRowsToCopy = (std::min)(aOldNbRows, aNewNbRows);
+    const size_t aNbColsToCopy = (std::min)(aOldNbCols, aNewNbCols);
+    const size_t aOldStride    = thePreserve2D ? aOldNbCols : aNbColsToCopy;
@@
-    for (size_t aRowIter = 0; aRowIter < aNbRowsToCopy; ++aRowIter)
-    {
-      for (size_t aColIter = 0; aColIter < aNbColsToCopy; ++aColIter)
-      {
-        NCollection_Array1<TheItemType>::at(aRowIter * aNewNbCols + aColIter) =
-          std::move(anOldPointer[aRowIter * aOldStride + aColIter]);
-      }
-    }
+    if constexpr (thePreserve2D)
+    {
+      for (size_t aRowIter = 0; aRowIter < aNbRowsToCopy; ++aRowIter)
+      {
+        for (size_t aColIter = 0; aColIter < aNbColsToCopy; ++aColIter)
+        {
+          NCollection_Array1<TheItemType>::at(aRowIter * aNewNbCols + aColIter) =
+            std::move(anOldPointer[aRowIter * aOldStride + aColIter]);
+        }
+      }
+    }
+    else
+    {
+      const size_t aNbToCopy =
+        (std::min)(aOldNbRows * aOldNbCols, aNewNbRows * aNewNbCols);
+      for (size_t aIdx = 0; aIdx < aNbToCopy; ++aIdx)
+      {
+        NCollection_Array1<TheItemType>::at(aIdx) = std::move(anOldPointer[aIdx]);
+      }
+    }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	//! Internal resize with data copy.
	//! @tparam thePreserve2D if true, copies the common sub-matrix preserving
	//! 2D element positions (row, col); if false, copies elements in linear order.
	template <bool thePreserve2D>
	void resizeImpl(int theRowLower, int theRowUpper, int theColLower, int theColUpper)
	{
	Standard_RangeError_Raise_if(theRowUpper < theRowLower || theColUpper < theColLower,
	"NCollection_Array2::Resize");
	const size_t aNewNbRows = theRowUpper - theRowLower + 1;
	const size_t aNewNbCols = theColUpper - theColLower + 1;
	if (mySizeRow == aNewNbRows && mySizeCol == aNewNbCols)
	{
	NCollection_Array1<TheItemType>::Resize(
	BeginPosition(theRowLower, theRowUpper, theColLower, theColUpper),
	LastPosition(theRowLower, theRowUpper, theColLower, theColUpper),
	false);
	mySizeRow = theRowUpper - theRowLower + 1;
	mySizeCol = theColUpper - theColLower + 1;
	myLowerRow = theRowLower;
	myLowerCol = theColLower;
	NCollection_Array1<TheItemType>::UpdateLowerBound(
	BeginPosition(theRowLower, theRowUpper, theColLower, theColUpper));
	return;
	}
	if (mySizeRow == 0 || mySizeCol == 0)
	{
	resizeNoData(theRowLower, theRowUpper, theColLower, theColUpper);
	return;
	}
	const size_t aNewNbRows = theRowUpper - theRowLower + 1;
	const size_t aNewNbCols = theColUpper - theColLower + 1;
	const size_t aNbRowsToCopy = (std::min)(mySizeRow, aNewNbRows);
	const size_t aNbColsToCopy = (std::min)(mySizeCol, aNewNbCols);
	const size_t aOldStride = thePreserve2D ? mySizeCol : aNbColsToCopy;
	NCollection_Array2<TheItemType> aTmpMovedCopy(std::move(*this));
	TheItemType* anOldPointer = &aTmpMovedCopy.ChangeFirst();
	NCollection_Array1<TheItemType>::Resize(
	BeginPosition(theRowLower, theRowUpper, theColLower, theColUpper),
	LastPosition(theRowLower, theRowUpper, theColLower, theColUpper),
	false);
	mySizeRow = aNewNbRows;
	mySizeCol = aNewNbCols;
	myLowerRow = theRowLower;
	myLowerCol = theColLower;
	size_t aOldInter = 0;
	mySizeRow = aNewNbRows;
	mySizeCol = aNewNbCols;
	myLowerRow = theRowLower;
	myLowerCol = theColLower;
	for (size_t aRowIter = 0; aRowIter < aNbRowsToCopy; ++aRowIter)
	{
	for (size_t aColIter = 0; aColIter < aNbColsToCopy; ++aColIter)
	{
	NCollection_Array1<TheItemType>::at(aRowIter * aNewNbCols + aColIter) =
	std::move(anOldPointer[aOldInter++]);
	std::move(anOldPointer[aRowIter * aOldStride + aColIter]);
	}
	}
	//! Internal resize with data copy.
	//! `@tparam` thePreserve2D if true, copies the common sub-matrix preserving
	//! 2D element positions (row, col); if false, copies elements in linear order.
	template <bool thePreserve2D>
	void resizeImpl(int theRowLower, int theRowUpper, int theColLower, int theColUpper)
	{
	Standard_RangeError_Raise_if(theRowUpper < theRowLower || theColUpper < theColLower,
	"NCollection_Array2::Resize");
	const size_t aNewNbRows = theRowUpper - theRowLower + 1;
	const size_t aNewNbCols = theColUpper - theColLower + 1;
	if (mySizeRow == aNewNbRows && mySizeCol == aNewNbCols)
	{
	myLowerRow = theRowLower;
	myLowerCol = theColLower;
	NCollection_Array1<TheItemType>::UpdateLowerBound(
	BeginPosition(theRowLower, theRowUpper, theColLower, theColUpper));
	return;
	}
	if (mySizeRow == 0 || mySizeCol == 0)
	{
	resizeNoData(theRowLower, theRowUpper, theColLower, theColUpper);
	return;
	}
	const size_t aOldNbRows = mySizeRow;
	const size_t aOldNbCols = mySizeCol;
	const size_t aNbRowsToCopy = (std::min)(aOldNbRows, aNewNbRows);
	const size_t aNbColsToCopy = (std::min)(aOldNbCols, aNewNbCols);
	const size_t aOldStride = thePreserve2D ? aOldNbCols : aNbColsToCopy;
	NCollection_Array2<TheItemType> aTmpMovedCopy(std::move(*this));
	TheItemType* anOldPointer = &aTmpMovedCopy.ChangeFirst();
	NCollection_Array1<TheItemType>::Resize(
	BeginPosition(theRowLower, theRowUpper, theColLower, theColUpper),
	LastPosition(theRowLower, theRowUpper, theColLower, theColUpper),
	false);
	mySizeRow = aNewNbRows;
	mySizeCol = aNewNbCols;
	myLowerRow = theRowLower;
	myLowerCol = theColLower;
	if constexpr (thePreserve2D)
	{
	for (size_t aRowIter = 0; aRowIter < aNbRowsToCopy; ++aRowIter)
	{
	for (size_t aColIter = 0; aColIter < aNbColsToCopy; ++aColIter)
	{
	NCollection_Array1<TheItemType>::at(aRowIter * aNewNbCols + aColIter) =
	std::move(anOldPointer[aRowIter * aOldStride + aColIter]);
	}
	}
	}
	else
	{
	const size_t aNbToCopy =
	(std::min)(aOldNbRows * aOldNbCols, aNewNbRows * aNewNbCols);
	for (size_t aIdx = 0; aIdx < aNbToCopy; ++aIdx)
	{
	NCollection_Array1<TheItemType>::at(aIdx) = std::move(anOldPointer[aIdx]);
	}
	}

🤖 Prompt for AI Agents

In `@src/FoundationClasses/TKernel/NCollection/NCollection_Array2.hxx` around
lines 388 - 432, resizeImpl currently performs a 2D min(rows)*min(cols) copy for
the linear path, which drops trailing elements when total sizes differ; modify
resizeImpl (template parameter thePreserve2D) so that when thePreserve2D ==
false you compute totalOld = mySizeRow * mySizeCol and totalNew = aNewNbRows *
aNewNbCols and copy count = std::min(totalOld, totalNew) elements in 1D order
from anOldPointer[i] into NCollection_Array1<TheItemType>::at(i) (i.e., a single
loop copying anOldPointer[i] -> at(i)); leave the existing 2D-preserve behavior
(and aOldStride) unchanged for thePreserve2D == true.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Init(myPoles, &myWeights) passes self-references — fragile and data-loss risk.

Init takes its arguments by const reference/pointer, but here myPoles and myWeights are aliased to the member fields being modified inside Init. Inside Init (line 1945), myPoles.Resize(..., false) would discard data if dimensions don't match — but then myPoles = thePoles reads from the same (now-garbage) object.

Today this is safe only because Resize is a no-op when dimensions already match (which they do after Increase has already resized via std::move). However, this is fragile and will silently break if Resize behavior changes or if dimensions diverge.

A safer approach: after Increase, just re-check rationality and clean up weights directly, without funneling through Init.

Proposed fix

-  Init(myPoles, (myURational || myVRational) ? &myWeights : nullptr);
+  // Re-check rationality after degree increase
+  if (myURational || myVRational)
+  {
+    Rational(myWeights, myURational, myVRational);
+    if (!(myURational || myVRational))
+    {
+      myWeights = NCollection_Array2<double>();
+    }
+  }
+  myMaxDerivInvOk = false;

🤖 Prompt for AI Agents

In `@src/ModelingData/TKG3d/Geom/Geom_BezierSurface.cxx` at line 608, The call
Init(myPoles, (myURational || myVRational) ? &myWeights : nullptr) passes member
objects into Init which then mutates them (via myPoles.Resize), causing
self-reference/data-loss risk; instead, after calling Increase(...) do not call
Init with member references—either update rationality flags and directly clear
or allocate myWeights as needed (modify myWeights in-place based on
myURational/myVRational) or pass a safe temporary/copy (e.g., a local const copy
of myWeights or nullptr) into Init; locate the usage in Geom_BezierSurface where
Increase and Init are called and change it so Init never receives
pointers/references to members that Init itself may resize (use direct
post-Increase handling of myWeights or pass a copy).

[coderabbitai]

⚠️ Potential issue | 🟠 Major

IsEqual skips knot comparison due to index reuse.

The knot loop continues from the pole index, so early knots are never compared. This can incorrectly report equality for curves with differing knot vectors.

✅ Suggested fix

-  for (; i <= myKnots.Length(); i++)
+  for (i = 1; i <= myKnots.Length(); i++)

🤖 Prompt for AI Agents

In `@src/ModelingData/TKG3d/Geom/Geom_BSplineCurve_1.cxx` around lines 731 - 771,
The knot-comparison loop reuses the pole loop index so knots with indices <=
myPoles.Length() are never checked; in the method IsEqual (compare uses myPoles,
myKnots, myMults and theOther->Knot), reset or use a fresh loop index for the
knot comparison—e.g. start a new loop with i = 1 (or use a new variable j) and
iterate up to myKnots.Length(), comparing fabs(myKnots(i) - theOther->Knot(i))
with Precision::Parametric(thePreci) so all knots are validated.