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 buoyancy not being applied for one iteration #1211

Merged
merged 5 commits into from
Nov 30, 2021

Conversation

arjo129
Copy link
Contributor

@arjo129 arjo129 commented Nov 16, 2021

Signed-off-by: Arjo Chakravarty [email protected]

🦟 Bug fix

Workaround #1210

Summary

This PR currently is a work around, Essentially, it skips the check introduced in #1064 for the uniform buoyancy. This is fine as #1064 is not really position dependent. We should however leave #1210 open as no fix has been applied for layered buoyancy.

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

Signed-off-by: Arjo Chakravarty <[email protected]>
@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Nov 16, 2021
@chapulina chapulina linked an issue Nov 16, 2021 that may be closed by this pull request
@chapulina chapulina self-requested a review November 16, 2021 16:41
@arjo129 arjo129 marked this pull request as ready for review November 24, 2021 08:01
@codecov
Copy link

codecov bot commented Nov 24, 2021

Codecov Report

Merging #1211 (9091a13) into ign-gazebo6 (525572f) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 9091a13 differs from pull request most recent head 4d9c500. Consider uploading reports for the commit 4d9c500 to get more accurate results
Impacted file tree graph

@@             Coverage Diff              @@
##           ign-gazebo6    #1211   +/-   ##
============================================
  Coverage        61.92%   61.92%           
============================================
  Files              275      275           
  Lines            22457    22457           
============================================
  Hits             13906    13906           
  Misses            8551     8551           
Impacted Files Coverage Δ
src/systems/buoyancy/Buoyancy.cc 83.01% <100.00%> (ø)

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 525572f...4d9c500. Read the comment docs.

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.

Nice catch! This fix works for me. I have a suggestion below for a more permanent fix.

{
// Skip entity if WorldPose and inertial are not yet ready
// TODO(arjo): Find a way of disabling gravity effects for
// this first iteration.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we may be able to avoid this skip if we use gazebo::worldPose instead of the WorldPose component. That function should provide the world pose straight away.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, but we need to use link::WorldInertialPose...

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh I see, maybe the implementation of that method can be improved 🤔

@chapulina chapulina changed the title Address #1210 Fix buoyancy not being applied for one iteration Nov 30, 2021
@arjo129 arjo129 merged commit 3ae7098 into ign-gazebo6 Nov 30, 2021
@arjo129 arjo129 deleted the arjo/fix/uniform_buoyancy branch November 30, 2021 05:51
@chapulina
Copy link
Contributor

Ouch, we should have squash-merged this. No need to go back and fix it, but something to keep in mind for next time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏯 fortress Ignition Fortress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression in Buoyancy Example, neutral vehicle slowly sinks
2 participants