Skip to content

[multibody] Name changes in support of composites to come#24443

Merged
sherm1 merged 1 commit into
RobotLocomotion:masterfrom
sherm1:name_changes_only
May 6, 2026
Merged

[multibody] Name changes in support of composites to come#24443
sherm1 merged 1 commit into
RobotLocomotion:masterfrom
sherm1:name_changes_only

Conversation

@sherm1

@sherm1 sherm1 commented Apr 22, 2026

Copy link
Copy Markdown
Member

This is a yak shave for the composites code. See #24350 for context.

In this PR there are still no composites being formed so there is exactly one link L on any mobilized body (mobod) B, and that link is the active link L₀ for that mobod. By construction, the L₀ frame and B frame are coincident (and always will be).

Terminology changes

These are internal terminology changes that help clarify some otherwise-muddy concepts:

  • Use "link" for RigidBody, use "mobod" as much as possible for mobilized bodies, with "body B" meaning a mobod while "link L" is a link, more than one of which may follow a mobilized body
  • Be careful to distinguish LinkOrdinal from LinkIndex from MobodIndex
  • Clarify the "active link" terminology for mobods -- one of the links on a composite is the "active link", which is the link whose joint connects to the inboard mobod.
  • Updated comments to describe our terminology troubles and to make them use the improved terminology (see e.g. the class comment for MultibodyTree).

New public symbols

  • Link<t> is an alias for RigidBody<T>
  • LinkIndex is an alias for BodyIndex
  • LinkOrdinal indexes packed and possibly reordered arrays of Links

Making these public now allows for consistent notation in the many unit tests that mix MbT and MbP APIs. They won't affect user-visible APIs. I don't think there is any need to mention them in the release notes, but we could.

RigidBody and BodyIndex will not be deprecated but the aliases can be used where the Link/Mobod distinction needs to be emphasized, most definitely when working with composites!

Other changes

Several other minor changes are included here so that the main composites PR can focus on the math:

  • Adds a public factory method SpatialVector::NaN() in prep for composite-induced missing items, like reaction forces for weld joints within a composite.
  • Introduces distinct link-frame vs. mobod-frame quantities in caches, e.g. X_LF and X_BF for precalculated frame offsets w.r.t. their Link or Mobod, resp., but sets them to the same value in this PR. This allows for consistent link terminology in this PR, and sets the stage for these values to differ in the next one. (Also includes provision for mass properties M_LLo_L vs M_BBo_B, and related position and velocity kinematics quantities.)
  • Removed a silly memory optimization from FrameBodyPoseCache in favor of a simpler indexing scheme that generalizes to composites.
  • Loosens a few unit tests that were too fussy so that they will continue to work when composites are introduced.
  • Clarifies ElementCollection API to distinguish valid indexes from the totality of indexes.
  • Moved a few internal implementations from MbP to MbT to make them accessible in tree tests.
  • Some opportunistic cleanup for out-of-date and buggy comments as I encountered them.
  • Fixed bug in spatial_inertia_test that caused a spurious failure in a Resolute build.

This change is Reviewable

@sherm1 sherm1 added status: do not merge status: do not review release notes: none This pull request should not be mentioned in the release notes labels Apr 22, 2026
@sherm1
sherm1 force-pushed the name_changes_only branch from 28bc075 to 761c4ff Compare April 22, 2026 02:03
@sherm1
sherm1 requested a review from Copilot April 22, 2026 21:13

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates MultibodyTree / topology terminology in preparation for upcoming composite mobods by distinguishing links (RigidBody) from mobods (mobilized bodies), introducing LinkOrdinal vs LinkIndex, and adjusting caches / APIs to store per-link kinematics where needed.

Changes:

  • Introduces link-vs-mobod terminology and new indexing types (e.g., LinkOrdinal) and updates kinematics caches to include per-link pose/velocity (X_WL, V_WL).
  • Renames internal APIs (world_bodyworld_link, link_ordinalactive_link_ordinal, etc.) across MultibodyTree, topology, and tests.
  • Moves BaseBodyJointType enum definition/binding from plant-centric location into the tree module and updates Python tests accordingly.

Reviewed changes

Copilot reviewed 71 out of 71 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
multibody/tree/velocity_kinematics_cache.h Adds per-link spatial velocity pool V_WL and related accessors.
multibody/tree/uniform_gravity_field_element.cc Switches scratch arrays and loops from bodies to links.
multibody/tree/test/weld_mobilizer_test.cc Updates world access to world_link().
multibody/tree/test/weld_joint_test.cc Updates world access to world_link().
multibody/tree/test/universal_mobilizer_test.cc Updates world access to world_link().
multibody/tree/test/universal_joint_test.cc Updates world access to world_link().
multibody/tree/test/tree_from_mobilizers_test.cc Renames counts to num_links() and world to world_link().
multibody/tree/test/tree_from_joints_test.cc Renames world access and array sizing to num_links().
multibody/tree/test/screw_mobilizer_test.cc Updates world access to world_link().
multibody/tree/test/screw_joint_test.cc Updates world access to world_link().
multibody/tree/test/rpy_floating_mobilizer_test.cc Updates world access to world_link().
multibody/tree/test/rpy_floating_joint_test.cc Updates world access to world_link().
multibody/tree/test/rpy_ball_mobilizer_test.cc Updates world access to world_link().
multibody/tree/test/revolute_spring_test.cc Updates world access to world_link().
multibody/tree/test/revolute_mobilizer_test.cc Updates world access to world_link().
multibody/tree/test/revolute_joint_test.cc Updates world access to world_link().
multibody/tree/test/quaternion_floating_mobilizer_test.cc Updates world access to world_link().
multibody/tree/test/quaternion_floating_joint_test.cc Updates world access to world_link().
multibody/tree/test/prismatic_spring_test.cc Updates world access to world_link().
multibody/tree/test/prismatic_mobilizer_test.cc Updates world access to world_link().
multibody/tree/test/prismatic_joint_test.cc Updates world access to world_link().
multibody/tree/test/planar_mobilizer_test.cc Updates world access to world_link().
multibody/tree/test/planar_joint_test.cc Updates world access to world_link().
multibody/tree/test/multibody_tree_test.cc Updates world access to world_link() and related usage.
multibody/tree/test/multibody_tree_creation_test.cc Renames num_bodies()num_links() in internal tree tests and updates world access.
multibody/tree/test/multibody_forces_test.cc Adjusts test expectation to match num_links() sizing.
multibody/tree/test/model_instance_test.cc Updates world access to world_link().
multibody/tree/test/linear_bushing_roll_pitch_yaw_test.cc Updates world access to world_link().
multibody/tree/test/joint_actuator_test.cc Updates world access to world_link().
multibody/tree/test/free_rotating_body_plant.cc Updates world access to world_link().
multibody/tree/test/frames_test.cc Updates world access to world_link().
multibody/tree/test/curvilinear_mobilizer_test.cc Updates world access to world_link().
multibody/tree/test/curvilinear_joint_test.cc Updates world access to world_link().
multibody/tree/test/ball_rpy_joint_test.cc Updates world access to world_link().
multibody/tree/test/articulated_body_algorithm_test.cc Updates world access to world_link().
multibody/tree/rigid_body.h Adds RigidBody::ordinal() and Link alias; updates active-link logic.
multibody/tree/position_kinematics_cache.h Adds per-link pose pool X_WL and accessors; updates docs.
multibody/tree/multibody_tree_system.h Minor include cleanup (removes unused <unordered_map>).
multibody/tree/multibody_tree_system.cc Updates cache entry sizing and naming for mobod/link terminology.
multibody/tree/multibody_tree_indexes.h Introduces LinkIndex alias, LinkOrdinal, and world_link_ordinal().
multibody/tree/multibody_tree.h Renames body collections to links_, adds world_link(), and moves BaseBodyJointType here.
multibody/tree/multibody_tree.cc Implements new base-body joint type plumbing; updates kinematics APIs to link-aware forms.
multibody/tree/multibody_forces.cc Sizes forces storage using num_links().
multibody/tree/multibody_element.h Ensures ordinals default to index in set_parent_tree.
multibody/tree/joint.h Updates doc text around mobilizer specialization.
multibody/tree/frame.h Updates doc text (notably around cached pose semantics).
multibody/tree/element_collection.cc Adjusts explicit template instantiation to use Link alias for ABI consistency.
multibody/tree/body_node_impl.cc Writes X_WL / V_WL for the active link; updates variable naming.
multibody/tree/body_node.h Renames inboard accessor to inboard_mobod().
multibody/topology/test/spanning_forest_test.cc Updates tests for new active-link / active-joint naming.
multibody/topology/spanning_forest_mobod.h Renames mobod accessors; documents active link/joint and composite concept.
multibody/topology/spanning_forest_inlines.h Renames mobod_to_link_ordinalmobod_to_active_link_ordinal.
multibody/topology/spanning_forest_debug.cc Updates sanity checks and graphviz output for new naming.
multibody/topology/spanning_forest.h Updates API names and docs for active-link accessors.
multibody/topology/spanning_forest.cc Updates implementation to use active-link/joint naming and inboard/outboard mobod accessors.
multibody/topology/link_joint_graph_defs.h Moves LinkIndex / LinkOrdinal out; adds operator~ for options.
multibody/topology/link_joint_graph_debug.cc Updates graphviz logic for active link ordinal naming.
multibody/topology/link_joint_graph.cc Updates path/ancestor logic to use active link ordinals and renamed mobod accessors.
multibody/rational/rational_forward_kinematics_internal.cc Updates traversal to use new inboard/outboard naming and active link ordinals.
multibody/rational/rational_forward_kinematics.cc Updates parent/child checks to use inboard_mobod().
multibody/plant/test/multibody_plant_forward_dynamics_test.cc Relaxes tolerance and adds a warm-up section to avoid heap noise in allocation checks.
multibody/plant/multibody_plant.h Removes duplicated BaseBodyJointType definition; forwards world-body count to num_links().
multibody/plant/multibody_plant.cc Delegates base-body joint type handling to MultibodyTree; updates active link accessor.
multibody/optimization/test/toppra_test.cc Adjusts tolerance constant.
multibody/benchmarks/kuka_iiwa_robot/make_kuka_iiwa_model.cc Updates world access to world_link().
multibody/benchmarks/kuka_iiwa_robot/drake_kuka_iiwa_robot.h Updates world access and link counts; resizes arrays to num_links().
bindings/pydrake/multibody/tree_py.cc Binds BaseBodyJointType enum in the tree module.
bindings/pydrake/multibody/test/plant_test.py Updates imports to pull BaseBodyJointType from pydrake.multibody.tree.
bindings/pydrake/multibody/plant_py.cc Removes binding for BaseBodyJointType from the plant module.
bindings/generated_docstrings/multibody_tree.h Adds generated docs for new symbols and updated wording.
bindings/generated_docstrings/multibody_plant.h Removes generated docs for BaseBodyJointType from plant docs; updates cross-references.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread multibody/tree/frame.h
Comment thread bindings/pydrake/multibody/plant_py.cc
Comment thread multibody/tree/multibody_tree_system.cc
Comment thread multibody/tree/multibody_tree_system.cc
Comment thread multibody/tree/multibody_tree.cc Outdated
@sherm1 sherm1 added release notes: announce This pull request contains changes that should be included in release announcements. and removed release notes: none This pull request should not be mentioned in the release notes labels Apr 22, 2026
@sherm1
sherm1 force-pushed the name_changes_only branch 4 times, most recently from f9f81d2 to 0f741f4 Compare April 28, 2026 15:00
@sherm1
sherm1 requested a review from Copilot April 28, 2026 16:49

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 83 out of 83 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread multibody/tree/frame.h
Comment thread multibody/tree/rigid_body.h
Comment thread multibody/tree/element_collection.h
@sherm1
sherm1 force-pushed the name_changes_only branch from 86cf40e to 2446ebc Compare April 29, 2026 23:01
@sherm1 sherm1 added release notes: none This pull request should not be mentioned in the release notes and removed release notes: announce This pull request contains changes that should be included in release announcements. status: do not merge status: do not review labels Apr 30, 2026
@sherm1
sherm1 marked this pull request as ready for review April 30, 2026 16:59

@sherm1 sherm1 left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+a:@rpoyner-tri for feature review per f2f, please

This PR has many changes but most of them (1270 lines) are repeated internal name changes. Cloc says:

Category            added  modified  removed  
----------------------------------------------
code                290    827       131      
comments            203    445       127      
blank               82     0         4        
----------------------------------------------
TOTAL               575    1272      262 

So ~600 substantive comment and code changes.

@sherm1 made 4 comments.
Reviewable status: LGTM missing from assignee rpoyner-tri(platform), needs at least two assigned reviewers (waiting on rpoyner-tri).


multibody/plant/multibody_plant.h line 256 at r1 (raw file):

  kRpyFloatingJoint,         ///< 6 dofs using 3 angles; has singularity.
  kWeldJoint,                ///< 0 dofs, fixed to World.
};

FYI This moved to multibody_tree.h to facilitate low-level unit test access, but remains a public symbol (along with other public symbols there).


multibody/plant/multibody_plant.cc line 1458 at r1 (raw file):

    return BaseBodyJointType::kWeldJoint;
  return BaseBodyJointType::kQuaternionFloatingJoint;
}

FYI the implementations moved to MultibodyTree for test access.


multibody/tree/multibody_tree_indexes.h line 26 at r1 (raw file):

}

}  // namespace internal

FYI just moved internal:: stuff to the end of this header

@sherm1
sherm1 force-pushed the name_changes_only branch from 2446ebc to 53aa3b2 Compare April 30, 2026 18:39

@rpoyner-tri rpoyner-tri left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checkpoint. I'm largely on board, having read the core header file changes. Still haven't read all of the call site changes yet.

@rpoyner-tri reviewed 20 files and all commit messages, and made 3 comments.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee rpoyner-tri(platform), needs at least two assigned reviewers (waiting on sherm1).


multibody/tree/linear_spring_damper.h line 48 at r3 (raw file):

  DRAKE_NO_COPY_NO_MOVE_NO_ASSIGN(LinearSpringDamper);

  /// Constructor for a spring-damper between a point P on link `bodyA` and a

nit this is one place where the internal terminology and and public API historical terminology might cause confusion. Do we need to explain more here, or perhaps refer to a longer explanation elsewhere?


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

    AddMultibodyPlantSceneGraph,
    ApplyMultibodyPlantConfig,
    BaseBodyJointType,

As presented here, this looks like a breaking change to me. We get away with the move in C++ because it remains in the same namespace and the clients will still see the needed header files. Here, we are moving a name from one module to another with no backup plan.

@rpoyner-tri rpoyner-tri left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: most concerning is the python api change.

@rpoyner-tri reviewed 66 files and made 3 comments.
Reviewable status: 4 unresolved discussions, needs at least two assigned reviewers (waiting on sherm1).


multibody/optimization/test/toppra_test.cc line 213 at r3 (raw file):

  // This tolerance was tuned to work with the given gridpoints.
  const double tol = 1e-13;

nit This tolerance change is unclear to me. Do you need this?


multibody/plant/multibody_plant.cc line 4063 at r3 (raw file):

void MultibodyPlant<T>::CalcBodyPosesOutput(
    const Context<T>& context,
    std::vector<math::RigidTransform<T>>* outupt) const {

btw 😆

@sherm1 sherm1 left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sherm1 made 1 comment.
Reviewable status: 4 unresolved discussions, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on rpoyner-tri).


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

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

As presented here, this looks like a breaking change to me. We get away with the move in C++ because it remains in the same namespace and the clients will still see the needed header files. Here, we are moving a name from one module to another with no backup plan.

Done (with some Claude help). PTAL to see if you think this works.

@sherm1 sherm1 added release notes: newly deprecated This pull request contains new deprecations and removed release notes: none This pull request should not be mentioned in the release notes labels Apr 30, 2026

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries, I hadn't thought I was that grumpy. ;)

@SeanCurtis-TRI reviewed 4 files and all commit messages, and made 2 comments.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on sherm1).


multibody/topology/link_joint_graph_defs.h line 82 at r6 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Done, also for the other two similar fake bitmasks PTAL. I wasn't sure what you meant by 2 above, LMK if you have better wording.

What I meant by (2) is that generally the following should be considered defective:

void foo(ForestBuildingOptions options) {
  if (options == ForestBuildingOptions::kDefault) {
    ...
  }
}

The idea being because we can mask multiple values in, there's no guarantee that any live instance is uniquely one of the named enumerations.

Or am I wrong in that regard?

@sherm1
sherm1 force-pushed the name_changes_only branch from f1d9d61 to b520c5f Compare May 4, 2026 21:49

@rpoyner-tri rpoyner-tri left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rpoyner-tri reviewed 2 files and all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on SeanCurtis-TRI and sherm1).

@jwnimmer-tri

Copy link
Copy Markdown
Collaborator

@drake-jenkins-bot retest this please

@rpoyner-tri rpoyner-tri left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rpoyner-tri reviewed 1 file.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on SeanCurtis-TRI and sherm1).

@sherm1
sherm1 force-pushed the name_changes_only branch from 82353b4 to 01848f2 Compare May 5, 2026 17:19
@jwnimmer-tri

Copy link
Copy Markdown
Collaborator

@drake-jenkins-bot linux-resolute-gcc-bazel-experimental-release please

@rpoyner-tri rpoyner-tri left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rpoyner-tri reviewed all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on SeanCurtis-TRI and sherm1).

@jwnimmer-tri

Copy link
Copy Markdown
Collaborator

@drake-jenkins-bot linux-resolute-gcc-bazel-experimental-release please

1 similar comment
@jwnimmer-tri

Copy link
Copy Markdown
Collaborator

@drake-jenkins-bot linux-resolute-gcc-bazel-experimental-release please

@sherm1
sherm1 force-pushed the name_changes_only branch from 01848f2 to 625ed81 Compare May 5, 2026 21:28

@rpoyner-tri rpoyner-tri left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rpoyner-tri reviewed all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on SeanCurtis-TRI and sherm1).

@sherm1 sherm1 left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sherm1 made 1 comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on rpoyner-tri and SeanCurtis-TRI).


multibody/tree/frame.h line 29 at r6 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Working. I made too many publicly-visible changes here. I'll undo most of them.

Done. I reworked this to what I hope is the minimum necessary for internal users of Frame to be able to work with Link rather than RigidBody. This should not require any changes (or even awareness) in user code except that I've made it clear that a RigidBody is a Link (that shouldn't be a surprise since both URDF and SDF call these things "links"!).

I don't think there is any need for that to leak out to PyDrake since none of the internals are in Python. PTAL

@sherm1
sherm1 force-pushed the name_changes_only branch from 625ed81 to d9b0125 Compare May 5, 2026 22:13
@sherm1 sherm1 changed the title Name changes in support of composites to come [multibody] Name changes in support of composites to come May 5, 2026
@sherm1
sherm1 force-pushed the name_changes_only branch from d9b0125 to 1036f7f Compare May 5, 2026 22:58

@sherm1 sherm1 left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All platform review comments addressed, PTAL.

@sherm1 made 3 comments.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on rpoyner-tri and SeanCurtis-TRI).


multibody/topology/link_joint_graph_defs.h line 82 at r6 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

What I meant by (2) is that generally the following should be considered defective:

void foo(ForestBuildingOptions options) {
  if (options == ForestBuildingOptions::kDefault) {
    ...
  }
}

The idea being because we can mask multiple values in, there's no guarantee that any live instance is uniquely one of the named enumerations.

Or am I wrong in that regard?

Done. I added a caution about using ordinary operators -- "advanced" users might have a good use for them.


multibody/tree/test/spatial_inertia_test.cc line 1558 at r12 (raw file):

      M_BBcm_B.CalcPrincipalHalfLengthsAndPoseForMinimumBoundingBox();
  EXPECT_TRUE(CompareMatrices(Vector3<double>(a, b, c), abc, kTolerance,
                              MatrixCompareType::relative));

FYI this was failing in CI for this PR on Resolute (just a bug in the test; the value of a is 180m so the test should have been relative).

@sherm1 sherm1 left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sherm1 made 1 comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on rpoyner-tri and SeanCurtis-TRI).


multibody/tree/frame.h line 29 at r6 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Done. I reworked this to what I hope is the minimum necessary for internal users of Frame to be able to work with Link rather than RigidBody. This should not require any changes (or even awareness) in user code except that I've made it clear that a RigidBody is a Link (that shouldn't be a surprise since both URDF and SDF call these things "links"!).

I don't think there is any need for that to leak out to PyDrake since none of the internals are in Python. PTAL

(Easiest to see the reworked changes comparing r12 to r10)

@jwnimmer-tri jwnimmer-tri left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jwnimmer-tri made 1 comment.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on rpoyner-tri, SeanCurtis-TRI, and sherm1).


multibody/tree/test/spatial_inertia_test.cc line 1558 at r12 (raw file):

this was failing in CI for this PR on Resolute ...

Only because this PR hadn't been rebased to incorporate this fix that has already merged:

https://github.com/RobotLocomotion/drake/pull/24503/changes#diff-7d5e1225466aad42783ca4f0b28bbc849f05aaaea903de7efd732302b8cd4660

Possibly this new fix is better, but I don't think we should have both fixes landed.

@sherm1 sherm1 left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sherm1 made 1 comment.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on jwnimmer-tri, rpoyner-tri, and SeanCurtis-TRI).


multibody/tree/test/spatial_inertia_test.cc line 1558 at r12 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

this was failing in CI for this PR on Resolute ...

Only because this PR hadn't been rebased to incorporate this fix that has already merged:

https://github.com/RobotLocomotion/drake/pull/24503/changes#diff-7d5e1225466aad42783ca4f0b28bbc849f05aaaea903de7efd732302b8cd4660

Possibly this new fix is better, but I don't think we should have both fixes landed.

Yes, I believe the fix here is the correct one. I can put it in a separate PR if you prefer or just rebase this one and fix it. WDYT?

@jwnimmer-tri jwnimmer-tri left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jwnimmer-tri made 1 comment.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on rpoyner-tri, SeanCurtis-TRI, and sherm1).


multibody/tree/test/spatial_inertia_test.cc line 1558 at r12 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Yes, I believe the fix here is the correct one. I can put it in a separate PR if you prefer or just rebase this one and fix it. WDYT?

For my part, I don't mind which PR it's in, but wherever that is needs to simultaneously revert the tolerance change. (Other reviewers here may want it booted out, I don't know.)

@sherm1
sherm1 force-pushed the name_changes_only branch from 1036f7f to c5f865e Compare May 5, 2026 23:47

@sherm1 sherm1 left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sherm1 made 1 comment.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on jwnimmer-tri, rpoyner-tri, and SeanCurtis-TRI).


multibody/tree/test/spatial_inertia_test.cc line 1558 at r12 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

For my part, I don't mind which PR it's in, but wherever that is needs to simultaneously revert the tolerance change. (Other reviewers here may want it booted out, I don't know.)

Done.

@jwnimmer-tri jwnimmer-tri left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jwnimmer-tri resolved 1 discussion.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on rpoyner-tri and SeanCurtis-TRI).

Also miscellaneous opportunistic comment cleanups.
@sherm1
sherm1 force-pushed the name_changes_only branch from c5f865e to 747216f Compare May 5, 2026 23:50

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:LGTM:

@SeanCurtis-TRI reviewed 5 files and all commit messages, made 2 comments, and resolved 1 discussion.
Reviewable status: 1 unresolved discussion (waiting on sherm1).


multibody/tree/frame.h line 39 at r14 (raw file):

/// elements like joints, actuators, and constraints usually employ two %Frames,
/// with one %Frame connected to one body and the other connected to a different
/// body. Every %Frame F can report the Link (RigidBody) L to which it is

BTW You started with "Rigidbody (Link)" and "RigidBodyFrame (LinkFrame)" and now you've reversed the semantics "Link (RigidBody)", etc. I don't mind one or the other, but probably not both, right? No stron gfeelings here.

@rpoyner-tri rpoyner-tri left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rpoyner-tri reviewed 5 files and all commit messages.
Reviewable status: 1 unresolved discussion (waiting on sherm1).

@sherm1 sherm1 left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All done -- thanks Rico, Sean, and Jeremy!

@sherm1 made 2 comments and resolved 1 discussion.
Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),SeanCurtis-TRI(platform) (waiting on sherm1).


multibody/tree/frame.h line 39 at r14 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW You started with "Rigidbody (Link)" and "RigidBodyFrame (LinkFrame)" and now you've reversed the semantics "Link (RigidBody)", etc. I don't mind one or the other, but probably not both, right? No stron gfeelings here.

I've been setting the ordering intentionally depending on context. Users will normally just be familiar with RigidBody and RigidBodyFrame so I wanted to start with that. But then I needed to use "L" for the 😢 RigidBody and RigidBodyFrame so I wanted to emphasize "Link" to make that make sense. Below I used the R(L) ordering for the body() method and the L(R) ordering for the synonymous link() method. Not saying it was necessarily a good idea, but it wasn't accidental!

@sherm1
sherm1 merged commit 76a9173 into RobotLocomotion:master May 6, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release notes: none This pull request should not be mentioned in the release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants