[multibody] Adds loop-breaking to MbP topology

[sherm1]

This is the fifth installment in the PR train for #20225, following #21295.

What's here:

• Straightforward loop breaking using the "one-level-at-a-time" algorithm depicted here.
• Status return indicating whether the resulting forest can be used for dynamics
• Unit tests for that
• ChangeJointType() (added for testing convenience now but could be useful later)

What's not here:

• Special handling of massless bodies to avoid terminal massless bodies
• Combining LinkComposites onto a single Mobod
• Some standalone graph-walking algorithms needed by MbP
• Modifications to MbP to use this stuff

There are still no user-visible changes here and MbP still uses the old topology.

---

This change is 

[sherm1]

+@joemasterjohn for feature review at your convenience, please

This is much less code than the last few, but has a lot of complicated test cases. LMK if you think the tests are too much or not enough.

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

[joemasterjohn]

Checkpoint, moving on to the tests.

Reviewed 5 of 8 files at r2, all commit messages.
Reviewable status: 5 unresolved discussions, LGTM missing from assignee joemasterjohn, needs platform reviewer assigned, needs at least two assigned reviewers

---

multibody/topology/link_joint_graph.h line 427 at r2 (raw file):

  @pre the joint index is in range and the type name is of a registered or
    predefined joint type. */
  void ChangeJointType(JointIndex existing_joint_index,

BTW what if the user attempts to change the type to the same type of the existing joint. Should that be a no-op and NOT invalidate the Forest? Or do you think letting it happen?

---

multibody/topology/link_joint_graph.cc line 505 at r2 (raw file):

  Link& shadow_link = data_.links.back();
  shadow_link.primary_link_ = primary_link_index;
  shadow_link.add_joint_as_child(shadow_joint_index);  // Always a child.

per f2f: The primary link of this shadow link is not always the child of "shadow_joint". This function should include the is_reversed argument to check whether add the joint to the child list or the parent list of the shadow link.

---

multibody/topology/spanning_forest.h line 361 at r2 (raw file):

  // will be written to the graph.
  // @returns true if the forest can be used for anything
  //          false if it can't be used for dynamics.

nit: Maybe an @see to either the graph docs for BuildForest() or why_no_dynamics() might a nice reference for devs.

---

multibody/topology/spanning_forest.h line 426 at r2 (raw file):

  void SetBaseBodyChoicePolicy();

  // This not-yet-modeled Joint connects two Links both of which are already

nit: Maybe we can someway mention in these docs that the joint in question is the one with index loop_joint_index?

Code quote:

  // This not-yet-modeled Joint connects two Links both of which are already

---

multibody/topology/spanning_forest.cc line 678 at r2 (raw file):

  AddShadowMobod(split_parent ? joint.parent_link() : joint.child_link(),
                 joint.index());
  /* Doesn't change the remaining number of links to process. */

nit: This is nice to know, but a bit out of place here because there is no reference to num_unprocessed_links in this scope so I think the comment isn't needed.

Code quote:

  /* Doesn't change the remaining number of links to process. */

[sherm1]

Current batch of comments addressed.

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

---

multibody/topology/link_joint_graph.h line 427 at r2 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

BTW what if the user attempts to change the type to the same type of the existing joint. Should that be a no-op and NOT invalidate the Forest? Or do you think letting it happen?

Done, clarified in doc. I prefer the unconditional behavior.

---

multibody/topology/link_joint_graph.cc line 505 at r2 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

per f2f: The primary link of this shadow link is not always the child of "shadow_joint". This function should include the is_reversed argument to check whether add the joint to the child list or the parent list of the shadow link.

Done. Added a unit test also per f2f.

---

multibody/topology/spanning_forest.h line 361 at r2 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

nit: Maybe an @see to either the graph docs for BuildForest() or why_no_dynamics() might a nice reference for devs.

Done. Also added some documentation for dynamics_ok() and why_no_dynamics().

---

multibody/topology/spanning_forest.h line 426 at r2 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

nit: Maybe we can someway mention in these docs that the joint in question is the one with index loop_joint_index?

Done

---

multibody/topology/spanning_forest.cc line 678 at r2 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

nit: This is nice to know, but a bit out of place here because there is no reference to num_unprocessed_links in this scope so I think the comment isn't needed.

Done

[joemasterjohn]

Ok final pass through the tests done. feature. Most of the comments are nits about language and suggestions to make things more readable, but the tests look great.

+@jwnimmer-tri for platform review, please.

Reviewed 2 of 8 files at r2, 2 of 5 files at r3, 2 of 4 files at r4, 3 of 3 files at r5, all commit messages.
Reviewable status: 9 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform)

