wpilibsuite / allwpilib

Official Repository of WPILibJ and WPILibC
https://wpilib.org/
Other
1.08k stars 611 forks source link

Add `DeferredCommand` for commands that need runtime construction #5150

Closed Oblarg closed 1 year ago

Oblarg commented 1 year ago

This would work similarly to ProxyCommand except it would not expose the wrapped command to the scheduler, making it more suitable for composition inside of single-subsystem command groups (ProxyCommand is designed for forking off to an unrelated requirement, and the semantics there are not well-suited to deferred execution inside of an ordinary composition).

A deferred() factory method should be added, as well.

Starlight220 commented 1 year ago

We had this problem with SelectCommand, which is why we moved the deferred functionality to ProxyCommand: when the command's construction is deferred, there's no way to figure out requirements etc. Hence, deferred are proxied.

Oblarg commented 1 year ago

DeferredCommand would declare its requirements normally, and because it would wrap the supplied command internally it would sidestep the current problem ProxyCommand has with the proxied command's requirements possibly conflicting with those of the group it's composed in.

Starlight220 commented 1 year ago

That requires duplicate declaration of requirements, which is easy to miss.

Oblarg commented 1 year ago

Not if we use subsystem factories to do the requirements implicitly.

Starlight220 commented 1 year ago

Not really... in my experience, most of the cases for deferred command construction is for multi-requirement commands.

Oblarg commented 1 year ago

I've seen a bunch of teams using it to do runtime state dependencies in their commands (e.g. generating a trajectory and then following it), which seems like a reasonable case that we should support in a way that handles the requirements correctly "by default." Currently we don't.