[multibody] Eliminate Body base class in favor of RigidBody

[sherm1]

Background

We originally conceived of multibody::Body as an abstract class that could represent both rigid and deformable bodies. In reality we took a different approach to deformable bodies leaving RigidBody as the only subclass of Body, leaving a bunch of unnecessary templates, virtual functions, and general awkwardness. There is also some unnecessary overhead in retrieving simple RigidBody quantities like center of mass, inertia, or attached frame locations because the Body class had to allow these to be state-dependent computed quantities in case the underlying body was deformable.

Proposal

This PR eliminates the abstract base class and moves its functionality to the concrete RigidBody class. Users were only ever able to use AddRigidBody() before, but in practice there is a mix of using Body or RigidBody to hold the result. We continue to use "Body" to refer to rigid bodies in functions like GetBodyByName() so I am not proposing to remove or deprecate the name "Body" but continue to allow it as a (dispreferred) alias for RigidBody. Only the Body class and all uses of it in Drake and PyDrake are gone. The body.h header file is deprecated.

BodyFrame is renamed RigidBodyFrame and the old name is deprecated. C++ will issue a warning for use of the deprecated name though Python will not.

Notes

• Using "rigid body" rather than "body" is good practice now that we also have deformable bodies. We should use that term as much as possible in our code, documentation, tutorials, etc. Only Drake code is addressed here.
• In C++, Body<T> works everywhere a RigidBody<T> is expected, but the forward declaration template <typename T> class Body; won't work now since Body isn't a class. However, our Stability Guide says "In C++, do not forward-declare anything from Drake." so this is not a breaking change per policy.
• Python code should work unchanged. However, as noted above no deprecation warning will be issued if any code uses the old name BodyFrame rather than RigidBodyFrame in Python.
• A nice bonus is that removing body type templatizing means the AddRigidBody() methods no longer have to be in a header file. They are moved to multibody_tree.cc here.
• There was a long standing dependence bug that prevented Body from being fully instantiated in body.cc due to MultibodyPlant not being defined yet. I nuked that dependency here so RigidBody can be fully instantiated in rigid_body.cc with no contortions needed.

Deprecations (for release notes)

• body.h header file
• BodyFrame in favor of RigidBodyFrame (no warning in Python)

---

This change is 

[sherm1]

BTW per f2f discussion at the on site: I am thinking of this minor change as being enough so that we don't need to leap all the way to "Link" -- that turned out to be very daunting in practice. Now the user-supplied objects are rigid bodies and deformable bodies. Internally the multibody code works only with mobilized bodies, where a single mobilized body may comprise a collection of welded-together rigid bodies. I think that's enough of a distinction to keep the internals untangled.

[jwnimmer-tri]

Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers

a discussion (no related file):
I think possibly we can ditch the "breaking change" label:

... a forward declaration like template class Body; won't work now since Body isn't a class.

Forward declaring Drake classes in end-user code is explicitly forbidden by the Stable API policy, so this is not a breaking change on that front.

... using drake::multibody::Body; conflicts with the alias if it is also defined.

I don't quite follow this, could you give an example? Is this only for code inside a namespace drake::multibody { ... } already? Thought not explicitly stated in our policy, I hope its generally understood that end-users are not allowed to open namespace drake.

---

[sherm1]

Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I think possibly we can ditch the "breaking change" label:

... a forward declaration like template class Body; won't work now since Body isn't a class.

Forward declaring Drake classes in end-user code is explicitly forbidden by the Stable API policy, so this is not a breaking change on that front.

... using drake::multibody::Body; conflicts with the alias if it is also defined.

I don't quite follow this, could you give an example? Is this only for code inside a namespace drake::multibody { ... } already? Thought not explicitly stated in our policy, I hope its generally understood that end-users are not allowed to open namespace drake.

I was referring to code like this and this from our examples. However, I see I had the alias temporarily commented out🤦🏼to ensure I found all the use cases in Drake. With that back in these using statements work fine, so since forward declarations are disallowed anyway I'll remove the label and fix the text.

---

[sherm1]

+@joemasterjohn for feature review, please (cc @amcastro-tri @RussTedrake)

Reviewer notes:

• lots of files touched but most of the changes are trivial name updates
• the new code in rigid_body.h is just the former contents of body.h, de-virtualized. That does need to be fully reviewed to make sure I've updated the comments properly but all the old unit tests remain unchanged and working.
• AddRigidBody code moved from multibody_tree-inl.h to multibody_tree.cc with only trivial changes which I've pointed out in Reviewable comments. That code doesn't need to be re-reviewed.
• the EvalSpatialAcceleration changes just make it work like Position & Velocity to eliminate a spurious dependency on MultibodyPlant that was preventing full cc-file instantiation of RigidBody<T> for various T's.

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

---

multibody/tree/multibody_tree.cc line 107 at r2 (raw file):

  const RigidBody<T>& body = this->AddRigidBodyImpl(
      std::make_unique<RigidBody<T>>(name, model_instance, M_BBo_B));

FYI this is the only code change -- now makes the new rigid body here; before it called a now-eliminated AddBody template method which created the body.

---

multibody/tree/multibody_tree.cc line 525 at r2 (raw file):

template <typename T>
const RigidBody<T>& MultibodyTree<T>::AddRigidBodyImpl(

FYI no code change here, just renamed and moved from inl.h to .cc.

[joemasterjohn]

Checkpoint. I think I've covered any new code + migrated code and documentation. I'll move on the the name changes next.

Reviewed 8 of 157 files at r1, 1 of 2 files at r2.
Reviewable status: 27 unresolved discussions, LGTM missing from assignee joemasterjohn, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @sherm1)

---

multibody/tree/rigid_body.h line 53 at r3 (raw file):

/// Body::body_frame(). This access is more than a convenience; you can use the
/// %BodyFrame to define other frames on the body and to attach other multibody
/// elements to it.

Comment still references deprecated Body class.

There are also a few uses of "body" rather than "rigid body" here and below in this file. I'll try to mark them all. I assume from your comment we might want to be strict about this, at least in this file. But doing /s/body/rigid body/ can make some of the sentence structure feel awkward/redundant, when the sentence has 2+ mentions of the/a "body".

Suggestion:

/// A %BodyFrame is a material Frame that serves as the unique reference frame
/// for a RigidBody.
///
/// Each RigidBody B, has a unique body frame for which we use the same symbol B
/// (with meaning clear from context). All properties of a rigid body are defined with
/// respect to its body frame, including its mass properties and attachment
/// locations for joints, constraints, actuators, geometry and so on. Run time
/// motion of the rigid body is defined with respect to the motion of its body frame.
/// We represent a body frame by a %BodyFrame object that is created whenever a
/// RigidBody is constructed and is owned by the RigidBody.
///
/// Note that the %BodyFrame associated with a rigid body does not necessarily need to
/// be located at its center of mass nor does it need to be aligned with the
/// rigid body's principal axes, although, in practice, it frequently is.
///
/// A %BodyFrame and RigidBody are tightly coupled concepts; neither makes sense
/// without the other. Therefore, a %BodyFrame instance is constructed in
/// conjunction with its RigidBody and cannot be constructed anywhere else. However,
/// you can still access the frame associated with a rigid body, see
/// RigidBody::body_frame(). This access is more than a convenience; you can use the
/// %BodyFrame to define other frames on the rigid body and to attach other multibody
/// elements to it.

---

multibody/tree/rigid_body.h line 57 at r3 (raw file):

/// @tparam_default_scalar
template <typename T>
class BodyFrame final : public Frame<T> {

BTW what do you think about renaming this class to RigidBodyFrame? It's so tightly coupled to RigidBody, across the board naming consistency would be nice. AFAICT, it is almost exclusively used internally with the return value of MultibodyPlant::world_body() being the one exception.

Suggestion:

class BodyFrame final : public Frame<T> { 

---

multibody/tree/rigid_body.h line 124 at r3 (raw file):

  // Only RigidBody objects can create BodyFrame objects since Body is a friend
  // of BodyFrame.

reference to Body

Suggestion:

  // Only RigidBody objects can create BodyFrame objects since RigidBody is a friend
  // of BodyFrame.

---

multibody/tree/rigid_body.h line 142 at r3 (raw file):

// Attorney-Client idiom to grant MultibodyTree access to a selected set of
// private methods in RigidBody.
// RigidBodyAttorney serves as a "proxy" to the Body class but only providing an

reference to Body

Suggestion:

// RigidBodyAttorney serves as a "proxy" to the RigidBody class but only providing an

---

multibody/tree/rigid_body.h line 220 at r3 (raw file):

  BodyIndex index() const { return this->template index_impl<BodyIndex>(); }

  /// Gets the `name` associated with `this` body. The name will never be empty.

nit: body -> rigid body

Suggestion:

  /// Gets the `name` associated with `this` rigid body. The name will never be empty.

---

multibody/tree/rigid_body.h line 245 at r3 (raw file):

        .get_mobilizer(topology_.inboard_mobilizer)
        .Lock(context);
  }

nit: body -> rigid body

Suggestion:

  /// For a floating rigid body, lock its inboard joint. Its generalized
  /// velocities will be 0 until it is unlocked.
  /// @throws std::exception if this rigid body is not floating.
  void Lock(systems::Context<T>* context) const {
    // TODO(rpoyner-tri): consider extending the design to allow locking on
    //  non-floating rigid bodies.
    if (!is_floating()) {
      throw std::logic_error(fmt::format(
          "Attempted to call Lock() on non-floating rigid body {}", name()));
    }
    this->get_parent_tree()
        .get_mobilizer(topology_.inboard_mobilizer)
        .Lock(context);
  }

---

multibody/tree/rigid_body.h line 259 at r3 (raw file):

        .get_mobilizer(topology_.inboard_mobilizer)
        .Unlock(context);
  }

nit: body -> rigid body

Suggestion:

  /// For a floating rigid body, unlock its inboard joint.
  /// @throws std::exception if this rigid body is not floating.
  void Unlock(systems::Context<T>* context) const {
    // TODO(rpoyner-tri): consider extending the design to allow locking on
    //  non-floating rigid bodies.
    if (!is_floating()) {
      throw std::logic_error(fmt::format(
          "Attempted to call Unlock() on non-floating rigid body {}", name()));
    }
    this->get_parent_tree()
        .get_mobilizer(topology_.inboard_mobilizer)
        .Unlock(context);
  }

---

multibody/tree/rigid_body.h line 269 at r3 (raw file):

        .get_mobilizer(topology_.inboard_mobilizer)
        .is_locked(context);
  }

Body -> RigidBody

Suggestion:

  /// Determines whether this %RigidBody is currently locked to its inboard (parent)
  /// %RigidBody. This is not limited to floating rigid bodies but generally
  /// Joint::is_locked() is preferable otherwise.
  /// @returns true if the rigid body is locked, false otherwise.
  bool is_locked(const systems::Context<T>& context) const {
    return this->get_parent_tree()
        .get_mobilizer(topology_.inboard_mobilizer)
        .is_locked(context);
  }

---

multibody/tree/rigid_body.h line 274 at r3 (raw file):

  /// computational directed forest structure of the owning MultibodyTree to
  /// which this %Body belongs. This serves as the BodyNode index and the index
  /// into all associated quantities.

Body -> RigidBody

Suggestion:

  /// (Advanced) Returns the index of the mobilized body ("mobod") in the
  /// computational directed forest structure of the owning MultibodyTree to
  /// which this %RigidBody belongs. This serves as the BodyNode index and the index
  /// into all associated quantities.

---

multibody/tree/rigid_body.h line 286 at r3 (raw file):

  /// @note A floating body is not necessarily modeled with a quaternion
  /// mobilizer, see has_quaternion_dofs(). Alternative options include a space
  /// XYZ parametrization of rotations, see SpaceXYZMobilizer.

nit: body -> rigid body

