MultibodyPlant adds DeformableModel by default

[xuchenhan-tri]

In addition, AddMultibodyPlantSceneGraph connects the deformable related SceneGraph ports automatically.

Closes #21355

---

This change is 

[amcastro-tri]

+@amcastro-tri for an early feedback review.

Reviewable status: LGTM missing from assignee amcastro-tri, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @xuchenhan-tri)

[amcastro-tri]

First pass, for now just a few questions.

But the big picture I have thus far is:

1. MbP always has a DeformableModel by default, though empty. For all scalar types.
2. Since the default deformable model is empty, no scalar support is removed.
3. Registering deformable bodies through the model is only supported for T = double, or otherwise throws.
4. Scalar converting an MbP will remove the unsupported scalars on the new clone.

The one thing I still couldn't see is this: If I have an MbP<double> and I register a deformable body, shoudn't I remove scalar support for T != double? maybe I missed this, where does this happen?

Reviewed 6 of 42 files at r1.
Reviewable status: 13 unresolved discussions, LGTM missing from assignee amcastro-tri, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @xuchenhan-tri)

---

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

  /// Returns the output port for vertex positions of the deformable bodies in
  /// `this` plant.

nit, describe the data type that goes through this port, an Eigen::VectorX, some other struct? etc

---

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

  /// @note `this` MultibodyPlant will no longer support scalar conversion to or
  /// from symbolic::Expression after a call to this method.
  void AddPhysicalModel(std::unique_ptr<PhysicalModel<T>> model);

wondering if you want to get rid of this alltogether. Would this be needed for future MPM work?

---

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

  /// Adding a non-double DeformableModel is allowed, but registering
  /// deformable bodies with non-double scalar types is not supported yet.
  DeformableModel<T>* AddDeformableModel(

if a deformable model is added by default at construction, do we need this method?

---

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

  /// deformable bodies with non-double scalar types is not supported yet.
  DeformableModel<T>* AddDeformableModel(
      std::unique_ptr<PhysicalModel<T>> model);

interesting to see that this method takes a unique_ptr to a base class PhysicalModel when I would've expected it to take a DeformableModel directly?

Also, document return and why the (mutable) pointer semantics.

---

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

  // @throws std::exception if model is nullptr or a DummyPhysicalModel is
  // already added.
  internal::DummyPhysicalModel<T>* AddDummyModel(

consider making this private and use it for testing throug a friend testing class.

---

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

  // Removes `this` MultibodyPlant's ability to convert to the scalar types
  // unsupported by the given `component`.
  void RemoveUnsupportedScalars(

also made public for testing?

---

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

  /// nullptr if this plant doesn't own a %DeformableModel.
  /// @experimental
  const DeformableModel<T>* deformable_model() const {

If I understand correctly, now you are adding a DeformableModel by default at construction, always.
So this could never be nulllptr?

---

multibody/plant/physical_model.h line 61 at r1 (raw file):

  /* Constructs a PhysicalModel owned by the given `owning_plant`. */
  explicit PhysicalModel(MultibodyPlant<T>* owning_plant)

I'd add here details on how this pointer is used:

1. When does it get nullified. At Finalize, but I believe in this scope that happens during DeclareSystemResources()?
2. Why mutable? could you say more about how it gets used?

---

multibody/plant/physical_model.h line 118 at r1 (raw file):

    DRAKE_DEMAND(owning_plant_ != nullptr);
    DoDeclareSystemResources();
    owning_plant_ = nullptr;

this should be documented.

---

multibody/plant/physical_model_collection.h line 35 at r1 (raw file):

   @throws std::exception if `model` is not a DeformableModel. */
  DeformableModel<T>* AddDeformableModel(
      std::unique_ptr<PhysicalModel<T>> model);

I wonder why this method takes unique_ptr<PhysicalModel> instead of unique_ptr<DeformableModel>? ditto below.

Also, why they pointer semantics? if the method succeeds then we can return a reference? if not, this already throws?

---

multibody/plant/physical_model_collection.h line 45 at r1 (raw file):

  /* For the given `plant`, removes support for scalars that are not supported
   any of the PhysicalModels owned by `this` PhysicalModelCollection. */

nit,

Suggestion:

 by any of the

---

multibody/plant/physical_model_collection.h line 46 at r1 (raw file):

  /* For the given `plant`, removes support for scalars that are not supported
   any of the PhysicalModels owned by `this` PhysicalModelCollection. */
  void RemoveUnsupportedScalars(MultibodyPlant<T>* plant);

question, should this method be const?

---

multibody/plant/test/multibody_plant_scalar_conversion_test.cc line 221 at r1 (raw file):

  // double -> Expression
  std::unique_ptr<MultibodyPlant<Expression>> plant_autodiff_to_symbolic;
  EXPECT_NO_THROW(plant_autodiff_to_symbolic =

dummy question (no pund intended). This changed to NO_THROW because now DummyModel supports these conversion?

[xuchenhan-tri]

Working.

The big pictures you described are all correct.

The scalar conversion support is removed at PhysicalModel::DeclareSystemResources() step. I still need to add unit tests to confirm the scalar support removal is successfully done.

Reviewable status: 11 unresolved discussions, LGTM missing from assignee amcastro-tri, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @xuchenhan-tri)

---

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

Previously, amcastro-tri (Alejandro Castro) wrote…

nit, describe the data type that goes through this port, an Eigen::VectorX, some other struct? etc

Done

---

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

wondering if you want to get rid of this alltogether.

I'm not sure what you are referring to by "this". Do you mean the AddDummyModel function? This for testing only and unrelated to MPM.

---

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

Previously, amcastro-tri (Alejandro Castro) wrote…

if a deformable model is added by default at construction, do we need this method?

Done, moved to be private.

---

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

Previously, amcastro-tri (Alejandro Castro) wrote…

interesting to see that this method takes a unique_ptr to a base class PhysicalModel when I would've expected it to take a DeformableModel directly?

Also, document return and why the (mutable) pointer semantics.

Done. I removed the parameter of this function altogether.

---

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

Previously, amcastro-tri (Alejandro Castro) wrote…

consider making this private and use it for testing throug a friend testing class.

I'll pass on that for now if that's ok. Seems like unnecessary code bloating for an already large PR.

---

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

Previously, amcastro-tri (Alejandro Castro) wrote…

also made public for testing?

This is made public for PhysicalModelCollection to remove unsupported scalars in the owning plant.

---

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

Previously, amcastro-tri (Alejandro Castro) wrote…

If I understand correctly, now you are adding a DeformableModel by default at construction, always.
So this could never be nulllptr?

Yes, that's true after construction. But it could still be null during construction and I thought it'd be nice to cover all cases.

---

multibody/plant/physical_model.h line 61 at r1 (raw file):

When does it get nullified.

Can you explain why this information is important as part of the contract?

Why mutable? could you say more about how it gets used?

I added some notes on that.

---

multibody/plant/physical_model.h line 118 at r1 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

this should be documented.

This is documented in the declaration of the private data member owning_plant_.

---

multibody/plant/physical_model_collection.h line 35 at r1 (raw file):

I wonder why this method takes unique_ptr<PhysicalModel> instead of unique_ptr<DeformableModel>?

This makes cloning the PhysicalModelCollection easier. Each concrete PhysicalModel clones to a unique_ptr to PhysicalModel. With this signature, when cloning a PhysicalModelCollection we can go through each model owned by the source and call clone->AddFooModel(this->foo_model()->Clone())

---

multibody/plant/physical_model_collection.h line 45 at r1 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

nit,

Done

---

multibody/plant/physical_model_collection.h line 46 at r1 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

question, should this method be const?

Done

---

multibody/plant/test/multibody_plant_scalar_conversion_test.cc line 221 at r1 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

dummy question (no pund intended). This changed to NO_THROW because now DummyModel supports these conversion?

:) I'm getting a sense that the pun is very much intended.

Yes, that's correct.

[amcastro-tri]

Thanks @xuchenhan-tri, I did another pass. It's much clearer now. I still need to go through unit tests.

Reviewed 1 of 42 files at r1, 5 of 16 files at r2.
Reviewable status: 11 unresolved discussions, LGTM missing from assignee amcastro-tri, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @xuchenhan-tri)

---

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

Previously, xuchenhan-tri wrote…

wondering if you want to get rid of this alltogether.

I'm not sure what you are referring to by "this". Do you mean the AddDummyModel function? This for testing only and unrelated to MPM.

definitely I was not talking about AddDummyModel. But clearly I missplaced the comment and honestly I don't remember what I was referring to. Sorry about the confusion, ignore.

---

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

Previously, xuchenhan-tri wrote…

I'll pass on that for now if that's ok. Seems like unnecessary code bloating for an already large PR.

fair

---

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

Previously, xuchenhan-tri wrote…

This is made public for PhysicalModelCollection to remove unsupported scalars in the owning plant.

ah, thanks.

---

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

Previously, xuchenhan-tri wrote…

Yes, that's true after construction. But it could still be null during construction and I thought it'd be nice to cover all cases.

I see what you are saying. However, if I understand correctly, that's invisible to the user. As far as a user is concerned, this can never be nullptr unless a constructor failed.
As a public API, I'd therefore return a reference.
In implementation code, you can query physical_models_->deformable_model() directly if you need to check for nullptr.

---

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

  // Adds a DeformableModel to this plant. The added DeformableModel is owned
  // by `this` MultibodyPlant and calls its `DeclareSystemResources()` method at

minor,

Suggestion:

method

---

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

  // when `this` MultibodyPlant is finalized to declare the system resources it
  // needs.
  // @returns a pointer to the added DeformableModel that's valid for the life

could you document when/if this method can return nullptr?

---

multibody/plant/multibody_plant.cc line 425 at r2 (raw file):

  // on the new MultibodyTree on U. Therefore we only Finalize the plant's
  // internals (and not the MultibodyTree).
  FinalizePlantOnly();

Unless I went to the PhysicalModel and read the implementation of DeclareSystemResources(), I found it very difficult to grok where exactly scalar support was removed. It turns out, it happens inside FinalizePlantOnly by a call to DeclareStateCacheAndPorts().
I'd move the call owning_plant_->RemoveUnsupportedScalars(*this); in the PhysicalModel::DeclareSystemResources() somewhere in this scope if possible, or at least outside DeclareStateCacheAndPorts() (whose name suggests nothing about scalar support).

The hope is that by doing so this workflow for scalar support is easer to read and understand by future developers.

---

multibody/plant/physical_model.h line 61 at r1 (raw file):

Previously, xuchenhan-tri wrote…

When does it get nullified.

Can you explain why this information is important as part of the contract?

Why mutable? could you say more about how it gets used?

I added some notes on that.

I think it's useful to document how the process of adding models and its coordination with MbP works. In particular, clearly I already forgot how this works. It's nice not having to reverse engineer it every time.
The amount of detail you added now is all I needed, thanks.

---

multibody/plant/physical_model.h line 86 at r2 (raw file):

  template <typename ScalarType>
  std::unique_ptr<PhysicalModel<ScalarType>> CloneToScalar(
      MultibodyPlant<ScalarType>* plant) const {

could you say more about why a mutable plant and not a const plant?
You mention DeclareSystemResources() here. Could you also comment on whether DeclareSystemResources() was already called on this "clone" or must be done separately by the caller?

---

multibody/plant/physical_model.h line 118 at r2 (raw file):

   declare additional system resources. This method is only meant to be called
   by MultibodyPlant. We pass in a MultibodyPlant pointer so that derived
   PhysicalModels can use specific MultibodyPlant cache tickets. */

nit, no longer true. True for the virtual implementation though, you could probably move it there.

Suggestion:

   by MultibodyPlant. */

---

multibody/plant/physical_model.cc line 58 at r2 (raw file):

systems::DiscreteStateIndex PhysicalModel<T>::DeclareDiscreteState(
    const VectorX<T>& model_value) {
  DRAKE_THROW_UNLESS(owning_plant_ != nullptr);

nit, add @pre in the header

---

multibody/plant/physical_model.cc line 69 at r2 (raw file):

    typename systems::LeafOutputPort<T>::CalcCallback calc_function,
    std::set<systems::DependencyTicket> prerequisites_of_calc) {
  DRAKE_THROW_UNLESS(owning_plant_ != nullptr);

nit, add @pre in the header

---

multibody/plant/physical_model.cc line 81 at r2 (raw file):

        vector_calc_function,
    std::set<systems::DependencyTicket> prerequisites_of_calc) {
  DRAKE_THROW_UNLESS(owning_plant_ != nullptr);

nit, add @pre in the header

---

multibody/plant/physical_model_collection.h line 35 at r1 (raw file):

Previously, xuchenhan-tri wrote…

I wonder why this method takes unique_ptr<PhysicalModel> instead of unique_ptr<DeformableModel>?

This makes cloning the PhysicalModelCollection easier. Each concrete PhysicalModel clones to a unique_ptr to PhysicalModel. With this signature, when cloning a PhysicalModelCollection we can go through each model owned by the source and call clone->AddFooModel(this->foo_model()->Clone())

not sure if I understand your concenr. I added a lil snippet as a comment in the cc file. I believe it does what you need and its simpler to read. PTAL.

---

multibody/plant/physical_model_collection.cc line 18 at r2 (raw file):

  deformable_model_ =
      static_cast<DeformableModel<T>*>(owned_models_.back().get());
  return deformable_model_;

I think here a simpler implementation would read:

template <typename T>
DeformableModel<T>& PhysicalModelCollection<T>::AddDeformableModel(
    std::unique_ptr<DeformableModel<T>> model) {
  DRAKE_THROW_UNLESS(deformable_model_ == nullptr);
  std::unique_ptr<PhysicalModel<T>> physical_model(model.release());
  owned_models_.push_back(std::move(physical_model));
  deformable_model_ =
      static_cast<DeformableModel<T>*>(owned_models_.back().get());
  return *deformable_model_;
}

---

multibody/plant/test/multibody_plant_scalar_conversion_test.cc line 221 at r1 (raw file):

Previously, xuchenhan-tri wrote…

:) I'm getting a sense that the pun is very much intended.

Yes, that's correct.

Thanks

[xuchenhan-tri]

Comments addressed. PTAL. There's not a whole lot of unit tests in this PR; the PR only serves as a mean of communication to show you the design. More concrete unit tests will show up in the more piecemeal PRs that I actually intend to merge.

+(status: do not merge)

Reviewable status: 6 unresolved discussions, LGTM missing from assignee amcastro-tri, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @xuchenhan-tri)

---

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

Previously, amcastro-tri (Alejandro Castro) wrote…

I see what you are saying. However, if I understand correctly, that's invisible to the user. As far as a user is concerned, this can never be nullptr unless a constructor failed.
As a public API, I'd therefore return a reference.
In implementation code, you can query physical_models_->deformable_model() directly if you need to check for nullptr.

Done

---

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

Previously, amcastro-tri (Alejandro Castro) wrote…

minor,

Done

---

multibody/plant/multibody_plant.h line 5668 at r2 (raw file):
I think the language as-is already communicates the returned pointer is not null. Happy to take suggestions if you think otherwise.

@returns a pointer ... that's valid ...

---

multibody/plant/multibody_plant.cc line 425 at r2 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

Unless I went to the PhysicalModel and read the implementation of DeclareSystemResources(), I found it very difficult to grok where exactly scalar support was removed. It turns out, it happens inside FinalizePlantOnly by a call to DeclareStateCacheAndPorts().
I'd move the call owning_plant_->RemoveUnsupportedScalars(*this); in the PhysicalModel::DeclareSystemResources() somewhere in this scope if possible, or at least outside DeclareStateCacheAndPorts() (whose name suggests nothing about scalar support).

The hope is that by doing so this workflow for scalar support is easer to read and understand by future developers.

Done

---

multibody/plant/physical_model.h line 86 at r2 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

could you say more about why a mutable plant and not a const plant?
You mention DeclareSystemResources() here. Could you also comment on whether DeclareSystemResources() was already called on this "clone" or must be done separately by the caller?

Done

---

multibody/plant/physical_model.h line 118 at r2 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

nit, no longer true. True for the virtual implementation though, you could probably move it there.

Done

---

multibody/plant/physical_model.cc line 81 at r2 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

nit, add @pre in the header

Done

---

multibody/plant/physical_model_collection.h line 35 at r1 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

not sure if I understand your concenr. I added a lil snippet as a comment in the cc file. I believe it does what you need and its simpler to read. PTAL.

I understand what the implementation would look like if I take a unique_ptr to DeformableModel, but like I said in the previous round of reply, it's easier to take a pointer to the base class because it's easier to clone the PhysicalModelCollection. See the PhysicalModelCollection::CloneToScalar.

[jwnimmer-tri]

FYI Remember that Anzu is already using this code. When we start connecting by default, we should try to prepare any Anzu fixup patches prior to merging the Drake changes.

[xuchenhan-tri]

Good call. I'll keep that in mind.

Reviewable status: 6 unresolved discussions, LGTM missing from assignee amcastro-tri, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @xuchenhan-tri)