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.

Sign up for GitHub

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
Open

Bugfix/fix 61 #67

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/DepthImageToLaserScan.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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.


if (depth_msg->encoding == sensor_msgs::image_encodings::TYPE_16UC1) {
convert<uint16_t>(depth_msg, cam_model_, scan_msg, scan_height_);
Expand Down