Add multithread support to boundary estimation #6389
Conversation
larshg
added
changelog: enhancement
There was a problem hiding this comment.
Pull request overview
This PR adds OpenMP-based multithreading support to the BoundaryEstimation class to enable parallel computation of boundary points. The implementation follows the same pattern used in other PCL features like NormalEstimationOMP.
Changes:
- Added
setNumberOfThreads()method to control thread count - Added OpenMP pragma directives to parallelize the main computation loops
- Added member variables
threads_andchunk_size_to manage parallel execution
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| features/include/pcl/features/boundary.h | Declares setNumberOfThreads() method and adds threads_ and chunk_size_ member variables |
| features/include/pcl/features/impl/boundary.hpp | Implements setNumberOfThreads() and adds OpenMP parallel directives to computeFeature() loops with appropriate variable scoping |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
larshg
force-pushed
the
addmtboundaryest
branch
from
6935412 to
9b9f691
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mvieth
left a comment
mvieth
left a comment
There was a problem hiding this comment.
getCoordinateSystemOnPlane(...) and isBoundaryPoint(...) should be marked as const, to make it clear that they do not change any class member. Looks good otherwise.
Calculcate normals only once.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| ////////////////////////////////////////////////////////////////////////////////////////////////////////////////// | ||
| TEST (PCL, BoundaryEstimation) | ||
| TEST(PCL, BoundaryEstimationOMP) |
There was a problem hiding this comment.
The test case name "BoundaryEstimationOMP" suggests OpenMP-specific testing, but the implementation adds multithreading directly to the base BoundaryEstimation class rather than creating a separate BoundaryEstimationOMP class. This is inconsistent with how other OMP features are tested in PCL (e.g., NormalEstimationOMP in test_normal_estimation.cpp). Consider renaming this test to something like "BoundaryEstimationMultithreaded" to better reflect that it's testing the multithreading capability of the base class, not a separate OMP-specific class.
| TEST(PCL, BoundaryEstimationOMP) | |
| TEST(PCL, BoundaryEstimationMultithreaded) |
2 participants
Fixes #6387