[cmd] Refactor RobotModeTriggers to use static variables

[spacey-sooty]

No description provided.

[github-actions]

This PR modifies commands. Please open a corresponding PR in Python Commands and include a link to this PR.

[spacey-sooty]

/format

[rzblue]

This is a breaking change. What's the motivation for this?

[spacey-sooty]

This is a breaking change. What's the motivation for this?

Using them in a factory just creates a footgun, it doesn't serve any purpose over them being a field.

[ThadHouse]

Would definitely prefer to keep them as functions. Fields lock us in way more to having to make breaking changes in the future if something needs to change. As functions, that might not always be required.

[spacey-sooty]

Functions add a really easy way for users to start causing problems with their code. Many teams have had issues with calling Trigger factories in loops or periodically creating huge amounts of objects and binding the check many times. This can cause multitudes of issues, all prevented by this.

[ThadHouse]

I'd much rather just make the function return a singleton if we wanted that behavior, rather then forcing it into a field.

[spacey-sooty]

I'd much rather just make the function return a singleton if we wanted that behavior, rather then forcing it into a field.

Fields are how this should be done in robot code and significantly simpler, why use a singleton?

[ThadHouse]

Because fields lock you down much more. Plus then this is now a breaking change. Keeping it as a function isn't breaking, and keeps our options in the future.

[spacey-sooty]

Because fields lock you down much more.

Locking down is the right behaviour here. The only way we'd want to change this is if the underlying function call changed or we needed to do more logic in the check, that can all be done in the constructor to the Triggers

[spacey-sooty]

/format

[spacey-sooty]

/format

[Starlight220]

I'm with the others here, it should stay a function -- perhaps with a caching field (similar to the HIDs).

If already, they should be functions with a defaulted EventLoop parameter (though I accept the claim that the few teams using custom EventLoops can create a trigger manually).

(Btw, modifier order should be static before final)

[rzblue]

Because fields lock you down much more.

Locking down is the right behaviour here.

Fields lock us down in the API/ABI sense. Function implementations can change safely without the user noticing; fields are much less flexible in this sense.

[Daniel1464]

Definitely agree with this change; all logic changes that could potentially rise from creating an autonomous() trigger can be easily put within the lambda of the Trigger class.

Fields lock us down in the API/ABI sense. Function implementations can change safely without the user noticing; fields are much less flexible in this sense.

In the edge case that this does happen, deprecation is a very easy option. Overall though, running side effects on the creation of an autonomous/teleop/test mode trigger feels like code smell(since the user expects them to be immutable).

[KangarooKoala]

I still don't get the motivation behind this change.

Things I've seen in this PR thread (in chronological order):

1. Factory is a footgun: How so? If you're talking about memory allocation, that's solved by caching the triggers like we do for CommandGenericHID.
2. Factory doesn't serve any purpose over being a field: As others have said, being a factory makes it easier for us to change implementation without affecting users.
3. Teams having issues with calling the factories in loops: See point 1.
4. Teams having issues periodically creating huge amounts of objects: This is solved by caching.
5. Teams having issues binding the check many times: Assuming you mean that teams are creating .onTrue() (and co.) bindings in periodic functions, how does changing from factories to variables fix this? This stems from a misunderstanding of what .onTrue() does, which isn't fixed by subtly changing how they access the trigger.
6. Fields are the way to reuse objects in robot code: WPILib is not robot code- We need to provide a clean interface to all teams that is relatively stable between years, whereas robot code is meant for one only team and has no constraints to stay the same between years.
7. Why use a singleton instead of a public field (continuation of point 6): Note that we also don't necessarily need to write the typical lazy-creation logic associated with singletons- We can just have private static final triggers and keep the API consistent by only exposing them through the factories. (Which gives us more freedom to change implementation without affecting teams)
8. Any changes to implementation could also be done with how the trigger is constructed: We can't make that guarantee about every possible change we might make later- Suppose we need to do some action upon access (e.g., usage reporting) or the implementation just gets complicated enough that stuffing everything in one expression isn't reasonable.
9. Deprecation is easy: Yes, but it's slow- It delays the change by at least a year, which in turn delays the reason for the change by at least a year.
10. Side effects from accessing the triggers are a code smell: I'm not sure where this came from, but stuff like usage reporting is an example of a somewhat reasonable kind of use. Printing a warning if the trigger is accessed from a periodic would be another kind of side effect that would be reasonable.

[Daniel1464]

I still don't get the motivation behind this change.

In my eyes removing the extra parentheses is a non-unnoticable boon in code readability, and helps distinguish a method call that returns a value from a Trigger.

10. Side effects from accessing the triggers are a code smell: I'm not sure where this came from, but stuff like usage reporting is an example of a somewhat reasonable kind of use. Printing a warning if the trigger is accessed from a periodic would be another kind of side effect that would be reasonable.

Usage reporting will end up over-reporting users that use the trigger multiple times within their code. For instance, these 2 usages wouldn't be equally reported:

autonomous()
    .onTrue(...)
    .whileTrue(...);

autonomous().onTrue(...);
autonomous().whileTrue(...);

So i don't think this should be a usage-reportable statistic in the first place. To accomplish something similar it would be better to add a handler lambda to the Trigger class that allows you to run side effects when a command is registered to the trigger.
Im not sure how you would accomplish a warning system for periodic calls of the trigger either.

[Gold856]

In my eyes removing the extra parentheses is a non-unnoticable boon in code readability

I think that the name of the method combined with the return type is already readable enough. It's not a verb like update or calculate where it's obviously doing something to get a result (updating something or calculating something). What does it mean to call autonomous or disabled?

and helps distinguish a method call that returns a value from a Trigger.

You would have to look at the return type anyways to figure out how to use the method.

Usage reporting will end up over-reporting users that use the trigger multiple times within their code. For instance, these 2 usages wouldn't be equally reported

We already do this kind of usage reporting from static methods.

Im not sure how you would accomplish a warning system for periodic calls of the trigger either.

private static int disabledCallCount = 0;
private static boolean disabledPeriodicWarning; // Bad name, but you know what I mean
public static Trigger disabled() {
  if (disabledCallCount < 200 && !disabledPeriodicWarning) {
    disabledCallCount++;
  } else {
    disabledPeriodicWarning = true;
    DriverStation.reportWarning("disabled Trigger is being called excessively. This might mean it's being called periodically. This should not be called in a periodic method.", false);
  }
  return new Trigger(DriverStation::isDisabled);
}

[Daniel1464]

private static int disabledCallCount = 0;
private static boolean disabledPeriodicWarning; // Bad name, but you know what I mean
public static Trigger disabled() {
  if (disabledCallCount < 200 && !disabledPeriodicWarning) {
    disabledCallCount++;
  } else {
    disabledPeriodicWarning = true;
    DriverStation.reportWarning("disabled Trigger is being called excessively. This might mean it's being called periodically. This should not be called in a periodic method.", false);
  }
  return new Trigger(DriverStation::isDisabled);
}

Makes sense; trigger invocations wouldn't be a problem if we cached them or just made them final properties but i guess if we were to usage-report the autonomous() trigger and such we could. I'm still a believer that it would be better accomplished via an onCommandBind() method of a trigger that can be re-mapped to usage-report the quantity of commands bound to a trigger

[spacey-sooty]

If someone's interested in solving the allocation footgun they can make these singletons