-
Notifications
You must be signed in to change notification settings - Fork 42
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
Apply gravity external to dartsim for added mass #462
Conversation
Codecov Report
@@ Coverage Diff @@
## gz-physics6 #462 +/- ##
===============================================
+ Coverage 75.43% 75.52% +0.09%
===============================================
Files 140 142 +2
Lines 7099 7138 +39
===============================================
+ Hits 5355 5391 +36
- Misses 1744 1747 +3
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
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.
The implementation looks good, my major comment is for the unit test, which only checks gravity application for a body with zero added mass.
Signed-off-by: Michael Carroll <[email protected]>
Signed-off-by: Michael Carroll <[email protected]> Co-authored-by: Alejandro Hernández Cordero <[email protected]>
Signed-off-by: Michael Carroll <[email protected]>
Signed-off-by: Michael Carroll <[email protected]>
…s_gravity Signed-off-by: Michael Carroll <[email protected]>
4dc22d6
to
2629c90
Compare
Signed-off-by: Michael Carroll <[email protected]>
Signed-off-by: Michael Carroll <[email protected]>
8c9fc92
to
2776827
Compare
Signed-off-by: Michael Carroll <[email protected]>
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've read the code, but I'm not familiar with gz-physics, so I'm leaning on Joan's review for the most part.
For the tests, I guess the top-level test is test/common_test/added_mass.cc
, and it's looking at 3 spheres without added mass, and 1 sphere with added mass. I don't know enough to say whether that's sufficient, but more tests can always be added later.
Since this is a new feature and doesn't break existing things, and CI is passing, it's probably okay.
🎉 New feature
Summary
When fluid added mass (#384) is used, dartsim will apply gravitational force to the total virtual mass of each link. Since this mass is only to model the inertial aspects of the dynamics, this is incorrect behavior.
In this PR, I disable dart's application of gravity to links that have added mass defined in their sdf file. Instead, gravity is applied as an external force right before the world update by multiplying the link mass by the gravity vector (F = ma!).
Test it
The unit test verifies that when added mass is set to all zeros, that gravity continues to work as expected.
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.