Update part of mobilizer API from snake_case to CamelCase

[mitiguy]

PR #21429 updates part of the internal C++ mobilizer API towards resolution of issue #21284.
This PR's details are described in an item of the itemized list in issue #21284.

---

This change is 

[mitiguy]

Feature review +@sherm1

Reviewable status: LGTM missing from assignee sherm1(platform), needs at least two assigned reviewers

[mitiguy]

Feature review -@sherm1
Feature review +@DamrongGuoy

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

[DamrongGuoy]

All renamed functions are internal.

Reviewed 38 of 38 files at r1.
Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers

[mitiguy]

Platform review +@jwnimmer-tri

Reviewable status: LGTM missing from assignee jwnimmer-tri(platform)

[jwnimmer-tri]

LGTM modulo discussions.

Reviewed 38 of 38 files at r1, all commit messages.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform)

---

multibody/tree/planar_mobilizer.h line 74 at r1 (raw file):

                           respectively.
   @returns A constant reference to `this` mobilizer. */
  const PlanarMobilizer<T>& SetTranslations(

Why is there an "s" at the end of this function name?

No other mobilizer uses plural "translations", even ones that have >1 translational dof. The below function doesn't even use the matching plurality (it says "SetTranslationRates" not "SetTranslationsRates").

If the purpose of the overall issue is to fix all of the names to be correct and consistent, then either it needs to be more clear why there is an "s" here (and not elsewhere), or else the checklist that tracks the remaining fixes as future work need to have an action item to circle back here and repair this irregularity (by removing the s, or adding the s to the other places with >1 translational dof).

---

multibody/tree/planar_mobilizer.h line 91 at r1 (raw file):

   @param[in] angle The desired angle in radians.
   @returns a constant reference to `this` mobilizer. */
  const PlanarMobilizer<T>& set_angle(systems::Context<T>* context,

I don't understand the rhyme or reason why this one is left alone (for now?).

Since the issue doesn't have a checklist to track all of the pending renames, I cannot tell whether this is merely planned as future work, or was an oversight in this PR. My guess was that this PR is supposed to have fixed all of the mobilizers, in which case this one got missed.

---

multibody/tree/revolute_mobilizer.h line 82 at r1 (raw file):

  // @param[in] angle The desired angle in radians.
  // @returns a constant reference to `this` mobilizer.
  const RevoluteMobilizer<T>& set_angle(

Ditto: I don't understand the rhyme or reason why this one is left alone (for now?).

---

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

   @param[in] angle The desired angle in radians.
   @returns a constant reference to `this` mobilizer. */
  const ScrewMobilizer<T>& set_angle(systems::Context<T>* context,

Ditto: I don't understand the rhyme or reason why this one is left alone (for now?).

[mitiguy]

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

---

multibody/tree/planar_mobilizer.h line 74 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Why is there an "s" at the end of this function name?

No other mobilizer uses plural "translations", even ones that have >1 translational dof. The below function doesn't even use the matching plurality (it says "SetTranslationRates" not "SetTranslationsRates").

If the purpose of the overall issue is to fix all of the names to be correct and consistent, then either it needs to be more clear why there is an "s" here (and not elsewhere), or else the checklist that tracks the remaining fixes as future work need to have an action item to circle back here and repair this irregularity (by removing the s, or adding the s to the other places with >1 translational dof).

Great question. The short answer is that SetTranslations() (which is unique to PlanarMobilizer) is used when there are more than 1 translational DOF, but not a full position vector. SetTranslationRates() is also unique to PlanarMobilizer. Perhaps we can rename these later (not during this PR) as SetPlanarTranslation() and SetPlanarTranslationRates(). Another name worth consideration (for parallelism with SetAngles() and SetAngularRates()) is SetTranslationalRates() -- which is closer to its cousins SetTranslationalVelocity().

This PR is a simple internal-only snake_case to CamelCase change to reflect the high cost of using the "set" functions. The desired public API may be worth review discussion before changing/deprecating public functions, ideally a discussion in conjunction with other possible future joints (currently we only implement a subset). The next PR will change set_angle() to SetAngles() -- but again only for Drake-internal code.

Note: There are other (possibly natural) inconsistencies, e.g., RpyFloatingMobilizer, RpyBallMobilizers, QuaternionFloatingMobilizer, ScrewMobilizer, RevoluteMobilizer, set the angular velocity (sometimes via setting an angular rate).
UniversalMobilzer::SetAngularRates(), sets the time-derivatives of angles which differs from SetAngularVelocity().

Note: There is some use of plural (s) already in Drake master with:
RpyFloatingMobilizer::SetAngles().
RpyBallMobilizer::SetAngles().

This PR also changes the internal use of
UniversalMobilzer::set_angles() -> SetAngles().
UniversalMobilzer::set_angular_rates() -> SetAngularRates().

---

multibody/tree/planar_mobilizer.h line 91 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I don't understand the rhyme or reason why this one is left alone (for now?).

Since the issue doesn't have a checklist to track all of the pending renames, I cannot tell whether this is merely planned as future work, or was an oversight in this PR. My guess was that this PR is supposed to have fixed all of the mobilizers, in which case this one got missed.

Changing set_angle() to SetAngle() is planned for future work.
Note: I added a comment to the PR discussion to clarify.
I'll work on that subsequently.

---

multibody/tree/revolute_mobilizer.h line 82 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Ditto: I don't understand the rhyme or reason why this one is left alone (for now?).

Ditto: set_angle() -> SetAngle() is for subsequent PR.

---

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

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Ditto: I don't understand the rhyme or reason why this one is left alone (for now?).

Ditto: set_angle() -> SetAngle() is for subsequent PR.

[jwnimmer-tri]

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

---

multibody/tree/planar_mobilizer.h line 74 at r1 (raw file):

The short answer is ...

The point isn't to tell me, the point is to tell the next person reading this code why it's this way. The code (+ comments) on the page needs to be clear on its own.

Note that the public API is already non-plural: PlanarJoint::set_default_translation. Only this one specific place is the oddball.

If the naming rule is supposed to be "1 or 3 translational dofs use singular, 2 (or 4+?) use plural" then the public API's naming mistake needs to be added to the issue checklist of things to fix in a future PR, and that rule needs to appear in a comment here (or nearby, possibly the mobilizer base class to lay out the naming policy for all subclasses).

The desired public API may be worth review discussion before changing/deprecating public functions, ideally a discussion in conjunction with other possible future joints (currently we only implement a subset).

I'm not sure why we are trying to stumble our way through this ad-hoc. The optimal approach would be to first make a plan for how everything should be named, get buy-in on that, and only then start to change the code.

In any case, I can live with a sub-optimal approach (doing it piecemeal, like this PR) but in that case the changes being proposed here need to be correct insofar as they go. Since this particular rename is to a not-correct name (or at least, a name without sufficient consensus), it can't be part of this PR. If it's important to land this PR hastily, then you could undo the SetTranslations rename. I think it's only a couple of lines, so shouldn't be hard to undo.

---

multibody/tree/planar_mobilizer.h line 91 at r1 (raw file):

Previously, mitiguy (Mitiguy) wrote…

Changing set_angle() to SetAngle() is planned for future work.
Note: I added a comment to the PR discussion to clarify.
I'll work on that subsequently.

Sure, that's a fine plan.

However -- the PR overview text is transient, soon to be left in the dust. I need to see a checklist of remaining tasks, collected in the issue 21284, in order to be convinced that the follow-up fix will actually happen. This particular problem is not currently mentioned at all in the issue.

The markdown syntax for a checklist looks like this:

- [ ] item
- [ ] item
- [ ] item

[mitiguy]

Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)

---

multibody/tree/planar_mobilizer.h line 74 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

The short answer is ...

The point isn't to tell me, the point is to tell the next person reading this code why it's this way. The code (+ comments) on the page needs to be clear on its own.

Note that the public API is already non-plural: PlanarJoint::set_default_translation. Only this one specific place is the oddball.

If the naming rule is supposed to be "1 or 3 translational dofs use singular, 2 (or 4+?) use plural" then the public API's naming mistake needs to be added to the issue checklist of things to fix in a future PR, and that rule needs to appear in a comment here (or nearby, possibly the mobilizer base class to lay out the naming policy for all subclasses).

The desired public API may be worth review discussion before changing/deprecating public functions, ideally a discussion in conjunction with other possible future joints (currently we only implement a subset).

I'm not sure why we are trying to stumble our way through this ad-hoc. The optimal approach would be to first make a plan for how everything should be named, get buy-in on that, and only then start to change the code.

In any case, I can live with a sub-optimal approach (doing it piecemeal, like this PR) but in that case the changes being proposed here need to be correct insofar as they go. Since this particular rename is to a not-correct name (or at least, a name without sufficient consensus), it can't be part of this PR. If it's important to land this PR hastily, then you could undo the SetTranslations rename. I think it's only a couple of lines, so shouldn't be hard to undo.

SetTranslations() is now reverted to set_translations().

---

multibody/tree/planar_mobilizer.h line 91 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Sure, that's a fine plan.

However -- the PR overview text is transient, soon to be left in the dust. I need to see a checklist of remaining tasks, collected in the issue 21284, in order to be convinced that the follow-up fix will actually happen. This particular problem is not currently mentioned at all in the issue.

The markdown syntax for a checklist looks like this:

- [ ] item
- [ ] item
- [ ] item

Done. Updated issue #21284

[jwnimmer-tri]

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