allwpilib
allwpilib copied to clipboard
ProxyAll and explicit requireUnion command factories
resolves #5604, resolves #5294
Based on further discussion in the discord, mutable command requirements shouldn't be supported. That leaves several possible implementations for optionally not requiring the union of member commands in command groups.
Commands.sequence(score(...), drive.followPath(...)).withUnionRequirements(false);
Commands.sequence(Commands.proxyAll(score(...), drive.followPath(...)));
Commands.sequence(score(...), drive.followPath(...)).withDefaultCommands(arm.holdState());
Commands.sequence(false, score(...), drive.followPath(...));
Out of these options, withUnionRequirements
and withDefaultCommands
would be methods of both SequentialCommandGroup
and ParallelComandGroup
. Currently, WPILib's command classes don't extend beyond the Command
interface at all, meaning these methods would be a strange exception for configuring CommandGroup
specific features. The implication of this, a general CommandGroup
superclass is undesirable, since it introduces an unnecessary inheritance tree to an otherwise simple framework.
One advantage that withUnionRequirements
over proxyAll
has is that it focuses more on results than implementation, resulting in a more obvious api. Discoverability is very important in this api, since teams must be able to easily and consistently avoid the pitfalls of default commands being blocked by requirement unions. proxyAll
is not discoverable enough, since every member of a programming team would need to constantly think about requirements to understand where to put proxyAll
, and could easily miss somewhere.
Instead, this pr introduces proxyAll
as a utility function for more discoverable command factory overloads.
For example, an auto sequence could be formed using:
Commands.sequence(false, score(Location.HIGH), drive.followPath(path), intake());
and existing commands such as
public Command followPath(Path path) {
return Commands.sequence(resetOdometry(path.start()), new ExamplePathFolowingCommand(path, odometry, kinematics, this::setVoltages), stop());
}
would still work.
It's currently half implemented in c++, any advice on how to make the code more pragmatic would be appreciated. The java version also uses streams, which could be undesirable.
I'm reverting this to purely proxyAll
for simplicity
Should I add tests here?