Modeling, Shape Healing - Optimize PCurve projection #573
Conversation
- Remove unused BuildCurveMode method and related boolean flag. - Update Perform and PerformByProjLib method signatures for better clarity. - Change Handle(Geom_Curve) to Handle(GeomAdaptor_Curve) where applicable. - Improve code readability by simplifying variable declarations and removing unnecessary comments.
- Introduced template helper function for ProjectDegenerated methods to enhance code reusability. - Updated method signatures to use NCollection types for better memory management. - Improved indexing handling for point containers to support both 0-based and 1-based indexing. - Cleaned up and optimized existing code for clarity and performance.
…ctCurveOnSurface - Updated parameter name from 'isFromCashe' to 'isFromCache' for consistency. - Renamed member variables 'myNbCashe', 'myCashe3d', and 'myCashe2d' to 'myNbCache', 'myCache3d', and 'myCache2d' respectively to correct spelling errors.
…eOnSurface for improved readability and maintainability
… and enhance code clarity
…mproved readability and consistency
…tion and parameter handling for curves
There was a problem hiding this comment.
Pull Request Overview
This PR optimizes the PCurve projection and improves the shape healing functionality. Key changes include adding const correctness in ShapeFix_Edge, refactoring API signatures to use GeometryAdaptor_Curve in ShapeConstruct_ProjectCurveOnSurface, and enhancing the handling of degenerated surface projections with templated helper functions in ShapeAnalysis_Surface.
- Updated const qualifiers and improved variable naming in edge handling.
- Refactored curve projection APIs and introduced dynamic array aliases.
- Enhanced degenerated projection logic with lambda helpers for index mapping.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/ModelingAlgorithms/TKShHealing/ShapeFix/ShapeFix_Edge.cxx | Added const correctness to the curve handle. |
| src/ModelingAlgorithms/TKShHealing/ShapeConstruct/ShapeConstruct_ProjectCurveOnSurface.hxx | Switched API parameters to use GeometryAdaptor_Curve and updated naming conventions. |
| src/ModelingAlgorithms/TKShHealing/ShapeAnalysis/ShapeAnalysis_Surface.hxx | Introduced new vector includes and added template helper declaration. |
| src/ModelingAlgorithms/TKShHealing/ShapeAnalysis/ShapeAnalysis_Surface.cxx | Implemented templated degenerated projection helper with lambda helpers for index mapping. |
Comments suppressed due to low confidence (1)
src/ModelingAlgorithms/TKShHealing/ShapeAnalysis/ShapeAnalysis_Surface.cxx:431
- Consider adding inline comments within the lambda functions (getPoint, getPoint2d, setPoint2d) to clarify the one-based versus zero-based indexing scheme. This clarification would improve maintainability and readability of the code.
auto getPoint = [&](const PointContainer& container, Standard_Integer index) -> const gp_Pnt& { ... };
…generation - Introduced new internal functions for generating points and parameters for B-spline and general curves. - Enhanced the handling of B-spline parameters using BSplCLib for better robustness and accuracy. - Added error handling for BSplCLib failures, falling back to simpler methods when necessary. - Consolidated critical parameters and improved spacing logic for adaptive parameter generation. - Updated function signatures to follow a consistent naming convention and improve readability. - Removed outdated commented code related to previous implementations of curve projection.
…add validation for B-spline curves
…d parameter handling
…rity and organization of member functions
…nerateGeneralPoints for parameter generation
…s more effectively
…tion logic and introduce caching for optimization
…arametersForPole function and improve cache variable naming for clarity
…alidation and streamline BSpline rebuilding logic
There was a problem hiding this comment.
Pull Request Overview
This PR refactors and optimizes PCurve projection routines by updating container types, tightening method signatures, and consolidating duplicated logic into a templated helper in the ShapeAnalysis_Surface module.
- Migrated from legacy TColgp/TColStd sequences to
NCollection_DynamicArrayfor in-memory buffers. - Unified and updated
PerformandPerformByProjLibinterfaces with stronger const-correctness and new default tolerances. - Introduced a generic
ProjectDegeneratedTemplateinShapeAnalysis_Surfaceto replace two nearly identical overloads.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/ModelingAlgorithms/TKShHealing/ShapeFix/ShapeFix_Edge.cxx | Made local variables const for precision and curve handle. |
| src/ModelingAlgorithms/TKShHealing/ShapeConstruct/ShapeConstruct_ProjectCurveOnSurface.hxx | Replaced public overloads, updated Perform signature, removed old sequences, added SequenceOf* aliases. |
| src/ModelingAlgorithms/TKShHealing/ShapeAnalysis/ShapeAnalysis_Surface.hxx | Added NCollection_Vector include and new ProjectDegenerated overload using NCollection_Vector. |
| src/ModelingAlgorithms/TKShHealing/ShapeAnalysis/ShapeAnalysis_Surface.cxx | Extracted two ProjectDegenerated overloads into a unified templated ProjectDegeneratedTemplate. |
Comments suppressed due to low confidence (3)
src/ModelingAlgorithms/TKShHealing/ShapeConstruct/ShapeConstruct_ProjectCurveOnSurface.hxx:88
- The public BuildCurveMode() accessor was removed, which may break existing callers. If this is a breaking change, consider marking it deprecated first or provide migration guidance.
Standard_EXPORT Standard_Boolean& BuildCurveMode();
src/ModelingAlgorithms/TKShHealing/ShapeConstruct/ShapeConstruct_ProjectCurveOnSurface.hxx:109
- The specialized PerformByProjLib overload with continuity, maxdeg, and nbinterval parameters was removed, which is a breaking API change. Consider providing an alternative overload or documenting how users should update their calls.
Standard_EXPORT Standard_Boolean PerformByProjLib(Handle(Geom_Curve)& c3d,
src/ModelingAlgorithms/TKShHealing/ShapeAnalysis/ShapeAnalysis_Surface.cxx:528
- New templated overloads for ProjectDegenerated were added but no tests were updated. Add unit tests covering both the sequence‐based and vector‐based variants to verify behavior in singularity handling.
Standard_Boolean ShapeAnalysis_Surface::ProjectDegenerated(const Standard_Integer theNbrPnt,
| const Standard_Real theFirst, | ||
| const Standard_Real theLast, | ||
| Handle(Geom2d_Curve)& theC2d, | ||
| const Standard_Real theTolFirst = Precision::Confusion(), |
There was a problem hiding this comment.
Default tolerance parameters changed from -1 to Precision::Confusion(), which can alter projection results. Document this behavioral change so users know how to reproduce previous defaults if needed.
| const Standard_Real preci, | ||
| const Standard_Boolean direct) | ||
| // Template helper function for ProjectDegenerated methods | ||
| template <typename PointContainer, typename Point2dContainer> |
There was a problem hiding this comment.
[nitpick] The generic ProjectDegeneratedTemplate introduces runtime branching and lambda overhead inside tight loops. For performance-critical paths, consider providing specialized non‐templated implementations to avoid branch mispredictions.
| const Standard_Real preci, | ||
| const Standard_Boolean direct) | ||
| // Template helper function for ProjectDegenerated methods | ||
| template <typename PointContainer, typename Point2dContainer> |
There was a problem hiding this comment.
[nitpick] ProjectDegeneratedTemplate is quite large and complex. Consider breaking it into smaller helper functions or methods to improve readability and ease future maintenance.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors and optimizes curve projection and degeneration–handling by:
- Adding
constqualifiers and cleaning up small local usages inShapeFix_Edge. - Replacing sequence containers with
NCollection_DynamicArrayinShapeConstruct_ProjectCurveOnSurfaceand streamlining method signatures. - Extracting a templated helper (
ProjectDegeneratedTemplate) inShapeAnalysis_Surfaceto unify degenerated‐curve projection logic and support both sequence and vector inputs.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/ModelingAlgorithms/TKShHealing/ShapeFix/ShapeFix_Edge.cxx | Added const to local variables for clarity and correctness |
| src/ModelingAlgorithms/TKShHealing/ShapeConstruct/ShapeConstruct_ProjectCurveOnSurface.hxx | Swapped out legacy sequence types for NCollection_DynamicArray, removed obsolete overloads, and updated method signatures |
| src/ModelingAlgorithms/TKShHealing/ShapeAnalysis/ShapeAnalysis_Surface.hxx | Introduced NCollection_Vector, new ProjectDegenerated overloads, and templated helper |
| src/ModelingAlgorithms/TKShHealing/ShapeAnalysis/ShapeAnalysis_Surface.cxx | Implemented ProjectDegeneratedTemplate and two wrapper overloads |
Comments suppressed due to low confidence (2)
src/ModelingAlgorithms/TKShHealing/ShapeAnalysis/ShapeAnalysis_Surface.hxx:194
- New overloads and the templated
ProjectDegeneratedTemplateboth require dedicated unit tests to verify correct behavior with bothTColgp_SequenceOfPntandNCollection_Vectorinputs across boundary conditions.
Standard_EXPORT Standard_Boolean ProjectDegenerated(const Standard_Integer theNbrPnt,
src/ModelingAlgorithms/TKShHealing/ShapeAnalysis/ShapeAnalysis_Surface.cxx:432
- NCollection_Vector uses 0-based indexing with operator[]; accessing it via container(index) may not compile. Consider using container[index-1] for the vector branch or .Value(index) for sequence containers.
auto getPoint = [&](const PointContainer& container, Standard_Integer index) -> const gp_Pnt& {
| const GeomAbs_Shape continuity = GeomAbs_C1, | ||
| const Standard_Integer maxdeg = 12, | ||
| const Standard_Integer nbinterval = -1); | ||
| Standard_EXPORT virtual Standard_Boolean Perform( |
There was a problem hiding this comment.
The removal of the old PerformByProjLib overload and BuildCurveMode accessor is a breaking API change. Consider marking the old methods deprecated or providing adapter overloads to maintain backward compatibility.
| std::vector<Standard_Real, NCollection_Allocator<Standard_Real>> anIntervals; | ||
| for (Standard_Integer i = aKnots.Lower() + 1; i <= aKnots.Upper(); i++) | ||
| { | ||
| const Standard_Real anInterval = aKnots(i) - aKnots(i - 1); | ||
| if (anInterval > Precision::Confusion()) | ||
| { | ||
| anIntervals.push_back(anInterval); | ||
| } | ||
| } | ||
|
|
||
| if (anIntervals.empty()) |
There was a problem hiding this comment.
Use NCollection_Vector<Standard_Real> instead of std::vector with NCollection_Allocator for consistency with OCCT conventions and to avoid mixing STL and OCCT containers.
| std::vector<Standard_Real, NCollection_Allocator<Standard_Real>> anIntervals; | |
| for (Standard_Integer i = aKnots.Lower() + 1; i <= aKnots.Upper(); i++) | |
| { | |
| const Standard_Real anInterval = aKnots(i) - aKnots(i - 1); | |
| if (anInterval > Precision::Confusion()) | |
| { | |
| anIntervals.push_back(anInterval); | |
| } | |
| } | |
| if (anIntervals.empty()) | |
| NCollection_Vector<Standard_Real> anIntervals; | |
| for (Standard_Integer i = aKnots.Lower() + 1; i <= aKnots.Upper(); i++) | |
| { | |
| const Standard_Real anInterval = aKnots(i) - aKnots(i - 1); | |
| if (anInterval > Precision::Confusion()) | |
| { | |
| anIntervals.Append(anInterval); | |
| } | |
| } | |
| if (anIntervals.IsEmpty()) |
| catch (Standard_Failure const&) | ||
| { | ||
| // If rebuilding fails, return original curve | ||
| return nullptr; |
There was a problem hiding this comment.
Use Handle nullification pattern Handle(Geom_BSplineCurve)() or .Nullify() instead of nullptr for OCCT Handle types, as this is the established OCCT convention.
| return nullptr; | |
| return Handle(Geom_BSplineCurve)(); |
| catch (Standard_Failure const&) | ||
| { | ||
| // If rebuilding fails, return original curve | ||
| return nullptr; |
There was a problem hiding this comment.
Use Handle nullification pattern Handle(Geom_BSplineCurve)() or .Nullify() instead of nullptr for OCCT Handle types.
| return nullptr; | |
| return Handle(Geom_BSplineCurve)(); |
| catch (Standard_Failure const&) | ||
| { | ||
| // If rebuilding fails, return original curve | ||
| return nullptr; |
There was a problem hiding this comment.
Use Handle nullification pattern Handle(Geom_BSplineCurve)() or .Nullify() instead of nullptr for OCCT Handle types.
| return nullptr; | |
| return Handle(Geom_BSplineCurve)(); |
| } | ||
| c2d = getLine(points, params, pnt2d, myPreci, isRecompute, isFromCasheLine); | ||
| Standard_Boolean isFromCacheLine = Standard_False; | ||
| setContainerValue(pnt2d, nbrPnt, gp_Pnt2d(0., 0.)); // init all by {0, 0} |
There was a problem hiding this comment.
This only sets one value at index nbrPnt, not all values. This should either use a loop to initialize all elements, or the comment should be corrected to reflect that only the last element is being initialized.
| setContainerValue(pnt2d, nbrPnt, gp_Pnt2d(0., 0.)); // init all by {0, 0} | |
| // Initialize all elements of pnt2d to {0, 0} | |
| for (Standard_Integer i = 1; i <= pnt2d.Length(); ++i) | |
| { | |
| pnt2d.ChangeValue(i) = gp_Pnt2d(0., 0.); | |
| } |
| // Lambda for inserting element at specified index using SetValue | ||
| auto insertAt = [](auto& theArray, const Standard_Integer theIndex, const auto& theValue) { | ||
| const Standard_Integer aLength = theArray.Length(); | ||
|
|
||
| // Extend array by setting a value at the new last position | ||
| theArray.SetValue(aLength, theArray.Value(aLength - 1)); | ||
|
|
||
| // Move elements from the end towards the insertion point | ||
| for (Standard_Integer i = aLength; i > theIndex; i--) | ||
| { | ||
| theArray.SetValue(i, theArray.Value(i - 1)); | ||
| } | ||
|
|
||
| // Set the new value at the insertion point | ||
| theArray.SetValue(theIndex, theValue); | ||
| }; | ||
|
|
There was a problem hiding this comment.
The lambda insertAt performs inefficient element-by-element copying. For NCollection_DynamicArray, consider using the container's built-in methods if available, or document why this manual approach is necessary.
| // Lambda for inserting element at specified index using SetValue | |
| auto insertAt = [](auto& theArray, const Standard_Integer theIndex, const auto& theValue) { | |
| const Standard_Integer aLength = theArray.Length(); | |
| // Extend array by setting a value at the new last position | |
| theArray.SetValue(aLength, theArray.Value(aLength - 1)); | |
| // Move elements from the end towards the insertion point | |
| for (Standard_Integer i = aLength; i > theIndex; i--) | |
| { | |
| theArray.SetValue(i, theArray.Value(i - 1)); | |
| } | |
| // Set the new value at the insertion point | |
| theArray.SetValue(theIndex, theValue); | |
| }; | |
| // Use built-in InsertBefore method for efficient insertion at specified index |
| auto getPoint = [&](const PointContainer& container, Standard_Integer index) -> const gp_Pnt& { | ||
| if (theIsOneBased) | ||
| { | ||
| return container(index); // 1-based indexing for sequences | ||
| } | ||
| else | ||
| { | ||
| return container(index - 1); // Convert to 0-based indexing for vectors | ||
| } | ||
| }; |
There was a problem hiding this comment.
The lambda functions for indexing conversion add complexity and runtime overhead. Consider using a template parameter or trait to handle indexing at compile-time, or create wrapper classes that abstract the indexing difference.
1 participant
Optimize ShapeConstruct_ProjectCurveOnSurface class.
TODO: BSpline matching (will help to make 30+-% performance improvement)