Modeling - Add extrema computation for non-coplanar circles #868
Conversation
- Implemented comprehensive tests for various scenarios involving circles, including coplanar, non-coplanar, concentric, and linked circles. - Verified correct computation of distances and extrema points for different configurations. - Ensured that tests cover edge cases such as identical circles, small circles, and large circles. - Updated FILES.cmake to include the new test file Extrema_ExtElC_Test.cxx.
dpasukhi
added
2. Bug
There was a problem hiding this comment.
Pull request overview
This PR implements extrema computation for non-coplanar circles using a numerical solver. Previously, IsDone() returned false for non-coplanar circle configurations. The implementation adds a new function class Extrema_FuncExtCircCirc that solves a 2D nonlinear system using math_FunctionSetRoot, with grid sampling and axis-aligned fallback for robust convergence.
Key changes:
- Adds numerical solver for non-coplanar circle extrema using Newton's method
- Refactors existing code to use modern C++ (constexpr, const qualifiers, member initializer lists)
- Includes comprehensive test coverage for both coplanar and non-coplanar circle configurations
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/ModelingData/TKGeomBase/GTests/FILES.cmake | Registers two new test files for extrema functionality |
| src/ModelingData/TKGeomBase/GTests/Extrema_ExtElC_Test.cxx | Adds 32 test cases for Extrema_ExtElC covering coplanar, non-coplanar, perpendicular, and edge cases |
| src/ModelingData/TKGeomBase/GTests/Extrema_ExtCC_Test.cxx | Adds 22 test cases for Extrema_ExtCC covering similar scenarios with adapted curve interface |
| src/ModelingData/TKGeomBase/Extrema/Extrema_ExtElC.cxx | Implements non-coplanar circle extrema solver, refactors existing code for modernization |
Comments suppressed due to low confidence (1)
src/ModelingData/TKGeomBase/Extrema/Extrema_ExtElC.cxx:1
- The return type is
intbut should beStandard_Integerto match OCCT conventions and the base class interface frommath_FunctionSetWithDerivatives.
// Copyright (c) 1995-1999 Matra Datavision
| constexpr gp_XYZ projectOnAxes(const gp_Dir& theDir, | ||
| const gp_Dir& theX, | ||
| const gp_Dir& theY, | ||
| const gp_Dir& theZ) |
There was a problem hiding this comment.
The projectOnAxes function is marked constexpr but calls gp_Dir::Dot() which is not a constexpr method. This will cause compilation errors. Remove the constexpr specifier from both overloads of projectOnAxes (lines 614 and 628).
| template <size_t N> | ||
| constexpr void initSqDistArray(double (&theSqDist)[N]) | ||
| { | ||
| myDone = Standard_False; | ||
| myIsPar = Standard_False; | ||
| myNbExt = 0; | ||
| for (size_t anIdx = 0; anIdx < sizeof(mySqDist) / sizeof(mySqDist[0]); anIdx++) | ||
| for (size_t i = 0; i < N; ++i) | ||
| { | ||
| mySqDist[anIdx] = RealLast(); | ||
| theSqDist[i] = RealLast(); | ||
| } | ||
| } |
There was a problem hiding this comment.
The initSqDistArray function is marked constexpr but calls RealLast() which is not a constexpr function. Remove the constexpr specifier, or use a constexpr-compatible value like std::numeric_limits<double>::max().
| static const math_Vector aTol(1, 2, Precision::PConfusion()); | ||
| static const math_Vector aInfBound(1, 2, 0.0); | ||
| static const math_Vector aSupBound(1, 2, THE_TWO_PI); |
There was a problem hiding this comment.
The static keyword on these variables introduces potential thread safety issues if computeNonCoplanarCircleExtrema is called concurrently. Consider making these thread_local or move them to function-local scope without static to ensure thread safety.
| static const math_Vector aTol(1, 2, Precision::PConfusion()); | |
| static const math_Vector aInfBound(1, 2, 0.0); | |
| static const math_Vector aSupBound(1, 2, THE_TWO_PI); | |
| const math_Vector aTol(1, 2, Precision::PConfusion()); | |
| const math_Vector aInfBound(1, 2, 0.0); | |
| const math_Vector aSupBound(1, 2, THE_TWO_PI); |
| Extrema_FuncExtCircCirc(const gp_Circ& theC1, const gp_Circ& theC2) | ||
| : myO1(theC1.Location()), | ||
| myO2(theC2.Location()), | ||
| myX1(theC1.XAxis().Direction()), | ||
| myY1(theC1.YAxis().Direction()), | ||
| myX2(theC2.XAxis().Direction()), | ||
| myY2(theC2.YAxis().Direction()), | ||
| myR1(theC1.Radius()), | ||
| myR2(theC2.Radius()), | ||
| myVx1(0.0), | ||
| myVy1(0.0), | ||
| myVx2(0.0), | ||
| myVy2(0.0), | ||
| myA11(0.0), | ||
| myA12(0.0), | ||
| myA21(0.0), | ||
| myA22(0.0) |
There was a problem hiding this comment.
[nitpick] The constructor has extensive member initialization. Consider extracting the computed values (myVx1, myVy1, myVx2, myVy2, myA11, myA12, myA21, myA22) into a helper initialization method to improve readability and avoid duplicating the computation logic in both the initializer list and constructor body.
| nbessai++; | ||
| } | ||
| } // while (nbessai<=2 && !done) { | ||
| } |
There was a problem hiding this comment.
[nitpick] The private method processRoots lacks documentation explaining its role in filtering and validating roots from the trigonometric solver. Add a brief comment explaining that this method normalizes, validates, and filters roots based on residual error.
1 participant
Implement numerical solver for finding extrema between non-coplanar circles using math_FunctionSetRoot with Extrema_FuncExtCircCirc
function class. Previously IsDone() returned false for non-coplanar configurations.
Changes: