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

Bugfix/fix 61 #67

Open
wants to merge 1 commit into
base: ros2
Choose a base branch
from

Conversation

nakai-omer
Copy link

Fixes #61, tools such as Nav2 expect reading that are out of range as infinity instead of nan.

@clalancette clalancette changed the base branch from melodic-devel to ros2 February 27, 2023 13:22
@@ -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());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@SteveMacenski SteveMacenski Feb 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@nakai-omer nakai-omer Feb 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SteveMacenski here is a printscreen from echoing gazebo's publish to /scan:
image

Copy link
Member

@SteveMacenski SteveMacenski Feb 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@nakai-omer nakai-omer Mar 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

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.

Out of range become nan instead of inf.
3 participants