RobotLocomotion / drake

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

Sugar from declaring context resources from outside of a System #15133

Closed xuchenhan-tri closed 2 years ago

xuchenhan-tri commented 3 years ago

15033 introduces mechanism to declare context resources from outside of a System. For a contrived example, see here.

Since these resources are declared from outside of the System that actually requests the resources, the sugar taking member methods as arguments (like this one) can't be used. Instead, the most generic form that takes an allocator and a calculator must be used. As a result, long lists of lambdas that are starting to form. See here for example.

It would be nice to have an additional sugar that takes a member method of a class that's not the system requesting the resource. Something along the line of:

template <class MyClass, typename OutputType>
LeafOutputPort<T>& DeclareAbstractOutputPort(
    std::variant<std::string, UseDefaultName> name, const MyClass& my_class,
    void (MyClass::*calc)(const Context<T>&, OutputType*) const,
    std::set<DependencyTicket> prerequisites_of_calc)

This would cut down the amount of boilerplate needed for the allocator.

jwnimmer-tri commented 3 years ago

From the overview text, I can't tell if you're asking for this sugar to be added to LeafSystem or to internal::DiscreteUpdateManger. Adding it to LeafSystem is insane, yet you've labeled the issue "system framework" and not "multibody plant". Can you clarify your proposal?

sherm1 commented 3 years ago

Wait, why is that insane?

jwnimmer-tri commented 3 years ago

The Declare methods are protected. The caller of them is necessarily this System, so in the typical case the Calc callback is on this. Do we have a lot of System classes that want to delegate their Calc functions to helper instance objects?

xuchenhan-tri commented 3 years ago

I originally meant adding the sugar to LeafSystem, but adding them to PhysicalManager (which is currently the only object needing extra state/ports) could also work. DiscreteUpdateManager really only needs cache entries. What about a adding similar sugar to SystemBase for cache entries? Those declares are public. Does that make the request less insane?

sherm1 commented 3 years ago

I see. I think it could still make sense to make these static methods of LeafSystem for organizational and documentation purposes. I do think we will see more helper classes that need System resources, though maybe the need is most acute for the mega-System MultibodyPlant. The sugar methods need to be able to call the (currently protected) most-general Declare method; that could be easier with a LeafSystem static. Making the most-general method public would facilitate remote sugar methods.

jwnimmer-tri commented 3 years ago

What about a adding similar sugar to SystemBase for cache entries? Those declares are public. ... Making the most-general method public would facilitate remote sugar methods.

It is a defect that they are public. Adding more public framework Declare... methods is a non-starter, whether that be widening the overload set for DeclareCacheEntry, or promoting some other DeclareFoo elements from protected to public.

These methods allow external code to violate the invariants of current systems, so must be disallowed by default. Any System that is still able to operate in the presence of externally-occurring Declare calls can opt-in to making them public, via a using-statement in its public section.

If MbP wants to elevate those methods to public with a using-statement, or provide attorney access to them for specific callers, that's within its rights. But the default access level in the framework for Declare methods that mutate this must not be public.

... need is most acute for the mega-System MultibodyPlant.

If the sole user is MbP, that is no reason to clog up the framework with more boilerplate. The overload sets for the Declare methods are already too confusing as it is.

In short, this use case is extraordinarily arcane, and any sugar to support it belongs in the MbP-related plumbing that is already specialized for this use case, not in the framework itself.

jwnimmer-tri commented 3 years ago

I should have linked it here a couple weeks ago, but for the record -- the new features in #15161 are intended to make it easier for an attorney to forward along access to the Declare methods. The details of callback sugar are encapsulated into a public class, so the only additional things that participate in forwarding are names, dependencies, etc., so in most cases a single forwarded method is sufficient to delegate access to the helpers.

xuchenhan-tri commented 3 years ago

Thanks for the link. Looking forward to the changes in #15161! The cache entry declaration for deformable sim is getting out of hand.

jwnimmer-tri commented 2 years ago

All of the #15161 PRs have merged. I believe this feature has successfully concluded.