search

ros-perception / depthimage_to_laserscan

Converts a depth image to a laser scan for use with navigation and localization.
257 stars 173 forks source link

Install includes to ${PROJECT_NAME} folder and use modern CMake #58

Closed sloretz closed 2 years ago

sloretz commented 2 years ago

~Targeting foxy-devel and not ros2 because the former seems to be marked as the upstream development branch, but I would strongly recommend releasing this change only to ROS Rolling. Maybe it's time to fast-forward ros2 to foxy-devel and change the ROS Rolling branch to ros2?~ Targeting ros2 now :tada:

https://index.ros.org/p/depthimage_to_laserscan/#rolling Part of ros2/ros2#1150

This installs includes to include/${PROJECT_NAME} to mitigate include directory search order issues when overriding packages in desktop.

~Part of ament/ament_cmake#365~

~This exports modern CMake targets and removes ament_export_libraries and ament_export_include_directories as they're redundant with the exported CMake targets.~ Edit: Added back to minimize disruption

Part of ament/ament_cmake#292

This replaces ament_target_dependencies() calls with target_link_libraries().

I also reduced the dependency on OpenCV from all of it, to just the core library as that's all that appears to be used.

clalancette commented 2 years ago

Maybe it's time to fast-forward ros2 to foxy-devel and change the ROS Rolling branch to ros2?

Yeah, totally agreed on that. I'll go ahead and do that.

clalancette commented 2 years ago

https://github.com/ros/rosdistro/pull/31688 is now merged in, so Rolling targets ros2. I'm thus going to retarget this PR to the 'ros2' branch.

clalancette commented 2 years ago

@ros-pull-request-builder retest this please

sloretz commented 2 years ago 
13:27:47 CMake Error at CMakeLists.txt:53 (add_executable):
13:27:47   Target "depthimage_to_laserscan_node" links to target
13:27:47   "image_geometry::image_geometry" but the target was not found.  Perhaps a
13:27:47   find_package() call is missing for an IMPORTED target, or an ALIAS target
13:27:47   is missing?

Ah, this probably requires the vision_opencv PR I'm about to open up

clalancette commented 2 years ago

Ah, this probably requires the vision_opencv PR I'm about to open up

I was just going to mention that :).

sloretz commented 2 years ago

Looks like vision-opencv is still 2.2.1 in http://repo.ros2.org/status_page/rolling_default.html?q=vision because the Rolling packages haven't been sync'd to testing yet.. I'll wait a bit longer to rerun the PR job.

clalancette commented 2 years ago

@ros-pull-request-builder retest this please