Sanitize values sent to KSP

[marianoapp]

Sometimes sending a double with a value of NaN or Inifinity to KSP causes the game to crash showing a black screen.
To avoid this we should sanitize all the values sent to KSP and raise an exception if we found either of this ones.

[Dunbaratu]

Values being overwritten and becoming NaN is also an effect of the black screen crash. Values that had not previously been NaN become NaN because of the crash, making it difficult to trace exactly whether or not the NaN is responsible for the crash.

[marianoapp]

The issue in your example was actually caused by the Infinity in the terminal velocity variable and all the NaNs were a consequence of the crash.
The throttle was set to Infinity without problem but when you locked the steering to something the steering code used the current throttle to calculate the engine torque and then everything crashed (apparently Vector3d vectors don't like Infinity components).

[Dunbaratu]

It's possible that something breaks when I try to use the value of infinity somewhere in an expression, but that somewhere cannot be the steering or throttle. The steering is locked to HEADING(compass,mypitch) and compass and mypitch neither use terminal velocity nor throttle in any way in their calculations, and even when I ensure the throttle never exceeds 1.0 it still happens. I have confirmed that for both steering and throttle, the "NaN"s appear to be externally caused (by the crash), and even when I ensure that nothing using infinity appears in either one of them, the crash still happens. Maybe this new change will help trace it because it will flag the exact moment when the fly by wire values first became any bogus value (Infinity, NaN, or Null), at a level more fine-grained than a line of kosscript. It's possible that the error happens in a lock expression or a "when" trigger, in which case trying to track when it happens in the kosscript doesn't quite work, as these are called outside the normal program flow. Perhaps the new change will help find it.

[marianoapp]

What I meant was that the kOS steering code (in the SteeringHelper class) uses the current throttle to calculate the engine torque. In that calculation it builds a Vector3d object with an Infinity component (the one calculated with the throttle) and that is what causes the crash.
I used the example you uploaded to the forum and verified that the throttle set to Infinity is the cause of the crash.

[Dunbaratu]

I'm trying to compile this new change and I get this error now that didn't happen before:

"The type or namespace name 'KSPLogger' could not be found (are you missing a using directive or an assembly reference?) (CS0246) - C:\Users\Dunbaratu\Documents\GitHub\KOS-1\Module\kOSProcessor.cs:113,38"

Do I need to add something more to my compiling environment for this? The version just prior to pull request #14 compiled just fine for me.

[Dunbaratu]

The title of the issue is "Sanitize values sent to KSP". But reading the code associated with the pull request #14, it looks like that code doesn't sanitize the values sent to KSP, but it appears to instead prevent NaN or infinity from ever being used by the script writer, ever, in any expression that will ever push its result onto the execution stack, regardless of whether there's any danger of that value ever being sent to KSP or not. If I'm reading it correctly, and I apologize if I'm not, then I really don't like that. It seems to be built upon the premise that there is no such thing as a valid reason ever for any programmer to ever have a value of Infinity or NaN. If that was true then the IEEE standards for floating point representations wouldn't have included them.

I realize I'm complaining a lot and coming across as impossible to please but in all honesty I really do think that It's a Bad Idea to deliberately design things so that a kosscript will bomb out and die in ALL cases where you get an Infnity in ANY math expression result or even just in a mathematical sub-expression pushed on the stack, especially since the script programmer is helpless to trap those exceptions.

For example, the terminal velocity should be infinity if there genuinely is no air. That's the correct answer in that case, not a programming error.

Can the values for throttle and steering be sanitized without destroying the ability to correctly use the IEEE standard values of Infinity and NaN?

Again, I don't want to sound like I'm not grateful for the effort you're putting into this. It's a lot of hard work, I just think this change may not be a good idea.

If, as you say, there's a problem with any and all vectors that contain infinity or NaN, then can the sanitizing happen there instead? Any attempt to build a vector out of three x,y,z values would be where the check happens?

[marianoapp]

I forgot to commit the updated project file, it's fixed now.

And yes, the fix prevents any Infinity or NaN value to be pushed onto the stack. I know it seems a bit too restrictive but adding validations everywhere we interact with KSP/Unity would be a maintenance nightmare. I also know that Infinity and NaN are valid values and have their place, but since Unity don't like them we have to find some workarounds.

Anyway Infinity doesn't really mean infinity, it's just a value used when the result doesn't fit in the precision of the data type (a calculation can return Infinity if performed with floats and a correct value if performed with doubles). In the case of the terminal velocity we could return 1000 km/s instead of Infinity and, from a practical point of view, the result would be the same (and the terminal velocity can never exceed the speed of light, so it will never be infinity anyway).
If you are writing a script where you might have a division by zero then you would have to add the required validations to avoid that, after all, throwing an exception on a division by zero is a completely valid reaction.

[Dunbaratu]

Thanks for fixing the update file - I just fetched it again and it compiles now.

Well, I guess I'll just have to live with that "solution", but it just doesn't feel right. Like I'm taking a step backward into the days before the more elegant solutions to these programming issues had been invented.

On the side topic: a value for terminal velocity that is higher than c is not incorrect. It just means "You won't be reaching terminal velocity." Terminal velocity doesn't actually have to be an attainable speed. Nothing in the definition of the term implies that. KSP doesn't implement speed of light limitations, and operates on an entirely Newtonian model anyway.