Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

on stage:ready doesn't work. #1376

Closed
benetherington opened this issue Jan 14, 2016 · 15 comments
Closed

on stage:ready doesn't work. #1376

benetherington opened this issue Jan 14, 2016 · 15 comments
Assignees
Labels
bug Weird outcome is probably not what the mod programmer expected.
Milestone

Comments

@benetherington
Copy link

The on command is supposed to activate a trigger whenever a boolean changes state. stage:ready is a boolean, but for some reason on stage:ready always evaluates to true.

@benetherington
Copy link
Author

For any users with the same issue, the below is a workaround.

function on_stage { // flipflops back and forth when you stage
  when stage:ready then {
    when not stage:ready then { print "staged!". on_stage(). }
  }
}

@hvacengi hvacengi added the bug Weird outcome is probably not what the mod programmer expected. label Jan 14, 2016
@hvacengi
Copy link
Member

Looks like some kind of bug with evaluating the trigger condition. If I just use

on stage:ready { print stage:ready. preserve. }

it continually prints true (no instances showing "false").

The following will not print anything, even when staging:

on (stage:ready) { print stage:ready. preserve. }

This gives an error that the variable is not defined:

lock ready to stage:ready.
on ready { print ready. preserve. }
wait until false.

While this works as expected, including flip flopping when staging:

set ready to stage:ready.
on ready { print ready. preserve. }
until false { set ready to stage:ready. wait 0. }

Obviously the on trigger must be getting evaluated differently than the when trigger, since when works in the example from @benetherington but I've shown that on does not. We'll have to look at the compiler and the on trigger execution to figure out what is going on.

@TDW89
Copy link
Contributor

TDW89 commented Feb 4, 2016

Thought i would expand this issue instead of creating a new one since i think they are the same thing.
Neither on or toggle work with any boolean suffix that i have found they also don't appear to work with lists or lexicons.
eg.

set foo to list(false).
set bar to lex().
 bar:add("x",false).

on foo[0] { // this never gets triggered
 print "foo[0] is now " +foo[0].
}
set foo[0] to true.

on bar["x"] { //neither does this
 print "bar[x] is now " +bar["x"].
}
set bar["x"] to true.

print "foo[0] = " +foo[0].
print "bar[x] = " +bar["x"].

toggle foo[0].
toggle bar["x"].
// these print the same as the ones above despite the toggle
print "foo[0] = " +foo[0].
print "bar[x] = " +bar["x"].

lock steering to heading(90,90). //need to enable the steering manager for to test toggling the vectors
print "Steering manager enabled: "+ steeringmanager:enabled.
on ag1 {  
 toggle steeringmanager:showfacingvectors. //pressing ag1 goes not toggle the vectors
 preserve.
}.
on steeringmanager:showfacingvectors { //this is not triggered by the set below
 print "Toggling vectors".
preserve.
}.
set steeringmanager:showfacingvectors to true.

wait until false.

@hvacengi
Copy link
Member

hvacengi commented Feb 4, 2016

Can either of you force an exception in the code and post the log file? I'd like to see the opcodes that are executing when you're seeing the issue. It could be a problem with the compiler not properly handling suffixes in these two code paths.

@TDW89
Copy link
Contributor

TDW89 commented Feb 4, 2016

here you go.
http://pastebin.com/ndebDnVX

@hvacengi
Copy link
Member

hvacengi commented Feb 4, 2016

Looks like the stack trace got cut off in the middle, and the opcode list didn't get output. I've seen this happen where KSP doesn't finish flushing the file for a while unless you close the program. Can you try again, making sure you closed KSP? I suppose it may also be a size limitation of pastebin, but I think we've used it before for log files.

@TDW89
Copy link
Contributor

TDW89 commented Feb 4, 2016

yep here is the op code list.
EDIT: shortened the script to reduce size of list.
Code:

set foo to list(false).

on foo[0] {
 print "foo[0] is now " +foo[0].
}

toggle foo[0].
print 1/0.

Log:

