[multibody topology] Handle merged composites #21755
Conversation
sherm1
added
status: do not merge
priority: medium
status: do not review
release notes: none
sherm1
force-pushed
the
mbp_topology_merge_composites
branch
2 times, most recently
from
51e418c to
2021c22
Compare
There was a problem hiding this comment.
+@joemasterjohn for feature review, please. This is the last hard one!
The PR is mostly comments and unit tests, but the algorithm mod to treat composites like single bodies is tricky. I've attempted to lay it out in words and then tie the code to that -- please let me know what's not clear. Also if there is anything I can do to make the unit tests easier to follow, please LMK. I am very grateful for your willingness to tackle these -- thanks!
Reviewable status: LGTM missing from assignee joemasterjohn, needs platform reviewer assigned, needs at least two assigned reviewers
There was a problem hiding this comment.
BTW @joemasterjohn I think it would be easiest to start with spanning_forest.cc so you can see the algorithm changes. The other files have misc helper functions and the like that won't make much sense out of the algorithmic context. The unit tests might also provide some context.
Reviewable status: LGTM missing from assignee joemasterjohn, needs platform reviewer assigned, needs at least two assigned reviewers
There was a problem hiding this comment.
Reviewable status: LGTM missing from assignee joemasterjohn, needs platform reviewer assigned, needs at least two assigned reviewers
a discussion (no related file):
FYI @sherm1, this might need to wait at least till next week. Our highest priority with @joemasterjohn right now is to land margin to solve issues in our Anzu sims. Consider another reviewer if time happens to be a constraint.
There was a problem hiding this comment.
-@joemasterjohn +@SeanCurtis-TRI (thanks!) for load balancing
Reviewable status: LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers
a discussion (no related file):
Previously, amcastro-tri (Alejandro Castro) wrote…
FYI @sherm1, this might need to wait at least till next week. Our highest priority with @joemasterjohn right now is to land margin to solve issues in our Anzu sims. Consider another reviewer if time happens to be a constraint.
Sean kindly agreed to take a shot at this to offload Joe.
There was a problem hiding this comment.
@SeanCurtis-TRI I updated the merge policy per f2f (2nd commit, r3)
Reviewable status: LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @sherm1)
There was a problem hiding this comment.
Alright, first pass. I'm going to hold my stamp until I've had some of my questions answered, just to make sure I feel sufficiently confident in my review.
Apologies if I'm asking questions whose answers are just a dozen lines outside of the lies I reviewed.
Reviewed 6 of 9 files at r1, 2 of 3 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: 16 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @sherm1)
a discussion (no related file):
BTW I'm curious about the two vectors that are coordinated to define the properties of a link composite. Why not create a vector of a single struct?
struct LinkComposite {
std::vector<BodyIndex> bodies;
bool is_massless{};
};It removes the burden of coordinating two vectors, of course. But I realize that's the end all and be all of design.
multibody/topology/link_joint_graph.cc line 622 at r2 (raw file):
std::vector<BodyIndex>{maybe_composite_link.index()}); data_.link_composite_is_massless.push_back( maybe_composite_link.treat_as_massless());
BTW You've got "treat as massless" and you have "is_massless" is there a subtle distinction? If not, unifying the language would be nice.
multibody/topology/link_joint_graph.cc line 626 at r2 (raw file):
data_.link_composites[*existing_composite].push_back(new_link.index()); // If any Link in the composite has mass, then the whole thing is massful. if (!new_link.treat_as_massless())
BTW Your comment is in terms of being massful, the code is in terms of being massless. The negation of the comment can make things a bit hard to read. Perhaps this would read better? (Too bad there's no &&= operator.)
// For the composite to be massless, all links must be massless.
data_.link_composite_is_massless[*existing_composite] &=
new_link.treat_as_massless();I have later comments about possibly reversing the semantics of "massless" becoming "has_mass". So, filter this comment through that one.
multibody/topology/spanning_forest.h line 422 at r2 (raw file):
// output parameter `J_out` (cleared on entry) on return // contains the set of Joints that should be modeled at the next level. // @pre the pointers are non-null and J_in is not empty. On return,
nit style; missing back ticks.
Suggestion:
`J_in`multibody/topology/spanning_forest.h line 544 at r2 (raw file):
// of composites, either explicitly or via inheritance from the global // settings. bool merge_link_composites(ModelInstanceIndex index) const {
nit: This name is ambiguous -- it feels like an imperative statement. Perhaps a prefixed requests_ or some such thing would help disambiguate? You have other related concepts that are prefixed with stronger predicates like "should_".
multibody/topology/spanning_forest.h line 563 at r2 (raw file):
// we will merge parent and child; return true. // - Otherwise we're not combining; return false; bool should_be_unmodeled_weld_in_composite(const Joint& joint) {
BTW I found the name of this hyper specific, to the point of sowing confusion. The predicate in practice is: is_weld && not must_be_modeled && some_model_instance_wants_composites_merged.
The name suffers from some colloquial compression. At my first read, I interpreted it as, "given a weld joint, should it be unmodeled in a composite?" (but it gets invoked on all joint types). Or, should the given joint be converted to an "unmodeled weld" in a composite (whatever that would mean).
How harmful would it be if the name were simpler? Simply, "should_omit_from_composite" or "should_exclude_from_composite" or "should_be_unmodeled_in_composite" or some such thing? It just seems emphasizing "weld" obscures the semantics.
------------------------- follow up -----------------
Related, but different.
The "unmodeled" is akin to "massless" as being negative, but the function is usually negated when used as a predicate; another instance of the double negative. Could we flip the semantics and name? should_include_in_composite()? Or some such thing.
multibody/topology/spanning_forest.h line 555 at r3 (raw file):
// decide to merge them, the Joint won't be modeled at all since it will be // interior to the composite. Here is the policy: // - If the joint is not a weld, we're not building a composite;
BTW You've simplified the code by throwing out the nots. I feel that helped the code readability and the documentation could likewise benefit.
To return true, the following must all be true:
- The joint must be a weld,
- the joint's model instance must request optimization, and
- the joint has not demanded modeling.
(Wrapped up in the appropriate nomenclature.)
multibody/topology/spanning_forest.cc line 410 at r2 (raw file):
/* Should be nothing left now except unjointed (single) Links. We'll attach with either a floating joint or a weld joint depending on modeling options. If a weld and we're optimizing composites the Link will just join the World
nit punctuation (missing comma)
... we're optimizing composites, the Link...
^
multibody/topology/spanning_forest.cc line 457 at r2 (raw file):
2 If we encounter a "treat as massless" Link (or massless merged LinkComposite) its Mobod can't be a terminal body of a branch. In that case we keep growing that branch until we can end with something massful.
nit: This now seems stale. It is now possible to end a branch with a massless body. It just gets tagged as such.
Code quote:
ase we keep growing that branch until we can end with something massful.multibody/topology/spanning_forest.cc line 566 at r2 (raw file):
/* If the outboard link is already modeled, this is a loop-closing joint. (A.2) */ if (link_is_already_in_forest(j_level_outboard_link_ordinal)) {
BTW We know that the newly acquired outboard ordinal is in the forest. What I couldn't convince myself of is that we likewise know that the newly acquired inboard ordinal is likewise in the forest. My imagination populated a world in which FindInboardOutboardLinks() reversed in/outboard to provide the link that was already known to be in the forest (the qualifying property of j_level). How do I know that the returned inboard link is defnitely in the forest at this point?
---------- Follow up from reading further ----------
I see that FindInboardMobod is able to look and see which joint link has a "valid mobod". Presumably, valid mobod implies that the link is in the spanning forest (associated with the Mobod instantiated for the forest).
I'll defer to you if it's worth putting a note here at the call site.
multibody/topology/spanning_forest.cc line 721 at r2 (raw file):
const BodyIndex outboard_link_index = FindOutboardLink(inboard_mobod_index, joint); if (!link_by_index(outboard_link_index).treat_as_massless()) return true;
BTW This comes up a lot. "treat as massless" is a negative statement. It is typically negated as a condition. Again, negation of negation is always harder to read. Why not simply flip its semantics and name?
multibody/topology/spanning_forest.cc line 970 at r2 (raw file):
if (link_is_already_in_forest(follower_link_ordinal)) { const Link& inboard_link = links(mobod->link_ordinal()); const Link& follower_link = links(follower_link_ordinal);
nit You use follower_link inside and outside of this block, so, rather than defining it twice, maybe you should pull it out of the branch and do it once?
multibody/topology/spanning_forest.cc line 985 at r2 (raw file):
const Joint& joint = joints(joint_ordinal); if (!should_be_unmodeled_weld_in_composite(joint)) { outboard_joint_indexes->push_back(joint_index);
nit: It's not clear to me that this joint should arbitrarily be added to outboard_joint_indexes it could already be modeled. As I see it, there's one of two principles in play here:
- The joint indicated by
joint_indexis, by construction, not modeled. - It doesn't matter if it's already modeled; the consumer of this list of joints filters based on that criteria.
Either way, some note here would help me. But I interpret the output set as being those joints which have one body in the forest and I'm not sure how to guarantee that at this point.
multibody/topology/spanning_forest_mobod.cc line 50 at r2 (raw file):
void SpanningForest::Mobod::Swap(Mobod& other) { std::swap(follower_link_ordinals_, other.follower_link_ordinals_); std::swap(has_massful_follower_link_, other.has_massful_follower_link_);
nit Why introduce Swap at all? You have move semantics; std::swap works perfectly well on two instances of Mobod and then you can erase the code and the warning that adding new members requires change to how Swap is implemented.
(I tried this on the one invocation of Swap() I could find in SpanningForest and its unit tests were perfectly happy.
multibody/topology/test/spanning_forest_test.cc line 1056 at r3 (raw file):
EXPECT_EQ( graph.link_composites(LinkCompositeIndex(1)), (std::vector<BodyIndex>{BodyIndex(13), BodyIndex(1), BodyIndex(4)}));
BTW This appears a bit fragile -- hardcoded ordering of indices. I note that in the next test, the body indices aren't ordered in increasing order. So, there's some arcane knowledge about the ordering and, unless the order is being tested, you might consider the following:
EXPECT_THAT(graph.link_composites(LinkCompositeIndex(0)),
testing::UnorderedElementsAre(BodyIndex(0), BodyIndex(5),
BodyIndex(7), BodyIndex(12)));Presumably, order does matter because the first entry indicates the "active" link? I'm not sure how important that is. Perhaps there's some higher documentation indicating the importance of defining and comparing composites that way and I missed it because I'm looking through a tube at this code.
Perhaps if you had a utility function for creating the expected LinkComposite that could receive the documentation?
Feel free to ignore.
multibody/topology/test/spanning_forest_test.cc line 1984 at r3 (raw file):
EXPECT_EQ(ssize(forest.mobods()), 4); // Only I5 should determin whether we merge {1} and {2}. Set the merge
nit typo
Suggestion:
determine
There was a problem hiding this comment.
Reviewable status: 15 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)
multibody/topology/spanning_forest_mobod.cc line 50 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit Why introduce
Swapat all? You have move semantics;std::swapworks perfectly well on two instances ofMobodand then you can erase the code and the warning that adding new members requires change to howSwapis implemented.(I tried this on the one invocation of
Swap()I could find inSpanningForestand its unit tests were perfectly happy.
What??? That's fantastic. I hated that function. Since that's unrelated to this PR I made a tiny one with just this and assigned it to you for sole review. See #21762. (Please review that first and then I'll rebase this one on it after it merges)
sherm1
force-pushed
the
mbp_topology_merge_composites
branch
from
b50a9c8 to
55c42a8
Compare
There was a problem hiding this comment.
Reviewed 4 of 4 files at r4, all commit messages.
Reviewable status: 15 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers
sherm1
force-pushed
the
mbp_topology_merge_composites
branch
from
55c42a8 to
2f89600
Compare
There was a problem hiding this comment.
Reviewable status: 12 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)
a discussion (no related file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW I'm curious about the two vectors that are coordinated to define the properties of a link composite. Why not create a vector of a single struct?
struct LinkComposite { std::vector<BodyIndex> bodies; bool is_massless{}; };It removes the burden of coordinating two vectors, of course. But I realize that's the end all and be all of design.
Done. This is clearly better -- I added the is_massless flag later and was lazy. r5 has this fix.
multibody/topology/spanning_forest.h line 544 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: This name is ambiguous -- it feels like an imperative statement. Perhaps a prefixed
requests_or some such thing would help disambiguate? You have other related concepts that are prefixed with stronger predicates like "should_".
Done
multibody/topology/test/spanning_forest_test.cc line 1984 at r3 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit typo
Done
There was a problem hiding this comment.
Lunch checkpoint
Reviewable status: 5 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)
multibody/topology/link_joint_graph.cc line 622 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW You've got "treat as massless" and you have "is_massless" is there a subtle distinction? If not, unifying the language would be nice.
Done. I switched to using is_massless for the link flag and link method. I'm keeping must_treat_as_massless() for the loophole that a massless body welded to a massful body has effective mass.
I've tried to nuke double negatives where possible. But "massless body" is a term of art for an unusual case and shouldn't be thought of as negative. "massful" is a neologism here so brings its own style of awkwardness. "has_mass" would be OK but is misleading when applied to a massless link that is welded to something massful - it still doesn't have mass but has something like effective mass.
multibody/topology/link_joint_graph.cc line 626 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW Your comment is in terms of being massful, the code is in terms of being massless. The negation of the comment can make things a bit hard to read. Perhaps this would read better? (Too bad there's no
&&=operator.)// For the composite to be massless, all links must be massless. data_.link_composite_is_massless[*existing_composite] &= new_link.treat_as_massless();
I have later comments about possibly reversing the semantics of "massless" becoming "has_mass". So, filter this comment through that one.
Done
multibody/topology/spanning_forest.h line 422 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit style; missing back ticks.
Done
multibody/topology/spanning_forest.h line 563 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW I found the name of this hyper specific, to the point of sowing confusion. The predicate in practice is: is_weld && not must_be_modeled && some_model_instance_wants_composites_merged.
The name suffers from some colloquial compression. At my first read, I interpreted it as, "given a weld joint, should it be unmodeled in a composite?" (but it gets invoked on all joint types). Or, should the given joint be converted to an "unmodeled weld" in a composite (whatever that would mean).
How harmful would it be if the name were simpler? Simply, "should_omit_from_composite" or "should_exclude_from_composite" or "should_be_unmodeled_in_composite" or some such thing? It just seems emphasizing "weld" obscures the semantics.
------------------------- follow up -----------------
Related, but different.
The "unmodeled" is akin to "massless" as being negative, but the function is usually negated when used as a predicate; another instance of the double negative. Could we flip the semantics and name?
should_include_in_composite()? Or some such thing.
Done. This is a hard thing to name but I think should_merge_parent_and_child() is easier to grasp. WDYT?
multibody/topology/spanning_forest.cc line 410 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit punctuation (missing comma)
... we're optimizing composites, the Link... ^
Done
multibody/topology/spanning_forest.cc line 457 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: This now seems stale. It is now possible to end a branch with a massless body. It just gets tagged as such.
Done
multibody/topology/spanning_forest.cc line 721 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW This comes up a lot. "treat as massless" is a negative statement. It is typically negated as a condition. Again, negation of negation is always harder to read. Why not simply flip its semantics and name?
OK. Hopefully slightly better with just is_massless(). See my earlier comment for why I want to stick with "massless".
multibody/topology/spanning_forest.cc line 970 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit You use
follower_linkinside and outside of this block, so, rather than defining it twice, maybe you should pull it out of the branch and do it once?
Done
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)
multibody/topology/spanning_forest.h line 555 at r3 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW You've simplified the code by throwing out the nots. I feel that helped the code readability and the documentation could likewise benefit.
To return true, the following must all be true:
- The joint must be a weld,
- the joint's model instance must request optimization, and
- the joint has not demanded modeling.
(Wrapped up in the appropriate nomenclature.)
Done
There was a problem hiding this comment.
Reviewed 1 of 5 files at r5, 9 of 9 files at r6, all commit messages.
Reviewable status: 3 unresolved discussions, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @sherm1)
multibody/topology/link_joint_graph.cc line 622 at r2 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
Done. I switched to using
is_masslessfor the link flag and link method. I'm keepingmust_treat_as_massless()for the loophole that a massless body welded to a massful body has effective mass.I've tried to nuke double negatives where possible. But "massless body" is a term of art for an unusual case and shouldn't be thought of as negative. "massful" is a neologism here so brings its own style of awkwardness. "has_mass" would be OK but is misleading when applied to a massless link that is welded to something massful - it still doesn't have mass but has something like effective mass.
Generally, I'm happy to accept your judgement; I just wanted to make sure it had been well considered.
re: treat_as_massless(). The documentation for this method indicates clearly what -- a massless link isn't treated as massless if it's part of a massful composite. The why is less clear. It might be worth adding to that documentation. My best understanding is that it's the difference between is_massless(L) and is_massless(L+). Sometimes we want to know about the link itself and sometimes we want to know about the composite that the link is a member of (with the understanding that it always belongs to a composite, even if its a composite consistent of a single link). But the point is that the link represents a composite.
Perhaps it would also be more clear if, instead of treat_as_massless(), the function were named links_composite_is_massless()? That renaming only makes sense if there's a clear understanding that a link is its own composite.
So, I'd vote for either some additional documentation on treat_as_massless() explaining the why and/or a rename emphasizing accessing a composite link property via an individual representative link. Otherwise, it feels like we're fudging a property of the link itself.
multibody/topology/spanning_forest.h line 563 at r2 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
Done. This is a hard thing to name but I think should_merge_parent_and_child() is easier to grasp. WDYT?
That reads very clear. The action based on the predicate being true is to act, so it really improves the call sites.
multibody/topology/test/spanning_forest_test.cc line 1056 at r3 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW This appears a bit fragile -- hardcoded ordering of indices. I note that in the next test, the body indices aren't ordered in increasing order. So, there's some arcane knowledge about the ordering and, unless the order is being tested, you might consider the following:
EXPECT_THAT(graph.link_composites(LinkCompositeIndex(0)), testing::UnorderedElementsAre(BodyIndex(0), BodyIndex(5), BodyIndex(7), BodyIndex(12)));Presumably, order does matter because the first entry indicates the "active" link? I'm not sure how important that is. Perhaps there's some higher documentation indicating the importance of defining and comparing composites that way and I missed it because I'm looking through a tube at this code.
Perhaps if you had a utility function for creating the expected
LinkCompositethat could receive the documentation?Feel free to ignore.
In this last pass, I particularly noted the documentation about the "active" link and its significance. However, the remaining order has no promise. So, a fully robust test would both confirm the first value and then not care about the order of the remaining.
There's very little likelihood that the algorithm implementation will change nor that the test cases will change significantly, so the brittleness may only be a problem of principle.
The "ignore this" side of the scale, has definitely gained more weight.
sherm1
force-pushed
the
mbp_topology_merge_composites
branch
from
ee98a28 to
7155381
Compare
There was a problem hiding this comment.
Reviewable status: needs at least two assigned reviewers
multibody/topology/spanning_forest.cc line 566 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW We know that the newly acquired outboard ordinal is in the forest. What I couldn't convince myself of is that we likewise know that the newly acquired inboard ordinal is likewise in the forest. My imagination populated a world in which
FindInboardOutboardLinks()reversed in/outboard to provide the link that was already known to be in the forest (the qualifying property ofj_level). How do I know that the returned inboard link is defnitely in the forest at this point?---------- Follow up from reading further ----------
I see that
FindInboardMobodis able to look and see which joint link has a "valid mobod". Presumably, valid mobod implies that the link is in the spanning forest (associated with the Mobod instantiated for the forest).I'll defer to you if it's worth putting a note here at the call site.
Done
multibody/topology/spanning_forest.cc line 985 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: It's not clear to me that this joint should arbitrarily be added to
outboard_joint_indexesit could already be modeled. As I see it, there's one of two principles in play here:
- The joint indicated by
joint_indexis, by construction, not modeled.- It doesn't matter if it's already modeled; the consumer of this list of joints filters based on that criteria.
Either way, some note here would help me. But I interpret the output set as being those joints which have one body in the forest and I'm not sure how to guarantee that at this point.
Done. PTAL -- I tightened this up substantially with docs and a DEMAND.
multibody/topology/test/spanning_forest_test.cc line 1056 at r3 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
In this last pass, I particularly noted the documentation about the "active" link and its significance. However, the remaining order has no promise. So, a fully robust test would both confirm the first value and then not care about the order of the remaining.
There's very little likelihood that the algorithm implementation will change nor that the test cases will change significantly, so the brittleness may only be a problem of principle.
The "ignore this" side of the scale, has definitely gained more weight.
Yeah, it's important that the active link comes first. The order of the others doesn't matter but it's unlikely that a future change will process them out of order. If so we can revise the test then.
There was a problem hiding this comment.
+@xuchenhan-tri for platform review per rotation please (or punt if you can't fit it in today)
Reviewable status: LGTM missing from assignee xuchenhan-tri(platform)
There was a problem hiding this comment.
Reviewed 2 of 2 files at r7, all commit messages.
Reviewable status: LGTM missing from assignee xuchenhan-tri(platform)
sherm1
force-pushed
the
mbp_topology_merge_composites
branch
from
7155381 to
4a114e2
Compare
There was a problem hiding this comment.
Reviewable status: LGTM missing from assignee xuchenhan-tri(platform)
multibody/topology/link_joint_graph.cc line 622 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
Generally, I'm happy to accept your judgement; I just wanted to make sure it had been well considered.
re:
treat_as_massless(). The documentation for this method indicates clearly what -- a massless link isn't treated as massless if it's part of a massful composite. The why is less clear. It might be worth adding to that documentation. My best understanding is that it's the difference betweenis_massless(L)andis_massless(L+). Sometimes we want to know about the link itself and sometimes we want to know about the composite that the link is a member of (with the understanding that it always belongs to a composite, even if its a composite consistent of a single link). But the point is that the link represents a composite.Perhaps it would also be more clear if, instead of
treat_as_massless(), the function were namedlinks_composite_is_massless()? That renaming only makes sense if there's a clear understanding that a link is its own composite.So, I'd vote for either some additional documentation on
treat_as_massless()explaining the why and/or a rename emphasizing accessing a composite link property via an individual representative link. Otherwise, it feels like we're fudging a property of the link itself.
Done. I don't generally support the abstraction that a link is its own composite (singleton links aren't present in the LinkComposites collection, e.g.). Renamed to "link_and_its_composite_are_massless()" with a comment noting that there might not be a composite. Not perfect, but I think it comes closer to your suggestion.
There was a problem hiding this comment.
Reviewed 4 of 4 files at r8, all commit messages.
Reviewable status: LGTM missing from assignee xuchenhan-tri(platform)
multibody/topology/link_joint_graph.cc line 622 at r2 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
Done. I don't generally support the abstraction that a link is its own composite (singleton links aren't present in the LinkComposites collection, e.g.). Renamed to "link_and_its_composite_are_massless()" with a comment noting that there might not be a composite. Not perfect, but I think it comes closer to your suggestion.
That's funny, because isn't that how you documented the algorithm? I certainly feel like I just borrowed that abstraction.
I think the new name perfectly serves. It's very literal and tells us something about a possible composite and that makes sense at the call sites.
sherm1
force-pushed
the
mbp_topology_merge_composites
branch
from
4a114e2 to
81614e9
Compare
There was a problem hiding this comment.
I won't be able to finish this today. I'll aim to finish by early Monday.
Reviewed 1 of 4 files at r4, 1 of 5 files at r5, 6 of 9 files at r6, 1 of 2 files at r7, 2 of 4 files at r8, 1 of 2 files at r9, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee xuchenhan-tri(platform)
multibody/topology/link_joint_graph.h line 452 at r7 (raw file):
std::string_view name, ModelInstanceIndex model_instance_index) const; /** @returns `true` if a Joint named `name` was added to `model_instance`.
BTW, I would have expected this function's name to be singular because it returns a single LinkComposite.
Suggestion:
[[nodiscard]] const LinkComposite& link_composite(
LinkCompositeIndex link_composite_index) const {
return link_composites().at(link_composite_index);
}multibody/topology/spanning_forest.h line 560 at r9 (raw file):
// - the joint's model instance must request merging composites, and // - the joint has _not_ demanded that it be modeled. bool should_merge_parent_and_child(const Joint& joint) {
nit const?
There was a problem hiding this comment.
Reviewed 2 of 2 files at r9, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee xuchenhan-tri(platform)
There was a problem hiding this comment.
Reviewed 1 of 4 files at r8, 1 of 2 files at r9.
Reviewable status: 7 unresolved discussions
multibody/topology/spanning_forest.cc line 494 at r9 (raw file):
Note: Every j ∈ Jᵢₙ and j ∈ Jₒᵤₜ has at least one of its two links (parent and child) already modeled in the current forest F. If both links are
nit, this comment confuses me. From reading everything above this sentence, I would expect each j ∈ Jᵢₙ has at least one of its links modeled in the current forest F (the input F), and for each j ∈ Jᵢₙ has at least one of its links modeled in the next forest F (the output F). Did I misunderstand?
Code quote:
Note: Every j ∈ Jᵢₙ and j ∈ Jₒᵤₜ has at least one of its two links (parent
and child) already modeled in the current forest F. If both links aremultibody/topology/spanning_forest.cc line 990 at r9 (raw file):
/* If any of outboard_link's other joints had been processed, outboard_link would have already been in the forest. Hence all it's non-merge joints are open (unprocessed). */
typo
Suggestion:
would have already been in the forest. Hence all its non-merge joints
are open (unprocessed). */multibody/topology/spanning_forest.cc line 999 at r9 (raw file):
/* We've found another unprocessed joint that needs merging onto this composite. One of it's links is the outboard_link (already in the forest at this point). Find the other link. */
typo
Suggestion:
composite. One of its links is the outboard_link (already in the forest
at this point). Find the other link. */multibody/topology/spanning_forest.cc line 1013 at r9 (raw file):
*outboard_link.composite()); continue; // On to the next Joint. }
nit, this seems to be repeating the logic at the beginning of this function and should be able to be automatically handled by the recursion below.
Code quote:
/* If the other link has already been processed this is a harmless
loop-within-merged-composite as above. */
if (link_is_already_in_forest(other_link_ordinal)) {
const Link& other_link = links(other_link_ordinal);
DRAKE_DEMAND(other_link.composite() == outboard_link.composite());
mutable_graph().AddUnmodeledJointToComposite(joint_ordinal,
*outboard_link.composite());
continue; // On to the next Joint.
}multibody/topology/test/spanning_forest_test.cc line 1139 at r9 (raw file):
EXPECT_FALSE(forest.dynamics_ok()); EXPECT_THAT( forest.why_no_dynamics(),
BTW, should there also be a test case for failing to find a massful link after going through step A8 and running into a loop joint between too massless links? I didn't find a test covering that "why_no_dynamics" message.
sherm1
force-pushed
the
mbp_topology_merge_composites
branch
from
81614e9 to
8aee7c0
Compare
There was a problem hiding this comment.
Great actionable observations! Thanks, @xuchenhan-tri and @SeanCurtis-TRI. All comments addressed, PTAL.
Reviewable status:
complete! all discussions resolved, LGTM from assignees SeanCurtis-TRI(platform),xuchenhan-tri(platform)
multibody/topology/link_joint_graph.h line 452 at r7 (raw file):
Previously, xuchenhan-tri wrote…
BTW, I would have expected this function's name to be singular because it returns a single LinkComposite.
This is consistent with the naming strategy I used for links(), joints(), etc. because I wanted to be able to write Link& link = link(3) but that failed due to shadowing of the local variable name. But ... I'm rethinking that since it strikes everyone as weird. For now I added a TODO.
multibody/topology/spanning_forest.h line 560 at r9 (raw file):
Previously, xuchenhan-tri wrote…
nit const?
Done
multibody/topology/spanning_forest.cc line 494 at r9 (raw file):
Previously, xuchenhan-tri wrote…
nit, this comment confuses me. From reading everything above this sentence, I would expect each j ∈ Jᵢₙ has at least one of its links modeled in the current forest F (the input F), and for each j ∈ Jᵢₙ has at least one of its links modeled in the next forest F (the output F). Did I misunderstand?
Done. Right! Thanks.
multibody/topology/spanning_forest.cc line 990 at r9 (raw file):
Previously, xuchenhan-tri wrote…
typo
Done
multibody/topology/spanning_forest.cc line 999 at r9 (raw file):
Previously, xuchenhan-tri wrote…
typo
Done
multibody/topology/spanning_forest.cc line 1013 at r9 (raw file):
Previously, xuchenhan-tri wrote…
nit, this seems to be repeating the logic at the beginning of this function and should be able to be automatically handled by the recursion below.
Done. Beautiful, thanks!
multibody/topology/test/spanning_forest_test.cc line 1139 at r9 (raw file):
Previously, xuchenhan-tri wrote…
BTW, should there also be a test case for failing to find a massful link after going through step A8 and running into a loop joint between too massless links? I didn't find a test covering that "why_no_dynamics" message.
Done. That was missing. I added a new test case covering several variants of massless loops, PTAL.
There was a problem hiding this comment.
Reviewed 4 of 4 files at r10, all commit messages.
Reviewable status: 1 unresolved discussion
multibody/topology/test/spanning_forest_test.cc line 1701 at r10 (raw file):
const std::vector<std::pair<int, int>> joints{{4, 1}, {4, 3}, {1, 2}, {2, 3}}; for (int i = 0; i < ssize(joints); ++i) { graph.AddJoint("joint" + std::to_string(i), default_model_instance(),
BTW I hadn't mentioned this in the past, and I don't think you need to act on this in this PR, but food for thought:
Instead of
"joint" + std::sto_string(i)consider
fmt::format("joint{}", i)It's more modern, it's (norminally) shorter, and it puts the important pieces of information closer together. I believe you currently have a significant usage of to_string().
There was a problem hiding this comment.
Reviewable status:
multibody/topology/test/spanning_forest_test.cc line 1701 at r10 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW I hadn't mentioned this in the past, and I don't think you need to act on this in this PR, but food for thought:
Instead of
"joint" + std::sto_string(i)consider
fmt::format("joint{}", i)It's more modern, it's (norminally) shorter, and it puts the important pieces of information closer together. I believe you currently have a significant usage of
to_string().
Thanks, that does look better. Will change my ways in the future.
There was a problem hiding this comment.
Reviewed 4 of 4 files at r10, all commit messages.
Reviewable status:
This is the 8th installment in the PR train for #20225, following #21731.
This is the last missing piece of basic topology functionality: the ability to merge welded-together links in the graph so that they follow only a single mobilized body in the forest.
What's here:
What's not here:
There are no user-visible changes in this PR.
This change is