Port depth image proc on ROS2 #362

yechun1 · 2018-10-22T08:23:54Z

No description provided.

* Port depth_image_proc of image_pipeline on ROS2 * rename Nodelets as Node * add launch examples, such as "ros2 launch depth_image_proc point_cloud_xyzi.launch.py" * verified point_cloud_xyzrgb, point_cloud_xyz, convert_metric based on Realsense camera(https://github.com/intel/ros2_intel_realsense). Signed-off-by: Chris Ye <chris.ye@intel.com>

Signed-off-by: Chris Ye <chris.ye@intel.com>

- rename raw topic as image_transport fixed the issue (ros-perception/image_common#96) - update test example of point_cloud_xyzrgb.launch Signed-off-by: Chris Ye <chris.ye@intel.com>

yechun1 · 2018-11-02T07:09:31Z

This PR is the first module(depth_image_proc) porting from image_pipeline. Before going ahead to port others, we hope the PR could be reviewed and merged, so that we could apply the same changes on later porting. And once the PR could be merged, we would also take effort to build and release depth_image_proc with bloom before Crystal release.

This PR passed the colcon test and most of nodes have be verified based on Realsense camera(as kinect not work on ROS2 right now, some features such as radial could not be verified with Realsense). The nodes could be implemented by ros2 launch, for example: to generate pointcloud with rgb and depth, with the command:
"ros2 launch depth_image_proc point_cloud_xyzrgb.launch.py"

Some dependent APIs are not implemented on ROS2, we have added TODOs with workaround in the code, and the depth_image_proc features will not be impact by those workaround, we think this PR is ready for merge, and we would submit other PRs in the future to fix TODOs.

@mjcarroll would you please help to review or ask someone help to review this patch? many thanks.

mkhansenbot · 2018-11-06T01:30:32Z

@mjcarroll, can you review this or is someone else from OSRF available to help?

mjcarroll

Some initial feedback, I need to check this out and build/run as well.

mjcarroll · 2018-11-06T18:57:26Z

depth_image_proc/CMakeLists.txt

+find_package(stereo_msgs REQUIRED)
+find_package(tf2 REQUIRED)
+find_package(tf2_ros REQUIRED)
+

 if(cv_bridge_VERSION VERSION_GREATER "1.12.0")


This logic is not needed now, as we're already setting the standard above.

Removed cv_bridge_VERSION check logic, fixed in c13e2a3

mjcarroll · 2018-11-06T18:57:54Z

depth_image_proc/CMakeLists.txt

 )
-target_link_libraries(${PROJECT_NAME} ${catkin_LIBRARIES} ${OpenCV_LIBRARIES})
+ament_target_dependencies(${PROJECT_NAME}
+  "image_transport"


Nit: The quotes aren't necessary here.

If remove "image_transport" from ament_target_dependencies, the build will fail with "fatal error: image_transport/image_transport.h: No such file or directory", so I just keep it.

ament_target_dependencies(${PROJECT_NAME} # "image_transport"

mjcarroll · 2018-11-06T19:01:11Z

depth_image_proc/CMakeLists.txt

-catkin_package(
-    INCLUDE_DIRS include
-    LIBRARIES ${PROJECT_NAME})
-
 find_package(Boost REQUIRED)


Is Boost still being used? It looks like it has been removed.

depth_image_proc/package.xml

* remove boost which is unused on ROS2 * remove cv_bridge version check logic as setted in find_package * update maintainer in package.xml Signed-off-by: Chris Ye <chris.ye@intel.com>

yechun1 · 2018-11-07T02:07:29Z

@mjcarroll thanks for your effort to review the code, I have updated the patch, would you please help to check? Any comments are welcome, We'll hurry up and update the patch ASAP. We'll try best to make the PR ready for merge and build bloom package in Crystal release.

pass test: ros2 launch depth_image_proc point_cloud_xyzrgb.launch.py ros2 launch depth_image_proc point_cloud_xyz.launch.py ros2 launch depth_image_proc convert_metric.launch.py ros2 launch depth_image_proc crop_foremost.launch.py ros2 launch depth_image_proc point_cloud_xyz_radial.launch.py ros2 launch depth_image_proc disparity.launch.py ros2 launch depth_image_proc register.launch.py ros2 launch depth_image_proc point_cloud_xyzi.launch.py ros2 launch depth_image_proc point_cloud_xyzi_radial.launch.py Signed-off-by: Chris Ye <chris.ye@intel.com>

yechun1 · 2018-11-07T09:56:12Z

One more commit: 5e5b54b
Passed implementation tests based on Realsense camera and display image/points on RVIZ.

Enabled demo launcher
ros2 launch depth_image_proc point_cloud_xyzrgb.launch.py
ros2 launch depth_image_proc point_cloud_xyz.launch.py
ros2 launch depth_image_proc convert_metric.launch.py
ros2 launch depth_image_proc crop_foremost.launch.py
ros2 launch depth_image_proc point_cloud_xyz_radial.launch.py
ros2 launch depth_image_proc disparity.launch.py
ros2 launch depth_image_proc register.launch.py
ros2 launch depth_image_proc point_cloud_xyzi.launch.py
ros2 launch depth_image_proc point_cloud_xyzi_radial.launch.py

mkhansenbot · 2018-11-16T00:05:32Z

@mjcarroll @tfoote - can we have a second review on this one, it is gating one of our packages for Crystal release

mjcarroll · 2018-11-16T01:34:43Z

Hey @mkhansen-intel I have been looking at this one this afternoon and it made me recognize a potential shortcoming in the way that image_transport and message_filters are implemented.

Currently, we make those two libraries take rclcpp::Node::SharedPtr, which makes the code a bit unweildy when it's used as part of an object that inherits from rclcpp::Node. I have some proposed suggestions to those two packages as well as a bit of refactoring here incoming in the next few hours.

mjcarroll · 2018-11-16T01:37:25Z

depth_image_proc/src/nodelets/convert_metric.cpp

 {
-  ros::NodeHandle& nh = getNodeHandle();
-  it_.reset(new image_transport::ImageTransport(nh));
+  rclcpp::Node::SharedPtr node = std::shared_ptr<rclcpp::Node>(this);


While this appears to work for you, it looks to be unsafe based on the documentation is here: https://en.cppreference.com/w/cpp/memory/enable_shared_from_this

The docs indicate:

enable_shared_from_this provides the safe alternative to an expression like std::shared_ptr(this), which is likely to result in this being destructed more than once by multiple owners that are unaware of each other (see example below)

The first workaround that I thought of is that we can just use shared_from_this, but using it in the constructor could also lead to potentially undefined behavior.

My proposed workaround is to allow message_filters and image_transport take a raw pointer to Node, because it's unlikely that the SharedPtr is providing real benefit or safety here.

mkhansenbot · 2018-11-16T01:37:27Z

Thanks @mjcarroll for giving this a high priority!

mjcarroll · 2018-11-16T06:10:22Z

I've made some progress but have to stop for a bit, you can see what I have so far here: https://github.com/ros-perception/image_pipeline/tree/mjcarroll/pointer_api_updates

I think that shows most of the direction that I would expect it to go in.

In the meantime, please take a look at, which both block those changes:

Signed-off-by: Chris Ye <chris.ye@intel.com>

yechun1 · 2018-11-16T08:54:56Z

Let me continue the progress and complete the code changes by 202411f

Block changes:

mjcarroll · 2018-11-16T15:16:53Z

@yechun1 Since we don't have any tests in here, it would be really helpful if you could test with your hardware or add tests.

I don't believe that any of these changes should fundamentally change behavior, but there is always an opportunity to introduce a bug or two in there.

mjcarroll · 2018-11-16T22:47:43Z

There is also an update to the transport plugins here: ros-perception/image_transport_plugins#31

yechun1 · 2018-11-17T00:28:54Z

@mjcaroll, we have added launch demo for all depth_image_proc nodes, and verified the published topic (include pointcloud and images) could be displayed via RVIZ.

below commands are workable but depends on three block changes list above.

ros2 launch depth_image_proc point_cloud_xyzrgb.launch.py
ros2 launch depth_image_proc point_cloud_xyz.launch.py
ros2 launch depth_image_proc convert_metric.launch.py
ros2 launch depth_image_proc crop_foremost.launch.py
ros2 launch depth_image_proc point_cloud_xyz_radial.launch.py
ros2 launch depth_image_proc disparity.launch.py
ros2 launch depth_image_proc register.launch.py
ros2 launch depth_image_proc point_cloud_xyzi.launch.py
ros2 launch depth_image_proc point_cloud_xyzi_radial.launch.py

yechun1 · 2018-11-17T00:36:15Z

The launcher tests based on Intel Realsense R435 Camera Hardware as resource input.

yechun1 · 2018-11-22T08:26:45Z

@mjcarroll thanks very much for merging this code :)
The dependent PR (ros-perception/image_common#104) is also require to merge

* Port depth_image_proc on ROS2 * Port depth_image_proc of image_pipeline on ROS2 * rename Nodelets as Node * add launch examples, such as "ros2 launch depth_image_proc point_cloud_xyzi.launch.py" * verified point_cloud_xyzrgb, point_cloud_xyz, convert_metric based on Realsense camera(https://github.com/intel/ros2_intel_realsense). Signed-off-by: Chris Ye <chris.ye@intel.com> * add ament_lint_auto test and adjust code style Signed-off-by: Chris Ye <chris.ye@intel.com> * update test example for depth_image_proc - rename raw topic as image_transport fixed the issue (ros-perception/image_common#96) - update test example of point_cloud_xyzrgb.launch Signed-off-by: Chris Ye <chris.ye@intel.com> * remove unused dependence in cmakelist * remove boost which is unused on ROS2 * remove cv_bridge version check logic as setted in find_package * update maintainer in package.xml Signed-off-by: Chris Ye <chris.ye@intel.com> * added all example launchers for demo test pass test: ros2 launch depth_image_proc point_cloud_xyzrgb.launch.py ros2 launch depth_image_proc point_cloud_xyz.launch.py ros2 launch depth_image_proc convert_metric.launch.py ros2 launch depth_image_proc crop_foremost.launch.py ros2 launch depth_image_proc point_cloud_xyz_radial.launch.py ros2 launch depth_image_proc disparity.launch.py ros2 launch depth_image_proc register.launch.py ros2 launch depth_image_proc point_cloud_xyzi.launch.py ros2 launch depth_image_proc point_cloud_xyzi_radial.launch.py Signed-off-by: Chris Ye <chris.ye@intel.com> * @wip update to use raw pointers. * continue to update to use raw pointer Signed-off-by: Chris Ye <chris.ye@intel.com>

Port depth image proc on ROS2 (ros-perception#362) * Port depth_image_proc on ROS2 * Port depth_image_proc of image_pipeline on ROS2 * rename Nodelets as Node * add launch examples, such as "ros2 launch depth_image_proc point_cloud_xyzi.launch.py" * verified point_cloud_xyzrgb, point_cloud_xyz, convert_metric based on Realsense camera(https://github.com/intel/ros2_intel_realsense). Signed-off-by: Chris Ye <chris.ye@intel.com> * add ament_lint_auto test and adjust code style Signed-off-by: Chris Ye <chris.ye@intel.com> * update test example for depth_image_proc - rename raw topic as image_transport fixed the issue (ros-perception/image_common#96) - update test example of point_cloud_xyzrgb.launch Signed-off-by: Chris Ye <chris.ye@intel.com> * remove unused dependence in cmakelist * remove boost which is unused on ROS2 * remove cv_bridge version check logic as setted in find_package * update maintainer in package.xml Signed-off-by: Chris Ye <chris.ye@intel.com> * added all example launchers for demo test pass test: ros2 launch depth_image_proc point_cloud_xyzrgb.launch.py ros2 launch depth_image_proc point_cloud_xyz.launch.py ros2 launch depth_image_proc convert_metric.launch.py ros2 launch depth_image_proc crop_foremost.launch.py ros2 launch depth_image_proc point_cloud_xyz_radial.launch.py ros2 launch depth_image_proc disparity.launch.py ros2 launch depth_image_proc register.launch.py ros2 launch depth_image_proc point_cloud_xyzi.launch.py ros2 launch depth_image_proc point_cloud_xyzi_radial.launch.py Signed-off-by: Chris Ye <chris.ye@intel.com> * @wip update to use raw pointers. * continue to update to use raw pointer Signed-off-by: Chris Ye <chris.ye@intel.com> enable rclcpp_register_node_plugins (ros-perception#368) this may be remarked while code debugging, should be enabled to build node plugin file and added points remap in point_cloud_xyzrgb.launch.py port image_publisher on ROS2 (ros-perception#366) * port image_publisher on ROS2 * switch to use cmake 3.5 * change nodelet to classloader * change ros::param to ros2 parameter APIs * use ros2 code style * enable ros2 camera_info_manager Changelogs. 2.0.0 code change for ros2 image_view porting run example: ros2 run image_view image_view --image /camera/color/image_raw add launch file to consist with ros image_view: ros2 launch image_view image_view.launch.py image:=<image topic> disparity_view: ros2 launch image_view disparity_view.launch.py image:=<disparity image topic> stereo_view: ros2 launch image_view stereo_view stereo:=<stereo namespace> image:=<image topic identifier> image_saver: ros2 launch image_view image_saver.launch.py image:=<image topic> extract_images: ros2 launch image_view extract_images.launch.py image:=<image topic> video_recorder: ros2 launch image_view video_rocorder.launch.py images:=<image topic> Details in README.md

yechun1 added 2 commits October 22, 2018 16:23

add ament_lint_auto test and adjust code style

958052d

Signed-off-by: Chris Ye <chris.ye@intel.com>

yechun1 mentioned this pull request Oct 23, 2018

Port Image_pipeline on ROS2 #360

Closed

7 tasks

yechun1 force-pushed the port_depth_image_proc branch from 65d3779 to fe8b68b Compare November 2, 2018 06:37

update test example for depth_image_proc

fe8b68b

- rename raw topic as image_transport fixed the issue (ros-perception/image_common#96) - update test example of point_cloud_xyzrgb.launch Signed-off-by: Chris Ye <chris.ye@intel.com>

mjcarroll self-requested a review November 6, 2018 13:35

mjcarroll requested changes Nov 6, 2018

View reviewed changes

remove unused dependence in cmakelist

c13e2a3

* remove boost which is unused on ROS2 * remove cv_bridge version check logic as setted in find_package * update maintainer in package.xml Signed-off-by: Chris Ye <chris.ye@intel.com>

yechun1 force-pushed the port_depth_image_proc branch from 67c166b to 5e5b54b Compare November 7, 2018 09:53

mkhansenbot mentioned this pull request Nov 15, 2018

ROS 2 Crystal Clemmys ros2/ros2#529

Closed

34 tasks

mjcarroll reviewed Nov 16, 2018

View reviewed changes

@wip update to use raw pointers.

c211475

continue to update to use raw pointer

202411f

Signed-off-by: Chris Ye <chris.ye@intel.com>

mjcarroll approved these changes Nov 21, 2018

View reviewed changes

mjcarroll merged commit 843b932 into ros-perception:ros2 Nov 21, 2018

yechun1 deleted the port_depth_image_proc branch November 27, 2018 05:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port depth image proc on ROS2 #362

Port depth image proc on ROS2 #362

yechun1 commented Oct 22, 2018

yechun1 commented Nov 2, 2018

mkhansenbot commented Nov 6, 2018

mjcarroll left a comment

mjcarroll Nov 6, 2018

yechun1 Nov 7, 2018

mjcarroll Nov 6, 2018

yechun1 Nov 7, 2018

mjcarroll Nov 6, 2018

yechun1 Nov 7, 2018

yechun1 commented Nov 7, 2018

yechun1 commented Nov 7, 2018 •

edited

Loading

mkhansenbot commented Nov 16, 2018

mjcarroll commented Nov 16, 2018

mjcarroll Nov 16, 2018

mjcarroll Nov 16, 2018

mkhansenbot commented Nov 16, 2018

mjcarroll commented Nov 16, 2018

yechun1 commented Nov 16, 2018

mjcarroll commented Nov 16, 2018

mjcarroll commented Nov 16, 2018

yechun1 commented Nov 17, 2018

yechun1 commented Nov 17, 2018

yechun1 commented Nov 22, 2018

Port depth image proc on ROS2 #362

Port depth image proc on ROS2 #362

Conversation

yechun1 commented Oct 22, 2018

yechun1 commented Nov 2, 2018

mkhansenbot commented Nov 6, 2018

mjcarroll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yechun1 commented Nov 7, 2018

yechun1 commented Nov 7, 2018 • edited Loading

mkhansenbot commented Nov 16, 2018

mjcarroll commented Nov 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkhansenbot commented Nov 16, 2018

mjcarroll commented Nov 16, 2018

yechun1 commented Nov 16, 2018

mjcarroll commented Nov 16, 2018

mjcarroll commented Nov 16, 2018

yechun1 commented Nov 17, 2018

yechun1 commented Nov 17, 2018

yechun1 commented Nov 22, 2018

yechun1 commented Nov 7, 2018 •

edited

Loading