Geom evaluator #66

dpasukhi · 2025-12-14T20:14:20Z

No description provided.

- Added `D0Local`, `D1Local`, `D2Local`, `D3Local` methods to `BSplCLib_Cache` and `BSplSLib_Cache` classes - Added accessor methods to `Geom_BSplineCurve` and `Geom_BSplineSurface` for direct handle access to internal arrays - Comprehensive test coverage for both curve and surface cache implementations

…d Introduce OffsetUtils - Removed GeomEvaluator classes for curves and surfaces, streamlining the evaluation process. - Introduced Geom2d_OffsetUtils and Geom_OffsetUtils for handling offset calculations. - Updated GeomAdaptor classes to utilize new utility functions for offset evaluations. - Enhanced package structure by removing obsolete files and optimizing includes. - Improved performance and maintainability by reducing dependencies on removed evaluator classes.

- Introduced Geom_ExtrusionUtils.pxx and Geom_RevolutionUtils.pxx to provide shared utility functions for evaluating points and derivatives on linear extrusion and revolution surfaces. - Updated Geom_SurfaceOfLinearExtrusion and Geom_SurfaceOfRevolution classes to utilize the new utility functions for D0, D1, D2, D3, and DN evaluations, improving code clarity and maintainability. - Enhanced GeomAdaptor_Surface to integrate the new utility functions for extrusion and revolution surface evaluations, streamlining the overall surface handling process.

… to 2025; refactor DN function to store original point for efficiency

…tructures - Merged various surface evaluation data structures into the GeomAdaptor_Surface class, including ExtrusionData, RevolutionData, OffsetData, BezierData, and BSplineData. - Updated method signatures and internal references to utilize the new structures, enhancing code clarity and maintainability. - Removed obsolete structures and streamlined the handling of surface-specific evaluation data, improving overall performance and organization.

- Replaced instances of Geom2dAdaptor_OffsetCurveData with Geom2dAdaptor_Curve::OffsetData across the Geom2dAdaptor_Curve and BRepAdaptor_Curve2d classes. - Updated method implementations to utilize the new OffsetData structure, enhancing code clarity and maintainability. - Streamlined the handling of curve-specific evaluation data, improving overall organization and performance.

- Unified the handling of curve-specific evaluation data by introducing BezierData and BSplineData structures. - Updated ShallowCopy methods in both Geom2dAdaptor_Curve and BRepAdaptor_Curve2d to accommodate the new data structures, improving code clarity and maintainability. - Streamlined the evaluation process for Bezier and B-spline curves, enhancing performance and organization of the codebase.

… functionality

…tions - Introduced Geom_OffsetCurveUtils and Geom_OffsetSurfaceUtils to provide dedicated utility functions for offset calculations in curves and surfaces. - Updated Geom_OffsetCurve and Geom_OffsetSurface classes to utilize the new utility functions, improving code clarity and maintainability. - Replaced instances of Geom_OffsetUtils with the new utility functions across relevant classes, streamlining the evaluation process for offset points and derivatives. - Enhanced GeomAdaptor classes to integrate the new utilities, optimizing the overall handling of offset evaluations.

…Clarity - Replaced instances of gp_Dir with gp_XYZ in Geom_ExtrusionUtils and related classes to ensure consistent handling of extrusion direction as normalized XYZ vectors. - Updated method signatures and internal references in Geom_SurfaceOfLinearExtrusion and Geom_SurfaceOfRevolution to utilize the new gp_XYZ type, enhancing clarity and maintainability. - Removed unnecessary includes and streamlined the codebase for improved organization and performance.

- Introduced AdjustDerivative function in Geom_OffsetCurveUtils to handle derivative adjustments at singular points using Taylor series approximation. - Implemented ShiftPoint function in Geom_OffsetSurfaceUtils to iteratively shift evaluation points towards the center of the parametric space when normal calculations fail. - Updated EvaluateD0, EvaluateD1, EvaluateD2, and EvaluateD3 functions in Geom_OffsetSurfaceUtils to incorporate retry mechanisms for singular points, improving robustness in offset evaluations. - Enhanced GeomAdaptor_Curve and GeomAdaptor_Surface classes to utilize the new utility functions, optimizing the handling of offset evaluations and ensuring better performance in singular scenarios.

…ndle zero derivatives - Added ReplaceDerivative function to replace a zero derivative by stepping away and recomputing, addressing the CSLib_InfinityOfSolutions case. - Updated ComputeSingularNormal and EvaluateD0 functions to utilize ReplaceDerivative for improved handling of singular cases and robustness in normal calculations. - Enhanced derivative computation logic to ensure accurate evaluations in scenarios where derivatives may be zero.

