Remove EachNew calls from sensor PreUpdates #1281

chapulina · 2022-01-11T22:52:29Z

🦟 Bug fix

Part of Gazebo system plugins use EntityComponentManager::EachNew in PreUpdate #797

Summary

Calling EachNew in PreUpdate is unreliable because new entities may be created during another system's PreUpdate and then you miss it. See #797 for a detailed description of the issue.

This PR tackles only non-rendering sensors. I'm using the same approach as in #1248 . This is the new flow:

SdfEntityCreator creates the sensor entity and fills some components from SDF
PreUpdate: the first PreUpdate doesn't do anything
Update: Physics populates the components created by SdfEntityCreator
PostUpdate: The sensor system does a few things:
1. Creates new ign-sensors objects for new sensors
2. Updates all sensors with the data from the components that were filled by physics
3. Removes sensors that need to be removed
The next PreUpdate creates new components for the new sensors created during the previous PostUpdate, which is only SensorTopic

There's a slight change in behaviour, which is that a sensor won't have a SensorTopic component in the first iteration. This means that any Each call that includes that component will only be called from the 2nd iteration after the component is created. Since that component is not critical to the sensor's functioning and I've only seen it being used by the GUI, I think this is a safe change. See the consequences on the updated tests.

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 and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

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

Signed-off-by: Louise Poubel <louise@openrobotics.org>

codecov · 2022-01-11T23:58:56Z

Codecov Report

Merging #1281 (75c78ad) into ign-gazebo3 (700b369) will increase coverage by 0.00%.
The diff coverage is 69.69%.

@@             Coverage Diff              @@
##           ign-gazebo3    #1281   +/-   ##
============================================
  Coverage        77.21%   77.21%           
============================================
  Files              227      227           
  Lines            13535    13575   +40     
============================================
+ Hits             10451    10482   +31     
- Misses            3084     3093    +9

Impacted Files	Coverage Δ
src/systems/air_pressure/AirPressure.cc	`72.83% <69.23%> (-1.14%)`	⬇️
src/systems/altimeter/Altimeter.cc	`73.49% <69.23%> (-1.18%)`	⬇️
src/systems/logical_camera/LogicalCamera.cc	`75.00% <69.23%> (-1.25%)`	⬇️
src/systems/magnetometer/Magnetometer.cc	`70.78% <69.23%> (-0.82%)`	⬇️
src/systems/imu/Imu.cc	`71.42% <71.42%> (-0.87%)`	⬇️
src/SimulationRunner.cc	`94.51% <0.00%> (+1.06%)`	⬆️

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 700b369...75c78ad. Read the comment docs.

adlarkin · 2022-01-13T20:43:32Z

It looks like there's a new warning in Ubuntu CI:

FindOpenGL.cmake:275

Policy CMP0072 is not set: FindOpenGL prefers GLVND by default when...

Should this be addressed in a separate PR?

chapulina · 2022-01-13T21:29:03Z

It looks like there's a new warning in Ubuntu CI:

That's been happening since CI has migrated to Focal, it's being tracked here: gazebosim/gz-rendering#360. I mentioned in #1274 (comment) that I think an ign-gui3 release will fix it, and I was waiting on gazebosim/gz-gui#335 to make an ign-gui3 release.

adlarkin

LGTM! There are a few methods that can be renamed for clarity, which I highlighted below.

src/systems/air_pressure/AirPressure.cc

src/systems/altimeter/Altimeter.cc

src/systems/air_pressure/AirPressure.cc

src/systems/imu/Imu.cc

src/systems/logical_camera/LogicalCamera.cc

src/systems/magnetometer/Magnetometer.cc

Signed-off-by: Louise Poubel <louise@openrobotics.org>

osrf-triage · 2022-03-01T18:31:22Z

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-03-01-citadel-edifice-fortress/1313/1

osrf-triage · 2022-04-13T21:31:19Z

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-04-13-fortress-edifice/1367/1

Split from #1471 to address force-torque sensor incrementally. Partially addresses #797 Not all sensor system implementations were updated as part of #1281, one of which being the ForceTorqueSensor. This makes the reset behavior incorrect and the sensor won't be respawned. Signed-off-by: Michael Carroll <michael@openrobotics.org> Signed-off-by: Louise Poubel <louise@openrobotics.org> Co-authored-by: Louise Poubel <louise@openrobotics.org>

chapulina added 2 commits January 11, 2022 14:41

Remove EachNew calls from sensor PreUpdates

c51a13b

Signed-off-by: Louise Poubel <louise@openrobotics.org>

Migration and codecheckl

de366fa

Signed-off-by: Louise Poubel <louise@openrobotics.org>

chapulina added bug Something isn't working sensors Sensors and sensor data labels Jan 11, 2022

osrf-triage added this to Inbox in Core development Jan 11, 2022

github-actions bot added the 🏰 citadel Ignition Citadel label Jan 11, 2022

This was referenced Jan 11, 2022

Add NavSat sensor (GPS) #1248

Merged

Gazebo system plugins use EntityComponentManager::EachNew in PreUpdate #797

Open

chapulina moved this from Inbox to In review in Core development Jan 11, 2022

adlarkin self-requested a review January 13, 2022 20:45

adlarkin mentioned this pull request Jan 15, 2022

Cleanup update call for non-rendering sensors #1282

Merged

7 tasks

adlarkin approved these changes Jan 15, 2022

View reviewed changes

PR feedback, rename create functions

75c78ad

Signed-off-by: Louise Poubel <louise@openrobotics.org>

chapulina merged commit 650b746 into ign-gazebo3 Jan 18, 2022

Core development automation moved this from In review to Done Jan 18, 2022

chapulina deleted the chapulina/3/sensor_preupdate branch January 18, 2022 16:59

chapulina mentioned this pull request Jan 25, 2022

Some sensor systems do not work when model is spawned #1245

Closed

mjcarroll mentioned this pull request May 3, 2022

Refactor common sensor setup interactions #1471

Closed

j-rivero removed this from Done in Core development May 6, 2022

mjcarroll mentioned this pull request Jun 7, 2022

Don't use 'EachNew' in ForceTorque PreUpdate function #1523

Merged

8 tasks

yschulz mentioned this pull request Nov 2, 2023

System plugins (contact sensor) still not working when spawned #2223

Open

18 tasks

yschulz mentioned this pull request Feb 2, 2024

Don't use 'EachNew' in Contact PreUpdate function #2303

Open

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove EachNew calls from sensor PreUpdates #1281

Remove EachNew calls from sensor PreUpdates #1281

chapulina commented Jan 11, 2022

codecov bot commented Jan 11, 2022 •

edited

Loading

adlarkin commented Jan 13, 2022

chapulina commented Jan 13, 2022

adlarkin left a comment

osrf-triage commented Mar 1, 2022

osrf-triage commented Apr 13, 2022

Remove EachNew calls from sensor PreUpdates #1281

Remove EachNew calls from sensor PreUpdates #1281

Conversation

chapulina commented Jan 11, 2022

🦟 Bug fix

Summary

Checklist

codecov bot commented Jan 11, 2022 • edited Loading

Codecov Report

adlarkin commented Jan 13, 2022

chapulina commented Jan 13, 2022

adlarkin left a comment

Choose a reason for hiding this comment

osrf-triage commented Mar 1, 2022

osrf-triage commented Apr 13, 2022

codecov bot commented Jan 11, 2022 •

edited

Loading