Port to ROS 2 ros_gz_point_cloud - #912
Conversation
Signed-off-by: Alejandro Hernandez Cordero <ahcorde@gmail.com>
| auto ns = _sdf->Get<std::string>("namespace", scoped_name).first; | ||
| std::string node_name = scoped_name; | ||
| std::ranges::replace(node_name, '/', '_'); | ||
| this->dataPtr->rosnode_ = std::make_shared<rclcpp::Node>(node_name, ns); |
There was a problem hiding this comment.
Should we add a try/catch here? I think this could throw if we use names such as 3d_lidar_link_sensor or my-robot_link_sensor not valid in ROS.
| msg.header.stamp.nanosec = sec_nsec.second; | ||
| msg.width = _width; | ||
| msg.height = _height; | ||
| msg.is_dense = true; |
There was a problem hiding this comment.
This is hardcoded to true. Should we use the same logic you applied in OnNewDepthFrame, where it can be false as well?
| @@ -234,33 +255,43 @@ | |||
| { | |||
| this->dataPtr->current_time_ = _info.simTime; | |||
There was a problem hiding this comment.
Should we protect this and the readers in separate threads with a mutex?
| } | ||
|
|
||
| this->dataPtr->scene_ = engine->SceneByName(this->dataPtr->scene_name_); | ||
| this->dataPtr->scene_ = engine->SceneByIndex(0); |
There was a problem hiding this comment.
This is hardcoded to scene at index 0. I think we should update the documentation about this parameter in point_cloud.hpp or parse it correctly.
| ros::init(argc, argv, "ignition", ros::init_options::NoSigintHandler); | ||
| ROS_INFO_NAMED("ros_gz_point_cloud", "Initialized ROS"); | ||
| if (!rclcpp::ok()) { | ||
| rclcpp::init(0, nullptr); |
There was a problem hiding this comment.
Should we use NodeOptions().start_parameter_services(false) or a shared context/executor? There's no rclcpp::shutdown, executor, or spin in the package.
| if (!this->dataPtr->scene_) { | ||
| auto loadedEngNames = gz::rendering::loadedEngines(); | ||
| if (loadedEngNames.empty()) { | ||
| RCLCPP_INFO(rclcpp::get_logger("ros_gz_point_cloud"), "No rendering engines loaded yet"); |
There was a problem hiding this comment.
RCLCPP_INFO_ONCE or throttle this message?
| } | ||
|
|
||
| ////////////////////////////////////////////////// | ||
| void PointCloudPrivate::LoadRgbCamera( |
There was a problem hiding this comment.
This function is almost similar to LoadDepthCamera(). Should we use a helper function with the right parameters?
| if (this->type_ == SensorType::RGBD_CAMERA && _channels != 1) { | ||
| ROS_WARN_NAMED( | ||
| "ros_gz_point_cloud", | ||
| if (this->type_ == RGBD_CAMERA && _channels != 1) { |
There was a problem hiding this comment.
I think this block can never be reached as the callback seems connected with DEPTH_CAMERA and GPU_LIDAR only.
| } | ||
| if ((this->type_ == SensorType::RGBD_CAMERA || | ||
| this->type_ == SensorType::DEPTH_CAMERA) && _format != "FLOAT32") | ||
| if ((this->type_ == RGBD_CAMERA || |
There was a problem hiding this comment.
Same here, I think we can remove the RGBD_CAMERA part.
| msg.header.stamp.nanosec = sec_nsec.second; | ||
| msg.width = _width; | ||
| msg.height = _height; | ||
| msg.row_step = msg.point_step * _width; |
| this->rgb_image_ = this->rgb_camera_->CreateImage(); | ||
| const auto sec_nsec = gz::math::durationToSecNsec(this->current_time_); | ||
|
|
||
| sensor_msgs::msg::PointCloud2 msg; |
There was a problem hiding this comment.
This setup block is almost the same as the one in OnNewDepthFrame. Could we move it to a shared helper so fixes only need to happen in one place?
Also, depth and lidar clouds have no color, but they still get an rgb field that is zeroed per point. Using only "xyz" fields for them would save 4 bytes per point and skip that work.
|
|
||
| <exec_depend>message_runtime</exec_depend> | ||
| <depend>gz_plugin_vendor</depend> | ||
| <depend>gz_rendering_vendor</depend> |
There was a problem hiding this comment.
The code uses gz-math and gz-common directly, but they are only pulled in through the other vendor packages. Could we declare gz_math_vendor and gz_common_vendor here?
| { | ||
| // Forward declarations. | ||
| class PointCloudPrivate; | ||
|
|
There was a problem hiding this comment.
This comment mentions RGBD and GPU lidar, but the plugin also supports depth cameras. Same for the line below.
|
|
||
| // Get sensor | ||
| auto sensor = this->scene_->SensorByName(sensor_name); | ||
| auto sensor = this->scene_->SensorByName(sensor_name + "_depth"); |
There was a problem hiding this comment.
The doxygen brief for this function (in the class declaration) still says "Get RGB camera from rendering", but it actually looks up the depth camera of the RGBD sensor.
Signed-off-by: Alejandro Hernandez Cordero <ahcorde@gmail.com>
3 participants
🎉 New feature
Related with #911
Summary
Update ros_gz_point_cloud to use ROS 2 and newer Gazebo versions
Backport Policy
Checklist
codecheckpassed (See contributing)Generated-by: Claude Opus 4.7
Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining
Signed-off-byandGenerated-bymessages.Backports: If this is a backport, please use Rebase and Merge instead.