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

Buoyancy engine #1009

Merged
merged 21 commits into from
Sep 17, 2021
Merged

Buoyancy engine #1009

merged 21 commits into from
Sep 17, 2021

Conversation

arjo129
Copy link
Contributor

@arjo129 arjo129 commented Sep 6, 2021

🎉 New feature

Summary

It is common among many commercial maritime vessels to have some form of a ballast tank to control the buoyancy of the vehicle. Ioiln smaller vehicles this could be an oil/gas filled bladder. In larger vehicles this is usually filled with air. When the vehicle needs to sink the air/gas/oil is removed and replaced with water thus increasing the mass of the vehicle. This plugin was developed for the MBARI LRAUV which has an oil bladder but may be used to model other types of ballast tanks as well.

Test it

TODO

  • docs and better PR explanations
  • Pass code check
  • Write unit tests
  • Create example

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • 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]>
Signed-off-by: Arjo Chakravarty <[email protected]>
@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Sep 6, 2021
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
@chapulina chapulina self-requested a review September 8, 2021 01:40
@chapulina chapulina added the beta Targeting beta release of upcoming collection label Sep 8, 2021
Signed-off-by: Arjo Chakravarty <[email protected]>
@arjo129 arjo129 marked this pull request as ready for review September 14, 2021 10:26
arjo129 and others added 3 commits September 14, 2021 18:28
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[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.

Works nicely!

I see some compiler warnings:

In file included from /home/chapulina/dev_focal/ws_fortress/src/ign-gazebo/src/systems/buoyancy_engine/BuoyancyEngine.cc:29:
/home/chapulina/dev_focal/ws_fortress/src/ign-gazebo/src/systems/buoyancy_engine/BuoyancyEngine.hh:70:3: warning: multi-line comment [-Wcomment]
   70 |   /// ign topic -t  /model/tethys/buoyancy_engine/ -m ignition.msgs.Double \
      |   ^
/home/chapulina/dev_focal/ws_fortress/src/ign-gazebo/src/systems/buoyancy_engine/BuoyancyEngine.hh:75:3: warning: multi-line comment [-Wcomment]
   75 |   /// ign topic -t  /model/tethys/buoyancy_engine/ -m ignition.msgs.Double \
      |   ^
/home/chapulina/dev_focal/ws_fortress/src/ign-gazebo/test/integration/buoyancy_engine.cc: In lambda function:
/home/chapulina/dev_focal/ws_fortress/src/ign-gazebo/test/integration/buoyancy_engine.cc:76:57: warning: unused parameter __info_ [-Wunused-parameter]
   76 |   testSystem.OnPostUpdate([&](const gazebo::UpdateInfo &_info,
      |                               ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~
/home/chapulina/dev_focal/ws_fortress/src/ign-gazebo/test/integration/buoyancy_engine.cc: In lambda function:
/home/chapulina/dev_focal/ws_fortress/src/ign-gazebo/test/integration/buoyancy_engine.cc:123:57: warning: unused parameter __info_ [-Wunused-parameter]
  123 |   testSystem.OnPostUpdate([&](const gazebo::UpdateInfo &_info,
      |   

src/systems/buoyancy_engine/BuoyancyEngine.hh Outdated Show resolved Hide resolved
src/systems/buoyancy_engine/BuoyancyEngine.hh Outdated Show resolved Hide resolved
src/systems/buoyancy_engine/BuoyancyEngine.hh Outdated Show resolved Hide resolved
test/worlds/buoyancy_engine.sdf Outdated Show resolved Hide resolved
test/integration/buoyancy_engine.cc Outdated Show resolved Hide resolved
src/systems/buoyancy_engine/BuoyancyEngine.cc Show resolved Hide resolved
src/systems/buoyancy_engine/BuoyancyEngine.hh Outdated Show resolved Hide resolved
src/systems/buoyancy_engine/BuoyancyEngine.cc Outdated Show resolved Hide resolved
src/systems/buoyancy_engine/BuoyancyEngine.cc Outdated Show resolved Hide resolved
src/systems/buoyancy_engine/BuoyancyEngine.hh Show resolved Hide resolved
arjo129 and others added 3 commits September 15, 2021 14:13
Signed-off-by: Arjo Chakravarty <[email protected]>
Co-authored-by: Louise Poubel <[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]>
@codecov
Copy link

codecov bot commented Sep 15, 2021

Codecov Report

Merging #1009 (c4df58b) into main (7ffcf4c) will increase coverage by 0.16%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1009      +/-   ##
==========================================
+ Coverage   63.30%   63.46%   +0.16%     
==========================================
  Files         239      240       +1     
  Lines       19464    19548      +84     
==========================================
+ Hits        12322    12407      +85     
+ Misses       7142     7141       -1     
Impacted Files Coverage Δ
src/systems/buoyancy_engine/BuoyancyEngine.cc 85.71% <85.71%> (ø)
...int_position_controller/JointPositionController.cc 60.57% <0.00%> (+7.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 7ffcf4c...c4df58b. Read the comment docs.

@arjo129
Copy link
Contributor Author

arjo129 commented Sep 15, 2021

@chapulina this is ready for round 2.

With regards to #818 I have given my opinion here: #1009 (comment)

Theoretically, this will not work well with it without some modification. However, in practice if the ballast is significantly smaller than the vehicle itself it should be fine. We can place a warning label in the documentation for now. However, as I mentioned, we probably want some way of reading loaded world fluid density properties to reduce duplication of configuration.

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.

Works great! I pushed some small tweaks directly onto this branch in c4df58b. @arjo129 I'll wait for your ok on those changes before merging.

@arjo129
Copy link
Contributor Author

arjo129 commented Sep 17, 2021

Looks Good To Me!

@chapulina chapulina merged commit c992f31 into main Sep 17, 2021
@chapulina chapulina deleted the arjo/buoyancy_engine branch September 17, 2021 16:06
WilliamLewww pushed a commit to WilliamLewww/ign-gazebo that referenced this pull request Dec 7, 2021
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>

Co-authored-by: Louise Poubel <[email protected]>
Signed-off-by: William Lew <[email protected]>
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 🏯 fortress Ignition Fortress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants