[planning] Match RobotDiagram default time step to MbP's default #21098
Conversation
jwnimmer-tri
added
the
release notes: fix
jwnimmer-tri
force-pushed
the
robot-diagram-builder-default-time-step
branch
from
e34a368 to
2b8e5fd
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
jwnimmer-tri
force-pushed
the
robot-diagram-builder-default-time-step
branch
from
2b8e5fd to
9d0741a
Compare
jwnimmer-tri
force-pushed
the
robot-diagram-builder-default-time-step
branch
from
9d0741a to
a08530d
Compare
|
+@rpoyner-tri for feature review, please. +@calderpg-tri for extra sanity check feature review, please. +@EricCousineau-TRI for platform review per schedule, please. +(priority: medium) FYI Anzu CI passes when run against this PR. |
There was a problem hiding this comment.
Reviewed 2 of 3 files at r1, 3 of 3 files at r3, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignees rpoyner-tri(platform),EricCousineau-TRI(platform),calderpg-tri
planning/robot_diagram_builder.h line 32 at r3 (raw file):
DRAKE_NO_COPY_NO_MOVE_NO_ASSIGN(RobotDiagramBuilder) /** Constructs with the specified time step for the contained plant. */
I think it would be helpful to document the default value here, and why a non-zero value makes sense for uses that aren't simulation.
This fixes some crashes when using models that need discrete-time-only modeling features, such as implicit PD control.
jwnimmer-tri
force-pushed
the
robot-diagram-builder-default-time-step
branch
from
a08530d to
326b169
Compare
There was a problem hiding this comment.
Reviewed 2 of 3 files at r1, 3 of 3 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignees rpoyner-tri(platform),EricCousineau-TRI(platform),calderpg-tri
planning/robot_diagram_builder.h line 32 at r3 (raw file):
Previously, calderpg-tri wrote…
I think it would be helpful to document the default value here, and why a non-zero value makes sense for uses that aren't simulation.
Good point; done.
In terms of explaining mimic joint stuff in detail, I'll defer to the MbP documentation to explain it. Lots of users are baffled by it, but I don't want to bite off solving those doc problems in this PR, since we don't have Alejandro available for review.
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignees rpoyner-tri(platform),EricCousineau-TRI(platform),calderpg-tri
planning/robot_diagram_builder.h line 32 at r3 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Good point; done.
In terms of explaining mimic joint stuff in detail, I'll defer to the MbP documentation to explain it. Lots of users are baffled by it, but I don't want to bite off solving those doc problems in this PR, since we don't have Alejandro available for review.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: LGTM missing from assignees rpoyner-tri(platform),EricCousineau-TRI(platform)
There was a problem hiding this comment.
Reviewed 1 of 3 files at r1, 3 of 3 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: LGTM missing from assignee rpoyner-tri(platform)
There was a problem hiding this comment.
Reviewed 1 of 3 files at r1, 3 of 3 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status:complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),EricCousineau-TRI(platform),calderpg-tri
…otLocomotion#21098) This fixes some crashes when using models that need discrete-time-only modeling features, such as implicit PD control.
This fixes some crashes when using models that need discrete-time-only modeling features, such as implicit PD control.
Closes #21095.
This change is