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.

Sign up for GitHub

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

Jump to bottom

Remove EachNew calls from sensor PreUpdates #1281

Merged
merged 3 commits into from
Jan 18, 2022
Merged

Remove EachNew calls from sensor PreUpdates #1281

merged 3 commits into from
Jan 18, 2022

Conversation

chapulina
Copy link
Contributor

🦟 Bug fix

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:

  1. SdfEntityCreator creates the sensor entity and fills some components from SDF
  2. PreUpdate: the first PreUpdate doesn't do anything
  3. Update: Physics populates the components created by SdfEntityCreator
  4. 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
  5. 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.

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

@chapulina chapulina added bug Something isn't working sensors Sensors and sensor data labels Jan 11, 2022
@github-actions github-actions bot added the 🏰 citadel Ignition Citadel label Jan 11, 2022
This was referenced Jan 11, 2022
Merged
Open
@codecov
Copy link

codecov bot commented Jan 11, 2022 •
edited
Loading

Codecov Report

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

Impacted file tree graph

@@             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
Copy link
Contributor

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?

@adlarkin adlarkin self-requested a review January 13, 2022 20:45
@chapulina
Copy link
Contributor Author

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 adlarkin mentioned this pull request Jan 15, 2022
Merged
7 tasks
adlarkin
adlarkin approved these changes Jan 15, 2022
View reviewed changes
Copy link
Contributor

@adlarkin adlarkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/systems/air_pressure/AirPressure.cc Outdated Show resolved Hide resolved
src/systems/altimeter/Altimeter.cc Outdated Show resolved Hide resolved
src/systems/air_pressure/AirPressure.cc Outdated Show resolved Hide resolved
src/systems/imu/Imu.cc Outdated Show resolved Hide resolved
src/systems/logical_camera/LogicalCamera.cc Outdated Show resolved Hide resolved
src/systems/magnetometer/Magnetometer.cc Outdated Show resolved Hide resolved
@chapulina chapulina merged commit 650b746 into ign-gazebo3 Jan 18, 2022
@chapulina chapulina deleted the chapulina/3/sensor_preupdate branch January 18, 2022 16:59
@chapulina chapulina mentioned this pull request Jan 25, 2022
Closed
@osrf-triage
Copy link

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
Copy link

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

@mjcarroll mjcarroll mentioned this pull request May 3, 2022
Closed
@mjcarroll mjcarroll mentioned this pull request Jun 7, 2022
Merged
8 tasks
mjcarroll added a commit that referenced this pull request Jul 21, 2022
@mjcarroll @chapulina
Don't use 'EachNew' in ForceTorque PreUpdate function (#1523)
8a651db 
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 <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
Co-authored-by: Louise Poubel <[email protected]>
@yschulz yschulz mentioned this pull request Nov 2, 2023
Open
18 tasks
@yschulz yschulz mentioned this pull request Feb 2, 2024
Open
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adlarkin adlarkin adlarkin approved these changes

Assignees
No one assigned
Labels
bug Something isn't working 🏰 citadel Ignition Citadel sensors Sensors and sensor data
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@chapulina @adlarkin @osrf-triage