Conversation
…and validation (Open-Cascade-SAS#897) Refactored BSplSLib_DataContainer and BSplCLib_DataContainer_T structs: - Use compile-time constexpr THE_MAX_DEGREE for array sizes - Remove constructors with runtime initialization overhead - Add standalone validateBSplineDegree() functions for degree checks - Eliminate unnecessary zero-initialization of stack buffers - Replace Standard_Real/Standard_Integer with native double/int types This improves performance by avoiding runtime memset of large arrays (~2800 doubles for surfaces, ~200 for curves) on every B-spline evaluation.
…aluation methods - Introduced new methods for evaluating B-spline curves and surfaces using pre-computed local parameters, bypassing periodic normalization. - Added validation for B-spline degrees to ensure they do not exceed the maximum supported degree. - Refactored existing evaluation methods to utilize the new local parameter methods for improved performance and clarity. - Updated header files to include new function declarations for local parameter evaluations.
…s function - Replaced existing D0, D1, and D2 local evaluation methods with a unified EvaluatePolynomials function for improved clarity and maintainability. - Introduced InvSpanLength in BSplCLib_CacheParams to optimize derivative calculations. - Enhanced derivative scaling by directly using inverse span lengths, improving performance in derivative evaluations. - Updated related methods to ensure consistent handling of rational and non-rational B-splines.
- Enhanced the EvaluatePolynomials function to improve clarity and maintainability by restructuring derivative calculations and parameter handling. - Introduced conditional logic to optimize derivative evaluations based on the maximum degree of parameters. - Updated the handling of rational and non-rational B-splines to ensure correct processing of results. - Improved the organization of derivative storage and evaluation, leading to better performance in polynomial evaluations.
- Refactored D0Local, D1Local, and D2Local methods in BSplCLib_Cache and BSplSLib_Cache to utilize NoDerivativeEvalPolynomial for improved performance and clarity. - Updated handling of rational and non-rational B-splines to ensure correct evaluation of points and derivatives. - Introduced comprehensive unit tests for BSplCLib_Cache and BSplSLib_Cache to validate the correctness of evaluations across various parameter values and configurations. - Added new test files for both BSplCLib and BSplSLib caches to ensure robust testing of B-spline functionalities.
WalkthroughDegree validation with a hard maximum was added across B‑spline curve and surface code. Fixed-size buffers were replaced by degree‑sized containers and numeric types standardized to double. Curve and surface evaluation gained local‑parameter entry points and helpers (D0Local/D1Local/D2Local/D3Local and supporting calculateDerivativeLocal), and BSplCLib_Cache received new derivative APIs. BSplSLib and BSplCLib caches were refactored to compute in local parameter space. Two GoogleTest suites (BSplCLib_Cache_Test and BSplSLib_Cache_Test) were added. Geom_BSplineCurve and Geom_BSplineSurface expose new HArray* accessors. Public APIs remain backward compatible. Pre-merge checks❌ Failed checks (1 warning, 2 inconclusive)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
src/ModelingData/TKG3d/Geom/Geom_BSplineCurve.hxx (1)
811-820: New HArray accessors look good; consider clarifying nullability/mutability expectations*These inline getters are straightforward and match the existing member layout, and the design mirrors the Geom_BSplineSurface pattern, so the additions themselves look fine.
Two small, non-blocking suggestions:
- You document that
HArrayWeights()may be null; ifpolesandflatknotsare guaranteed non-null after construction and updates, it may be worth stating that explicitly in their comments for callers relying on these for tight loops. If they are not guaranteed, also call that out so users know to checkIsNull().- Since you’re now exposing the underlying handles directly, external code can keep long‑lived aliases to internal arrays and potentially mutate them. If the intent is “read‑only, for efficient evaluation”, consider either mentioning that in the comments or (longer‑term) tightening constness around these arrays in the implementation.
These are just API-clarity nits; no changes are strictly required for correctness.
src/FoundationClasses/TKMath/BSplSLib/BSplSLib_Cache.cxx (1)
376-400: Consider adding assertions for array bounds safety.The derivative extraction relies on specific offsets (
aShift,aShift2, etc.) intoaPntDeriv[36]. While the logic appears correct, debug assertions could help catch layout mismatches during development.Consider adding a debug assertion to validate array access:
+#ifdef _DEBUG + // Validate maximum offset used: aShift6 + 2 = 6*3 + 2 = 20 < 36 + Standard_ASSERT_VOID(aShift6 + 2 < 36, "D2Local: array bounds check"); +#endifsrc/FoundationClasses/TKMath/BSplSLib/BSplSLib.cxx (1)
712-1452: Validation + new surface data container are wired correctly; you may want to centralize the degree check
- Adding
validateBSplineDegree(UDegree, VDegree);and switching to the sharedBSplSLib_DataContainerinHomogeneousD0,D1,HomogeneousD1,D2,D3, andDNis consistent: all code paths that use the staticpoles/knots*/dersarrays now guard degrees byTHE_MAX_DEGREE.- The indexing in the rational/non‑rational branches still matches the original layout: worst‑case accesses stay within
4 * (THE_MAX_DEGREE + 1)^2forpolesand48forders.DNpassesStandard_FalseforAllinRationalDerivative, so only a single derivative is copied intodc.ders[0..2]and the 48‑element buffer is more than sufficient.Optional: for consistency of behavior across the API, you might also consider calling
validateBSplineDegreein the cache evaluation helpers (CacheD0,CacheD1,CacheD2), even though they only use dynamically sizedNCollection_LocalArrayand do not touchBSplSLib_DataContainer.src/FoundationClasses/TKMath/BSplSLib/BSplSLib_Cache.hxx (1)
107-145: Local-parameter surface APIs are clear; consider aligning types with existing APIThe new
D0Local/D1Local/D2Localentry points are well documented and match the intended optimization (bypassing periodic normalization using precomputed local[-1,1]parameters). One minor consistency nit:
- All existing BSplSLib/BSplSLib_Cache methods use
Standard_Real(OCCT typedef) for scalar reals, while the new locals use rawdouble. UsingStandard_Realhere as well would keep the public API homogeneous and avoid mixed signatures for essentially the same scalar type.Behavior‑wise, assuming the implementation scales derivatives by the inverse span lengths from
myParamsU/myParamsV, the interface looks correct.src/FoundationClasses/TKMath/GTests/BSplCLib_Cache_Test.cxx (2)
38-57: Flat-knot helper is correct but assumes exact target size
createFlatKnotscorrectly flattens(knots, mults)intotheFlatKnots, and the test callers pass arrays with exactlysum(mult)length, so no overflow occurs.If you ever reuse this helper in more generic tests, consider either:
- Asserting
theFlatKnots.Length() == sum(mult)in debug, or- Accepting
theFlatKnotsbyHandle(TColStd_HArray1OfReal)and sizing it internally.For the current tests it’s fine as is.
448-517: D3 cache test only checks X components; consider asserting full vectorsIn
D3_NonRationalCurve3D, the test currently:
- Compares X of point, tangent, curvature, and torsion, but does not check Y/Z for any of them.
Given the cost is small and the
BSplCLib::D3reference already computes full 3D data, it would be more robust to assert all components:- EXPECT_NEAR(aCachePnt.X(), aDirectPnt.X(), THE_TOLERANCE) << ... + EXPECT_NEAR(aCachePnt.X(), aDirectPnt.X(), THE_TOLERANCE) << ...; + EXPECT_NEAR(aCachePnt.Y(), aDirectPnt.Y(), THE_TOLERANCE) << ...; + EXPECT_NEAR(aCachePnt.Z(), aDirectPnt.Z(), THE_TOLERANCE) << ...; // similarly for aCacheTan, aCacheCurv, aCacheTorsNot a blocker, but it would tighten coverage of the D3 path.
src/FoundationClasses/TKMath/BSplCLib/BSplCLib_Cache.hxx (2)
125-159: 3D curve*Localmethods are a clean extension; minor naming/consistency nitThe added
D0Local/D1Local/D2Local/D3Localmethods:
- Make the intended optimization explicit: they operate on
(Param - SpanStart) / SpanLengthwithout re‑doing span search.- Nicely separate the 3D local API from the existing 2D/3D global ones.
Minor points you may want to consider:
- For consistency with the rest of the class, it might be better to use
Standard_Realinstead of rawdoublein the new signatures.- The helper is named
calculateDerivativeLocal(lowercase ‘c’) whileCalculateDerivativeuses PascalCase; aligning the naming style would make the protected interface more uniform.Functionally, assuming these methods simply forward to a shared
calculateDerivativeLocalwith appropriate unpacking intogp_Pnt/gp_Vec, the interface looks good.
173-179:calculateDerivativeLocalcontract is under-specified; document expected derivative rangeThe protected
calculateDerivativeLocal(double theLocalParam, int theDerivative, Standard_Real* theDerivArray)is a key primitive now used by allD*Localmethods. It would help maintainers if the header explicitly stated:
- Allowed range of
theDerivative(looks like it should be1..3here).- Expected layout of
theDerivArray(e.g.,(k+1)*Dimentries for derivatives 0..k, or only derivatives 1..k, etc.).This is especially useful since
CalculateDerivativeuses a different signature (Standard_Real&) and slightly different semantics. Clarifying this in a brief comment will reduce the risk of misuse in future extensions.src/FoundationClasses/TKMath/BSplCLib/BSplCLib_Cache.cxx (1)
120-125: Consider using consistent type annotations.The new methods use
intfor dimension and derivative parameters (lines 121, 124, 137, etc.) while existing code usesStandard_Integer. While these are typically equivalent, usingStandard_Integerconsistently would align with OCCT conventions.-void BSplCLib_Cache::calculateDerivativeLocal(double theLocalParam, - int theDerivative, +void BSplCLib_Cache::calculateDerivativeLocal(double theLocalParam, + Standard_Integer theDerivative, Standard_Real* theDerivArray) const { - const int aDimension = myRowLength; + const Standard_Integer aDimension = myRowLength;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/FoundationClasses/TKMath/BSplCLib/BSplCLib_2.cxx(5 hunks)src/FoundationClasses/TKMath/BSplCLib/BSplCLib_Cache.cxx(5 hunks)src/FoundationClasses/TKMath/BSplCLib/BSplCLib_Cache.hxx(2 hunks)src/FoundationClasses/TKMath/BSplCLib/BSplCLib_CacheParams.hxx(3 hunks)src/FoundationClasses/TKMath/BSplCLib/BSplCLib_CurveComputation.pxx(9 hunks)src/FoundationClasses/TKMath/BSplSLib/BSplSLib.cxx(9 hunks)src/FoundationClasses/TKMath/BSplSLib/BSplSLib_Cache.cxx(3 hunks)src/FoundationClasses/TKMath/BSplSLib/BSplSLib_Cache.hxx(1 hunks)src/FoundationClasses/TKMath/GTests/BSplCLib_Cache_Test.cxx(1 hunks)src/FoundationClasses/TKMath/GTests/BSplSLib_Cache_Test.cxx(1 hunks)src/FoundationClasses/TKMath/GTests/FILES.cmake(1 hunks)src/ModelingData/TKG3d/Geom/Geom_BSplineCurve.hxx(1 hunks)src/ModelingData/TKG3d/Geom/Geom_BSplineSurface.hxx(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-07-13T22:08:43.235Z
Learnt from: CR
Repo: dpasukhi/OCCT PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-13T22:08:43.235Z
Learning: Applies to src/*/*/GTests/*.cxx : GTest files for a toolkit must be placed in src/Module/Toolkit/GTests/ and registered in FILES.cmake.
Applied to files:
src/FoundationClasses/TKMath/GTests/FILES.cmakesrc/FoundationClasses/TKMath/GTests/BSplSLib_Cache_Test.cxx
📚 Learning: 2025-07-13T22:08:43.235Z
Learnt from: CR
Repo: dpasukhi/OCCT PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-13T22:08:43.235Z
Learning: Applies to src/*/*/FILES.cmake : When adding or removing files, you must edit the corresponding src/Module/Toolkit/FILES.cmake file.
Applied to files:
src/FoundationClasses/TKMath/GTests/FILES.cmake
📚 Learning: 2025-07-13T22:08:43.235Z
Learnt from: CR
Repo: dpasukhi/OCCT PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-13T22:08:43.235Z
Learning: Applies to src/*/*/FILES.cmake : After adding a file, ALWAYS update the corresponding src/Module/Toolkit/FILES.cmake file.
Applied to files:
src/FoundationClasses/TKMath/GTests/FILES.cmake
🧬 Code graph analysis (3)
src/FoundationClasses/TKMath/BSplCLib/BSplCLib_2.cxx (1)
src/FoundationClasses/TKMath/BSplSLib/BSplSLib.cxx (2)
validateBSplineDegree(54-58)validateBSplineDegree(54-54)
src/FoundationClasses/TKMath/BSplSLib/BSplSLib.cxx (1)
src/ModelingData/TKG3d/Geom/Geom_BSplineCurve.cxx (2)
MaxDegree(220-223)MaxDegree(220-220)
src/FoundationClasses/TKMath/BSplSLib/BSplSLib_Cache.cxx (1)
src/FoundationClasses/TKMath/BSplCLib/BSplCLib_Cache.cxx (10)
D0Local(205-223)D0Local(205-205)D1Local(251-259)D1Local(251-251)D2Local(290-303)D2Local(290-293)D1(225-238)D1(225-227)D1(240-247)D1(240-242)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Prepare and Build on macOS with Clang (x64)
- GitHub Check: Prepare and Build on Ubuntu with Clang (x64)
🔇 Additional comments (27)
src/ModelingData/TKG3d/Geom/Geom_BSplineSurface.hxx (1)
1109-1120: Acknowledge this exposes mutable internal handles per established OCCT pattern, but consider encapsulation trade-offs.The
const Handle(...)&return type prevents reassignment but allows mutation through the handle sinceHandle<T>is mutable. This pattern matchesGeom_BSplineCurveand appears intentional for performance ("direct access to internal storage" and "efficient grid evaluation"), but technically breaks encapsulation.If intentional for performance optimization, either document explicitly that callers must not modify returned arrays, or consider alternatives like
Handle<const T>or const array views for stronger immutability guarantees.src/FoundationClasses/TKMath/BSplCLib/BSplCLib_2.cxx (1)
139-144: LGTM: Consistent degree validation pattern.The addition of
validateBSplineDegree(Degree)before creating the data container ensures proper bounds checking. This pattern is applied consistently across all derivative evaluation functions (D0, D1, D2, D3, DN).src/FoundationClasses/TKMath/GTests/FILES.cmake (1)
10-11: LGTM: Test files properly registered.New test files are correctly added in alphabetical order and follow the project convention for GTest file registration. Based on learnings, GTest files must be placed in the GTests directory and registered in FILES.cmake.
src/FoundationClasses/TKMath/BSplCLib/BSplCLib_CacheParams.hxx (1)
117-119: LGTM: Caching inverse span length for performance.Pre-computing
InvSpanLengthavoids repeated divisions in derivative calculations. The division is safe here sinceSpanLengthcomes from consecutive knot differences, and valid B-spline knot vectors guarantee non-zero span lengths in parameter regions where evaluation occurs.src/FoundationClasses/TKMath/GTests/BSplSLib_Cache_Test.cxx (3)
36-55: LGTM: Well-structured test fixture.The test fixture provides a clean helper for creating flat knots arrays. The empty
SetUp()is fine as no common initialization is needed.
61-153: Comprehensive D0 surface cache testing.Good coverage testing cached vs direct evaluation across a parameter grid including boundary values. The cache validity check and rebuild pattern correctly mirrors production usage.
799-888: Good edge case coverage for asymmetric degrees.Testing surfaces with different U/V degrees (3,2) and (2,3) ensures the degree-ordering logic in
EvaluatePolynomialsand derivative extraction works correctly regardless of which direction has the higher degree.Also applies to: 890-979
src/FoundationClasses/TKMath/BSplSLib/BSplSLib_Cache.cxx (6)
31-50: Well-documented internal helper for polynomial evaluation.The comprehensive documentation of parameters is helpful for understanding this complex function. The function correctly handles bivariate polynomial evaluation with derivatives for both rational and non-rational cases.
93-139: Optimized derivative evaluation paths.The conditional logic correctly optimizes derivative computations for non-rational surfaces by computing only the required derivatives, while rational surfaces need the full grid for
RationalDerivativeprocessing.
141-165: Correct handling of degree ordering for rational derivatives.The argument swapping for
RationalDerivativewhen V degree exceeds U degree is necessary because the result array layout is V-major in that case. The comment explains the reasoning well.
251-287: Clean D0Local implementation with degree-aware evaluation order.The implementation correctly evaluates the polynomial along the higher-degree direction first, then the lower-degree direction, optimizing the intermediate coefficient storage.
337-341: Correct derivative scaling with cached inverse span length.The scaling factor
2.0 * InvSpanLengthcorrectly accounts for the half-span convention used by BSplSLib. Using the pre-computed inverse span length avoids division operations in the hot path.
55-62: C++17 structured bindings are compatible with OCCT.OCCT requires a minimum C++17 standard (as of version 8.0.0), so the structured binding
auto [aMinParam, aMaxParam] = ...is fully supported. No changes needed.src/FoundationClasses/TKMath/BSplSLib/BSplSLib.cxx (2)
33-72: Degree cap and container sizing look consistent, but rely onBSplCLib::MaxDegree()being a compile‑time constant
- Using
static constexpr int THE_MAX_DEGREE = BSplCLib::MaxDegree();and stack arrays inBSplSLib_DataContaineris fine iffBSplCLib::MaxDegree()is aconstexpr(or otherwise an integral constant expression). If it remains a plain runtime function, this will not compile because array bounds andconstexprinitializers require constant expressions.- The
polesandknots1/knots2sizes are consistent with usage:
poles[4 * (THE_MAX_DEGREE + 1) * (THE_MAX_DEGREE + 1)]safely covers rational surfaces (dim = 4) for alld1, d2 <= THE_MAX_DEGREE.knots1/knots2[2 * THE_MAX_DEGREE]matches previous2 * Degreeallocation pattern from local arrays.ders[48]exactly matches the worst rational case (RationalDerivative(d1, d2, 3, 3, ..., All=true)→(3+1)^2 * 3 = 48reals).Please double‑check that
BSplCLib::MaxDegree()is updated to be aconstexpr(or otherwise usable in constant expressions) so these static buffers are well‑formed. If not, you’ll need a different mechanism (e.g., an internalenum { THE_MAX_DEGREE = 25 };) to keep the bound compile‑time.
2182-2444: NewBuildCacheoverload with dense cache array aligns with the degree cap and container layout
- The new
BuildCacheoverload that fillsTColStd_Array2OfReal& theCacheArraycorrectly reusesPrepareEval, operates on the sameBSplSLib_DataContainer, and guards withvalidateBSplineDegree(theUDegree, theVDegree);.- The cache packing logic (
aDimension,aCacheShift, row/column iteration) is consistent with the olderCachePoles/CacheWeightsvariant and respects rational vs. polynomial surfaces; the “extra” weight column for locally polynomial but globally rational surfaces is initialized to 0 and seeded to 1 at (0,0).No issues detected here; the changes look structurally sound.
src/FoundationClasses/TKMath/GTests/BSplCLib_Cache_Test.cxx (2)
63-236: Non-rational 3D cache vs direct evaluations are well covered
- D0/D1/D2 tests construct simple cubic Bezier curves, build a cache once, then rebuild on span changes (
IsCacheValid(u)+BuildCache(u, ...)) and compare againstBSplCLib::D0/D1/D2with a tight1e-10tolerance.- Knot/multiplicity setup ({0,1} with mults 4/4) and flat knots of length 8 are consistent.
This should give good confidence that the cache path matches direct evaluation for polynomials.
242-442: Rational 3D cache tests look solid
- Rational quadratic Bezier setup uses a classic circular arc example with weights
{1, sqrt(2)/2, 1}and multiplicities 3/3, matching degree 2.- D0/D1/D2 tests cover point, tangent, and curvature, reusing the same cache‑validity pattern and comparing to
BSplCLib::D*with the same tolerance.This is a good regression net for the rational cache path.
src/FoundationClasses/TKMath/BSplCLib/BSplCLib_CurveComputation.pxx (3)
26-27: Curve-levelTHE_MAX_DEGREE/DataContainer_T mirror surface logic; same constexpr caveat
static constexpr int THE_MAX_DEGREE = BSplCLib::MaxDegree();plusBSplCLib_DataContainer_T<Dimension>with stack arrays is structurally fine, but, as inBSplSLib.cxx, it depends onBSplCLib::MaxDegree()being aconstexpr(integral constant expression). Otherwise the array bounds andTHE_MAX_DEGREEdefinition are ill-formed.- Buffer sizing matches usage:
poles[(THE_MAX_DEGREE + 1) * (Dimension + 1)]covers both non‑rational (dim = Dimension) and rational (dim = Dimension + 1) curves up toDegree <= THE_MAX_DEGREE.knots[2 * THE_MAX_DEGREE]matches the legacy2 * Degreerequirement ofBuildKnots.ders[Dimension * 4]is sufficient forPLib::RationalDerivative(Degree, N, Dimension, ..., All=true)withN <= 3.Again, please confirm
BSplCLib::MaxDegree()is changed to a constexpr‑friendly form so this compiles cleanly.Also applies to: 248-268
783-1058: Degree validation and container usage in D0/D1/D2/D3/DN are consistent
- Each of
BSplCLib_D0/D1/D2/D3/DNnow:
- Copies
UandIndexinto locals.- Calls
validateBSplineDegree(Degree)before allocatingBSplCLib_DataContainer_T<Dimension>.- Uses
PrepareEval_Tto populate local knots/poles based onDegree,Poles,Knots,Mults, etc.- For rational cases:
D1/D2/D3hand off toPLib::RationalDerivativewith N=1..3 and dimensionDimension, bounded byders[Dimension*4].DNuses a small stackStandard_Real v[Dimension]withAll=false, so only the requested derivative is written;dc.dersis not used in this path.- Non‑rational low‑degree shortcuts (zeroing
V2/V3ifDegree < 2/3) are preserved.This looks correct and should guard all container-backed evaluation paths against degrees larger than
BSplCLib::MaxDegree().
1449-1590:BSplCLib_BuildCachenow correctly enforces the degree cap and uses the shared container
- The point/array and 2D cache builders both call
validateBSplineDegree(theDegree)before constructing aBSplCLib_DataContainer_T<Dimension>, so any attempt to cache a curve beyond the supported degree will raiseStandard_OutOfRange.- Both functions reuse
PrepareEval_Tto fill local poles/knots and then run a singleBohmcall withtheDegreeas derivative order, matching the prior behavior.- The packing into cache (including the optional extra weight column when
theWeights != NULL && !isRational) is unchanged logically, just fed from the new container.No functional issues spotted here.
src/FoundationClasses/TKMath/BSplCLib/BSplCLib_Cache.cxx (7)
113-116: LGTM!Clean refactoring to delegate to the new local-parameter variant. The parameter normalization pattern
(param - SpanStart) / SpanLengthis consistent with the other D* methods.
155-163: Derivative unnormalization logic is correct.The accumulated factor
aFactor *= myParams.InvSpanLengthcorrectly applies the chain rule for each derivative order. For derivative n, the factor becomesInvSpanLength^n, which is mathematically correct for the transformation from local to global parameter space.
196-223: LGTM!The refactoring cleanly separates parameter normalization from polynomial evaluation.
D0Localenables callers with pre-computed local parameters to avoid redundant normalization, which is beneficial for performance-critical paths.
251-259: LGTM!The buffer size of 8 elements is sufficient for the rational case (2 derivatives × 4 dimensions). The dimension adjustment for extracting results after
RationalDerivativeprocessing is correct.
290-303: LGTM!Buffer allocation and offset calculations are correct. The use of
aDim << 1for the second derivative offset is efficient and clear.
339-355: LGTM!The implementation correctly handles all four derivative orders (0-3) with proper offset calculations. Buffer size of 16 elements matches the maximum requirement.
137-145: The code correctly handles derivatives beyond the polynomial degree for both rational and non-rational curves. While the zeroing logic appears asymmetric (only explicit for non-rational curves), rational curves are handled correctly through EvalPolynomial's initialization (which zeros the entire result array) and RationalDerivative's explicit handling of higher derivatives. No issue exists here.
…erformance - Updated BSplCLib_Cache to rename CalculateDerivative to calculateDerivative, enhancing consistency with naming conventions. - Added comments to clarify the purpose of methods and improve code readability. - Introduced a new toLocalParams function in BSplSLib_Cache to streamline the conversion of global UV parameters to local parameters, improving maintainability. - Refactored D0, D1, and D2 methods in BSplSLib_Cache to utilize the new toLocalParams function, reducing code duplication and enhancing clarity.
…ace operations - Added temporary storage for output in RationalDerivative to prevent memory overlap issues during evaluations. - Updated D0Local, D1Local, and D2Local methods to ensure zero-initialization of result storage, improving reliability and clarity. - Enhanced comments to clarify the purpose of changes and improve code readability.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/FoundationClasses/TKMath/BSplSLib/BSplSLib_Cache.cxx (1)
386-450: LGTM! D2Local correctly computes curvatures with proper unnormalization.The implementation correctly:
- Evaluates point and derivatives up to second order
- Extracts derivative vectors based on degree order (lines 422-441)
- Applies proper unnormalization: tangents by
anInvU/V, curvatures byanInvU² / anInvV², mixed byanInvU * anInvVNote: The variable naming is consistent (
anInvU,anInvV), indicating the previous review comment about a typo has been addressed.
🧹 Nitpick comments (1)
src/FoundationClasses/TKMath/BSplCLib/BSplCLib_Cache.cxx (1)
119-183: Consider using Standard_Integer for consistency with OCCT conventions.The derivative calculation logic is correct and the refactoring to support local-parameter paths is well-executed. However, there are several local variables declared as
intrather thanStandard_Integer:
- Line 134:
const int aDimension- Line 147:
int aDerivative- Line 166:
for (int deriv = 1; ...)- Line 169:
for (int ind = 0; ...)While functionally correct, OCCT conventionally uses
Standard_Integerfor indices and dimensions.Suggested changes for consistency
- const int aDimension = myRowLength; + const Standard_Integer aDimension = myRowLength;- int aDerivative = theDerivative; + Standard_Integer aDerivative = theDerivative;- for (int deriv = 1; deriv <= aDerivative; deriv++) + for (Standard_Integer deriv = 1; deriv <= aDerivative; deriv++)- for (int ind = 0; ind < aDimension; ind++) + for (Standard_Integer ind = 0; ind < aDimension; ind++)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/FoundationClasses/TKMath/BSplCLib/BSplCLib_Cache.cxx(11 hunks)src/FoundationClasses/TKMath/BSplCLib/BSplCLib_Cache.hxx(1 hunks)src/FoundationClasses/TKMath/BSplSLib/BSplSLib_Cache.cxx(4 hunks)src/FoundationClasses/TKMath/BSplSLib/BSplSLib_Cache.hxx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/FoundationClasses/TKMath/BSplCLib/BSplCLib_Cache.cxx (4)
src/FoundationClasses/TKMath/BSplCLib/BSplCLib_2.cxx (2)
D0(129-150)D0(129-137)src/FoundationClasses/TKMath/BSplCLib/BSplCLib_1.cxx (2)
D0(248-267)D0(248-256)src/FoundationClasses/TKMath/BSplCLib/BSplCLib_3.cxx (2)
D0(249-268)D0(249-257)src/FoundationClasses/TKMath/BSplCLib/BSplCLib_BzSyntaxes.cxx (4)
D0(89-97)D0(89-92)D0(101-109)D0(101-104)
🪛 Cppcheck (2.18.0)
src/FoundationClasses/TKMath/BSplSLib/BSplSLib_Cache.cxx
[error] 24-24: There is an unknown macro here somewhere. Configuration is required. If IMPLEMENT_STANDARD_RTTIEXT is a macro then please configure it.
(unknownMacro)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Prepare and Build on Ubuntu with Clang (x64)
- GitHub Check: Prepare and Build on macOS with Clang (x64)
- GitHub Check: Prepare and Build on Windows with MSVC (x64)
🔇 Additional comments (14)
src/FoundationClasses/TKMath/BSplCLib/BSplCLib_Cache.hxx (3)
125-159: LGTM! Well-designed local-parameter API.The new
D0Local,D1Local,D2Local, andD3Localmethods provide an optimized pathway for pre-computed local parameters, bypassing periodic normalization. The documentation clearly explains the parameter convention(Param - SpanStart) / SpanLengthand the methods follow the established pattern of the existing D0/D1/D2/D3 interfaces.
163-179: LGTM! Refactored derivative calculation with local-parameter support.The refactored
calculateDerivativeand newcalculateDerivativeLocalmethods provide a clean separation between global and local parameter handling. The documentation is clear about the parameter conventions and array structure.
182-183: LGTM! Proper copy-control for cache objects.Explicitly deleting the copy constructor and assignment operator is appropriate for cache objects that manage internal state and should not be copied.
src/FoundationClasses/TKMath/BSplCLib/BSplCLib_Cache.cxx (4)
210-237: LGTM! D0Local implementation is correct.The
D0Localmethod efficiently evaluates the point on the curve using pre-computed local parameters, correctly handling both rational and non-rational cases. The delegation fromD0toD0Localis clean and preserves the existing API.
254-273: LGTM! D1Local implementation is correct.The
D1Localmethod correctly computes the point and first derivative using the local-parameter path, with proper dimension handling for rational vs non-rational curves.
292-317: LGTM! D2Local implementation is correct.The
D2Localmethod correctly computes the point and first two derivatives, with proper indexing arithmetic for extracting tangent and curvature vectors from the derivative array.
340-369: LGTM! D3Local implementation is correct.The
D3Localmethod correctly computes the point and first three derivatives, with proper offset calculations for tangent, curvature, and torsion vectors.src/FoundationClasses/TKMath/BSplSLib/BSplSLib_Cache.hxx (2)
107-145: LGTM! Local-parameter API for surface cache.The new
D0Local,D1Local, andD2Localmethods provide optimized evaluation for pre-computed local UV parameters. The documentation clearly explains the parameter convention(U - SpanMid) / SpanHalfLenin[-1, 1]range, which differs from BSplCLib's[0, 1]range convention.Note: The absence of
D3Local(third derivative) appears intentional for surface evaluation.
150-151: LGTM! Proper copy-control for surface cache objects.Explicitly deleting copy operations is appropriate for surface cache objects.
src/FoundationClasses/TKMath/BSplSLib/BSplSLib_Cache.cxx (5)
26-58: LGTM! Well-documented parameter convention translation.The
toLocalParamsfunction correctly handles the different span parameter conventions between BSplCLib and BSplSLib. The documentation clearly explains that BSplSLib uses mid-point and half-span, while BSplCLib uses start-point and full-span.
62-209: LGTM! Complex but correct polynomial evaluation with rational derivative handling.The
EvaluatePolynomialsfunction correctly:
- Determines evaluation order based on degree (max degree parameter first)
- Evaluates bivariate polynomials in two stages
- Optimizes non-rational cases by computing only needed derivatives
- Handles rational derivatives with separate temporary storage (lines 172-207) to avoid in-place operation issues
The use of
NCollection_LocalArrayfor dynamic temporary storage is appropriate for degree-dependent buffer sizes.
292-328: LGTM! D0Local correctly evaluates surface points.The implementation correctly evaluates the point using two-stage polynomial evaluation (max degree, then min degree) and handles rational surfaces by dividing by the weight component.
332-382: LGTM! D1Local correctly computes tangents with proper unnormalization.The implementation correctly:
- Calls
EvaluatePolynomialsto compute point and first derivatives- Extracts tangent vectors based on degree order (lines 359-376)
- Applies proper unnormalization factors (lines 378-381):
2.0 * InvSpanLengthaccounts for the half-span convention
22-22: C++17 structured bindings are already supported by OCCT's build configuration.OCCT's CMakeLists.txt sets C++17 as the default and minimum C++ standard (
BUILD_CPP_STANDARDdefaults to "C++17" withCMAKE_CXX_STANDARD_REQUIRED ON). The structured bindings used in this file at lines 286, 460, and 475 are fully compatible with this requirement.
…lity - Updated BSplCLib_Cache and BSplSLib_Cache to use direct division by SpanLength instead of multiplication by InvSpanLength, enhancing numerical stability for very small span lengths. - Removed InvSpanLength from BSplCLib_CacheParams to streamline parameter handling and reduce redundancy. - Enhanced comments to clarify the rationale behind changes and improve code readability.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/FoundationClasses/TKMath/BSplSLib/BSplSLib_Cache.cxx (3)
29-34: MakeConvertArray()const-correct (avoid casting away const).
ConvertArray()returnsStandard_Real*but all call sites in this file only read; the C-style cast also obscures intent. Prefer returningconst Standard_Real*and updating consumers accordingly (and keep a separate non-const accessor only where mutation is required).-Standard_Real* ConvertArray(const Handle(TColStd_HArray2OfReal)& theHArray) +const Standard_Real* ConvertArray(const Handle(TColStd_HArray2OfReal)& theHArray) { const TColStd_Array2OfReal& anArray = theHArray->Array2(); - return (Standard_Real*)&(anArray(anArray.LowerRow(), anArray.LowerCol())); + return &(anArray(anArray.LowerRow(), anArray.LowerCol())); }Also applies to: 72-85
46-58: Dropconston the returned pair type fromtoLocalParams()(it’s a by-value return).
Returningconst std::pair<...>doesn’t add safety and can get in the way of move/assignment patterns.-const std::pair<double, double> toLocalParams(...) +std::pair<double, double> toLocalParams(...)
72-209: Add an explicit derivative-order contract/guard inEvaluatePolynomials()(current implementation is effectively D0–D2 only).
The logic only materializes up to second derivatives along the “max-degree” direction (blocks 0/1/2). If someone later callsEvaluatePolynomials(..., theUDerivMax=3, theVDerivMax=3, ...), the buffer sizing might still succeed but the computed layout will be incomplete/incorrect.void EvaluatePolynomials(..., int theUDerivMax, int theVDerivMax, Standard_Real* theResultArray) { + // Current implementation supports up to 2nd derivatives (D0/D1/D2 use-cases). + // Extend block logic before raising this limit. + if (theUDerivMax > 2 || theVDerivMax > 2) + { + throw Standard_OutOfRange("EvaluatePolynomials supports derivatives up to order 2"); + } ... }src/FoundationClasses/TKMath/BSplCLib/BSplCLib_Cache.cxx (1)
119-140: Consider asserting/guarding the supported derivative max (stack buffer assumes <=3).
aTmpContainer[16]only fits(3+1) * 4(max D3 with rational 3D). IfcalculateDerivative*()is callable with arbitrarytheDerivative, add a guard (or switch to dynamic local array) to avoid future OOB writes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/FoundationClasses/TKMath/BSplCLib/BSplCLib_Cache.cxx(11 hunks)src/FoundationClasses/TKMath/BSplSLib/BSplSLib_Cache.cxx(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/FoundationClasses/TKMath/BSplSLib/BSplSLib_Cache.cxx (1)
src/FoundationClasses/TKMath/BSplCLib/BSplCLib_Cache.cxx (14)
D0(189-208)D0(189-189)D0(212-217)D0(212-212)D0Local(221-239)D0Local(221-221)D1Local(267-275)D1Local(267-267)D2Local(306-319)D2Local(306-309)D1(241-254)D1(241-243)D1(256-263)D1(256-258)
src/FoundationClasses/TKMath/BSplCLib/BSplCLib_Cache.cxx (2)
src/FoundationClasses/TKMath/BSplCLib/BSplCLib_2.cxx (2)
D0(129-150)D0(129-137)src/FoundationClasses/TKMath/BSplCLib/BSplCLib_1.cxx (2)
D0(248-267)D0(248-256)
🪛 Cppcheck (2.18.0)
src/FoundationClasses/TKMath/BSplSLib/BSplSLib_Cache.cxx
[error] 24-24: There is an unknown macro here somewhere. Configuration is required. If IMPLEMENT_STANDARD_RTTIEXT is a macro then please configure it.
(unknownMacro)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Prepare and Build on macOS with Clang (x64)
- GitHub Check: Prepare and Build on Windows with MSVC (x64)
- GitHub Check: Prepare and Build on Ubuntu with Clang (x64)
🔇 Additional comments (2)
src/FoundationClasses/TKMath/BSplSLib/BSplSLib_Cache.cxx (1)
282-288: Nice cleanup: routing global (U,V) through local-param entrypoints is clearer and matches BSplCLib_Cache.
Centralizing local parameter mapping removes duplicated span amendments and makes D0/D1/D2 consistent.Also applies to: 458-466, 470-488
src/FoundationClasses/TKMath/BSplCLib/BSplCLib_Cache.cxx (1)
212-240: Local entrypoints (D0Local/D1Local/D2Local/D3Local) improve reuse and reduce duplicated normalization.
The wrapper pattern matches the updated surface cache approach and keeps call sites clearer.Also applies to: 267-275, 306-319, 355-371
| void BSplCLib_Cache::calculateDerivative(double theParameter, | ||
| int theDerivative, | ||
| Standard_Real* theDerivArray) const | ||
| { | ||
| Standard_Real aNewParameter = myParams.PeriodicNormalization(theParameter); | ||
| aNewParameter = (aNewParameter - myParams.SpanStart) / myParams.SpanLength; | ||
| double aLocalParam = myParams.PeriodicNormalization(theParameter); | ||
| aLocalParam = (aLocalParam - myParams.SpanStart) / myParams.SpanLength; | ||
| calculateDerivativeLocal(aLocalParam, theDerivative, theDerivArray); | ||
| } | ||
|
|
||
| Standard_Real* aPolesArray = const_cast<Standard_Real*>(myPolesWeightsBuffer); | ||
| const Standard_Integer aDimension = myRowLength; | ||
| //================================================================================================== | ||
|
|
||
| void BSplCLib_Cache::calculateDerivativeLocal(double theLocalParam, | ||
| int theDerivative, | ||
| Standard_Real* theDerivArray) const | ||
| { | ||
| const int aDimension = myRowLength; | ||
| Standard_Real* aPolesArray = const_cast<Standard_Real*>(myPolesWeightsBuffer); | ||
|
|
||
| // Temporary container. The maximal size of this container is defined by: | ||
| // 1) maximal derivative for cache evaluation, which is 3, plus one row for function values, | ||
| // 2) and maximal dimension of the point, which is 3, plus one column for weights. | ||
| Standard_Real aTmpContainer[16]; | ||
|
|
||
| // When the PLib::RationaDerivative needs to be called, use temporary container | ||
| Standard_Real* aPntDeriv = myIsRational ? aTmpContainer : &theDerivArray; | ||
| // When the PLib::RationalDerivative needs to be called, use temporary container | ||
| Standard_Real* aPntDeriv = myIsRational ? aTmpContainer : theDerivArray; | ||
|
|
||
| // When the degree of curve is lesser than the requested derivative, | ||
| // nullify array cells corresponding to greater derivatives | ||
| Standard_Integer aDerivative = theDerivative; | ||
| int aDerivative = theDerivative; | ||
| if (!myIsRational && myParams.Degree < theDerivative) | ||
| { | ||
| aDerivative = myParams.Degree; | ||
| for (Standard_Integer ind = myParams.Degree * aDimension; | ||
| ind < (theDerivative + 1) * aDimension; | ||
| ind++) | ||
| for (int ind = myParams.Degree * aDimension; ind < (theDerivative + 1) * aDimension; ind++) | ||
| { | ||
| aPntDeriv[ind] = 0.0; | ||
| // clang-format off | ||
| (&theDerivArray)[ind] = 0.0; // should be cleared separately, because aPntDeriv may look to another memory area | ||
| // clang-format on | ||
| } | ||
| } | ||
|
|
||
| PLib::EvalPolynomial(aNewParameter, | ||
| PLib::EvalPolynomial(theLocalParam, | ||
| aDerivative, | ||
| myParams.Degree, | ||
| aDimension, | ||
| aPolesArray[0], | ||
| aPntDeriv[0]); | ||
|
|
||
| // Unnormalize derivatives since those are computed normalized | ||
| // Use division by SpanLength instead of multiplication by precomputed inverse | ||
| // for better numerical stability with very small span lengths | ||
| Standard_Real aFactor = 1.0; | ||
| for (Standard_Integer deriv = 1; deriv <= aDerivative; deriv++) | ||
| for (int deriv = 1; deriv <= aDerivative; deriv++) | ||
| { | ||
| aFactor /= myParams.SpanLength; | ||
| for (Standard_Integer ind = 0; ind < aDimension; ind++) | ||
| for (int ind = 0; ind < aDimension; ind++) | ||
| { | ||
| aPntDeriv[aDimension * deriv + ind] *= aFactor; | ||
| } | ||
| } | ||
|
|
||
| if (myIsRational) // calculate derivatives divided by weights derivatives | ||
| PLib::RationalDerivative(aDerivative, aDerivative, aDimension - 1, aPntDeriv[0], theDerivArray); | ||
| if (myIsRational) | ||
| { | ||
| PLib::RationalDerivative(aDerivative, | ||
| aDerivative, | ||
| aDimension - 1, | ||
| aPntDeriv[0], | ||
| theDerivArray[0]); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix degree-vs-derivative handling for rational curves (possible uninitialized reads / wrong high-order derivatives).
When myIsRational == true and myParams.Degree < theDerivative, you don’t clamp aDerivative, and you don’t clear the “extra” derivatives in theDerivArray. That can produce incorrect D2/D3 results for low-degree rational curves (common edge case).
void BSplCLib_Cache::calculateDerivativeLocal(double theLocalParam, int theDerivative, Standard_Real* theDerivArray) const
{
...
- int aDerivative = theDerivative;
- if (!myIsRational && myParams.Degree < theDerivative)
+ int aDerivative = theDerivative;
+ if (myParams.Degree < theDerivative)
{
aDerivative = myParams.Degree;
- for (int ind = myParams.Degree * aDimension; ind < (theDerivative + 1) * aDimension; ind++)
+ // Ensure higher derivatives are zero (for both rational and non-rational paths).
+ for (int ind = myParams.Degree * aDimension; ind < (theDerivative + 1) * aDimension; ++ind)
{
aPntDeriv[ind] = 0.0;
}
}
...
if (myIsRational)
{
PLib::RationalDerivative(aDerivative, aDerivative, aDimension - 1, aPntDeriv[0], theDerivArray[0]);
+
+ // If caller requested higher derivative order than degree, ensure output tail is zeroed too.
+ const int anOutDim = aDimension - 1;
+ for (int deriv = aDerivative + 1; deriv <= theDerivative; ++deriv)
+ for (int ind = 0; ind < anOutDim; ++ind)
+ theDerivArray[anOutDim * deriv + ind] = 0.0;
}
}🤖 Prompt for AI Agents
In src/FoundationClasses/TKMath/BSplCLib/BSplCLib_Cache.cxx around lines
119-185, when myIsRational==true and myParams.Degree < theDerivative you must
clamp the computed derivative order and zero out the extra output slots in the
final theDerivArray (currently only non-rational branch does that, leading to
uninitialized/wrong high-order rational derivatives). Change the logic so
aDerivative is set = min(theDerivative, myParams.Degree) for both rational and
non-rational cases, and when myIsRational is true and aDerivative <
theDerivative explicitly fill the range from (aDimension * (aDerivative+1)) up
to ((theDerivative+1) * aDimension) in theDerivArray with 0.0 before calling
PLib::RationalDerivative; keep using the temporary container for intermediate
rational evaluation.
| void BSplSLib_Cache::D1Local(double theLocalU, | ||
| double theLocalV, | ||
| gp_Pnt& thePoint, | ||
| gp_Vec& theTangentU, | ||
| gp_Vec& theTangentV) const | ||
| { | ||
| Standard_Real aPntDeriv[16] = {}; // Result storage for D1, zero-initialized | ||
| EvaluatePolynomials(myPolesWeights, | ||
| myParamsU, | ||
| myParamsV, | ||
| myIsRational, | ||
| theLocalU, | ||
| theLocalV, | ||
| 1, | ||
| 1, | ||
| aPntDeriv); | ||
|
|
||
| // After RationalDerivative (for rational surfaces), the output dimension is 3 (not 4) | ||
| // because weights have been processed out | ||
| const Standard_Integer aDimension = 3; | ||
|
|
||
| thePoint.SetCoord(aPntDeriv[0], aPntDeriv[1], aPntDeriv[2]); | ||
|
|
||
| // Tangents are stored after the point coordinates | ||
| // Order depends on which parameter has higher degree | ||
| // If U degree > V degree: layout is [P, DV, DU, ...] | ||
| // If V degree >= U degree: layout is [P, DU, DV, ...] | ||
| if (myParamsU.Degree > myParamsV.Degree) | ||
| { | ||
| aParameters[0] = aNewV; | ||
| aParameters[1] = aNewU; | ||
| theTangentV.SetCoord(aPntDeriv[aDimension], | ||
| aPntDeriv[aDimension + 1], | ||
| aPntDeriv[aDimension + 2]); | ||
| theTangentU.SetCoord(aPntDeriv[aDimension << 1], | ||
| aPntDeriv[(aDimension << 1) + 1], | ||
| aPntDeriv[(aDimension << 1) + 2]); | ||
| } | ||
| else | ||
| { | ||
| aParameters[0] = aNewU; | ||
| aParameters[1] = aNewV; | ||
| theTangentU.SetCoord(aPntDeriv[aDimension], | ||
| aPntDeriv[aDimension + 1], | ||
| aPntDeriv[aDimension + 2]); | ||
| theTangentV.SetCoord(aPntDeriv[aDimension << 1], | ||
| aPntDeriv[(aDimension << 1) + 1], | ||
| aPntDeriv[(aDimension << 1) + 2]); | ||
| } | ||
|
|
||
| // clang-format off | ||
| NCollection_LocalArray<Standard_Real> aTransientCoeffs(aCacheCols<<1); // array for intermediate results | ||
| // clang-format on | ||
|
|
||
| // Calculate intermediate values and derivatives of bivariate polynomial along variable with | ||
| // maximal degree | ||
| PLib::EvalPolynomial(aParameters[1], | ||
| 1, | ||
| aMinMaxDegree[1], | ||
| aCacheCols, | ||
| aPolesArray[0], | ||
| aTransientCoeffs[0]); | ||
|
|
||
| // Calculate a point on surface and a derivative along variable with minimal degree | ||
| PLib::EvalPolynomial(aParameters[0], | ||
| 1, | ||
| aMinMaxDegree[0], | ||
| aDimension, | ||
| aTransientCoeffs[0], | ||
| aPntDeriv[0]); | ||
|
|
||
| // Calculate derivative along variable with maximal degree | ||
| PLib::NoDerivativeEvalPolynomial(aParameters[0], | ||
| aMinMaxDegree[0], | ||
| aDimension, | ||
| aMinMaxDegree[0] * aDimension, | ||
| aTransientCoeffs[aCacheCols], | ||
| aPntDeriv[aDimension << 1]); | ||
| // Use direct division for better numerical stability with very small span lengths | ||
| const Standard_Real aSpanLengthU = 0.5 * myParamsU.SpanLength; | ||
| const Standard_Real aSpanLengthV = 0.5 * myParamsV.SpanLength; | ||
| theTangentU.Divide(aSpanLengthU); | ||
| theTangentV.Divide(aSpanLengthV); | ||
| } |
There was a problem hiding this comment.
Potential divide-by-zero hazard: tangents scaled by 0.5 * SpanLength without a safeguard.
If a degenerate/invalid span produces myParamsU.SpanLength == 0 (or extremely close), Divide(aSpanLengthU) can blow up. If the library guarantees non-zero span lengths after LocateParameter(), consider asserting it (in debug) to make the contract explicit.
🤖 Prompt for AI Agents
In src/FoundationClasses/TKMath/BSplSLib/BSplSLib_Cache.cxx around lines 332 to
383, the code divides tangents by 0.5 * SpanLength without guarding against
SpanLength == 0 (or near zero), which can cause divide-by-zero or huge values;
fix by checking each span length before dividing: define a small epsilon (e.g.
1e-12), assert(spanLength > 0) in debug to document the contract, and at runtime
only call Divide when spanLength > epsilon, otherwise set the tangent to a safe
fallback (zero vector or leave unscaled) to avoid instability. Ensure both U and
V use the same guard and keep the existing comment about numerical stability.
| void BSplSLib_Cache::D2Local(double theLocalU, | ||
| double theLocalV, | ||
| gp_Pnt& thePoint, | ||
| gp_Vec& theTangentU, | ||
| gp_Vec& theTangentV, | ||
| gp_Vec& theCurvatureU, | ||
| gp_Vec& theCurvatureV, | ||
| gp_Vec& theCurvatureUV) const | ||
| { | ||
| Standard_Real aPntDeriv[36] = {}; // Result storage for D2, zero-initialized | ||
| EvaluatePolynomials(myPolesWeights, | ||
| myParamsU, | ||
| myParamsV, | ||
| myIsRational, | ||
| theLocalU, | ||
| theLocalV, | ||
| 2, | ||
| 2, | ||
| aPntDeriv); | ||
|
|
||
| // After RationalDerivative (for rational surfaces), the output dimension is 3 (not 4) | ||
| // because weights have been processed out | ||
| const Standard_Integer aDimension = 3; | ||
| const Standard_Integer aShift = aDimension; // Shift for first derivatives | ||
| const Standard_Integer aShift2 = aDimension << 1; | ||
| const Standard_Integer aShift3 = aShift2 + aDimension; | ||
| const Standard_Integer aShift4 = aShift3 + aDimension; | ||
| const Standard_Integer aShift6 = 6 * aDimension; | ||
|
|
||
| thePoint.SetCoord(aPntDeriv[0], aPntDeriv[1], aPntDeriv[2]); | ||
|
|
||
| // Derivatives are stored consecutively | ||
| // If Max=U (U degree > V degree): | ||
| // [0]=P, [Dim]=DV, [2Dim]=DVV | ||
| // [3Dim]=DU, [4Dim]=DUV | ||
| // [6Dim]=DUU | ||
| if (myParamsU.Degree > myParamsV.Degree) | ||
| { | ||
| theTangentV.SetCoord(aResult[aDimension], aResult[aDimension + 1], aResult[aDimension + 2]); | ||
| Standard_Integer aShift = aDimension << 1; | ||
| theTangentU.SetCoord(aResult[aShift], aResult[aShift + 1], aResult[aShift + 2]); | ||
| theTangentV.SetCoord(aPntDeriv[aShift], aPntDeriv[aShift + 1], aPntDeriv[aShift + 2]); | ||
| theCurvatureV.SetCoord(aPntDeriv[aShift2], aPntDeriv[aShift2 + 1], aPntDeriv[aShift2 + 2]); | ||
| theTangentU.SetCoord(aPntDeriv[aShift3], aPntDeriv[aShift3 + 1], aPntDeriv[aShift3 + 2]); | ||
| theCurvatureUV.SetCoord(aPntDeriv[aShift4], aPntDeriv[aShift4 + 1], aPntDeriv[aShift4 + 2]); | ||
| theCurvatureU.SetCoord(aPntDeriv[aShift6], aPntDeriv[aShift6 + 1], aPntDeriv[aShift6 + 2]); | ||
| } | ||
| else | ||
| { | ||
| theTangentU.SetCoord(aResult[aDimension], aResult[aDimension + 1], aResult[aDimension + 2]); | ||
| Standard_Integer aShift = aDimension << 1; | ||
| theTangentV.SetCoord(aResult[aShift], aResult[aShift + 1], aResult[aShift + 2]); | ||
| // If Max=V (V degree >= U degree): | ||
| // [0]=P, [Dim]=DU, [2Dim]=DUU | ||
| // [3Dim]=DV, [4Dim]=DUV (DVU is symmetric) | ||
| // [6Dim]=DVV | ||
| theTangentU.SetCoord(aPntDeriv[aShift], aPntDeriv[aShift + 1], aPntDeriv[aShift + 2]); | ||
| theCurvatureU.SetCoord(aPntDeriv[aShift2], aPntDeriv[aShift2 + 1], aPntDeriv[aShift2 + 2]); | ||
| theTangentV.SetCoord(aPntDeriv[aShift3], aPntDeriv[aShift3 + 1], aPntDeriv[aShift3 + 2]); | ||
| theCurvatureUV.SetCoord(aPntDeriv[aShift4], aPntDeriv[aShift4 + 1], aPntDeriv[aShift4 + 2]); | ||
| theCurvatureV.SetCoord(aPntDeriv[aShift6], aPntDeriv[aShift6 + 1], aPntDeriv[aShift6 + 2]); | ||
| } | ||
| theTangentU.Multiply(anInvU); | ||
| theTangentV.Multiply(anInvV); | ||
|
|
||
| // Use direct division for better numerical stability with very small span lengths | ||
| const Standard_Real aSpanLengthU = 0.5 * myParamsU.SpanLength; | ||
| const Standard_Real aSpanLengthV = 0.5 * myParamsV.SpanLength; | ||
| const Standard_Real aSpanLengthU2 = aSpanLengthU * aSpanLengthU; | ||
| const Standard_Real aSpanLengthV2 = aSpanLengthV * aSpanLengthV; | ||
| theTangentU.Divide(aSpanLengthU); | ||
| theTangentV.Divide(aSpanLengthV); | ||
| theCurvatureU.Divide(aSpanLengthU2); | ||
| theCurvatureV.Divide(aSpanLengthV2); | ||
| theCurvatureUV.Divide(aSpanLengthU * aSpanLengthV); | ||
| } |
There was a problem hiding this comment.
Same scaling concern for curvature normalization (uses squared half-span).
aSpanLengthU2 / aSpanLengthV2 can underflow/zero → division issues. If you don’t want runtime checks, at least add debug assertions documenting the precondition.
…erivative evaluations - Introduced a new toLocalParams function for derivative evaluations that computes local UV parameters and inverse span lengths, enhancing numerical consistency. - Updated D0, D1, and D2 methods to utilize the new toLocalParams function, ensuring consistent handling of parameter transformations and derivative scaling. - Enhanced comments to clarify the purpose of changes and improve code readability.
…ve methods - Standardized formatting of variable declarations in D1 and D2 methods for improved readability. - Maintained numerical consistency by ensuring the same inverse values are used for parameter transformation and derivative scaling. - Enhanced comments to clarify the purpose of the changes and improve code clarity.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/FoundationClasses/TKMath/BSplSLib/BSplSLib_Cache.cxx (2)
396-400: Address divide-by-zero in tangent scaling.Tangents are scaled by
0.5 * SpanLength(lines 397-398) and then divided (lines 399-400) without safeguards. If span lengths are zero or near-zero, this causes numerical issues.This is the same concern raised in the previous review comments for these lines. The fix should be coordinated with the guards added to
toLocalParams(lines 41-76) to maintain consistency.
462-471: Address divide-by-zero in curvature scaling.Curvatures are scaled by squared half-spans (lines 465-466) and divided (lines 469-471) without guards. If span lengths are zero or near-zero, division causes numerical instability.
This is the same concern raised in the previous review comments for these lines. Coordinate the fix with guards in
toLocalParamsandD1Localfor consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/FoundationClasses/TKMath/BSplSLib/BSplSLib_Cache.cxx(4 hunks)
🧰 Additional context used
🪛 Cppcheck (2.18.0)
src/FoundationClasses/TKMath/BSplSLib/BSplSLib_Cache.cxx
[error] 24-24: There is an unknown macro here somewhere. Configuration is required. If IMPLEMENT_STANDARD_RTTIEXT is a macro then please configure it.
(unknownMacro)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Prepare and Build on Ubuntu with Clang (x64)
- GitHub Check: Prepare and Build on macOS with Clang (x64)
- GitHub Check: Prepare and Build on Windows with MSVC (x64)
🔇 Additional comments (6)
src/FoundationClasses/TKMath/BSplSLib/BSplSLib_Cache.cxx (6)
22-23: LGTM!The
<utility>header is required for thestd::pairreturn types used in the new helper functions.
26-34: LGTM!Good encapsulation moving
ConvertArrayinto an unnamed namespace to limit scope to this translation unit.
90-226: LGTM!The
EvaluatePolynomialsfunction is well-structured with:
- Proper memory management using
NCollection_LocalArray- Safe handling of rational surfaces with separate temporary storage (avoiding in-place operation hazards)
- Appropriate optimization for non-rational cases
- Clear comments explaining the memory layout and logic
The complexity is warranted given the requirements for handling various derivative orders and rational/non-rational surfaces.
300-346: LGTM!The refactoring cleanly separates parameter conversion (in
D0) from evaluation (inD0Local). The logic is correct and maintains the original behavior while improving code organization.
476-523: LGTM!The refactored
D1method correctly:
- Computes local parameters and inverse span lengths using
toLocalParams- Delegates polynomial evaluation to
EvaluatePolynomials- Scales derivatives using
Multiplywith pre-computed inverses for better numerical stabilityThe comment on line 520 clearly documents the scaling approach.
527-584: LGTM!The refactored
D2method correctly:
- Computes local parameters and inverse span lengths using
toLocalParams- Delegates polynomial evaluation to
EvaluatePolynomials- Scales derivatives using
Multiplywith pre-computed inverses (lines 579-583)- Properly handles both first and second-order derivatives
The approach is consistent with
D1and maintains numerical stability.
| std::pair<double, double> toLocalParamsD0(double theU, | ||
| double theV, | ||
| const BSplCLib_CacheParams& theParamsU, | ||
| const BSplCLib_CacheParams& theParamsV) | ||
| { | ||
| const double aNewU = theParamsU.PeriodicNormalization(theU); | ||
| const double aNewV = theParamsV.PeriodicNormalization(theV); | ||
| const double aSpanLengthU = 0.5 * theParamsU.SpanLength; | ||
| const double aSpanStartU = theParamsU.SpanStart + aSpanLengthU; | ||
| const double aSpanLengthV = 0.5 * theParamsV.SpanLength; | ||
| const double aSpanStartV = theParamsV.SpanStart + aSpanLengthV; | ||
| return {(aNewU - aSpanStartU) / aSpanLengthU, (aNewV - aSpanStartV) / aSpanLengthV}; | ||
| } | ||
|
|
||
| //! Computes local UV parameters and inverse span lengths for derivative evaluation. | ||
| //! The same inverse values are used for both parameter transformation and derivative scaling | ||
| //! to maintain numerical consistency with the original implementation. | ||
| std::pair<double, double> toLocalParams(double theU, | ||
| double theV, | ||
| const BSplCLib_CacheParams& theParamsU, | ||
| const BSplCLib_CacheParams& theParamsV, | ||
| double& theInvU, | ||
| double& theInvV) | ||
| { | ||
| const double aNewU = theParamsU.PeriodicNormalization(theU); | ||
| const double aNewV = theParamsV.PeriodicNormalization(theV); | ||
| const double aSpanLengthU = 0.5 * theParamsU.SpanLength; | ||
| const double aSpanStartU = theParamsU.SpanStart + aSpanLengthU; | ||
| const double aSpanLengthV = 0.5 * theParamsV.SpanLength; | ||
| const double aSpanStartV = theParamsV.SpanStart + aSpanLengthV; | ||
| // Compute inverses once and reuse for both parameter transformation and derivative scaling | ||
| // to maintain numerical consistency with the original implementation | ||
| theInvU = 1.0 / aSpanLengthU; | ||
| theInvV = 1.0 / aSpanLengthV; | ||
| return {(aNewU - aSpanStartU) * theInvU, (aNewV - aSpanStartV) * theInvV}; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the file and locate LocateParameter definition
fd -e cxx -e h . | xargs grep -l "LocateParameter" | head -5Repository: dpasukhi/OCCT
Length of output: 421
🏁 Script executed:
# Search for LocateParameter implementation in BSplCLib_CacheParams
rg -A 20 "class BSplCLib_CacheParams" --type=cppRepository: dpasukhi/OCCT
Length of output: 39
🏁 Script executed:
# Look at lines 350-401 to understand the derivative scaling context mentioned in the comment
sed -n '350,401p' src/FoundationClasses/TKMath/BSplSLib/BSplSLib_Cache.cxxRepository: dpasukhi/OCCT
Length of output: 2145
🏁 Script executed:
# Search for any existing epsilon checks or division guards in the file
rg -n "epsilon|e-12|e-10|fabs|Abs|Epsilon" src/FoundationClasses/TKMath/BSplSLib/BSplSLib_Cache.cxxRepository: dpasukhi/OCCT
Length of output: 39
🏁 Script executed:
# Search for LocateParameter definition
rg -n "LocateParameter" src/FoundationClasses/TKMath/BSplSLib/ -A 5 | head -50Repository: dpasukhi/OCCT
Length of output: 3233
🏁 Script executed:
# Search for BSplCLib_CacheParams definition and SpanLength initialization
rg -n "SpanLength" src/FoundationClasses/TKMath/BSplSLib/ -B 2 -A 2 | head -80Repository: dpasukhi/OCCT
Length of output: 7913
🏁 Script executed:
# Look for the complete context of lines around 41-76 and beyond
sed -n '1,100p' src/FoundationClasses/TKMath/BSplSLib/BSplSLib_Cache.cxx | head -100Repository: dpasukhi/OCCT
Length of output: 5196
🏁 Script executed:
# Find LocateParameter implementation in BSplCLib
fd -e cxx -e h "BSplCLib" src/FoundationClasses/TKMath/ | xargs grep -l "LocateParameter"Repository: dpasukhi/OCCT
Length of output: 217
🏁 Script executed:
# Look for LocateParameter implementation and documentation
rg -n "LocateParameter" src/FoundationClasses/TKMath/ -B 5 -A 15 --type=cpp --type=hxx | grep -A 15 "method\|procedure\|void\|Standard_Integer" | head -100Repository: dpasukhi/OCCT
Length of output: 84
🏁 Script executed:
# Search for BSplCLib_CacheParams documentation and LocateParameter
fd "BSplCLib" src/FoundationClasses/TKMath/ -name "*.hxx" -exec grep -l "LocateParameter\|CacheParams" {} \;Repository: dpasukhi/OCCT
Length of output: 287
🏁 Script executed:
# Look at the header file for BSplCLib_CacheParams class
fd -e hxx "BSplCLib" src/FoundationClasses/TKMath/ -exec cat {} \; 2>/dev/null | head -200Repository: dpasukhi/OCCT
Length of output: 432
🏁 Script executed:
cat src/FoundationClasses/TKMath/BSplCLib/BSplCLib_CacheParams.hxxRepository: dpasukhi/OCCT
Length of output: 4869
🏁 Script executed:
# Search for LocateParameter implementation in BSplCLib.cxx
rg "LocateParameter" src/FoundationClasses/TKMath/BSplCLib/BSplCLib.cxx -A 30 | head -150Repository: dpasukhi/OCCT
Length of output: 5703
🏁 Script executed:
# Search for preconditions or assertions related to span validation
rg "assert|Assert|OCCT_THROW|throw|Standard_THROW" src/FoundationClasses/TKMath/BSplSLib/BSplSLib_Cache.cxx -B 3 -A 3 | head -80Repository: dpasukhi/OCCT
Length of output: 39
🏁 Script executed:
# Check how toLocalParams and toLocalParamsD0 are called
rg "toLocalParams" src/FoundationClasses/TKMath/BSplSLib/BSplSLib_Cache.cxx -B 2 -A 2Repository: dpasukhi/OCCT
Length of output: 1514
🏁 Script executed:
# Look for any validation of span length before these functions are called
sed -n '240,280p' src/FoundationClasses/TKMath/BSplSLib/BSplSLib_Cache.cxxRepository: dpasukhi/OCCT
Length of output: 2095
🏁 Script executed:
# Check the class documentation and method signatures
sed -n '200,260p' src/FoundationClasses/TKMath/BSplSLib/BSplSLib_Cache.cxxRepository: dpasukhi/OCCT
Length of output: 2809
🏁 Script executed:
# Search for any comments about valid parameter ranges or preconditions
rg "precondition|prerequisite|require|guarantee|valid" src/FoundationClasses/TKMath/BSplSLib/BSplSLib_Cache.cxx -i | head -20Repository: dpasukhi/OCCT
Length of output: 307
Add guards against division by degenerate spans in parameter conversion.
Both toLocalParamsD0 (line 52) and toLocalParams (lines 73-74) divide by aSpanLengthU and aSpanLengthV without checking for zero or near-zero values. In B-spline theory, when consecutive knots coincide (multiple knots), SpanLength becomes zero, creating degenerate spans. This will cause divide-by-zero errors.
Add runtime epsilon checks (e.g., if (std::fabs(aSpanLengthU) < 1e-12)) with safe fallback behavior, or if the implementation should never encounter degenerate spans, add explicit assertions to document this precondition. The same issue affects D1Local and D2Local methods (lines 399-400, 467-471).
🤖 Prompt for AI Agents
In src/FoundationClasses/TKMath/BSplSLib/BSplSLib_Cache.cxx around lines 41-76
(and also update the similar places at lines ~399-400 and ~467-471), the code
divides by aSpanLengthU and aSpanLengthV without guarding against zero/near-zero
span lengths; add runtime guards using an epsilon (e.g., if
(std::fabs(aSpanLengthU) < 1e-12) ) and handle the degenerate case: set
theInvU/theInvV = 0.0 and return/clamp the corresponding local parameter
component to 0.0 (or choose an explicit assert if the code must never encounter
degenerate spans), ensuring the same guarded logic is applied consistently in
toLocalParamsD0, toLocalParams, D1Local and D2Local to prevent divide-by-zero
and NaNs.
…alculations - Simplified variable declarations in calculateDerivativeLocal and D0Local methods for enhanced readability. - Replaced the use of const_cast with direct access to myPolesWeightsBuffer, improving code clarity and consistency. - Updated comments to better explain the changes and their impact on derivative evaluations.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/FoundationClasses/TKMath/BSplCLib/BSplCLib_Cache.cxx (1)
130-184: Fix rational Degree-vs-Derivative handling + zero tail outputs (still broken).
The clamp/zero logic is still gated by!myIsRational(Line 147), so for rational curves withmyParams.Degree < theDerivativeyou can (a) callPLib::EvalPolynomial()with derivative order > degree and (b) return uninitialized/garbage for higher derivatives. This matches the earlier review concern.Proposed fix (clamp for both paths + zero caller-visible tail in
theDerivArray):void BSplCLib_Cache::calculateDerivativeLocal(double theLocalParam, int theDerivative, Standard_Real* theDerivArray) const { const int aDimension = myRowLength; @@ - int aDerivative = theDerivative; - if (!myIsRational && myParams.Degree < theDerivative) + int aDerivative = theDerivative; + if (myParams.Degree < theDerivative) { aDerivative = myParams.Degree; - for (int ind = myParams.Degree * aDimension; ind < (theDerivative + 1) * aDimension; ind++) + // Ensure any requested derivative order above degree is returned as 0. + for (int deriv = aDerivative + 1; deriv <= theDerivative; ++deriv) + for (int ind = 0; ind < (myIsRational ? (aDimension - 1) : aDimension); ++ind) + theDerivArray[deriv * (myIsRational ? (aDimension - 1) : aDimension) + ind] = 0.0; + + // For non-rational path, also clear the intermediate buffer tail (same layout as output). + if (!myIsRational) + for (int ind = aDerivative * aDimension; ind < (theDerivative + 1) * aDimension; ++ind) + aPntDeriv[ind] = 0.0; } @@ if (myIsRational) { PLib::RationalDerivative(aDerivative, aDerivative, aDimension - 1, aPntDeriv[0], theDerivArray[0]); } }(If you prefer: compute/clear the output tail after
RationalDerivative()instead.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/FoundationClasses/TKMath/BSplCLib/BSplCLib_Cache.cxx(11 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/FoundationClasses/TKMath/BSplCLib/BSplCLib_Cache.cxx (1)
src/FoundationClasses/TKMath/BSplSLib/BSplSLib_Cache.cxx (8)
D0(300-306)D0(300-302)D0Local(310-346)D0Local(310-310)D1Local(350-401)D1Local(350-354)D2Local(405-472)D2Local(405-412)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Prepare and Build on Windows with MSVC (x64)
- GitHub Check: Prepare and Build on Ubuntu with Clang (x64)
- GitHub Check: Prepare and Build on macOS with Clang (x64)
🔇 Additional comments (5)
src/FoundationClasses/TKMath/BSplCLib/BSplCLib_Cache.cxx (5)
119-126: Avoid relying on stale span data incalculateDerivative()(ensure cache is built/located for theParameter).
calculateDerivative()usesmyParams.SpanStart/SpanLengthbut doesn’t callmyParams.LocateParameter()(unlikeBuildCache). If the cache can be evaluated for arbitrary parameters, this needs to locate/refresh span first (or document/assert that caller already validated & built cache for that span).
211-237: D0 now cleanly delegates to local-param path (LGTM).
TheD0()→D0Local()split makes the “already normalized” contract explicit and avoids duplicated normalization logic.
254-273: D1Local layout looks consistent with rational/non-rational output.
UsingaDim = myIsRational ? myRowLength - 1 : myRowLengthmatches the post-PLib::RationalDerivative()packing expectation.
292-317: D2Local indexing via shifts is correct and readable.
aShift = aDim << 1and the subsequent slices align with (P, D1, D2) packing.
340-369: D3Local packing is consistent (P, D1, D2, D3).
Shift math (aShift2,aShift3) matches the expected derivative blocks.
| void BSplCLib_Cache::calculateDerivativeLocal(double theLocalParam, | ||
| int theDerivative, | ||
| Standard_Real* theDerivArray) const | ||
| { | ||
| const int aDimension = myRowLength; | ||
|
|
||
| // Temporary container. The maximal size of this container is defined by: | ||
| // 1) maximal derivative for cache evaluation, which is 3, plus one row for function values, | ||
| // 2) and maximal dimension of the point, which is 3, plus one column for weights. | ||
| Standard_Real aTmpContainer[16]; | ||
|
|
||
| // When the PLib::RationaDerivative needs to be called, use temporary container | ||
| Standard_Real* aPntDeriv = myIsRational ? aTmpContainer : &theDerivArray; | ||
| // When the PLib::RationalDerivative needs to be called, use temporary container | ||
| Standard_Real* aPntDeriv = myIsRational ? aTmpContainer : theDerivArray; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l src/FoundationClasses/TKMath/BSplCLib/BSplCLib_Cache.cxxRepository: dpasukhi/OCCT
Length of output: 117
🏁 Script executed:
head -200 src/FoundationClasses/TKMath/BSplCLib/BSplCLib_Cache.cxx | tail -80Repository: dpasukhi/OCCT
Length of output: 3288
🏁 Script executed:
sed -n '130,175p' src/FoundationClasses/TKMath/BSplCLib/BSplCLib_Cache.cxxRepository: dpasukhi/OCCT
Length of output: 1881
🏁 Script executed:
sed -n '100,175p' src/FoundationClasses/TKMath/BSplCLib/BSplCLib_Cache.cxxRepository: dpasukhi/OCCT
Length of output: 3110
🏁 Script executed:
grep -n "calculateDerivative\|myRowLength" src/FoundationClasses/TKMath/BSplCLib/BSplCLib_Cache.cxx | head -30Repository: dpasukhi/OCCT
Length of output: 1412
🏁 Script executed:
fd -e hxx -e h BSplCLib_Cache src/FoundationClasses/TKMath/Repository: dpasukhi/OCCT
Length of output: 176
🏁 Script executed:
cat src/FoundationClasses/TKMath/BSplCLib/BSplCLib_Cache.hxxRepository: dpasukhi/OCCT
Length of output: 11756
🏁 Script executed:
sed -n '245,275p' src/FoundationClasses/TKMath/BSplCLib/BSplCLib_Cache.cxxRepository: dpasukhi/OCCT
Length of output: 1307
🏁 Script executed:
sed -n '265,320p' src/FoundationClasses/TKMath/BSplCLib/BSplCLib_Cache.cxxRepository: dpasukhi/OCCT
Length of output: 2548
🏁 Script executed:
sed -n '156,162p' src/FoundationClasses/TKMath/BSplCLib/BSplCLib_Cache.cxxRepository: dpasukhi/OCCT
Length of output: 292
🏁 Script executed:
grep -n "aTmpContainer\|Standard_Real.*\[.*\];" src/FoundationClasses/TKMath/BSplCLib/BSplCLib_Cache.cxxRepository: dpasukhi/OCCT
Length of output: 629
🏁 Script executed:
rg -n "calculateDerivativeLocal\|PLib::RationalDerivative" src/FoundationClasses/TKMath/BSplCLib/BSplCLib_Cache.cxx -A 15Repository: dpasukhi/OCCT
Length of output: 39
🏁 Script executed:
sed -n '155,170p' src/FoundationClasses/TKMath/BSplCLib/BSplCLib_Cache.cxxRepository: dpasukhi/OCCT
Length of output: 675
🏁 Script executed:
grep -B5 -A10 "PLib::EvalPolynomial" src/FoundationClasses/TKMath/BSplCLib/BSplCLib_Cache.cxx | head -30Repository: dpasukhi/OCCT
Length of output: 579
🏁 Script executed:
# Check if there's any validation before calculateDerivative is called
rg "calculateDerivative\(" src/FoundationClasses/TKMath/BSplCLib/ -B 3Repository: dpasukhi/OCCT
Length of output: 2227
🏁 Script executed:
rg "class.*BSplCLib_Cache\|: public.*BSplCLib_Cache\|: BSplCLib_Cache" src/FoundationClasses/TKMath/ -A 5Repository: dpasukhi/OCCT
Length of output: 39
🏁 Script executed:
# Check if calculateDerivativeLocal is virtual
grep -n "virtual\|calculateDerivativeLocal" src/FoundationClasses/TKMath/BSplCLib/BSplCLib_Cache.hxxRepository: dpasukhi/OCCT
Length of output: 122
🏁 Script executed:
grep -n "Standard_Real.*\[.*\];.*Temporary\|// Temporary\|aTmp" src/FoundationClasses/TKMath/BSplCLib/BSplCLib_Cache.cxxRepository: dpasukhi/OCCT
Length of output: 254
🏁 Script executed:
# Verify the actual dimensions once more
sed -n '34p;48p;130,145p' src/FoundationClasses/TKMath/BSplCLib/BSplCLib_Cache.cxxRepository: dpasukhi/OCCT
Length of output: 1001
Add input validation for theDerivative parameter in calculateDerivativeLocal to prevent stack buffer overflow.
The aTmpContainer[16] buffer at line 139 is sized for (3+1) * 4 = 16 elements, assuming maximum derivative order of 3 with rational 3D curves (dimension 4). When myIsRational=true, aPntDeriv points to this temporary buffer and is passed to EvalPolynomial, which writes (theDerivative + 1) * aDimension elements. If theDerivative > 3, this causes a stack buffer overflow. Add a guard to enforce the documented constraint:
void BSplCLib_Cache::calculateDerivativeLocal(double theLocalParam,
int theDerivative,
Standard_Real* theDerivArray) const
{
+ if (theDerivative < 0 || theDerivative > 3)
+ {
+ throw Standard_OutOfRange("BSplCLib_Cache::calculateDerivativeLocal(): derivative order must be in [0..3]");
+ }
const int aDimension = myRowLength;
Standard_Real aTmpContainer[16];1 participant
No description provided.