Skip to content

[multibody] Auto loop breaking: add weld constraints and shadow link visualization - #24917

Open
sherm1 wants to merge 1 commit into
RobotLocomotion:masterfrom
sherm1:auto_loop_breaking_add_weld_constraints
Open

[multibody] Auto loop breaking: add weld constraints and shadow link visualization#24917
sherm1 wants to merge 1 commit into
RobotLocomotion:masterfrom
sherm1:auto_loop_breaking_add_weld_constraints

Conversation

@sherm1

@sherm1 sherm1 commented Aug 25, 2026

Copy link
Copy Markdown
Member

This is the third PR in the auto-loop-breaking train leading to full functionality in #24843.

What's in this PR

What's left for the next PR

  • working examples showing that we can actually run simulations of closed-topology systems

There are no user-visible changes here since enabling automatic loop breaking is "internal use only" for now. No release note is needed.


This change is Reviewable

@sherm1 sherm1 added priority: medium release notes: none This pull request should not be mentioned in the release notes labels Aug 25, 2026
@sherm1
sherm1 requested a balanced review from Copilot August 25, 2026 18:22

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

Adds weld constraints and visualization support needed for automatically broken multibody loops.

Changes:

  • Welds shadow links back to their primary links.
  • Registers shadow SceneGraph frames and visualizes their inertia.
  • Adds scalar-conversion and closed-topology tests.

Reviewed changes

Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
visualization/test/inertia_visualizer_test.cc Tests shadow inertia visualization.
visualization/inertia_visualizer.cc Handles bodies without SceneGraph frames.
multibody/tree/weld_joint.cc Uses effective joint frames.
multibody/tree/shadow_frame.h Declares mirrored shadow frames.
multibody/tree/shadow_frame.cc Implements shadow frames and cloning.
multibody/tree/rigid_body.h Clarifies floating-base semantics.
multibody/tree/revolute_joint.cc Uses retargeted tree frames.
multibody/tree/prismatic_joint.cc Uses retargeted tree frames.
multibody/tree/multibody_tree.cc Creates shadow frames and handles floating loops.
multibody/tree/joint.h Tracks effective shadow-side frames.
multibody/tree/BUILD.bazel Builds shadow-frame sources.
multibody/topology/link_joint_graph.h Exposes loop-constraint count.
multibody/topology/link_joint_graph_link.h Reports retargeted joints.
multibody/topology/link_joint_graph_inlines.h Implements loop-constraint count.
multibody/plant/test/auto_closed_topology_test.cc Expands loop-modeling coverage.
multibody/plant/multibody_plant.h Adds loop-constraint API.
multibody/plant/multibody_plant.cc Registers shadows and creates weld constraints.
multibody/plant/constraint_specs.h Marks generated constraints ephemeral.
bindings/pydrake/multibody/test/plant_test.py Tests Python loop-topology APIs.
bindings/pydrake/multibody/plant_py.cc Binds loop-topology APIs.
bindings/generated_docstrings/multibody_tree.h Updates generated tree documentation.
bindings/generated_docstrings/multibody_plant.h Updates generated plant documentation.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

/// Returns the number of ephemeral weld constraints that Finalize() added in
/// order to close topological loops. Each of these welds a shadow link to the
/// link it is a copy of; see SetEnableLoopTopology(). These are included in
/// num_constraints() and num_weld_constraints(), and are indistinguishable

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.

Done

Comment on lines +1666 to +1667
// N.B. We add the spec directly rather than calling AddWeldConstraint(),
// which is a pre-finalize-only API.

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.

TAMSI is deprecated and will be removed next week so this is moot.

@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 please
Note that this has been rebased onto a still-open PR that Joe is reviewing. That's the first commit here and should be ignored. The second commit (from r1->r2) is about 500 lines, mostly unit tests.

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

@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.

First pass done. I'd like to poke it a bit more before I sign off.

@rpoyner-tri reviewed 22 files and all commit messages, and made 3 comments.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee rpoyner-tri(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on sherm1).


multibody/plant/multibody_plant.cc line 1526 at r2 (raw file):

  // geometry arrays: a shadow never carries any geometry of its own -- it's an
  // internal modeling artifact -- but these arrays are indexed by BodyIndex and
  // so must stay dense over num_bodies(); see GetVisualGeometriesForBody().

grammar, clarity? I don't even know.

This last sentence has just accumulated so many ideas, and punctuation, and somehow manages to be totally baffling. Can I get a human to maybe split it into two or three sentences, and clarify?

Code quote:

  // no entries in the per-body geometry arrays yet. First, the per-body
  // geometry arrays: a shadow never carries any geometry of its own -- it's an
  // internal modeling artifact -- but these arrays are indexed by BodyIndex and
  // so must stay dense over num_bodies(); see GetVisualGeometriesForBody().

multibody/plant/test/auto_closed_topology_test.cc line 8 at r2 (raw file):

shadows, the retargeting of a loop joint onto a shadow link, and the
ephemeral weld constraints that hold each shadow link to the link it is a
copy of.

nit Here is another sentence that overflows my registers. Maybe split it up, or make it into a bullet list of test objectives?

Code quote:

underlying LinkJointGraph/SpanningForest. This file tests that adding shadow
links splits mass properties properly among the primary link and all of its
shadows, the retargeting of a loop joint onto a shadow link, and the
ephemeral weld constraints that hold each shadow link to the link it is a
copy of.

@sherm1
sherm1 force-pushed the auto_loop_breaking_add_weld_constraints branch from a67e86b to b483930 Compare August 26, 2026 01:41

@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.

Initial comments addressed, thanks.

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


multibody/plant/multibody_plant.cc line 1526 at r2 (raw file):

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

grammar, clarity? I don't even know.

This last sentence has just accumulated so many ideas, and punctuation, and somehow manages to be totally baffling. Can I get a human to maybe split it into two or three sentences, and clarify?

Yikes, rewritten! Also the next few Claudisms.


multibody/plant/test/auto_closed_topology_test.cc line 8 at r2 (raw file):

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

nit Here is another sentence that overflows my registers. Maybe split it up, or make it into a bullet list of test objectives?

Done. I can't blame Claude for this one. I think it was a rebase error on my part that mangled the text.

/// Returns the number of ephemeral weld constraints that Finalize() added in
/// order to close topological loops. Each of these welds a shadow link to the
/// link it is a copy of; see SetEnableLoopTopology(). These are included in
/// num_constraints() and num_weld_constraints(), and are indistinguishable

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.

Done

Comment on lines +1666 to +1667
// N.B. We add the spec directly rather than calling AddWeldConstraint(),
// which is a pre-finalize-only API.

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.

TAMSI is deprecated and will be removed next week so this is moot.

@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:

@rpoyner-tri reviewed 14 files and all commit messages, made 2 comments, and resolved 1 discussion.
Reviewable status: 1 unresolved discussion, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on sherm1).


a discussion (no related file):
This is maybe a question for the whole dev branch. Does this implementation of loop breaking work on a graph where multiple separate loops get broken at a single link?

@sherm1
sherm1 force-pushed the auto_loop_breaking_add_weld_constraints branch from b483930 to 2983228 Compare September 3, 2026 18:03

@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.

This has now been rebased onto the now-merged #24902 so there is just one commit here now

@sherm1 made 2 comments and resolved 1 discussion.
Reviewable status: needs at least two assigned reviewers (waiting on rpoyner-tri).


a discussion (no related file):

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

This is maybe a question for the whole dev branch. Does this implementation of loop breaking work on a graph where multiple separate loops get broken at a single link?

Yes, and there is a test case ClosedTopologyTest.OneLinkCanHaveTwoShadows at auto_closed_topology_test.cc:542.

@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:@SeanCurtis-TRI for platform review per rotation (Tue) please
I marked this as priority:high in the hope that we can get this merged before RoboSim (the four bar example in the next PR depends on this one)

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

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sherm1
sherm1 force-pushed the auto_loop_breaking_add_weld_constraints branch from 2983228 to da8c857 Compare September 3, 2026 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

priority: high 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.

4 participants