Make inertial warnings optional when setting tensor

[chapulina]

• Part of Hydrodynamic added mass gazebosim/gz-sim#1462
• Used by Support fluid added mass gazebosim/gz-physics#384

---

We're adding support for fluid added mass to Gazebo, see the Fluid Added Mass Proposal. This feature makes use of values in a body's spatial tensor which break the current assumptions in the inertia class. Since this is a valid use case for us, we'd like to suppress those warnings.

So far I haven't noticed any other places where DART misbehaves when passing those values to the spatial tensor. As an example, without this PR I see warnings like this:

Warning [Inertia.cpp:287] [Inertia::verifySpatialTensor] Invalid entry for (3,3): 0.261666. Value should be exactly zero.
Warning [Inertia.cpp:296] [Inertia::verifySpatialTensor] Invalid entry for (4,3): 0.261666. Value should be exactly zero.
Warning [Inertia.cpp:287] [Inertia::verifySpatialTensor] Invalid entry for (3,3): 0.261666. Value should be exactly zero.
Warning [Inertia.cpp:296] [Inertia::verifySpatialTensor] Invalid entry for (5,3): 0.261666. Value should be exactly zero.
Warning [Inertia.cpp:185] [Inertia::setSpatialTensor] Passing in an invalid spatial inertia tensor. Results might not be physically accurate or meaningful.
Warning [Inertia.cpp:287] [Inertia::verifySpatialTensor] Invalid entry for (3,3): 0.261666. Value should be exactly zero.
Warning [Inertia.cpp:296] [Inertia::verifySpatialTensor] Invalid entry for (4,3): 0.261666. Value should be exactly zero.
Warning [Inertia.cpp:185] [Inertia::setSpatialTensor] Passing in an invalid spatial inertia tensor. Results might not be physically accurate or meaningful.

I added the option to suppress the warnings in a way that doesn't break ABI, so that this change can be included in future releases in a backwards-compatible manner. I considered skipping the verification completely when !_printWarnings, please let me know if you'd like me to do that instead.

I'm not sure about DART's backporting policy, but it would be great to have this change be released in a 6.X release. Please let me know if this is the correct branch to target this change.

Before creating a pull request

• Document new methods and classes
• Format new code files using ClangFormat by running make format
• Build with -DDART_TREAT_WARNINGS_AS_ERRORS=ON and resolve all the compile warnings

Before merging a pull request

• Set version target by selecting a milestone on the right side
• Summarize this change in CHANGELOG.md
• Add unit test(s) for this change
• Add Python bindings for new methods and classes

[jslee02]

Looking at the proposal, it seems there is an difference in the convention of the inertia between the proposal and DART. Suppose we denote M in the proposal as M = [A, B; B^T C] where A = mI (so I assume the off-diagonal of A still should be zero), then the inertia tensor in DART is actually M' = [C B^T; B A]. This is why DART complains that the off-diagonal terms of A is not zero. (I didn't look at the Gazebo code yet, so I could be wrong here though)

I think we could safely replace void setSpatialTensor(const Eigen::Matrix6d& _spatial) with void setSpatialTensor(const Eigen::Matrix6d& _spatial, bool _printWarnings = true) without having to add an additional function, while keeping the backward compatibility (by the defaulting _printWarnings to true).

Regarding the target branch, master branch is the right one as it's aimed for 6.13.

[chapulina]

the off-diagonal of A still should be zero

Note that what we pass to DART is the sum of matrices $\bf{M}$ (the body mass matrix) and $\bf{\mu}$ (the added mass matrix), and $\bf{\mu}$ may hold any non-zero off-diagonal terms.

it seems there is an difference in the convention of the inertia between the proposal and DART.

The convention on Gazebo is indeed different from DART / Featherstone though, because Gazebo's translational terms come on the top. That's being taken into account before feeding the values to DART here:

https://github.com/gazebosim/gz-physics/pull/384/files

we could safely replace void setSpatialTensor(const Eigen::Matrix6d& _spatial) with void setSpatialTensor(const Eigen::Matrix6d& _spatial, bool _printWarnings = true) without having to add an additional function, while keeping the backward compatibility

That would be safe from an API standpoint, but it would break ABI. So any downstream projects that have been built against DART <= 6.12 would have esoteric runtime issues when running against 6.13. That includes Gazebo. I've seen other PRs to this repository trying to avoid ABI breaks in the past, but I can make the suggested change if ABI is not a concern.

[jslee02]

All sounds good to me! DART hasn't kept the ABI backward compatibilities for minor version ups (but just API) historically, but it wouldn't harm anything from keeping it when it's possible I think.