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

Added reflectance output image for DepthCamera #2966

Open
wants to merge 18 commits into
base: gazebo11
Choose a base branch
from

Conversation

WilliamLewww
Copy link
Contributor

When specified in the SDF file, the depth camera will output a reflectance image that can be viewed in the topic visualizer.

reflectance

@chapulina chapulina requested a review from ahcorde April 14, 2021 17:37
Copy link
Contributor

@ahcorde ahcorde left a 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 ?

gazebo/sensors/DepthCameraSensor.cc Outdated Show resolved Hide resolved
WilliamLewww and others added 2 commits April 15, 2021 11:18
Co-authored-by: Alejandro Hernández Cordero <[email protected]>
@WilliamLewww
Copy link
Contributor Author

I added a test for both enabled and disabled reflectance output.

gazebo/sensors/DepthCameraSensor_TEST.cc Outdated Show resolved Hide resolved
@WilliamLewww
Copy link
Contributor Author

WilliamLewww commented Apr 16, 2021

@ahcorde
Copy link
Contributor

ahcorde commented Apr 16, 2021

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

Copy link
Contributor

@ahcorde ahcorde left a 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 ?

@scpeters
Copy link
Member

Steve do you mind to take another quick look ?

I assume you meant me. Sure I'll take another look

if (this->dataPtr->depthCamera->GetOutputReflectance())
{
ignition::transport::AdvertiseMessageOptions opts;
opts.SetMsgsPerSec(50);
Copy link
Member

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?

Copy link
Contributor Author

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:

https://github.com/osrf/gazebo/blob/26b69a7cff29312164558d23344eb551a96508ba/gazebo/sensors/CameraSensor.cc#L110-L112

https://github.com/osrf/gazebo/blob/26b69a7cff29312164558d23344eb551a96508ba/gazebo/sensors/WideAngleCameraSensor.cc#L146-L148

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.

Copy link
Member

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

gazebo/sensors/DepthCameraSensor.cc Outdated Show resolved Hide resolved
@WilliamLewww
Copy link
Contributor Author

Inverted the reflectance buffer to correct the inverted values for depth images.

a

Signed-off-by: William Lew <[email protected]>
@WilliamLewww WilliamLewww force-pushed the wlew/depth_camera_reflectance branch from a47c8ef to c776173 Compare April 26, 2021 21:49
@scpeters
Copy link
Member

I've merged with gazebo11 to fix the abichecker build

public: bool GetOutputReflectance() const;

/// \brief All things needed to get reflectance data
/// \return The reflectance buffer as a float array
Copy link
Member

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

Copy link
Contributor

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.

@WilliamLewww WilliamLewww marked this pull request as draft May 6, 2021 03:04
@WilliamLewww
Copy link
Contributor Author

To prevent the buffer from inverting, I set the reflectance message format to UNKNOWN_PIXEL_FORMAT so that the image frame would be able to detect that the message isn't a depth image. The pixel format R_FLOAT32 is hardcoded to be recognized as a depth buffer, and to maintain the pixel format while being able to distinguish depth vs. reflectance would require pixel analysis.

A cleaner solution is to add a new data member to the Image message specifying what type of image the message contains. The change could remove the hardcoded pixel format to image type mapping but will make the Image message larger. I am not too sure how disruptive this change may be so I opted for the first solution.

@WilliamLewww WilliamLewww marked this pull request as ready for review June 28, 2021 22:41
@chapulina
Copy link
Contributor

What's the status of this PR?

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.

5 participants