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

Add support for 8 bit thermal camera image format #92

Merged
merged 8 commits into from
Feb 10, 2021

Conversation

iche033
Copy link
Contributor

@iche033 iche033 commented Feb 5, 2021

Summary

Depends on gazebosim/gz-rendering#235

Adds the necessary hooks to set ign-rendering thermal camera sensor to 8 bit image format and publish the generated image data.

Note that ign-rendering still returns uint16_t in the image callback but it actually contains 8 bit data. This was done to avoid breaking ABI. However, the image that's published by ign-sensors will have the correct format in a 8 bit unsigned char array

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
    Note to maintainers: Remember to use Squash-Merge

@iche033 iche033 requested a review from adlarkin February 5, 2021 09:04
@iche033 iche033 added enhancement New feature or request needs upstream release Blocked by a release of an upstream library 🔮 dome Ignition Dome labels Feb 5, 2021
@codecov
Copy link

codecov bot commented Feb 5, 2021

Codecov Report

Merging #92 (e87e030) into ign-sensors4 (46b320f) will increase coverage by 0.12%.
The diff coverage is 87.87%.

Impacted file tree graph

@@               Coverage Diff                @@
##           ign-sensors4      #92      +/-   ##
================================================
+ Coverage         76.00%   76.13%   +0.12%     
================================================
  Files                23       23              
  Lines              2359     2388      +29     
================================================
+ Hits               1793     1818      +25     
- Misses              566      570       +4     
Impacted Files Coverage Δ
src/ThermalCameraSensor.cc 60.00% <87.50%> (+3.29%) ⬆️
src/Sensor.cc 87.90% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 46b320f...c6b39cc. Read the comment docs.

src/ThermalCameraSensor.cc Show resolved Hide resolved
test/integration/thermal_camera_plugin.cc Outdated Show resolved Hide resolved
src/ThermalCameraSensor.cc Outdated Show resolved Hide resolved
test/integration/thermal_camera_plugin.cc Outdated Show resolved Hide resolved
test/integration/thermal_camera_plugin.cc Show resolved Hide resolved
@iche033 iche033 requested a review from ahcorde February 8, 2021 22:55
@adlarkin
Copy link
Contributor

adlarkin commented Feb 9, 2021

There are a few test failures on macOS, but I don't think they're related to this PR. If that's true, then everything else looks good to me, assuming that defaulting to 16 bit is okay.

@iche033
Copy link
Contributor Author

iche033 commented Feb 9, 2021

assuming that defaulting to 16 bit is okay.

yes that also preserves existing behavior

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.

Something is broken on the macOS github actions. It's related with OGRE2

[Err] [Ogre2RenderEngine.cc:436] Unable to find Ogre Plugin[/usr/local/opt/ogre2.1/lib/OGRE-2.1/RenderSystem_GL3Plus]. Rendering will not be possible.Make sure you have installed OGRE properly.
	 13 - INTEGRATION_camera_plugin (SEGFAULT)
	 15 - INTEGRATION_depth_camera_plugin (SEGFAULT)
	 17 - INTEGRATION_gpu_lidar_sensor_plugin (SEGFAULT)
	 19 - INTEGRATION_rgbd_camera_plugin (SEGFAULT)

Otherwise LGTM

@adlarkin
Copy link
Contributor

adlarkin commented Feb 10, 2021

When running the integration test for an invalid configuration in gazebosim/gz-sim#614, I notice the following output:

[Wrn] [ThermalCameraSensor.cc:275] 8 bit thermal camera image format selected. The temperature linear resolution needs to be higher than 1.0. Defaulting to 3.0, range = [0, 255*3] K
[Wrn] [RenderUtil.cc:810] Unable to set thermal camera temperature linear resolution. Value must be greater than 0. Using the default value: 3. 
[Wrn] [RenderUtil.cc:823] Unable to set thermal camera temperature range.Max temperature must be greater or equal to min. Using the default values : [-inf, inf].

It's confusing to me to see two different ranges. I am assuming that [0, 255*3] K is the correct range. Should we modify this warning output? If so, I think the modifications should be made in ign-gazebo (the correct output is coming from this PR).

cc @iche033

@nkoenig
Copy link
Contributor