[LOG 15:21:23.510] System.Exception: Tried to push Infinity into the stack.
  at kOS.Safe.Execution.Stack.ThrowIfInvalid (System.Object item) [0x00000] in <filename unknown>:0 
  at kOS.Safe.Execution.Stack.Push (System.Object item) [0x00000] in <filename unknown>:0 
  at kOS.Safe.Execution.CPU.PushStack (System.Object item) [0x00000] in <filename unknown>:0 
  at kOS.Safe.Compilation.BinaryOpcode.Execute (ICpu cpu) [0x00000] in <filename unknown>:0 
  at kOS.Safe.Execution.CPU.ExecuteInstruction (IProgramContext context) [0x00000] in <filename unknown>:0 
[LOG 15:21:23.514] Code Fragment
File                 Line:Col IP   label opcode operand
----                 ----:--- ---- -------------------------------  
                        0:0   0000  jump +27 
archive/zzz             3:1   0001 @0001 push $old-foo 
archive/zzz             3:1   0002 @0002 push $foo 
archive/zzz             3:1   0003 @0003 eq 
archive/zzz             3:1   0004 @0004 not 
archive/zzz             3:1   0005 @0005 br.false +21 
archive/zzz             3:1   0006 @0006 push $remove-on-2046218382 
archive/zzz             3:1   0007 @0007 push True 
archive/zzz             3:1   0008 @0008 storeglobal 
archive/zzz             3:11  0009 @0009 pushscope 1 0 
archive/zzz             4:2   0010 @0010 push _KOSArgMarker_ 
archive/zzz             4:8   0011 @0011 push foo[0] is now  
archive/zzz             4:26  0012 @0012 push $foo 
archive/zzz             4:30  0013 @0013 push 0 
archive/zzz             4:30  0014 @0014 getindex 
archive/zzz             4:25  0015 @0015 add 
archive/zzz             4:25  0016 @0016 call print() 
archive/zzz             4:25  0017 @0017 pop 
archive/zzz             5:1   0018 @0018 popscope 1 
archive/zzz             5:1   0019 @0019 push $old-foo 
archive/zzz             5:1   0020 @0020 push $foo 
archive/zzz             5:1   0021 @0021 store 
archive/zzz             5:1   0022 @0022 push $remove-on-2046218382 
archive/zzz             5:1   0023 @0023 br.false +3 
archive/zzz             5:1   0024 @0024 push 1 
archive/zzz             5:1   0025 @0025 removetrigger 
archive/zzz             5:1   0026 @0026 EOF 
archive/zzz             1:1   0027 @0027 argbottom 
archive/zzz             1:5   0028 @0028 push $foo 
archive/zzz             1:16  0029 @0029 push _KOSArgMarker_ 
archive/zzz             1:17  0030 @0030 push False 
archive/zzz             1:17  0031 @0031 call list() 
archive/zzz             1:17  0032 @0032 store 
archive/zzz             3:1   0033 @0033 push $old-foo 
archive/zzz             3:1   0034 @0034 push $foo 
archive/zzz             3:1   0035 @0035 store 
archive/zzz             3:1   0036 @0036 push 1 
archive/zzz             3:1   0037 @0037 addtrigger 
archive/zzz             7:8   0038 @0038 push $foo 
archive/zzz             7:12  0039 @0039 push 0 
archive/zzz             7:12  0040 @0040 getindex 
archive/zzz             7:8   0041 @0041 push $foo 
archive/zzz             7:12  0042 @0042 push 0 
archive/zzz             7:12  0043 @0043 getindex 
archive/zzz             7:12  0044 @0044 bool 
archive/zzz             7:12  0045 @0045 not 
archive/zzz             7:12  0046 @0046 store 
archive/zzz             8:1   0047 @0047 push _KOSArgMarker_ 
archive/zzz             8:7   0048 @0048 push 1 
archive/zzz             8:9   0049 @0049 push 0 
archive/zzz             8:8   0050 @0050 div <<--INSTRUCTION POINTER--
archive/zzz             8:8   0051 @0051 call print() 
archive/zzz             8:8   0052 @0052 pop 
                        0:0   0053  pop 
                        0:0   0054  EOP 

[LOG 15:21:23.514] kOS: Stack dump: stackPointer = 2
002 SP-> _KOSArgMarker_
001      _KOSArgMarker_
000      0

@TDW89
Copy link
Contributor

TDW89 commented Feb 4, 2016

not sure if it is related but when using a normal variable on triggers with any change not just true/false.

set x to false.
on x {
 print x.
 preserve.
}
set x to "hello world".
set x to list(1,2,3).
set x to 12.

This only works if you type it into the terminal, running it as a script doesn't cause it to trigger.

@hvacengi
Copy link
Member

hvacengi commented Feb 4, 2016

OK, I think I know what needs to be done to make this work. The issue is that on only looks for the variable identifier when writing the opcodes:

archive/zzz            30:1   0098 @0088 push $old-steeringmanager 
archive/zzz            30:1   0099 @0089 push $steeringmanager 
archive/zzz            30:1   0100 @0090 eq 
archive/zzz            30:1   0101 @0091 not 
archive/zzz            30:1   0102 @0092 br.false +20 

While when actually evaluates the expression:

interpreter history     1:6   0001 @0001 push $steeringmanager 
interpreter history     1:22  0002 @0002 push showfacingvectors 
interpreter history     1:22  0003 @0003 getmember 
interpreter history     1:22  0004 @0004 br.false +17

This also means that you can't work around it using locks, or functions, because both cases will throw an exception that the identifier is not a valid variable.

We will have to modify the compiler code so that it evaluates on values similarly to how when values are evaluated. There will be a small difference, because there will still need to be an "old" variable to compare to.

@TDW89 While your 2nd example doesn't work as written, it does work like this:

set x to false.
on x {
    print x.
    preserve.
}
wait 0.
set x to "hello world".
wait 0.
set x to list(1,2,3).
wait 0.
set x to 12.
wait 0.

This is because when and on triggers are evaluated only once at the beginning of each physics frame. This is different from how most lock objects work, where they are simply a delegate function which is evaluated every time it is called. (Steering and throttle locks are actually evaluated like when and on triggers, once per update).

It is interesting to note that on simply checks "does this object equal the original object". So as TDW's example points out, you don't have to "toggle" the value between true and false, you can also change the value from 0.00001 to 0.00002. That's not really the original intention, but it is an interesting ability. It would probably be more useful after we allow expressions to be evaluated, so that you can use things like round(time:seconds).

@hvacengi hvacengi self-assigned this Feb 4, 2016
@hvacengi hvacengi modified the milestone: On Deck new features Feb 9, 2016
@Dunbaratu
Copy link
Member

I hadn't noticed this issue when it was first opened. Just looking at it now.

Yes the problem is that the compiler, when it visits the ON statement, is hardcoded to assume the thing is just a simple identifier and nothing else. If it has more things in the expression, it ignores everything but the first identifier it finds - so an expression like 7*x + 4*y just gets the 'x' used and nothing else.

@alismatales
Copy link

Hi,

I'm the guy from this reddit thread: https://www.reddit.com/r/Kos/comments/499udi/what_am_i_doing_wrong_with_the_on_trigger/

I was still running 0.18.something at that point, have updated to 0.19.2 since, and now the problem has changed.

Script looks like this:

CLEARSCREEN.
SAS OFF.

LOCK Charge to SHIP:ELECTRICCHARGE. 
Print "Flight Status: Nominal.".

ON (SHIP:ELECTRICCHARGE < 50) {
        PRINT "POWER LOW. SHUTTING DOWN.    " AT (15,0).    
        LOCK STEERING TO UP.
        ship:partsdubbed("RTGigaDish1")[0]:getmodule("ModuleRTAntenna"):doaction("deactivate", 1).
        ship:partsdubbed("mumech.MJ2.ar202")[0]:getmodule("mechjebcore"):setfield("mechjeb", false).    
        PRESERVE.
}
ON (SHIP:ELECTRICCHARGE > 1830) {
        PRINT "POWER RESTORED. BOOTING UP.  " AT (15,0).    
        ship:partsdubbed("RTGigaDish1")[0]:getmodule("ModuleRTAntenna"):doaction("activate", 1).
        ship:partsdubbed("mumech.MJ2.ar202")[0]:getmodule("mechjebcore"):setfield("mechjeb", true).
        PRESERVE.
}


