lock steering and lock throttle broke in triggers

[hvacengi]

It looks like something I pulled from upstream today broke using lock steering and lock throttle from inside a when ... then ... trigger. The version of kOS that I compiled yesterday morning still works fine. This morning I compiled from the latest "develop" source, and now my launch script stalls goes straight up and doesn't change orientation (I use triggers to change the lock based on altitude and orbital parameters). No errors are thrown, it just doesn't turn or adjust the throttle while the script continues to process. When it gets to the point where it executes a node to circulize, it can steer the ship and adjust throttle just fine (this section does not use triggers). The issue persisted through recompiling the code again.

I pulled the git source yesterday around 8:30AM (EDT) and today around 10:00AM (EDT), so the change happened some where between those times. I'm guessing it has something to do with the preparsing changes, but I can't be sure. I don't see any errors in the debug log surrounding the lock commands.

[abenkovskii]

Could you please:

1. post the program containing a problem.
2. force it to throw an exception (just add set x to 1/0. before the first line) and post your output_log.txt (not a KSP.log)
   P.S. for additional score points: reduce a problem to a minimum number of lines. This will save us a lot of time. Thanks.

[abenkovskii]

PR #772 by @Dunbaratu actually changed preparsing logic and might break something.

[hvacengi]

I dug further into the logs. It looks like there may be a deferred exception showing up in the preprocessing that is being attributed to the wrong line of code. You can find the log here:
https://dl.dropboxusercontent.com/u/19195825/KSP%20kOS%20lock.log
The applicable code shows up at line 37548. It looks like an index out of range in the pre processing.

[hvacengi]

You posted while I was uploading. You can find a copy of my scripts here:
https://dl.dropboxusercontent.com/u/19195825/AllScripts.zip
That's an old zip (I'll get it updated) that doesn't use the new user functions, but the launch script is nearly identical.

I had already seen the merged pull by @Dunbaratu which is why I suspected the preprocessing before I saw the error. It appears to be the only section of code changed in the last 30 hours that might affect the lock statement.

[Dunbaratu]

How long ago was the last time you used it when it worked?
I'd like to narrow down which changes might be the cause.

[hvacengi]

Literally it worked yesterday, and then it broke today. I was trying to narrow down a memory leak (good news, it isn't kOS) that I found last night while working on my docking script. I updated to the newest source today to make sure it wasn't the source of the memory leak, and that's when the steering stopped working. When I revert to the version based on the source that was pulled yesterday morning the script works again (which allowed me to continue troubleshooting the memory leak).

[Dunbaratu]

Well okay it's going to be hard for me to diagnose it at the moment as I'm already working on a heavily edited branch. I'll try to give it a go after that gets done.

Your zip has a zillion scripts in it - what are the steps to start the entire test, and do I need any specific rules for the craft I use?

[hvacengi]

Sorry, I'll pair it down. I forgot boot scripts don't work for you. The zip file has been updated with only the files required to reach a 100km holding orbit. Open the terminal, switch to the archive and then type run runholdingorbit. It will automatically switch back to the local disk and copy the needed files.

[Dunbaratu]

Okay sounds good. Looking at your log the problem is hard to track because the parser died before the program finished compiling. It means the program dump is useless - it will only show the opcode of the attempt to load the program.

[hvacengi]

I'm leaving work now to grab dinner. When I get home I'll see if I can dig a little deeper. I think I'm just going to go line by line through your modifications, but I know how much that can be like looking for a needle in a haystack.

I should point out that the script I uploaded is not the exact script that threw the error in the log. That's because the error was thrown while attempting to load my launch to docking script.

[hvacengi]

I can confirm that reverting both commits 030f58f and 5b0763f from PR #772 returned the function to my script. I'm going to see if I can figure out what's causing the error from those two.

[hvacengi]

It looks like the issue is in a result of changing the order in the PreProcess method, line 178 of compilier.cs used to be executed before what is now line 177. If I move 178 back to in front of 177, I can execute a lock inside of a when block, but it breaks using when blocks inside of user functions. In other words,
This works with functest12.ks, but not with a lock inside a when block:

172        private void PreProcess(ParseTree tree)
173        {
174            ParseNode rootNode = tree.Nodes[0];
175            TraverseScopeBranch(rootNode);
176            IterateUserFunctions(rootNode, IdentifyUserFunctions);
177            PreProcessStatements(rootNode);
178            IterateUserFunctions(rootNode, PreProcessUserFunctionStatement);
179        }

While this works with a lock in a when block, but not functest12.ks:

172        private void PreProcess(ParseTree tree)
173        {
174            ParseNode rootNode = tree.Nodes[0];
175            TraverseScopeBranch(rootNode);
176            IterateUserFunctions(rootNode, IdentifyUserFunctions);
178            IterateUserFunctions(rootNode, PreProcessUserFunctionStatement);
177            PreProcessStatements(rootNode);
179        }

In an attempt to make it easier to diagnose, I made myself an additional function test script. Unfortunetely the failure to set the lock doesn't seem to throw an exception, but I added a divide by zero to get the opcode output into the log as @abenkovskii suggested.
Script: https://dl.dropboxusercontent.com/u/19195825/functest14.ks
Log: https://dl.dropboxusercontent.com/u/19195825/KSP%20functest14.log
(Just scroll all the way to the bottom of the log to see the kOS output).
If you run the script and it executes properly, it should print 0 for 5s, and then print the current value of time:secionds for 5s.

[Dunbaratu]

Thanks for the detailed look at the problem. Unfortunately fixing it can't be as simple as putting the order back the way it was because it was flipped around specifically to make the test case functest12 work right. But at least I know where to start looking at the problem.

I'll add your functest14 to the test suite (but it will get called something else, since I'm already up to functest21 in my own branch by now).

[Dunbaratu]

I added this test (it is called functest22 in my branch), and it passes the test now. You can try your script again after the next update of the "canary".

[hvacengi]

I will pull the source when I get home tonight and test. Thanks for checking in to it!

[hvacengi]

Well, locks now work inside of a when statement. But for some reason my launch script still isn't working. I wonder if it's an issue with nested when statements. I'll mess around a little bit and see if I can come up with conditions to recreate the issue. Maybe I'll find an error in my script instead.

[hvacengi]

OK, I had to run a quick test this morning before posting because KSP crashed in my final round of tests last night and I couldn't wait for it to reload. The following script will work:

when alt:radar > 50 then {
    lock steering to (up + r(0,-45,0)).
    when alt:radar > 500 then {
        lock steering to ship:prograde:vector.
    }
}
lock steering to up.
lock throttle to 1.
stage.
wait 30.
unlock steering.
unlock throttle.

however removing the steering lock and throttle lock from outside of the when block does not work:

when alt:radar > 50 then {
    lock steering to (up + r(0,-45,0)).
    lock throttle to 1. //added
    when alt:radar > 500 then {
        lock steering to ship:prograde:vector.
    }
}
//lock steering to up.
//lock throttle to 1.
stage.
wait 30.
unlock steering.
unlock throttle.

I tried to come up with a test case similar to the previous one I shared, but it doesn't seem to produce the same results. This script works just fine:

set ut to time:seconds.
when time:seconds > ut + 5 then {
    lock t to time:seconds.
    when time:seconds > ut + 7 then {
        print t.
    }
}
wait 10.
print "done with test".

That makes me wonder if it is something specific to the throttle and steering variables. You can find a log that goes with the 2nd (middle) script above, with set x to 1/0. tacked on, at this link:
https://dl.dropboxusercontent.com/u/19195825/KSP%20-%20kos%20lock%20steering%20throttle.log

This issue will be easy enough to work around (I just need to initialize the locks outside of the when block). But I think that it could come up for a few people. It only comes up for me because I use SAS to keep the launched vessel pointing up until it has climbed 50m, delaying any vessel rotation until it clears the launch clamps.

[Dunbaratu]

The issue is that, I think, in order for the system to realize it's dealing with the magic special control variables, it needs them to be global in scope. But locks are supposed to be default-global so I'm not sure wha's going on. I'll use your test case today and see what happens.

[Dunbaratu]

Hmm. It looks like the function call for the lock is working, but when dealing with the two magic values for cooked steering, throttle and steering, they need an additional trigger inserted to run each fixedupdate that does this:

archive/functest23     -1:1   0103 push $throttle 
archive/functest23     -1:1   0104 push $<argstart> 
archive/functest23     -1:1   0105 call $throttle* 
archive/functest23     -1:1   0106 store 
archive/functest23     -1:1   0107 EOF 

(Calls the throttle function (the lock) and puts the result into the passive 'dumb' variable of the same name. Then flybywire can read the "dumb" variable to do its work.)

When that doesn't occur, the lock is present, but it's not updating the actual passive 'dumb' variable that flybywire is really reading. And for some reason that trigger is only getting inserted when the lock is mentioned at global scope.

Okay I hope this won't take long to track down.