Add thermal sensor system for configuring thermal camera properties #614

iche033 · 2021-02-05T09:11:56Z

Summary

depends on

related but not necessarily needed:

Add L16 pixel format to Camera pixel format conversion function sdformat#487 - I've already worked around this issue

Added a new thermal-camera system for setting thermal camera properties:

temperature linear resolution
min and max temperature range

This is required for simulating an 8 bit thermal camera sensor. The existing default params are suited for 16 bit image output, e.g. resolution defaults to 0.01 which is too high for an 8 bit camera, so we'll need a plugin for configuring these properties.

A better long term plan for this is to add these params to the sdf spec.

To Test

I've updated the thermal camera example world into include an 8 bit thermal camera:

ign gazebo -v 4 thermal_camera.sdf

Here is a screenshot of the ign gazebo with 16 bit and 8 bit image display side by side.

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

Signed-off-by: Ian Chen <[email protected]>

adlarkin · 2021-02-05T16:28:23Z

I'm seeing slightly different results when running the example (take a look at the cell phone in my image). @iche033 can you tell me what version of the cell phone fuel model you're using? I just tried updating to the most recent model, and it looks like I am using version 6.

adlarkin · 2021-02-05T16:32:07Z

Also, it looks like gazebosim/sdformat#487 has been approved in case we'd like to make use of that PR here.

ahcorde

  /github/workspace/src/rendering/RenderUtil.cc:308:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
  /github/workspace/src/systems/thermal/ThermalSensor.cc:93:  Lines should be <= 80 characters long  [whitespace/line_length] [2]

src/systems/thermal/CMakeLists.txt

src/systems/thermal/ThermalSensor.cc

adlarkin

Overall, looks good to me so far. I can give this another review once we add in the remaining pieces like tests.

examples/worlds/thermal_camera.sdf

src/EntityComponentManager.cc

src/rendering/RenderUtil.cc

src/systems/sensors/Sensors.cc

src/rendering/RenderUtil.cc

Signed-off-by: Ian Chen <[email protected]>

…gn-gazebo into thermal_sensor_system

Signed-off-by: Ian Chen <[email protected]>

iche033 · 2021-02-06T09:22:24Z

@iche033 can you tell me what version of the cell phone fuel model you're using?

I was using a modified local copy of the phone model. I just removed then redownloaded it. Now I get the same image as yours.

I can give this another review once we add in the remaining pieces like tests.

I've added an test in ee13143. As for documentation, let's update the tutorial separately and include all the new changes we made in the past few weeks.

codecov · 2021-02-06T09:55:50Z

Codecov Report

Merging #614 (d56d772) into ign-gazebo4 (766bb23) will decrease coverage by 0.07%.
The diff coverage is 100.00%.

@@               Coverage Diff               @@
##           ign-gazebo4     #614      +/-   ##
===============================================
- Coverage        77.40%   77.32%   -0.08%     
===============================================
  Files              218      216       -2     
  Lines            12254    12205      -49     
===============================================
- Hits              9485     9438      -47     
+ Misses            2769     2767       -2

Impacted Files	Coverage Δ
include/ignition/gazebo/components/Temperature.hh	`100.00% <100.00%> (ø)`
src/SystemLoader.cc	`72.13% <0.00%> (-4.92%)`	⬇️
src/systems/thermal/Thermal.hh
src/systems/thermal/Thermal.cc

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 766bb23...d56d772. Read the comment docs.

adlarkin · 2021-02-08T16:35:22Z

examples/worlds/thermal_camera.sdf

@@ -292,7 +292,7 @@
          <always_on>1</always_on>
          <update_rate>30</update_rate>
          <visualize>true</visualize>
-          <topic>thermal_camera_8bit</topic>
+          <topic>thermal_camera_8bit/image</topic>


For consistency, should we modify the other thermal camera's topic to be thermal_camera/image? Right now, the 8 bit camera has topics /thermal_camera_8bit/image and /thermal_camera_8bit/camera_info, but the original thermal camera has topics /thermal_camera and /camera_info, which might confuse users. I think it would be good to make the original thermal camera use the topics /thermal_camera/image and /thermal_camera/camera_info so that there's consistency between the two thermal cameras.

I didn't touch the existing topics in case there are users out there relying on these topics in this world.

How about we change it in Edifice?

That's fine 👍 I'll just make sure I explicitly point out which topics are associated with which camera in the tutorial

src/rendering/RenderUtil.cc

test/integration/thermal_system.cc

test/worlds/thermal.sdf

test/integration/thermal_system.cc

Signed-off-by: Ian Chen <[email protected]>

adlarkin · 2021-02-09T15:55:11Z

@iche033 I noticed that it looks like we are only configuring thermal visuals in RenderUtil.cc if things have not been initialized yet: https://github.com/ignitionrobotics/ign-gazebo/blob/555a84ea54a6f92b46929eb2f1a43daa13b1fadb/src/rendering/RenderUtil.cc#L822-L877

However, I believe that we also want to add any new thermal visuals that may appear after initialization as well: https://github.com/ignitionrobotics/ign-gazebo/blob/555a84ea54a6f92b46929eb2f1a43daa13b1fadb/src/rendering/RenderUtil.cc#L1033-L1068

If you agree with me on this, should we make this update in this PR, or in a separate PR?

Signed-off-by: Ian Chen <[email protected]>

iche033 · 2021-02-09T18:48:33Z

@iche033 I noticed that it looks like we are only configuring thermal visuals in RenderUtil.cc if things have not been initialized yet:

good catch. I've updated it in this PR. f377905

adlarkin · 2021-02-09T22:57:30Z

test/worlds/thermal_invalid.sdf

+          <plugin
+            filename="ignition-gazebo-thermal-system"
+            name="ignition::gazebo::systems::Thermal">
+            <temperature>600.0</temperature>


Can we change the temperature for the sphere to be below the camera's default minimum temperature value so that we can make sure that values below the minimum are clamped to the minimum (just like how we are checking that values above the max are clamped to the max with the cylinder)?

I'm working on adding this test case, but haven't been able to get the results I want yet. I will keep working on it.

I've created some draft PRs to show the current status of what I am trying:

Add thermal camera test for object temperature below 0 #621

Handle non-positive object temperatures gz-rendering#243

I think the issue is that ign-rendering does not accept temperature below zero thus giving it ambient temperature.

How about we clamp negative values to 0 in the thermal system (or in ign-rendering) and print out a warning? Theoretically nothing can be colder than absolute zero (0 K) so it's reasonable to do a quick sanity check.

I tried clamping negative to 0 in gazebosim/gz-rendering#243, but that didn't seem to work. Perhaps I am doing it too late in the pipeline. I will try this in the gazebo plugin to see if I get better results

The PRs above seem to successfully add in this test case now. We will merge the PRs above later since we need to merge this ASAP for an ign-gazebo release (merging these PRs would delay the release since it would require a patch release in ign-rendering).

Signed-off-by: Ian Chen <[email protected]>

ahcorde · 2021-02-10T15:14:42Z

src/systems/thermal/ThermalSensor.cc

+    ignerr << "The thermal sensor system can only be attached to a "
+           << "thermal sensor" << std::endl;


is this trace right ?

Yeah. Perhaps the wording is a little confusing, but the thermal sensor system allows users to customize the configuration/parameters of the thermal sensor/camera it's attached to. Would you recommend using different wording to help minimize confusion?

test/integration/thermal_system.cc

test/integration/thermal_sensor_system.cc

Signed-off-by: Ashton Larkin <[email protected]>

ahcorde · 2021-02-10T17:01:56Z

Some tests are failing

https://build.osrfoundation.org/job/ignition_gazebo-ci-pr_any-homebrew-amd64/4946/
https://build.osrfoundation.org/job/ignition_gazebo-ci-pr_any-ubuntu_auto-amd64/5464/

adlarkin · 2021-02-10T17:09:55Z

Some tests are failing

I believe this is because we need some releases for ign-rendering and ign-sensors. I will check back on this once those releases are out.

Signed-off-by: Ian Chen <[email protected]>

nkoenig · 2021-02-10T20:33:48Z

Some tests are failing

I believe this is because we need some releases for ign-rendering and ign-sensors. I will check back on this once those releases are out.

The releases are now out.

adlarkin · 2021-02-10T20:36:03Z

All checks are running now. I'm just waiting for the results from CI and GitHub actions.

Signed-off-by: Ian Chen <[email protected]>

Comments have been addressed, and we need to merge this for an ign-gazebo release.

iche033 added 2 commits February 5, 2021 00:41

add thermal sensor system

cb6e37a

Signed-off-by: Ian Chen <[email protected]>

fix segfaul remove comment

b3dbd32

Signed-off-by: Ian Chen <[email protected]>

iche033 requested a review from adlarkin February 5, 2021 09:11

iche033 requested a review from chapulina as a code owner February 5, 2021 09:11

github-actions bot added the 🔮 dome Ignition Dome label Feb 5, 2021

iche033 mentioned this pull request Feb 5, 2021

Visualize single channel 8 bit image data gazebosim/gz-gui#176

Merged

6 tasks

iche033 added enhancement New feature or request needs upstream release Blocked by a release of an upstream library labels Feb 5, 2021

Merge branch 'ign-gazebo4' into thermal_sensor_system

6b10f2a

iche033 changed the title ~~Add thermal sensor system for configuring thermal camera sensors~~ Add thermal sensor system for configuring thermal camera properties Feb 5, 2021

ahcorde requested changes Feb 5, 2021

View reviewed changes

src/systems/thermal/CMakeLists.txt Outdated Show resolved Hide resolved

src/systems/thermal/ThermalSensor.cc Show resolved Hide resolved

adlarkin reviewed Feb 5, 2021

View reviewed changes

iche033 added 3 commits February 6, 2021 00:58

feedback and add test

ee13143

Signed-off-by: Ian Chen <[email protected]>

Merge branch 'thermal_sensor_system' of github.com:ignitionrobotics/i…

62eccaa

…gn-gazebo into thermal_sensor_system

update example world image display topic

03cfb99

Signed-off-by: Ian Chen <[email protected]>

iche033 requested a review from ahcorde February 6, 2021 09:25

adlarkin reviewed Feb 8, 2021

View reviewed changes

This was referenced Feb 8, 2021

Add Light Usercommand and include Light parameters in the componentInspector #482

Merged

Fix seg fault in SetRemovedComponentsMsgs when TransformControl tool is used #495

Merged

iche033 added 4 commits February 8, 2021 17:35

add test for checking invalid properties

1e538c9

Signed-off-by: Ian Chen <[email protected]>

Merge branch 'ign-gazebo4' into thermal_sensor_system

1d5ebc7

white space

bd203a2

Signed-off-by: Ian Chen <[email protected]>

typo

ea8b55d

Signed-off-by: Ian Chen <[email protected]>

read thermal data from spawned visuals

f377905

Signed-off-by: Ian Chen <[email protected]>

adlarkin mentioned this pull request Feb 9, 2021

Particle system - Part4 #615

Merged

adlarkin reviewed Feb 9, 2021

View reviewed changes

iche033 added 2 commits February 9, 2021 22:04

split test

3935064

Signed-off-by: Ian Chen <[email protected]>

add test src file

2413f4d

Signed-off-by: Ian Chen <[email protected]>

iche033 mentioned this pull request Feb 10, 2021

Sensors system not unloaded properly between tests #620

Open

ahcorde previously requested changes Feb 10, 2021

View reviewed changes

adlarkin mentioned this pull request Feb 10, 2021

4 4 0 prep gazebosim/gz-rendering#242

Merged

4 tasks

use common::joinPaths() in tests

ba08f99

Signed-off-by: Ashton Larkin <[email protected]>

adlarkin requested a review from ahcorde February 10, 2021 16:41

adlarkin mentioned this pull request Feb 10, 2021

Add support for 8 bit thermal camera image format gazebosim/gz-sensors#92

Merged

6 tasks

adlarkin mentioned this pull request Feb 10, 2021

Add thermal camera test for object temperature below 0 #621

Merged

8 tasks

better msg

c830f45

Signed-off-by: Ian Chen <[email protected]>

nkoenig removed the needs upstream release Blocked by a release of an upstream library label Feb 10, 2021

fixing CI

d56d772

Signed-off-by: Ian Chen <[email protected]>

adlarkin approved these changes Feb 10, 2021

View reviewed changes

adlarkin merged commit e647570 into ign-gazebo4 Feb 10, 2021

adlarkin deleted the thermal_sensor_system branch February 10, 2021 22:59

This was referenced Feb 11, 2021

4.4.0 #625

Merged

4 ➡️ 5 #622

Merged

adlarkin mentioned this pull request Feb 18, 2021

Updated thermal camera tutorial: include varying temperature objects gazebosim/gz-sensors#79

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add thermal sensor system for configuring thermal camera properties #614

Add thermal sensor system for configuring thermal camera properties #614

iche033 commented Feb 5, 2021 •

edited by adlarkin

Loading

adlarkin commented Feb 5, 2021

adlarkin commented Feb 5, 2021

ahcorde left a comment

adlarkin left a comment

iche033 commented Feb 6, 2021 •

edited

Loading

codecov bot commented Feb 6, 2021 •

edited

Loading

adlarkin Feb 8, 2021

iche033 Feb 8, 2021

adlarkin Feb 9, 2021

adlarkin commented Feb 9, 2021

iche033 commented Feb 9, 2021

adlarkin Feb 9, 2021

adlarkin Feb 10, 2021

adlarkin Feb 10, 2021 •

edited

Loading

iche033 Feb 10, 2021

adlarkin Feb 10, 2021

adlarkin Feb 10, 2021

ahcorde Feb 10, 2021

adlarkin Feb 10, 2021

iche033 Feb 10, 2021

ahcorde commented Feb 10, 2021

adlarkin commented Feb 10, 2021

nkoenig commented Feb 10, 2021

adlarkin commented Feb 10, 2021

		ignerr << "The thermal sensor system can only be attached to a "
		<< "thermal sensor" << std::endl;

Add thermal sensor system for configuring thermal camera properties #614

Add thermal sensor system for configuring thermal camera properties #614

Conversation

iche033 commented Feb 5, 2021 • edited by adlarkin Loading

Summary

To Test

Checklist

adlarkin commented Feb 5, 2021

adlarkin commented Feb 5, 2021

ahcorde left a comment

Choose a reason for hiding this comment

adlarkin left a comment

Choose a reason for hiding this comment

iche033 commented Feb 6, 2021 • edited Loading

codecov bot commented Feb 6, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adlarkin commented Feb 9, 2021

iche033 commented Feb 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adlarkin Feb 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahcorde commented Feb 10, 2021

adlarkin commented Feb 10, 2021

nkoenig commented Feb 10, 2021

adlarkin commented Feb 10, 2021

iche033 commented Feb 5, 2021 •

edited by adlarkin

Loading

iche033 commented Feb 6, 2021 •

edited

Loading

codecov bot commented Feb 6, 2021 •

edited

Loading

adlarkin Feb 10, 2021 •

edited

Loading