RobotLocomotion / drake

Model-based design and verification for robotics.
https://drake.mit.edu
Other
3.33k stars 1.26k forks source link

Add a Simulator option to return status rather than throw when an event handler fails #20298

Open sherm1 opened 1 year ago

sherm1 commented 1 year ago

Currently the Simulator's Initialize() and AdvanceTo() methods throw an exception if an event handler returns status "failed". There should be an option to have the Simulator functions return "event handler failed" status instead of throwing. This will allow calling code to recover cleanly from an event handler failure. (Motivation for this was Drake Gym's need to recover from bad samples.)

Depends on PR #20035 which enables event handlers to return status to the Simulator.

jwnimmer-tri commented 1 year ago

BTW I'm satisfied with this feature request for what it is, but I think it's worth clarifying ...

(Motivation for this was Drake Gym's need to recover from bad samples.)

The most important factor of the Drake Gym workflow is that MultibodyPlant should not throw an exception due to a failed discrete update, rather it needs to return an EventStatus failure instead (and have that status not be ignored). That's the majority of the request. The Simulator feature requested here is just icing on the cake.

I worry that the team is not sufficiently clear about all of the MultibodyPlant changes still required to properly support Drake Gym, and that those changes are the priority, not this issue.

Rationale: it's on the plausible side of the fence to have DrakeGym python code catch the Simulator's thrown exception (since we could plausibly make (or assume) a basic exception safety guarantee promise for just that one function, instead of offering a new/different function; it's catching the MultibodyPlant's thrown exception that is way too brittle, because there are way too many functions on that call stack.

In short: I'd advocate for this request being relatively low priority, and the MbP fixes (which are possibly not even filed as an issue yet) as higher priority.

sherm1 commented 1 year ago

Filed #20300 for MbP's part