Fixes #2239 user callbacks immediately#2277
Fixes #2239 user callbacks immediately#2277Dunbaratu wants to merge 5 commits intoKSP-KOS:developfrom
Conversation
The previous value for an ON (to detect the it has changed) is still suing a global value determined at runtime so it can't store more than one of them, which means multiple ON's from the same line of code don't work right. WHENs are working though.
On triggers' previous values are now tracked separately merely by taking advantage of the local scoping that was made available several months ago for triggers. We just weren't using it because we still had the old code that tried using the Global instead of Local storage Opcodes in the trigger's boilerplate code blocks that tracked the previous value.
When Adding a trigger, the caller can now choose if that trigger should wait for the next KOSFixedUpdate, or if it should execute immediately. If Cpu.AddTrigger() is called with arg Immediate=False: The system works like before - the trigger will not get pushed onto the callstack until the start of the next KOSFixedUpdate. If there are instructions remaining this Update, then the rest of the IPU's worth of instructions will happen first, before this trigger will fire off. If Cpu.AddTrigger() is called with arg Immediate=True: The trigger's call will get pushed onto the callstack just before the next Opcode would have been executed in the ContinueExecution() main loop, even if that's in the middle of a KOSFixedUpdate. If we are not in the middle of a KOSFixedUpdate right now, then the effect ends up being the same as if Immediate=False, since the time the "next opcode would have been executed" is during the next KOSFixedUpdate anyway. From the point of view of C# code that's waiting for a return value from the user's callback, there's not really any difference between these two since it has to wait for the user's code to happen anyway before it can get the answer. Immediate=True should never be used in cases where a trigger gets re-inserted automatically on a timer, as that can make it recurse forever and never give time back to the mainline code.
|
Added "Not Ready" because I just realized a problem on thinking about this code and I need to fix that problem first. It should be pretty easy to fix but I ran out of time tonight to address it yet. The way it was before: There has been (for a while now) a sanity check in the system that would delay re-inserting the pending triggers until at least one opcode of mainline program got executed. That way recurring triggers like WHEN and ON wouldn't starve mainline code of all IPU if they last longer than one update - they won't re-insert themselves until at least some mainline code is reached again. What has just changed in this PR: Now it's possible for callback triggers to start executing immediately within the main code, and it's possible (and should be totally legal) for a user to design things like GUI callback code where the majority of the program's time is spent inside GUI callback hooks rather than in the mainline flow. But the system described above won't distinguish this kind of "trigger" from the recurring kind of automatic timer "trigger". If you were in a GUI callback hook, and got interrupted by a WHEN trigger that wanted to be preserved, that WHEN trigger won't re-insert itself as long as you're still in the GUI callback hook. The system doesn't realize the GUI callback hook should totally count as the user's own normal code and should allow WHEN and ON to re-trigger. (In other words when it sees a WHEN check that wants to interrupt a GUI ONCLICK, it behaves the same as if it was a WHEN check being interrupted by another WHEN check and tells the interrupting WHEN check to hold off until the other triggers are done.). For the sake of that check, the rule should be changed: Old rule: "Recurring triggers (WHEN/ON) must be held off in the pending trigger list until other recurring triggers (WHEN/ON) are done, but once mainline code happens then they can be moved to the real trigger list. New rule: "Recurring triggers (WHEN/ON) must be held off in the pending trigger list until other recurring triggers (WHEN/ON) are done, but once mainline code or any non-recurring user callback code happens then they can be moved to the real trigger list. |
|
Looking at the code after processing our conversation in #2239, this code now enables immediate triggers, but they are actually not executed immediately if the source happens to be inside trigger (which now also includes click-handlers), it only works for main program, right? It does not solve the code in the first post. The output won't change a bit, because What is needed is to really distinguish recurring triggers - WHEN/ON/LOCK vs. non-recurring user callbacks ..... the naming should simply become triggers vs. callbacks (as my interrupts vs. events). The result should be what I have described as weird here: #2239 (comment)
Possible problems:
|
|
I could not stop thinking about this. If callbacks remain triggers and triggers cannot interrupt other triggers, then what is the point of immediate callbacks that are not immediate inside callbacks? That was specificaly my complaint, that I don't think the current state of this PR changes the output (did not try it, but looked at the code). But there is second problem we seem to be trying to solve as well: I have proposed three run levels (see You do not excpect callbacks (click-handlers) to be interrupted by other callback, unless you perform action that specifically allows this - either calling I am trying to warn you, that lowering priority of callbacks to the level of main program will introduce its own problems! Imagine the callback being interrupted by itself by two fast clicks. You could somehow fix this, but then use two buttons that you click on both in short time... complete mess and total confusion. Modern C# introduced Lastly, every GUI could have its own queue of callbacks and pass them one-by-one to CPU (waiting for it to be finished before passing another). That would, btw, solve the only problem my proposal with run levels seem to have, except the complexity that may be hard to understand, but I hope that the behaviour is so intuitive that most of kerboscript programmers would not need to now how it works. It would require marking the callbacks with a group. Callbacks cannot interrupt other callbacks from same group (same GUI), maybe except for group 0 (special marker), these can interrupt other callbacks and can be interrupted by any callback. That would either have to be managed by the GUI (never passing more than one callback of same group), or by CPU - having table of pending callbacks from same group (Dictionary<Group,List<Callback>>). Same technique can be used if you decide to run callbacks at run level of main program (but I really don't like such solution because of the problems it can cause). P.S.: The question is if e.g. |
|
OK, I will assume that WHEN/ON, LOCK THROTTLE/STEERING and VECUPDATER will use That sounds like strictly immiediate callbacks, although you can still possibly choose not to execute them on Also solving triggers vs. callbacks and running scripts from click-handlers (because callbacks run on What would be the result?! btn2 is pressed It would require something like |
Running it, after I rewound the most recent commit (so I was being fair and using the PR in the state it was in when you wrote this), did this: The old output that you said was wrong was in this order when I did the same thing: It definitely changed the output and I remember knowing this because I actually did test your specific example before I made this PR in the first place. I tested this again just last night after reverting the most recent commit, to be sure I wasn't remembering it wrong. You say:
User function calls and immediate triggers are already doing the same thing as each other in regards to this. In a sense UserDelegate.TriggerOnNextOpcode() is just a proxy for what OpcodeCall().Execute() already does, but in a way designed to work regardless of what part of the C# code is doing it and which part of the Unity updating system is doing it (i.e, the FixedUpdate pass, the Update pass, or the GUI pass). It ensures the UserDelegate call will be put onto the stack and get jumped to at the very next point in time that it is at all possible. If an Opcode is currently Executing (If |
|
It boils down to whether you take the trigger out of the queue unconditionally (as you seem to do and my assumption was wrong) or conditionally (only if in Cpu.Section.Main - which was my wrong assumption because you did not like immediate callbacks inside trigger, or at least I got that impression). Ok, immediate callbacks are solved, they are strictly immediate no matter what. Good. What about the hazards created by making callbacks execute as Cpu.Section.Main? |
|
I'm beginning to think the case of having a button launch a program isn't worth trying to support. It doesn't work now in the public release, so not supporting it doesn't actually break a previously working thing. The problem isn't the cases you've brought up - they are cases I can handle with some slight tweaking to the exclusivity checks that already exist to prevent multiple copies of the same trigger. The problem is the case where somebody decides to have the same callback attached to more than one hook. ("When widgetA changes, call foo(), but also call foo() when widgetB or widgetC change.") In that case there's no good way to identify what the exclusivity of calling foo() should be, without reading the user's mind and seeing how they thought it should work. There's two equally valid interpretations, one that says "same callback, means don't let it interrupt itself", and another that says "triggered by three different causes, only make it exclusive if it was the same cause". Depending on how they wrote the code in the callback, that might work.... but we can't prove one way or the other how they wrote it. It's also a problem when one hook has more than one callback (we have a few of these I was experimentally working on, where the hook is actually a list of callbacks so you can set up more than one notifyee of the same event if you like.) Again, the definition of what counts as exclusivity here requires reading the user's mind about their intentions. While I really don't like the simplistic rule that no callback is allowed to interrupt another callback, it's the simplest way to deal with it that makes it possible to get this thing out the door in a reasonable time and I have other parts of the mod I want to move on to. I'm not sure if I like the idea of doing it with runlevels, as it requires a bit of restructuring, but there's plenty of ways to get the same basic algorithm to happen. |
At this point they're so intertwined that they're all touching the same code and it might be easier to just handle them as a single PR. Whatever is done for #2280 has to occur "on top of" what this PR does anyway, and may even slightly undo some of what this PR did and rebuild it a different way (thus invalidating any time spent reviewing this). |
|
It is totally up to you. Solving #2280 properly won't be easy task and I agree that it may not be worth the effort. I will test the version you mark as ready (or say it is ready for testing). I can, maybe, try to implement my ideas if you leave #2280 unsolved and move to other things. I will focus on the tester (scopes) otherwise, if I even find the time. You know, temperatures are rising, my house and garden are screaming for attention XD. |
|
The more I think about it, the more I lean towards changing I have no idea how to actually implement it, but try to think about it for a while, how do you like it? It could even be used for cooperative threads: and of course |
|
The problem is that while I mentioned the case of doing I implored the team a while ago to enact this rule: The code changes and the user doc changes have to be wrapped up together in the same PR so merging them is an atomic operation where you aren't allowed to make the docs temporarily wrong because of a new change that isn't documented for the users yet. This rule was accepted. This rule had a second happy side effect that was not part of my original plan but I've noticed it being useful as an accidental benefit - programmers start thinking to themselves, "How the heck am I going to actually explain this to people?" up front before they finish a PR. That ends up discouraging ideas that are going to be confusing to the userbase. And I think that making What might be simpler to explain is a simple command that just says "allow interruptions now" that you can execute from inside your callback if you like, similar to some of the things you've been talking about. |
|
There are multiple ways of tackling this, but I came to conclusion, that making
Triggers run at the beginning of each update, interrupting main program either at |
|
The reason for all the conflicts is because this problem was already fixed a different way in PR #2296, which touched much of the same code. |
EDIT: Please look at PR #2296 and merge it instead of this. This PR has had extra work put on top of it, and that extra work has become PR #2296. I believe #2296 does a much better job of handling this issue. (Plus, the commits from this branch are also in the branch for #2296 so you should get this PR merged as a side-effect of merging #2296).
Fixes #2239 with the ability to make immediate user callbacks.
Now any callback created via UserDelegate.TriggerNextUpdate() is going to actually trigger next opcode instead (the method name has been edited to reflect this).
Please merge #2275 first - this branch was made on top of that branch, because they edited some of the same lines of code and would have had a merge conflict if I tried to implement them separately.
(Some of the diff's you see will go away if you merge 2275 first.)