-
Notifications
You must be signed in to change notification settings - Fork 254
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
base: gz-sim8
Are you sure you want to change the base?
Conversation
…Update step Signed-off-by: Yannick Schulz <[email protected]>
Thanks for the contribution @yschulz. It looks like this broke the |
I noticed, I will have a look first thing on Monday. |
…ct sensor to start up Signed-off-by: Yannick Schulz <[email protected]>
@azeey I had look earlier. What happens is that most tests failed, except the parent entity removal and entity spawning test. So basically I created the inverse Problem. Starting the As far as I understand, the Also, as I can see, the major difference is that I fixed the failing tests now by adding a check to the However, I am not sure if that is a desired fix for the problem. |
@azeey to take another look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed the failing tests now by adding a check to the TouchPlugin PreUpdate function so it only runs when it already ran once before.
However, I am not sure if that is a desired fix for the problem.
I added a comment regarding this. We can check to make sure the ContactSensorData
component type exists first before initializing the touch plugin.
@@ -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) |
There was a problem hiding this comment.
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:
if ((!this->dataPtr->initialized) && this->dataPtr->sdfConfig && this->dataPtr->ranOnce) | |
if ((!this->dataPtr->initialized) && this->dataPtr->sdfConfig && | |
_ecm.HasComponentType(components::ContactSensorData::typeId)) |
continue; | ||
} | ||
|
||
for(const auto &collisionEntity : it->second->collisionEntities){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
coding style nit:
for(const auto &collisionEntity : it->second->collisionEntities){ | |
for (const auto &collisionEntity : it->second->collisionEntities) | |
{ |
One thing I realized about this PR is that it's trying to spawn the |
I just had a look at the PR and indeed, that makes for a much cleaner solution. This also eliminates the need to delay the |
cc @scpeters |
I used a logarithmic progression in the example because it's easier for me to visually see that numbers with more digits are larger. I also used a logarithmic progression in #2500, but a linear progression could be used as well. If you anticipate a cascade of N systems, then having a priority difference of at least |
For harmonic, we backported the ability to set system priority explicitly in xml in a world file, but we made a better fix in Ionic, which sets some default system priorities to better values, including the UserCommands system. Would you be willing to test the new behavior with Ionic to see if a change is needed there? We would still be willing to merge a fix for Harmonic, but I'd like to know if it's a fix that we shouldn't merge forward or if a fix is needed in both places |
🦟 Bug fix
Fixes #2223
Summary
The contact sensor system plugin could not be created through the
UserCommand
system and the factory service. The reason was a call toEachNew
in thePreUpdate
step. Similar to #1523 and as part of #1281 this PR separates the component from sensor creation in the pre and post updates.Checklist
codecheck
passed (See contributing)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.