Implement capability to define coupler constraints

[amcastro-tri]

In addition to the corresponding unit tests, this PR updates the simple gripper demo to use coupler constraints.

cc'ing @dmcconachie-tri

---

This change is 

[amcastro-tri]

+@joemasterjohn for feature review please.

Reviewable status: LGTM missing from assignee joemasterjohn, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @joemasterjohn)

[joemasterjohn]

Checkpoint, PTAL. Will look at the tests tomorrow morning.

Reviewed 11 of 14 files at r1, 2 of 2 files at r2.
Reviewable status: 14 unresolved discussions, LGTM missing from assignee joemasterjohn, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @amcastro-tri and @joemasterjohn)

---

examples/simple_gripper/simple_gripper.cc line 108 at r2 (raw file):

DEFINE_double(
    coupler_gear_ratio, -1.0,
    "Coupler constraint gear ratio when using the SAP solver. If TAMSI used, "

nit: mention that the parameter is unit-less because the joints coupled are both prismatic?

---

examples/simple_gripper/simple_gripper.cc line 291 at r2 (raw file):

  //   2. Impose a constant force to the left finger. That is, a harmonic
  //      forcing with "zero" frequency.
  const Vector3<double> amplitudes(f0, FLAGS_gripper_force, 0.0);

per f2f: gripper force should be double for SAP in order to see the same state / reaction force balance as in TAMSI.

Code quote:

FLAGS_gripper_force

---

examples/simple_gripper/simple_gripper.sdf line 131 at r2 (raw file):

             limit. We do want an actuator for this joint. -->
        <limit>
          <effort>500</effort>

nit: remove actuator for right finger for correct modelling demonstration. Only one of the joints in a coupler constraint should be actuated.

Code quote:

          <effort>500</effort>

---

multibody/plant/compliant_contact_manager.cc line 889 at r2 (raw file):

  const VectorX<T> q0 = plant().GetPositions(context);

  // Couplers have not impulse limits, they are bi-lateral constraints. Each

nit: grammar

Suggestion:

Couplers do not have impulse limits

---

multibody/plant/compliant_contact_manager.cc line 926 at r2 (raw file):

      MatrixX<T> J = MatrixX<T>::Zero(1, nv);
      // J = dg/dv
      J(0, dof0) = 1.0;

nit: It seems like the index into the constraint Jacobian should be the dof's index in the tree, but dof0 and dof1 are the indices in the global state vector, correct? Are they not different, or am I just confused here?

Code quote:

      J(0, dof0) = 1.0;
      J(0, dof1) = -info.gear_ratio;

---

multibody/plant/constraint_specs.h line 24 at r2 (raw file):

  // Second joint with position q₁.
  JointIndex joint1_index;
  // Gear ratio ρ.

nit: Maybe mention that gear_ratio may or may not have units here depending on the types of joint0 and joint1.

---

multibody/plant/discrete_update_manager.h line 222 at r2 (raw file):

  const std::vector<internal::CouplerConstraintSpecs<T>>&
  coupler_constraints_sepcs() const;

nit: typo

Suggestion:

oupler_constraints_specs() const;

---

multibody/plant/discrete_update_manager.cc line 168 at r2 (raw file):

DiscreteUpdateManager<T>::coupler_constraints_sepcs() const {
  return MultibodyPlantDiscreteUpdateManagerAttorney<
      T>::coupler_constraints_sepcs(*plant_);

nit: typo

Suggestion:

T>::coupler_constraints_specs(*plant_);

---

multibody/plant/multibody_plant.h line 1164 at r2 (raw file):

  /// Finalize() time. At this point the user has the option to either change
  /// the contact solver with set_discrete_contact_solver() or in the
  /// MultibodyPlantConfig or, to re-define the model so that such a constraint

nit: grammar

Suggestion:

MultibodyPlantConfig, or to re-define the model so that such a constraint

---

multibody/plant/multibody_plant.h line 1171 at r2 (raw file):

  int num_constraints() const { return coupler_constraints_sepcs_.size(); }

  /// Defines a holonomic constraint between two single-dof constraints `joint0`

nit: typo

Suggestion:

single-dof joints

---

multibody/plant/multibody_plant.h line 1176 at r2 (raw file):

  /// can have units if the units of q₀ and q₁ are different. For instance,
  /// between a prismatic and a revolute joint the gear ratio will specify the
  /// "pitch" of the resulting mechanism. As define, `offset` has units of `q₀`.

nit: grammar

Suggestion:

As defined,

---

multibody/plant/multibody_plant.h line 5080 at r2 (raw file):

  // Vector of coupler constraints specifications.
  std::vector<internal::CouplerConstraintSpecs<T>> coupler_constraints_sepcs_;

nit: typo. I won't note all of the locations with comments, but you have sepcs where specs should be across a few files.

Suggestion:

std::vector<internal::CouplerConstraintSpecs<T>> coupler_constraints_specs_;

---

multibody/plant/multibody_plant_discrete_update_manager_attorney.h line 121 at r2 (raw file):

  static const std::vector<internal::CouplerConstraintSpecs<T>>&
  coupler_constraints_sepcs(const MultibodyPlant<T>& plant) {
    return plant.coupler_constraints_sepcs_;

nit: typo from above

Suggestion:

coupler_constraints_specs_

---

multibody/tree/multibody_tree_indexes.h line 42 at r2 (raw file):

/// Type used to identify constraints by index within a multibody system.
using ConstraintIndex = TypeSafeIndex<class JointActuatorElementTag>;

Seems like a copy/paste error for this tag name. Also, the comment above mentions to define these in tree_py.cc in the same order, but this new index is missing from there in this PR.

Suggestion:

using ConstraintIndex = TypeSafeIndex<class ConstraintTag>;

[joemasterjohn]

OK, first pass all done. PTAL.

Reviewed 1 of 14 files at r1.
Reviewable status: 18 unresolved discussions, LGTM missing from assignee joemasterjohn, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @amcastro-tri and @joemasterjohn)

---

multibody/plant/test/compliant_contact_manager_test.cc line 1178 at r2 (raw file):

    EXPECT_EQ(plant_.num_constraints(), 0);

    // Constraint the gripper fingers to be coupled.

nit: grammar

Suggestion:

Constrain

---

multibody/plant/test/compliant_contact_manager_test.cc line 1754 at r2 (raw file):

// the coupler constraints specified in the MultibodyPlant model.
TEST_F(KukaIiwaArmTests, CouplerConstraints) {
  // Specify a state in which all kNumJionts are within limits so that we know

nit: typo

Suggestion:

kNumJoints

---

multibody/plant/test/compliant_contact_manager_test.cc line 1798 at r2 (raw file):

  // Parameters used by the test fixture to specify the coupler constraint.
  const double gear_ratio = -1.5;

nit: You could just make these members of the fixture class with accessors so you don't have to worry about the literals going out of sync.

Code quote:

  // Parameters used by the test fixture to specify the coupler constraint.
  const double gear_ratio = -1.5;
  const double offset = 3.1;

---

multibody/plant/test/compliant_contact_manager_test.cc line 1815 at r2 (raw file):

      gear_ratio * VectorXd::Unit(kNumJoints, right_index);
  const MatrixXd& J = constraint->first_clique_jacobian();
  EXPECT_EQ(constraint->first_clique_jacobian(), J_expected);

nit: duplicate check here.

Code quote:

  const MatrixXd& J = constraint->first_clique_jacobian();
  EXPECT_EQ(constraint->first_clique_jacobian(), J_expected);
  EXPECT_EQ(J, J_expected);

[joemasterjohn]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: 18 unresolved discussions, LGTM missing from assignee joemasterjohn, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @amcastro-tri)

[amcastro-tri]

Reviewable status: 4 unresolved discussions, LGTM missing from assignee joemasterjohn, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @amcastro-tri and @joemasterjohn)

---

examples/simple_gripper/simple_gripper.cc line 291 at r2 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

per f2f: gripper force should be double for SAP in order to see the same state / reaction force balance as in TAMSI.

Done. Added what I think is a much simpler to follow explanation.

---

examples/simple_gripper/simple_gripper.sdf line 131 at r2 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

nit: remove actuator for right finger for correct modelling demonstration. Only one of the joints in a coupler constraint should be actuated.

working...

---

multibody/plant/compliant_contact_manager.cc line 889 at r2 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

nit: grammar

that's my Yoda talk haha

---

multibody/plant/compliant_contact_manager.cc line 926 at r2 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

nit: It seems like the index into the constraint Jacobian should be the dof's index in the tree, but dof0 and dof1 are the indices in the global state vector, correct? Are they not different, or am I just confused here?

wow, nice catch! fixed.

---

multibody/tree/multibody_tree_indexes.h line 42 at r2 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

Seems like a copy/paste error for this tag name. Also, the comment above mentions to define these in tree_py.cc in the same order, but this new index is missing from there in this PR.

Done.

[amcastro-tri]

Thanks! back to you.

Reviewable status: 4 unresolved discussions, LGTM missing from assignee joemasterjohn, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @amcastro-tri and @joemasterjohn)