PRINT "Electric Charge:".

UNTIL 1>2 { 
    WAIT 0.001. 
    PRINT Charge AT (18, 1).
}   

But now with 0.19.2 i get the following error:

Internal Error.  Contact the kOS developers with the phrase 'impossible FromPrimitiveWithAssert(<null>) was attempted'

Log says:

[LOG 01:08:51.125] kOS: At boot_hibernate.ks on archive, line 14
ON (SHIP:ELECTRICCHARGE < 50) {
^

[LOG 01:08:51.125] kOS.Safe.Exceptions.KOSException: Internal Error.  Contact the kOS developers with the phrase 'impossible FromPrimitiveWithAssert(<null>) was attempted'.
Also include the output log if you can.
  at kOS.Safe.Encapsulation.Structure.FromPrimitiveWithAssert (System.Object value) [0x00000] in <filename unknown>:0 
  at kOS.Safe.Compilation.Opcode.PopStructureAssertEncapsulated (ICpu cpu, Boolean barewordOkay) [0x00000] in <filename unknown>:0 
  at kOS.Safe.Compilation.OpcodeStore.Execute (ICpu cpu) [0x00000] in <filename unknown>:0 
  at kOS.Safe.Execution.CPU.ExecuteInstruction (IProgramContext context) [0x00000] in <filename unknown>:0 
[LOG 01:08:51.140] Code Fragment
File                 Line:Col IP   label opcode operand
----                 ----:--- ---- -------------------------------  
archive/boot_hibernate.ks    3:1   0223 @0223 pop 
archive/boot_hibernate.ks    8:1   0224 @0224 push $charge* 
archive/boot_hibernate.ks    8:1   0225 @0225 pushdelegate 1 
archive/boot_hibernate.ks    8:1   0226 @0226 storeglobal 
archive/boot_hibernate.ks    9:1   0227 @0227 push $powerlow* 
archive/boot_hibernate.ks    9:1   0228 @0228 pushdelegate 10 
archive/boot_hibernate.ks    9:1   0229 @0229 storeglobal 
archive/boot_hibernate.ks   10:1   0230 @0230 push $powernominal* 
archive/boot_hibernate.ks   10:1   0231 @0231 pushdelegate 21 
archive/boot_hibernate.ks   10:1   0232 @0232 storeglobal 
archive/boot_hibernate.ks   12:1   0233 @0233 push _KOSArgMarker_ 
archive/boot_hibernate.ks   12:7   0234 @0234 push Flight Status: Nominal. 
archive/boot_hibernate.ks   12:7   0235 @0235 call print() 
archive/boot_hibernate.ks   12:7   0236 @0236 pop 
archive/boot_hibernate.ks   14:1   0237 @0237 push $old-steering 
archive/boot_hibernate.ks   14:1   0238 @0238 push $steering 
archive/boot_hibernate.ks   14:1   0239 @0239 store <<--INSTRUCTION POINTER--
archive/boot_hibernate.ks   14:1   0240 @0240 push 44 
archive/boot_hibernate.ks   14:1   0241 @0241 addtrigger 
archive/boot_hibernate.ks   22:1   0242 @0242 push $old-ship 
archive/boot_hibernate.ks   22:1   0243 @0243 push $ship 
archive/boot_hibernate.ks   22:1   0244 @0244 store 
archive/boot_hibernate.ks   22:1   0245 @0245 push 121 
archive/boot_hibernate.ks   22:1   0246 @0246 addtrigger 
archive/boot_hibernate.ks   30:1   0247 @0247 push _KOSArgMarker_ 
archive/boot_hibernate.ks   30:7   0248 @0248 push Electric Charge: 
archive/boot_hibernate.ks   30:7   0249 @0249 call print() 
archive/boot_hibernate.ks   30:7   0250 @0250 pop 
archive/boot_hibernate.ks   32:7   0251 @0251 push 1 
archive/boot_hibernate.ks   32:9   0252 @0252 push 2 
archive/boot_hibernate.ks   32:8   0253 @0253 gt 
archive/boot_hibernate.ks   32:8   0254 @0254 not 
archive/boot_hibernate.ks   32:8   0255 @0255 br.false +27 

[LOG 01:08:51.140] kOS: Stack dump: stackPointer = 2
002 SP-> $old-steering (type: System.String)
001      _KOSArgMarker_ (type: kOS.Safe.Execution.KOSArgMarkerType)
000      0 (type: System.Int32)

[LOG 01:08:51.140] kOS: Popping context 2
[LOG 01:08:51.140] kOS: Removed Context File                 Line:Col IP   label opcode operand
[LOG 01:08:51.140] kOS: Deleting 0 pointers and restoring 0 pointers
[LOG 01:08:51.140] kOS: New current context File                 Line:Col IP   label opcode operand
[LOG 01:08:51.156] kOS: Executing Opcode 0006/0009 @0006 pop
Prior to exeucting, stack looks like this:
Stack dump: stackPointer = -1

Executing Opcode 0007/0009 @0007 pop
Prior to exeucting, stack looks like this:
Stack dump: stackPointer = -1

Executing Opcode 0008/0009  EOF
Prior to exeucting, stack looks like this:
Stack dump: stackPointer = -1


[LOG 01:08:51.203] kOS: Executing Opcode 0008/0009  EOF
Prior to exeucting, stack looks like this:
Stack dump: stackPointer = -1


[LOG 01:08:51.250] kOS: Executing Opcode 0008/0009  EOF
Prior to exeucting, stack looks like this:
Stack dump: stackPointer = -1


[LOG 01:08:51.265] kOS: Executing Opcode 0008/0009  EOF
Prior to exeucting, stack looks like this:
Stack dump: stackPointer = -1

@Dunbaratu
Copy link
Member

I have an inkling what might be wrong, but can you try this quick workaround:
Make it so that the trigger body isn't the only place you're using LOCK STEERING. i.e. Try doing a LOCK STEERING TO UP. UNLOCK STEERING. in the setup steps up above before the first ON trigger. That would lock and unlock it right away so it should have no real effect, but I think it might help un-confuse the compiler to have the STEERING be mentioned somewhere else in the code too that is outside the trigger's body.

I suspect that because the steering isn't mentioned in the main body of code, the compiler isn't recognizing the need to build the right initial setup for it.

@alismatales
Copy link

Your suspicion that it has something to do with LOCK STEERING was right - the error disappears when i remove the it from the ON trigger.

Unfortunately locking & unlocking it before the ON trigger doesnt work.

@Dunbaratu
Copy link
Member

Hmm. This problem is... sort-of new and sort-of old.

old problem: Attempting to lock steering makes the steering function have a value of null for a bit before it activates for real.

new problem: Now that all values must be derived from Structure, we convert all primitives into things of our Structure type. The attempt to convert the "primitive" of <null> into a Structure fails because it can't read what the object's type is when it's null.

Possible solutions:
1 - Find out how to stop it from being null.
or
2 - Make FromPrimitive tolerant of nulls by making a dummy Null type derived from Structure that it can convert them into. Then at least it doesn't bomb out when trying the conversion.

I'm of a mixed mind about it. On the one hand it shouldn't be null. On the other hand being incapable of dealing properly with cases where a value does end up being null makes kOS more fragile than it should be.

@hvacengi
Copy link
Member

Can we just execute the trigger right before calling toggleflybywire()? The trigger body of lock steering basically boils down to on time:seconds { set steering to <body>. }, so we just call set steering to <body>. before enabling the steering itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Weird outcome is probably not what the mod programmer expected.
Projects
None yet
Development

No branches or pull requests

5 participants