Shape Healing - Revert BSpline check for ShapeConstruct_ProjectCurveO nSurface#894
Conversation
dpasukhi
commented
Dec 8, 2025
- Updated isBSplineCurveInvalid to check for uneven parameterization speed and determine the need for ProjLib usage.
- Enhanced the logic for computing parameterization speed across knot intervals.
- Simplified the handling of B-spline curves with problematic knot spacing by directly utilizing ProjLib for projection.
- Improved overall clarity and maintainability of the code by removing redundant checks and streamlining parameter handling.
…nSurface - Updated isBSplineCurveInvalid to check for uneven parameterization speed and determine the need for ProjLib usage. - Enhanced the logic for computing parameterization speed across knot intervals. - Simplified the handling of B-spline curves with problematic knot spacing by directly utilizing ProjLib for projection. - Improved overall clarity and maintainability of the code by removing redundant checks and streamlining parameter handling.
There was a problem hiding this comment.
Pull request overview
This PR reverts and simplifies the B-spline curve handling in ShapeConstruct_ProjectCurveOnSurface by replacing complex knot spacing validation and reconstruction logic with a straightforward parameterization speed analysis.
Key changes:
- Replaced multi-faceted knot interval validation with a single ratio-based check of parameterization speed variation
- Removed the
rebuildBSplinefunction that attempted to fix problematic knot vectors - Simplified the main
Performmethod to directly use ProjLib for curves with uneven parameterization
| return Standard_True; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The loop initializes anIdx to 1 and increments it, but if no knot satisfies the condition theBSpline->Knot(anIdx) > theFirst, the loop exits with anIdx potentially equal to theBSpline->NbKnots() + 1. This could cause an out-of-bounds access in the subsequent loop at line 445 when calling theBSpline->Knot(anIdx). Consider adding a bounds check after this loop or ensuring anIdx is valid before proceeding.
| // Ensure anIdx is within bounds before proceeding | |
| if (anIdx > theBSpline->NbKnots()) | |
| { | |
| // No valid knot interval found; avoid out-of-bounds access | |
| return Standard_False; | |
| } |
|
|
||
| //================================================================================================= | ||
| GeomAdaptor_Curve aC3DAdaptor(theCurve); | ||
| Standard_Real aMinParSpeed = Precision::Infinite(); |
There was a problem hiding this comment.
Initializing aMinParSpeed to Precision::Infinite() means that if no valid distance is computed in the loop (lines 473-483), aMinParSpeed remains infinite. This will cause the condition at line 499 to be true even when no actual speed was calculated, potentially leading to incorrect behavior. Initialize to a sentinel value and validate that at least one speed was computed before using it.
| anEvenlyCoeff = *std::max_element(aKnotCoeffs.begin(), aKnotCoeffs.end()) | ||
| / *std::min_element(aKnotCoeffs.begin(), aKnotCoeffs.end()); |
There was a problem hiding this comment.
Calling std::max_element and std::min_element separately iterates through aKnotCoeffs twice. Consider iterating once to find both the minimum and maximum values simultaneously to improve performance.
| anEvenlyCoeff = *std::max_element(aKnotCoeffs.begin(), aKnotCoeffs.end()) | |
| / *std::min_element(aKnotCoeffs.begin(), aKnotCoeffs.end()); | |
| Standard_Real aMinCoeff = aKnotCoeffs.First(); | |
| Standard_Real aMaxCoeff = aKnotCoeffs.First(); | |
| for (Standard_Integer i = 2; i <= aKnotCoeffs.Size(); ++i) | |
| { | |
| const Standard_Real aCoeff = aKnotCoeffs.Value(i); | |
| if (aCoeff < aMinCoeff) aMinCoeff = aCoeff; | |
| if (aCoeff > aMaxCoeff) aMaxCoeff = aCoeff; | |
| } | |
| anEvenlyCoeff = aMaxCoeff / aMinCoeff; |
| anEvenlyCoeff = *std::max_element(aKnotCoeffs.begin(), aKnotCoeffs.end()) | ||
| / *std::min_element(aKnotCoeffs.begin(), aKnotCoeffs.end()); |
There was a problem hiding this comment.
Division by the result of std::min_element at line 495 could cause a division by zero if the minimum coefficient is zero or very close to zero. Although line 486 checks std::abs(aCoeff) > gp::Resolution() before appending, it's safer to add an additional guard here to prevent potential floating-point division issues.
| anEvenlyCoeff = *std::max_element(aKnotCoeffs.begin(), aKnotCoeffs.end()) | |
| / *std::min_element(aKnotCoeffs.begin(), aKnotCoeffs.end()); | |
| const Standard_Real aMinCoeff = *std::min_element(aKnotCoeffs.begin(), aKnotCoeffs.end()); | |
| if (std::abs(aMinCoeff) > gp::Resolution()) | |
| { | |
| anEvenlyCoeff = *std::max_element(aKnotCoeffs.begin(), aKnotCoeffs.end()) / aMinCoeff; | |
| } | |
| else | |
| { | |
| // Avoid division by zero or near-zero; treat as pathological case | |
| anEvenlyCoeff = Precision::Infinite(); | |
| } |
| //! Checks if B-spline curve has problematic knot spacing that could cause issues. | ||
| //! Detects cases where knot intervals are extremely small relative to the median | ||
| //! interval or the working parameter range, or where the curve has degenerate sections. | ||
| //! Checks if B-spline curve has uneven parameterization requiring special handling. |
There was a problem hiding this comment.
[nitpick] The word 'parameterization' is spelled correctly in American English, but the codebase may use British English spelling 'parameterisation'. Verify consistency with the project's spelling conventions.
Update private test reports after Open-Cascade-SAS#892 Open-Cascade-SAS#890 Open-Cascade-SAS#894 Open-Cascade-SAS#889