[joemasterjohn]

nice,

Reviewed 10 of 10 files at r4, all commit messages.
Reviewable status: 2 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @amcastro-tri)

---

examples/simple_gripper/simple_gripper.cc line 291 at r2 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

Done. Added what I think is a much simpler to follow explanation.

Nice! I like the explanation.

---

examples/simple_gripper/simple_gripper.cc line 292 at r4 (raw file):

  // force depends on how the gripper is modeled.
  // When we model the gripper as having the left finger locked and only the
  // actuated right finger moves, it is clear that the actuation force directly

nit: just to be consistent with the code below.

Suggestion:

  // When we model the gripper as having the right finger locked and only the
  // actuated left finger moves, it is clear that the actuation force directly

---

multibody/plant/compliant_contact_manager.cc line 926 at r2 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

wow, nice catch! fixed.

Ok, seemed suspicious. Maybe a unit test that could have sussed this out is missing?

---

multibody/tree/multibody_tree_indexes.h line 42 at r2 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

Done.

OK. Still missing in tree_py.cc though, any reason to not add it now?

[joemasterjohn]

+@EricCousineau-TRI for platform review, please.

Reviewable status: 2 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform) (waiting on @amcastro-tri and @EricCousineau-TRI)

[EricCousineau-TRI]

Sweet! First pass done, question about feature trajectory and design