---

multibody/topology/link_joint_graph.cc line 505 at r2 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Done. Added a unit test also per f2f.

Awesome, thanks.

---

multibody/topology/spanning_forest.h line 168 at r5 (raw file):

  /** If dynamics_ok() returns `false`, returns a human-readable message
  explaining why. */

nit: maybe add ", otherwise returns an empty string."

Code quote:

  /** If dynamics_ok() returns `false`, returns a human-readable message
  explaining why. */

---

multibody/topology/test/spanning_forest_test.cc line 765 at r2 (raw file):

connection to World in the input ({3} and {9} get connected later). If we're
giving every Link its own mobilizers (rather than making composites from
welded-together ones) we expect this forest of 3 trees and 17 Mobods:

nit: Can you mention in the comment before the graph that the nodes of the graph are <reordered_mobod_index>{link_index}.

---

multibody/topology/test/spanning_forest_test.cc line 839 at r5 (raw file):

  EXPECT_EQ(graph.links(BodyIndex(1)).num_shadows(), 1);
  EXPECT_EQ(graph.links(BodyIndex(16)).primary_link(), 8);
  EXPECT_EQ(graph.links(BodyIndex(8)).num_shadows(), 1);

nit: Maybe a small comment to mention that for all of these loops the parent and child of the loop joint were at the same level in the tree, so the child link was always chosen to be split to preserve joint/mobilizer directionality. I think you mention it only for link 1 in the comment above, but that's a bit far from here.

Code quote:

  EXPECT_EQ(graph.links(BodyIndex(14)).primary_link(), 11);
  EXPECT_EQ(graph.links(BodyIndex(11)).num_shadows(), 1);
  EXPECT_EQ(graph.links(BodyIndex(15)).primary_link(), 1);
  EXPECT_EQ(graph.links(BodyIndex(1)).num_shadows(), 1);
  EXPECT_EQ(graph.links(BodyIndex(16)).primary_link(), 8);
  EXPECT_EQ(graph.links(BodyIndex(8)).num_shadows(), 1);

---

multibody/topology/test/spanning_forest_test.cc line 860 at r5 (raw file):

  for (auto& mobod : forest.mobods())
    EXPECT_EQ(mobod.level(), expected_level[mobod.index()]);

nit: is it also worth checking the link index to expected mobod index map as shown in the diagram?

---

multibody/topology/test/spanning_forest_test.cc line 884 at r5 (raw file):

            10       5  6

     ..........0..........   */

nit: the spacing here is a little too close to see that 5 and 6 are the free links. At first I thought there was a joint (5, 9).

Suggestion:

      1   2*
        3      4*  7
        8*       9
            10         5    6

     ..........0..........   */

---

multibody/topology/test/spanning_forest_test.cc line 1030 at r5 (raw file):

        =====                                =====          =====
        # 3 #              massful           # 2 # - Weld -># 4 #

nit: Should this say "loop constraint" instead to be consistent with the spanning tree nomenclature? Also 4* to visually indicate which is the shadow mobod.

Suggestion:

# 2 # - Loop constraint -># 4* #

---

multibody/topology/test/spanning_forest_test.cc line 1111 at r5 (raw file):

  {12}    {14}     {13}
  {125}   {147}    {136}
  {1256s} {1476ss}  where 6s and 6ss are shadows 1 and 2 of Link 6.

nit: missing {136} on the last line here.

---

multibody/topology/test/spanning_forest_test.cc line 1116 at r5 (raw file):

            {2}--->{5}--->{8}                  [2]-->[3]-->[4]  branch 1
           0 ^  3      6   # weld 0             ^

nit: should be refering to these as loop constraints, not welds.

Suggestion:

loop constraint 0

---

multibody/topology/test/spanning_forest_test.cc line 1131 at r5 (raw file):

  - Model Joint 8 is the added floating joint to World.
  - Link {8} is {6s} (shadow 1 of Link {6}); {9} is {6ss} (shadow 2).
  - Welds should be oriented 6->8 and 6->9.

This orientation means primary link -> shadow link, correct? Is the idea of "orientation" documented for loop constraints?
I think if we refer to these as "welds" the orientation might get confused for parent->child orientation in a weld joint.

Code quote:

  - Welds should be oriented 6->8 and 6->9.

[joemasterjohn]

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

---

multibody/topology/test/spanning_forest_test.cc line 1030 at r5 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

nit: Should this say "loop constraint" instead to be consistent with the spanning tree nomenclature? Also 4* to visually indicate which is the shadow mobod.

Sorry "4s" to indicate shadow, you used * to indicate massless.

[joemasterjohn]

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

---

multibody/topology/test/spanning_forest_test.cc line 1290 at r5 (raw file):

          +---->{1}------,                 +----->[1]{1}----->[2]{3}
          |              V                 |                   .
      {0} |             {3}            [0] |                   . Weld 0

nit: just to be more in line with the spanning tree nomenclature. Ditto below.

Suggestion:

 LoopConstraint 0

[sherm1]

@xuchenhan-tri volunteered to platform review this out of order (he did the previous few in the train). Feel free to keep or reassign.

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

[xuchenhan-tri]

+@xuchenhan-tri I'd be happy to review this, but might not be able to finish by EOD today.

Reviewed 1 of 8 files at r2, all commit messages.
Reviewable status: 10 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),xuchenhan-tri(platform)

[sherm1]

All feature review comments addressed, ptal @joemasterjohn.

Reviewable status: 1 unresolved discussion, LGTM missing from assignees jwnimmer-tri(platform),xuchenhan-tri(platform)

---

multibody/topology/spanning_forest.h line 168 at r5 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

nit: maybe add ", otherwise returns an empty string."

Done

---

multibody/topology/test/spanning_forest_test.cc line 765 at r2 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

nit: Can you mention in the comment before the graph that the nodes of the graph are <reordered_mobod_index>{link_index}.

Done (with a few more clarifications).

---

multibody/topology/test/spanning_forest_test.cc line 839 at r5 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

nit: Maybe a small comment to mention that for all of these loops the parent and child of the loop joint were at the same level in the tree, so the child link was always chosen to be split to preserve joint/mobilizer directionality. I think you mention it only for link 1 in the comment above, but that's a bit far from here.

Done. I added a more substantive discussion in the test comment, PTAL. Also the previous comment was out of date w.r.t. the updated splitting policy.

---

multibody/topology/test/spanning_forest_test.cc line 860 at r5 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

nit: is it also worth checking the link index to expected mobod index map as shown in the diagram?

Done

---

multibody/topology/test/spanning_forest_test.cc line 884 at r5 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

nit: the spacing here is a little too close to see that 5 and 6 are the free links. At first I thought there was a joint (5, 9).

Done

---

multibody/topology/test/spanning_forest_test.cc line 1030 at r5 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

Sorry "4s" to indicate shadow, you used * to indicate massless.

Done. I redrew that in the less fancy style of the other tests so I could use the same notation. I actually haven't been using the "4s" notation to mean a link whose index is 4, but rather the shadow of link 4, whose index could be anything. Hopefully easier to understand now, PTAL. Also the explanation didn't properly reflect the revised splitting policy.

---

multibody/topology/test/spanning_forest_test.cc line 1111 at r5 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

nit: missing {136} on the last line here.

Done

---

multibody/topology/test/spanning_forest_test.cc line 1116 at r5 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

nit: should be refering to these as loop constraints, not welds.

Done.

---

multibody/topology/test/spanning_forest_test.cc line 1131 at r5 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

This orientation means primary link -> shadow link, correct? Is the idea of "orientation" documented for loop constraints?
I think if we refer to these as "welds" the orientation might get confused for parent->child orientation in a weld joint.

Done, ptal.

---

multibody/topology/test/spanning_forest_test.cc line 1290 at r5 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

nit: just to be more in line with the spanning tree nomenclature. Ditto below.

Done. I looked through all the tests and made sure "weld" is only used to refer to joints. I used "Loop" rather than LoopConstraint which is too long for some of that ascii artwork.

[jwnimmer-tri]

I have enough time open to make progress, so I'll keep it assigned to me for platform.

(An important part of platform review on-call and PR size limits is so that we get a variety of eyeballs on large chunks of code as they proceed bit by bit. If we route around that and assign subject matter experts as platform reviewers out-of-order, then we lose some of the arms-length second opinion that's one of the upsides of our process. Xuchen is welcome to stay assigned for review here as a bonus feature reviewer, or to become familiar with the code, or etc.)

[sherm1]

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

---

multibody/topology/link_joint_graph.h line 107 at r6 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

If you need a macro that only declares, but not defines, then I suggest you create that macro (e.g., DRAKE_DECLARE_COPY_AND_MOVE_AND_ASSIGN). It's not the first time this has come up as a missing capability in drake_copyable.h.

SeanCurtis-TRI requested that I make the comments as close to the ones generated by the macro so that users wouldn't be misled to think there was something they have to worry about.

Mostly, I think you were mistaken to agree to that. This text in an internal-only class is not consumed in its rendered form, rather only onscreen for local Drake developers in their text editor, where the extra clutter is unhelpful and confusing. (Maybe Sean mistakenly thought the file was part of Doxygen, given the /** */ everywhere.)

But anyway, even taking Sean's request as a requirement, and even without creating the helpful macro, the answer should have been to copy-and-paste the existing doxygenated macro output, remove the = default, and call it done. The crux of my complaint here is the weird / irregular / confusing comments on every individual special member function, moreso than the overall approach. Even just reverting to a copy-past of the existing macro (sans = default) would be enough to satisfy me here.

Done using the new macro (from PR #21419)

[jwnimmer-tri]

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

---

-- commits line 5 at r7:
BTW You probably want to cherry-pick 619e305 back to life? Some style fixes got lost in the latest push.

[sherm1]

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

---

-- commits line 5 at r7:

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW You probably want to cherry-pick 619e305 back to life? Some style fixes got lost in the latest push.

Arrggh! Thanks. I'm mystified how that got lost :(

[sherm1]

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

---

-- commits line 5 at r7:

Previously, sherm1 (Michael Sherman) wrote…

Arrggh! Thanks. I'm mystified how that got lost :(

Looks like I missed my usual morning fetch! Will fix as soon as I can locate that commit.

[jwnimmer-tri]

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

---

-- commits line 5 at r7:

Previously, sherm1 (Michael Sherman) wrote…

Looks like I missed my usual morning fetch! Will fix as soon as I can locate that commit.

Try:

git fetch origin 619e305
git cherry-pick 619e305

At worst there is always git fetch upstream refs/reviewable/pr21349/r7

[sherm1]

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

---

-- commits line 5 at r7:

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Try:

git fetch origin 619e305
git cherry-pick 619e305

At worst there is always git fetch upstream refs/reviewable/pr21349/r7

Thanks, I found it on my laptop.

[jwnimmer-tri]

Reviewed 8 of 8 files at r9, all commit messages.
Reviewable status: 25 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @sherm1)

[sherm1]

Checkpoint

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

---

multibody/topology/link_joint_graph.h line 189 at r6 (raw file):

Previously, xuchenhan-tri wrote…

nit missing period

Done

---

multibody/topology/link_joint_graph.h line 244 at r6 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW I don't follow what this exception text means. I guess this a constraint on the flags parameter? If so, then maybe clearer would be in the docs for flags to list which flags are acceptable / supported.

Done. The enum does document the Shadow flag as "internal use only". I clarified the documentation here.

---

multibody/topology/link_joint_graph.h line 253 at r6 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

typo

Done. Changed to model_instance_index here and in the implementation.

---

multibody/topology/link_joint_graph.h line 353 at r6 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW "during modeling" is pretty vague. Atop the file we have a nice explanation of "ephemeral" or "as built". That kind of wording seems much more clear to me, and so is probably worth a mention or xref here.

Done

---

multibody/topology/link_joint_graph.h line 359 at r6 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

After the Forest has been built ...

This begs the question is what happens before the forest has been built? Does the function crash, or return a lie, or ...?

Ditto for all the other functions below whose API contract has this guard condition but doesn't directory say whether it's a pre-condition, or something else.

Done

---

multibody/topology/link_joint_graph.h line 363 at r6 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

"See below? I'm not sure what that's pointing me at. (Maybe the inlines used to be at the bottom of this file? They are in a different file now.)

Done

---

multibody/topology/link_joint_graph.h line 423 at r6 (raw file):

Previously, xuchenhan-tri wrote…

nit missing period

Done

---

multibody/topology/link_joint_graph.h line 530 at r7 (raw file):

Previously, xuchenhan-tri wrote…

nit odd return type

Done

[jwnimmer-tri]

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

[sherm1]

Checkpoint 2. Still some more changes coming.

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

---

multibody/topology/link_joint_graph.cc line 423 at r6 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW If you prefer, it's very easy to convince clang-format to line-wrap this better:

    throw std::logic_error(fmt::format(
        "{}(): can't change type of joint {} (in model instance {}) from {} "
        "to {} because it connects static link {} to World; only a weld is "
        "permitted for a static link.",
        __func__, joint.name(), joint.model_instance(),
        joint_types(joint.type_index()).name, name_of_new_type,
        static_link_name));

In general we can use 70 characters per line of string literal (not 58), by putting the fmt::format up onto the prior line. Once you get it up there the first time, clang will leave it alone.

Done, TIL! Beautiful.

---

multibody/topology/link_joint_graph.cc line 487 at r6 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I am not sure why this comment is here. What are we supposed to learn from this sentence that wasn't already explained much more carefully in the header file? I'm not following.

Done. Historical reason only I guess :)

---

multibody/topology/link_joint_graph.cc line 497 at r6 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

My reading of the TODO above is that a user might accidentally trip this DEMAND, through no fault of their own? If so, then we can't use DEMAND (which is only for Drake developer mistakes), we need to use THROW or similar so that we don't abort their entire process.

Done This could only trigger if the user had a link they had named something like mylink$1. I suppose that is possible. I don't suppose we have any "internal use only" characters? I could modify this to throw with a message telling them they can't use Drake with links of that form, but I think I might as well put it in a "uniquifier" loop and get rid of the failure possibility altogether.

---

multibody/topology/link_joint_graph.cc line 498 at r6 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW Do we have sufficient ASSERT checks of num_user_links versus this new link index? This might be one place we could check that.

Done. Probably had plenty already but no harm in an extra check.

---

multibody/topology/link_joint_graph_inlines.h line 35 at r6 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

See https://drake.mit.edu/styleguide/cppguide.html#Inline_Functions.

This function does not match any of the 5 prongs that allow being inline in a header. (The only prong it might fall under is number 5, and we're already eating a function call so that option is mooted.)

Done.

---

multibody/topology/link_joint_graph_inlines.h line 66 at r6 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Ditto https://drake.mit.edu/styleguide/cppguide.html#Inline_Functions

Done

---

multibody/topology/link_joint_graph_link.h line 151 at r6 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

"pushing the limit" is generous -- there is no gray area here, this is forbidden by our GSG because it's neither O(1) nor shorter than 5 lines.

Done.

---

multibody/topology/spanning_forest.cc line 662 at r6 (raw file):

Previously, xuchenhan-tri wrote…

BTW, question, will we end up here if there exists other places to break the loop that might not be the optimal in balancing the branch lengths? In other words, do we ever only give up on a producing a forest that allow dynamics when it's absolutely impossible?

My current reading from the unit tests is no to the first question and yes to the second, but it would be nice if you could explicitly confirm that understanding.

With most of the massless link-handling code stubbed out as it is in this PR, there are definitely cases where dynamics would have been possible if we were smarter (several massless bodies in a row is the main failing). Let's revisit this question with the full heuristic in place. I can say that it won't fail on any case that can be done without backtracking. I'm not 100% certain there aren't cases where backtracking could save the day, but I don't know of any yet.

---

multibody/topology/spanning_forest.cc line 674 at r6 (raw file):

Previously, xuchenhan-tri wrote…

nit, I don't think this is necessarily true. We could still land here even if both links are massless.

Done. Good catch -- that was leftover from an earlier policy.

---

multibody/topology/spanning_forest.cc line 709 at r6 (raw file):

Previously, xuchenhan-tri wrote…

I see, thanks.

Done (both: moved to just before use and dumped the dangerous reference)

---

multibody/topology/test/spanning_forest_test.cc line 1312 at r6 (raw file):

Previously, xuchenhan-tri wrote…

BTW, by "body" here, do you mean mobod?

Done

[sherm1]

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

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

The topology query API here that passes strings by-const-reference (like const std::string& type and const std::string& name) doesn't follow modern C++20 convention and is unnecessarily inefficient.

Function arguments that are just lookup keys (and not retained by the callee) should be passed as std::string_view, so that they don't force the caller to allocate a short-lived string. The member fields should be using our helpers like string_unordered_map to enable transparent lookups of a string_view.

Done (using string_view now)

---

[jwnimmer-tri]

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

---

multibody/topology/link_joint_graph_link.h line 151 at r6 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Done.

Moving it to be non-inline is an important improvement, but the defect was about snake_case vs CamelCase. This needs to be renamed something like ClearModel, to reinforce that isn't not a getter/setter.

[sherm1]

Almost done now. Fixed CI, responded to all comments. Will make a final pass to catch any further misuse of is_valid. Will discuss premature use of /** in today's 1:1 call.

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

---

multibody/topology/link_joint_graph.h line 29 at r6 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I don't agree (yet?). We can cover this in our next phone chat.

Let's resolve this today.

---

multibody/topology/link_joint_graph.h line 401 at r6 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Otherwise the returned index will be invalid.

Using is_valid() as a pseudo-optional violates the API requirements of TypeSafeIndex docs:

While there is the concept of an "invalid" index, this only exists to support default construction where appropriate (e.g., using indices in STL containers)

To remind the caller that null-ness checks are required, we must return optional<JointIndex> here.

Done

---

multibody/topology/link_joint_graph.h line 512 at r6 (raw file):

Previously, xuchenhan-tri wrote…

BTW, I'm not clear on what it means for the primary to be the "Weld (constraint)'s parent". Could you elaborate?

Done, ptal

---

multibody/topology/link_joint_graph_link.h line 66 at r6 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Using is_valid() as a pseudo-optional violates the API requirements of TypeSafeIndex docs.

To remind us that null-ness checks are required, we must spell optional<LinkCompositeIndex> as the return type of the accessor.

Done

---

multibody/topology/link_joint_graph_link.h line 114 at r6 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW Here's where we need optional<>, per the above discussion.

Done

---

multibody/topology/link_joint_graph_link.h line 151 at r6 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Moving it to be non-inline is an important improvement, but the defect was about snake_case vs CamelCase. This needs to be renamed something like ClearModel, to reinforce that isn't not a getter/setter.

Done. Derp! (To quote Eric)

---

multibody/topology/link_joint_graph_link.h line 155 at r6 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Here's what I wrote about the same topic:

I don't understand whatever heuristic is being used here for which member fields to clear and which ones not to clear. It seems like this is doing something other than "reset back to a default-constructed link" but I'm not sure what it's supposed to be. Probably the lack of any API comment is the underlying problem. Is it just supposed to clear the "below here" stuff, maybe? But then why do we lose this line? What are the invariants that relate all of these member fields to each other?

Done. Explained in an API comment and clarified with some reordering and a couple of assertions. primary_link_ is the most confusing since it is set at construction to "self" except for shadow links. And ClearModel() isn't called on shadow links. PTAL and let me know if it is clear now.

[jwnimmer-tri]

platform.

Reviewed 7 of 7 files at r12, all commit messages.
Reviewable status: 3 unresolved discussions

---

multibody/topology/link_joint_graph.cc line 332 at r12 (raw file):

  if (std::optional<JointIndex> existing_joint_index =
          MaybeGetJointBetween(parent_link_index, child_link_index);
      existing_joint_index.has_value()) {

BTW Possibly you prefer this existing_joint_index.has_value() written out longhand to reinforce what's happening here, but if-test will still do exactly what you want if we remove it, and would be slightly more conventional.

---

multibody/topology/link_joint_graph_link.h line 67 at r12 (raw file):

  bool is_anchored() const {
    return is_world() || is_static() ||
           (composite().has_value() && composite() == LinkCompositeIndex(0));

BTW I don't think we need the has_value guard here anymore? If it's nullopt, then it won't compare as equal to the zero index.

[jwnimmer-tri]

Reviewable status: 3 unresolved discussions

---

multibody/topology/link_joint_graph.h line 29 at r6 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Let's resolve this today.

Yup, we'll discuss it, but also for this PR I think it's fair to consider this a pre-existing condition that need not block merging, so long as we fix it (if necessary) in the very next PR.

[sherm1]

Reviewable status: complete! all discussions resolved, LGTM from assignees jwnimmer-tri(platform),joemasterjohn,xuchenhan-tri(platform)

---

multibody/topology/link_joint_graph.h line 29 at r6 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Yup, we'll discuss it, but also for this PR I think it's fair to consider this a pre-existing condition that need not block merging, so long as we fix it (if necessary) in the very next PR.

Good, thanks.

---

multibody/topology/link_joint_graph.cc line 332 at r12 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW Possibly you prefer this existing_joint_index.has_value() written out longhand to reinforce what's happening here, but if-test will still do exactly what you want if we remove it, and would be slightly more conventional.

Do you mean writing it just "existing_joint_index" without the ".has_value()"? I know that will work but I think writing it out is clearer here. The preceding local variable declaration buries the conditional.

---

multibody/topology/link_joint_graph_link.h line 67 at r12 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW I don't think we need the has_value guard here anymore? If it's nullopt, then it won't compare as equal to the zero index.

Done

[jwnimmer-tri]

Reviewed 3 of 3 files at r13, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignees jwnimmer-tri(platform),joemasterjohn,xuchenhan-tri(platform)

---

multibody/topology/link_joint_graph.cc line 332 at r12 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Do you mean writing it just "existing_joint_index" without the ".has_value()"? I know that will work but I think writing it out is clearer here. The preceding local variable declaration buries the conditional.

No, I mean lose everything after the semicolon. When functions return an optional there's a canonical spelling for call sites:

// Some helper.
std::optional<int> maybe();

// Do a thing.
int stuff() {
  if (std::optional<int> x = maybe()) {
    return *x;
  }
  return -1;
}

When the if declares a single scoped local var, and there is no extra condition, the if-clause is the bool operator called on the scoped var. Writing out the guard is redundant.

[sherm1]

I changed the policy on shadow body name collision resolution to match the one we already use for floating joint names: just keep prepending underscores to the link name until it's good. I also added a unit test to make sure it stays nailed down.

Reviewable status: complete! all discussions resolved, LGTM from assignees jwnimmer-tri(platform),joemasterjohn,xuchenhan-tri(platform)

---

multibody/topology/link_joint_graph.cc line 332 at r12 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

No, I mean lose everything after the semicolon. When functions return an optional there's a canonical spelling for call sites:

// Some helper.
std::optional<int> maybe();

// Do a thing.
int stuff() {
  if (std::optional<int> x = maybe()) {
    return *x;
  }
  return -1;
}

When the if declares a single scoped local var, and there is no extra condition, the if-clause is the bool operator called on the scoped var. Writing out the guard is redundant.

Oh, that's very nice, gracias! I didn't know that worked. TIL, and TI made the change.

[jwnimmer-tri]

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

[sherm1]

Thanks, Joe, Xuchen, and Jeremy for your valuable contributions! Much appreciated. Onwards ...

Reviewable status: complete! all discussions resolved, LGTM from assignees jwnimmer-tri(platform),joemasterjohn,xuchenhan-tri(platform)