+@jwnimmer-tri for feature review please.

Reviewable status: LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers

Here's a checkpoint on some low-level style stuff, so you can start making changes.

Tomorrow I'll circle back and start a top-down review where I check for and missing/wrong documentation and work down from there.

Reviewed 3 of 7 files at r1, all commit messages.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers

-- commits line 2 at r1:
BTW What commit subject to you want us to use when we merge this? The PR title, or this subject? (Maybe you can change the unwanted one to match the other.)

multibody/plant/multibody_plant.h line 5736 at r1 (raw file):

  // MultibodyPlant::get_actuation_input_port()). A model instance is "armed" if
  // model_instance_has_armed_pds[JointActuator::model_instance()] is `true`.
  std::pair<VectorX<T>, std::vector<bool>> AssembleDesiredStateInput(

Here and throughout --

See https://drake.mit.edu/styleguide/cppguide.html#Structs_vs._Tuples. Since these quantities could (should) have meaningful names, we should do so by using a helper struct.

multibody/plant/multibody_plant.h line 5736 at r1 (raw file):

  // MultibodyPlant::get_actuation_input_port()). A model instance is "armed" if
  // model_instance_has_armed_pds[JointActuator::model_instance()] is `true`.
  std::pair<VectorX<T>, std::vector<bool>> AssembleDesiredStateInput(

Here and throughout --

The class std::vector<bool> is an abomination, and must never be used. (Sorry, we don't have this in our style guide yet.)

An example of valid alternatives would be VectorX<bool> or std::vector<uint8_t>. Or, we could use a set instead like std::unordered_set<ModelInstanceIndex> disarmed_model_instances;.

Code quote:

std::vector<bool>

Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers

multibody/plant/multibody_plant.h line 5736 at r1 (raw file):

wow! I knew this was a problem like 10 yrs ago, I would've thought someone put and end to it!
Sighs.... really? VectorX<bool>? that sounds as horrible to me.
Anyway, I'll use one of your alternatives.

Checkpoint on everything except the tests.

Reviewed 3 of 7 files at r1.
Reviewable status: 8 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers

multibody/plant/multibody_plant.h line 613 at r1 (raw file):

An exception will be thrown when evaluating the actuation input ports if only a subset of the actuators in a model instance is PD controlled.

Do we want this to still be true? If someone loads a model that has 2 joints, and then later assigns PD gains to only one of the joints, but never connects the desired state input port, should we still throw an exception? There isn't actually any problem with our computation -- we can unambiguously compute the dynamics. Throwing would only be trying to "save them from themselves" in case in a future version of their models they did connect the port but forgot to set the gains.

multibody/plant/multibody_plant.h line 5736 at r1 (raw file):

C++ doesn't have epochs, so basically they can never change it.

a discussion (no related file):
BTW In joint_actuator.h, do we want to add more text to clarify?

  /// Returns `true` if controller gains have been specified with a call to
  /// set_controller_gains().
  bool has_controller() const { return pd_controller_gains_.has_value(); }

We might mention that the controller is created by setting gains, but will only be armed if the MbP input port is connected and the joint has not been locked.

multibody/plant/multibody_plant.h line 1143 at r1 (raw file):

  /// const double qd_actuator = xd[actuator.input_start()];
  /// const double vd_actuator =
  ///    xd[actuator.input_start() + plant.num_actuated_dofs()];

This offset doesn't seem right to me. Everything in this sample code is per-model-instance offsets, isn't it?

Actually, on second look, the input_start() index is also plant-wide isn't it, but this input port is per-model-instance, so the entire sample code is broken, I think?

Suggestion:

plant.num_actuated_dofs(model_instance)

multibody/plant/multibody_plant.h line 1151 at r1 (raw file):

  ///
  /// @warning It is required to connect this port for PD controlled model
  /// instances.

This is no longer true.

Code quote:

  /// @warning It is required to connect this port for PD controlled model
  /// instances.

multibody/plant/sap_driver.cc line 742 at r1 (raw file):

        plant().get_joint_actuator(actuator_index);
    if (!model_instance_has_armed_pds[actuator.model_instance()]) continue;
    if (actuator.has_controller()) {

nit It's inconsistent to have one guard use continue but the other guard indent inside the if-true block.

Probably changing both to be "continue" is best, but it would also be okay to conjoin both conditions into the existing if guard.

Suggestion:

    if (!model_instance_has_armed_pds[actuator.model_instance()]) continue;
    if (!actuator.has_controller()) { continue; }