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

update depth_image_proc components #543

Merged
merged 2 commits into from Jun 11, 2020
Merged

update depth_image_proc components #543

merged 2 commits into from Jun 11, 2020

Conversation

mikeferguson
Copy link
Member

@mikeferguson mikeferguson commented Jun 11, 2020

This makes them loadable with the rclcpp_components
interface. I've fully tested PointCloudXYZRGB and
ConvertMetric, my use case doesn't use the others.
I also lack a setup to test the launch files fully,
but ran them with the realsense commented out and
they appear to be OK.

This makes them loadable with the rclcpp_components
interface. I've fully tested PointCloudXYZRGB and
ConvertMetric, my use case doesn't use the others.
I also lack a setup to test the launch files fully,
but ran them with the realsense commented out and
they appear to be OK.
@mikeferguson
Copy link
Member Author

mikeferguson commented Jun 11, 2020

CI seems to have failed in image_view -- which I'm not changing...

@SteveMacenski
Copy link
Member

SteveMacenski commented Jun 11, 2020

@mikeferguson yeah, you can ignore the image_view failures in the PR builder, but Circle should pass without an issue

  >>>
 
build/depth_image_proc/test_results/depth_image_proc/flake8.xunit.xml: 2 tests, 0 errors, 2 failures, 0 skipped
 
- depth_image_proc.flake8 E127 (./launch/point_cloud_xyzrgb.launch.py:67:34)
 
  <<< failure message
 
    continuation line over-indented for visual indent:
 
                                     ('points', '/camera/depth_registered/points')]
 
  >>>
 
- depth_image_proc.flake8 E501 (./launch/register.launch.py:68:100)
 
  <<< failure message
 
    line too long (105 > 99 characters):
 
                                    ('depth_registered/camera_info', '/camera/depth_registered/camera_info')]
 
  >>>
 

Looks like you need to lint a few lines. I get daily failure emails reminding me about image_view, I'm hoping to get to it this week, its an annoying boost issue I just need to sit down and play with.

Copy link
Member

@SteveMacenski SteveMacenski left a comment

LGTM when CircleCi passes

Copy link
Collaborator

@JWhitleyWork JWhitleyWork left a comment

It looks like you're missing the CMake changes necessary to support registration as composable nodes. First, the nodes have to be part of a shared library target (not an executable target). Then, you need to register them with an rclcpp_components macro:

rclcpp_components_register_nodes(library_target_name Plugin::Class::Name)

@mikeferguson
Copy link
Member Author

mikeferguson commented Jun 11, 2020

@SteveMacenski - I'll get the python fixed up later today.

@JWhitleyWork - it's already a library and the rclcpp_components_register_nodes directives were already there. Basically, without my changes, ComponentManager could already find the library and load it, but was unable to find the plugins since they had the wrong ClassLoader directive and constructor signature (I'm guessing they were ported at an earlier time when the API of components was still volatile). I have actually loaded these plugins and used them to construct a point cloud from a Primesense camera using a horrible hacky port of openni2_camera.

@mikeferguson
Copy link
Member Author

mikeferguson commented Jun 11, 2020

@SteveMacenski circle ci is now passing!

@SteveMacenski SteveMacenski merged commit d929692 into ros-perception:ros2 Jun 11, 2020
1 of 2 checks passed
@SteveMacenski
Copy link
Member

SteveMacenski commented Jun 11, 2020

Thanks for the contribution!

@mikeferguson mikeferguson deleted the depth_image_proc_components branch Jun 12, 2020
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.

None yet

3 participants