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

Using NaN instead of range_max+1 to remove scans in angular_bounds_filter_in_place #202

Merged
merged 4 commits into from
Aug 26, 2024

Conversation

bely2803
Copy link
Contributor

Problem statement

The current implementation of angular_bounds_filter_in_place uses range_max + 1.0 to filter laser scans, which can lead to unintended behaviours when these scans are passed to packages like AMCL and slam_toolbox.

Proposed Change

This PR modifies the code to use NaN instead of range_max + 1 for filtering out-of-range scans. Using NaN is a more robust approach because it eliminates the need for other packages to verify whether each scan falls within the range specified in the message header.

This change aligns with the existing methodology used in box_filter, enhancing consistency across the codebase. We've ported this approach to angular_bounds_filter_in_place to ensure more reliable and predictable performance.

@jonbinney
Copy link
Contributor

This is a good change! We'll have to be careful how we apply it though. There may be people with robots in the field who depend on the current behavior. Normally we only make user-facing changes along with new ROS distros. I propose the following:

  • add a param to this filter called replace_with_nan and have it default to false
  • when the param is set to true, use NaN, else use range_max + 1. this will preserve the old behavior as default for the moment, but will allow you to start using the (more correct) value
  • next time there is a new ROS distro, we'll swap the default value to false. This will still probably break some peoples' robots, but they'll be looking for bugs since it is a new distro.

Additionally, put a comment in the code where the parameter is declared describing this and linking to this PR.

@bely2803 bely2803 reopened this Aug 26, 2024
Copy link
Contributor

@jonbinney jonbinney left a comment

Choose a reason for hiding this comment

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

Looks good!

@jonbinney jonbinney merged commit 15a6e7d into ros-perception:ros2 Aug 26, 2024
9 checks passed
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.

2 participants