Include ellipsoid as a math shape #182
Conversation
Signed-off-by: Stephen Brawner <[email protected]>
github-actions
bot
added
🏢 edifice
Codecov Report
@@ Coverage Diff @@
## ign-math6 #182 +/- ##
==========================================
Coverage 99.19% 99.20%
==========================================
Files 61 63 +2
Lines 5982 6024 +42
==========================================
+ Hits 5934 5976 +42
Misses 48 48
Continue to review full report at Codecov.
|
Signed-off-by: Stephen Brawner <[email protected]>
| template<typename T> | ||
| std::optional< MassMatrix3<T> > Ellipsoid<T>::MassMatrix() const | ||
| { | ||
| if (this->radii.X() < 0 || this->radii.Y() < 0 || this->radii.Z() < 0) |
There was a problem hiding this comment.
I think we want to use <= for these comparisons, since the inertial properties are invalid if any radii are 0
There was a problem hiding this comment.
Completely reasonable and fixed, but then that begs another question. If 0 and negative values are invalid, should they be checked during construction/SetRadii()? And an exception thrown in that case?
There was a problem hiding this comment.
If 0 and negative values are invalid, should they be checked during construction/
SetRadii()? And an exception thrown in that case?
Currently the constructors and Get/Set methods act like plain old data structures, and there's only validity checking in the methods dealing with inertia computations. I'm inclined to keep it that way for ign-math6 so that the shape classes have the same exception behavior. I think we could consider that for ignition-math7, but it would be good to consider the impact it would have on downstream code in libsdformat, ign-gazebo, etc.
Signed-off-by: Stephen Brawner <[email protected]>
Signed-off-by: Stephen Brawner <[email protected]>
|
Forgot to update a unit test for the stricter check that was added. |
Signed-off-by: Stephen Brawner <[email protected]>
brawner
force-pushed
the
brawner/ellipsoid-simple-shape
branch
from
396f673
to
941ea07
Compare
|
@scpeters Is there anything else you would like to see from this PR? |
include/ignition/math/Ellipsoid.hh
Outdated
| namespace math | ||
| { | ||
| // Foward declarations | ||
| class EllipsoidPrivate; |
There was a problem hiding this comment.
Did you mean to create this class? I see the same thing sneaked into the Capsule class.
There was a problem hiding this comment.
I'll take the liberty of removing this so it can rerun the windows job
There was a problem hiding this comment.
there is no CapsulePrivate, so that forward declaration could be removed as well
There was a problem hiding this comment.
Thanks @scpeters. We can blame my gratuitous use of copy/paste.
Signed-off-by: Steve Peters <[email protected]>
|
Thanks again @scpeters for the review and quick fix. Looks like all checks now pass. |
This follows #163 pretty closely, but I'm happy to make changes where needed for this own class.
I noticed that the API for different shapes has diverged, and it may also be worth revisiting how bad parameters are dealt with across the different shapes.
Signed-off-by: Stephen Brawner [email protected]