This is an example of where a naiive /s/body/rigid body/ can make a sentence feel awkward. Do we want to say "the parent rigid body of this rigid body's ..."? Or does having one "rigid body" upfront suffice to use "body" as shorthand later in the sentence / paragraph?

Suggestion:

  /// (Advanced) Returns `true` if `this` rigid body is granted 6-dofs by a Mobilizer
  /// and the parent body of this body's associated 6-dof joint is `world`.
  /// @note A floating body is not necessarily modeled with a quaternion
  /// mobilizer, see has_quaternion_dofs(). Alternative options include a space
  /// XYZ parametrization of rotations, see SpaceXYZMobilizer.

---

multibody/tree/rigid_body.h line 297 at r3 (raw file):

  /// include a quaternion, which occupies the first four elements of q. Note
  /// that this does not imply that the body is floating since it may have
  /// fewer than 6 dofs or its inboard body could be something other than World.

nit: body -> rigid body

Suggestion:

  /// (Advanced) If `true`, this rigid body's generalized position coordinates q
  /// include a quaternion, which occupies the first four elements of q. Note
  /// that this does not imply that the rigid body is floating since it may have
  /// fewer than 6 dofs or its inboard rigid body could be something other than World.

---

multibody/tree/rigid_body.h line 315 at r3 (raw file):

  /// @throws std::exception if called pre-finalize
  /// @pre `this` is a floating body
  /// @see is_floating(), has_quaternion_dofs(), MultibodyPlant::Finalize()

Body -> RigidBody
and
body -> rigid body

Suggestion:

  /// (Advanced) For floating rigid bodies (see is_floating()) this method returns the
  /// index of this %RigidBody's first generalized position in the vector q of
  /// generalized position coordinates for a MultibodyPlant model.
  /// Positions q for this %RigidBody are then contiguous starting at this index.
  /// When a floating %RigidBody is modeled with quaternion coordinates (see
  /// has_quaternion_dofs()), the four consecutive entries in the state starting
  /// at this index correspond to the quaternion that parametrizes this %RigidBody's
  /// orientation.
  /// @throws std::exception if called pre-finalize
  /// @pre `this` is a floating rigid body
  /// @see is_floating(), has_quaternion_dofs(), MultibodyPlant::Finalize()

---

multibody/tree/rigid_body.h line 329 at r3 (raw file):

  /// @throws std::exception if called pre-finalize
  /// @pre `this` is a floating body
  /// @see floating_velocities_start_in_v()

Body -> RigidBody

Suggestion:

  /// (Advanced, Deprecated) For floating rigid bodies (see is_floating()) this
  /// method returns the index of this %RigidBody's first generalized velocity in the
  /// _full state vector_ for a MultibodyPlant model, under the dubious
  /// assumption that the state consists of [q v] concatenated.
  /// Velocities for this rigid body are then contiguous starting at this index.
  /// @throws std::exception if called pre-finalize
  /// @pre `this` is a floating rigid body
  /// @see floating_velocities_start_in_v()

---

multibody/tree/rigid_body.h line 347 at r3 (raw file):

  /// @throws std::exception if called pre-finalize
  /// @pre `this` is a floating body
  /// @see is_floating(), MultibodyPlant::Finalize()

Suggestion:

  /// (Advanced) For floating rigid bodies (see is_floating()) this method returns the
  /// index of this %RigidBody's first generalized velocity in the vector v of
  /// generalized velocities for a MultibodyPlant model.
  /// Velocities v for this %RigidBody are then contiguous starting at this index.
  /// @throws std::exception if called pre-finalize
  /// @pre `this` is a floating rigid body
  /// @see is_floating(), MultibodyPlant::Finalize()

---

multibody/tree/rigid_body.h line 358 at r3 (raw file):

  /// be in [0, 7) if `has_quaternion_dofs()` is true, otherwise in [0, 6).
  /// @throws std::exception if called pre-finalize
  /// @pre `this` is a floating body

nit: body -> rigid body

Suggestion:

  /// @pre `this` is a floating rigid body

---

multibody/tree/rigid_body.h line 376 at r3 (raw file):

  /// be in [0,6).
  /// @throws std::exception if called pre-finalize
  /// @pre `this` is a floating body

nit: body -> rigid body

Suggestion:

  /// @pre `this` is a floating rigid body

---

multibody/tree/rigid_body.h line 437 at r3 (raw file):

  /// Returns the pose `X_WB` of this body B in the world frame W as a function
  /// of the state of the model stored in `context`.

nit: body -> rigid body

Suggestion:

  /// Returns the pose `X_WB` of this rigid body B in the world frame W as a function
  /// of the state of the model stored in `context`.

---

multibody/tree/rigid_body.h line 446 at r3 (raw file):

  /// @param[in] context Contains the state of the model.
  /// @retval V_WB_W `this` body B's spatial velocity in the world frame W,
  /// expressed in W (for point Bo, the body frame's origin).

nit: body -> rigid body

Suggestion:

  /// Evaluates V_WB, `this` rigid body B's spatial velocity in the world frame W.
  /// @param[in] context Contains the state of the model.
  /// @retval V_WB_W `this` rigid body B's spatial velocity in the world frame W,
  /// expressed in W (for point Bo, the body frame's origin).

---

multibody/tree/rigid_body.h line 459 at r3 (raw file):

  /// @note When cached values are out of sync with the state stored in context,
  /// this method performs an expensive forward dynamics computation, whereas
  /// once evaluated, successive calls to this method are inexpensive.

nit: body -> rigid body

Suggestion:

  /// Evaluates A_WB, `this` rigid body B's spatial acceleration in the world frame W.
  /// @param[in] context Contains the state of the model.
  /// @retval A_WB_W `this` rigid body B's spatial acceleration in the world frame W,
  /// expressed in W (for point Bo, the body frame's origin).
  /// @note When cached values are out of sync with the state stored in context,
  /// this method performs an expensive forward dynamics computation, whereas
  /// once evaluated, successive calls to this method are inexpensive.

---

multibody/tree/rigid_body.h line 467 at r3 (raw file):

  /// Gets the spatial force on `this` body B from `forces` as F_BBo_W:
  /// applied at body B's origin Bo and expressed in world frame W.

nit: body -> rigid body

Suggestion:

  /// Gets the spatial force on `this` rigid body B from `forces` as F_BBo_W:
  /// applied at rigid body B's origin Bo and expressed in world frame W.

---

multibody/tree/rigid_body.h line 476 at r3 (raw file):

  /// Adds the spatial force on `this` body B, applied at body B's origin Bo and
  /// expressed in the world frame W into `forces`.

nit: body -> rigid body

Suggestion:

  /// Adds the spatial force on `this` rigid body B, applied at rigid body B's origin Bo and
  /// expressed in the world frame W into `forces`.

---

multibody/tree/rigid_body.h line 500 at r3 (raw file):

  ///   A multibody forces objects that on output will have `F_Bp_E` added.
  /// @throws std::exception if `forces` is nullptr or if it is not consistent
  /// with the model to which `this` body belongs.

nit: body -> rigid body

Suggestion:

  /// Adds the spatial force on `this` rigid body B, applied at point P and
  /// expressed in a frame E into `forces`.
  /// @param[in] context
  ///   The context containing the current state of the model.
  /// @param[in] p_BP_E
  ///   The position of point P in B, expressed in a frame E.
  /// @param[in] F_Bp_E
  ///   The spatial force to be applied on rigid body B at point P, expressed in
  ///   frame E.
  /// @param[in] frame_E
  ///   The expressed-in frame E.
  /// @param[out] forces
  ///   A multibody forces objects that on output will have `F_Bp_E` added.
  /// @throws std::exception if `forces` is nullptr or if it is not consistent
  /// with the model to which `this` rigid body belongs.

---

multibody/tree/rigid_body.h line 796 at r3 (raw file):

          "From '" + std::string(source_method) +
          "'. The model to which this body belongs must be finalized. See "
          "MultibodyPlant::Finalize().");

nit: body -> rigid body

Suggestion:

      throw std::runtime_error(
          "From '" + std::string(source_method) +
          "'. The model to which this rigid body belongs must be finalized. See "
          "MultibodyPlant::Finalize().");

---

multibody/tree/rigid_body.h line 840 at r3 (raw file):

  }

  // MultibodyTree has access to the mutable BodyFrame through BodyAttorney.

BodyAttorney -> RigidBodyAttorney

Suggestion:

  // MultibodyTree has access to the mutable BodyFrame through RigidBodyAttorney.

---

multibody/tree/rigid_body.h line 845 at r3 (raw file):

  }

  // A string identifying the body in its model.

nit: body -> rigid body

Suggestion:

  // A string identifying the rigid body in its model.

---

multibody/tree/rigid_body.h line 850 at r3 (raw file):

  const std::string name_;

  // Body frame associated with this body.

nit: body -> rigid body

Suggestion:

  // BodyFrame associated with this rigid body.

[sherm1]

Awesome -- great stuff so far, Joe!

Reviewable status: 27 unresolved discussions, LGTM missing from assignee joemasterjohn, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)

---

multibody/tree/rigid_body.h line 57 at r3 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

BTW what do you think about renaming this class to RigidBodyFrame? It's so tightly coupled to RigidBody, across the board naming consistency would be nice. AFAICT, it is almost exclusively used internally with the return value of MultibodyPlant::world_body() being the one exception.

I thought of RigidBodyFrame but rejected the idea because BodyFrame is another public API class. But I hadn't realized how rarely it is used until you pointed it out -- everyone just uses Frame instead! I'll reconsider that now.

[joemasterjohn]

Ok full pass done after checking the name changes, PTAL. I think the one remaining discrepancy is the use of "body" vs. "rigid body" remaining in some comments or variable names. How strict do we want to be? Changing all instances of "body B" in comments to "rigid body B" seems really tedious.

Reviewed 135 of 157 files at r1, 1 of 2 files at r2, 12 of 12 files at r3, all commit messages.
Reviewable status: 27 unresolved discussions, LGTM missing from assignee joemasterjohn, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @sherm1)

[sherm1]

Checkpoint. Did most of the changes in rigid_body.h including rename BodyFrame -> RigidBodyFrame and BodyTopology -> RigidBodyTopology. See my comments about the use of "body" (as well as "rigid body") to refer to RigidBody objects. PTAL at rigid_body.h changes to see if you buy my argument there.

Reviewable status: 22 unresolved discussions, LGTM missing from assignee joemasterjohn, needs platform reviewer assigned, needs at least two assigned reviewers

---

multibody/tree/rigid_body.h line 53 at r3 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

Comment still references deprecated Body class.

There are also a few uses of "body" rather than "rigid body" here and below in this file. I'll try to mark them all. I assume from your comment we might want to be strict about this, at least in this file. But doing /s/body/rigid body/ can make some of the sentence structure feel awkward/redundant, when the sentence has 2+ mentions of the/a "body".

Done, PTAL. I want to get rid of all mentions of "Body" (the old class name), but I still am fine with calling a RigidBody a "body". This is inevitable because we have so many functions with names like GetBodyByName() and it is impractical to rename them. And a rigid body is a kind of body so that seems OK to me. Internally I will be using "mobod" everywhere rather than "body" to refer to mobilized bodies in the multibody forest. I would have liked to rename Body to "Link" to make the names really distinct, but that was too difficult in practice because we can't abbreviate "Link" as "body" so would have to rename hundreds of member functions.

I went through all the comments in rigid_body.h. Shouldn't be any more references to "Body" but "body" and "rigid body" are in use. LMK if you think anything is confusing -- in cases where it's clear I don't think it matters whether we say "rigid body", or "body". Also a rigid body will have a body frame (rigid is redundant there), although now the object representing a body frame is a RigidBodyFrame per your suggestion.

---

multibody/tree/rigid_body.h line 57 at r3 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

I thought of RigidBodyFrame but rejected the idea because BodyFrame is another public API class. But I hadn't realized how rarely it is used until you pointed it out -- everyone just uses Frame instead! I'll reconsider that now.

Done

---

multibody/tree/rigid_body.h line 124 at r3 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

reference to Body

Done.

---

multibody/tree/rigid_body.h line 142 at r3 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

reference to Body

Done.

---

multibody/tree/rigid_body.h line 269 at r3 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

Body -> RigidBody

Done (left "body")

---

multibody/tree/rigid_body.h line 274 at r3 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

Body -> RigidBody

Done.

---

multibody/tree/rigid_body.h line 286 at r3 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

nit: body -> rigid body

This is an example of where a naiive /s/body/rigid body/ can make a sentence feel awkward. Do we want to say "the parent rigid body of this rigid body's ..."? Or does having one "rigid body" upfront suffice to use "body" as shorthand later in the sentence / paragraph?

OK, I think

---

multibody/tree/rigid_body.h line 297 at r3 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

nit: body -> rigid body

OK, I think

---

multibody/tree/rigid_body.h line 315 at r3 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

Body -> RigidBody
and
body -> rigid body

Done, though left some "body"s

---

multibody/tree/rigid_body.h line 329 at r3 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

Body -> RigidBody

Done (class name)

---

multibody/tree/rigid_body.h line 347 at r3 (raw file):

  /// @throws std::exception if called pre-finalize
  /// @pre `this` is a floating body
  /// @see is_floating(), MultibodyPlant::Finalize()

Done. (class name)

---

multibody/tree/rigid_body.h line 358 at r3 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

nit: body -> rigid body

OK

---

multibody/tree/rigid_body.h line 376 at r3 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

nit: body -> rigid body

OK

---

multibody/tree/rigid_body.h line 437 at r3 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

nit: body -> rigid body

Done (used class name)

---

multibody/tree/rigid_body.h line 446 at r3 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

nit: body -> rigid body

OK, I think

---

multibody/tree/rigid_body.h line 459 at r3 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

nit: body -> rigid body

OK, I think

---

multibody/tree/rigid_body.h line 467 at r3 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

nit: body -> rigid body

Done (class name)

---

multibody/tree/rigid_body.h line 476 at r3 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

nit: body -> rigid body

Done (class name)

---

multibody/tree/rigid_body.h line 500 at r3 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

nit: body -> rigid body

OK ?

---

multibody/tree/rigid_body.h line 796 at r3 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

nit: body -> rigid body

Done

---

multibody/tree/rigid_body.h line 840 at r3 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

BodyAttorney -> RigidBodyAttorney

Done.

---

multibody/tree/rigid_body.h line 845 at r3 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

nit: body -> rigid body

OK ?

---

multibody/tree/rigid_body.h line 850 at r3 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

nit: body -> rigid body

OK? It's a body frame though now the object is RigidBodyFrame

[joemasterjohn]

Nice. I agree with your comments about "body" vs. "rigid body".

I was going through and checking for lingering references to Body or Body:: in comments / code snippets and I decided giving you a commit you could just patch would be easier than a bunch of reviewable comments:

8a9436a

I also did make a small code change for MultibodyTree::GetBodyByName(). The actual implementation of this method (in MultibodyTree::GetElementByName()) is templated on the element index type (BodyIndex in this case) to generate class names for error messages (messages that users see as errors when they call public APIs like MultibodyPlant::GetRigidBodyByName(), etc.) So I changed the element class name string that BodyIndex maps to from Body to RigidBody. After that it seemed redundant to keep GetBodyByName() so I removed it. The only code that called that method, MultibodyPlant::GetBodyByName(), now uses MultibodyPlant::GetRigidBodyByName(). Maybe removing it isn't the right thing to do, but the class name in the error message should be changed in my opinion.

Let me know what you think.

Reviewed 14 of 14 files at r4, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee joemasterjohn, needs platform reviewer assigned, needs at least two assigned reviewers

---

multibody/tree/rigid_body.h line 53 at r3 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Done, PTAL. I want to get rid of all mentions of "Body" (the old class name), but I still am fine with calling a RigidBody a "body". This is inevitable because we have so many functions with names like GetBodyByName() and it is impractical to rename them. And a rigid body is a kind of body so that seems OK to me. Internally I will be using "mobod" everywhere rather than "body" to refer to mobilized bodies in the multibody forest. I would have liked to rename Body to "Link" to make the names really distinct, but that was too difficult in practice because we can't abbreviate "Link" as "body" so would have to rename hundreds of member functions.

I went through all the comments in rigid_body.h. Shouldn't be any more references to "Body" but "body" and "rigid body" are in use. LMK if you think anything is confusing -- in cases where it's clear I don't think it matters whether we say "rigid body", or "body". Also a rigid body will have a body frame (rigid is redundant there), although now the object representing a body frame is a RigidBodyFrame per your suggestion.

Got it. That is perfectly reasonable. I think I read one of your original comments and interpreted it as saying you wanted to use "rigid body" everywhere. That's the only reason for all the "body" -> "rigid body" comments.

---

multibody/tree/rigid_body.h line 57 at r3 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Done

Nice, thanks.

[sherm1]

Incorporated your commit -- thanks! I updated the PR description to include BodyFrame->RigidBodyFrame and note the deprecation of BodyFrame.

Reviewable status: 1 unresolved discussion, LGTM missing from assignee joemasterjohn, needs platform reviewer assigned, needs at least two assigned reviewers

[sherm1]

Feature review complete (not sure Joe is around for an LGTM today though)

+@rpoyner-tri for platform review Jan 2 (if anyone wants to do it earlier, please do!)

Reviewable status: LGTM missing from assignees joemasterjohn,rpoyner-tri(platform)

a discussion (no related file):

Previously, sherm1 (Michael Sherman) wrote…

I was referring to code like this and this from our examples. However, I see I had the alias temporarily commented out🤦🏼to ensure I found all the use cases in Drake. With that back in these using statements work fine, so since forward declarations are disallowed anyway I'll remove the label and fix the text.

Done

---

---

multibody/tree/rigid_body.h line 220 at r3 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

nit: body -> rigid body

Done

---

multibody/tree/rigid_body.h line 245 at r3 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

nit: body -> rigid body

Done (some changes)

---

multibody/tree/rigid_body.h line 259 at r3 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

nit: body -> rigid body

Done

[sherm1]

(Platform reviewer: please see "notes to reviewer" above)

Reviewable status: LGTM missing from assignees rpoyner-tri(platform),joemasterjohn

[joemasterjohn]

great changes!

Reviewed 22 of 22 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: LGTM missing from assignee rpoyner-tri(platform)

[rpoyner-tri]

python aliasing/deprecation question

Reviewed 124 of 157 files at r1, 7 of 12 files at r3, 7 of 14 files at r4, 22 of 22 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: 1 unresolved discussion

---

bindings/pydrake/multibody/tree_py.cc line 297 at r6 (raw file):

    // TODO(sherm1) This is deprecated; remove 2024-04-01.
    m.attr("BodyFrame") = m.attr("RigidBodyFrame");

Does this aliasing step also need the "BodyFrame_" version aliased?

[sherm1]

Reviewable status: 1 unresolved discussion

---

bindings/pydrake/multibody/tree_py.cc line 297 at r6 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

Does this aliasing step also need the "BodyFrame_" version aliased?

Done. Yes! Good catch, thanks.

[sherm1]

@rpoyner-tri added the missing BodyFrame_ binding and imported the symbols in a test file toensure they exist. Also added a check for the default RigidBody name, which was missing. Back to you to unblock if satisfied.

Reviewable status: 1 unresolved discussion

[joemasterjohn]

Reviewed 2 of 2 files at r7, all commit messages.
Reviewable status: 1 unresolved discussion

[rpoyner-tri]

Reviewed 2 of 2 files at r7, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),joemasterjohn

[jwnimmer-tri]

The Anzu patching got missed. I'll work on that now.

[sherm1]

Rats -- this wasn't supposed to break anything in Anzu :( What broke?

[jwnimmer-tri]

Both deprecated items (the body.h header file path, and the class name BodyFrame in Python) were still being used, and so needed to be re-spelled.