allwpilib icon indicating copy to clipboard operation
allwpilib copied to clipboard

ProxyAll and explicit requireUnion command factories

Open AngleSideAngle opened this issue 1 year ago • 2 comments

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.

AngleSideAngle avatar Sep 03 '23 20:09 AngleSideAngle

I'm reverting this to purely proxyAll for simplicity

AngleSideAngle avatar Nov 11 '23 01:11 AngleSideAngle

Should I add tests here?

AngleSideAngle avatar Nov 12 '23 20:11 AngleSideAngle