[multibody] Rename spaceXYZ mobilizers to more closely match joint terminology

[sherm1]

(This is a yak shave for adding RpyFloatingJoint per #14949)

Renames

• SpaceXyzFloatingMobilizer -> RpyFloatingMobilizer
• SpaceXyzMobilizer -> RpyBallMobilizer

Updates related names and comments.

This provides consistency internally, though the user-visible names for Joints (quaternion_floating, rpy_floating vs. ball_rpy) don't quite match. However, this PR deals only in internal changes.

---

This change is 

[sherm1]

+@amcastro-tri for feature review, please

Reviewable status: LGTM missing from assignee amcastro-tri, needs platform reviewer assigned, needs at least two assigned reviewers

[amcastro-tri]

Thank you Sherm, this makes sense to me.
Probably a good idea to capture either in a TODO or an issue what the plan is with the now called BallRpyJoint, which we should probably now name RpyBallJoint.

Reviewed 17 of 17 files at r1, all commit messages.
Reviewable status: 4 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers

---

multibody/tree/ball_rpy_joint.h line 27 at r1 (raw file):

/// @tparam_default_scalar
template <typename T>
class BallRpyJoint final : public Joint<T> {

would we like to name this RpyBallJoint? are you simply trying to avoid deprecation or this is not even in the plan?

---

multibody/tree/rigid_body.h line 292 at r1 (raw file):

  /// @note A floating body is not necessarily modeled with a quaternion
  /// mobilizer, see has_quaternion_dofs(). Alternative options include a
  /// roll-pitch-yaw (Rpy) parametrization of rotations, see

minor, I'd probably write rpy (preferred) or RPY

---

multibody/tree/rpy_ball_mobilizer.h line 22 at r1 (raw file):

and in fact we call it an "rpy_ball_joint" in Drake

Is this some nomenclature we'll use in the future? AFAIKT the term `rpy_ball_joint" only shows in this one line

---

multibody/tree/rpy_ball_mobilizer.h line 29 at r1 (raw file):

// roll-pitch-yaw angles (also known as space-fixed x-y-z Euler angles or
// extrinsic angles). That is, the generalized coordinates q for this mobilizer
// correspond to angles q=[θ₀ θ₁ θ₂], for a sequence of rotations about the Fx,

minor, I persnally like spaces around the = sign.

[sherm1]

Feature review comments resolved, with an open discussion about naming, PTAL.

Reviewable status: 2 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers

---

multibody/tree/ball_rpy_joint.h line 27 at r1 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

would we like to name this RpyBallJoint? are you simply trying to avoid deprecation or this is not even in the plan?

Ideally we would name all the joints consistently. However, my feeling now is that it isn't really worth the pain to rename existing joints. Instead I went with making the new floating joint consistent with the old floating joint, and making the internal mobilizers consistent internally. If we add a quaternion version of Ball joint, we can call it BallQuaternionJoint or decide on renaming at that time. The current PR avoids public changes altogether, which is a plus.

The ideal names IMO would have been BallRpy, BallQuaternion, FloatingRpy, and FloatingQuaternion because then the joint variants would alphabetize together. We could move that way by renaming the existing floating joint to floating_quaternion_joint. But is this really going to solve any user problems? I'm guessing that people are able to find the joints they need. And the floating joints are most often added automatically under the covers without the user needing to name them.

WDYT?

---

multibody/tree/rigid_body.h line 292 at r1 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

minor, I'd probably write rpy (preferred) or RPY

Done

---

multibody/tree/rpy_ball_mobilizer.h line 22 at r1 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

and in fact we call it an "rpy_ball_joint" in Drake

Is this some nomenclature we'll use in the future? AFAIKT the term `rpy_ball_joint" only shows in this one line

Done, thanks -- that was a typo.

---

multibody/tree/rpy_ball_mobilizer.h line 29 at r1 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

minor, I persnally like spaces around the = sign.

Done

[sherm1]

+@jwnimmer-tri for platform review per rotation, please

Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform)

[jwnimmer-tri]

platform.

Reviewed 15 of 17 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: 8 unresolved discussions

---

multibody/plant/test/compliant_contact_manager_test.cc line 29 at r2 (raw file):

#include "drake/multibody/tree/prismatic_joint.h"
#include "drake/multibody/tree/revolute_joint.h"
#include "drake/multibody/tree/rpy_ball_mobilizer.h"

BTW The lack of any other changes in this file probably indicates that this is a dead include, and could be removed instead of renamed.

---

multibody/rational/rational_forward_kinematics.cc line 11 at r2 (raw file):

#include "drake/multibody/tree/prismatic_mobilizer.h"
#include "drake/multibody/tree/revolute_mobilizer.h"
#include "drake/multibody/tree/rpy_ball_mobilizer.h"

BTW The lack of any other changes in this file probably indicates that this is a dead include, and could be removed instead of renamed.

---

multibody/tree/rpy_floating_mobilizer.h line 78 at r2 (raw file):

  // Overloads to define the suffix names for the position and velocity
  // elements.

This is bug fix which is important to mention in the commit message.

I guess "release notes: none" is still good though, since there is no way and end-user could ever be using this class -- it's an internal class with no public joint for it yet.

---

multibody/tree/rpy_floating_mobilizer.cc line 18 at r2 (raw file):

template <typename T>
std::string RpyFloatingMobilizer<T>::position_suffix(

Missing unit tests for both of these new functions.

See QuaternionFloatingJointTest.NameSuffix for an example of what we need.

---

multibody/tree/rpy_floating_mobilizer.cc line 20 at r2 (raw file):

std::string RpyFloatingMobilizer<T>::position_suffix(
    int position_index_in_mobilizer) const {
  // Note: The order of variables here is documented in get_quaternion().

nit The phrase "documented in get_quaternion" does not seem relevant here.

---

multibody/tree/rpy_floating_mobilizer.cc line 23 at r2 (raw file):

  switch (position_index_in_mobilizer) {
    case 0:
      return "qx";

I imagine that we want r not qx, etc?

Ditto throughout the two new functions.

[jwnimmer-tri]

Reviewable status: 8 unresolved discussions

---

multibody/tree/rpy_floating_mobilizer.cc line 23 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I imagine that we want r not qx, etc?

Ditto throughout the two new functions.

Ah, I guess "r,p,y,x,y,z" would have a duplicate use of "y". Still, is "qx" a sufficiently clear abbreviation for "space-x" or "roll"?

[jwnimmer-tri]

Reviewable status: 8 unresolved discussions

---

multibody/tree/rpy_floating_mobilizer.cc line 23 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Ah, I guess "r,p,y,x,y,z" would have a duplicate use of "y". Still, is "qx" a sufficiently clear abbreviation for "space-x" or "roll"?

Huh, it's what RpyBallMobilizer already uses. I guess it's consistent, so I'll unblock myself here.

[jwnimmer-tri]

Reviewable status: 7 unresolved discussions

---

multibody/tree/rpy_floating_mobilizer.cc line 23 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Huh, it's what RpyBallMobilizer already uses. I guess it's consistent, so I'll unblock myself here.

I finally found #14971 (review). Withdrawn.

[sherm1]

Reviewable status: 2 unresolved discussions

---

multibody/plant/test/compliant_contact_manager_test.cc line 29 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW The lack of any other changes in this file probably indicates that this is a dead include, and could be removed instead of renamed.

Done. There were a lot of dead includes in this test.

---

multibody/rational/rational_forward_kinematics.cc line 11 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW The lack of any other changes in this file probably indicates that this is a dead include, and could be removed instead of renamed.

Done.

---

multibody/tree/rpy_floating_mobilizer.h line 78 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

This is bug fix which is important to mention in the commit message.

I guess "release notes: none" is still good though, since there is no way and end-user could ever be using this class -- it's an internal class with no public joint for it yet.

Done (updated commit message & added tests)

---

multibody/tree/rpy_floating_mobilizer.cc line 18 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Missing unit tests for both of these new functions.

See QuaternionFloatingJointTest.NameSuffix for an example of what we need.

Done

---

multibody/tree/rpy_floating_mobilizer.cc line 20 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit The phrase "documented in get_quaternion" does not seem relevant here.

Done

---

multibody/tree/rpy_floating_mobilizer.cc line 23 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I finally found #14971 (review). Withdrawn.

Yeah, I started out thinking r,p,y but then saw what RpyBall did. #14971 discussion shows lots of thought went into these names already -- better to stick to the plan!

[jwnimmer-tri]

Reviewed 4 of 4 files at r3, all commit messages.
Reviewable status: 2 unresolved discussions

[amcastro-tri]

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

---

multibody/tree/ball_rpy_joint.h line 27 at r1 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Ideally we would name all the joints consistently. However, my feeling now is that it isn't really worth the pain to rename existing joints. Instead I went with making the new floating joint consistent with the old floating joint, and making the internal mobilizers consistent internally. If we add a quaternion version of Ball joint, we can call it BallQuaternionJoint or decide on renaming at that time. The current PR avoids public changes altogether, which is a plus.

The ideal names IMO would have been BallRpy, BallQuaternion, FloatingRpy, and FloatingQuaternion because then the joint variants would alphabetize together. We could move that way by renaming the existing floating joint to floating_quaternion_joint. But is this really going to solve any user problems? I'm guessing that people are able to find the joints they need. And the floating joints are most often added automatically under the covers without the user needing to name them.

WDYT?

I love the way you explained this, thanks. But I am afraid it'll get lost. Could we captgure this in an issue?

---

multibody/tree/rpy_ball_mobilizer.h line 22 at r1 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Done, thanks -- that was a typo.

still, this string only shows in file names (like ball_rpy_joint.h), the comment seems to suggest otherwise. I'd suggest that if you are talking about the class, simply write BallRpyJoint. If you are talking of the name (i.e. BallRpyJoint::kTypeName) then ball_rpy.

[jwnimmer-tri]

Reviewable status: 1 unresolved discussion

---

multibody/tree/ball_rpy_joint.h line 27 at r1 (raw file):

Could we captgure this in an issue?

I must have misunderstood something. What would the issue say?

The premise of Sherm's response is that no additional renaming is necessary, so nothing here is left undone.

[sherm1]

Reviewable status: 1 unresolved discussion

---

multibody/tree/ball_rpy_joint.h line 27 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Could we captgure this in an issue?

I must have misunderstood something. What would the issue say?

The premise of Sherm's response is that no additional renaming is necessary, so nothing here is left undone.

Done. Yes, I'm advocating "won't fix" but I think Alejandro is asking for a broader discussion so I captured this in #20841. We can close it "won't fix" if no one feels strongly about changing existing names.

---

multibody/tree/rpy_ball_mobilizer.h line 22 at r1 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

still, this string only shows in file names (like ball_rpy_joint.h), the comment seems to suggest otherwise. I'd suggest that if you are talking about the class, simply write BallRpyJoint. If you are talking of the name (i.e. BallRpyJoint::kTypeName) then ball_rpy.

Done

[sherm1]

Dismissed @amcastro-tri from a discussion.
Reviewable status: complete! all discussions resolved, LGTM from assignees jwnimmer-tri(platform),amcastro-tri

[jwnimmer-tri]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignees jwnimmer-tri(platform),amcastro-tri