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

Physics: set link velocity from *VelocityReset components #2489

Merged
merged 11 commits into from
Aug 14, 2024

Conversation

yaswanth1701
Copy link
Contributor

🎉 New feature

Closes #

Summary

Follow-up PR to #2440. Added link velocity reset component for linear and angular velocity. Updated the physics system plugin to support the reset components of link velocity.

Test it

Have to add test case.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

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.

@github-actions github-actions bot added the 🏛️ ionic Gazebo Ionic label Jul 22, 2024
Copy link

codecov bot commented Jul 22, 2024

Codecov Report

Attention: Patch coverage is 55.68182% with 39 lines in your changes missing coverage. Please review.

Project coverage is 65.87%. Comparing base (098085b) to head (f3244fa).
Report is 73 commits behind head on main.

Files Patch % Lines
src/systems/physics/Physics.cc 53.57% 39 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2489      +/-   ##
==========================================
- Coverage   65.95%   65.87%   -0.08%     
==========================================
  Files         327      330       +3     
  Lines       31319    31831     +512     
==========================================
+ Hits        20655    20969     +314     
- Misses      10664    10862     +198     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@azeey azeey added the beta Targeting beta release of upcoming collection label Jul 29, 2024
@scpeters scpeters changed the title added link velocity components and changes to physics systems Physics: set link velocity from *VelocityReset components Jul 30, 2024
Copy link
Member

@scpeters scpeters left a 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 and left some comments; I'll make a suggestion shortly about how to test this

Comment on lines 34 to 36
/// \brief Link initial angular velocity in its own frame and
/// in SI units (rad/s).The angular velocity of entity is
/// represented by gz::math::vector3d.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// \brief Link initial angular velocity in its own frame and
/// in SI units (rad/s).The angular velocity of entity is
/// represented by gz::math::vector3d.
/// \brief Angular velocity of an entity in its own frame
/// and in SI units (rad/s). The angular velocity is
/// represented by gz::math::Vector3d.

include/gz/sim/components/AngularVelocityReset.hh Outdated Show resolved Hide resolved
include/gz/sim/components/LinearVelocityReset.hh Outdated Show resolved Hide resolved
include/gz/sim/components/LinearVelocityReset.hh Outdated Show resolved Hide resolved
src/systems/physics/Physics.cc Outdated Show resolved Hide resolved
src/systems/physics/Physics.cc Outdated Show resolved Hide resolved
math::eigen3::convert(worldAngularVel));

return true;
});
Copy link
Member

Choose a reason for hiding this comment

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

nit: the indentation level of this block has an odd number of spaces in some parts. please check and match the indentation level of similar code blocks

src/systems/physics/Physics.cc Outdated Show resolved Hide resolved
@scpeters
Copy link
Member

for a test, I suggest a similar structure to the test for JointVelocityReset in https://github.com/gazebosim/gz-sim/blob/gz-sim8/test/integration/physics_system.cc#L750-L847

if we borrow a 1x4x9 box from the boxes benchmark with the "complex" case (gravity and gyroscopic tumbling) we could set the initial linear velocity and expect it to change according to gravity, and we could set the initial angular velocity and expect it to rotate while preserving angular momentum in the world frame

@yaswanth1701 yaswanth1701 force-pushed the link_reset_components branch 2 times, most recently from 21f0b9a to 11db468 Compare July 31, 2024 15:50
@yaswanth1701
Copy link
Contributor Author

for a test, I suggest a similar structure to the test for JointVelocityReset in https://github.com/gazebosim/gz-sim/blob/gz-sim8/test/integration/physics_system.cc#L750-L847

if we borrow a 1x4x9 box from the boxes benchmark with the "complex" case (gravity and gyroscopic tumbling) we could set the initial linear velocity and expect it to change according to gravity, and we could set the initial angular velocity and expect it to rotate while preserving angular momentum in the world frame

okay, I will check this out and will add the test.

@scpeters
Copy link
Member

scpeters commented Aug 2, 2024

I've made an example test for WorldLinearVelocityReset in yaswanth1701#1

please merge that in and then add one for WorldAngularVelocityReset if you have time

@yaswanth1701
Copy link
Contributor Author

I've made an example test for WorldLinearVelocityReset in yaswanth1701#1

please merge that in and then add one for WorldAngularVelocityReset if you have time

yeah, sure I will add the test for angular velocity reset.

@yaswanth1701 yaswanth1701 force-pushed the link_reset_components branch 3 times, most recently from 6472309 to 23ccac4 Compare August 8, 2024 14:19
Copy link
Member

@scpeters scpeters left a comment

Choose a reason for hiding this comment

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

just a few small suggestions; the new test looks good!

include/gz/sim/components/LinearVelocityReset.hh Outdated Show resolved Hide resolved
test/integration/physics_system.cc Outdated Show resolved Hide resolved
test/integration/physics_system.cc Outdated Show resolved Hide resolved
include/gz/sim/components/AngularVelocityReset.hh Outdated Show resolved Hide resolved
include/gz/sim/components/AngularVelocityReset.hh Outdated Show resolved Hide resolved
yaswanth1701 and others added 10 commits August 14, 2024 14:30
Signed-off-by: yaswanth1701 <[email protected]>
Signed-off-by: Steve Peters <[email protected]>
Signed-off-by: yaswanth1701 <[email protected]>
Signed-off-by: yaswanth1701 <[email protected]>
Co-authored-by: Steve Peters <[email protected]>
Signed-off-by: Yaswanth <[email protected]>
Co-authored-by: Steve Peters <[email protected]>
Signed-off-by: Yaswanth <[email protected]>
@yaswanth1701 yaswanth1701 force-pushed the link_reset_components branch from 36015a3 to 4477f77 Compare August 14, 2024 09:00
@yaswanth1701
Copy link
Contributor Author

just a few small suggestions; the new test looks good!

done

@scpeters scpeters merged commit 5039e20 into gazebosim:main Aug 14, 2024
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta Targeting beta release of upcoming collection 🏛️ ionic Gazebo Ionic
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants