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

[examples] Plumb SceneGraphConfig through hardware_sim #21443

Merged
merged 1 commit into from
May 20, 2024

Conversation

rpoyner-tri
Copy link
Contributor

@rpoyner-tri rpoyner-tri commented May 16, 2024

This change is Reviewable

Copy link
Contributor Author

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

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

+@jwnimmer-tri for (maybe both) review.

Reviewable status: LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, missing label for release notes (waiting on @rpoyner-tri)

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:

I am OK with "both" but possibly you'd like to ask a dynamics person to check that the example physical setting (point contact thing) is a sensible thing for our centerpiece example.

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: 1 unresolved discussion, needs at least two assigned reviewers, missing label for release notes (waiting on @rpoyner-tri)


examples/hardware_sim/hardware_sim.py line 86 at r1 (raw file):

    # Plant configuration (time step and contact parameters).
    plant_config: MultibodyPlantConfig = MultibodyPlantConfig()
    scene_graph_config: SceneGraphConfig = SceneGraphConfig()

nit To match the C++ file:

Suggestion:

      # SceneGraph configuration.
      scene_graph_config: SceneGraphConfig = SceneGraphConfig()

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: 2 unresolved discussions, needs at least two assigned reviewers, missing label for release notes (waiting on @rpoyner-tri)

a discussion (no related file):
BTW Please update examples/hardware_sim/example_scenarios.yaml to have a scene_graph setting. Users will probably not check the test file to read about stuff.


Copy link
Contributor Author

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

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

I changed the example setting to just repeat the compile time default compliance_type: "undefined".

Reviewable status: needs at least two assigned reviewers, missing label for release notes (waiting on @rpoyner-tri)

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW Please update examples/hardware_sim/example_scenarios.yaml to have a scene_graph setting. Users will probably not check the test file to read about stuff.

Done. I felt a little funny about adding scene_graph_config to a file that doesn't even have explicit plant_config.


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 3 of 3 files at r2, all commit messages.
Reviewable status: needs at least two assigned reviewers, missing label for release notes (waiting on @rpoyner-tri)

a discussion (no related file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

Done. I felt a little funny about adding scene_graph_config to a file that doesn't even have explicit plant_config.

I am probably too familiar with all of these details to offer an opinion about what a good example looks like. The "undefined" seems slightly awkward to my first impression.

We should probably recruit a robotics person to weigh in.


Copy link
Contributor Author

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

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

+@EricCousineau-TRI for robotics-cred and advice.

The motive here is to provide an example with SceneGraphConfig as explicit yaml data. The question for a roboticist is what kind of (hopefully non-destructive) to put in to the yaml files. We are not intending to build out a fully-docmented example here, just give a flavor of the data flow, similar to what is done here with MultibodyPlant's plant_config.

Thoughts on what data from SceneGraphConfig to use for a very-modest example?

Reviewable status: LGTM missing from assignee EricCousineau-TRI(platform), missing label for release notes (waiting on @rpoyner-tri)

@jwnimmer-tri
Copy link
Collaborator

I think as frequent scenario authors it might be worth tagging @dmcconachie-tri @IanTheEngineer @rcory for feedback on what a good example would look like.

@jwnimmer-tri
Copy link
Collaborator

Oh, also @hongkai-dai is making scenarios now too.

Copy link
Contributor

@dmcconachie-tri dmcconachie-tri left a comment

Choose a reason for hiding this comment

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

My thought would be that the example would be the mode that D&S would like users to default to; i.e.; hyodroelastics rather than simply repeat the default value of the config.

Reviewable status: LGTM missing from assignee EricCousineau-TRI(platform), missing label for release notes (waiting on @rpoyner-tri)

Copy link
Contributor

@EricCousineau-TRI EricCousineau-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_strong: with +1 to Dale's point

Reviewed 2 of 4 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: 1 unresolved discussion, missing label for release notes (waiting on @rpoyner-tri)


examples/hardware_sim/example_scenarios.yaml line 8 at r2 (raw file):

  scene_graph_config:
    default_proximity_properties:
      compliance_type: "undefined"

+1 to Dale's point - I assume this would be compliance_type: "compliant"

I assume that we want default to compliant since scene_graph_config docs state that rigid-rigid hydroelastic is unsupported

Copy link
Contributor Author

@rpoyner-tri rpoyner-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, missing label for release notes (waiting on @rpoyner-tri)


examples/hardware_sim/example_scenarios.yaml line 8 at r2 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

+1 to Dale's point - I assume this would be compliance_type: "compliant"

I assume that we want default to compliant since scene_graph_config docs state that rigid-rigid hydroelastic is unsupported

Done.

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 2 of 2 files at r3, all commit messages.
Dismissed @EricCousineau-TRI from a discussion.
Reviewable status: missing label for release notes (waiting on @rpoyner-tri)

@jwnimmer-tri jwnimmer-tri added the release notes: none This pull request should not be mentioned in the release notes label May 20, 2024
@rpoyner-tri rpoyner-tri merged commit c953af0 into RobotLocomotion:master May 20, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: none This pull request should not be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants