Evaluate all trigger conditions before executing any actions.

[truher]

see [CD thread] (https://www.chiefdelphi.com/t/trigger-condition-evaluation/500280)

Describe the bug
The EventLoop evaluates Triggers in insertion order. Early trigger actions can affect the antecedents of later trigger evaluations, which can have surprising effects: imagine there are two triggers on the same condition, but the first one resets the condition: the second one will never run. This is easy to see in a trigger-driven "state machine" -- one of the common triggers involves state matching, and one of the common actions is to change the state.

To Reproduce
Here's the example I posted on CD.

Here’s an example of the bad thing, which chains the “exit foo” event in a way that it is never seen (i.e. the “B” never appears):

class Bar {
    boolean foo; 
    void bar() {
        new Trigger(() -> foo).and(m_controller::getXButton)
                .onTrue(parallel(runOnce(() -> {
                    foo = false;
                }), print("A")))
                // onFalse never fires, because its 'previous' condition was false, and the
                // onTrue handler above resets it to false before onFalse handler sees the true
                // state.
                .onFalse(print("B"));
        new Trigger(() -> !foo).and(m_controller::getYButton)
                // here onFalse fires, because it sees the true transition
                .onFalse(print("D"))
                .onTrue(parallel(runOnce(() -> {
                    foo = true;
                }), print("C")));
    }
}

Here’s an example that seems better, with “on enter” and “on exit” events separately bound. It's not equivalent, but for most uses, it's probably better anyway?

class Bar {
    boolean foo; 
    void bar() {
        // transition foo -> not-foo
        new Trigger(() -> foo).and(m_controller::getXButton).onTrue(runOnce(() -> {
            foo = false;
        }));
        // enter foo
        new Trigger(() -> foo).onTrue(print("A"));
        // exit foo
        new Trigger(() -> foo).onFalse(print("B"));
        // transition not-foo -> foo
        new Trigger(() -> !foo).and(m_controller::getYButton).onTrue(runOnce(() -> {
            foo = true;
        }));
        // enter not-foo
        new Trigger(() -> !foo).onTrue(print("C"));
        // exit not-foo
        new Trigger(() -> !foo).onFalse(print("D"));
    }
}

Expected behavior
I would expect all the conditions to be evaluated before any actions are executed. This wouldn't completely solve the problem -- imagine conditions with condition-modifying side effects -- but I bet if you write a condition with a side effect, you really know what you're doing. sigh. const.

Additional context
The most obvious way to fix this, to me, is to change EventLoop considerably (so it uses some sort of task object instead of Runnable), but EventLoop is used all over. There could be a new thing, just for Triggers.

Another option is to just document the behavior, and come back to this topic in 2027, or something.

Eh?

[Starlight220]

The thing is that EventLoop doesn't actually see the booleans, it just calls run on each trigger -- which is responsible for its own condition polling etc. This allows support for non-boolean events (which didn't get beyond the initial concept phase).

[truher]

yeah, 100%, that's why I'm kinda thinking of something that isn't EventLoop at all. I'll try that in our code to see how it feels.

[SamCarlberg]

Fundamentally, this happens because runOnce commands are evaluated immediately when the command is scheduled. A proper fix would be to add scheduled commands to a queue, then process that queue after the event loop is polled and triggers are evaluated, instead of eagerly initializing commands in schedule.

You can work around this by using run(() -> ...).until(() -> true) to act like runOnce but execute after triggers are polled.

new Trigger(() -> foo)
    .onTrue(Commands.run(() -> {
      System.out.println("A");
      foo = false;
    }).until(() -> true))
    .onFalse(Commands.runOnce(() -> {
      System.out.println("B");
    }));

foo = true;
scheduler.run(); // prints "A"
scheduler.run(); // prints "B"

[truher]

the fact that this is fixed by commands v3 pushes it even closer to WONTFIX...

[SamCarlberg]

the fact that this is fixed by commands v3 pushes it even closer to WONTFIX...

The plan is for v2 and v3 to coexist for several seasons; it's still worthwhile to fix this behavior in v2.