- Removed deprecated calculateD0, calculateD1, calculateD2, and calculateD3 methods from Geom_OffsetSurface, streamlining the class interface. - Enhanced EvaluateD0, EvaluateD1, EvaluateD2, and EvaluateD3 functions in Geom_OffsetSurfaceUtils to utilize a more robust mechanism for handling singular points and derivatives. - Introduced a new EvaluateDN function to compute arbitrary order derivatives of the offset surface, improving the handling of singular cases. - Updated GeomAdaptor_Surface to include a Bounds method for better parameter management, enhancing the overall usability of the surface class. - Improved code clarity and maintainability by consolidating derivative evaluation logic and removing redundant code.

- Reformatted function calls in offsetD0, offsetD1, offsetD2, offsetD3, and offsetDN methods for better readability by aligning parameters vertically. - Enhanced error handling in offsetDN to ensure proper exception throwing when evaluation fails, improving robustness in derivative calculations. - Streamlined the code structure to enhance clarity and maintainability, aligning with recent updates in Geom_OffsetSurfaceUtils.

- Replaced existing derivative calculation methods in Geom_OffsetCurve with new EvaluateD0, EvaluateD1, EvaluateD2, and EvaluateD3 functions for improved clarity and maintainability. - Removed the adjustDerivative method to streamline the class interface, leveraging the new utility functions for handling singular points and derivatives. - Updated GeomAdaptor_Curve to utilize the new evaluation functions, optimizing the handling of offset evaluations and ensuring better performance in singular scenarios.

- Updated D0, D1, D2, and D3 methods in Geom_OffsetCurve to include error handling for offset point calculations, throwing exceptions when evaluations fail. - Modified Geom_OffsetCurveUtils to return boolean values for evaluation functions, indicating success or failure, and adjusted corresponding methods in GeomAdaptor_Curve to handle these changes. - Improved robustness and clarity in derivative evaluations, ensuring proper exception management in case of calculation issues.

…et Evaluations - Updated D0, D1, D2, and D3 methods in Geom2d_OffsetCurve to utilize new EvaluateD0, EvaluateD1, EvaluateD2, and EvaluateD3 functions, enhancing error handling and robustness in offset point calculations. - Removed the adjustDerivative method to streamline the interface, leveraging utility functions for handling singular points and derivatives. - Modified Geom2dAdaptor_Curve to incorporate the new evaluation functions, ensuring better performance and clarity in offset evaluations. - Improved exception management by throwing Standard_NullValue when evaluations fail, enhancing overall reliability.

…vative Evaluations - Updated the DN method in Geom2d_OffsetCurve to utilize the new EvaluateDN function, improving error handling and clarity in derivative calculations. - Simplified the derivative evaluation logic by removing the switch-case structure and leveraging the utility function for better maintainability. - Modified Geom2dAdaptor_Curve to integrate the EvaluateDN function, ensuring consistent handling of offset evaluations and enhancing overall robustness. - Improved exception management by throwing specific errors when derivative calculations fail, aligning with recent updates in the offset utilities.

…ve Evaluation - Updated the DN method in Geom_OffsetCurve to utilize the EvaluateDN function, enhancing error handling and clarity in derivative calculations. - Simplified the logic for derivative evaluations by removing the switch-case structure and leveraging the utility function for better maintainability. - Modified GeomAdaptor_Curve to integrate the EvaluateDN function, ensuring consistent handling of offset evaluations and improving robustness. - Enhanced exception management by throwing specific errors when derivative calculations fail, aligning with recent updates in the offset utilities.

…larity - Updated the D3 and D2 derivative calculation methods in Geom2d_OffsetUtils to enhance clarity and maintainability. - Simplified the logic for computing derivatives by adjusting the mathematical expressions, ensuring more straightforward evaluations. - Improved overall readability of the code while maintaining the functionality of offset evaluations.

dpasukhi · 2025-12-14T20:14:32Z

@coderabbitai full review

coderabbitai · 2025-12-14T20:14:37Z

✅ Actions performed

Full review triggered.

coderabbitai · 2025-12-14T20:25:38Z

Walkthrough

This pull request refactors geometric evaluation across B-spline curves, surfaces, and offset geometry. Changes include adding local-parameter evaluation methods to B-spline caching classes, replacing evaluator-based implementations with template utility functions for offset curves and surfaces, and restructuring adaptor classes to use variant-based data storage. The evaluator packages are removed entirely and superseded by new utility modules for extrusion and revolution surfaces. New public accessors expose B-spline array handles, and comprehensive test coverage is added for B-spline cache functionality across 2D and 3D cases with rational and non-rational geometries.

Pre-merge checks

