parsing: Add support for SDFormat 1.8 model composition #14401
Conversation
There was a problem hiding this comment.
+@EricCousineau-TRI -- assigning feature reviewer, to make sure this gets timely attention, per https://drake.mit.edu/platform_reviewer_checklist.html?highlight=platform
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)
There was a problem hiding this comment.
Oooh, sweet! I know it's a draft, but I added some quick comments!
Reviewed 2 of 11 files at r1.
Reviewable status: 5 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @azeey and @EricCousineau-TRI)
multibody/parsing/detail_sdf_parser.cc, line 52 at r1 (raw file):
// If the give name is not scoped, this will return an empty string for the // parent model name and the given name as the local name. std::pair<std::string, std::string> SplitName(
I believe we have functionality for this already in Drake...
Will post a link.
multibody/parsing/detail_sdf_parser.cc, line 774 at r1 (raw file):
// frame is not the world. At present, we assume the parent frame is the // world. // if (model.Element()->GetParent()->GetName() == "world") {
Can you state why this is commented?
Can it be removed?
multibody/parsing/detail_sdf_parser.cc, line 990 at r1 (raw file):
model, model.Name(), {}, plant, package_map, root_dir)); // const ModelInstanceIndex model_instance = AddModelFromSpecification(
Can you state why this is commented out?
Work in Progress?
multibody/parsing/parser.h, line 56 at r1 (raw file):
/// more than one `<model>` element. However, this function might create /// additional model instances corresponding to nested models found in the /// top level model.
Is there a way to reference SDFormat documentation that states what a "nested model" is here?
In isolation, I wouldn't know what this meant precisely.
multibody/parsing/test/detail_scene_graph_test.cc, line 827 at r1 (raw file):
IllustrationProperties material = MakeVisualPropertiesFromSdfVisual(*sdf_visual, NoopResolveFilename); Vector4<double> expected_diffuse{0, 1, 1, 1};
Can you state why this changed?!
There was a problem hiding this comment.
Reviewable status: 5 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @azeey and @EricCousineau-TRI)
multibody/parsing/test/detail_scene_graph_test.cc, line 811 at r1 (raw file):
} // Case: Too few channel values -- fill in with 0 for b and 1 for alpha.
nit Per f2f, should update this to be true.
multibody/parsing/test/detail_scene_graph_test.cc, line 827 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Can you state why this changed?!
OK Per f2f, this was for malformed input. Meh.
There was a problem hiding this comment.
Reviewable status: 6 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @azeey and @EricCousineau-TRI)
a discussion (no related file):
Working: Will ensure that you have the bits so that Jenkins will run CI.
|
@drake-jenkins-bot ok to test |
There was a problem hiding this comment.
Reviewable status: 5 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @azeey and @EricCousineau-TRI)
a discussion (no related file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Working: Will ensure that you have the bits so that Jenkins will run CI.
Done. See Slack: https://drakedevelopers.slack.com/archives/C0KDGF4V9/p1607027529020700?thread_ts=1607027228.020100&cid=C0KDGF4V9
There was a problem hiding this comment.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI)
multibody/parsing/detail_sdf_parser.cc, line 52 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
I believe we have functionality for this already in Drake...
Will post a link.
Okay, fyi, I took this from https://github.com/RobotLocomotion/drake/pull/13128/files#diff-397f65195f4a84856760793c9f2882a6c11b286b55f387e71cc23ef573934d8dR438.
multibody/parsing/detail_sdf_parser.cc, line 774 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Can you state why this is commented?
Can it be removed?
I've removed it and the TODO since this now handles the case where a model is not a child of "world".
multibody/parsing/detail_sdf_parser.cc, line 990 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Can you state why this is commented out?
Work in Progress?
Yeah, it was WIP. It's uncommented now.
multibody/parsing/parser.h, line 56 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Is there a way to reference SDFormat documentation that states what a "nested model" is here?
In isolation, I wouldn't know what this meant precisely.
Done in 572e540
multibody/parsing/test/detail_scene_graph_test.cc, line 811 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
nit Per f2f, should update this to be true.
Done in b3d43fa
multibody/parsing/test/detail_scene_graph_test.cc, line 827 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
OK Per f2f, this was for malformed input. Meh.
Done in b3d43fa
There was a problem hiding this comment.
Reviewed 7 of 11 files at r1, 8 of 8 files at r2.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI)
a discussion (no related file):
Working: Will take this for a test run on Anzu.
Excited!!!
multibody/parsing/detail_sdf_parser.cc, line 52 at r1 (raw file):
Previously, azeey (Addisu Z. Taddese) wrote…
Okay, fyi, I took this from https://github.com/RobotLocomotion/drake/pull/13128/files#diff-397f65195f4a84856760793c9f2882a6c11b286b55f387e71cc23ef573934d8dR438.
OK Thanks!
|
a discussion (no related file): Will need to either fail fast in this branch for those shapes (i.e. Drake doesn't support libsdformat flavor) or add support directly here. |
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @azeey and @EricCousineau-TRI)
a discussion (no related file):
For gazebosim/sdformat#433, we will put out a poll in Drake Slack about using model //model files from the root.
If people use it (hopefully not), we put more effort into tick-tocking the deprecation.
Otherwise, we just rip it out and only accept one root-level model in Drake.
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @azeey and @EricCousineau-TRI)
a discussion (no related file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
For gazebosim/sdformat#433, we will put out a poll in Drake Slack about using model
//modelfiles from the root.If people use it (hopefully not), we put more effort into tick-tocking the deprecation.
Otherwise, we just rip it out and only accept one root-level model in Drake.
Drake Slack post: https://drakedevelopers.slack.com/archives/C2WBPQDB7/p1611856134001200
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @azeey and @EricCousineau-TRI)
a discussion (no related file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Working: Will take this for a test run on Anzu.
Excited!!!
Building now: For internal Anzu-master-Drake-PR, this is build 95.
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @azeey and @EricCousineau-TRI)
a discussion (no related file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Building now: For internal Anzu-master-Drake-PR, this is build 95.
Updated - made Anzu PR 6253 to track and modify.
Most runtime failures are due to pose parsing errors about //pose not having 6 separate floats, which should be easy to trace down -- though not as easy as it could be ;) gazebosim/sdformat#479
However, I am getting some errors about duplicate frames and name collisions.
Will see if I can pinpoint that.
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @azeey and @EricCousineau-TRI)
a discussion (no related file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Updated - made Anzu PR 6253 to track and modify.
Most runtime failures are due to pose parsing errors about
//posenot having 6 separate floats, which should be easy to trace down -- though not as easy as it could be ;) gazebosim/sdformat#479However, I am getting some errors about duplicate frames and name collisions.
Will see if I can pinpoint that.
Gotta timebox for now - but will try to return to this.
|
a discussion (no related file): Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Done in 8ac87ca |
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @azeey and @EricCousineau-TRI)
a discussion (no related file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Gotta timebox for now - but will try to return to this.
I'm going to make my own version of this PR, but pulls in the model regression components from #12061, to try and identify some awkward Anzu failures
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @azeey and @EricCousineau-TRI)
a discussion (no related file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
I'm going to make my own version of this PR, but pulls in the model regression components from #12061, to try and identify some awkward Anzu failures
Filed #14704
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @azeey and @EricCousineau-TRI)
a discussion (no related file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Filed #14704
I think this is fixed by 29b0bd4
There was a problem hiding this comment.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @azeey and @EricCousineau-TRI)
a discussion (no related file):
Working: Going to update this branch to resolve merge conflicts (upgrade to libsdformat 10.3)
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @azeey and @EricCousineau-TRI)
a discussion (no related file):
Previously, azeey (Addisu Z. Taddese) wrote…
I think this is fixed by 29b0bd4
Checking now...
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @azeey and @EricCousineau-TRI)
a discussion (no related file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Working: Going to update this branch to resolve merge conflicts (upgrade to libsdformat 10.3)
Done.
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee sammy-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @azeey and @EricCousineau-TRI)
a discussion (no related file):
Previously, azeey (Addisu Z. Taddese) wrote…
Could you point me to which file I should update about this?
Release notes are collected per the instructions here: https://drake.mit.edu/release_playbook.html
While not state explicitly, you should word the relevant portions of the release notes into the merged commit.
If we're using a squash, then you can add it into the PR overview, which can then be reviewed before Squashing and Merging.
multibody/parsing/test/detail_sdf_parser_test.cc, line 1327 at r9 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Working: Lemme dig in briefly to double-check what's feasible and if my ask makes sense. Un momentito
Above, for Parser::AddAllModelsFromFile:
/// @returns The set of model instance indices for the newly added models,
/// including nested models.
At present, this contract is not honored; please see my meta-PR:
azeey#1
$ bazel run //multibody/parsing:parser_test -- --gtest_filter=FileParserTest.MultiModelViaWorldIncludesTest
...
[ RUN ] FileParserTest.MultiModelViaWorldIncludesTest
multibody/parsing/test/parser_test.cc:147: Failure
Expected equality of these values:
model_names_actual
Which is: { "parent_model" }
model_names_expected
Which is: { "parent_model", "parent_model::robot_1", "parent_model::robot_2" }
[ FAILED ] FileParserTest.MultiModelViaWorldIncludesTest (57 ms)
...
Can you make this test pass?
Also up for discussion: Is "including nested models" the right expectation? (maybe it could just be root models, which is what we have at present)
@sammy-tri Can I ask your opinion?
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee sammy-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @azeey and @EricCousineau-TRI)
multibody/parsing/test/detail_sdf_parser_test.cc, line 1327 at r9 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Above, for
Parser::AddAllModelsFromFile:/// @returns The set of model instance indices for the newly added models, /// including nested models.At present, this contract is not honored; please see my meta-PR:
azeey#1$ bazel run //multibody/parsing:parser_test -- --gtest_filter=FileParserTest.MultiModelViaWorldIncludesTest ... [ RUN ] FileParserTest.MultiModelViaWorldIncludesTest multibody/parsing/test/parser_test.cc:147: Failure Expected equality of these values: model_names_actual Which is: { "parent_model" } model_names_expected Which is: { "parent_model", "parent_model::robot_1", "parent_model::robot_2" } [ FAILED ] FileParserTest.MultiModelViaWorldIncludesTest (57 ms) ...Can you make this test pass?
Also up for discussion: Is "including nested models" the right expectation? (maybe it could just be root models, which is what we have at present)
@sammy-tri Can I ask your opinion?
Good catch!
My first thought is that the issue is that AddModelFromSpecification returns a single ModelInstanceIndex even though it can now add multiple models (recursively). Should we change the name to the (more correct) AddModelsFromSpecification and return a std::vector<ModelInstanceIndex> which includes the recursively added models? and then combine the resulting vectors for the output from AddModelsFromSdf?
One question is what to do if you call AddModelFromSdf and you get recursively nested models. I guess just return the top level model's index (like this PR does now).
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee sammy-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @azeey and @EricCousineau-TRI)
multibody/parsing/test/detail_sdf_parser_test.cc, line 1327 at r9 (raw file):
Previously, sammy-tri (Sam Creasey) wrote…
Good catch!
My first thought is that the issue is that
AddModelFromSpecificationreturns a singleModelInstanceIndexeven though it can now add multiple models (recursively). Should we change the name to the (more correct)AddModelsFromSpecificationand return astd::vector<ModelInstanceIndex>which includes the recursively added models? and then combine the resulting vectors for the output fromAddModelsFromSdf?One question is what to do if you call
AddModelFromSdfand you get recursively nested models. I guess just return the top level model's index (like this PR does now).
or, yeah, we could just update the documentation. I think I slightly prefer the extra model indexes to be returned.
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee sammy-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @azeey and @EricCousineau-TRI)
multibody/parsing/test/detail_sdf_parser_test.cc, line 1327 at r9 (raw file):
Previously, sammy-tri (Sam Creasey) wrote…
or, yeah, we could just update the documentation. I think I slightly prefer the extra model indexes to be returned.
I think AddModelsFromSpecification (adding s) sounds better, having it include resursively included model seems simplest for now.
In this case, it should strive to have the first root model be first in the vector. Then for AddModelFromSdf, it simply extracts the first element in the vector and returns it. For AddAllModelsFromSdfFile, it would then just return the vector.
@azeey What say ye?
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee sammy-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @azeey and @EricCousineau-TRI)
multibody/parsing/test/detail_sdf_parser_test.cc, line 957 at r9 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Ah - this test was a negative test on an invalid
//pose/@relative_tospecification; can you restore it?
Will do. This uncovered a bug in libsdformat gazebosim/sdformat#567.
multibody/parsing/test/detail_sdf_parser_test.cc, line 1327 at r9 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
I think
AddModelsFromSpecification(addings) sounds better, having it include resursively included model seems simplest for now.In this case, it should strive to have the first root model be first in the vector. Then for
AddModelFromSdf, it simply extracts the first element in the vector and returns it. ForAddAllModelsFromSdfFile, it would then just return the vector.@azeey What say ye?
Adding the s and having AddModelFromSdf return the first element sounds good to me too.
There was a problem hiding this comment.
Reviewed 1 of 2 files at r11.
Reviewable status: 3 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)
azeey
changed the title
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, 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…
Release notes are collected per the instructions here: https://drake.mit.edu/release_playbook.html
While not state explicitly, you should word the relevant portions of the release notes into the merged commit.
If we're using a squash, then you can add it into the PR overview, which can then be reviewed before Squashing and Merging.
I have updated the PR overview. If that looks good to you, I'll squash my commits and use the text in the overview as the commit message.
multibody/parsing/test/detail_sdf_parser_test.cc, line 1308 at r8 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
OK Thanks!
FWIW, though, it may be better to include commit / version (e.g. `
Done in f7b1ed2
multibody/parsing/test/detail_sdf_parser_test.cc, line 957 at r9 (raw file):
Previously, azeey (Addisu Z. Taddese) wrote…
Will do. This uncovered a bug in libsdformat gazebosim/sdformat#567.
Done in db5d0a9. I have changed where ThrowIfPoseFrameSpecified is called because //pose/@relative_to is invalid only for top-level models in a model file.
multibody/parsing/test/detail_sdf_parser_test.cc, line 1327 at r9 (raw file):
Previously, azeey (Addisu Z. Taddese) wrote…
Adding the
sand havingAddModelFromSdfreturn the first element sounds good to me too.
Done in 6a1b538. Let me know if you'd prefer passing an output vector by reference instead. I chose to do this because it made the code clearer when added nested models and extracting the immediate child model for adding the associated frame
drake/multibody/parsing/detail_sdf_parser.cc
Lines 773 to 780 in e2909c3
There was a problem hiding this comment.
Reviewed 3 of 3 files at r12.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee sammy-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI)
multibody/parsing/test/detail_sdf_parser_test.cc, line 1327 at r9 (raw file):
Previously, azeey (Addisu Z. Taddese) wrote…
Done in 6a1b538. Let me know if you'd prefer passing an output vector by reference instead. I chose to do this because it made the code clearer when added nested models and extracting the immediate child model for adding the associated frame
drake/multibody/parsing/detail_sdf_parser.cc
Lines 773 to 780 in e2909c3
I'm good with the current formulation. Thanks!
There was a problem hiding this comment.
Good to go on my side of things! (just two minor nits)
Reviewed 1 of 4 files at r10, 1 of 2 files at r11, 3 of 3 files at r12.
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)
a discussion (no related file):
Previously, azeey (Addisu Z. Taddese) wrote…
I have updated the PR overview. If that looks good to you, I'll squash my commits and use the text in the overview as the commit message.
OK Excellent - thanks!
multibody/parsing/detail_sdf_parser.cc, line 46 at r12 (raw file):
namespace { // The relative_name of an object in this function is the name of the object in
nit This docstring is stale (relative_name doesn't exist here?)
multibody/parsing/detail_sdf_parser.cc, line 50 at r12 (raw file):
// name can contain the scope delimiter to reference objects nested in child // models. // Given a relative_name to an object, this function returns the model
nit Weird to read an exception about a parameter, and then see the overview for the function.
Can you switch the order, and then describe above paragraph using @param?
multibody/parsing/test/detail_sdf_parser_test.cc, line 1308 at r8 (raw file):
Previously, azeey (Addisu Z. Taddese) wrote…
Done in f7b1ed2
OK Thanks! (and derp, forgot to close out the comment hehe)
multibody/parsing/test/detail_sdf_parser_test.cc, line 957 at r9 (raw file):
Previously, azeey (Addisu Z. Taddese) wrote…
Done in db5d0a9. I have changed where
ThrowIfPoseFrameSpecifiedis called because//pose/@relative_tois invalid only for top-level models in a model file.
OK Gotcha, thank you for documenting in the code! (detail_sdf_parser.cc)
multibody/parsing/test/detail_sdf_parser_test.cc, line 1327 at r9 (raw file):
Previously, sammy-tri (Sam Creasey) wrote…
I'm good with the current formulation. Thanks!
OK Likewise, totes fine with return value!
(we're parsing anywho, so I doubt we'd gain much performance relative to the operation hehe)
There was a problem hiding this comment.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee sammy-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @azeey)
multibody/parsing/detail_sdf_parser.cc, line 756 at r12 (raw file):
// "P" is the parent frame. If the model is in a child of //world or //sdf, // this world be the world frame. Otherwise, this would be the parent model
nit typo "this world" => "this will"
(also, tense: "would" => "will"?)
multibody/parsing/detail_sdf_parser.cc, line 758 at r12 (raw file):
// this world be the world frame. Otherwise, this would be the parent model // frame. const RigidTransformd X_PmM = ResolveRigidTransform(model.SemanticPose());
nit Can you distinguish Pm w.r.t. P? Or can you replace Pm with P?
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.
There was a problem hiding this comment.
Reviewable status: LGTM missing from assignee sammy-tri(platform) (waiting on @EricCousineau-TRI and @sammy-tri)
multibody/parsing/detail_sdf_parser.cc, line 46 at r12 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
nit This docstring is stale (
relative_namedoesn't exist here?)
Done.
multibody/parsing/detail_sdf_parser.cc, line 50 at r12 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
nit Weird to read an exception about a parameter, and then see the overview for the function.
Can you switch the order, and then describe above paragraph using
@param?
Done. I went ahead and used @param for all of the parameters. Is this what you had in mind?
multibody/parsing/detail_sdf_parser.cc, line 756 at r12 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
nit typo "this world" => "this will"
(also, tense: "would" => "will"?)
Ok, changed to "will"
multibody/parsing/detail_sdf_parser.cc, line 758 at r12 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
nit Can you distinguish
Pmw.r.t.P? Or can you replacePmwithP?
Ok, replaced Pm with P. I can't remember why I picked Pm in the first place.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r13.
Reviewable status:complete! all discussions resolved, LGTM from assignees sammy-tri(platform),EricCousineau-TRI(platform) (waiting on @azeey)
There was a problem hiding this comment.
Reviewed 1 of 1 files at r13.
Reviewable status:
multibody/parsing/detail_sdf_parser.cc, line 50 at r12 (raw file):
Previously, azeey (Addisu Z. Taddese) wrote…
Done. I went ahead and used @param for all of the parameters. Is this what you had in mind?
OK Yup - perfect!
multibody/parsing/detail_sdf_parser.cc, line 758 at r12 (raw file):
Previously, azeey (Addisu Z. Taddese) wrote…
Ok, replaced
PmwithP. I can't remember why I pickedPmin the first place.
OK Sweet, thanks!
|
FYI I think I'm setting a behavior change here beyond what was documented in the commit message. (See Anzu 7013 for details.) It seems like a frame name that was previously unique across all model instances now exists on >1 instance. I'm working on isolating exactly what happened, and will post back once I know more. |
|
Sorry, yes, that is one thing we've encountered before. I had fixed that in Anzu, and had a Drake PR, but did a poor job of making it traceable. |
…)" This reverts commit 315334b.
|
multibody/parsing/detail_sdf_parser.cc, line 828 at r13 (raw file):
FYI I think this is where the PR goes wrong, by adding a frame name to the world model instance. If I remove this http://sdformat.org/spec?ver=1.8&elem=model In a first search through http://sdformat.org/tutorials?tut=composition_proposal, I did not find any mention of using model names as |
|
multibody/parsing/detail_sdf_parser.cc, line 828 at r13 (raw file): Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
You're right in that there is no explicit mention of models being used for I can look into how we can remove |
|
In addition to Addisu's point - the defect introduced in Drake w.r.t. backwards compatibility is that we wanted parity between the "interface elements" in SDFormat (e.g. a user could refer to this implicit model frame) and what Drake presents (registration of the implicit model frame). I think simply removing the code in question could service it; ideally, all the resolution on SDFormat side of things means we can make it work. If there's some minor indirection (e.g. "Is this SDFormat frame not going to be registered in Drake, and what's its proxy"), I think that can be handled within Drake. @azeey Is this already what ya had in mind? |
|
Yeah, that's what I had in mind. I'm hoping we'd be able to use the |
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. 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 #14295. * Do not create frame named after model Signed-off-by: Addisu Z. Taddese <[email protected]>
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