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.

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

Contact system: don't use 'EachNew' in PreUpdate #2303

Open
wants to merge 3 commits into
base: gz-sim8
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 36 additions & 9 deletions src/systems/contact/Contact.cc
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ class gz::sim::systems::ContactPrivate
{
/// \brief Create sensors that correspond to entities in the simulation
/// \param[in] _ecm Mutable reference to ECM.
public: void CreateSensors(EntityComponentManager &_ecm);
public: void CreateSensors(const EntityComponentManager &_ecm);

/// \brief Update and publish sensor data
/// \param[in] _ecm Immutable reference to ECM.
Expand All @@ -100,6 +100,11 @@ class gz::sim::systems::ContactPrivate
/// \brief A map of Contact entity to its Contact sensor.
public: std::unordered_map<Entity,
std::unique_ptr<ContactSensor>> entitySensorMap;

/// \brief Keep list of sensors that were created during the previous
/// `PostUpdate`, so that components can be created during the next
/// `PreUpdate`.
public: std::unordered_set<Entity> newSensors;
};

//////////////////////////////////////////////////
Expand Down Expand Up @@ -154,7 +159,7 @@ void ContactSensor::Publish()
}

//////////////////////////////////////////////////
void ContactPrivate::CreateSensors(EntityComponentManager &_ecm)
void ContactPrivate::CreateSensors(const EntityComponentManager &_ecm)
{
GZ_PROFILE("ContactPrivate::CreateSensors");
_ecm.EachNew<components::ContactSensor>(
Expand Down Expand Up @@ -193,10 +198,6 @@ void ContactPrivate::CreateSensors(EntityComponentManager &_ecm)
// We assume that if childEntities is not empty, it only has one
// element.
collisionEntities.push_back(childEntities.front());

// Create component to be filled by physics.
_ecm.CreateComponent(childEntities.front(),
components::ContactSensorData());
}
}

Expand All @@ -206,6 +207,7 @@ void ContactPrivate::CreateSensors(EntityComponentManager &_ecm)
sensor->Load(_contact->Data(), defaultTopic, collisionEntities);
this->entitySensorMap.insert(
std::make_pair(_entity, std::move(sensor)));
this->newSensors.insert(_entity);

return true;
});
Expand All @@ -218,12 +220,17 @@ void ContactPrivate::UpdateSensors(const UpdateInfo &_info,
GZ_PROFILE("ContactPrivate::UpdateSensors");
for (const auto &item : this->entitySensorMap)
{
// Check if any new sensors have been created in the last preUpdate
// if yes, omit update for those
if (this->newSensors.find(item.first) != this->newSensors.end())
{
continue;
}

for (const Entity &entity : item.second->collisionEntities)
{
auto contacts = _ecm.Component<components::ContactSensorData>(entity);

// We will assume that the ContactData component will have been created if
// this entity is in the collisionEntities list
if (contacts->Data().contact_size() > 0)
{
item.second->AddContacts(_info.simTime, contacts->Data());
Expand Down Expand Up @@ -263,7 +270,25 @@ Contact::Contact() : System(), dataPtr(std::make_unique<ContactPrivate>())
void Contact::PreUpdate(const UpdateInfo &, EntityComponentManager &_ecm)
{
GZ_PROFILE("Contact::PreUpdate");
this->dataPtr->CreateSensors(_ecm);

// Create components
for (auto entity : this->dataPtr->newSensors)
{
auto it = this->dataPtr->entitySensorMap.find(entity);
if (it == this->dataPtr->entitySensorMap.end())
{
gzerr << "Entity [" << entity
<< "] isn't in sensor map, this shouldn't happen." << std::endl;
continue;
}

for(const auto &collisionEntity : it->second->collisionEntities){
Copy link
Contributor

Choose a reason for hiding this comment

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

coding style nit:

Suggested change
for(const auto &collisionEntity : it->second->collisionEntities){
for (const auto &collisionEntity : it->second->collisionEntities)
{

_ecm.CreateComponent(collisionEntity, components::ContactSensorData());
}

}

this->dataPtr->newSensors.clear();
}

//////////////////////////////////////////////////
Expand All @@ -280,6 +305,8 @@ void Contact::PostUpdate(const UpdateInfo &_info,
<< "s]. System may not work properly." << std::endl;
}

this->dataPtr->CreateSensors(_ecm);

if (!_info.paused)
{
this->dataPtr->UpdateSensors(_info, _ecm);
Expand Down
7 changes: 6 additions & 1 deletion src/systems/touch_plugin/TouchPlugin.cc
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,9 @@ class gz::sim::systems::TouchPluginPrivate
/// \brief Mutex for variables mutated by the service callback.
/// The variables are: touchPub, touchStart, enabled
public: std::mutex serviceMutex;

/// \brief Flag if the first simulation step has already passed
public: bool ranOnce{false};
};

//////////////////////////////////////////////////
Expand Down Expand Up @@ -384,7 +387,8 @@ void TouchPlugin::Configure(const Entity &_entity,
void TouchPlugin::PreUpdate(const UpdateInfo &, EntityComponentManager &_ecm)
{
GZ_PROFILE("TouchPlugin::PreUpdate");
if ((!this->dataPtr->initialized) && this->dataPtr->sdfConfig)

if ((!this->dataPtr->initialized) && this->dataPtr->sdfConfig && this->dataPtr->ranOnce)
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of using ranOnce, you can check to make sure the ContactSensorData component type exists first:

Suggested change
if ((!this->dataPtr->initialized) && this->dataPtr->sdfConfig && this->dataPtr->ranOnce)
if ((!this->dataPtr->initialized) && this->dataPtr->sdfConfig &&
_ecm.HasComponentType(components::ContactSensorData::typeId))

{
// We call Load here instead of Configure because we can't be guaranteed
// that all entities have been created when Configure is called
Expand All @@ -405,6 +409,7 @@ void TouchPlugin::PreUpdate(const UpdateInfo &, EntityComponentManager &_ecm)
});
this->dataPtr->AddTargetEntities(_ecm, potentialEntities);
}
this->dataPtr->ranOnce = true;
}

//////////////////////////////////////////////////
Expand Down
Loading