zivid / zivid-ros

Official ROS driver for Zivid 3D cameras
BSD 3-Clause "New" or "Revised" License
55 stars 43 forks source link

ROS2: Colors `rgba` why use float32 #103

Closed fwarmuth closed 4 hours ago

fwarmuth commented 4 days ago

Hey,

Thanks for releasing a ROS2 Driver variant, finally getting rid of the ROS1 bridge and lowering the complexity.

I noticed that I cannot use the "normal" approach to convert ROS2 point clouds to PCL point clouds. Here is a callback function:

void pointCloudCallback(const sensor_msgs::msg::PointCloud2::SharedPtr msg) {
    // Convert ROS2 PointCloud2 message to PCL PointCloud
        pcl::PointCloud<pcl::PointXYZRGBA>::Ptr pcl_cloud(new pcl::PointCloud<pcl::PointXYZRGBA>);
        pcl::fromROSMsg(*msg, *pcl_cloud);
        ....

This will fail due to the wrong type of the rgba field being FLOAT32. As implemented here.

The workaround is to tell PCL about a new datatype:

struct EIGEN_ALIGN16 RGBAPoint
{
  PCL_ADD_POINT4D;
  union
    {
      struct
      {
        std::uint8_t r;
        std::uint8_t g;
        std::uint8_t b;
        std::uint8_t a;
      };
      float rgba; // in standard pcl format, rgba is defined as uint32 -> however, zivid uses float!
    };
  PCL_MAKE_ALIGNED_OPERATOR_NEW
};

} 
POINT_CLOUD_REGISTER_POINT_STRUCT(perception_datatypes::RGBAPoint,
                                  (float, x, x)(float, y, y)(float, z, z)(float, rgba, rgba))

And use it similar to this:

 // Convert ros pointcloud to PCL data type
        pcl::PointCloud<RGBAPoint>::Ptr cloud(new pcl::PointCloud<RGBAPoint>);
        pcl::fromROSMsg(*msg, *cloud);

Is this the intended way? Cheers

apartridge commented 3 days ago

Hi @fwarmuth. Thanks for testing the ROS2 preview.

I believe in ROS1 that rgba field had to be FLOAT32 for some technical reason. But this probably isn't the case anymore in ROS2. Could you check with the following change if it works as you expect? I tested it via rviz and it seems to work fine.

diff --git a/zivid_camera/src/zivid_camera.cpp b/zivid_camera/src/zivid_camera.cpp
index 300d125..0a4673c 100644
--- a/zivid_camera/src/zivid_camera.cpp
+++ b/zivid_camera/src/zivid_camera.cpp
@@ -737,7 +737,7 @@ void ZividCamera::publishPointCloudXYZRGBA(
   msg->fields.push_back(createPointField("x", 0, sensor_msgs::msg::PointField::FLOAT32, 1));
   msg->fields.push_back(createPointField("y", 4, sensor_msgs::msg::PointField::FLOAT32, 1));
   msg->fields.push_back(createPointField("z", 8, sensor_msgs::msg::PointField::FLOAT32, 1));
-  msg->fields.push_back(createPointField("rgba", 12, sensor_msgs::msg::PointField::FLOAT32, 1));
+  msg->fields.push_back(createPointField("rgba", 12, sensor_msgs::msg::PointField::UINT32, 1));
   msg->is_dense = false;

   // Note that the "rgba" field is actually byte order "bgra" on little-endian systems. For this
fwarmuth commented 1 day ago

Hey, thanks for the reply! I will test it asap, i guess this will be the solution.

cheers

fwarmuth commented 4 hours ago

Test! now, the convention to PCL point cloud works without any custom data types. I would say that would be nice to see in the next release, its more the "ros-way-of-life" to use uint32.

Regards!

apartridge commented 3 hours ago

Hi @fwarmuth , we will include this fix in the ROS2 driver