KSP-KOS / KOS

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

Getting any wrong BODY suffix throws exception #1944

Closed romanran closed 7 years ago

romanran commented 7 years ago

BODY("nosuchbody"):istype("BODY"). throws an exception "BODY structure seems to be empty". `BODY("nosuchbody"):EMPTY.BODY("nosuchbody"):HASSUFFIX(...` any suffix, gives the same exception. Please change the exception catching for getting suffixes of a wrong object.

Dunbaratu commented 7 years ago

Please change it to what? The body by that name doesn't exist. It's never going to work. Are you just asking to change the phrasing of the error message? If so, what are you suggesting to change it to?

romanran commented 7 years ago

https://ksp-kos.github.io/KOS/structures/collections/lexicon.html#structure No. Change the line that throws exception on any suyffix. The body structure is there, but empty. Change it ,so we know there is an error and handle that exception (by getting :hassuffix or :length or :empty), instead of crashing whole program when getting any of structure suffix.

Dunbaratu commented 7 years ago

There is no such thing as an empty structure in kOS at the moment. What you're asking for is a very large change, not just a change to the body structure, but a change to ALL structures.

Basically, the Body structure is the weird outlier here. None of the other structures have this notion of "existing yet being empty". For any other structure, it would just fail to create the structure in the first place. But nothing would create a structure that exists yet doesn't exist.

What you're talking about would be much better represented with a NULL and the ability to test for whether or not a thing is null. There's been a longstanding argument internally on the dev team about whether or not NULLs should be supported in kerboscript.

romanran commented 7 years ago

Did you check it for yourself? If you do set example to body("nosuchbody"). and do print example. it gives structure. Try it

Dunbaratu commented 7 years ago

Repeating myself:

Basically, the Body structure is the weird outlier here. None of the other structures have this notion of "existing yet being empty".

In other words, there is no such thing as a generic "empty structure" to return. But what does exist, is such a thing as an empty Body, because someone decided to make that one class be a weird exception where when the constructor fails to make an object, it doesn't complain about that right away (like all the other structures do) and instead it defers that complaint until the next step.

romanran commented 7 years ago

Yeah you're right. I checked the srouce code https://github.com/Nivekk/KOS/blob/master/Structures/BodyTarget.cs

if (BodyRef == null) throw new kOSException("BODY structure appears to be empty!"); There is no way to check if its correct in the constructor or return something when creating an object?

Dunbaratu commented 7 years ago

Sadly, no. But the fix isn't to try to claim it's wrong for Body to fail on a nonexistent body name, like your issue claims. It's correct for it to fail. The problem is that a script is powerless to detect this failure unless we make a special exception for this one case and implement a HASSomething suffix for it. There's no generic way for a kerboscript program to test for null, owing to a design decision that I have never agreed with myself.

romanran commented 7 years ago

Yes I get it. Thank you very much for your time on this very minor issue of mine! Appreciated!

Dunbaratu commented 7 years ago

Don't apologise for opening the issue. The more of these we get the better case I can make for reversing the decision to make kerboscript lack any kind of a NULL check.

At any rate I was very surprised to see that special case exception in the Body structure, and wouldn't have noticed it if you hadn't started this issue. Everywhere else we would have just failed to construct the object in the first place and thrown the error then.

But the idea of making Structure actually be a real class type that can be instanced might be a good way to deal with the NULL issue. :ISNULL as a generic Structure suffix might make sense.

romanran commented 7 years ago

Yes, try/catch would be perfect, but at least knowing that returning value is null or that infinity is getting pushed to a stack :D could give us a tool to handle such exceptions. Now I feel completely helpless, even when I know exactly where is an issue. By the way, do you know if you can reboot a cpu from another cpu? That way we could listen if a "mother unit" is still running, and if it's not responding, reboot it by sending like aPROCESSOR("mu"):reboot. message. I think I have gone too far out with this one :D

Dunbaratu commented 7 years ago

You can use the generic partmodules system to activate the other CPU's "toggle power" GUI button, perhaps?

romanran commented 7 years ago

GENIUS! I think I may actually try that haha

hvacengi commented 7 years ago

Additionally kosprocessor has both activate and deactivate suffixes with which you could toggle the power. http://ksp-kos.github.io/KOS_DOC/structures/vessels/kosprocessor.html#method:KOSPROCESSOR:ACTIVATE

hvacengi commented 7 years ago

Sadly, no. But the fix isn't to try to claim it's wrong for Body to fail on a nonexistent body name, like your issue claims. It's correct for it to fail. The problem is that a script is powerless to detect this failure unless we make a special exception for this one case and implement a HASSomething suffix for it. There's no generic way for a kerboscript program to test for null, owing to a design decision that I have never agreed with myself.

In this particular case, I would say that it makes sense for the failure to occur exactly at the time of creating it (body("nosuchname")), which is what I always assumed would happen if you used an unknown name anyways. That's how the vessel function works. Which currently leaves us with the limitation where the only way to determine if a body or vessel exists is to list them (list vessels in vlist) and then iterate through looking for a matching name. While functional, it isn't exactly elegant. Perhaps it makes sense for kuniverse to have suffixes that match the global list parameter names, and then we could add lexicon versions. The only issue there is that vessels may have duplicate names, which would mean vessels could very well be hidden, but that isn't any different from the current function syntax.

Dunbaratu commented 7 years ago

It's too bad we don't really have static suffixes (a kerboscript program can't access the VesselTarget class without using an instance of one). If we did, it would make logical sense for querying for a Vessel's existence to be a static suffix method of VesselTarget. (and have a similar one for BodyTarget).

I don't think kuniverse is the right place for the thing you're talking about, @hvacengi, because I always viewed kuniverse as fourth-wall-breaking things about the game simulation environment, and "does this vessel exist" is definitely an in-character sort of thing.

But I don't know where else it would go. It's definitely a global-to-the-universe kind of thing.

By the way, I never liked the list blarg in foo syntax myself because it prevents accessing the list directly to quickly get one thing. You must stuff it into a variable first. I think we kept that on purpose though to "teach" users to save a copy and use the copy rather than regenerating the list again and again each time you want one new thing about it.