Unlocking steering doesn't work from gui button onclick callback.

[poisonbl]

As identified by Warrigal on discord, unlocking steering from a gui button callback appears to unlock for a brief instant (the debug console announces kOS: FlightControlManager: ToggleFlyByWire: steering False appropriately and the pitch/roll/yaw inputs appear to stutter towards 0), then it goes back to locked (without an announce on the debug console). From an on trigger, inline in the code, etc. unlock steering. works as expected. Tested against kOS 1.1.3.2 on kSP 1.3.1 (just kOS and module manager).

Any craft with a CPU appears to be sufficient to reproduce it.

clearscreen.
print "Demonstrating issues unlocking steering from a gui button:onclick.".
print "".
print "Hotkeys:".
print "AG8:    Steer On.".
print "AG9:    Steer Off.".
print "LIGHTS: Exit.".

clearguis().
lights off.

// Deligates
set f1 to {lock steering to heading(90,45).}.
set f2 to {unlock steering.}.

// Action groups.
on AG8 {f1(). preserve.}
on AG9 {f2(). preserve.} // Trigger. Works.

set w to GUI(200,200).

w:addlabel("Steering:").

set ba to w:addbutton("Steer On").
set ba:onclick to f1. // Works as expected.

set bb to w:addbutton("onclick={f2().} - broken").
set bb:onclick to {f2().}. // Indirect deligate call from button. Broken.

set bc to w:addbutton("onclick=f2 - broken").
set bc:onclick to f2. // Direct deligate call from button. Broken.

set bd to w:addbutton("onclick={unlock steering.} - broken").
set bd:onclick to {unlock steering.}. // Direct callback. Broken.

set be to w:addbutton("onclick={toggle AG9.} - works").
set be:onclick to {toggle AG9.}. // Indirectly trigger the action group. Works.

set bf to w:addbutton("Exit (lights on).").
set bf:onclick to {lights on.}.

w:show().

wait until lights.
print "Finished. Unlocking steering.".
unlock steering. // Inline. Works.
lights off.
w:dispose().

[Dunbaratu]

Callbacks happen by inserting a call into the trigger list. The root problem might be that unlocking steering doesn't work if done during the step where the CPU executes the triggers, because the lock steering expression that re-runs every tick also happens when the CPU executes triggers, before mainline code. It might be a problem where the lock steering expression is un-doing the work the unlock command did, when they happen back-to-back in the same update tick during the triggers part of it.

[hvacengi]

I went through and did some debugging this weekend when it was first reported on discord. I think that you're right @Dunbaratu, because stepping though the operation it looked like the call to removetrigger showed the trigger lists (both the ProgramContext.Triggers and triggers to TriggersToInsert) as empty. I haven't finished walking through the code, but my assumption was that TriggersToInsert should be where you find triggers that are waiting to be added back in.

As to why this wouldn't have been an issue in previous releases: I just modified the logic for steering and throttle controls to enable based on being set for the most recent release. Which means that in previous versions it was possible to have this same race condition, but that the steering manager only paid attention to toggleflybywire instead of a value being set. I made that change to eliminate the hard requirement that only one processor control steering or throttle at a given time.

[Dunbaratu]

Yes, I think the trigger order is the problem. I just read through the code by eye.

The normal order it does things is this:

(1) (This happens in Cpu.ProcessTriggers()) First, go through the list of triggers and insert into the stack a bunch of fake nested function calls that will call each trigger thing. Don't actually execute any of them yet - just set up those nested function calls that will make the normal execution call them later.

(2) It removes all triggers that got calls inserted in step 1 above from the trigger list. By default triggers remove themselves after executing once.

(3) While executing the trigger body (later during Cpu.ContinueExecution(), if that trigger body returns true, that means the trigger is trying to preserve itself, so the system takes that "return true" to mean it should re-insert that trigger into the trigger list, effectively un-doing what happened in step (2) above.

So what happens in this case (I think) is that this happened:

In step 1 both the trigger expression for lock steering to expr., and the callback trigger containing the UNLOCK STEERING. get the 'function call' inserted into the stack for them.

But the triggers are inserted in such a way that the UNLOCK STEERING callback will happens first, before the trigger for lock steering.

And the trigger for lock steering ends up re-inserting itself (in step (3) above) after the callback for unlock steering removed it. In a sense there isn't any trigger for Unlock steering to remove during the brief time window it's being run, because during the execution of the callbacks, it's normal for the trigger to be missing, as per step (2) above.

In general we need a new way to track the removal of triggers that so it works when done during execution of other triggers.

EDIT: Perhaps we could have a "deferred trigger removal" list in Cpu that gets populated by any OpcodeRemoveTrigger calls that happen during the physics update, then at the end of the physics update it goes through that list and removes the things in it. That way if you have a fight between "this trigger said to preserve itself" and "that trigger said to remove this trigger", the remover wins, after the trigger it removed still had one more go this tick. Thus the rule would be "when removing a trigger from inside a trigger, the removal happens in between this physics tick and the next one. During this physics tick the trigger might still execute one last time, and you can't predict whether that will happen or not because it's a race condition between the two interrupts."

[hvacengi]

I feel like (and will need to verify that) the only time we actually call removetrigger is for steering events. If that's true, one of my desired future modifications would fix this issue: I want to move the trigger management into the manager classes instead of compiling in the add trigger call. The idea would be that the steering manager (for example) would recognize that the $steering* pointer is referencing something other than the default function, and then would queue that function much like how vector renderers use the vecdelegate suffix to automatically update the vec suffix.

But that's neither here nor there, because I won't have the above fix ready for the next release, and possibly not the release after. As such I'm in favor of possibly modifying the remove trigger logic go queue a list of triggers to remove, and then actually remove them during ProcessTriggers. The problem with this of course is the potential race condition where a trigger is both removed and legitimately added in the same tick. So maybe we instead need a list of removed triggers and that list is checked when analyzing the true return, but not when the opcode addtrigger is evaluated. The list of removed triggers would then be cleared any time the program reaches mainline code.

[Dunbaratu]

I would like to see language changes that let us turn off triggers from outside the trigger (a means to name a trigger and then a command to cancel a trigger by name later), so in the future there may be uses for RemoveTrigger other than steering.

The problem is that we make no guarantee about what order triggers happen in. It truly is a race condition in real life in the kerboscript language. if someone does this:

set future_time to time:seconds + `10.
when time:seconds > future_time then {
  lock steering to up.
}
when time:seconds > future_time then {
  unlock steering.
}

Then the language itself makes no claims whatsoever about which of those two triggers happens first. I have no problem with coming up with a definition that says in all such cases the "unlock" will win because unlocks happen in-between ticks not during them.

[hvacengi]

But what about this code:

set future_time to time:seconds + 10.
when time:seconds > future_time then {
  lock steering to up.
}
when time:seconds > future_time then {
  unlock steering.
  print "hi".
  lock steering to up.
}

Now, that should actually work today, because the two steering delegates are not actually the same due to file name and location encoding. But if you called that trigger multiple times within a program session it would have potential for a pure race condition within the trigger itself. Similar to:

set future_time to time:seconds + 10.
lock steering to up.
when time:seconds > future_time then {
  unlock steering.
  print "hi".
  set future_time to time:seconds + 10.
  lock steering to up.
  return true.
}

[Dunbaratu]

Okay yeah that is a problem.

The problem is that because it's normal for a preserving trigger to be removed at the start of the update tick and not get re-inserted until the trigger executes, there's nothing really for RemoveTrigger to operate on during the time window between when execution starts for the tick, and when that trigger gets executed.

In the case of the SAME trigger body doing both an unlock steering and a lock steering back to back I think we can be okay because the unlock steering will schedule a trigger removal, but then the lock steering can un-schedule it again during the body. We can make part of the logic of the OpcodeAddTrigger be that when you OpcodeAddTrigger, it checks to see if the trigger has been put in the removed list and un-remove it. OpcodeAddTrigger (that should happen when the code does lock steering to whatever is a bit different from the adding of the trigger that happens automatically upon returning true from the trigger. Since they follow a different path, they can be treated differently.

If re-inserting a trigger due to returning true from a trigger, that doesn't remove it from the removal list. If re-inserting a trigger because OpcodeAddTrigger happened, then that does remove it from the removal list because this is an explicit insertion of a new trigger, not an automatic "keep alive" of a previous one.

[Dunbaratu]

i.e. we just add the logic to OpcodeAddTrigger to say "If this trigger is in the scheduled-for-removal list, then as well as adding it to the pending trigger list (which it already does), also remove it from the scheduled-for-removal list.

OpcodeAddTrigger happens when you explicitly LOCK STEERING TO something, whereas it does not when the trigger merely returns true to preserve itself.

Thus the rule would be "if in the same tick you both start a trigger and remove it in two different trigger bodies, it's a race condition and you can't tell which will win. But if in the same tick you merely preserve a trigger and remove it in two different trigger bodies, then the removal always wins.

Removal beats mere preservation of an existing trigger, but doesn't necessarily beat explicit creation of a trigger.