-
Notifications
You must be signed in to change notification settings - Fork 481
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
Added reflectance output image for DepthCamera #2966
base: gazebo11
Are you sure you want to change the base?
Added reflectance output image for DepthCamera #2966
Conversation
Signed-off-by: William Lew <[email protected]>
Signed-off-by: William Lew <[email protected]>
Signed-off-by: William Lew <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add a test to check the ign topic ?
Co-authored-by: Alejandro Hernández Cordero <[email protected]>
Signed-off-by: William Lew <[email protected]>
Signed-off-by: William Lew <[email protected]>
Encapsulated data into test class
Should I create a different pull request to encapsulate all the date for the other tests that are already there? |
You can do it in this PR |
Signed-off-by: William Lew <[email protected]>
Signed-off-by: William Lew <[email protected]>
Encapsulated variables and functions for rest of tests
Signed-off-by: William Lew <[email protected]>
Fixed formatting with pointers and references
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Steve do you mind to take another quick look ?
I assume you meant me. Sure I'll take another look |
gazebo/sensors/DepthCameraSensor.cc
Outdated
if (this->dataPtr->depthCamera->GetOutputReflectance()) | ||
{ | ||
ignition::transport::AdvertiseMessageOptions opts; | ||
opts.SetMsgsPerSec(50); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we use the Sensor::UpdateRate()
instead of hard-coding to 50 Hz?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that the other cameras are using a hard-coded 50Hz:
When I tried changing the 50
to this->UpdateRate()
, I get a broken topic because I think the update rate has not been set and still is the value 0.000.
I could change this and initialize the update rate before advertising the topic but I am unsure if this will cause problems to the other components on the depth camera.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gazebo::transport::Node::Advertise
has two optional numeric parameters, first the queue size and then the rate of publishing in Hz. In these examples from CameraSensor and WideAngleCameraSensor, 50
is actually the queue size. In the ignition::transport code (that has now been removed) it was a bit more clear that SetMsgsPerSec
was about the rate of publishing
I think no additional changes are needed to the queue size
Signed-off-by: William Lew <[email protected]>
Signed-off-by: William Lew <[email protected]>
a47c8ef
to
c776173
Compare
I've merged with |
public: bool GetOutputReflectance() const; | ||
|
||
/// \brief All things needed to get reflectance data | ||
/// \return The reflectance buffer as a float array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you clarify what the values in this array mean physically? does 0 mean nonreflective and 1 mean reflective?
I'm asking because you inverted the data in c776173
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh we should not invert the data on the sensors side. We should only update the visualization code. The gui visualization inverts the data when it detects that the underlying image has a FLOAT type:
https://github.com/osrf/gazebo/blob/gazebo11/gazebo/gui/viewers/ImageFrame.cc#L157-L167
The logic there was added for visualization of depth data, i.e. black pixels means far away while white means close to camera. However, now that the reflectance image output is also in FLOAT, we don't want the inversion to be applied to the reflectance image. I think there needs to be some way on the gui side to tell reflectance images from depth images so that the correct visualization is applied.
Signed-off-by: William Lew <[email protected]>
Signed-off-by: William Lew <[email protected]>
To prevent the buffer from inverting, I set the reflectance message format to A cleaner solution is to add a new data member to the |
What's the status of this PR? |
When specified in the SDF file, the depth camera will output a reflectance image that can be viewed in the topic visualizer.