Update theora_image_transport to ROS2 #28
Update theora_image_transport to ROS2 #28mjcarroll merged 5 commits intoros-perception:ros2-develfrom
Conversation
tfoote
left a comment
There was a problem hiding this comment.
A read through looks good to me. We should just make sure we've got validated CI.
mikaelarguedas
left a comment
There was a problem hiding this comment.
quick review of the CMake code, didnt review the actual code change
| find_package(sensor_msgs REQUIRED) | ||
| find_package(std_msgs REQUIRED) | ||
|
|
||
| find_package(OpenCV REQUIRED) |
There was a problem hiding this comment.
It looks like only the imgproc module from OpenCV is used. We should use that instead to avoid including / linking all OpenCV
| install(TARGETS ${LIBRARY_NAME} | ||
| ARCHIVE DESTINATION lib | ||
| LIBRARY DESTINATION lib | ||
| RUNTIME DESTINATION bin |
There was a problem hiding this comment.
This target is installed L109 so this block can be removed
| ${PC_THEORADEC_LIBRARIES} | ||
| install( | ||
| DIRECTORY "include/" | ||
| DESTINATION include |
There was a problem hiding this comment.
This directory is installed L115 so this install statement can be removed
|
|
||
| ament_export_dependencies(rosidl_default_runtime) | ||
|
|
||
| install(TARGETS ${LIBRARY_NAME} ogg_saver |
There was a problem hiding this comment.
if ogg_saver is an executable that we expect to run using ros2 run, we should install it in to lib/${PROJECT_NAME}
| ARCHIVE DESTINATION ${CATKIN_PACKAGE_LIB_DESTINATION} | ||
| LIBRARY DESTINATION ${CATKIN_PACKAGE_LIB_DESTINATION} | ||
| RUNTIME DESTINATION ${CATKIN_PACKAGE_BIN_DESTINATION} | ||
| ament_target_dependencies(ogg_saver |
There was a problem hiding this comment.
this target requires rcutils headers. so we should add a dependency and include / link against it as well
| rosidl_target_interfaces(ogg_saver | ||
| ${PROJECT_NAME} "rosidl_typesupport_cpp") | ||
|
|
||
| ament_export_dependencies(rosidl_default_runtime) |
There was a problem hiding this comment.
Is this the only library needed by consuming packages?
| DESTINATION include | ||
| ) | ||
|
|
||
| pluginlib_export_plugin_description_file(image_transport theora_plugins.xml) |
There was a problem hiding this comment.
can you please update this plugin file to remove any os specific library prefix or suffix:
e.g. libtheora_image_transport -> theora_image_transport
There was a problem hiding this comment.
removed in e20b578, not sure if I need to change it in other places.
* Ignore other packages for now. * Update to new plugin API. * Add support for parameters. * Address reviewer feedback. * Switch to RCLCPP loggers. * Update compress_depth_image_transport to ROS2 (#27) * Port compress_depth_image_transport to ROS2 * Specifiers final for methods * Use rcutils logging * Implement parameters for encoding * Use RCLCPP_* macros instead of rcutils * Update theora_image_transport to ROS2 (#28) * Port theora plugin to ROS2 * Implement network options from ROS1 into rmw_qos_profile_t * Improve comment to clarify the use of dynamic reconfigure in ROS1 * Use RCLCPP logging instead of RCUTILS * Addressed reviewer comments
TODO: