Drake: Split simulator_gflags methods up to enable downstream use

[sherm1]

@amcastro-tri created some very useful methods for dealing with common flags in Drake simulation examples. Per discussion in PR #12900 those can be used only in Drake because they are entangled with gflags which is a nightmare for installation and distribution (per @jwnimmer-tri). Jeremy had a nice proposal there for how these could be made more generally useful, which I've captured below:

---

However, is there any substantive reason they couldn't be used downstream?

Yes, from an installation and distribution perspective, linking gflags into downstreams is kind of a nightmare. We had an eensy bit of it a few months ago and purged even that much because it was troublesome.

They seem very useful ...

The best we can probably do is to factor out the non-gflags functions into helpers that we can ship. So instead of an internal-only function that we have now ...

IntegratorBase<double>& ResetIntegratorFromGflags(Simulator<double>* simulator);

... we _also_ would offer a new function like this ...

IntegratorBase<double>& ResetIntegratorByName(const std::string& integrator_name, Simulator<double>* simulator);

The new reset-by-string function would also play much nicer with pydrake (argparse) than the gflags stuff.

Basically, just tease apart how the flags are scraped from argv (gflags) from the actions to take based upon the provided strings (these new functions). Then the simulator_gflags.h becomes just a tiny bit of sugar that wires the two concepts together.

This is what we've already done with set_log_level() from common/text_logging.h (public) vs common/add_text_logging_gflags.cc (internal).

[sherm1]

Assigning to @amcastro-tri initially for lack of an obvious assignee. Very low priority since no current demand; could be a warmup project for a new Drake developer?

[sherm1]

Did #13013 resolve this issue @jwnimmer-tri ?

[jwnimmer-tri]

It's certainly much easier now, but looking at ...

https://github.com/jwnimmer-tri/drake/blob/3cae4801eac3f9cc3d948c193022ebb8bfba5124/systems/analysis/simulator_gflags.cc#L62-L103

... there's still some flags-manipulation that is non-trivial for downstream code to replicate. In other words, there is more to those methods than just pasting gflags globals into function calls.

It's possible that we need a struct SimulatorParams helper to really make the gflags integration simple.

[jwnimmer-tri]

I need this now; I'll work on it.

[jwnimmer-tri]

\CC @avalenzu

[jwnimmer-tri]

We have a bit of code in Anzu that helps with this. We'll work on publishing it soon.

• [x] #14103 Copy files from Anzu & adapt to Drake namespace.
• [x] #14135 Integrate SimulatorConfig into simulator_[g]flags.{h,cc}.
• [x] #14150 Ensure congruence between Simulator default configuration and struct SimulatorConfig defaults.
• [x] #14150 Resolve TODO at https://github.com/RobotLocomotion/drake/pull/14103/files#diff-cc75a177e3b7894bc09f71cfa8d42627R24

[dmcconachie-tri]

For the integrator defaults, there appear to be 3 locations that these are currently stored:

• https://github.com/RobotLocomotion/drake/blob/5b7efad9ae369c82becec75ac2c820cb30d93f06/systems/analysis/simulator_gflags.cc#L44
• https://github.com/RobotLocomotion/drake/blob/5b7efad9ae369c82becec75ac2c820cb30d93f06/systems/analysis/simulator_config.h#L31
• https://github.com/RobotLocomotion/drake/blob/5b7efad9ae369c82becec75ac2c820cb30d93f06/systems/analysis/simulator.cc#L30

Presumably SimulatorConfig and simulator_gflags.cc have the default values that we want, and thus I should update the values in simulator.cc correct?

[sherm1]

Hmmm. It does seem like those should be synchronized but it's disturbing that they exist in multiple places. What do you think about these defaults @edrumwri ?

[amcastro-tri]

definitely 1.0e-3 secs is a bad value for max step size if using error control. I'd prefer it to be more like 0.1 secs.
I think 1.0e-3 secs might have made it into simulator_gflags.cc so that fixed step integrators had a more or less fair chance to deal with some of our problems. However, mostly they fail, so I IMO the max step size should be set to something sensible for error-controlled integrators.

That is:

• 1.0e-3 is a safe number if we want both error-controlled and fixed step integrators to work, though still not guaranteed.
• 0.1 makes more sense for error-controlled integrators given we want to allow them to take steps as large as they possibly can while satisfying the accuracy settings.

[sherm1]

Yeah, that's my thought also. And the default integrator is RK3, which is error controlled. I believe we thought hard long ago about the defaults in Simulator (or at least @edrumwri did) -- my inclination is to leave those as the defaults. Best I think would be if the Simulator could be asked for its defaults and then those could be used by simulator_gflags.

[jwnimmer-tri]

@sherm1 we have "Ensure congruence between Simulator default configuration and struct SimulatorConfig defaults" as a task noted above, so yes we're going to make them all align. However, we're not going to refactor the config struct to ask the simulator, because then the build footprint of the simple schema is the hugeness of all of the simulator, framework, etc. Instead, the plan is to add a regression test confirming that all of the defaults are the same. (Probably the gflags default can ask the config struct default, though -- so only two definitions of each value in the code.)

[sherm1]

That makes sense. Let's adopt the existing Simulator defaults then rather than trying to match the older adhoc gflags stuff. There might be some fallout from that in a few of the simulator_gflags use cases but those should be easily fixed.

[dmcconachie-tri]

I found additional (conflicting with simulator.cc) defaults here:
https://github.com/RobotLocomotion/drake/blob/5b7efad9ae369c82becec75ac2c820cb30d93f06/systems/analysis/simulator.h#L789
Given that the values in use are the ones in simulator.cc I'll use those values and update all the others to match/replace with a single source where reasonable.

Also here: https://github.com/RobotLocomotion/drake/blob/5b7efad9ae369c82becec75ac2c820cb30d93f06/systems/analysis/runge_kutta3_integrator.cc#L14

[sherm1]

Looks like the original intent was to have simulator.h define constants for the defaults and simulator.cc use them. So updating the simulator.h constants to the currently in-use values and then making simulator.cc use them seems like the right thing to do!

The integrators all have their own defaults. Simulator isn't obligated to use those, though it probably should.