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

Fix graded buoyancy problems #1297

Merged
merged 15 commits into from
Jan 20, 2022
Merged

Fix graded buoyancy problems #1297

merged 15 commits into from
Jan 20, 2022

Conversation

arjo129
Copy link
Contributor

@arjo129 arjo129 commented Jan 18, 2022

🦟 Bug fix

Fixes #1296 and #1210

Summary

The primary target of this PR is #1296. This PR changes an incorrect sign line 224 of the buoyancy plugin when it was running in graded buoyancy mode. Essentially when running the example in #1296 now both graded and uniform mode are unstable. This is the correct behavior. For a stable example see the added tests in this PR. In particular both SDFs should be stable and have small oscillations, but previously the graded buoyancy example would have kept on spinning.

Additionally #1210 would mean the graded buoyancy test would keep sinking. This PR removes the need for checking the newPose as we nolonger need the inertial pose. The reason I was initially using the inertialpose was because I had thought that Link::AddWorldWrench applies a force at the center of mass, but in reality the original behaviour in uniform buoyancy was correct as Link::AddWorldWrench applies a force at the link itself.

TL;DR

Before times when running https://github.com/ignitionrobotics/ign-gazebo/blob/1d461b849b92104299bc03879bb67db368fe10c9/test/worlds/buoyancy_graded_restoring_moments.sdf :

buoyancy_before_times

After times:
buoyancy_after_times

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • 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: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
@arjo129 arjo129 requested a review from chapulina as a code owner January 18, 2022 06:25
@arjo129 arjo129 self-assigned this Jan 18, 2022
@arjo129 arjo129 linked an issue Jan 18, 2022 that may be closed by this pull request
@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Jan 18, 2022
Signed-off-by: Arjo Chakravarty <[email protected]>
@arjo129 arjo129 changed the title Arjo/fix/graded buoyancy Fix graded buoyancy problems Jan 18, 2022
@codecov
Copy link

codecov bot commented Jan 18, 2022

Codecov Report

Merging #1297 (987eca7) into ign-gazebo6 (05b9232) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@               Coverage Diff               @@
##           ign-gazebo6    #1297      +/-   ##
===============================================
- Coverage        62.25%   62.25%   -0.01%     
===============================================
  Files              278      278              
  Lines            23208    23203       -5     
===============================================
- Hits             14449    14444       -5     
  Misses            8759     8759              
Impacted Files Coverage Δ
src/systems/buoyancy/Buoyancy.hh 100.00% <ø> (ø)
src/systems/buoyancy/Buoyancy.cc 82.60% <100.00%> (-0.42%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 05b9232...987eca7. Read the comment docs.

@arjo129 arjo129 added the bug Something isn't working label Jan 18, 2022
Signed-off-by: Louise Poubel <[email protected]>
@chapulina
Copy link
Contributor

I think there may still be some offset calculations that are incorrect, see

Signed-off-by: Louise Poubel <[email protected]>
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Besides #1301, I just have some more minor comments here.

test/integration/buoyancy.cc Outdated Show resolved Hide resolved
test/integration/buoyancy.cc Outdated Show resolved Hide resolved
test/integration/buoyancy.cc Outdated Show resolved Hide resolved
test/integration/buoyancy.cc Show resolved Hide resolved
test/integration/buoyancy.cc Show resolved Hide resolved
test/worlds/buoyancy_graded_restoring_moments.sdf Outdated Show resolved Hide resolved
test/worlds/buoyancy_graded_restoring_moments.sdf Outdated Show resolved Hide resolved
…ded_buoyancy

Documentation and more examples for #1297
@caguero caguero mentioned this pull request Jan 19, 2022
39 tasks
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
@arjo129 arjo129 requested a review from chapulina January 20, 2022 06:04
@chapulina
Copy link
Contributor

I pushed d013573 to disable the buoyancy engine test that was introduced on #1298 on Windows. It's not directly related to this PR, but it's also "graded buoyancy", so I'm taking advantage of the CI run here.

Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

LGTM with clean CI. I think we should eventually look deeper into why the 2 servers on the test are giving slightly different results though.

@chapulina chapulina merged commit 9dee519 into ign-gazebo6 Jan 20, 2022
@chapulina chapulina deleted the arjo/fix/graded_buoyancy branch January 20, 2022 19:43
arjo129 added a commit to osrf/lrauv that referenced this pull request Jan 24, 2022
This should address #38.  It requires the following upstream fixes and relevant forward ports:

* gazebosim/gz-sim#1297
* gazebosim/gz-sim#1298

⚠️ Do not merge till the CI goes from ❌ to :heavy_checkmark:

Signed-off-by: Arjo Chakravarty <[email protected]>
@osrf-triage
Copy link

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-01-citadel-edifice-fortress/1313/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working 🏯 fortress Ignition Fortress
Projects
None yet
3 participants