Add laser retro support in Ogre2 #194
Conversation
There was a problem hiding this comment.
nice, thanks for the contribution!
I'm curious where you find the 2000 limit in gazebo? I was looking through gazebo code but couldn't see where that limit is set.
We ran into precision issue with thermal camera readings as well. Need to investigate further to find out how to resolve this
ogre2/src/Ogre2GpuRays.cc
Outdated
| if (!subItem->hasCustomParameter(this->customParamIdx)) | ||
| { | ||
| // limit laser retro value to 2000 (as in gazebo) | ||
| if (retro_value > 2000.0){ |
There was a problem hiding this comment.
hmm interesting, where did you find this limit?
There was a problem hiding this comment.
See discussion below
You're right, there is no limit in gazebo classic, I somehow remembered that the max was 2000.0. I know for sure that I never encountered a laser_retro value (or ROS laserscan intensityhttp://docs.ros.org/en/api/sensor_msgs/html/msg/LaserScan.html) above 2000. But since the intensity is a lidar vendor specific unit maybe we should left it unbounded. What do you think ?
For my use case, since lidar intensity measurement are always noisy, this is fine. |
Thinking a bit more about making it unbounded, I think we have a non trivial issue about the range we can accept due to the need of normalizing the value to [0,1] on a float to pass it to the pipeline: Float precision is not constant within its range: https://blog.demofox.org/2017/11/21/floating-point-precision/ Also, what do we want as a minimal laser_retro value step ? 0.1 ? 0.01 ? |
|
thanks for looking into the precision issue. Sounds like we still need an upper bound.
I tried to play around with different upper bound vs small retro values and noticed that we can't really have small step sizes due to the the precision / noise issue which you also noticed in your test. So I would not worry about trying to support a small step size for now until we fixed this problem. |
Ok, I guess leaving it bounded to 2000.0 for now should be ok (and easily changeable to another value if needed). |
doisyg
force-pushed
the
add_laser_retro
branch
2 times, most recently
from
a79a30c
to
1866311
Compare
Signed-off-by: Guillaume Doisy <guillaume.doisy@wyca.fr>
doisyg
force-pushed
the
add_laser_retro
branch
from
1866311
to
60e823e
Compare
|
Fixed DCO mess by re-comiting from scratch |
|
Anything else blocking the merge ? |
As discussed here #181 and suggested by @iche033 . Taking huge inspiration and copypasting from Ogre2ThermalCamera (not sure I get the full shader logic). The implementation here is similar as the temperature tag from the Ogre2ThermalCamera.
laser retro values are valid on the range 0.0 to 2000.0 as it was, I believe, on Gazebo Classic. It can be an opportunity to discuss and change this behavior if needed.
There seem to be a loss of precision of around 5 retro value (see the test), don't know if it is due to some noise added in the pipeline or some shader witchery, but it works well for my use case (simulating lidar reflective tape).
With this PR, a pending PR in sdformat (gazebosim/sdformat#454) and one in ign-gazebo that I will soon post (but depends on sdformat), laser retro value will be settable from a sdf/urdf file.