Remove SpatialInertia::MakeTestCube() in favor of SpatialInertia::MakeUnitary().

[mitiguy]

This change is 

[mitiguy]

Feature review +@sherm1

Reviewable status: LGTM missing from assignee sherm1(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @sherm1)

[mitiguy]

Note: Ignore commit history as I branched from an existing branch that may (or may not) be subsequently abandoned.

Reviewable status: LGTM missing from assignee sherm1(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @sherm1)

[mitiguy]

Feature review -@sherm1
Feature review +@xuchenhan-tri

Reviewable status: LGTM missing from assignee xuchenhan-tri(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @xuchenhan-tri)

[xuchenhan-tri]

Feature review done. PTAL.

Reviewed 18 of 18 files at r1.
Reviewable status: 10 unresolved discussions, LGTM missing from assignee xuchenhan-tri(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @mitiguy)

a discussion (no related file):
This PR also introduces new constructors for UnitInertia and RotationInertia, but none of our existing tests/examples seems to use it. That leads to my question:

a. are they added as new sugars and part of the new features? or
b. are they added in support of the new constructor for SpatialInertia?

If a, then they should also show up in the PR notes. If b, then it seems like you don't need them and can use existing constructors.

---

---

multibody/plant/test/compliant_contact_manager_test.cc line 1676 at r1 (raw file):

  // To avoid unnecessary warnings/errors, use a non-zero spatial inertia.
  const RigidBody<double>& body = plant.AddRigidBody("DummyBody",
      SpatialInertia<double>(InertiaValue::kSdformat));

nit, from all the test cases in the PR, it seems like InertiaValue::kSdformat is a better default as it is used almost everywhere whereas kNaN only shows up in a couple places.

---

multibody/tree/rotational_inertia.h line 31 at r1 (raw file):

/// filled with NaNs or construct inertias that are consistent with the default
/// values specified in the SDFormat <inertial> tag, found at:
/// http://sdformat.org/spec?elem=link

BTW, would this be a better link

Suggestion:

http://sdformat.org/spec?elem=link#inertial_inertia

---

multibody/tree/rotational_inertia.h line 32 at r1 (raw file):

/// values specified in the SDFormat <inertial> tag, found at:
/// http://sdformat.org/spec?elem=link
enum class InertiaValue {kNaN, kSdformat};

Why don't we call it kIdentitiy instead? It's nicely parallel with kNaN, more intuitive, and we don't have to worry here in this class about the convention of Sdformat and whether it would change over time. It will be also more readable at callsites. Besides, it just seems like an odd fit to have Sdformat (parsing related) show up in multibody/tree.

---

multibody/tree/rotational_inertia.h line 175 at r1 (raw file):

  DRAKE_DEFAULT_COPY_AND_MOVE_AND_ASSIGN(RotationalInertia)

  /// Creates a rotational inertia that depends on the argument inertiaValue.

nit violation of GSG naming convention

Suggestion:

inertia_value

---

multibody/tree/spatial_inertia.h line 122 at r1 (raw file):

  }

  /// Creates a spatial inertia that depends on the argument inertiaValue.

nit violation of GSG naming convention

Suggestion:

inertia_value

---

multibody/tree/spatial_inertia.h line 135 at r1 (raw file):

  /// @note The default parameters mass = 2 and length = 3 correspond to a mass
  /// moment of inertia of 3 for any line that pass through Bcm.
  static SpatialInertia<T> MakeTestCube(T mass = T(2), T length = T(3));

What's the reasoning behind not going through the deprecation process for this function?

---

multibody/tree/unit_inertia.h line 44 at r1 (raw file):

  DRAKE_DEFAULT_COPY_AND_MOVE_AND_ASSIGN(UnitInertia)

  /// Creates a unit inertia that depends on the argument inertiaValue.

nit violation of GSG naming convention

Suggestion:

inertia_value

---

multibody/tree/test/multibody_tree_creation_test.cc line 844 at r1 (raw file):

// cube whose outward normal is -Bx. Hence, the position vector from Bo to Bcm
// (B's center of mass) is p_BoBcm_B = Lx/2 Bx.
const UnitInertia<double> MakeTestCubeUnitInertia(const double length = 1.0) {

nit const with no effect

See https://drake.mit.edu/styleguide/cppguide.html#Use_of_const

Suggestion:

UnitInertia<double> MakeTestCubeUnitInertia(const double length = 1.0) {

---

bindings/pydrake/multibody/tree_py.cc line 144 at r1 (raw file):

  {
    using Enum = InertiaValue;

The new bindings need to be tested.

---

bindings/pydrake/multibody/tree_py.cc line 861 at r1 (raw file):

    cls  // BR
        .def(py::init<InertiaValue>(),
            py::arg("inertiaValue") = InertiaValue::kNaN,

nit and ditto below

Suggestion:

            py::arg("inertia_value") = InertiaValue::kNaN,

[mitiguy]

Reviewable status: 5 unresolved discussions, LGTM missing from assignee xuchenhan-tri(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @sherm1 and @xuchenhan-tri)

a discussion (no related file):

Previously, xuchenhan-tri wrote…

This PR also introduces new constructors for UnitInertia and RotationInertia, but none of our existing tests/examples seems to use it. That leads to my question:

a. are they added as new sugars and part of the new features? or
b. are they added in support of the new constructor for SpatialInertia?

If a, then they should also show up in the PR notes. If b, then it seems like you don't need them and can use existing constructors.

@sherm1 Let me know if you have a preference here. @xuchenhan-tri makes a good point.
Alternatively, it is nice to have parallelism in the constructors for SpatialInertia, UnitInertia, RotationInertia.
Pushing the changes that should "stick" now.

---

---

multibody/plant/test/compliant_contact_manager_test.cc line 1676 at r1 (raw file):

Previously, xuchenhan-tri wrote…

nit, from all the test cases in the PR, it seems like InertiaValue::kSdformat is a better default as it is used almost everywhere whereas kNaN only shows up in a couple places.

Per f2f -- prior discussions with others suggested that the default should be kept as InertiaValue::kNaN.

---

multibody/tree/rotational_inertia.h line 31 at r1 (raw file):

Previously, xuchenhan-tri wrote…

BTW, would this be a better link

Done.

---

multibody/tree/rotational_inertia.h line 32 at r1 (raw file):

Previously, xuchenhan-tri wrote…

Why don't we call it kIdentitiy instead? It's nicely parallel with kNaN, more intuitive, and we don't have to worry here in this class about the convention of Sdformat and whether it would change over time. It will be also more readable at callsites. Besides, it just seems like an odd fit to have Sdformat (parsing related) show up in multibody/tree.

@sherm1 -- it would be good to have you weigh in on this as well.
I prefer @xuchenhan-tri kIdentity instead of kSdformat.

---

multibody/tree/rotational_inertia.h line 175 at r1 (raw file):

Previously, xuchenhan-tri wrote…

nit violation of GSG naming convention

Done here and elsewhere.

---

multibody/tree/spatial_inertia.h line 122 at r1 (raw file):

Previously, xuchenhan-tri wrote…

nit violation of GSG naming convention

Done here and elsewhere.

---

multibody/tree/spatial_inertia.h line 135 at r1 (raw file):

Previously, xuchenhan-tri wrote…

What's the reasoning behind not going through the deprecation process for this function?

1. It is relatively new (a few weeks).
2. It was marked as Internal use only.
3. There is no Python binding for it.

---

multibody/tree/unit_inertia.h line 44 at r1 (raw file):

Previously, xuchenhan-tri wrote…

nit violation of GSG naming convention

Done here and elsewhere.

---

multibody/tree/test/multibody_tree_creation_test.cc line 844 at r1 (raw file):

Previously, xuchenhan-tri wrote…

nit const with no effect

See https://drake.mit.edu/styleguide/cppguide.html#Use_of_const

Done.

---

bindings/pydrake/multibody/tree_py.cc line 144 at r1 (raw file):

Previously, xuchenhan-tri wrote…

The new bindings need to be tested.

I am unsure how to do this.
Perhaps I did a poor job of copying what was done in the code just prior:

using Enum = JacobianWrtVariable;
constexpr auto& enum_doc = doc.JacobianWrtVariable;
py::enum_<Enum> enum_py(m, "JacobianWrtVariable", enum_doc.doc);
enum_py  // BR
    .value("kQDot", Enum::kQDot, enum_doc.kQDot.doc)
    .value("kV", Enum::kV, enum_doc.kV.doc);

---

bindings/pydrake/multibody/tree_py.cc line 861 at r1 (raw file):

Previously, xuchenhan-tri wrote…

nit and ditto below

Done here and elsewhere.

[jwnimmer-tri]

From an API design standpoint, I think it's worth considering another option.

When Eigen provides sugar for filling in constant matrices, it uses factory functions like Vector3d::Zero() or Vector3d::Constant(NaN) or Matrix3d::Identity(), i.e., static member functions. Broadly speaking, I think that's a better C++ (and python) spelling to use to select between different constructor options, compared to a single-purpose enum.

The enum is just an extra free-floating typename that the user needs to be import before they can use it, extra python bindings to maintain, etc. This effect doesn't show up in multibody unit tests because they are already in the multibody namespace, but it does show up in the few other places here that start using the new constructor, and would be an extra wart in end-user code.

Perhaps the default constructor can remain as NaN as it is today, but then we can add a new factory function such as SpatialInertia::Identity(), SpatialInertia::Default(), SpatialInertia::Diagonal() or something. I agree with @xuchenhan-tri that using "sdformat" as the name is completely out of place.

The only real reason to use an enum is if the choice on constructor needs to be delegated -- that is to say, one piece of code chooses the enum value and then through a series of function calls needs to pass that choice down to a different piece of code. That doesn't seem to be the case for inertia construction.

[sherm1]

Reviewable status: 5 unresolved discussions, LGTM missing from assignee xuchenhan-tri(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @sherm1 and @xuchenhan-tri)

---

multibody/tree/rotational_inertia.h line 32 at r1 (raw file):

Previously, mitiguy (Mitiguy) wrote…

@sherm1 -- it would be good to have you weigh in on this as well.
I prefer @xuchenhan-tri kIdentity instead of kSdformat.

I don't think there is such a concept as an "identity" inertia. It does happen to have ones on the diagonal, but those are kg-m^2 not unitless ones like an identity matrix. I suppose we could call it a "unit inertia" kUnit but that might be confusing with the UnitInertia class?

[sherm1]

My preference now would be:

• Change the behavior of the default constructors for RotationalInertia and SpatialInertia from NaNs to Ones.
• Fix documentation and note change in release notes (not exactly a breaking change since NaNs break everything anyway)
• Update tests to verify new defaults
• Undo all other changes

WDYT?

Reviewable status: 5 unresolved discussions, LGTM missing from assignee xuchenhan-tri(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @mitiguy, @sherm1, and @xuchenhan-tri)

---

multibody/plant/test/compliant_contact_manager_test.cc line 1676 at r1 (raw file):

Previously, mitiguy (Mitiguy) wrote…

Per f2f -- prior discussions with others suggested that the default should be kept as InertiaValue::kNaN.

Are you sure? I remember the idea of changing the urdf default got nixed but did anyone object to changing the SpatialInertia() default? If we can change this to the "all ones" value then we could just make that the only option and do away with the enum altogether. I'm not sure the NaN default is actually doing anyone any good.

[jwnimmer-tri]

I love it.

[xuchenhan-tri]

I don't have a strong opinion either way. I'm ok with both what @sherm1 proposed and a factory function.

Reviewed 6 of 6 files at r2.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee xuchenhan-tri(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @mitiguy, @sherm1, and @xuchenhan-tri)

a discussion (no related file):

Previously, mitiguy (Mitiguy) wrote…

@sherm1 Let me know if you have a preference here. @xuchenhan-tri makes a good point.
Alternatively, it is nice to have parallelism in the constructors for SpatialInertia, UnitInertia, RotationInertia.
Pushing the changes that should "stick" now.

Per f2f, the only reason that new constructors for Unit/RotationInertia were added is to support the (potential) alternative construction of SpatialInertia. That, however, is not necessary (regardless of whether we want an alternative way to construct SpatialInertia) because the existing constructors for Unit/RotationInertia are already sufficient to support construction of an arbitrary SpatialInertia. So I think it's best to (at least) undo the changes in Unit/RotationInertia.

---

---

multibody/tree/spatial_inertia.h line 135 at r1 (raw file):

Previously, mitiguy (Mitiguy) wrote…

1. It is relatively new (a few weeks).
2. It was marked as Internal use only.
3. There is no Python binding for it.

Ah I missed the "Internal use only" note. Thanks.

---

bindings/pydrake/multibody/tree_py.cc line 144 at r1 (raw file):

Previously, mitiguy (Mitiguy) wrote…

I am unsure how to do this.
Perhaps I did a poor job of copying what was done in the code just prior:

using Enum = JacobianWrtVariable;
constexpr auto& enum_doc = doc.JacobianWrtVariable;
py::enum_<Enum> enum_py(m, "JacobianWrtVariable", enum_doc.doc);
enum_py  // BR
    .value("kQDot", Enum::kQDot, enum_doc.kQDot.doc)
    .value("kV", Enum::kV, enum_doc.kV.doc);

See

drake/bindings/pydrake/test/geometry_scene_graph_test.py

Line 22 in 5857b20

def test_hydroelastic_contact_representation_enum(self):

for an example of a test. Although, per the discussion in the main thread in the PR, this might not be necessary.

[mitiguy]

Reviewable status: 4 unresolved discussions, LGTM missing from assignee xuchenhan-tri(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @sherm1 and @xuchenhan-tri)

a discussion (no related file):

Previously, xuchenhan-tri wrote…

Per f2f, the only reason that new constructors for Unit/RotationInertia were added is to support the (potential) alternative construction of SpatialInertia. That, however, is not necessary (regardless of whether we want an alternative way to construct SpatialInertia) because the existing constructors for Unit/RotationInertia are already sufficient to support construction of an arbitrary SpatialInertia. So I think it's best to (at least) undo the changes in Unit/RotationInertia.

Per f2f with Sherm and Xuchent, this PR is basically reverting to a name change:
MakeTestCube(default parameters) -> MakeUnitary().
I think this addresses most of the objections.
I recommend a new review, comparing the original master against the last commit.

---

---

multibody/plant/test/compliant_contact_manager_test.cc line 1676 at r1 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Are you sure? I remember the idea of changing the urdf default got nixed but did anyone object to changing the SpatialInertia() default? If we can change this to the "all ones" value then we could just make that the only option and do away with the enum altogether. I'm not sure the NaN default is actually doing anyone any good.

Per f2f with Sherm and Xuchent, this PR is basically reverting to a name change:
MakeTestCube(default parameters) -> MakeUnitary()

---

bindings/pydrake/multibody/tree_py.cc line 144 at r1 (raw file):

Previously, xuchenhan-tri wrote…

See

drake/bindings/pydrake/test/geometry_scene_graph_test.py

Line 22 in 5857b20

def test_hydroelastic_contact_representation_enum(self):

for an example of a test. Although, per the discussion in the main thread in the PR, this might not be necessary.

No longer needed.

---

multibody/tree/rotational_inertia.h line 32 at r1 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

I don't think there is such a concept as an "identity" inertia. It does happen to have ones on the diagonal, but those are kg-m^2 not unitless ones like an identity matrix. I suppose we could call it a "unit inertia" kUnit but that might be confusing with the UnitInertia class?

No longer needed.

[sherm1]

Looks good to me with one question below.

Reviewed 1 of 6 files at r2, 17 of 17 files at r3, all commit messages.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee xuchenhan-tri(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @mitiguy and @xuchenhan-tri)

---

bindings/pydrake/multibody/tree_py.cc line 299 at r3 (raw file):

#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
    cls.def("get_default_mass",

BTW this seems out of place in this PR

[xuchenhan-tri]

with two minor things

1. The title and the description of the PR needs an update.
2. Two python binding files need to be reverted.

+@EricCousineau-TRI for platform review per rotation please.

Reviewed 13 of 17 files at r3.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI and @mitiguy)

---

bindings/pydrake/multibody/test/plant_test.py line 413 at r3 (raw file):

        self.assertIsInstance(body.floating_positions_start(), int)
        self.assertIsInstance(body.floating_velocities_start(), int)
        self.assertIsInstance(body.default_mass(), float)

nit, please revert this file too.

[EricCousineau-TRI]

platform, but raising Xuchen's point (2) to blocking

Reviewed 1 of 6 files at r2, 12 of 17 files at r3, all commit messages.
Reviewable status: 2 unresolved discussions, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @sherm1 and @xuchenhan-tri)

---

bindings/pydrake/multibody/tree_py.cc line 299 at r3 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW this seems out of place in this PR

Raising to blocking: Yes, it is. Can you revert or split this out?

---

bindings/pydrake/multibody/test/plant_test.py line 413 at r3 (raw file):

Previously, xuchenhan-tri wrote…

nit, please revert this file too.

Raising to blocking.

[mitiguy]

1. Renamed title of the PR.
2. Weird: Did not need to revert the two Python files. There was a glitch in reviewable and it has been addressed by rebasing and squashing.

Reviewable status: 2 unresolved discussions, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI, @sherm1, and @xuchenhan-tri)

---

bindings/pydrake/multibody/tree_py.cc line 299 at r3 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Raising to blocking: Yes, it is. Can you revert or split this out?

Done. There was a glitch in reviewable and it has been addressed by rebasing and squashing.

---

bindings/pydrake/multibody/test/plant_test.py line 413 at r3 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Raising to blocking.

Done. There was a glitch in reviewable and it has been addressed by rebasing and squashing.

[EricCousineau-TRI]

Reviewed 4 of 4 files at r4, all commit messages.
Reviewable status: commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @mitiguy)

---

bindings/pydrake/multibody/tree_py.cc line 299 at r3 (raw file):

Previously, mitiguy (Mitiguy) wrote…

Done. There was a glitch in reviewable and it has been addressed by rebasing and squashing.

OK Thanks! (btw, having done it myself, most likely one of many PEBKACs w/ git, not a glitch in reviewable ;)

---

bindings/pydrake/multibody/test/plant_test.py line 413 at r3 (raw file):

Previously, mitiguy (Mitiguy) wrote…

Done. There was a glitch in reviewable and it has been addressed by rebasing and squashing.

OK Thanks!