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 depth image proc on ROS2 #362

Merged
merged 7 commits into from Nov 21, 2018

Conversation

Projects
None yet
3 participants
@yechun1

yechun1 commented Oct 22, 2018

No description provided.

yechun1 added some commits Oct 12, 2018

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>

@yechun1 yechun1 referenced this pull request Oct 23, 2018

Open

Port Image_pipeline on ROS2 #360

2 of 7 tasks complete

@yechun1 yechun1 force-pushed the yechun1:port_depth_image_proc branch from 65d3779 to fe8b68b Nov 2, 2018

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>
@yechun1

This comment has been minimized.

yechun1 commented Nov 2, 2018

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.

@mkhansen-intel

This comment has been minimized.

mkhansen-intel commented Nov 6, 2018

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

@mjcarroll mjcarroll self-requested a review Nov 6, 2018

@mjcarroll

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

find_package(stereo_msgs REQUIRED)
find_package(tf2 REQUIRED)
find_package(tf2_ros REQUIRED)
if(cv_bridge_VERSION VERSION_GREATER "1.12.0")

This comment has been minimized.

@mjcarroll

mjcarroll Nov 6, 2018

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

This comment has been minimized.

@yechun1

yechun1 Nov 7, 2018

Removed cv_bridge_VERSION check logic, fixed in c13e2a3

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

This comment has been minimized.

@mjcarroll

mjcarroll Nov 6, 2018

Nit: The quotes aren't necessary here.

This comment has been minimized.

@yechun1

yechun1 Nov 7, 2018

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"
catkin_package(
INCLUDE_DIRS include
LIBRARIES ${PROJECT_NAME})
find_package(Boost REQUIRED)

This comment has been minimized.

@mjcarroll

mjcarroll Nov 6, 2018

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

This comment has been minimized.

@yechun1
Show resolved Hide resolved depth_image_proc/package.xml
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>
@yechun1

This comment has been minimized.

yechun1 commented Nov 7, 2018

@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.

@yechun1 yechun1 force-pushed the yechun1:port_depth_image_proc branch from 67c166b to 5e5b54b Nov 7, 2018

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>
@yechun1

This comment has been minimized.

yechun1 commented Nov 7, 2018

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

@mkhansen-intel mkhansen-intel referenced this pull request Nov 15, 2018

Open

ROS 2 Crystal Clemmys #529

13 of 34 tasks complete
@mkhansen-intel

This comment has been minimized.

mkhansen-intel commented Nov 16, 2018

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

@mjcarroll

This comment has been minimized.

mjcarroll commented Nov 16, 2018

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.

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

This comment has been minimized.

@mjcarroll

mjcarroll Nov 16, 2018

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.

This comment has been minimized.

@mjcarroll

mjcarroll Nov 16, 2018

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.

@mkhansen-intel

This comment has been minimized.

mkhansen-intel commented Nov 16, 2018

Thanks @mjcarroll for giving this a high priority!

@mjcarroll

This comment has been minimized.

mjcarroll commented Nov 16, 2018

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:

continue to update to use raw pointer
Signed-off-by: Chris Ye <chris.ye@intel.com>
@yechun1

This comment has been minimized.

yechun1 commented Nov 16, 2018

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

Block changes:

@mjcarroll

This comment has been minimized.

mjcarroll commented Nov 16, 2018

@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

This comment has been minimized.

mjcarroll commented Nov 16, 2018

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

@yechun1

This comment has been minimized.

yechun1 commented Nov 17, 2018

@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

This comment has been minimized.

yechun1 commented Nov 17, 2018

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

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

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
@yechun1

This comment has been minimized.

yechun1 commented Nov 22, 2018

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

@yechun1 yechun1 deleted the yechun1:port_depth_image_proc branch Nov 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment