MultibodyPlant option to choose contact solver

[amcastro-tri]

With SAP coming online, we need a mechanism for the user to choose their solver for discrete MultibodyPlant's. Right now we'll offer two options: TAMSI(default, for backwards compatibility) and SAP.

The first idea is of course having an additional field in MultibodyPlantConfig, say MultibodyPlantConfig::contact_solver.

More specific questions include:

1. When should the user be able to choose the solver?
   1. Once at construction.
   2. pre-finalize
   3. post-finalize.

Option 1.i has the advantage that we would know at "parsing time" what solver the user chose. That would allow the parser to error/warn whenever a given model feature is not supported by a given solver. Option 1.ii would allow checking model/solver consistency at Finalze() Option 1.iii would require for a lazy check of model/solver consistency during the actual computation.

2. For modeling elements specified at parsing that a given solver does not support (e.g. Hung&Crossley dissipation is no supported by SAP, and will never be)
   1. When should we error/warn? parsing, finalize or computation time?

Proposed Option.

Have a MultibodyPlant constructor from a MultibodyPlantConfig, so that the solver is chosen right at construction and can never be changed again. We could relax this in the future, but probably this is the simplest way to make immediate progress?

Then we'd have:

1. MultibodyPlant(const MultibodyPlantConfig& config). Once instantiated, the model is fixed.
2. DiscreteContactSolver get_discrete_contact_solver() const; // so that for instance the parser can perform appropriate checking.

[jwnimmer-tri]

The MultibodyPlantConfig should only ever be sugar atop an existing API. (Same goes for any of the other Config structs.) The MbPConfig component depends on the MbP component, not the other way around.

That means adding enum ChooseSomeSolver { ... } nearby the plant, and then taking that enum either in the constructor, or via a Set... function.

Another valid alternative would be something like SetSolver(unique_ptr<MySolveSubclass>), which would be more extensible. You know I hate it when "someone wants to research a new contact solver" means "fork drake and hack the build". I would really like to have (eventually) the ability for a third-party researcher to write a solver, without forking Drake.

Option 1.i has the advantage that we would know at "parsing time" what solver the user chose. That would allow the parser to error/warn whenever a given model feature is not supported by a given solver.

Allowing the solver to change pre-finalize would not preclude the parser from warning about things. The parser could still warn in the common case where the user has set their solver and then parsed a model. It's true, it might miss out on warning in case the user changes the solver later, but that's not the end of the world. The call to Finalize would still bomb with a nice message, as a last resort.

[amcastro-tri]

Another valid alternative would be something like SetSolver(unique_ptr)

We have an "experimental" API to do this already. So advanced users who would like to do this can (I've done it for the last two years).

Now, I would not promote that API to a stable API yet, that's a separate (and large) project on its own. With SAP with learned that our ContactSolver abstraction is not the best when it comes to writing a production ready solver. Yes it's great however for testing your solver prototypes. For that reason, I would not go this route. Not yet at least.

MbPConfig component depends on the MbP component

That doesn't seem to be true today? However, the multibody_plant_config_functions do depend on MBP. It seems to me however it is because a number of helper functions to convert back and forth form enum to string were placed in there, instead of placing them right next to the enums. Not sure if that was the result of a design requirement or simply for convenience. E.g. wouldn't it make sense for GetContactSurfaceRepresentationFromString() to live in contact_surface.h right under the definition of the enum HydroelasticContactRepresentation?. I ask because this would seem to remove the dependency you mention. But, before trying to hack my way around it, I'd like to understand this design requirement better.

[jwnimmer-tri]

... helper functions to convert back and forth form enum to string were placed in there, instead of placing them right next to the enums ...

If you want to relocate GetContactModelFromString and GetStringFromContactModel to be part of the enum ContactModel layer, it's acceptable to do that. Ditto for any other enums and their to/from string parsers.

I did not anticipate that anyone except config parsing would like to use those functions, so I chose not to bloat the MbP header file with those declarations. A bajillion places include the plant header, while only tens of places include config header, and the less we shove into the plant header, the faster everyone's builds will be. However, if you think that people besides the config parser would frequently enjoy using those functions, it's fine to promote them closer to the enum declaration.

MbPConfig component depends on the MbP component

That doesn't seem to be true today?

You're right, I'd forgotten about the split between multibody_plant_config.h and multibody_plant_config_functions.h.

Even if we could get the build system and header files working, the software architecture remains the same -- the config should be sugar atop the baseline MbP API.

A single Config struct will have an ever-growing number of settings (basically, most of setters on the MbP all rolled into one). Forcing the user to use that heavy struct as the only way to customize the solver mixes too many concerns together.

Similarly, we don't want to make users type bare strings into their program to configure things. We want to offer an enum for compile-time checking, so that we fail-fast when then make a typo. The bare strings should be reserved for config files.

[amcastro-tri]

Thanks for the explanation Jeremy.

the config should be sugar atop the baseline MbP API.

I completely agree.

Forcing the user to use that heavy struct as the only way to customize the solver mixes too many concerns together.

Maybe. Withe the "MbPConfig passed at construction" idea I was trying to make it so that the solver was set on stone while parsing, which in my mind led to simpler code. But this does not seem to be a problem in your mind.

So to be clear, you think the best API to choose a solver would be something like: MbP::set_contact_solver(SomeContactSolverTypeEnum)?

[jwnimmer-tri]

I don't have a strong opinion on constructor vs setter, I think it's fine for #dynamics to decide that. We require time_step in the constructor, and the contact solver maybe isn't that different.

My point is that you could require both the time_step and the enum SomeContactSolverTypeEnum for the constructor if you prefer (as two arguments); you don't need use the Config struct for that.

[sherm1]

A non-zero time step in the MbP constructor is specifically for use with a discrete solver. So it makes sense to me that the constructor could also specify which solver at the same time (with a suitable default that we can change to SAP later). Should barf if a 0 time step comes with a discrete solver specified though.

[amcastro-tri]

I like this idea of being able to specify the solver as an additional argument with a default in the constructor. I like it in that in a first pass the solver is set in stone and known at parsing time. I think this would ease the process of error/warnings when a given model feature is not supported by the given solver.

Related to #17087 on the parameterization of dissipation for SAP. @rpoyner-tri, any thoughts?

[jwnimmer-tri]

Assuming that this is the current proposal:

MultibodyPlant::MultibodyPlant(double time_step, DiscreteContactSolver solver = DiscreteContactSolver::kTamsi);
DiscreteContactSolver MultibodyPlant::discrete_solver_type() const;

What will happen when time_step == 0.0 (i.e., continuous time)?

It seems to me like discrete_solver_type() should not be valid on a continuous-time plant, and allowing the user to pass a custom solver enum to the constructor along with 0.0 indicates a bug in the user's understanding of how things work.

How about this instead?

/// When `time_step > 0`, the given `discrete solver` will be used (or if none is given
/// a default solver will be chosen automatically).  When `time_step == 0.0`, the solver
/// must not be provided (i.e., must be null).
MultibodyPlant::MultibodyPlant(
    double time_step,
    std::optional<DiscreteContactSolver> solver = std::nullopt);

/// When this plant `is_discrete()`, returns the (non-null) discrete solver in use.
/// When this plant is continuous, returns null.
std::optional<DiscreteContactSolver> MultibodyPlant::discrete_contact_solver() const;

[RussTedrake]

Seems like we have set_discrete_contact_solver API (and MultibodyPlantConfig support). Should we close this?

[rpoyner-tri]

Seems closeable to me.