Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Model Directives mechanism for scene assembly. #13899

Merged

Conversation

ggould-tri
Copy link
Contributor

@ggould-tri ggould-tri commented Aug 18, 2020

Toward #13899.

Model Directives mechanism for scene assembly.

  • A domain-specific language for assembling MultibodyPlant scenes
    from multiple SDF files.

  • Helpful for assembling large scenes without huge unwieldy sdf/xacro
    files.

  • A temporary accomodation until sdformat adds similar functionality.

    Co-authored-by: Eric Cousineau [email protected]
    Co-authored-by: Jeremy Nimmer [email protected]
    Co-authored-by: Grant Gould [email protected]
    Co-authored-by: Calder Phillips-Grafflin [email protected]
    Co-authored-by: Siyuan Feng [email protected]


This change is Reviewable

Copy link
Contributor Author

@ggould-tri ggould-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 10 of 10 files at r1.
Reviewable status: needs at least one assigned reviewer, labeled "do not merge"

Copy link
Contributor Author

@ggould-tri ggould-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r2.
Reviewable status: needs at least one assigned reviewer, labeled "do not merge"

@ggould-tri ggould-tri changed the title Verbatim copy of model directives system from project anzu Model Directives mechanism for scene assembly. Aug 18, 2020
@ggould-tri ggould-tri force-pushed the model_directives_bashmu branch 2 times, most recently from a3a471b to 84f70bf Compare August 18, 2020 14:10
Copy link
Contributor Author

@ggould-tri ggould-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-(status: do not review) +@jwnimmer-tri +(priority: low)
r1/r2 is the verbatim copy.
r3 is the renamespacing plus a smattering of bugfixes necessary to make it compile and lint.

Let me know if you'd prefer the non-dev parts of r3 moved to a separate PR (particularly the bazel change, which I made with limited bazel knowledge).

Reviewable status: LGTM missing from assignee jwnimmer-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 @ggould-tri and @jwnimmer-tri)

@ggould-tri ggould-tri force-pushed the model_directives_bashmu branch 3 times, most recently from c077ee9 to 0af7888 Compare August 18, 2020 15:14
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checkpoint. I'll look over the README soon.

Reviewed 10 of 10 files at r1, 1 of 1 files at r2, 12 of 15 files at r3, 1 of 1 files at r4.
Reviewable status: 12 unresolved discussions, LGTM missing from assignee jwnimmer-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 @ggould-tri and @jwnimmer-tri)

a discussion (no related file):
nit The commits need a rebase:

(1) The first commit is the one with the copied content, so should use "Co-authored-by" syntax and list everyone. The second commit should not mention anyone except you.

(2) Saying "Resolves" in the second commit message is inaccurate -- since the code is in dev, the issue is not yet finished.



multibody/parsing/dev/BUILD.bazel, line 7 at r4 (raw file):

    "drake_cc_googletest",
    "drake_cc_library",
    "drake_cc_package_library",

nit package_library is unused.


