Bugfix/fix 61 #67
Bugfix/fix 61 #67
Conversation
| @@ -153,7 +153,7 @@ sensor_msgs::msg::LaserScan::UniquePtr DepthImageToLaserScan::convert_msg( | |||
|
|
|||
| // Calculate and fill the ranges | |||
| uint32_t ranges_size = depth_msg->width; | |||
| scan_msg->ranges.assign(ranges_size, std::numeric_limits<float>::quiet_NaN()); | |||
| scan_msg->ranges.assign(ranges_size, std::numeric_limits<float>::infinity()); | |||
There was a problem hiding this comment.
This package has been publishing out-of-range values as NaN since 2012. As such, this would represent a pretty big behavior change for it. Can you give an example of what problems this is causing? Also, it would be nice to get some input from @SteveMacenski on this one.
There was a problem hiding this comment.
I don't see the strong need. Inf can mean max-range but doesn't need to be and is even a parameter. The way laser scans are handled in Nav2 are the same as they have been since ROS 1. Making it Inf would make something work better if you wanted to use the inf is valid callback in the obstacle layer. Its not a big difference, but is a difference.
I agree on questioning the motivation of why this is necessary. There may be a real use-case, but I can't think of one at the moment.
There was a problem hiding this comment.
@SteveMacenski @clalancette Thanks for taking time to look at this.
What led me to this PR is this answer, and us having a similar problem with costmap not clearing: https://answers.ros.org/question/190170/local-cost-map-not-updated/
But I understand now, that it might not be such as strong answer.
Actual lidars we have used all reported inf, so I thought this was some kind industry standard.
After this discussion I went to check some lidar repos and found the following:
Using inf:
SlamTec - https://github.com/Slamtec/sllidar_ros2
Lightware - https://github.com/LightWare-Optoelectronics/lightwarelidar2
Velodyne - https://github.com/ros-drivers/velodyne
RoboSense - https://github.com/RoboSense-LiDAR/rslidar_laserscan
Using nan:
Hokuyo URG - https://github.com/ros-drivers/urg_node
Gazebo lidar module also reports infs.
With all of the above said, I do understand this is not a strong enough case, but maybe there should be some kind of ROS defined standard for this.
There was a problem hiding this comment.
I'm OK with this change here if you can show the code for gazebo reporting INF on no-hits.
@clalancette to be fair, it might be a decade old, but its also only really used by hobbyists and this would be a bug that few ran into and/or cared enough about to fix.
There was a problem hiding this comment.
@SteveMacenski here is a printscreen from echoing gazebo's publish to /scan:
There was a problem hiding this comment.
I'd like to see it in code to verify, that way we can see if that change is recent and the context behind that decision. Sorry, maybe I should have made it clear why I needed to see the code itself.
There was a problem hiding this comment.
@SteveMacenski So sorry for late response, found it at:
https://github.com/gazebosim/gazebo-classic/blob/5e1b165e6d5f30e41034ca69170b0cd1b765de3d/gazebo/sensors/RaySensor.cc#L482
Which also references to REF 117, which states:
Detections that are too close to the sensor to quantify shall be represented by -Inf. Erroneous detections shall be represented by quiet (non-signaling) NaNs. Finally, out of range detections will be represented by +Inf.
BTW, we use this library for enabling your wonderful SLAM Toolbox to run underwater, where a regular lidar is worthless, but a calibrated depth camera can work well.
Fixes #61, tools such as Nav2 expect reading that are out of range as infinity instead of nan.