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

Fix bug in clipping of block laser depth values #902

Merged

Conversation

sloretz
Copy link
Contributor

@sloretz sloretz commented Mar 7, 2019

This fixes a bug where range values near the max range of a depth camera are incorrectly reported as max range. The range value was being clipped to (-infinity, maxRange-minRange], but it should be clipped to [minRange, maxRange]. Range readings past maxRange - minRange were reported as maxRange.

Before this pull request the range visualization in gazebo is correct, but the published point cloud in RViz shows all max range.

gazebo_ros_depth_max_range_issue

After this pull request the point cloud in RViz shows the correct depths.
gazebo_ros_depth_max_range_issue_with_fix

I'd like to add a test, but I haven't looked through the test code in this repo yet so I'm not yet sure where to add it.

@sloretz
Copy link
Contributor Author

sloretz commented Mar 7, 2019

Added block laser clipping test in ce8f33c

@sloretz
Copy link
Contributor Author

sloretz commented Mar 7, 2019

While writing the test I discovered that the problem is a little worse than I first reported. Without this PR all ranges from gazebo_ros_block_laser are wrong; they're all minRange too big.

Test world
gazebo_ros_block_laser_clipping

output without this PR
gazebo_ros_block_laser_clipping_pre_fix

output with this PR
gazebo_ros_block_laser_clipping_post_fix

Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Without this PR all ranges from gazebo_ros_block_laser are wrong; they're all minRange too big.

While a fix, this change seems to big for a released distro, since downstream users may already be compensating for this difference. How do you feel about retargeting this to noetic?

gazebo_plugins/src/gazebo_ros_block_laser.cpp Outdated Show resolved Hide resolved
Ranges were clipped to maxRange - minRange when they should have been
clipped to only maxRange

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz sloretz force-pushed the depth_camera_max_range_clipping branch from ce8f33c to ad7edd2 Compare April 16, 2020 22:31
@sloretz sloretz changed the base branch from melodic-devel to noetic-devel April 16, 2020 22:33
@sloretz
Copy link
Contributor Author

sloretz commented Apr 16, 2020

While a fix, this change seems to big for a released distro, since downstream users may already be compensating for this difference. How do you feel about retargeting this to noetic?

@chapulina rebased and retargeted at noetic-devel.

@chapulina chapulina merged commit 3201b45 into ros-simulation:noetic-devel May 15, 2020
cohen39 pushed a commit to cohen39/gazebo_ros_pkgs that referenced this pull request Nov 15, 2021
* Fix bug in block laser with large min range
* Add block laser clipping test
* REP-117 clipping

Ranges were clipped to maxRange - minRange when they should have been
clipped to only maxRange

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
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