Drake: MultibodyPlant should check Context validity

[avalenzu]

Passing a diagram context into Frame::CalcPose() results in the following assertion:

abort: Failure at bazel-out/k8-dbg/bin/external/drake/systems/framework/_virtual_includes/cache_and_dependency_tracker/drake/systems/framework/cache.h:702 in get_cache_entry_value():
condition 'has_cache_entry_value(index)' failed.

I spent a fair few minutes wondering what had gone wrong in my diagram before I realized that the issue was a context mismatch. If the assertion had come from a MultibodyPlant method this would have been more apparent. We could add

DRAKE_ASSERT_VOID(System<double>::CheckValidContext(context));

in user-facing non-systems-framework methods of MultibodyPlant.

[amcastro-tri]

would you prefer ASSERT (only debug) or DEMAND?

[sherm1]

Context-validating is potentially very expensive.

[avalenzu]

I think ASSERT, definitely.

[amcastro-tri]

See related issue #11801.

[amcastro-tri]

Should this be an MBP feature or a System feature? @sherm1?

[jwnimmer-tri]

The check proposed by #12564 should never be disarmed (i.e., not only in Debug builds). For the "correctly-mated System vs Context" checking:

• System methods (like EvalTimeDerivatives) will embed the check, but
• for MbP-specific functions like GetFreeBodyPose, the MbP must call ValidateContext, because the framework can't do it.

[sherm1]

Agreed. Eventually you would probably hit a framework method that would barf on the mismatch, but the delayed detection would be less helpful for users. (Would be better if the ValidateContext method would report the name of the failing API though.)

[jwnimmer-tri]

I think that #14389 resolved this.

[jwnimmer-tri]

While #14389 resolved the issue for MbP's direct API, there are several other public classes on the MbT-related side that still lack calls to ValidateContxt, e.g. Frame::CalcPoseInWorld as noted in https://github.com/RobotLocomotion/drake/issues/12560#issuecomment-786366018.

[amcastro-tri]

How expensive are those ValidateContext()? can they safely be placed on release builds?
Also, I imagine we want them in MBP's public APIs, not MBT?

[jwnimmer-tri]

Per its API Doc, calling ValidateContext is sufficiently fast for performance sensitive code.

PR #14389 already added the checks to all of the MbP methods.

What's still needed here is to add it to any of the multibody/tree/... methods that take a Context, e.g., Frame::CalcPoseInWorld and others.

[rpoyner-tri]

@amcastro-tri do you have any work-in-progress on this? If not, I think i have a patch that should do the job.

[amcastro-tri]

Your patch is excellent. Thanks @rpoyner-tri!