multibody/parsing/dev/BUILD.bazel, line 17 at r4 (raw file):

    hdrs = ["process_model_directives.h"],
    data = [
        "@drake//manipulation/models/jaco_description:models",

nit Drop the redundant @drake prefix.


multibody/parsing/dev/BUILD.bazel, line 48 at r4 (raw file):

        "//common:name_value",
        "//common/schema/dev:transform",
        "//math",

nit There is a lot of extra junk in //math; use a narrower component here.


multibody/parsing/dev/BUILD.bazel, line 62 at r4 (raw file):

install_data(
    extra_tags = ["nolint"],  # `dev` content is never actually installed.
)

We should not use install_data merely to declare a filegroup; we should only use it when we actually want to install things.

In parsing/BUILD.bazel we have:

filegroup(
    name = "test_models",
    testonly = 1,

which is more along the lines of what we should use here.


multibody/parsing/dev/model_directives.h, line 22 at r4 (raw file):

namespace multibody {
namespace parsing {
namespace dev {

nit Usually we would not use "dev" in the namespace hierarchy; omitting it makes it easier later to relocate the file with less churn. Not strictly speaking a defect, though.


multibody/parsing/dev/model_directives.h, line 23 at r4 (raw file):

namespace parsing {
namespace dev {
namespace schema {

Meta GSG violation -- the namespace name "schema" is not based on any directory name. I think we should remove it here.


multibody/parsing/dev/process_model_directives.cc, line 412 at r4 (raw file):

}

nit Extra blank line.


multibody/parsing/dev/test/process_model_directives_test.cc, line 32 at r4 (raw file):

  auto parser = std::make_unique<Parser>(plant);
  const drake::filesystem::path abspath_xml = FindResourceOrThrow(
          "drake/multibody/parsing/dev/test/models/package.xml");

nit Consider factoring the "drake/multibody/parsing/dev/test/models/" string into a constant, to de-boilerplate all of the below tests and make it easier to rename things (non-dev) in the future.


multibody/parsing/dev/test/models/package.xml, line 3 at r4 (raw file):

<?xml version="1.0"?>
<package format="2">
  <name>process_model_directives_test</name>

Especially once we lose the /dev/ part of the pathname for all of this test data, the folder name "models" does not seem sufficiently distinct from the other data that will be in multibody/parsing/test. Perhaps this folder should renamed to "process_model_directives_test", so that it is more unique and matches the name given here?


multibody/parsing/dev/test/models/simple_model.sdf, line 16 at r3 (raw file):

        <material>
          <!--
          Test usage of diffuse map from a non-neighboring Anzu resource.

nit The term "Anzu" should generally not appear by the final revision.


multibody/parsing/dev/test/models/simple_model.sdf, line 19 at r3 (raw file):

          -->
          <drake:diffuse_map>
            package://process_model_directives_test/circle.png

The comment claims that this is "non-neighboring", which is perhaps inaccurate now?

Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 15 files at r3.
Reviewable status: 13 unresolved discussions, LGTM missing from assignee jwnimmer-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 @ggould-tri and @jwnimmer-tri)


multibody/parsing/dev/model_directives.h, line 88 at r4 (raw file):

};

struct AddPackagePath {

nit Per Anzu issue 4989 this directive will die ASAP. It's probably worth noting that here, or in the README where it describes the schema, or both.

Copy link
Contributor Author

@ggould-tri ggould-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 5 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @ggould-tri and @jwnimmer-tri)

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit The commits need a rebase:

(1) The first commit is the one with the copied content, so should use "Co-authored-by" syntax and list everyone. The second commit should not mention anyone except you.

(2) Saying "Resolves" in the second commit message is inaccurate -- since the code is in dev, the issue is not yet finished.

Do we typically commit these with curated commits or do we squash them?



multibody/parsing/dev/BUILD.bazel, line 62 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

We should not use install_data merely to declare a filegroup; we should only use it when we actually want to install things.

In parsing/BUILD.bazel we have:

filegroup(
    name = "test_models",
    testonly = 1,

which is more along the lines of what we should use here.

Done.


multibody/parsing/dev/model_directives.h, line 23 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Meta GSG violation -- the namespace name "schema" is not based on any directory name. I think we should remove it here.

Done.


multibody/parsing/dev/test/models/package.xml, line 3 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Especially once we lose the /dev/ part of the pathname for all of this test data, the folder name "models" does not seem sufficiently distinct from the other data that will be in multibody/parsing/test. Perhaps this folder should renamed to "process_model_directives_test", so that it is more unique and matches the name given here?

Done.


multibody/parsing/dev/test/models/simple_model.sdf, line 19 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

The comment claims that this is "non-neighboring", which is perhaps inaccurate now?

Done and clarified the actually important thing under test (that the sdf parser gets the package map from its caller)

Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM pending review of the merge plan.

Reviewed 14 of 15 files at r5.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @ggould-tri)

a discussion (no related file):

Previously, ggould-tri wrote…

Do we typically commit these with curated commits or do we squash them?

In #13823, I did use curated. I don't think squashing, or not, is a defect either way.

However, if squashing is the plan you should do it now and repush as just a single commit to the PR, so that I can review the intended squashed commit message to see the Co-authored-by syntax.

Or if you intend to curate, you should apply that label now to signal the intent. Also, the test data rename should be part of the first commit in that case; the second commit should only add and tweak files, not move them.

In either case, the merge plan should be clear and in scope for the review.


Copy link
Contributor Author

@ggould-tri ggould-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @jwnimmer-tri)

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

In #13823, I did use curated. I don't think squashing, or not, is a defect either way.

However, if squashing is the plan you should do it now and repush as just a single commit to the PR, so that I can review the intended squashed commit message to see the Co-authored-by syntax.

Or if you intend to curate, you should apply that label now to signal the intent. Also, the test data rename should be part of the first commit in that case; the second commit should only add and tweak files, not move them.

In either case, the merge plan should be clear and in scope for the review.

I think squashed is going to be simpler (backfilling the rename is likely to be messy) so I will create a squashed commit.


Copy link
Contributor Author

@ggould-tri ggould-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), labeled "do not merge" (waiting on @jwnimmer-tri)

a discussion (no related file):

Previously, ggould-tri wrote…

I think squashed is going to be simpler (backfilling the rename is likely to be messy) so I will create a squashed commit.

Squashed version pushed. This way we'll have examples of both to link from the playbook :-)


Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), labeled "do not merge" (waiting on @ggould-tri)

a discussion (no related file):

Previously, ggould-tri wrote…

Squashed version pushed. This way we'll have examples of both to link from the playbook :-)

At first glance, it doesn't seem to be showing up as multiple authors.
See https://github.blog/2018-01-29-commit-together-with-co-authors/ for instructions.
I think the problem is lack of <> on the emails.

Also, you should not probably not be a Co-authored-by yourself? Dunno.

Earlier you had "copy of model directives system from project anzu" in the message; I think something along those lines is worth keeping, to note that this is a copy (with modifications).


 * Toward RobotLocomotion#13282
 * A domain-specific language for assembling MultibodyPlant scenes
   from multiple SDF files.
 * Helpful for assembling large scenes without huge unwieldy sdf/xacro
   files.
 * A temporary accomodation until sdformat adds similar functionality.

This code is copied and adapted from TRI's Project Anzu.

Co-authored-by: Eric Cousineau <[email protected]>
Co-authored-by: Siyuan Feng <[email protected]>
Co-authored-by: Jeremy Nimmer <[email protected]>
Co-authored-by: Calder Phillips-Grafflin <[email protected]>
Copy link
Contributor Author

@ggould-tri ggould-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), labeled "do not merge" (waiting on @jwnimmer-tri)

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

At first glance, it doesn't seem to be showing up as multiple authors.
See https://github.blog/2018-01-29-commit-together-with-co-authors/ for instructions.
I think the problem is lack of <> on the emails.

Also, you should not probably not be a Co-authored-by yourself? Dunno.

Earlier you had "copy of model directives system from project anzu" in the message; I think something along those lines is worth keeping, to note that this is a copy (with modifications).

I didn't know that the co-authors was a parsed grammar rather than a convention, so I just copied from the commit log; TIL!


@jwnimmer-tri
Copy link
Collaborator

jwnimmer-tri commented Aug 18, 2020

:lgtm: feature. (No platform needed.)

The authors stuff is working now:
image

@jwnimmer-tri
Copy link
Collaborator

FYI This is ready to merge, excepting the "do not merge" tag.

Copy link
Contributor Author

@ggould-tri ggould-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-(status: do not merge) Looking good to me as well. Onward to victory.

Reviewable status: labeled "do not merge"

@ggould-tri ggould-tri merged commit 30ebf3c into RobotLocomotion:master Aug 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants