Skip to content

Add Capsule class with inertia calculation method#163

Merged
scpeters merged 18 commits intogazebosim:ign-math6from
scpeters:capsule
Nov 7, 2020
Merged

Add Capsule class with inertia calculation method#163
scpeters merged 18 commits intogazebosim:ign-math6from
scpeters:capsule

Conversation

@scpeters
Copy link
Copy Markdown
Member

@scpeters scpeters commented Oct 13, 2020 •
edited
Loading

This package has classes for several shapes (Box, Cylinder, Sphere) as well as methods in MassMatrix3 for computing inertial properties of these shapes assuming uniform density. This pull request adds a new Capsule shape class that is like a Cylinder with hemispheres capping each end. Methods for computing the inertial properties of a Capsule are added to MassMatrix3 and validated in unit tests by comparing to the formulae used by Open Dynamics Engine.

Update: I've removed the MassMatrix3 methods and removed the Quaternion parameter from the Capsule class.

@scpeters
Cylinder.hh, MassMatrix3.hh: fix typos
3ba03fc 
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@scpeters
MassMatrix3: add SetFromCapsuleZ methods
6617e81 
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@scpeters
Add Capsule class based on Cylinder
4e1c428 
Copy the Cylinder class and unit test to create Capsule,
which is similar to the Cylinder but with hemispheres
capping either end of the cylinder.

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@scpeters
bump to 6.7.0~pre1
bcb199d 
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@scpeters scpeters mentioned this pull request Oct 13, 2020
Merged
@chapulina chapulina added enhancement New feature or request 🏢 edifice Ignition Edifice 🏰 citadel Ignition Citadel 📜 blueprint Ignition Blueprint 🔮 dome Ignition Dome labels Oct 13, 2020
EricCousineau-TRI
EricCousineau-TRI approved these changes Oct 14, 2020
View reviewed changes
Copy link
Copy Markdown

@EricCousineau-TRI EricCousineau-TRI left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good at a high-level. The API here seems to be highly redundant and coupled (e.g. shapes + offsets, inertia computations, etc.), but understandably it'd be hard to refactor any time soon.

Made some picky comments, but nothing blocking.

Is there also a high-level issue that this is aiming towards?
e.g. gazebosim/sdformat#176 gazebosim/sdformat#376

include/ignition/math/Capsule.hh Outdated
/// equivalent to a cylinder with capped with hemispheres. Radius and
/// length are in meters. See Material for more on material properties.
/// By default, a capsule's length is aligned with the Z axis. The
/// rotational offset encodes a rotation from the z axis.
Copy link
Copy Markdown

@EricCousineau-TRI EricCousineau-TRI Oct 14, 2020

Choose a reason for hiding this comment

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

It would be good to also state that the radius defines the circle when the capsule intersects with G's XY-plane.

@EricCousineau-TRI
Copy link
Copy Markdown

EricCousineau-TRI commented Oct 14, 2020

Also, the shapes defined in ign-math seem like they have a shared contract, but there doesn't seem to be any form of contract enforcement (e.g. dynamic inheritance, static inheritance / mixins with CRTP, etc.).

Am I missing something there?

@scpeters
Copy link
Copy Markdown
Member Author

scpeters commented Oct 16, 2020

Is there also a high-level issue that this is aiming towards?
e.g. osrf/sdformat#176 osrf/sdformat#376

it's in support of gazebosim/sdformat#376, along with gazebosim/sdformat#389

@scpeters
Copy link
Copy Markdown
Member Author

scpeters commented Oct 17, 2020

The API here seems to be highly redundant and coupled (e.g. shapes + offsets, inertia computations, etc.), but understandably it'd be hard to refactor any time soon.

It took me a little while to remember, but I remembered a bit of the original intent behind this design. We added the MassMatrix3 class (started in 2.3.0 and a bit more in 2.4.0) before adding the shape classes like Sphere and Cylinder (5.0.0). The MassMatrix3 class is intended to represent a scalar mass and a 3x3 moment of inertia matrix. If you diagonalize the inertia matrix to get the principal moments of inertia and the orientation of its principal axes, then you can use these values along with its mass to compute the dimensions and orientation of a box of uniform density that has equivalent inertial properties (see MassMatrix3::EquivalentBox from 2.4.0 for reference).

That EquivalentBox is used to visualize the moment of inertia in Gazebo. After adding EquivalentBox, we added its inverse SetFromBox as well as SetFromSphere and SetFromCylinderZ for good measure (we also use these in gazebo). The quaternion parameter was added to SetFromBox for symmetry with EquivalentBox and to SetFromCylinderZ to accommodate cylinders with different axis alignments.

When the shape functions were added in 5.0.0, they included a Quaternion parameter in the constructor, and in retrospect, I think they might be confusing things more than helping. I'll remove them from the Capsule constructors; they'll still be in the other shape classes, but we'll need a major version bump to get rid of those

I'll think further on whether we need MassMatrix3::SetFromCapsuleZ functions or not; if not I'll just put the math in the Capsule class

