-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Atlas passive sim. #11398
Atlas passive sim. #11398
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+@avalenzu for feature review please.
Reviewable status: LGTM missing from assignee avalenzu, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @avalenzu)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review pass done. PTAL.
Reviewed 2 of 2 files at r1.
Reviewable status: 5 unresolved discussions, LGTM missing from assignee avalenzu, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @amcastro-tri)
examples/atlas/BUILD.bazel, line 6 at r1 (raw file):
load("//tools/lint:lint.bzl", "add_lint_tests") package(default_visibility = ["//visibility:public"])
Why change the default visibility?
examples/atlas/BUILD.bazel, line 8 at r1 (raw file):
"drake_cc_binary", "drake_cc_googletest", "drake_cc_library",
I think drake_cc_googletest
and drake_cc_library
are unused and should be removed.
examples/atlas/BUILD.bazel, line 20 at r1 (raw file):
add_test_rule = 1, data = [":models"], # Some test.
Nit: This comment doesn't add much. I'd take it out.
examples/atlas/run_dynamics.cc, line 70 at r1 (raw file):
// (the allowable drift speed during stiction). For two points in contact, // this is the maximum sliding speed for the points to be regarded as // stationary relative to each other (so that static friction is used).
How does this square with your above statement that "for a time-stepping model only static friction is used?"
examples/atlas/run_dynamics.cc, line 74 at r1 (raw file):
// Sanity check model size. DRAKE_DEMAND(plant.num_velocities() == 36);
Oh, good old 36. One of our favorite magic numbers.
examples/atlas/run_dynamics.cc, line 78 at r1 (raw file):
// Publish contact results for visualization. if (FLAGS_time_step > 0)
What would happen in the MultibodyPlant
constructor if FLAGS_time_step
were less than or equal to zero? I'm guessing we could never get to this point if that were the case, right?
00dfb2a
to
5e4ed4e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee avalenzu, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @avalenzu)
examples/atlas/BUILD.bazel, line 6 at r1 (raw file):
Previously, avalenzu (Andres Valenzuela) wrote…
Why change the default visibility?
Done.
examples/atlas/BUILD.bazel, line 8 at r1 (raw file):
Previously, avalenzu (Andres Valenzuela) wrote…
I think
drake_cc_googletest
anddrake_cc_library
are unused and should be removed.
Done.
examples/atlas/run_dynamics.cc, line 70 at r1 (raw file):
Previously, avalenzu (Andres Valenzuela) wrote…
How does this square with your above statement that "for a time-stepping model only static friction is used?"
I rephrased.
examples/atlas/run_dynamics.cc, line 78 at r1 (raw file):
Previously, avalenzu (Andres Valenzuela) wrote…
What would happen in the
MultibodyPlant
constructor ifFLAGS_time_step
were less than or equal to zero? I'm guessing we could never get to this point if that were the case, right?
It could be exactly equal to zero and the plant would run in continuous mode. However, joint limits are only supported in discrete mode and therefore I am requiring the time step to be strictly positive. I also reduced the time step a bit so that joint limits were better captured (as you now know from your direct collocation experience, those converge quadratically with the time step).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+@sammy-tri for platform review.
Reviewed 2 of 2 files at r2.
Reviewable status: LGTM missing from assignee sammy-tri(platform) (waiting on @sammy-tri)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First pass complete, just a couple of comments about naming things.
Reviewed 2 of 2 files at r2.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee sammy-tri(platform) (waiting on @amcastro-tri)
examples/atlas/BUILD.bazel, line 14 at r2 (raw file):
drake_cc_binary( name = "run_dynamics",
Consider changing this to something like atlas_run_dynamics
? I know it's in it's own directory, but the existing pattern seems to be for more descriptive names:
% find . -name \*run_dynamics\*
./examples/multibody/cylinder_with_multicontact/cylinder_run_dynamics.cc
./examples/multibody/bouncing_ball/bouncing_ball_run_dynamics.cc
./examples/scene_graph/bouncing_ball_run_dynamics.cc
./examples/scene_graph/dev/bouncing_ball_run_dynamics.cc
./examples/scene_graph/dev/solar_system_run_dynamics.cc
./examples/scene_graph/solar_system_run_dynamics.cc
examples/atlas/run_dynamics.cc, line 20 at r2 (raw file):
namespace { DEFINE_double(target_realtime_rate, 0.1,
https://gflags.github.io/gflags/#define says that DEFINE_foo
should always be in the global namespace.
examples/atlas/run_dynamics.cc, line 23 at r2 (raw file):
"Desired rate relative to real time (usually between 0 and 1). " "This is documented in Simulator::set_target_realtime_rate()."); DEFINE_double(simulation_time, 2.0, "Simulation duration in seconds");
BTW I was going to suggest changing this to the more common simulation_sec
, but it looks like there's 17 uses of simulation_sec
and 15 of simulation_time
, so perhaps there's not really a consensus.
5e4ed4e
to
1ee4fa7
Compare
There was a problem hiding this 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, LGTM missing from assignee sammy-tri(platform) (waiting on @sammy-tri)
examples/atlas/BUILD.bazel, line 14 at r2 (raw file):
Previously, sammy-tri (Sam Creasey) wrote…
Consider changing this to something like
atlas_run_dynamics
? I know it's in it's own directory, but the existing pattern seems to be for more descriptive names:% find . -name \*run_dynamics\* ./examples/multibody/cylinder_with_multicontact/cylinder_run_dynamics.cc ./examples/multibody/bouncing_ball/bouncing_ball_run_dynamics.cc ./examples/scene_graph/bouncing_ball_run_dynamics.cc ./examples/scene_graph/dev/bouncing_ball_run_dynamics.cc ./examples/scene_graph/dev/solar_system_run_dynamics.cc ./examples/scene_graph/solar_system_run_dynamics.cc
Fair enough. Done.
examples/atlas/run_dynamics.cc, line 20 at r2 (raw file):
Previously, sammy-tri (Sam Creasey) wrote…
https://gflags.github.io/gflags/#define says that
DEFINE_foo
should always be in the global namespace.
Done. Note that a quick search in Drake reveals a mix of inside/outside global namespace. As discussed f2f, I don't quite like the wording of those docs but probably it means we need to fix the rest of Drake?
1ee4fa7
to
f446314
Compare
@avalenzu, @sammy-tri, I added a small note on simulation speed/accuracy in the help message. I believe that will help understand the choice of time-step/target-real-time-rate. |
There was a problem hiding this 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.
Reviewable status:complete! all discussions resolved, LGTM from assignees sammy-tri(platform),avalenzu
Towards #6381.
Another case on which we can measure performance.
cc'ing @mposa, @RussTedrake
This change is