Optimize sensor updates

[iche033]

🦟 Bug fix

Depends on gazebosim/gz-sensors#222

Summary

This PR optimizes sensor updates by avoiding any unnecessary computation when:

• it's not time for the sensor to update
• there are not subscribers or event connections to the sensors

For non-rendering sensors, before these changes, the sensors themselves are throttled correctly but we are still doing updates (copying data from sim to sensor object) every iteration in PostUpdate function.

For rendering sensors, i.e. sensors system, I noticed that the RGBDCameraSensor and GpuLidarSensor are still doing updates when there are no subscribers so I added the check in the sensors system.

After these changes, the sensors_demo.sdf world gives me consistent ~9x% RTF after closing all ImageDisplays. Before the changes, the RTF still fluctuates between 3x% to 8x% after closing the ImageDisplays (because GpuLidarSensor and RGBDCameraSensor are still updating)

I verified the behavior by enabling remotery in ign-sensors. Below are outputs from running sensors_demo.sdf world.

The output below shows the sensor updates after the sensors_demo.sdf world is launched and running. We see that GpuLidarSensor is no longer updating because there are no subscribers. On the other hand, all the other rendering sensors are updating because there are ImageDisplays connecting to the sensors.

The output below shows the result after closing the RGBD cameras and thermal camera's ImageDisplays. Now only the depth and regular camera are updating, which is the desired behavior.

Alternatives considered

The alternative approach, which I implemented but then reverted, was to check for connection count inside the sensor classes in ign-sensors. The main reason I didn't go with this approach is because classes like Lidar (which GpuLidarSensor is derived from) also offers Ranges APIs to get range data directly. If I skipped updates when there are no connections, Ranges returned invalid / empty data, and that broke integration tests.

Checklist

• Signed all commits for DCO
• Added tests
• Updated documentation (as needed)
• Updated migration guide (as needed)
• Consider updating Python bindings (if the library has them)
• codecheck passed (See contributing)
• All tests passed (See test coverage)
• While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

[mjcarroll]

Looks like we are still getting some issues even with upstream release (6.4.0)
Edit: Nevermind, the required PR wasn't included in the 6.4.0 release, we need another release of ign-sensors.

  [ 51%] Building CXX object src/systems/air_pressure/CMakeFiles/ignition-gazebo6-air-pressure-system.dir/AirPressure.cc.o
  /github/workspace/src/systems/air_pressure/AirPressure.cc: In member function 'virtual void ignition::gazebo::v6::systems::AirPressure::PostUpdate(const ignition::gazebo::v6::UpdateInfo&, const ignition::gazebo::v6::EntityComponentManager&)':
  /github/workspace/src/systems/air_pressure/AirPressure.cc:152:22: error: 'class ignition::sensors::v6::AirPressureSensor' has no member named 'HasConnections'
             it.second->HasConnections())
                        ^~~~~~~~~~~~~~

[ahcorde]

@osrf-jenkins run tests please!

[codecov]

Codecov Report

Merging #1480 (f768d45) into ign-gazebo6 (62272db) will not change coverage.
The diff coverage is n/a.

❗ Current head f768d45 differs from pull request most recent head 8b611f8. Consider uploading reports for the commit 8b611f8 to get more accurate results

@@             Coverage Diff              @@
##           ign-gazebo6    #1480   +/-   ##
============================================
  Coverage        33.58%   33.58%           
============================================
  Files               44       44           
  Lines             2260     2260           
============================================
  Hits               759      759           
  Misses            1501     1501           

---

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 26f67fa...8b611f8. Read the comment docs.

[chapulina]

LGTM, just a couple of minor comments.

[azeey]

@iche033 RgbdCameraTest.RgbdCameraBox and SensorsFixture.SensorsBatteryState seem to be broken on ign-gazebo6 due to this PR. And SensorsFixture.SensorsBatteryState and WideAngleCameraTest.WideAngleCameraBox on main (reverting this PR on main fixes SensorsFixture.SensorsBatteryState and WideAngleCameraTest.WideAngleCameraBox). Do you mind taking a look?

[iche033]

ok yeah I'll take a look

[iche033]

#1528 should fix WideAngleCameraTest.WideAngleCameraBox
#1529 should fix SensorsFixture.SensorsBatteryState

hmm I'm not able to reproduce RgbdCameraTest.RgbdCameraBox. It could be fixed by #1513?

[azeey]

Thanks! Yes, I think RgbdCameraTest.RgbdCameraBox was fixed by #1513: https://build.osrfoundation.org/view/ign-fortress/job/ignition_gazebo-ci-ign-gazebo6-focal-amd64/63/#showFailuresLink