Reviewed 2 of 14 files at r1, 1 of 2 files at r2, 6 of 10 files at r4, all commit messages.
Reviewable status: 8 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform) (waiting on @amcastro-tri)

a discussion (no related file):
Nice!

Is there a plan to add this to URDF / SDFormat / MJCF parsing, in some form?

---

---

examples/simple_gripper/simple_gripper.cc line 228 at r4 (raw file):

      plant.GetJointByName<PrismaticJoint>("right_slider");

  // TAMSI does not support general constraints. If using TAMSI, we simplify the

This seems confusing, and may be very unexpected to user.

Can you emit show this to user via drake::log()->warn()?

---

examples/simple_gripper/simple_gripper.cc line 293 at r4 (raw file):

  // When we model the gripper as having the left finger locked and only the
  // actuated right finger moves, it is clear that the actuation force directly
  // balances the grip force.

BTW Consider mentioning TAMSI for locked-finger case, and SAP (and maybe continuous?) for two-finger case.

---

examples/simple_gripper/simple_gripper.sdf line 131 at r2 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

working...

BTW Raised this a point below.

---

examples/simple_gripper/simple_gripper.sdf line 70 at r4 (raw file):

    </link>
    <link name="left_finger">
      <pose>-0.055 0.029 0 0 0 0</pose>

nit Unclear why you want to change these.

Consider mentioning this in your commit message or PR overview.

---

multibody/plant/multibody_plant.h line 1165 at r4 (raw file):

  /// the contact solver with set_discrete_contact_solver() or in the
  /// MultibodyPlantConfig, or to re-define the model so that such a constraint
  /// is not needed.

What should users expect about actuation for these joints?

To Joe's point, is it possible to add guidance / mitigations when using coupling across actuation? e.g.

If you need to couple actuation of two joints, make sure only one has an actuator. If you are using a model file that already defines actuators, unfortunately, you will need to change the model
```
or if mitigating
```
You will need to add magical argument `remove_actuator_for_j1` when calling the add function ...
```
or whatever.

---

multibody/plant/multibody_plant.h line 1166 at r4 (raw file):

  /// MultibodyPlantConfig, or to re-define the model so that such a constraint
  /// is not needed.
  /// @{

Dumb question: Can constraints be chained?

I'd assume generally so, e.g. to couple (q0, q1, q2), you could couple (q0, q1) and (q1, q2), or any combination therein.
Consider adding a mention of this?

---

multibody/plant/multibody_plant.h line 1179 at r4 (raw file):

  /// `q₀`.
  ///
  /// @throws std::exception if the %MultibodyPlant has already been finalized.

nit Should more explicitly mention @pre joint0 and joint1 are single-dof joints? (or @throws if not ...)

[amcastro-tri]

Reviewable status: 8 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform) (waiting on @amcastro-tri, @EricCousineau-TRI, and @joemasterjohn)

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Nice!

Is there a plan to add this to URDF / SDFormat / MJCF parsing, in some form?

Not in the short term or not from me at least.
I'd be interested to see if this is something that results useful to people and then I'd imagine if is, it'd make it into a parser.

---

---

multibody/plant/compliant_contact_manager.cc line 926 at r2 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

Ok, seemed suspicious. Maybe a unit test that could have sussed this out is missing?

I added a whole bunch of unit tests. PTAL.

---

multibody/tree/multibody_tree_indexes.h line 42 at r2 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

OK. Still missing in tree_py.cc though, any reason to not add it now?

I think it'd be useful. Though TBH I'd need help setting those up.

[amcastro-tri]

Reviewable status: 8 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform) (waiting on @amcastro-tri, @EricCousineau-TRI, and @joemasterjohn)

---

multibody/plant/multibody_plant.h line 1166 at r4 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Dumb question: Can constraints be chained?

I'd assume generally so, e.g. to couple (q0, q1, q2), you could couple (q0, q1) and (q1, q2), or any combination therein.
Consider adding a mention of this?

not a dumb question at all. Yes, what you propose actually would be the right way to do it!

[amcastro-tri]

Reviewable status: 8 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform) (waiting on @amcastro-tri, @EricCousineau-TRI, and @joemasterjohn)

---

multibody/plant/multibody_plant.h line 1165 at r4 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

What should users expect about actuation for these joints?

To Joe's point, is it possible to add guidance / mitigations when using coupling across actuation? e.g.

If you need to couple actuation of two joints, make sure only one has an actuator. If you are using a model file that already defines actuators, unfortunately, you will need to change the model
```
or if mitigating
```
You will need to add magical argument `remove_actuator_for_j1` when calling the add function ...
```
or whatever.

The reason I didn't mention it here in these docs is because these coupler constraints and actuation are two orthogonal modeling capabilities. There is no limitation on whether you can add a coupler constraint to an already actuated joint.
I can add text.

[EricCousineau-TRI]

Reviewable status: 8 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform) (waiting on @amcastro-tri, @EricCousineau-TRI, and @joemasterjohn)

a discussion (no related file):

Previously, amcastro-tri (Alejandro Castro) wrote…

Not in the short term or not from me at least.
I'd be interested to see if this is something that results useful to people and then I'd imagine if is, it'd make it into a parser.

Yeah, I think people are gonna quickly fall in love with the feature and are gonna want to know how to use with more stuff.
Consider opening issue so peeps know?

---

---

multibody/plant/multibody_plant.h line 1165 at r4 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

The reason I didn't mention it here in these docs is because these coupler constraints and actuation are two orthogonal modeling capabilities. There is no limitation on whether you can add a coupler constraint to an already actuated joint.
I can add text.

But what happens when you add an actuator for both of the actuated joints? Will they fight each other, will it cause an error, will pigs fly?

That will be a common workflow question, and I think it'd be good to have some form of guidance for this, perhaps even asserting, even for this prelim feature?

[amcastro-tri]

Reviewable status: 5 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform) (waiting on @EricCousineau-TRI and @joemasterjohn)

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Yeah, I think people are gonna quickly fall in love with the feature and are gonna want to know how to use with more stuff.
Consider opening issue so peeps know?

#17663

---

---

examples/simple_gripper/simple_gripper.cc line 228 at r4 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

This seems confusing, and may be very unexpected to user.

Can you emit show this to user via drake::log()->warn()?

Done.

---

examples/simple_gripper/simple_gripper.sdf line 131 at r2 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

BTW Raised this a point below.

Done.

---

examples/simple_gripper/simple_gripper.sdf line 70 at r4 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit Unclear why you want to change these.

Consider mentioning this in your commit message or PR overview.

ah.. that was me experimenting with number.
What I did now is what we could not before, that is, I made the model symmetric. I added notes along with that in this file.
I'll update the PR description accrodingly.

---

multibody/plant/multibody_plant.h line 1165 at r4 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

But what happens when you add an actuator for both of the actuated joints? Will they fight each other, will it cause an error, will pigs fly?

That will be a common workflow question, and I think it'd be good to have some form of guidance for this, perhaps even asserting, even for this prelim feature?

Updated docs with notes on this.

---

multibody/plant/multibody_plant.h line 1166 at r4 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

not a dumb question at all. Yes, what you propose actually would be the right way to do it!

Good question. I updated notes with this example.

[joemasterjohn]

Reviewed 1 of 1 files at r5, 3 of 3 files at r6, all commit messages.
Reviewable status: 7 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform) (waiting on @amcastro-tri and @EricCousineau-TRI)

---

multibody/plant/test/compliant_contact_manager_test.cc line 1781 at r6 (raw file):

// This test verifies that the manager properly added holonomic constraints for
// the coupler constraints specified in the MultibodyPlant model.
TEST_F(KukaIiwaArmTests, CouplerConstraints) {

BTW: nice test!

---

multibody/plant/test/compliant_contact_manager_test.cc line 1866 at r6 (raw file):

  // Verify each of the coupler constraints.
  for (int i = 0; i < 3; ++i) {
    // Get the one and only constraint of the problem.

nit: comment needs update.

Code quote:

// Get the one and only constraint of the problem.

---

multibody/plant/test/compliant_contact_manager_test.cc line 1874 at r6 (raw file):

    ASSERT_NE(constraint, nullptr);

    // There is a single clique in this model corresponding to the single

nit: comment needs update.

Code quote:

    // There is a single clique in this model corresponding to the single
    // kinematic tree composed of the robot arm and gripper.

---

multibody/tree/multibody_tree_indexes.h line 42 at r2 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

I think it'd be useful. Though TBH I'd need help setting those up.

I believe it would just be adding the single line:

  BindTypeSafeIndex<ConstraintIndex>(m, "ConstraintIndex", doc.ConstraintIndex.doc);

to this function

[amcastro-tri]

Reviewable status: 7 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform) (waiting on @amcastro-tri and @EricCousineau-TRI)

---

multibody/tree/multibody_tree_indexes.h line 42 at r2 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

I believe it would just be adding the single line:

  BindTypeSafeIndex<ConstraintIndex>(m, "ConstraintIndex", doc.ConstraintIndex.doc);

to this function

I don't see the value for binding this if we do not bind AddCouplerConstraint (this PR?)
If this PR, what is the closest template example to this method and how should I (smoke?) test?

[amcastro-tri]

@joemasterjohn-TRI, @EricCousineau-TRI, could you double-check my python bindings+testing strategy? It compiles and runs in my machine....

Reviewable status: 7 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform) (waiting on @amcastro-tri, @EricCousineau-TRI, and @joemasterjohn)

[joemasterjohn]

on the bindings+testing.

Reviewed 3 of 3 files at r7, 1 of 1 files at r8, all commit messages.
Reviewable status: 6 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform) (waiting on @amcastro-tri and @EricCousineau-TRI)

---

bindings/pydrake/multibody/test/plant_test.py line 1924 at r8 (raw file):

            gear_ratio=1.2, offset=3.4)

        # Constraints indexes are assigned in increasing order starint at zero.

nit: typo

Suggestion:

staring

[EricCousineau-TRI]

platform, looks great! Just some small remaining comments, and one blocker on comment accuracy

Reviewed 1 of 2 files at r2, 3 of 10 files at r4, 3 of 3 files at r6, 2 of 3 files at r7, 1 of 1 files at r8, 1 of 1 files at r9, all commit messages.
Reviewable status: 4 unresolved discussions (waiting on @amcastro-tri and @EricCousineau-TRI)

a discussion (no related file):

Previously, amcastro-tri (Alejandro Castro) wrote…

#17663

OK Thanks!

---

---

examples/simple_gripper/simple_gripper.sdf line 7 at r9 (raw file):

       This file defines the model for a simple gripper having two fingers.
       The right finger is fixed to the body of the gripper. The left finger

The comment regarding "right finger is fixed" is no longer accurate.

Can that be updated?

---

examples/simple_gripper/simple_gripper.sdf line 17 at r9 (raw file):

       simple_gripper.cc to add contact geometry programmatically.

      simple_gripper.cc adds pads to the model programatically. Including the

nit Indentation here is one space off.

---

multibody/plant/constraint_specs.cc line 7 at r6 (raw file):

DRAKE_DECLARE_CLASS_TEMPLATE_INSTANTIATIONS_ON_DEFAULT_SCALARS(
    struct ::drake::multibody::internal::CouplerConstraintSpecs);

BTW Is this a rogue / extra newline?

---

multibody/plant/multibody_plant.h line 1165 at r4 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

Updated docs with notes on this.

Thanks!

The only thing missing here is the suggested workflow (or a TODO to define it as such).
i.e. "For simplicity, you may only want to actuate one of the coupled joints."

Consider adding one of the two? (the suggestion, or the TODO?)
(non-blocking, feel free to ignore)

---

multibody/plant/test/compliant_contact_manager_test.cc line 1781 at r6 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

BTW: nice test!

BTW yeah, indeed!!!

[EricCousineau-TRI]

Reviewable status: 3 unresolved discussions (waiting on @amcastro-tri)

---

examples/simple_gripper/simple_gripper.sdf line 70 at r4 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

ah.. that was me experimenting with number.
What I did now is what we could not before, that is, I made the model symmetric. I added notes along with that in this file.
I'll update the PR description accrodingly.

OK Thanks!

[amcastro-tri]

Reviewable status: 3 unresolved discussions (waiting on @amcastro-tri and @EricCousineau-TRI)

---

multibody/plant/multibody_plant.h line 1165 at r4 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Thanks!

The only thing missing here is the suggested workflow (or a TODO to define it as such).
i.e. "For simplicity, you may only want to actuate one of the coupled joints."

Consider adding one of the two? (the suggestion, or the TODO?)
(non-blocking, feel free to ignore)

Not sure if the comment belongs here? or maybe to the AddCouplerConstraint below?

In any case, I think attempting to explain the cross product of all combinations possible is not a good idea at this scope?
The docs are open simply because the options are truly open; constraints/actuators are orthogonal modeling choices. My hope is that the examples showcase this clearly enough.

[EricCousineau-TRI]

Reviewable status: 2 unresolved discussions (waiting on @amcastro-tri)

---

multibody/plant/multibody_plant.h line 1165 at r4 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

Not sure if the comment belongs here? or maybe to the AddCouplerConstraint below?

In any case, I think attempting to explain the cross product of all combinations possible is not a good idea at this scope?
The docs are open simply because the options are truly open; constraints/actuators are orthogonal modeling choices. My hope is that the examples showcase this clearly enough.

OK I'm pretty sure people will be confused, so it'll come up; but yeah, maybe just wait for questions on StackOverflow or what not. Thanks!

[amcastro-tri]

Reviewable status: 1 unresolved discussion (waiting on @EricCousineau-TRI)

---

examples/simple_gripper/simple_gripper.sdf line 7 at r9 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

The comment regarding "right finger is fixed" is no longer accurate.

Can that be updated?

good catch, done.

---

examples/simple_gripper/simple_gripper.sdf line 17 at r9 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit Indentation here is one space off.

nice, thanks!

---

multibody/plant/multibody_plant.h line 1165 at r4 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

OK I'm pretty sure people will be confused, so it'll come up; but yeah, maybe just wait for questions on StackOverflow or what not. Thanks!

No matter how much docs we write, new questions will arise. However there is a place for different kind of questions. IMO some of your questions belong to a tutorial rather (I really wanna learn how to write one this quarter!).

Now, did you see the @notes I put below? I believe those address this questions you have?

[amcastro-tri]

From f2f, I dismissed you from the discussion in the SDF file @EricCousineau-TRI since I believe I addressed the problem.
I'll go ahead and merge.
Leh me know if there's anything else I need to address and I can push a quick fix PR.

Dismissed @EricCousineau-TRI from a discussion.
Reviewable status: complete! all discussions resolved, LGTM from assignees EricCousineau-TRI(platform),joemasterjohn (waiting on @EricCousineau-TRI and @joemasterjohn)

[EricCousineau-TRI]

multibody/plant/multibody_plant.h line 1165 at r4 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

No matter how much docs we write, new questions will arise. However there is a place for different kind of questions. IMO some of your questions belong to a tutorial rather (I really wanna learn how to write one this quarter!).

Now, did you see the @notes I put below? I believe those address this questions you have?

OK Ya, that answers the general question; and sweet that you wanna write a tutorial! Yeah, I think using the tutorial as a means to provide a suggested workflow could work too (if it makes sense).
Thanks!!!

[sherm1]

Very nice, Alejandro!

[jwnimmer-tri]

Reviewable status: 3 unresolved discussions

---

bindings/pydrake/multibody/test/plant_test.py line 1897 at r10 (raw file):

    @numpy_compare.check_all_types
    def test_coupler_constraint_api(self, T):
        MultibodyPlantConfig()

This is quite an odd line to begin this test case with. How is a no-crash check on the config struct related to the coupler constraint api?

---

bindings/pydrake/multibody/test/plant_test.py line 1901 at r10 (raw file):

                                      discrete_contact_solver="sap")
        self.assertEqual(config.time_step, 0.01)
        self.assertEqual(config.discrete_contact_solver, "sap")

Why are we checking the MultibodyPlantConfig struct for property-getter correctness as part of the the coupler constraint unit test? Isn't that what test_multibody_plant_config is supposed to be for?

---

bindings/pydrake/multibody/test/plant_test.py line 1907 at r10 (raw file):

        plant, scene_graph = AddMultibodyPlant(config, builder)
        self.assertIsNotNone(plant)
        self.assertIsNotNone(scene_graph)

Why are testing checking for nulls here? The test_multibody_plant_config already checks for correctness of AddMultibodyPlant. If they are null somehow from an unexpected cause, we'll get an error message later on in this test code anyway (e.g. "can't call method on None").