gazebosim / gz-sim

Open source robotics simulator. The latest version of Gazebo.
https://gazebosim.org
Apache License 2.0
630 stars 251 forks source link

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

Open adlarkin opened 3 years ago

adlarkin commented 3 years ago

Environment

Description

795 points out that EachNew should not be used in system plugin's PreUpdate function, because entities are created during PreUpdate, so depending on the order in which systems are run, calling EachNew in a system's PreUpdate method may result in missing newly created entities. However, there are several system plugins in the ign-gazebo repository that call EachNew in PreUpdate:

These systems should be updated so that EachNew is not being called in PreUpdate, and we should also probably add/update tests for these systems as needed that spawn entities after startup.

Steps to reproduce

For a concrete example of how/why this is problematic, launch sensors.sdf, which has the UserCommands system come after the Imu system. Then, add an imu sensor from the command line after startup. Since this is processed through UserCommands, which has its PreUpdate run after Imu's PreUpdate, the Imu will not register this sensor (thanks to @chapulina for providing this test case).

Here are the steps to test/reproduce this scenario:

  1. Start a running simulation using sensors.sdf:
    ign gazebo sensors.sdf -r
  2. Create an imu sensor from the command line
    ign service -s /world/sensors/create --reqtype ignition.msgs.EntityFactory --reptype ignition.msgs.Boolean --timeout 300 --req 'sdf: ''"<?xml version=\"1.0\" ?>''<sdf version=\"1.6\">''<model name=\"spawned_model\">''<link name=\"link\"><sensor name=\"imu\" type=\"imu\"><topic>test_imu</topic></sensor></link>''</model>''</sdf>" ''pose: {position: {z: 10}}'

You'll then see the following error message:

[Err] [Imu.cc:224] Failed to update IMU: 19. Entity not found.
adlarkin commented 3 years ago

The following issue could also be related: https://github.com/ignitionrobotics/ign-gazebo/issues/83

chapulina commented 2 years ago