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: fix center of volume's reference frame #1302

Merged
merged 3 commits into from
Jan 21, 2022

Conversation

chapulina
Copy link
Contributor

🦟 Bug fix

Summary

While working with a model that uses NED / FSK orientation, I noticed that the center of volume calculation on the buoyancy plugin was using the wrong reference frame. According to the documentation and the usage, the reference frame is expected to be the parent link. But the value provided was being calculated in the world frame. That's evident when using a model that's oriented with Z down.

Test it

I added a test world with 4 models that are supposed to be neutrally buoyant. Without this fix, the model that has a CoV offset and starts with a non-zero orientation isn't stable:

cov_1

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: Louise Poubel <louise@openrobotics.org>
@chapulina chapulina added the bug Something isn't working label Jan 20, 2022
@github-actions github-actions bot added the 🏰 citadel Ignition Citadel label Jan 20, 2022
@osrf-triage osrf-triage added this to Inbox in Core development Jan 20, 2022
@chapulina chapulina moved this from Inbox to In review in Core development Jan 20, 2022
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@codecov
Copy link

codecov bot commented Jan 20, 2022

Codecov Report

Merging #1302 (0bb551d) into ign-gazebo3 (6f58c91) will decrease coverage by 0.08%.
The diff coverage is 71.01%.

❗ Current head 0bb551d differs from pull request most recent head 134056a. Consider uploading reports for the commit 134056a to get more accurate results
Impacted file tree graph

@@               Coverage Diff               @@
##           ign-gazebo3    #1302      +/-   ##
===============================================
- Coverage        77.25%   77.16%   -0.09%     
===============================================
  Files              227      227              
  Lines            13535    13574      +39     
===============================================
+ Hits             10457    10475      +18     
- Misses            3078     3099      +21     
Impacted Files Coverage Δ
...clude/ignition/gazebo/components/CenterOfVolume.hh 100.00% <ø> (ø)
src/systems/air_pressure/AirPressure.cc 72.83% <69.23%> (-1.14%) ⬇️
src/systems/altimeter/Altimeter.cc 73.49% <69.23%> (-1.18%) ⬇️
src/systems/logical_camera/LogicalCamera.cc 75.00% <69.23%> (-1.25%) ⬇️
src/systems/magnetometer/Magnetometer.cc 70.78% <69.23%> (-0.82%) ⬇️
src/systems/imu/Imu.cc 71.42% <71.42%> (-0.87%) ⬇️
src/systems/buoyancy/Buoyancy.cc 75.00% <100.00%> (-0.29%) ⬇️
src/SimulationRunner.cc 93.45% <0.00%> (-1.07%) ⬇️

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 650b746...134056a. Read the comment docs.

Copy link
Contributor

@arjo129 arjo129 left a comment

Choose a reason for hiding this comment

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

Thanks for catching this. Tested before and after the fix and can confirm this fix indeed does solve the problem at hand. Same problem affects graded buoyancy ☠️ I'll go take alook and fix it.

@chapulina chapulina merged commit 749884d into ign-gazebo3 Jan 21, 2022
@chapulina chapulina deleted the chapulina/3/cov branch January 21, 2022 17:10
Core development automation moved this from In review to Done Jan 21, 2022
@iche033 iche033 mentioned this pull request Feb 4, 2022
@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

@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-04-13-fortress-edifice/1367/1

@j-rivero j-rivero removed this from Done in Core development May 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working 🏰 citadel Ignition Citadel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants