-
Notifications
You must be signed in to change notification settings - Fork 276
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
Populate names of colliding entities in contact points message #1351
Populate names of colliding entities in contact points message #1351
Conversation
Signed-off-by: Aditya <[email protected]>
Signed-off-by: Aditya <[email protected]>
Signed-off-by: Aditya <[email protected]>
Can you also fix the codecheck errors? |
Signed-off-by: Aditya <[email protected]>
Codecov Report
@@ Coverage Diff @@
## ign-gazebo6 #1351 +/- ##
===============================================
+ Coverage 62.98% 62.99% +0.01%
===============================================
Files 301 301
Lines 24199 24208 +9
===============================================
+ Hits 15242 15251 +9
Misses 8957 8957
Continue to review full report at Codecov.
|
Thanks for the PR, it looks useful! However, I would really like to see a performance analysis of this. Try to create a world with say 10 000 contacts and measure the update rate with and without this PR... |
Performance benchmarking :Generate world fileI used the following
Then, Data collectionI collected the real time factor data in a text file (let the simulation run for around 30 seconds) : Results
On an average, the real time factor for this branch seems to be slower by 1.5% |
Signed-off-by: Aditya <[email protected]>
2e2e1dd
to
e08983e
Compare
Added an optional parameter so that the names of colliding entities will not be populated and performance won't be affected. This parameter remains true by default (so the names will be populated). |
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.
Looks great! Just a couple of minor comments.
Also could you document the parameter in |
Signed-off-by: Aditya <[email protected]>
60a263b
to
ed305a9
Compare
I've added the docstring, not sure if that is the preferred style though. |
@@ -64,6 +64,19 @@ namespace systems | |||
|
|||
/// \class Physics Physics.hh ignition/gazebo/systems/Physics.hh | |||
/// \brief Base class for a System. | |||
/// Includes optional parameter : <include_entity_names>. When set |
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.
This is good. For other systems we have been listing the parameters in a bullet list, but since this is the only parameter here, I think the format is fine.
Thanks a lot for the benchmark, it's good to see the performance isn't affected that heavily. In normal situations with just a few contacts, the difference would be negligible. |
Signed-off-by: Aditya <[email protected]>
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-25-fortress-edifice-citadel/1343/1 |
🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸
🎉 New feature
Summary
Currently, the contact points message has a
Enitity
member whosename
field is not populated and just contains theid
. This PR populates the name with the entity's full name, for e.g.world:shapes:model:ground_plane:link:link:collision:collision
so it is clear in the message which pair of entities is colliding.Test it
Modified an existing test case to check the full name of a colliding entity (
contact_system.cc
integration test)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.🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