Setting expression properties on `state_entered` causes some weird behaviors with automatic transitions

[MidZik]

I have a statechart which has a part that looks roughly like this:

x (compound state)
    Idle
        On Grab (Go to Try Grabbing)
    Try Grabbing ( state_entered / do_grab_logic() )
        To Grabbing Something (Go to Grabbing Something automatically)
    Grabbing Something
        On grab_amount == 0 (Go to Grabbing Nothing)
    Grabbing Nothing
    On Release (Go to Idle)

do_grab_logic() updates the grab_amount expression property.

The end result is that when transitioning to Try Grabbing, do_grab_logic() causes automatic transitions to be evaluated due to the expression property being changed. This breaks some things, as transitions are now being evaluated and executed in the middle of a transition. Check out the history as shown in the debugger:

[576]: Transition: On Grab from x/Idle to x/Try Grabbing
[576]: exiT: Idle 
[576]: Transition: To Grabbing from x/Try Grabbing to x/Grabbing Something
[576]: exiT: Try Grabbing
[576]: Enter: Grabbing Something
[576]: Enter: Try Grabbing

The exit of Try Grabbing and the entrance of Grabbing Something occurs before the entrance of Try Grabbing. If you have multiple signals connected to state_entered you may end up triggering some of those connections after the state has already exited, possibly breaking some invariants.

There is also another minor problem this causes: the automatic transitions are evaluated multiple times, once due to the expression property update, and then again from the original On Grab transition. The second time it's evaluated the state is no longer active so you end up with a warning that the transition is ignored.

This isn't a blocking issue for me, as connecting do_grab_logic() to state_exited instead seems to work around all these issues, but the out-of-order entrance and exit can potentially cause some big problems.

[derkork]

The change with the automatic transitions seems to be the gift that keeps on giving... Yes of course this is the same problem that would occur if you run a send_event inside of an event handler. I'll check it out and devise a fix for this, thanks a lot for reporting this!

[derkork]

I think we'll have to put the whole chart on lockdown while a call to the state chart is being processed. So if you do a send_event then while this is fully processing all further calls to send_event get queued and the same thing needs to happen for the set property. otherwise you have really unclear semantics as half of the chart would work on the old properties and the other half on the new ones.

[derkork]

[0]: 🟢 Enter: X 
[0]: 🟢 Enter: Idle 
[221]: ⚡ Event received: grab 
[221]: ↪️ Transition: On Grab from X/Idle to X/TryGrabbing 
[221]: 🔴 Exit: Idle 
[221]: 🟢 Enter: TryGrabbing 
[221]: ↪️ Transition: To Grabbing Something from X/TryGrabbing to X/GrabbingSomething 
[221]: 🔴 Exit: TryGrabbing 
[221]: 🟢 Enter: GrabbingSomething 
[221]: ↪️ Transition: To Grabbing Nothing from X/GrabbingSomething to X/GrabbingNothing 
[221]: 🔴 Exit: GrabbingSomething 
[221]: 🟢 Enter: GrabbingNothing 
[456]: ⚡ Event received: release 
[456]: ↪️ Transition: On Release from X to X/Idle 
[456]: 🔴 Exit: GrabbingNothing 
[456]: 🟢 Enter: Idle 

Now that is at least consistent, but there is a catch. All internal state transitions will be processed before your code has a chance to set the grab_amount property. So with the setup you posted you will always end up in the Grabbing Nothing state assuming you initialize grab_amount with 0. I gotta think about this a bit...

[derkork]

Ok that has been rather easy, you just need to initialize grab_amount with -1 then no transition will be taken until you actually set it to a valid value. So what works looks like this:

On Release also sets grab_amount back to -1.

[derkork]

Fixed in 0.13.1

[MidZik]

I don't think delaying the setting of expression properties is a good idea. I'm fine with delaying their processing, but it's very counterintuitive and surprising that if I set grab_amount as part of the state_entered action in Try Grabbing, that its change isn't even visible in Grabbing Something, the state that comes after.

I'd recommend setting the expression property immediately, but queuing the processing for later: https://github.com/MidZik/godot-statecharts/commit/49126718bb4e1d76b12208e39a0d2906d51f4deb

This could perhaps be optimized some more, as having multiple PendingPropertyChanges queued up isn't necessary, and it could probably be reduced to a simple flag _pending_property_change that runs at the end of _run_change. See: https://github.com/MidZik/godot-statecharts/compare/f8ce0df...750dd5c

[derkork]

I'm a bit divided on this case. On one side I think if every change to the state chart is a transaction that happens in full before anything else happens makes this whole thing more predictable. You send an event or change a property and then defined stuff happens and you cannot start anything new on the state chart before this change is completely handled and we are now in a stable new state. This also implies that all guards are checked on the same base state for each event passed into the state chart.

On the other side I can see how the behaviour you describe is useful in your case. Then again this opens up a lot of potential edge cases. For example, say you have a property foo which you set to 11 at the beginning. Then you have a setup like this:

Now if you send a thing_happened event the question arises, whether the lower part of parallel state should now be in state C or D. If you allow property changes during event processing, then the lower part may be in state D (provided that the plugin guarantees any order for the transitions of parallel states being processed). But then again at the point in time when the thing_changed event was sent, foo actually was less than 20 (it was only changed later as a side effect of state_entered for the B state) so this transition should never have triggered and the lower part should remain in C.

I guess this is a situation in which whatever you do can be wrong.

[MidZik]

Yes, handling simultaneous things isn't defined in statecharts at all, so it is up to the implementations to decide what to do. At the end of the day the implementation must decide on an order, as it is easy to design a statechart that has no good solution, such as:

As far as the statechart is concerned, it's undefined if x should be 1 or 2 after signal. This issue exists even if we're not assigning to the same variable. If C has state_entered / do_c_action() and E has state_entered / do_e_action(), we can't do both actions simultaneously. One must happen before the other.

On the other hand, I think this statechart is well defined:

After signal b, the end state should be D.

And I think the same would be true for this one as well:

The code as it exists now will end up in E. But if you print(x) after that point, you'll get 1. I'd consider this incorrect behavior.

Actions should take place immediately by default. If a user desired the change to be visible later, they can easily use await or call_deferred to delay setting variables or sending events. If actions are always delayed, there is no way to make them immediate after the fact, and wonky workarounds would need to be used to get such behavior.

The only issue (which is an issue with statecharts themselves) is the parallel states and deciding which order to handle "simultaneous" events. We use ordered trees to define our statecharts, so the order of the nodes can be used to determine the order of event handling, just like it is with transitions.

A decision would still need to be made of course: do you check all transitions first and then execute them once you've decided? In your example, that would lead to:

Handling event "thing_happened"
   Checking for transitions
      Checking A
         Queued A->B transition
      Checking C
   Executing transitions
      A->B
         B state_entered
Final state: (B, C)

Or another possibility, check and execute transitions immediately?

Handling event "thing_happened"
   Running transitions
      Checking A
         A->B
            B state_entered
      Checking C
         C->D
Final state: (B, D)

And then there is the potential of automatic transitions. If B had automatic transitions, would they run and execute before checking C as well? Many decisions to make.

As you said, either can be wrong, but the other way of wording it is that either one can be correct. So you can choose whichever one provides the best value (however that is decided), document it properly, and it can be correct.

[derkork]

Matt, thank you very much for taking the time to write this up. It's really super helpful to discuss this and get some other perspective than my own to come up with a useful solution! I think you have made quite a compelling case here for setting the expression properties immediately - it gives the developer more choices while if the default is to always delay them, there is no more choice and the developer needs to resort to workarounds.

So then under this premise lets walk the examples to see if they work out nicely:

if we send signal we would end up in active states C and E and x == 2. This is because parallel states are processed in tree order and given that E would be lower down the tree their change would come last.

Then for:

if we send b we would enter B, this sets x := 1, then B auto-transitions to C which will trigger evaluation of C's automatic transitions and we end up in D.

Now for:

we send b, which enters B. Now B's state_entered callbacks run, which set x := 1. After that, B checks its automatic transitions because it was just entered which will trigger a transition to D. After this we will still do a check of all automatic transitions in the currently active states across the whole chart because setting x := 1 queued a change and that may have effects on other parts of the state charts.

And finally for:

we send thing_happened which will hit the parallel state. the upper section will transition to B which will set foo to 27. Now this branch is finished and we evaluate the lower section. This will now transition to D because foo is now 27. Afterwards like before we run another check of all automatic transitions in the currently active states because we changed foo and this may have an effect elsewhere.