KSP-KOS / KOS

Fully programmable autopilot mod for KSP. Originally By Nivekk
Other
697 stars 230 forks source link

Can we implement locking of SetSuffixes? #1517

Open Dunbaratu opened 8 years ago

Dunbaratu commented 8 years ago

You can do this:

set thing:suffix to (expression).

But you can't do this:

lock thing:suffix to (expression).

Lock only works with kos varibles and not with suffixes. Thus we can't really use the same kinds of "lock" logic on things like ship:control:pitch as can be used on lock steering to ____.

While this intuitively looks like something you should be able to do in kerboscript, in implementation it's a bit messy because you'd have to be able to use a function or KOSDelegate as the value to all SetSuffixes. So a SetSuffix currently defined to only accept, say, a Scalar, would now also have to be able to accept a KOSDelegate or function pointer that returns a Scalar. And then do we want to do it for ALL suffixes? Will there be some for which it makes no sense? It may be a lot messier than it seems on the surface to implement this universally for all suffixes.

hvacengi commented 8 years ago

I would expect that this would be implemented much the same as how lock steering is done. The compiler adds a trigger which is executed at the start of the update frame and stores the value in the steering bound variable. That eliminates the need to accept a delegate in the suffix itself, but more importantly prevents structures from having to be "update" aware themselves. If this was implemented on the structure itself, it would require that the structure tie into the UpdateHandler and might prevent garbage collection even after the KOS handle to the object was lost.

Dunbaratu commented 8 years ago

The difficulty is that I'd like to support the idea of it working the same as normal locks, in that you don't pay the expense of running the expression every tick unless it's a control value that kOS itself has to query. Normal locks are not updated every tick. Only throttle, steering, wheelthrottle, and wheelsteering are, and that's only because the steering system needs to query them every tick. Normally you only run the expression when someone is trying to look at the lock.

That's why this hadn't been done in the past - it wasn't possible to make it universal and the same as variable locking. if I say lock x to y/2. then the expression y/2 does not happen every tick - only when I try using x in some other expression. But if I lock mycolor:red to 1.0/(ship:liquidfuel/1000+0.001). to make a color redder the less fuel there is, the only way I can get that "only re-evaluate color:red when asked for its value" behavior is for it to somehow know that when it sees mycolor:red in an expression that the value of it is set to a user expression. And that would mean that every SetSuffix that takes type foo would have to also take type delegate returning a foo as an alternative, and geeze that's getting complex.

To a certain extent I wish the language didn't have locks and had had proper functions from the start instead. It makes the system easier because then it's up to the caller to specify "this thing I'm getting the value of isn't a normal variable -this is code I'm trying to call". Then steering, throttle, etc would be callbacks instead: function throttle { return 1. }.

sigh. This may just be impossible from the start. I mean, if we only support it for some but not all suffixes then we don't even need to use lock at all - we just make some suffixes that accept KOSDelegates and have the logic in themselves to notice when they've been set to a KOSDelegate and behave accordingly.

hvacengi commented 8 years ago

How does a KOSDelegate handle being invoked outside of the normal cpu execution sequence? If I have code setup like this:

lock testLock to alt:radar * 1000.
function getTestLock {
    return testLock.
}
set del to getTestLock@.
until false {
    print del:call().
    wait 0.
}

Does the lock object in that instance get fully evaluated each call to del:call()? If the underlying kerboscript is evaluated each time, without the need for cpu instructions, then we could simply make the Suffixes types themselves aware of the delegate, and then make it so that any time a variable is accessed, the suffixes evaluate any delegates saved.

Another thing that will produce an issue is that the underlying C# almost always returns new instances of variables, unless you take the time to store it in a user variable. For instance, the ship bound variable returns a new instance of VesselTarget every time, so storing the lock information on an instance of ship will give unexpected results, since you never will have a handle to that same object again. (Side note: we might want to cache that value somehow since it's a frequently accessed variable and calling constructors repeatedly is expensive).

hvacengi commented 8 years ago

I think there's also a small conflict in the concept of "evaluate when I ask for the value". For instance, the value of red in your color example above may be queried by the user, but it is used by the system when part of a vecdraw. In the underlying C# we don't ask for the suffix, but access the property instead. This means we'd need some magic to make the property truly update for user and for system objects.

Dunbaratu commented 8 years ago

And that's why I'm unsure about wanting to do this. It creates a place where the documentation differs depending on type and I don't like that. "LOCK has a totally different semantic meaning when used on these things than when used on those things..." To do it I'd rather change it so lock works that way for everything so it's consistent, and then.... we'd be breaking with how we've been (how, I have been) constantly telling our users it works. Suddenly it becomes part of the expense of IPU per update when it wasn't before.

AlchemistCH commented 8 years ago

And yet there's difference between turning a variable into a function to be called on every query and making to autoupdate each frame (or first time it's used in a frame) and return this value to all queries this frame. The second option is more in line for anything that has to do anything with the vessel itself: steering & throttle, raw control, tweaking part parameters... Same goes for functions based on data about vessel state - locking them would make sense exactly for them to autoupdate each frame and be used for calculations in multiple places. And GUI. Maybe it's worth to make all locks in kOS like this - once per frame update. Maybe update on first demand per frame for anything fully internal to the program, and at the start of every frame for anything that has external output (like throttle and steering already are)

Dunbaratu commented 8 years ago

But there isn't currently a difference between lock steering and lock homemade_variable, if explained thusly, which is how I've been doing it: "Locks only get calculated when someone queries the value. In the case of these 4 special variables, the system itself will want to query them every physics tick, so they'll happen all the time, but that's because they are being queried - just by the system instead of by you. If the system itself doesn't care what the value is and only you do in your own code, then it only evalulates where you choose to query it."

That is a consistent rule - they only update when queried. Oh, and by the way, the system itself will be querying these 4 variables even if you aren't.

AlchemistCH commented 8 years ago

Wait, if you query throttle in the script it will reevaluate the expression again based on current data or return the value it last sent out? In the light of what these special variables really do once-per-frame update may be more proper

Heh, I haven't even realized that LOCK already works with any variables, not just the cooked control.

dewiniaid commented 8 years ago

In the light of what these special variables really do once-per-frame update may be more proper

While using functions is almost certainly bette when doable, a once-per-frame update potentially breaks a lock expression that has side effects, E.g LOCK nextnum To Counter()