@scpeters
Remove Quaternion from Capsule class
4de985e 
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@scpeters
Compute Capsule inertia by adding Inertials
0ac1488 
This is more elegant than the MassMatrix3::SetFromCapsuleZ
helper functions.

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@scpeters
Remove MassMatrix3::SetFromCapsuleZ helpers
d9c8d73 
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@scpeters
Copy link
Copy Markdown
Member Author

scpeters commented Oct 17, 2020

I'll think further on whether we need MassMatrix3::SetFromCapsuleZ functions or not; if not I'll just put the math in the Capsule class

I thought about it, and we can use the Inertial class directly from Capsule, which we can't do from MassMatrix3, so I've done it in 0ac1488, and it's a bit more elegant

I've removed the MassMatrix3::SetFromCapsuleZ helper functions in d9c8d73

@scpeters scpeters changed the title Add Capsule class and MassMatrix3 methods Add Capsule class with inertia calculation method Oct 19, 2020
@chapulina chapulina self-requested a review October 26, 2020 18:40
chapulina
chapulina approved these changes Oct 31, 2020
View reviewed changes
Copy link
Copy Markdown
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.

Looks good to me, I just have a question about the hemispheres, but I'm trusting the test that the math checks out.

@scpeters
Copy link
Copy Markdown
Member Author

scpeters commented Oct 31, 2020

Also, the shapes defined in ign-math seem like they have a shared contract, but there doesn't seem to be any form of contract enforcement (e.g. dynamic inheritance, static inheritance / mixins with CRTP, etc.).

Am I missing something there?

no I don't think you're missing anything. I think the shape objects have similar API's but it's not enforced

@EricCousineau-TRI
Copy link
Copy Markdown

EricCousineau-TRI commented Nov 4, 2020

Aye, sounds good. Minor concern is that the Capsule API may now deviates from others (given that it computes its own inertia, rather than kinda doing some psuedo static-double-dispatch things).

azeey
azeey reviewed Nov 4, 2020
View reviewed changes
azeey
azeey reviewed Nov 4, 2020
View reviewed changes
include/ignition/math/Capsule.hh
{
/// \brief Default constructor. The default radius and length are both
/// zero.
public: Capsule() = default;
Copy link
Copy Markdown
Contributor

@azeey azeey Nov 4, 2020

Choose a reason for hiding this comment

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

What do you think about not providing a default constructor? That way the only time we would have invalid Capsule objects would be if the input length and radius values are invalid.

Copy link
Copy Markdown
Member Author

@scpeters scpeters Nov 6, 2020

Choose a reason for hiding this comment

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

I like having the default constructor. If you want to create a shape without remembering the order of the arguments in the constructor:

Capsuled capsule;
capsule.SetLength(length);
capsule.SetRadius(radius);

if other people don't like it, we can remove it, but I'm inclined to keep it

Copy link
Copy Markdown
Contributor

@mxgrey mxgrey Nov 9, 2020

Choose a reason for hiding this comment

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

If I put on my pedantry hat, the "right" way to do what @scpeters is suggesting would be to have something like a Capsule::Builder class that lets you fill in the information necessary to build a Capsule object, and then you could call build() on it (or something similar) and it constructs a capsule for you, perhaps throwing an exception if the parameters are invalid (e.g. radius <= 0, length <= 0).

Some would (fairly) argue that's overkill for this use case, but the more I've had to deal with writing production-quality code, the more I've been feeling it's worth the effort to make every assumption explicit inside the code itself.

@scpeters
Capsule.hh: respond to review comments
f0419f1 
* Remove destructor
* Remove duplicate SetLength method
* Add tparam doc-string
* Fix comment about parameter that has been removed

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@scpeters
Merge branch 'ign-math6' into capsule
ac1e990
@scpeters
Return NaN if density is invalid instead of -1
2628fab 
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@scpeters
Return std::optional<MassMatrix3> instead of bool
d379a5b 
With the prior API that passes a reference to a MassMatrix3,
it is unclear whether the object is mutated when the parameters
are invalid. Using std::optional removes the ambiguity and
ensures that invalid values are not returned.

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@scpeters
cpplint.py: allow c++14/17 headers
f02ea70 
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@scpeters
Add link to gamedev.net article on capsule inertia
7f729a2 
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@scpeters scpeters merged commit b084f66 into gazebosim:ign-math6 Nov 7, 2020
@scpeters scpeters deleted the capsule branch November 7, 2020 01:37
@brawner brawner mentioned this pull request Dec 11, 2020
Merged
@chapulina chapulina mentioned this pull request Dec 17, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mxgrey mxgrey mxgrey left review comments

@azeey azeey azeey approved these changes

+2 more reviewers

@EricCousineau-TRI EricCousineau-TRI EricCousineau-TRI approved these changes

@chapulina chapulina chapulina approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

📜 blueprint Ignition Blueprint 🏰 citadel Ignition Citadel 🔮 dome Ignition Dome 🏢 edifice Ignition Edifice enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@scpeters @EricCousineau-TRI @azeey @mxgrey @chapulina @j-rivero