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

Add force offset support to ApplyLinkWrench system and to Link API #2026

Merged
merged 15 commits into from
Aug 9, 2023

Conversation

Henrique-BO
Copy link
Contributor

@Henrique-BO Henrique-BO commented Jun 30, 2023

🎉 New feature

Summary

Adds support for specifying the application point of a force in the ApplyLinkWrench system. This is done through an offset expressed in the link frame (thus, relative to the link origin).

This uses the force_offset field that was already present in the Wrench message, but wasn't being used in the system. Also, an overloaded method AddWorldWrench was added to the Link interface.

Test it

Open a test world that has the ApplyLinkWrench system:

gz sim src/gz-sim/examples/worlds/apply_link_wrench.sdf

Send a message to the /world/<world_name>/wrench topic:

gz topic -t "/world/apply_link_wrench/wrench" -m gz.msgs.EntityWrench  -p "entity: {name: 'cylinder::link', type: LINK}, wrench: {force: {z: 100}, force_offset: {x: 0, y: 100, z: 0}}"

Screencast from 30-06-2023 18_08_34

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.

Signed-off-by: Henrique-BO <henrique.barrosoliveira@usp.br>
@github-actions github-actions bot added the 🌱 garden Ignition Garden label Jun 30, 2023
@codecov
Copy link

codecov bot commented Jun 30, 2023

Codecov Report

Merging #2026 (5bba53c) into gz-sim7 (e19f26d) will increase coverage by 0.00%.
The diff coverage is 96.42%.

❗ Current head 5bba53c differs from pull request most recent head f36ac69. Consider uploading reports for the commit f36ac69 to get more accurate results

@@           Coverage Diff            @@
##           gz-sim7    #2026   +/-   ##
========================================
  Coverage    65.07%   65.08%           
========================================
  Files          354      354           
  Lines        28720    28741   +21     
========================================
+ Hits         18690    18705   +15     
- Misses       10030    10036    +6     
Files Changed Coverage Δ
include/gz/sim/Link.hh 100.00% <ø> (ø)
src/systems/apply_link_wrench/ApplyLinkWrench.hh 100.00% <ø> (ø)
src/systems/apply_link_wrench/ApplyLinkWrench.cc 78.83% <88.88%> (+1.05%) ⬆️
src/Link.cc 95.34% <100.00%> (+0.57%) ⬆️

... and 2 files with indirect coverage changes

ahcorde
ahcorde previously requested changes Jul 4, 2023
Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

can you add a test to this new method?

Signed-off-by: Henrique-BO <henrique.barrosoliveira@usp.br>
@Henrique-BO
Copy link
Contributor Author

Added a test in 34101f7. Since there already was a LinkAddWorldForce test doing very similar checks and setup, I just added a few lines to the existing function. Should we keep it like this (and so maybe rename the test) or is it better to have a separate test?

@azeey
Copy link
Contributor

azeey commented Jul 11, 2023

@osrf-jenkins run tests please

Copy link
Contributor

@quarkytale quarkytale left a comment

Choose a reason for hiding this comment

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

Works for me

@quarkytale quarkytale requested a review from ahcorde July 11, 2023 23:11
include/gz/sim/Link.hh Outdated Show resolved Hide resolved
Signed-off-by: Henrique-BO <henrique.barrosoliveira@usp.br>
@azeey azeey added the beta Targeting beta release of upcoming collection label Jul 31, 2023
@@ -80,16 +80,17 @@ class gz::sim::systems::ApplyLinkWrenchPrivate
/// \param[in] _msg Entity message. If it's a link, that link is returned. If
/// it's a model, its canonical link is returned.
/// \param[out] Force to apply.
/// \param[out] Offset of the force application point relative to the
Copy link
Contributor

Choose a reason for hiding this comment

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

This implies that the offset in msgs::EntityWrench and by association, in the component::ExternalWorldWrench. is relative to the COM, but the point of application is the link origin. Am I right? If so, I think we should avoid having two different reference points. If the point of application is the link origin, the offset should also be from the link origin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed in ee693bc so that the Link API and the ApplyLinkWrench system interpret the offset in the link frame. This way, the semantics of the offset are consistent between them, the msgs::EntityWrench message and the component::ExternalWorldWrench component.

Signed-off-by: Henrique-BO <henrique.barrosoliveira@usp.br>
src/Link.cc Show resolved Hide resolved
include/gz/sim/Link.hh Outdated Show resolved Hide resolved
Signed-off-by: Henrique-BO <henrique.barrosoliveira@usp.br>
@azeey
Copy link
Contributor

azeey commented Aug 9, 2023

The windows node was killed after building for 3 hours (https://build.osrfoundation.org/job/ign_gazebo-pr-win/5927/). Going to merge without it. /cc @j-rivero.

@azeey azeey dismissed ahcorde’s stale review August 9, 2023 22:21

Feedback has been addressed.

@azeey azeey merged commit 579013f into gazebosim:gz-sim7 Aug 9, 2023
3 of 4 checks passed
@Henrique-BO Henrique-BO deleted the force_offset branch August 10, 2023 13:20
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 🌱 garden Ignition Garden
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants