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

Volume below a plane #219

Merged
merged 88 commits into from
Sep 22, 2021
Merged

Volume below a plane #219

merged 88 commits into from
Sep 22, 2021

Conversation

arjo129
Copy link
Contributor

@arjo129 arjo129 commented Aug 12, 2021

🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸

🎉 New feature

Adds a VolumeBelow(Plane<T> &_plane) function to some of the shapes. This is useful for simulating surface behavior when the vehicle is buoyant. It also introduces a CenterOfVolumeBelow(const Planed &_plane) which is useful for calculating the center of buoyancy. This is to support gazebosim/gz-sim#818.

Summary

Currently the following shapes have a VolumeBelow:

  • Box
  • Sphere

The sphere calculation is relatively simple formula can be found here.
The box calculation is relatively difficult. However the essence of it is we find out the points at which the plane intersects with the plane itself. We then reconstruct a triangle mesh using the resulting vertices. Subsequently we use the generic triangle mesh volume formula.

TODO:

  • Cylinders
    Cylinder slices are relatively simple to calculate using Cylinder wedges. [To Be done in a Separate PR]

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]>
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]>
@github-actions github-actions bot added 🏢 edifice Ignition Edifice 🏯 fortress Ignition Fortress 🏰 citadel Ignition Citadel 🔮 dome Ignition Dome labels Aug 12, 2021
@codecov
Copy link

codecov bot commented Aug 12, 2021

Codecov Report

Merging #219 (b4f9ddc) into ign-math6 (4af8a25) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@              Coverage Diff              @@
##           ign-math6     #219      +/-   ##
=============================================
+ Coverage      99.40%   99.41%   +0.01%     
=============================================
  Files             66       67       +1     
  Lines           6185     6347     +162     
=============================================
+ Hits            6148     6310     +162     
  Misses            37       37              
Impacted Files Coverage Δ
include/ignition/math/Box.hh 100.00% <ø> (ø)
include/ignition/math/Sphere.hh 100.00% <ø> (ø)
include/ignition/math/Vector3.hh 98.84% <ø> (ø)
include/ignition/math/Line3.hh 100.00% <100.00%> (ø)
include/ignition/math/Plane.hh 100.00% <100.00%> (ø)
include/ignition/math/detail/Box.hh 100.00% <100.00%> (ø)
include/ignition/math/detail/Sphere.hh 100.00% <100.00%> (ø)
include/ignition/math/detail/WellOrderedVector.hh 100.00% <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 4af8a25...b4f9ddc. Read the comment docs.

@chapulina chapulina self-requested a review August 12, 2021 23:12
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]>
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
@arjo129 arjo129 mentioned this pull request Aug 30, 2021
10 tasks
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
@arjo129
Copy link
Contributor Author

arjo129 commented Sep 21, 2021

@scpeters I've addressed your feedback and removed the controversial function.

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]>
Signed-off-by: Arjo Chakravarty <[email protected]>
Copy link
Member

@scpeters scpeters 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 your patience with my review! the API looks good to me. I haven't gone over the test cases, but I think it is good for merging once CI is happy

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 🏰 citadel Ignition Citadel 🔮 dome Ignition Dome 🏢 edifice Ignition Edifice 🏯 fortress Ignition Fortress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants