Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Port Euclidean Cluster Extraction to ROS2 #15

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Tags01
Copy link
Contributor

@Tags01 Tags01 commented Jan 3, 2025

Changes

Modified euclidean_cluster_extraction.cpp

  • Return all found clusters from the service instead of only the first one
  • Replaced iterator for loops with equivalent ranged for b\loops
  • Replaced std::shared_ptr construction with new with equivalent std::make_shared
  • Added in assert statements and static_cast to eliminate possibly unsafe implicit conversions. (Also good for self documentation purposes)
  • Placed the class in an anonymous namespace to avoid ODR violations if duplicate symbols appear during linking.
    Unlikely but possible if two classes have the same name.

Added simple_test_euclidean_cluster_extraction.cpp

Features

Description: Takes in a real-time camera-feed, extracts clusters, then published a new point
cloud with each cluster colored differently.

  • Retrieve a point cloud from a user specified camera topic
  • Down-sample the point cloud to improve performance with pcl::CropBox and pcl::VoxelGrid
  • Use euclidean_cluster_extraction to retrieve the clusters from the point cloud
  • For each cluster in the point cloud, do the following:
    • Pick a visible hue offset, create a pcl::PointXYZHSV, then convert the point to RGB.
    • Linear interpolate this color with the color of the point within a cluster.
    • Add the point to one final point cloud.
  • Publish a PointCloud message to the topic with the clustered colored.
  • Modified the naming convention for constants CONSTANT -> kConstant to follow to the style guide requirement.

Changed

  • Replaced PointCloud&& with PointCloud: This better aligns to the style guide, has the same behavior, and
    better aligned with the usage of the point cloud in the code.
  • Placed the class in an anonymous namespace

Modified PCLEuclideanClusterExtraction.srv

  • Replaced target_cloud_out and obstacle_cloud_list_out with cloud_list_out since the first point cloud is no longer assumed to be the point cloud of interest.

Modified CMakeLists.txt

  • PCL using definition for point types in pcl/point_cloud.h that are not standard compliant. Removed the -pedantic option from simple_test_euclidean_cluster_extraction.cpp since it produces warnings for these definitions. No warnings are produced as a result of my code.
  • Moved the Clang/GCC check under build testing down to after the targets were defined.
  • Updated the comment about the resolved Boost MPL/Numeric Clang compiler error.
  • Added PCL_NO_PRECOMPILE to address issues with Address Sanitizer in gcc.
  • Added dependency on the transitive dependency, Eigen, which used in the simple test.

Compatability/Testing

All tests done on Ubuntu 22.04

Platform Compiler Compilation Asan Lsan Ubsan Simple Test
x64 clang++ (19.1.0)
x64 g++ (11.4.0) ✓ (1)
x64 icpx (2024.2.1) x (2) x (2) x (2) x (2) x (2)
aarch64 clang++ (19.1.6)
aarch64 g++ (11.4.0) ✓ (1)

Asan = Address Sanitizer; Lsan = Leak Sanitizer; Ubsan = Undefined Behavior Sanitizer
icpx was not tested on aarch64 since it failed on x64

  1. Address sanitizer shows a heap buffer overflow of 8 bytes, however this error went away when adding the PCL
  2. icpx triggered a segmentation fault on compilation. This was in the compiler itself which is very unusual but did not occur in gcc or clang. This appears to be an internal compiler issue.

Notes

  • valgrind: no memory errors, two shown memory unrelated memory leaks (do not result from my code).
  • Compiler produces no warnings on -Wall -Wextra. -Wpedantic produces warnings only from PCL header files.
  • There's a known issue in ROS2 with new_delete_type_mismatch, to disable the error, run
    export ASAN_OPTIONS=new_delete_type_mismatch=0

Results

test.webm

Tags01 added 3 commits August 20, 2024 14:29
…,ubsan,lint,Wall/Wextra/pedantic

internal segfault in icpx (compiler bug), workaround for g++ asan

* Modified euclidean_cluster_extraction.cpp
- Return all found clusters from the service instead of only the first one
- Replaced iterator for loops with equivalent ranged for b\loops
- Replaced `std::shared_ptr` construction with `new` with equivalent `std::make_shared`
- Added in `assert` statements and `static_cast` to eliminate possibly unsafe implicit conversions. (Also good for self documentation purposes)
- Placed the class in an anonymous namespace to avoid ODR violations if duplicate symbols appear during linking.
Unlikely but possible if two classes have the same name.

* Added simple_test_euclidean_cluster_extraction.cpp
** Features
Description: Takes in a real-time camera-feed, extracts clusters, then published a new point
cloud with each cluster colored differently.
- Retrieve a point cloud from a user specified camera topic
- Down-sample the point cloud to improve performance by using `pcl::CropBox` and `pcl::VoxelGrid`
- Use `euclidean_cluster_extraction` to retrieve the clusters from the point cloud
- For each cluster in the point cloud, do the following:
    - Pick a visible hue offset, create a `pcl::HSVPoint`, then convert the point to RGB
    - Linear interpolate this color with the color of the point within a cluster
    - Add the point to one final point cloud
- Publish a `PointCloud` message to the topic with the clustered colored.
- Modified the naming convention for constants `CONSTANT -> kConstant` to follow to the style guide requirement.

** Changed
- Replaced `PointCloud&&` with `PointCloud`: This better aligns to the style guide, has the same behavior, and
better aligned with the usage of the point cloud in the code.
- Placed the class in an anonymous namespace

* Modified PCLEuclideanClusterExtraction.srv
- Replaced `target_cloud_out` and `obstacle_cloud_list_out` with `cloud_list_out` since the first point cloud is no longer assumed to be the point cloud of interest.

* Modified CMakeLists.txt
- PCL using definition for point types in `pcl/point_cloud.h` that are not standard compliant. Removed the `-pedantic` option from `simple_test_euclidean_cluster_extraction.cpp` since it produces warnings for these definitions. No warnings are produced as a result of my code.
- Moved the Clang/GCC check under build testing down to after the targets were defined.
- Updated the comment about the resolved Boost MPL/Numeric Clang compiler error.
- Added `PCL_NO_PRECOMPILE` to address issues with Address Sanitizer in `gcc`.
cleaned Up comments in CMakeLists.txt
@Tags01 Tags01 self-assigned this Jan 3, 2025
@Tags01 Tags01 linked an issue Jan 3, 2025 that may be closed by this pull request
@Tags01
Copy link
Contributor Author

Tags01 commented Jan 6, 2025

Fixed the issue where add_definitions(PCL_NO_PRECOMPILE) treats PCL_NO_PRECOMPILE as a compile flag not an option. Corrected to -DPCL_NO_PRECOMPILE

Copy link
Member

@GregoryLeMasurier GregoryLeMasurier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly ordering for includes, need to check with the others about no precompile, and a question about lerp

)

include_directories(${PCL_INCLUDE_DIRS})
link_directories(${PCL_LIBRARY_DIRS})
add_definitions(${PCL_DEFINITIONS})
# PCL_NO_PRECOMPILE is required to avoid address sanitizer issues in clang,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should discuss this with the group - Does this impact compile time?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I believe is does, I have not profiled the exact time yet

pcl_utilities/src/euclidean_cluster_extraction_service.cpp Outdated Show resolved Hide resolved
Copy link
Member

@GregoryLeMasurier GregoryLeMasurier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me - will resolve merge conflicts then pass it along to others to review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Port pcl_euclidean_cluster_extraction_service.cpp to ROS2
2 participants