Modeling - Refactor BRepClass and FaceClassifier Implementation - #1167
Merged
Conversation
dpasukhi
commented
Mar 23, 2026
dpasukhi
commented
Member
- Removed the TopClass_FaceClassifier.lxx file and migrated its content to TopClass_FaceClassifier.pxx for better organization.
- Introduced BRepClass_FClass2dOfFClassifier and BRepClass_FClassifier to streamline face classification processes.
- Updated BRepCheck_Edge to enhance polygon and curve checks on triangulations.
- Simplified BRepClass_Edge and BRepClass_FaceExplorer by removing inline implementations and unnecessary header inclusions.
- Added BRepClass_FacePassiveClassifier to handle passive classification scenarios.
- Cleaned up redundant files and definitions across BRepClass and BRepTopAdaptor modules.
- Improved code readability and maintainability by consolidating methods and reducing file dependencies.
- Removed the TopClass_FaceClassifier.lxx file and migrated its content to TopClass_FaceClassifier.pxx for better organization. - Introduced BRepClass_FClass2dOfFClassifier and BRepClass_FClassifier to streamline face classification processes. - Updated BRepCheck_Edge to enhance polygon and curve checks on triangulations. - Simplified BRepClass_Edge and BRepClass_FaceExplorer by removing inline implementations and unnecessary header inclusions. - Added BRepClass_FacePassiveClassifier to handle passive classification scenarios. - Cleaned up redundant files and definitions across BRepClass and BRepTopAdaptor modules. - Improved code readability and maintainability by consolidating methods and reducing file dependencies.
dpasukhi
added
1. Modeling
dpasukhi
changed the title
There was a problem hiding this comment.
Pull request overview
This PR refactors OCCT face/2D classification code by consolidating legacy .gxx/.lxx-based template implementations into .pxx helpers and introducing/rewiring BRep/Geom2dHatch classifier wrappers. It also includes targeted algorithmic cleanups (e.g., edge/triangulation checks) and adds new container APIs to reduce allocations and improve ergonomics.
Changes:
- Migrated
TopClass_FaceClassifier/TopClass_Classifier2dimplementations to.pxxand updated BRepClass/Geom2dHatch wrappers to use them (removing_0.cxxgenerator glue). - Updated BRep class helper headers to use header inlines instead of separate
.lxxfiles, and added/rewired passive/face classifier implementations. - Extended
NCollection_DynamicArray/NCollection_KDTreeAPIs (insert helpers, callback-based range query) and added GTests for the new vector insert operations.
Reviewed changes
Copilot reviewed 40 out of 40 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ModelingData/TKBRep/BRepTools/BRepTools_NurbsConvertModification.hxx | Minor comment formatting tweak. |
| src/ModelingData/TKBRep/BRepTools/BRepTools_GTrsfModification.hxx | Minor comment formatting tweak. |
| src/ModelingAlgorithms/TKTopAlgo/BRepTopAdaptor/FILES.cmake | Removes deleted .lxx from package file list. |
| src/ModelingAlgorithms/TKTopAlgo/BRepTopAdaptor/BRepTopAdaptor_HVertex.lxx | Deletes now-inlined trivial accessors. |
| src/ModelingAlgorithms/TKTopAlgo/BRepTopAdaptor/BRepTopAdaptor_HVertex.hxx | Inlines Vertex()/ChangeVertex() and drops .lxx include. |
| src/ModelingAlgorithms/TKTopAlgo/BRepClass3d/BRepClass3d_SolidExplorer.cxx | Uses modern TopExp_Explorer constructor pattern and adds null-curve guard. |
| src/ModelingAlgorithms/TKTopAlgo/BRepClass/FILES.cmake | Removes legacy _0.cxx/.lxx and adds new classifier .cxx files. |
| src/ModelingAlgorithms/TKTopAlgo/BRepClass/BRepClass_FacePassiveClassifier_0.cxx | Removes old generator-based implementation. |
| src/ModelingAlgorithms/TKTopAlgo/BRepClass/BRepClass_FacePassiveClassifier.hxx | Inlines simple getters and removes generator macro/include block. |
| src/ModelingAlgorithms/TKTopAlgo/BRepClass/BRepClass_FacePassiveClassifier.cxx | New implementation using TopClass_Classifier2d.pxx. |
| src/ModelingAlgorithms/TKTopAlgo/BRepClass/BRepClass_FaceExplorer.lxx | Deletes now-inlined trivial methods. |
| src/ModelingAlgorithms/TKTopAlgo/BRepClass/BRepClass_FaceExplorer.hxx | Inlines trivial iteration methods and drops .lxx include. |
| src/ModelingAlgorithms/TKTopAlgo/BRepClass/BRepClass_FClassifier_0.cxx | Removes old generator-based implementation. |
| src/ModelingAlgorithms/TKTopAlgo/BRepClass/BRepClass_FClassifier.hxx | Inlines simple getters and removes generator macro/include block. |
| src/ModelingAlgorithms/TKTopAlgo/BRepClass/BRepClass_FClassifier.cxx | New implementation using TopClass_FaceClassifier.pxx. |
| src/ModelingAlgorithms/TKTopAlgo/BRepClass/BRepClass_FClass2dOfFClassifier_0.cxx | Removes old generator-based implementation. |
| src/ModelingAlgorithms/TKTopAlgo/BRepClass/BRepClass_FClass2dOfFClassifier.hxx | Inlines simple getters and removes generator macro/include block. |
| src/ModelingAlgorithms/TKTopAlgo/BRepClass/BRepClass_FClass2dOfFClassifier.cxx | New implementation using TopClass_Classifier2d.pxx. |
| src/ModelingAlgorithms/TKTopAlgo/BRepClass/BRepClass_Edge.lxx | Deletes now-inlined trivial accessors. |
| src/ModelingAlgorithms/TKTopAlgo/BRepClass/BRepClass_Edge.hxx | Inlines Edge()/Face() accessors and drops .lxx include. |
| src/ModelingAlgorithms/TKTopAlgo/BRepCheck/BRepCheck_Edge.cxx | Adds early filtering for polygon-on-triangulation checks to avoid unnecessary work. |
| src/ModelingAlgorithms/TKGeomAlgo/TopClass/TopClass_FaceClassifier.pxx | New .pxx implementation (template helpers + lightweight API) replacing .gxx/.lxx. |
| src/ModelingAlgorithms/TKGeomAlgo/TopClass/TopClass_FaceClassifier.lxx | Removed legacy inline-accessor .lxx. |
| src/ModelingAlgorithms/TKGeomAlgo/TopClass/TopClass_FaceClassifier.gxx | Removed legacy generator implementation .gxx. |
| src/ModelingAlgorithms/TKGeomAlgo/TopClass/TopClass_Classifier2d.pxx | New .pxx implementation replacing .gxx/.lxx. |
| src/ModelingAlgorithms/TKGeomAlgo/TopClass/TopClass_Classifier2d.lxx | Removed legacy inline-accessor .lxx. |
| src/ModelingAlgorithms/TKGeomAlgo/TopClass/TopClass_Classifier2d.gxx | Removed legacy generator implementation .gxx. |
| src/ModelingAlgorithms/TKGeomAlgo/TopClass/FILES.cmake | Switches TopClass package file list from .gxx/.lxx to .pxx. |
| src/ModelingAlgorithms/TKGeomAlgo/Geom2dHatch/Geom2dHatch_FClass2dOfClassifier_0.cxx | Removes old generator-based implementation. |
| src/ModelingAlgorithms/TKGeomAlgo/Geom2dHatch/Geom2dHatch_FClass2dOfClassifier.hxx | Inlines simple getters and removes generator macro/include block. |
| src/ModelingAlgorithms/TKGeomAlgo/Geom2dHatch/Geom2dHatch_FClass2dOfClassifier.cxx | New implementation using TopClass_Classifier2d.pxx. |
| src/ModelingAlgorithms/TKGeomAlgo/Geom2dHatch/Geom2dHatch_Classifier_0.cxx | Removes old generator-based implementation. |
| src/ModelingAlgorithms/TKGeomAlgo/Geom2dHatch/Geom2dHatch_Classifier.hxx | Inlines simple getters and removes generator macro/include block. |
| src/ModelingAlgorithms/TKGeomAlgo/Geom2dHatch/Geom2dHatch_Classifier.cxx | New implementation using TopClass_FaceClassifier.pxx. |
| src/ModelingAlgorithms/TKGeomAlgo/Geom2dHatch/FILES.cmake | Updates Geom2dHatch sources list to new .cxx implementations. |
| src/FoundationClasses/TKernel/NCollection/NCollection_KDTree.hxx | Adds ForEachInRange() callback-based range query. |
| src/FoundationClasses/TKernel/NCollection/NCollection_DynamicArray.hxx | Adds InsertAfter/InsertBefore, destructor optimizations, and memcpy-based copy path for trivial types. |
| src/FoundationClasses/TKernel/GTests/NCollection_Vector_Test.cxx | Adds GTests covering InsertAfter/InsertBefore scenarios (incl. move-only type). |
| src/FoundationClasses/TKMath/CSLib/CSLib_Class2d.hxx | Adds constructor overload accepting NCollection_Vector<gp_Pnt2d>. |
| src/FoundationClasses/TKMath/CSLib/CSLib_Class2d.cxx | Implements new vector-based constructor via existing init() template. |
| { | ||
| const size_t aCount = | ||
| (myUsedSize - aUsedSize < myInternalSize) ? myUsedSize - aUsedSize : myInternalSize; | ||
| std::memcpy(aNewBlock, aCurStart, aCount * sizeof(TheItemType)); |
There was a problem hiding this comment.
In copyDate(), std::memcpy() copies bytes into freshly allocated storage that does not have object lifetime started for TheItemType. For C++17 this can be undefined behavior for non-trivial/implicit-lifetime rules, and it also bypasses allocator-aware construction. Consider using std::uninitialized_copy_n (which compilers typically optimize to memcpy for trivially copyable types) or explicitly constructing elements via myAlloc.construct() in a loop.
Suggested change
| std::memcpy(aNewBlock, aCurStart, aCount * sizeof(TheItemType)); | |
| std::uninitialized_copy_n(aCurStart, aCount, aNewBlock); |
Comment on lines
+243
to
+254
| reference InsertAfter(const int theIndex, const TheItemType& theValue) | ||
| { | ||
| Standard_OutOfRange_Raise_if(theIndex < 0 || static_cast<size_t>(theIndex) >= myUsedSize, | ||
| "NCollection_DynamicArray::InsertAfter: index out of range"); | ||
| // Grow by one element at the end. | ||
| Appended(); | ||
| // Shift elements [theIndex+1 .. myUsedSize-2] right by one position. | ||
| for (size_t i = myUsedSize - 1; i > static_cast<size_t>(theIndex) + 1; --i) | ||
| at(i) = std::move(at(i - 1)); | ||
| at(static_cast<size_t>(theIndex) + 1) = theValue; | ||
| return at(static_cast<size_t>(theIndex) + 1); | ||
| } |
There was a problem hiding this comment.
InsertAfter()/InsertBefore() grow the container via Appended(), which default-constructs a new element before shifting. This makes these insert APIs unusable for element types that are not default-constructible (even if they are otherwise movable/copyable). Either rework insertion to create the extra slot without requiring default construction, or document/enforce the constraint (e.g., a static_assert on std::is_default_constructible_v).
Comment on lines
+378
to
+387
| template <typename Functor> | ||
| void ForEachInRange(const ThePointType& theQuery, double theRadius, Functor theFunctor) const | ||
| { | ||
| if (IsEmpty() || theRadius < 0.0) | ||
| { | ||
| return; | ||
| } | ||
| const double aRadiusSq = theRadius * theRadius; | ||
| forEachInRangeRecursive(theQuery, aRadiusSq, 1, static_cast<int>(mySize), 0, theFunctor); | ||
| } |
There was a problem hiding this comment.
ForEachInRange() takes theFunctor by value, which forces an extra copy and prevents use with non-copyable / move-only functors. It also means any state accumulated inside the functor wonât be visible to the caller when passing an lvalue functor. Consider taking Functor&& (or Functor&) and forwarding it through the recursive calls.
Comment on lines
+371
to
+387
| //! Calls theFunctor for each point within theRadius of theQuery. | ||
| //! Zero-allocation alternative to RangeSearch - avoids DynamicArray overhead. | ||
| //! Indices passed to theFunctor are 1-based (same convention as RangeSearch). | ||
| //! @tparam Functor callable with signature void(size_t theIndex) | ||
| //! @param[in] theQuery query point | ||
| //! @param[in] theRadius search radius | ||
| //! @param[in] theFunctor callback invoked for each found 1-based index | ||
| template <typename Functor> | ||
| void ForEachInRange(const ThePointType& theQuery, double theRadius, Functor theFunctor) const | ||
| { | ||
| if (IsEmpty() || theRadius < 0.0) | ||
| { | ||
| return; | ||
| } | ||
| const double aRadiusSq = theRadius * theRadius; | ||
| forEachInRangeRecursive(theQuery, aRadiusSq, 1, static_cast<int>(mySize), 0, theFunctor); | ||
| } |
There was a problem hiding this comment.
NCollection_KDTree has extensive GTest coverage (see existing RangeSearch tests), but the newly added ForEachInRange() callback-based variant is currently untested. Please add unit tests that compare its results against RangeSearch() for a few representative datasets (empty tree, all-in-range, some-in-range, zero radius, negative radius).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
