Skip to content

Volume below a plane#219

Merged
chapulina merged 88 commits intoign-math6from
arjo/volume_below
Sep 22, 2021
Merged

Volume below a plane#219
chapulina merged 88 commits intoign-math6from
arjo/volume_below

Conversation

@arjo129
Copy link
Copy Markdown
Contributor

@arjo129 arjo129 commented Aug 12, 2021 •
edited
Loading

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

🎉 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

arjo129 added 9 commits August 3, 2021 14:44
@arjo129
Add volume below primitives
56b214e 
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
@arjo129
fix tests
008aa1b 
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
@arjo129
nearly there for boxes
0e40a5f 
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
@arjo129
Fix boxes
c18b735 
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
@arjo129
use convex hull algo
40fa5b3 
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
@arjo129
Add overlap method
e275ff8 
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
@arjo129
Finally Box::VolumeBelow() works 🎉
dfec15c 
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
@arjo129
signed volume
13f0ade 
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
@arjo129
codecheck
5cfe42c 
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
@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
Copy Markdown

codecov Bot commented Aug 12, 2021 •
edited
Loading

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
arjo129 added 9 commits August 14, 2021 17:21
@arjo129
Add center of volume recalculation
d82a6ba 
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
@arjo129
Add center of volume functions
27287dc 
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
@arjo129
codecheck
f241019 
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
@arjo129
Trying to get windows CI to pass
61258cd 
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
@arjo129
Add preliminary cylinder logic.
69af275 
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
@arjo129
Add plane transformation
3095359 
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
@arjo129
fix function return value
37b3842 
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
@arjo129
fix types
bbed807 
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
@arjo129
Preliminary work for cylinders
c6c7e00 
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
@arjo129 arjo129 mentioned this pull request Aug 30, 2021
Merged
10 tasks
arjo129 added 2 commits August 30, 2021 12:50
@arjo129
remove unessecary include
278dc07 
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
@arjo129
remove more useless logging
f4e70c7 
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
@arjo129 arjo129 force-pushed the arjo/volume_below branch from 19e5f67 to f4e70c7 Compare August 30, 2021 04:50
arjo129 added 2 commits August 30, 2021 13:00
@arjo129
remove unused transform function.
11180ee 
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
@arjo129
style fixes
6521336 
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
scpeters
scpeters reviewed Sep 21, 2021
View reviewed changes
Comment thread include/ignition/math/detail/Box.hh Outdated
Comment thread include/ignition/math/detail/Box.hh Outdated
Comment thread include/ignition/math/Vector3.hh Outdated
Comment thread include/ignition/math/Line3.hh Outdated
@arjo129
Address @SCPETER 's feedback
1492375 
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
@arjo129
Address @scpeters feedback about Line3
72d7779 
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
@arjo129 arjo129 requested a review from scpeters September 21, 2021 22:00
@arjo129
Copy link
Copy Markdown
Contributor Author

arjo129 commented Sep 21, 2021

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

chapulina
chapulina reviewed Sep 21, 2021
View reviewed changes
Comment thread include/ignition/math/Plane.hh
chapulina
chapulina reviewed Sep 21, 2021
View reviewed changes
Comment thread include/ignition/math/Line3.hh Outdated
@arjo129
Reverting, because the dividee by zero will never occur
e356258 
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
scpeters
scpeters reviewed Sep 21, 2021
View reviewed changes
Comment thread include/ignition/math/Line3.hh Outdated
scpeters
scpeters reviewed Sep 21, 2021
View reviewed changes
Comment thread src/Line3_TEST.cc
@arjo129
Added comment
b06b483 
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
@arjo129
Add test cases and assert
91a6b43 
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
@arjo129
Whelps compile error.
f547e71 
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
scpeters
scpeters reviewed Sep 21, 2021
View reviewed changes
Comment thread include/ignition/math/detail/Box.hh Outdated
scpeters
scpeters reviewed Sep 21, 2021
View reviewed changes
Comment thread include/ignition/math/detail/Box.hh
scpeters
scpeters reviewed Sep 21, 2021
View reviewed changes
Comment thread include/ignition/math/detail/Box.hh Outdated
Comment thread include/ignition/math/detail/Box.hh
scpeters
scpeters reviewed Sep 21, 2021
View reviewed changes
Comment thread include/ignition/math/detail/Box.hh
@arjo129
Add docs
69b0448 
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
@arjo129
whitespaces
bacd9f9 
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
scpeters
scpeters reviewed Sep 21, 2021
View reviewed changes
Comment thread include/ignition/math/detail/Box.hh Outdated
scpeters
scpeters reviewed Sep 21, 2021
View reviewed changes
Comment thread include/ignition/math/detail/Box.hh Outdated
@arjo129
More docs fixes
4c0bd02 
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
@arjo129
update scalar value
b4f9ddc 
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
scpeters
scpeters reviewed Sep 21, 2021
View reviewed changes
Comment thread include/ignition/math/detail/Box.hh Outdated
scpeters
scpeters approved these changes Sep 21, 2021
View reviewed changes
Copy link
Copy Markdown
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

@chapulina chapulina enabled auto-merge (squash) September 21, 2021 23:52
@chapulina chapulina merged commit 274378c into ign-math6 Sep 22, 2021
@chapulina chapulina deleted the arjo/volume_below branch September 22, 2021 00:01
@caguero caguero mentioned this pull request Nov 2, 2021
Closed
40 tasks
@chapulina chapulina mentioned this pull request Mar 14, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@scpeters scpeters scpeters approved these changes

+1 more reviewer

@chapulina chapulina chapulina approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

beta Targeting beta release of upcoming collection 🏰 citadel Ignition Citadel 🔮 dome Ignition Dome 🏢 edifice Ignition Edifice 🏯 fortress Ignition Fortress

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@arjo129 @chapulina @scpeters @j-rivero @osrf-triage @LolaSegura