Closed ghost closed 6 years ago
Can anybody help me understand what is needed to actually call a delegate from inside C# code (that gets synchronously called by iterpreter/CPU)? E.g. from Button.ScheduleCallbacks
and I mean synchronously, not scheduling trigger for later execution, execute it immediatelly. It probably needs some stack-procedure from CPU
, something like handling triggers, pushing return address (current program counter).... getting lost in this.
BTW, I took completely different approach when designing my bee (or my FScript in 2002). Why bothering with low-level code? My opcodes know high-level constructs like for
, if
, try..catch
. Why bothering with stack? My engine just uses the application/native stack which means that any function can be called any time, becase code is just byte array and I can swap the executing code and code pointer any time and restore it any time which makes things much much easier. Why linking it into single memory space when you can just swap the whole code-memory bank? Ahh, kOS is complicated under the hood 😆
I'm not sure what you're saying the actual issue is here. I tested your example script and it behaves like I'd expect (although I don't quite know why you're doing it this way instead of with a radio button group).
Can you explain what it's doing that you are saying looks wrong? When clicking one button, you explicitly set the other button to false, and the other button gets an event dispatched to it as a result since it's being effectively "clicked", but by the script instead of by a person. Is it that you're saying it's wrong for the other button to get its onclick
fired off?
I mean synchronously, not scheduling trigger for later execution, execute it immediatelly.
This is deliberately not the option we chose, specifically because we have zero control over what a user writes in their callback and we cannot prove it finishes in reasonable time. (The standard stopping problem - our software can't prove how long another piece of software will take before it finishes running, or even if it will finish instead of looping infinitely). All this work we are doing is occurring inside Unity's FixedUpdate()
, which is inside the main game engine's thread. If we take too long to return, we freeze the rest of the game until we do return. Rather than allow users, some of whom are entirely new to programming, the opportunity to break the game in an infinite loop that freezes all UI and they have to kill the game, we went with the scheduled approach where their callback cannot take more than IPU number of instructions per fixed update and if that means we have to wait several updates before we can get the answer back, then so be it. It's better to put the burden on us to deal with that complication than allow a user's script to freeze the entire game and require an external kill of the KSP process.
The very problem is that I would expect the output to be
1.TEST=True
2.TEST=False
3.OTHER=True
because events in .NET (e.g. WinForms) are called immediately (e.g. OnPropertyChanged
, OnListChanged
) when you change anything, not posted to global message queue to be executed later.
Why I see it as a problem? Imagine having lock steering
in onToggle(true)
and unlock all
in onToggle(false)
- you see the problem? The unlock
should clean the state before another button locks its own (if it even does it), but it gets called in different order thus breaking the lock
from new click by delayed cleanup.
I did try using CallPassingArgs
instead of AddTrigger
inside Button.ScheduleCallbacks
but it corrupted the stack. Cannot see why.
P.S.: There is another potential problem with click events - triggers are always top priority which creates huge problem if you try to run a script from onClick
- does not work because the script is inside a trigger. TakePress
-polling in main has to be used for such things. But this is a bit more complex, would require some low-priority triggers that would be executed in main (no priority, other triggers would interrupt that, no waiting for finish). And when I think about that, it would also be good to maybe configure minimal number of instructions executed in main (it is now 1) before triggers are re-enabled. And maybe even some critical section - imagine getting interrupted between positionAt
and velocityAt
...or some more complex function like stateAt
that would return a structure containing all the info (position, velocity, body:position, true anomaly.....).
I think I finally understand what you've been trying to describe. It was very unclear because you were talking about how it doesn't call the callback immediately on a click in the GUI. The OnGUI code doesn't run via FixedUpdate which is why it doesn't just immediately jump into the Cpu.ContinueExecution loop and instead lets it happen on the next FixedUpdate. I thought you wanted the OnGUI C# code to stop what it was doing, jump over into the CPU code to run some kerboscript callback instructions, then go back to the C# code to continue the rest of the OnGUI call. The reason that doesn't happen and is deferred is precisely because then we'd either allow the game to freeze with long callbacks, or have to have a more complex system tracking what IPU means since some instructions would be executing in-between FixedUpdates from other Unity hooks like OnGUI.
Now I suspect what you meant is NOT about when a click occurs, but when the kerboscript code simulates a click by changing the boolean value attached to a button, via script code,, you want that to fire off an onclick and jump into that kerboscript code before the kerboscript code that changed the value continues.
Is that what you've been trying to say? That this isn't about a desire to run the callback immediately from the C# OnGUI code as soon as it detects the user's finger pressed the button, but rather respond immediately when another part of the script changes the boolean value of the button.
That might be do-able. It would require a bit of infrastructure change and create the complication of triggering things differently when it's a script that causes the event than when it's the C# code that causes it. Right now it just triggers whenever the Getter/Setter property of the button's Boolean property detects a change, regardless whether that change occurred via a user's script instruction that was executing via Cpu.ContinueExecution()' loop or something else (like the C# OnGUI code that runs on a different schedule interleaved between FixedUpdates, while the CPU isn't moving instructions forward) Right now that property doesn't need to track the difference because it always schedules the callback for the next update no matter who caused it.
after changing state by kerboscript should have been clear enought. you want that to fire off an onclick and jump into that kerboscript code before the kerboscript code that changed the value continues. Is that what you've been trying to say? YES, now we understand each other
I was trying to create new UserDelegate.Trigger
accepting bool sync
calling TriggerNextUpdate
if sync=false
and CallPassingArgs
if sync=true
and call that from Button.ScheduleCallbacks
passing FindGUI().SyncCallbacks
which I added as new suffix for testing. It did not break existing code, but crashed after set gui:syncCallbacks to true
(and few clicks) and thrown weird errors at me about unrelated variables - that is why I suspect that stack gets corrupted (maybe the reference to the delegate is left there, or maybe something missing, don't know).
You seem to be using VisualPressed
and such from OnGUI
which do not call ScheduleCallbacks
but rather some Communicate
mechanism.
P.S.: I would personally remove DeltaInstructionPointer
and follow RISC's fetch-decode-execute pipeline, where InstructionPointer
gets first incremented as part of fetch/decode state and can later be modified by execute state (e.g. by jump). It would require ExecuteInstruction
be modified like this:
private bool ExecuteInstruction(ProgramContext context, bool doProfiling)
{
Opcode opcode = context.Program[context.InstructionPointer++];
(note the post-increment)
and change any Opcode modifiying its DeltaInstructionPointer
to change InstructionPointer
directly. (And any code that assumes that InstructionPointer
gets incremented afterwards - seen such code.)
Not sure if that is the root of the problem that I could not call CallPassingArgs
from SetMember
(through Button.ScheduleCallbacks
).
I think the change to assume all instructions move the pointer one step except for the ones that say otherwise is a good change even if it doesn't fix the problem. It reduces the information unnecessarily stored in the Opcode. The Opcodes that can't jump don't need to explicitly say "I go one instruction forward", they just lack the information about jumping at all, so the CPU does the default thing.
If all opcodes have a presumed "+1" that happens before they execute, though, we'd have to be careful to make sure we shift the relative jump offset down by one in all the opcodes that do jump to a relative location (most jumps are not "jump to this exact location" but rather "jump +5 instructions" or "jump -6 instructions" - those would all be off by one now and need an additional '-1' applied to them.
Where would you prefer that -1 offset happens? When creating the opcode's arg (so what was a jump -5
would now become a jump -6
when viewing the program dump)? Or leave the opcode data the same way it is now (still stores a '-5' as the argument), but add an additional -1 when executing it? The difference is whether or not the opcode argument means "next instruction is this far away from me" or "next instruction is this far away from where I would have otherwise ended up if I hadn't jumped".
That is up to you and I think the problem is elsewhere - corrupted stack, not instruction pointer. But I found that delta when searching for it and the mess in OpcodeCall.StaticExecute
which is really too much for me now, don't want to touch that until I am 100% sure I know what I am doing.
Ask yourself a different question: How should NOP (NOOP - No Operation) look like? Jump+1 or Jump+0? I would not change compilation / bytecode unless really necessary, but dump could be changed. My opinion.
P.S.: ARM (32bit) uses target-ip-8 because of three-state pipeline, so, after merging fetch+decode to one pipeline-state and set instruction size = 1, that would be target-ip-1 or in other words: address relative to pointer after the instruction (which makes sense because that is what execute will see and can just set ip+=offset).
Question: Why do you use string labels? I would use direct links to instructions (opcodes). I would remove the Distance
alltogether and replace it with Opcode Target
... and make parsing ML two-phase process just like compile + link - first will create all the opcodes (compile either from text or ML), second will fix all jumps (link). Well, first would probably create array of opcode wrappers with fields needed for the parsing/compilatin and then replace all of them with the wrapped opcode, but that is just memory optimization, nothing against having TargetIP
on Opcode
. Compilation from text probably won't even need linking phase if you use some kind of end of block
instruction (probably scope pop) which you create but not place at start of block compilation, give that to instructions that would like to jump to end of the block (if/else and such) and place it when you are done.
I would also replace Opcode.Id
by final address (what InstructionPointer
has to be if executing the instruction and that can later be used where labels currently are (dump and ML creation). That Id
would be -1
during parsing / before placing and rather after unloading (in case there is some bug and reference to unloaded opcode still exists).
P.S.: One more step: you don't even need InstructionPointer
, well, not a number, that too can be reference to Opcode
.
P.P.S.: Distance
and DistanceFrom
would be best. DistanceFrom = this
by default, later (linking) replaced by target instruction while zeroing Distance
. That should be easiest to use and won't break existing code (instructions next to each other have distance 1 not 0). This could even form a linked list and replace the array (DistanceFrom
becomes NextInstruction
while Distance
is compile-time only, always zero during execution).
I am currently working on tester application, that should be able to step individual instructions and show the stack, scopes etc. It should help me understand how kOS really works as I don't want to break it by touching something I do not fully understand. Will see, maybe you will like it too and use for testing and debugging.
P.S.: Thanks for fixing some of the warnings, I like the error and warnings window clean :)
P.P.S.: Do you plan support for 1.3.1? GetAudioClip(true)
should work for both 1.3.1 and 1.4(.1).
About 1.3.1 - It depends - is Realism Overhaul officially updated yet to 1.3.1? RO is my only real motivation for maintaining a backport.
I didn't think I would want to keep maintaining a backport after 1.4 came out because I was expecting a much bigger difference, but when 1.4 came out it turns out it wasn't that many lines of code that needed changing so it might still be reasonable to do it.
@Dunbaratu Just found your description of possible solution here: https://github.com/KSP-KOS/KOS/issues/2191#issuecomment-350520566
All I can add is, that if you trigger the event from script (not from GUI), then it should be called immediatelly, not later.
@firda-cze - in your opinion, what should happen if I make this change to fire callbacks immediately, and then someone has this kerboscript code.
local wnd is gui(0).
local bt1 is wnd:addButton("TEST").
local function bt1_toggle {
parameter val.
print "bt1 was toggled to " + val + ". Now I'll toggle it back.".
set bt1:pressed to (not val).
}
set bt1:ontoggle to bt1_toggle@.
wnd:show().
wait until false. // ctrl-C to end. Bad, I know - this is just a quick example.
When someone clicks the button, the user's ontoggle callback happens, which in turn toggles the button, which makes the user's ontoggle callback happen, which in turn toggles the button.... Infinite Recursion.
Right now that's delayed by not executing the trigger until next update, and having logic that refuses to insert the same trigger again when it's already waiting to be pushed onto the callstack.. But with this change, it would just recurse forever (because when the trigger is executing, it's no longer in the active trigger list - that's just a list of triggers waiting to be pushed onto the callstack, not a list of triggers currently being executed that are already on the callstack. So there's no information to check for "trying to insert a trigger that is running right now".)
(Unless I add some ugly checking that prevents adding the function you are currently inside of as a callback trigger.)
I'm inclined to just say that the infinite recursion is "correct behavior" according to how it's documented and thus it's the users fault for writing it this way.
I agree, infinite recursion, stack overflow.
You can always unsubscribe/unhook the event before assigning and subscribe/hook it again after, or use some variable (updating
) to quickly exit the handler.
That is exactly the thing you would do in UI (e.g. with OnTextChanged
where you want to check and maybe modify the text that was just entered).
Fixing this is going down a deep rabbit hole of infrastructure effects that I think require a bit of careful decision making because they have long-lasting implications. I've tried a few different ways to solve this and all of them have some problems (thus the "not ready" label on my PR).
@firda-cze I think I have a new idea that solves your concerns while at the same time still satisfying mine. Tell me what you think of this so I can go forward with the idea if you agree:
kOS has a kind of trigger that repeatedly gets re-called on the next update after it returns, forever and ever till stopped (this includes A WHEN/THEN trigger, ON trigger, or the math expression of a LOCK THROTTLE, any other similar "repeating trigger" callback (like VECDRAW:UPDATEVEC).
While it is perfectly fine for a one-shot callback like a GUI ONCLICK or ONTOGGLE to get interrupted partway through, it's not good if those repeating triggers mentioned above get interrupted like that. The reason is that there's logic in there to prevent them from repeating more often than they can execute - that logic is that if an instance of such a trigger is currently on the callstack, that trigger refuses to repeat until after the previous instance has had its OpcodeReturn happen. (the trigger must finish, and the rest of the leftover IPU must execute, before a new instance gets pushed on the callstack on the next FixedUpdate.) Thus if a user makes a very long recurring trigger - one so long that it lasts more than one IPU to reach the bottom, a new one doesn't get called in between FixedUpdates while the first one hasn't finished yet.
The problem with interrupting that kind of trigger with GUI callback code, is that unlike that kind of trigger, it's perfectly fine for GUI callback code to take an indefinitely long time to return. (i.e. press the launch button, and the ONCLICK jumps to a subroutine that opens a new window to control the launch, and doesn't return till the launch is over). If we allowed infinitely-repeating triggers to get interrupted by that kind of GUI callback trigger, then the infinitely-repeating trigger would be frozen (on the callstack underneath the GUI callback trigger) a very long time, never returning, and therefore never scheduling a new call to it. It would not be doing its job of running repeatedly.
I'd like the make it so the rule is this:
expr
in lock steering to expr.
or lock throttle to expr.
. It includes WHEN/THEN or ON triggers (even if the trigger returns prematurely choosing not to do the body yet, it's still calling the trigger again the next update in order to do the Boolean check at the top.) It includes the delegate you assign to VECDRAW:VECUPDATER
. It does not include GUI callbacks like ONTOGGLE or ONCLICK, which would be classified as "one shot immediate" triggers.When a "recurring trigger" starts executing, no other triggers are allowed to interrupt immediately. The request to interrupt is still remembered, but its deferred in a queue until the next FixedUpdate after the "recurring triggers" are done. Note, that all the recurring triggers have to finish before any new recurring triggers pending in the queue happen (note this is already the way it works now, and is the source of your original complaint is that it was doing this with with GUI callback code.)
But we make an exception for the "one shot immediate triggers" like ONCLICK and ONTOGGLE. When one of them starts executing, other triggers are allowed to interrupt it immediately on the next opcode even right in the middle of a FixedUpdate.
Trying to make everything from (1) work like (2) is making the entire system messy and broken when a user has a "recurring trigger" that lasts longer than 1 update.. But if I make this distinction, I can do it.
tl;dr - Recurring triggers need to be less aggressive than you're talking about (Not interrupt on the very next opcode.) Since they are made to schedule a new call as soon as the previous one returns, having them interrupt immediately on the next opcode would starve everything else of CPU time. And similarly, they need to be allowed to reach the end of their subroutine or else they never repeat like they're supposed to.
I would classify them as
LOCK
and such.set btn:pressed to...
) and that includes interrupting interrupt, because it is just like calling a function. Does not matter what state it is.
b) get scheduled after interrupts if the source is asynchronous (real click), but run in unpriviledged mode, that interrupts can interrupt it. BTW try running launch script from onClick
now, it does not work because it behaves like interrupt.a) get executed immediately if the source is synchronous (set btn:pressed to...) and that includes interrupting interrupt, because it is just like calling a function. Does not matter what state it is.
That explicitly continues to have the problem I mentioned. If an onclick
interrupts a When
and it worked like you mention, then the When
won't be over until the onclick
is over, and thus the When
won't repeat if the onclick
is designed to last a while. (The When
doesn't repeat until the previous one is done and the previous one isn't done until the onclick
is done.)
But that onClick
interrupts it if and only if you made it so by executing set btn:pressed
, not by clicking (that is the b) asynchronous. It is your responsibility (as a kerbo-programmer) to prevent it if you don't want it.
What is the real problem? Did you understand the synchronous vs. asynchronous?
But you seem to be using "asynchronous" and "synchronous" as synonyms for "done by real click" versus "done by code" and that creates a problem here:
WHEN altitude > 20000 then {
... stuff..
set btn:pressed to true. //synchronous or not?
... stuff..
}
The code is contained inside the context of a WHEN/THEN that itself was invoked asynchronously. I would call that "asynchronous" still because according to the callstack, it's still inside the asynchronous WHEN/THEN trigger. But it sounds like you'd consider this to be "synchronous", which means you'd still want a long onclick
callback to be able to freeze the WHEN/THEN.
YES, synchronous, no matter what the state.
SYNCHRONOUS = executed by the script (anywhere).
ASYNCHRONOUS = external event (real click).
The programmer of the script is responsible and should use the updating
flag (to exit the handler) or unsibscribe-set-resubscribe. You should not use wait
inside when
, right? Same logic. You should not have infinite loops or long-lasting functions. You should not change properties that trigger events if those events take long. You just have to be careful in interrupts.
main CPU loop:
If IPU reached:
LOCK
and such go to queue1.
onClick
from real clicks (asynchronous) go to queue2.
set btn:pressed
invokes the handler as function call, no queue, no special logic.
If you really want to improve kOS and help solving this interrupt/event dilema, then it would be nice to have new keyword to say e.g. when ... then later { ... }
meaning: use queue2 for this (execute the code as event, not as interrupt).
Otherwise your code should be rewritten as:
WHEN altitude > 20000 then {
set btn:onClick to nodelegate.
set btn:pressed to true.
set btn:onClick to original.
}
or
WHEN altitude > 20000 then {
set press_later to true.
}
while true {
if press_later { set btn:pressed to true. set press_later to false. }
}
Or built-in function runLater(delegate)
to push it to queue2
. That would solve it.
More reasoning: Imagine classic UI problem, when you have BackgroundWorker
(or Thread
) that wants to change state of UI control - you cannot do it directly, it throws exception! You have to use ISynchronizeInvoke
of the control and use control.Invoke(() => control.PerformClick())
(or BeginInvoke
) to schedule the code on main thread. It puts the delegate to the message queue, the UI thread pops it and executes it (Invoke
then waits on signal that it was finished to resume the background thread).
P.S.: ISynchronizeInvoke also has InvokeRequired
. Some similar global variable/built-in could also help, but I would just execute synchronously (as direct call) if you use runLater
outside of interrupt/event (could be usefull to schedule events from events to be executed after, not immediately).
ASYNCHRONOUS = external event (real click).
As are WHEN triggers like the one in the example I gave.
The clearer description for making the decision come out the way you describe would be to talk of "explicit" and "implicit" causes rather than "synchronous" and "asynchronous" ones:
That's because BOTH user clicks AND the lines of code that run in my example when altitude rises above 20000 are things that are perfectly accurate to describe as happening "asynchronously". What is different between them is that the altitude test is a user-defined test written in kerboscript while the test to fire off the user click effect is done in ways that are invisible to the script, in the underlying C# code. The big difference between them is whether the callback's cause is implicit or explicit. If it is explicit, you want the same effect regardless of whether that explicit code is happening asynchronously or not.
That is a much more sensible description because it clarifies what the dividing line is: If the script-writer caused the callback, then they're in control of the effect and can be blamed for cascading effects. If the script-writer did not cause the callback, then they can be screwed by something that's not their fault if there's cascading effects from that.
Looks like we came to agreement. I was talking about synchronous/asynchronous SOURCE of the event, script = synchronous source, click = asynchronous source (like a different thread, you cannot touch CPU directly just like you cannot touch UI from backround thread).
WHEN/LOCK run on (high priority) interrupt level, they can be viewed as asynchronous sources but I would not call the run level synchronous or asynchronous, it is just something completely different - kOS is not multi-core, everything runs synchronously with context-switching and execution level changes. WHEN/LOCK is not a source at all, it is like IF
that is executed on interrupt level every time the CPU decides it is time to run on interrupt level (update after at least one instruction on lower level was executed).
EVENTs that either came from asynchronous source or from runLater
run on event level (low priority interrupt level)
EVENTs that came from changing property by the script (synchronous source) are executed immediately on the same execution level or at least event level, which ever is higher (watch out! you don't want events triggered from main program be interrupted or even nested when you also click on the button while still executing the same handler).
MAIN program is the main/standard execution level (lowest).
That is terminology I would use, but I am happy that we understand each other, use whatever terminology you see fit.
P.S.: MAIN could also be seen as Idle Event, execute one instruction from main program whenever the event queue is empty. That should simplify the implementation logic.
What I implement will probably be functionally very similar to what you said above, but with different terms and a different looping structure to cause it.
The "Interrupt" vs "Event" terminology: They both "interrupt" your code so I don't think this is a good distinction.
The demand that the event handler is blocked from ever invoking any other events ever while inside an event callback moves kOS a bit away from being newbie friendly, as we have to start telling people to think in ways similar to threading. Instead of then being able to say "This is my launcher window that launches my other programs. When this button is clicked, run this program" they have to think "When this button is clicked, just set a flag and get out. Then that will make my main loop run this program when it notices the flag changed". I can see why it's a bad idea to have additional buttonA:onclick
invocations interrupt you while you're in the middle of one already, but I'd have liked to allow a way for, say buttonB:onclick
to interrupt you while you're in the middle of buttonA:onclick
. (make the exclusivity specific to THAT event callback, not all event callbacks)
But sadly, I can't really think of an easy way to deal with that without introducing ugly amounts of work for myself I really don't want to do. having to track every individual callback to say "this callback isn't allowed right now, but these other ones are" is a pain. I mean the infrastructure is actually there in how UserDelegates are invoked from C# code, for the C# code that scheduled the UserDelegate to be able to also detect when that UserDelegate is done and what its return value was (this is needed for those cases where the C# code wants to read the delegate's return value on a later update). It's there, but it means adding the checking to all GUI widgets. ("If the previous onclick delegate still hasn't finished, and a new one needs to be invoked, then queue it up for later when the onclick delegate is done."). I don't relish the idea of adding that to everything, so I may just have to live with that simplistic "all event handlers must end before a new event can happen." rule.
You are right, my event run level would break running programs containing GUI from click-handlers, although it solves the problems with LOCK (launch script without GUI could be run from onClick).
Two problems with current state:
takePress
now or similar logic).Solution 1: Execute events as function calls asap, but not as triggers - separate queue checked before executing any instruction. Problem: A lot of nesting, handler can even be interrupted by itself. Would also need two queues - one for events triggered from LOCK/WHEN that will be checked even at interrupt level, the second will not (on main level only).
Solution 2? Don't know right now, execution level won't work (run program from handler that also uses GUI). Delaying same events could cause more trouble - changing the property that should execute the the same handler should do what? Probably nest, but real click should not interrupt themselves, which contradicts the logic, but can be doable - immediate function call or separate queue that won't be checked for uniqueness.... but this is getting really ugly and I am really not sure now if it is good and flawless.
Well, what about executing handlers as part of main even when triggered by script in interrupt (LOCK/WHEN)? That sounds weird, because the handlers will be called immediatelly when you change property from main or from handler, but later if you do that from trigger (LOCK/WHEN).
...but it would avoid prolonging interrupts and would be easy to implement: deque (doubly-ended queue). GUI would add handlers to the end, script to the beginning. You can then check the deque before every instruction running at MAIN, but ignore the deque when running interrupt/trigger.
If you add stack of currently executed handlers (and you may need return values from some), then you can increase counter in the stack (of handlers and counters) if you find them there instead of pushing at the end of the queue (async source). You can then equeue it when you return from the handler (if the counter was not zero - either add counter to the queue or push as many times as the counter reads). That should prevent the problem with being interrupted by click while still executing the handler (either from previous click or from changing property) and would still not break running scripts with GUI from click handlers. Would also allow execution of clicks from different buttons (nested interrupt).
In any way, I need a break. Will look at your code and think about it for a while, this is not easy to solve.
Wouldn't executing events/callbacks immediatelly (after changing state by kerboscript) be more understandable and usefull (e.g. for custom radio groups spread over multiple boxes)?
Output:
Would require changing
TriggerNextUpdate
toCallPassingArgs
, probably. And btw, the code will crash until you accept #2238https://ksp-kos.github.io/KOS/structures/gui.html#gui-callback-technique will interrupt whatever else you are doing and call a function you wrote called myClickFunction whenever that button is clicked.