👩‍🌾 Make thermal sensor test more robust

[chapulina]

🦟 Bug fix

Fixes #667

Summary

I spent some time debugging #667. It's interesting that the test is pretty robust on Dome, but very flaky on Edifice and Fortress.

Even more interesting is that the test only runs for 10 iterations on Dome:

https://github.com/ignitionrobotics/ign-gazebo/blob/9cafd6d9f43f73fb82b809b38cee4b316a390489/test/integration/thermal_sensor_system.cc#L182

But it runs for 35 iterations on Edifice and Fortress:

https://github.com/ignitionrobotics/ign-gazebo/blob/0c401c17c1d4b748233c61921c47da5725ce9d5c/test/integration/thermal_sensor_system.cc#L189

So in theory, the test should be more robust on the later versions.

I verified locally that 10 iterations are not enough for the test to pass on Edifice. Specifically, this piece of code for creating light visuals makes the rendering update take so long, that all 10 Sensors::PostUpdates happen before the thermal sensor is ready to publish.

https://github.com/ignitionrobotics/ign-gazebo/blob/0c401c17c1d4b748233c61921c47da5725ce9d5c/src/rendering/RenderUtil.cc#L725-L738

The solution I have here is not to create light visuals when sensors are enabled, because we don't need those on the backend. This will need to be revisited on Fortress, in the context of #556.

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)
• 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

🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸

https://github.com/osrf/buildfarmer/issues/224

[adlarkin]

Thanks for looking into this! While I understand the cause of the issue, I'm still trying to understand why we don't need to create light visuals when sensors are enabled - you mention that we don't need them on the backend, but what about the GUI side? Does having sensors enabled change the behavior of RenderUtil::Update to only operate on the backend? (my understanding of how RenderUtil works within ign-gazebo is lacking, so hopefully I can learn something new 😄)

[chapulina]

Good questions, @adlarkin ! The light visuals are these green lines that represent the position of lights in the scene:

They're meant to help users while looking at the GUI, but we don't want camera sensors seeing those, for example. We prevent cameras from seeing them through the use of visibility flags. So those visuals are created on the backend, but no one sees them...

One way to differentiate between the GUI scene and the sensors (backend) scene is that the backend scene has sensors enabled, while the GUI doesn't. That happens here:

https://github.com/ignitionrobotics/ign-gazebo/blob/0c401c17c1d4b748233c61921c47da5725ce9d5c/src/systems/sensors/Sensors.cc#L377-L379

To have sensors enabled means that things like depth and thermal cameras will be spawned into the scene. We don't want those to be spawned in the user's scene, because that doesn't have all the logic to update these sensors in the correct frame rate and publish their messages. That's the responsibility of the backend scene.

Now, once we have both scenes combined (GUI and sensors sharing the same scene), light visuals and sensors will need to coexist in the same scene, so we'll need to live with the delays there.

[adlarkin]

Thanks for the explanation - that all makes sense 🤓 LGTM!