nkoenig commented Feb 10, 2021

When running the integration test for an invalid configuration in ignitionrobotics/ign-gazebo#614, I notice the following output:

[Wrn] [ThermalCameraSensor.cc:275] 8 bit thermal camera image format selected. The temperature linear resolution needs to be higher than 1.0. Defaulting to 3.0, range = [0, 255*3] K
[Wrn] [RenderUtil.cc:810] Unable to set thermal camera temperature linear resolution. Value must be greater than 0. Using the default value: 3. 
[Wrn] [RenderUtil.cc:823] Unable to set thermal camera temperature range.Max temperature must be greater or equal to min. Using the default values : [-inf, inf].

It's confusing to me to see two different ranges. I am assuming that [0, 255*3] K is the correct range. Should we modify this warning output? If so, I think the modifications should be made in ign-gazebo (the correct output is coming from this PR).

cc @iche033

the thermal_system integration test on gazebosim/gz-sim#614 works for me and I don't see the warning messages you posted. Is there something else I should test?

@adlarkin
Copy link
Contributor

the thermal_system integration test on ignitionrobotics/ign-gazebo#614 works for me and I don't see the warning messages you posted. Is there something else I should test?

There are now two thermal integration tests: INTEGRATION_thermal_system (this checks object temperatures) and INTEGRATION_thermal_sensor_system (this checks thermal camera configurations). INTEGRATION_thermal_sensor_system is the new test added in gazebosim/gz-sim#614, and is the one that would produce these warnings (in particular, the ThermalSensorSystemInvalidConfig test). Did you try running the new INTEGRATION_thermal_sensor_system test?

@nkoenig
Copy link
Contributor

nkoenig commented Feb 10, 2021

The warning messages look correct to me. There are multiple problems with the thermal_invalid.sdf file. The first two warning message refer to the incorrect parameter, and the third warning message refers to the incorrect <max_temp> value.

@adlarkin
Copy link
Contributor

The thing that seems confusing to me is that the first warning indicates that the camera's temperature range is between [0, 255*3] K, but the third warning makes it seem like the camera's temperature range will be between [-inf, inf], which contradicts the information given in the first warning. Does that make sense, or am I misunderstanding something?

@nkoenig
Copy link
Contributor

nkoenig commented Feb 10, 2021

The thing that seems confusing to me is that the first warning indicates that the camera's temperature range is between [0, 255*3] K, but the third warning makes it seem like the camera's temperature range will be between [-inf, inf], which contradicts the information given in the first warning. Does that make sense, or am I misunderstanding something?

Maybe the warning messages could be reworded. The third warning says the that min and max ranges of the camera are between [-inf and inf]. These are ranges that the camera can detect.

The first warning message is about the resolution of the output. My understanding is that there is a difference between what the camera can detect and the data that is produced.

@adlarkin
Copy link
Contributor

My understanding is that there is a difference between what the camera can detect and the data that is produced.

That makes sense. Okay, what if I change the first warning to this (notice the addition of the word output):

[Wrn] [ThermalCameraSensor.cc:275] 8 bit thermal camera image format selected. The temperature linear resolution needs to be higher than 1.0. Defaulting to 3.0, output range = [0, 255*3] K

I think the other warnings are fine as-is.

@adlarkin
Copy link
Contributor

Okay, what if I change the first warning to this (notice the addition of the word output)

Made the change in c6b39cc, I think we can merge this.

@nkoenig nkoenig merged commit a9517f8 into ign-sensors4 Feb 10, 2021
@nkoenig nkoenig deleted the thermal_8bit branch February 10, 2021 18:08
@iche033
Copy link
Contributor Author

iche033 commented Feb 10, 2021

a little late but this sums it up:

My understanding is that there is a difference between what the camera can detect and the data that is produced.

  • output range = [0, 255*3] K: this the range of possible values that can be generated by an 8 bit camera with a resolution of 3.
  • [-inf, inf]: are the user specified min and max temperature values.

e.g. user can limit the max temp produced by the camera by specifying something like [0, 200]. If the user specifies something like [0, 800], the max reading will be the min(800, 255*3), which is 765.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔮 dome Ignition Dome enhancement New feature or request needs upstream release Blocked by a release of an upstream library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants