parsing: Add support for SDFormat 1.8 model composition (redo) #15099
Conversation
There was a problem hiding this comment.
Assigning +@EricCousineau-TRI for feature review, or delegation to another reviewer.
Reviewable status: LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI)
|
Thanks for the ping! I compared patch resulting from #14401 and this PR's current patch.
cd drake
# 315334b - old pr, merged
# 1041c6c - current pr, current rev
git diff 315334b~ 315334b > /tmp/patch-old.diff
git diff $(git merge-base 1041c6c upstream/master) 1041c6c > /tmp/patch-new.diff
meld /tmp/patch-{old,new}.diff |
There was a problem hiding this comment.
Also, just a minor BTW on testing code.
Reviewed 12 of 12 files at r1.
Reviewable status: 1 unresolved discussion, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @azeey)
a discussion (no related file):
nit CI is failing due to lint failures.
Can you fix this?
multibody/parsing/test/detail_sdf_parser_test.cc, line 498 at r1 (raw file):
"Verify that static models don't need to have a canonical link"); PlantAndSceneGraph pair; DRAKE_ASSERT_NO_THROW(pair = ParseTestString(R"""(
BTW Still not sure if I see the value of ASSERT_NO_THROW. If this fails, I don't really care about subsequent code running. (In fact, future steps will prolly segfault?)
Consider removing?
multibody/parsing/test/detail_sdf_parser_test.cc, line 498 at r1 (raw file):
"Verify that static models don't need to have a canonical link"); PlantAndSceneGraph pair; DRAKE_ASSERT_NO_THROW(pair = ParseTestString(R"""(
BTW Consider changing PlantAndSceneGraph to std::pair<...> so you could use a structured binding.
There was a problem hiding this comment.
+@sammy-tri for follow-up platform review (since you reviewed the first) - thank!
Reviewable status: 1 unresolved discussion, LGTM missing from assignee sammy-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @azeey and @sammy-tri)
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee sammy-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @azeey, @EricCousineau-TRI, and @sammy-tri)
a discussion (no related file):
Working: Testing in Anzu CI.
|
multibody/parsing/test/detail_sdf_parser_test.cc, line 498 at r1 (raw file): Previously, EricCousineau-TRI (Eric Cousineau) wrote…
I'm used to CI infrastructure where tests with segfaults end up with different names on the dashboard than the test suite name, so we prefer to assert anything that might cause a segfault. I'll remove it if that's not an issue. |
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee sammy-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI and @sammy-tri)
a discussion (no related file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
nit CI is failing due to lint failures.
Can you fix this?
Done in a1a1064
multibody/parsing/test/detail_sdf_parser_test.cc, line 498 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
BTW Consider changing
PlantAndSceneGraphtostd::pair<...>so you could use a structured binding.
Done in a1a1064
multibody/parsing/test/detail_sdf_parser_test.cc, line 498 at r1 (raw file):
Previously, azeey (Addisu Z. Taddese) wrote…
I'm used to CI infrastructure where tests with segfaults end up with different names on the dashboard than the test suite name, so we prefer to assert anything that might cause a segfault. I'll remove it if that's not an issue.
Done in a1a1064
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee sammy-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI and @sammy-tri)
multibody/parsing/test/detail_sdf_parser_test.cc, line 498 at r1 (raw file):
Previously, azeey (Addisu Z. Taddese) wrote…
Done in a1a1064
OK Thanks! I don't think I've seen it cause an issue with bazel's test logging, nor the jUnit stuff (either in Drake's CDash parsing, nor in Anzu Jenkin's jUnit parsing).
Changes in behavior: * Models nested via the `<include>` tag are no longer flattened, which means each included model will have its own model instance. * `drake::multibody::Parser::AddAllModelsFromFile` now returns all added models including nested models Note: This does not use a custom parser for URDFs via libsdformat's Interface API, and thus may incur unexpected behavior when including URDF files from SDFormat pending the full resolution of RobotLocomotion#14295.
Signed-off-by: Addisu Z. Taddese <[email protected]>
EricCousineau-TRI
force-pushed
the
sdf18_composition
branch
from
a1a1064 to
dc465a9
Compare
There was a problem hiding this comment.
Reviewable status: LGTM missing from assignee sammy-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI and @sammy-tri)
a discussion (no related file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Working: Testing in Anzu CI.
OK Testing passed! Anzu PR 7073
There was a problem hiding this comment.
Reviewed 3 of 3 files at r2.
Reviewable status: LGTM missing from assignee sammy-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @sammy-tri)
There was a problem hiding this comment.
Reviewable status: commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @sammy-tri)
sammy-tri
added
the
status: squashing now
There was a problem hiding this comment.
+(status: squashing now)
Reviewable status:
complete! all discussions resolved, LGTM from assignees sammy-tri(platform),EricCousineau-TRI(platform) (waiting on @sammy-tri)
This removes additional implicit model frames created for nested models that were missed in #15099.
This a redo of #14401 but does not create a frame for each model in the parent scope of the model. There is still a frame associated with each model, but it's named
__model__and is found inside the model instance.Description from #14401:
Changes in behavior:
<include>tag are no longer flattened, which means each included model will have its own model instance.drake::multibody::Parser::AddAllModelsFromFilenow returns all added models including nested modelsNote: This does not use a custom parser for URDFs via libsdformat's Interface API, and thus may incur unexpected behavior when including URDF files from SDFormat pending the full resolution of #14295.
This change is