search

koide3 / small_gicp

Efficient and parallel algorithms for point cloud registration [C++, Python]
MIT License
364 stars 46 forks source link

feat: add package.xml #26

Closed wep21 closed 4 months ago

wep21 commented 4 months ago
koide3 commented 4 months ago

Might be better to drop the dependency on PCL and TBB as well to avoid forcing people to install the huge dependency.

I'm wondering if we should directly add package.xml to small_gicp or we should create a separate vendor package. I tend to want to keep a package isolated from ROS when it's possible.

@valgur Do you have any opinions?

koide3 commented 4 months ago

I think it would be reasonable to add package.xml because many ROS people would be interested in this package. I would like to merge this PR once libpcl-all-dev and tbb are removed.

valgur commented 4 months ago

The library is definitely relevant to ROS, so ROS 1/2 packaging support directly in the repo would be welcome, I think.

However, the current PR is incomplete for a catkin package. It's missing find_package(catkin ...), catkin_package() and catkin-specific install() commands. Creating a separate cmake/catkin.cmake that could then be conditionally included might make sense to keep things clean. The install commands in the main CMakeLists.txt can be made configurable, so they can be overridden for catkin, perhaps.

I would also strongly encourage adding a minimal CI workflow to test this.

Might be worth looking into adding ROS 2 / Ament support at the same time as well.

wep21 commented 4 months ago

make pcl and tbb optional in package.xml at https://github.com/koide3/small_gicp/pull/26/commits/9ff9b1b68d44d15536f9e80272a8cecaeff55a9e.

codecov-commenter commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 91.77%. Comparing base (06114f8) to head (d331f08).

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #26 +/- ## ======================================= Coverage 91.77% 91.77% ======================================= Files 35 35 Lines 1179 1179 Branches 182 182 ======================================= Hits 1082 1082 Misses 97 97 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

wep21 commented 4 months ago

I guess This minimum change make the package behave as both catkin/ament package.

koide3 commented 4 months ago

Thanks. I think we can merge the PR in the current form for now and add further modifications later.