❌ Failed checks (2 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title 'Geom evaluator' is vague and generic, using a non-descriptive term that does not convey meaningful information about the changeset's primary intent or scope.	Revise the title to be more specific and descriptive. Examples: 'Refactor geometry evaluation to use utility-based approach' or 'Replace GeomEvaluator with inline offset and extrusion utilities' would better communicate the core changes.
Description check	❓ Inconclusive	No pull request description was provided by the author, making it impossible to assess whether a description exists or relates to the changeset.	Add a detailed pull request description explaining the rationale for replacing GeomEvaluator classes with utility-based implementations and the benefits of this refactoring.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 84.40% which is sufficient. The required threshold is 80.00%.

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

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

coderabbitai

Actionable comments posted: 10

Caution

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

⚠️ Outside diff range comments (8)

src/ModelingData/TKG3d/GeomAdaptor/GeomAdaptor_SurfaceOfRevolution.cxx (1)
298-303: Missing null check after DownCast in VTrim.

Similar to Load(), the DownCast result should be validated. If BasisCurve()->Trim() returns a non-GeomAdaptor_Curve type, HC will be null.
 Handle(Adaptor3d_Surface) GeomAdaptor_SurfaceOfRevolution::VTrim(const Standard_Real First,
                                                                  const Standard_Real Last,
                                                                  const Standard_Real Tol) const
 {
   Handle(GeomAdaptor_Curve) HC =
     Handle(GeomAdaptor_Curve)::DownCast(BasisCurve()->Trim(First, Last, Tol));
+  if (HC.IsNull())
+  {
+    throw Standard_TypeMismatch("GeomAdaptor_SurfaceOfRevolution::VTrim - Trim returned incompatible type");
+  }
   Handle(GeomAdaptor_SurfaceOfRevolution) HR =
     new GeomAdaptor_SurfaceOfRevolution(GeomAdaptor_SurfaceOfRevolution(HC, myAxis));
   return HR;
 }
src/ModelingData/TKG3d/Geom/Geom_OffsetSurface.cxx (1)
117-238: Reset myOscSurf on SetBasisSurface to avoid stale osculating data.

If SetBasisSurface() is called after previously setting a BSpline/Bezier surface, and the new basis surface is not BSpline/Bezier, myOscSurf currently remains pointing to the previous surface and can corrupt singular evaluation.
 void Geom_OffsetSurface::SetBasisSurface(const Handle(Geom_Surface)& S,
                                          const Standard_Boolean      isNotCheckC0)
 {
+  // Always reset; will be re-created below for BSpline/Bezier surfaces.
+  myOscSurf.reset();
+
   Standard_Real aUf, aUl, aVf, aVl;
   S->Bounds(aUf, aUl, aVf, aVl);
@@
   if (aCheckingSurf->IsKind(STANDARD_TYPE(Geom_BSplineSurface))
       || aCheckingSurf->IsKind(STANDARD_TYPE(Geom_BezierSurface)))
   {
@@
     myOscSurf                   = std::make_unique<Geom_OsculatingSurface>(aCheckingSurf, Tol);
   }
 }
src/ModelingData/TKG3d/GeomAdaptor/GeomAdaptor_Curve.cxx (2)
609-795: Consider throwing NotImplemented for DN(N>3) unconditionally (not only on EvaluateDN failure).

Right now, if EvaluateDN() ever returns true for N > 3, you’d return a value despite the message stating it’s unsupported.
 gp_Vec GeomAdaptor_Curve::DN(const Standard_Real U, const Standard_Integer N) const
 {
   switch (myTypeCurve)
   {
@@
     case GeomAbs_OffsetCurve: {
+      if (N > 3)
+      {
+        throw Standard_NotImplemented("GeomAdaptor_Curve::DN: Derivative order > 3 not supported");
+      }
       const auto& anOffsetData = std::get<GeomAdaptor_OffsetCurveData>(myCurveData);
       gp_Vec      aDN;
       if (!Geom_OffsetCurveUtils::EvaluateDN(U,
@@
       {
-        if (N > 3)
-        {
-          throw Standard_NotImplemented(
-            "GeomAdaptor_Curve::DN: Derivative order > 3 not supported");
-        }
         throw Standard_NullValue("GeomAdaptor_Curve::DN: Unable to calculate offset DN");
       }
       return aDN;
     }
609-795: Add missing #include <Standard_NullValue.hxx> header.

The code throws Standard_NullValue multiple times (lines 609-795), but the header is not explicitly included in the translation unit. Add #include <Standard_NullValue.hxx> to prevent compilation failures on toolchains with strict header dependency checking, consistent with other exception includes like Standard_NotImplemented.hxx and Standard_NoSuchObject.hxx already present in the file.
src/ModelingData/TKG2d/Geom2dAdaptor/Geom2dAdaptor_Curve.cxx (1)
231-309: BUG: “same curve, different params” doesn’t update OffsetData::BasisAdaptor
In the myCurve == C branch you only nullify Bezier/BSpline caches (Line 299-307). If this adaptor is reused with a different [UFirst, ULast], the nested OffsetData.BasisAdaptor keeps the old range, which can break boundary handling/caching and even correctness depending on adaptor internals.
@@
   else
   {
     // Same curve but potentially different parameters - invalidate cache
     if (auto* aBSplineData = std::get_if<BSplineData>(&myCurveData))
     {
       aBSplineData->Cache.Nullify();
     }
     else if (auto* aBezierData = std::get_if<BezierData>(&myCurveData))
     {
       aBezierData->Cache.Nullify();
     }
+    else if (auto* anOffsetData = std::get_if<OffsetData>(&myCurveData))
+    {
+      // Keep nested adaptor in sync with new trim range
+      Handle(Geom2d_OffsetCurve) anOffsetCurve = Handle(Geom2d_OffsetCurve)::DownCast(myCurve);
+      anOffsetData->BasisAdaptor =
+        new Geom2dAdaptor_Curve(anOffsetCurve->BasisCurve(), UFirst, ULast);
+    }
   }
src/ModelingData/TKG3d/GeomAdaptor/GeomAdaptor_Surface.cxx (3)
359-455: Load(): nested adaptors ignore trim ranges + won’t update if only bounds change
For revolution/extrusion you construct GeomAdaptor_Curve without [myVFirst,myVLast] / [myUFirst,myULast], and the whole mySurfaceData population only happens when mySurface != S. If the same surface instance is reloaded with new bounds, nested adaptors (including offset BasisAdaptor) keep stale ranges. This is risky for boundary logic and cache validity.
@@
     else if (TheType == STANDARD_TYPE(Geom_SurfaceOfRevolution))
     {
@@
-      aRevData.BasisCurve = new GeomAdaptor_Curve(aRevSurf->BasisCurve());
+      aRevData.BasisCurve = new GeomAdaptor_Curve(aRevSurf->BasisCurve(), myVFirst, myVLast);
@@
     else if (TheType == STANDARD_TYPE(Geom_SurfaceOfLinearExtrusion))
     {
@@
-      anExtData.BasisCurve = new GeomAdaptor_Curve(anExtSurf->BasisCurve());
+      anExtData.BasisCurve = new GeomAdaptor_Curve(anExtSurf->BasisCurve(), myUFirst, myULast);
@@
     else if (TheType == STANDARD_TYPE(Geom_OffsetSurface))
     {
@@
       anOffsetData.BasisAdaptor = new GeomAdaptor_Surface(anOffSurf->BasisSurface(),
                                                           myUFirst,
                                                           myULast,
                                                           myVFirst,
                                                           myVLast,
                                                           myTolU,
                                                           myTolV);
1007-1188: BUG: cache paths ignore clamped (u,v) and still use raw (U,V)
You compute clamped u/v (Line 1014-1034) but Bezier/BSpline cache evaluation uses (U,V) for IsCacheValid, RebuildCache, and D1/D2 calls. That defeats the boundary snapping logic and can produce inconsistent results near trim bounds.
@@
     case GeomAbs_BezierSurface: {
       auto& aCache = std::get<BezierData>(mySurfaceData).Cache;
-      if (aCache.IsNull() || !aCache->IsCacheValid(U, V))
-        RebuildCache(U, V);
-      aCache->D1(U, V, P, D1U, D1V);
+      if (aCache.IsNull() || !aCache->IsCacheValid(u, v))
+        RebuildCache(u, v);
+      aCache->D1(u, v, P, D1U, D1V);
       break;
     }
@@
     case GeomAbs_BSplineSurface: {
@@
       else
       {
-        if (aBSplData.Cache.IsNull() || !aBSplData.Cache->IsCacheValid(U, V))
-          RebuildCache(U, V);
-        aBSplData.Cache->D1(U, V, P, D1U, D1V);
+        if (aBSplData.Cache.IsNull() || !aBSplData.Cache->IsCacheValid(u, v))
+          RebuildCache(u, v);
+        aBSplData.Cache->D1(u, v, P, D1U, D1V);
       }
       break;
     }
(Apply the same (U,V) → (u,v) fix in the D2 cache paths at Line 1125-1142.)

301-355: BezierData variant missing from ShallowCopy causes runtime failure—add explicit handling.

The variant declaration includes BezierData as a valid alternative (in addition to Extrusion, Revolution, Offset, and BSpline data), but ShallowCopy() omits an explicit branch to copy it. When mySurfaceType == GeomAbs_BezierSurface, subsequent calls to std::get<BezierData>(mySurfaceData) in methods like RebuildCache(), D0(), and D1() will throw std::bad_variant_access because the variant holds std::monostate instead of BezierData. Add an explicit else-if branch after the BSplineData handler:
  else if (auto* aBSplineData = std::get_if<GeomAdaptor_Surface::BSplineData>(&mySurfaceData))
  {
    GeomAdaptor_Surface::BSplineData aNewData;
    aNewData.Surface = aBSplineData->Surface;
    // Cache is not copied - will be rebuilt on demand
    aCopy->mySurfaceData = aNewData;
  }
+  else if (std::holds_alternative<GeomAdaptor_Surface::BezierData>(mySurfaceData))
+  {
+    aCopy->mySurfaceData = GeomAdaptor_Surface::BezierData{};
+  }

🧹 Nitpick comments (19)

src/FoundationClasses/TKMath/GTests/BSplSLib_Cache_Test.cxx (1)
879-888: Consider extending assertions for Y and Z components.

The D1_DifferentDegrees_* tests only compare X components for point, tangentU, and tangentV. For completeness, consider adding Y and Z comparisons as done in other tests, or add a comment explaining why X-only checks are sufficient for these specific test cases.
       // Compare
       EXPECT_NEAR(aCachePnt.X(), aDirectPnt.X(), THE_TOLERANCE)
         << "DifferentDeg D1 point X mismatch at u=" << u << ", v=" << v;
+      EXPECT_NEAR(aCachePnt.Y(), aDirectPnt.Y(), THE_TOLERANCE)
+        << "DifferentDeg D1 point Y mismatch at u=" << u << ", v=" << v;
+      EXPECT_NEAR(aCachePnt.Z(), aDirectPnt.Z(), THE_TOLERANCE)
+        << "DifferentDeg D1 point Z mismatch at u=" << u << ", v=" << v;
       EXPECT_NEAR(aCacheTanU.X(), aDirectTanU.X(), THE_TOLERANCE)
         << "DifferentDeg D1 tanU X mismatch at u=" << u << ", v=" << v;
+      EXPECT_NEAR(aCacheTanU.Y(), aDirectTanU.Y(), THE_TOLERANCE)
+        << "DifferentDeg D1 tanU Y mismatch at u=" << u << ", v=" << v;
+      EXPECT_NEAR(aCacheTanU.Z(), aDirectTanU.Z(), THE_TOLERANCE)
+        << "DifferentDeg D1 tanU Z mismatch at u=" << u << ", v=" << v;
       EXPECT_NEAR(aCacheTanV.X(), aDirectTanV.X(), THE_TOLERANCE)
         << "DifferentDeg D1 tanV X mismatch at u=" << u << ", v=" << v;
+      EXPECT_NEAR(aCacheTanV.Y(), aDirectTanV.Y(), THE_TOLERANCE)
+        << "DifferentDeg D1 tanV Y mismatch at u=" << u << ", v=" << v;
+      EXPECT_NEAR(aCacheTanV.Z(), aDirectTanV.Z(), THE_TOLERANCE)
+        << "DifferentDeg D1 tanV Z mismatch at u=" << u << ", v=" << v;
Also applies to: 970-977
src/FoundationClasses/TKMath/BSplSLib/BSplSLib_Cache.hxx (1)

112-112: Minor inconsistency in parameter types.

The new D*Local methods use double for parameters, while the existing D0/D1/D2 methods use const Standard_Real&. Consider aligning the types for API consistency, though this is a minor style concern.

Also applies to: 121-122, 137-138
src/ModelingData/TKG2d/Geom2d/Geom2d_OffsetCurve.hxx (1)
296-298: Consider using Standard_Real for consistency with public API.

The private member offsetValue uses double while all public methods (lines 97, 115, 128, 223) use Standard_Real. Although Standard_Real is typedef'd to double in OCCT, maintaining consistent type usage across the class improves readability and follows OCCT conventions.
 private:
   Handle(Geom2d_Curve) basisCurve;
-  double               offsetValue;
+  Standard_Real        offsetValue;
   GeomAbs_Shape        myBasisCurveContinuity;
src/ModelingData/TKG3d/Geom/Geom_OffsetCurve.hxx (1)
294-297: Prefer Standard_Real over double for consistency.

The offsetValue member was changed from Standard_Real to double. While functionally equivalent (Standard_Real is typedef'd to double), OCCT convention consistently uses Standard_Real for numeric members. Consider reverting to maintain codebase consistency.
 private:
   Handle(Geom_Curve) basisCurve;
   gp_Dir             direction;
-  double             offsetValue;
+  Standard_Real      offsetValue;
   GeomAbs_Shape      myBasisCurveContinuity;
src/FoundationClasses/TKMath/GTests/BSplCLib_Cache_Test.cxx (2)
90-91: Consider integer-based loop to avoid floating-point accumulation.

The loop for (double u = 0.0; u <= 1.0; u += 0.1) may miss u = 1.0 exactly due to floating-point accumulation errors. This pattern appears in all tests.
-  for (double u = 0.0; u <= 1.0; u += 0.1)
+  for (int i = 0; i <= 10; ++i)
   {
+    double u = i * 0.1;
     gp_Pnt aCachePnt, aDirectPnt;
444-447: Consider adding D3_RationalCurve3D test for complete coverage.

The rational curve tests cover D0, D1, and D2, but D3 is only tested for non-rational curves. Adding a D3 test for rational curves would provide symmetric coverage.

Do you want me to generate a D3_RationalCurve3D test following the existing pattern?
src/ModelingData/TKG3d/GeomAdaptor/GeomAdaptor_Curve.hxx (1)
33-42: Consider using Standard_Real instead of double for consistency.

The Offset member uses double while the rest of OCCT typically uses Standard_Real. Although they're typically the same type, using Standard_Real maintains consistency with OCCT conventions.
 struct GeomAdaptor_OffsetCurveData
 {
   Handle(GeomAdaptor_Curve) BasisAdaptor; //!< Adaptor for basis curve
-  double                    Offset;       //!< Offset distance
+  Standard_Real             Offset;       //!< Offset distance
   gp_Dir                    Direction;    //!< Offset direction
 };
src/ModelingData/TKG2d/Geom2dAdaptor/Geom2dAdaptor_Curve.hxx (1)

54-76: LGTM - consistent with 3D adaptor pattern.

The variant-based data storage pattern mirrors GeomAdaptor_Curve.hxx. The nested structs (OffsetData, BezierData, BSplineData) are well-documented with Doxygen comments. The mutable qualifier on Cache members is appropriate since caching doesn't affect logical const-ness.

Minor: Consider using Standard_Real instead of double for Offset (line 59) to maintain OCCT type consistency, similar to the 3D version.

src/FoundationClasses/TKMath/BSplSLib/BSplSLib_Cache.cxx (2)

41-76: Consider using Standard_Real (and guarding against zero span length) in local-param helpers.

Mostly consistency with OCCT conventions (and avoiding divide-by-zero UB if span length could ever be 0). Based on learnings, …

350-473: Local derivative mapping/scaling is readable; consider factoring shared layout logic.

Both D1Local and D2Local duplicate the “which param has higher degree” layout branching; a tiny helper to map indices could reduce maintenance risk.

src/FoundationClasses/TKMath/BSplCLib/BSplCLib_Cache.cxx (1)

26-50: Constructor overloads using unused parameters for type distinction.

The constructors use unused TColgp_Array1OfPnt2d and TColgp_Array1OfPnt parameters solely to distinguish between 2D and 3D variants. While this works, consider adding a brief comment explaining this design choice for clarity, as the /* only used to distinguish */ comments are helpful but could be more explicit about why this pattern was chosen over alternatives (like tag dispatch).

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

91-147: Nested offset unwrapping: good, but keep scalar-type guideline consistent
The loop correctly unwraps Geom2d_TrimmedCurve and flattens nested Geom2d_OffsetCurve by summing offsets (Line 98-114), and preserves trimming using original aUf/aUl (Line 139-146). Only concern here is the new double/bool local types vs repo conventions. Based on learnings, ...

src/ModelingData/TKG3d/Geom/Geom_RevolutionUtils.pxx (1)

42-70: Assumes theAxis is unit-length—document or enforce
ShiftPoint/RotatePoint use dot/cross as if theAxis is normalized; if callers ever pass non-unit gp_XYZ, rotation and projection magnitudes will be wrong. Consider either normalizing once (cost) or adding a clear contract comment / debug assert.

src/ModelingData/TKG2d/Geom2d/Geom2d_OffsetUtils.pxx (1)

293-358: Type guideline drift (double/int/bool) in src/
This file is in src/** and uses double, int, bool extensively despite the repo guideline to prefer Standard_Real/Standard_Integer/Standard_Boolean. If this is a deliberate modernization, consider aligning the guideline; otherwise, adapt types here for consistency. Based on learnings, ...
src/ModelingData/TKG3d/Geom/Geom_OffsetSurfaceUtils.pxx (4)
74-90: Consider extracting the normalization logic to reduce duplication.

IsSingular and ComputeNormal share identical normalization code (lines 78-86 and 104-111). This could be extracted into a private helper.
+//! Normalizes derivatives if their magnitude exceeds 1.0 for numerical stability.
+inline void NormalizeDerivatives(gp_Vec& theD1U, gp_Vec& theD1V)
+{
+  double aD1UNorm2 = theD1U.SquareMagnitude();
+  double aD1VNorm2 = theD1V.SquareMagnitude();
+  if (aD1UNorm2 > 1.0)
+    theD1U /= std::sqrt(aD1UNorm2);
+  if (aD1VNorm2 > 1.0)
+    theD1V /= std::sqrt(aD1VNorm2);
+}
Also applies to: 98-120

250-262: CalculateD2 does not compute D2 derivatives.

The function name suggests it computes D2 but it delegates entirely to CalculateD1. The output parameters theD2U, theD2V, theD2UV are never modified - they are passed as const inputs. This is documented in the comments but could be confusing for callers. Consider renaming or adding a clearer note about why D2 is not computed in the non-singular case.

1144-1152: Redundant re-computation of D2 derivatives.

The D2U, D2V, D2UV were already computed from the D3 call at line 1035, but are re-fetched using DN at lines 1145-1147. The comment indicates this matches old behavior, but it adds unnecessary computation overhead.

1331-1348: Same redundant re-computation pattern in EvaluateD3.

Similar to EvaluateD2, the D2 and D3 derivatives are re-fetched via DN calls despite already being computed from the initial D3 call. If this is intentional for matching legacy behavior, consider adding a TODO to optimize in a future refactor.
src/ModelingData/TKG3d/Geom/Geom_OsculatingSurface.cxx (1)
383-386: Unused local variables.

Variables u1, u2, v1, v2 are declared and populated via myBasisSurf->Bounds() but never used afterward. Consider removing them or using std::ignore pattern if bounds are needed only for the call's side effects.
-    double u1, u2, v1, v2;
-    theT = false;
-    myBasisSurf->Bounds(u1, u2, v1, v2);
+    theT = false;
+    // Bounds are queried implicitly through knot arrays below
Also applies to: 445-448

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2919e1b and 15e1fe7.

📒 Files selected for processing (65)

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)
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/ModelingAlgorithms/TKBool/TopOpeBRepTool/TopOpeBRepTool_GEOMETRY.cxx (1 hunks)
src/ModelingAlgorithms/TKShHealing/ShapeUpgrade/ShapeUpgrade_SplitSurface.cxx (1 hunks)
src/ModelingData/TKBRep/BRepAdaptor/BRepAdaptor_Curve2d.cxx (1 hunks)
src/ModelingData/TKG2d/Adaptor2d/Adaptor2d_OffsetCurve.cxx (5 hunks)
src/ModelingData/TKG2d/Geom2d/FILES.cmake (1 hunks)
src/ModelingData/TKG2d/Geom2d/Geom2d_OffsetCurve.cxx (9 hunks)
src/ModelingData/TKG2d/Geom2d/Geom2d_OffsetCurve.hxx (1 hunks)
src/ModelingData/TKG2d/Geom2d/Geom2d_OffsetUtils.pxx (1 hunks)
src/ModelingData/TKG2d/Geom2dAdaptor/Geom2dAdaptor_Curve.cxx (25 hunks)
src/ModelingData/TKG2d/Geom2dAdaptor/Geom2dAdaptor_Curve.hxx (4 hunks)
src/ModelingData/TKG2d/Geom2dEvaluator/FILES.cmake (0 hunks)
src/ModelingData/TKG2d/Geom2dEvaluator/Geom2dEvaluator.cxx (0 hunks)
src/ModelingData/TKG2d/Geom2dEvaluator/Geom2dEvaluator.hxx (0 hunks)
src/ModelingData/TKG2d/Geom2dEvaluator/Geom2dEvaluator_Curve.hxx (0 hunks)
src/ModelingData/TKG2d/Geom2dEvaluator/Geom2dEvaluator_OffsetCurve.cxx (0 hunks)
src/ModelingData/TKG2d/Geom2dEvaluator/Geom2dEvaluator_OffsetCurve.hxx (0 hunks)
src/ModelingData/TKG2d/PACKAGES.cmake (0 hunks)
src/ModelingData/TKG3d/Adaptor3d/Adaptor3d_HSurfaceTool.cxx (1 hunks)
src/ModelingData/TKG3d/Geom/FILES.cmake (3 hunks)
src/ModelingData/TKG3d/Geom/Geom_BSplineCurve.hxx (1 hunks)
src/ModelingData/TKG3d/Geom/Geom_BSplineSurface.hxx (1 hunks)
src/ModelingData/TKG3d/Geom/Geom_ExtrusionUtils.pxx (1 hunks)
src/ModelingData/TKG3d/Geom/Geom_OffsetCurve.cxx (9 hunks)
src/ModelingData/TKG3d/Geom/Geom_OffsetCurve.hxx (1 hunks)
src/ModelingData/TKG3d/Geom/Geom_OffsetCurveUtils.pxx (1 hunks)
src/ModelingData/TKG3d/Geom/Geom_OffsetSurface.cxx (13 hunks)
src/ModelingData/TKG3d/Geom/Geom_OffsetSurface.hxx (3 hunks)
src/ModelingData/TKG3d/Geom/Geom_OffsetSurfaceUtils.pxx (1 hunks)
src/ModelingData/TKG3d/Geom/Geom_OsculatingSurface.cxx (10 hunks)
src/ModelingData/TKG3d/Geom/Geom_OsculatingSurface.hxx (0 hunks)
src/ModelingData/TKG3d/Geom/Geom_OsculatingSurface.pxx (1 hunks)
src/ModelingData/TKG3d/Geom/Geom_RevolutionUtils.pxx (1 hunks)
src/ModelingData/TKG3d/Geom/Geom_SurfaceOfLinearExtrusion.cxx (7 hunks)
src/ModelingData/TKG3d/Geom/Geom_SurfaceOfLinearExtrusion.hxx (0 hunks)
src/ModelingData/TKG3d/Geom/Geom_SurfaceOfRevolution.cxx (7 hunks)
src/ModelingData/TKG3d/Geom/Geom_SurfaceOfRevolution.hxx (2 hunks)
src/ModelingData/TKG3d/GeomAdaptor/GeomAdaptor.cxx (1 hunks)
src/ModelingData/TKG3d/GeomAdaptor/GeomAdaptor_Curve.cxx (10 hunks)
src/ModelingData/TKG3d/GeomAdaptor/GeomAdaptor_Curve.hxx (2 hunks)
src/ModelingData/TKG3d/GeomAdaptor/GeomAdaptor_Surface.cxx (31 hunks)
src/ModelingData/TKG3d/GeomAdaptor/GeomAdaptor_Surface.hxx (4 hunks)
src/ModelingData/TKG3d/GeomAdaptor/GeomAdaptor_SurfaceOfLinearExtrusion.cxx (5 hunks)
src/ModelingData/TKG3d/GeomAdaptor/GeomAdaptor_SurfaceOfLinearExtrusion.hxx (2 hunks)
src/ModelingData/TKG3d/GeomAdaptor/GeomAdaptor_SurfaceOfRevolution.cxx (5 hunks)
src/ModelingData/TKG3d/GeomAdaptor/GeomAdaptor_SurfaceOfRevolution.hxx (2 hunks)
src/ModelingData/TKG3d/GeomEvaluator/FILES.cmake (0 hunks)
src/ModelingData/TKG3d/GeomEvaluator/GeomEvaluator_Curve.hxx (0 hunks)
src/ModelingData/TKG3d/GeomEvaluator/GeomEvaluator_OffsetCurve.cxx (0 hunks)
src/ModelingData/TKG3d/GeomEvaluator/GeomEvaluator_OffsetCurve.hxx (0 hunks)
src/ModelingData/TKG3d/GeomEvaluator/GeomEvaluator_OffsetSurface.cxx (0 hunks)
src/ModelingData/TKG3d/GeomEvaluator/GeomEvaluator_OffsetSurface.hxx (0 hunks)
src/ModelingData/TKG3d/GeomEvaluator/GeomEvaluator_Surface.hxx (0 hunks)
src/ModelingData/TKG3d/GeomEvaluator/GeomEvaluator_SurfaceOfExtrusion.cxx (0 hunks)
src/ModelingData/TKG3d/GeomEvaluator/GeomEvaluator_SurfaceOfExtrusion.hxx (0 hunks)
src/ModelingData/TKG3d/GeomEvaluator/GeomEvaluator_SurfaceOfRevolution.cxx (0 hunks)
src/ModelingData/TKG3d/GeomEvaluator/GeomEvaluator_SurfaceOfRevolution.hxx (0 hunks)
src/ModelingData/TKG3d/PACKAGES.cmake (0 hunks)
src/ModelingData/TKGeomBase/Extrema/Extrema_GenExtPS.cxx (1 hunks)

💤 Files with no reviewable changes (21)

src/ModelingData/TKG2d/PACKAGES.cmake
src/ModelingData/TKG2d/Geom2dEvaluator/Geom2dEvaluator_Curve.hxx
src/ModelingData/TKG3d/Geom/Geom_SurfaceOfLinearExtrusion.hxx
src/ModelingData/TKG3d/GeomEvaluator/FILES.cmake
src/ModelingData/TKG3d/GeomEvaluator/GeomEvaluator_Surface.hxx
src/ModelingData/TKG2d/Geom2dEvaluator/Geom2dEvaluator_OffsetCurve.hxx
src/ModelingData/TKG2d/Geom2dEvaluator/Geom2dEvaluator_OffsetCurve.cxx
src/ModelingData/TKG3d/PACKAGES.cmake
src/ModelingData/TKG3d/GeomEvaluator/GeomEvaluator_SurfaceOfExtrusion.cxx
src/ModelingData/TKG3d/GeomEvaluator/GeomEvaluator_SurfaceOfExtrusion.hxx
src/ModelingData/TKG3d/GeomEvaluator/GeomEvaluator_Curve.hxx
src/ModelingData/TKG2d/Geom2dEvaluator/Geom2dEvaluator.hxx
src/ModelingData/TKG3d/GeomEvaluator/GeomEvaluator_OffsetCurve.hxx
src/ModelingData/TKG3d/GeomEvaluator/GeomEvaluator_OffsetSurface.cxx
src/ModelingData/TKG3d/GeomEvaluator/GeomEvaluator_SurfaceOfRevolution.cxx
src/ModelingData/TKG3d/GeomEvaluator/GeomEvaluator_OffsetCurve.cxx
src/ModelingData/TKG3d/Geom/Geom_OsculatingSurface.hxx
src/ModelingData/TKG2d/Geom2dEvaluator/FILES.cmake
src/ModelingData/TKG3d/GeomEvaluator/GeomEvaluator_SurfaceOfRevolution.hxx
src/ModelingData/TKG2d/Geom2dEvaluator/Geom2dEvaluator.cxx
src/ModelingData/TKG3d/GeomEvaluator/GeomEvaluator_OffsetSurface.hxx

🧰 Additional context used

🧠 Learnings (7)