Merge BodyTarget and VesselTarget's overlapping functionality into a base class.

[Dunbaratu]

(This is the result of a google chat with @erendrake I just finished.)

There is a lot about BodyTarget and VesselTarget that comes from the fact that both of them are moving objects that orbit. For example, they both have orbits, positions, the parent body being oribited, velocities, apoapsis, periapsis, current altitude, and so on.

But there's a lot more about them that should be congruent but isn't. Like the fact that you can get the BEARING of a vessel from your ship but not the bearing of a body from your ship, or the fact discovered in an issue by @baloan recently, that the type of object returned by a Vessel's VELOCITY suffix and the type of object returned by a body's VELOCITY suffix aren't the same type (one is a VesselVelocity, the other is a Vector).

I've also had difficulty writing trying to write code that gives users the 'closest approach' data without having to repeat almost the the exact same code cut-and-pasted into BodyTarget and VesselTarget. (and when you find yourself cutting-and-pasting almost the same code into two classes, it's a good clue that the design could take advantage of more layers of subclassing.)

So the thing I'm going to be working on next is this:

Make a new class, called Orbiter, and move as much of the functionality into it as possible from BodyTarget and VesselTarget, then make BodyTarget and VesselTarget into subclasses of Orbiter. After this change, BodyTarget and VesselTarget only contain the code that needs to do things differently from each other. All the code that works the same (or should work the same but currently has quirks that make it not the same) between the two of them ends up in the base class Orbiter instead.

My personal motivation for doing this is that a portion of the code I'm trying to write for the closest approach solver is common between BodyTarget and VesselTarget, while a portion of it differs between the two. This split will allow me to put the common code in the base class and the differing code in the subclasses, as per good design principles.

I'm announcing this in an issue because it may affect other people ( ie @marianoapp ) as it does mean rearranging classes that others may have their fingers in right now.

[baloan]

+1. For inspiration and cross-checking I recommend looking at the kRPC documentation here https://github.com/djungelorm/krpc/wiki/SpaceCenter-Service which presents the organization of classes and its properties. The organization IMHO is easy to understand, property assignment is logical and makes sense to me. I also like the presentation as it clearly states argument and return types. It is not perfect as some properties that are available in kOS (like mass, maxthrust, etc) are missing. The hierarchy makes property names very long. For example:

flight = ksp.space_center.active_vessel.flight(reference_frame=ksp.orbital)
ksp.space_center.active_vessel.auto_pilot.set_direction(flight.retrograde)

which corresponds to kOS lock steering to retrograde. Admittedly you can leave out the ksp.space_center which is provided as a root to the ksp property data. Unfortunately @djungelorm, the owner of kRPC, has not logged any activity since May 22nd.

[Dunbaratu]

This is a checklist of the user-visible changes I've done so far in my branch so I remember to document them all later on if my work gets accepted and pulled in. I'll edit this comment as I keep working on it:

---

Breaking

• all velocities are returned as a pair of surface and orbit types All instances of the :VELOCITY suffix in different objects now return a Velocities type object (which stores the pair of :ORBIT and :SURFACE velocities) rather than just the orbital velocity alone. This makes everything work congruently, but might break some old scripts that expected body's VELOCITY to just be the orbital type alone (Velocities is the class that used to be called VesselVelocity - I changed the name because it's not just for Vessels anymore and now also works on Bodies.)
• Objects of type Orbit now need the :NAME suffix in order to show their string name as a bare word. They will now return ORBIT of _name_ as their string value if used in a string context (the print command). This is to clearly differentiate to users that Body(Mun) versus Orbit of Mun are not the same type. Body(Mun) has a lot more utility and more suffix terms so it's an important distinction.

---

Changes to class: OrbitInfo The class OrbitInfo still exists, and has the same properties as before, for backward compatibility, and because there's a few places were objects of type OrbitInfo exist that are neither bodies nor vessels (manuever nodes) and thus cannot be replaced.

In addition, the following new fields are in OrbitInfo: "NAME" "POSITION" "VELOCITY" They give the same thing as the name and position and velocity of the body or vessel, but tend to have a slightly sloppier error tolerance because they're based on the orbit prediction path at time=now.

---

New class: Orbitable The new base class of VesselTarget and BodyTarget is now Orbitable, with some of the common functionality moved into it. Orbitable is an abstract class that required VesselTarget and BodyTarget to implement the slightly different syntax of how to get a position, how to get a velocity, and so on for the two types.

ALL objects of type Orbitable, meaning both bodies and vessels, now have these suffixes handled in common:

(These are the same as OrbitInfo's similar suffixes:) "APOAPSIS": "PERIAPSIS": "BODY": "PERIOD": "INCLINATION": "ECCENTRICITY": "SEMIMAJORAXIS": "SEMIMINORAXIS": "TRANSITION":

(These are moved into OrbitInfo from VesselTarget or BodyTarget:) "NAME": The name of either the vessel or the body. "UP" : same as Vessel's UP, but now works for bodies too (body's orbit's UP relative to its parent body) "NORTH": same as Vessel's NORTH but now works for bodies too (body's orbit's NORTH mapped onto it's parent body, NOT the NORTH of the body itself. - i.e. just as if the body was a vessel orbiting a planet and you took the vessel's NORTH.) "OBT": (The OrbitInfo of the object, just like before) "POSITION": "VELOCITY": (Remember it returns a Velocities now, not a single vector) "DISTANCE": "DIRECTION": "LATITUDE": (i.e. for the body Mun: the latitude on Kerbin where the Mun is directly above) "LONGITUDE": (i.e. for the body Mun: the longitude on Kerbin where the Mun is directly above) "ALTITUDE": (i.e. for the body Mun: the altitude of the Mun's position above the surface of Kerbin) "GEOPOSTION": (THE LATLNG() of the object on its parent SOI body) "PROGRADE" "RETROGRADE" "SRFPROGRADE" "SRFRETROGRADE" "ANGULARVEL"

---

Future update in a new issue and new pull request will have future orbit solver Once this issue is accepted and merged I will then add the functions to calculate the predicted positions of any Orbitable object at a future arbitrary time, and thus also the intercept solver to give you an idea of the closest approach chevrons. But first I want this issue merged in, as that orbit solver will be dependant on the changes made here (so I can use the same code for both intercepting bodies and intercepting vessels for docking), which was the impetus for wanting to make this change.

Then I also plan to implement a closest approach solver that makes use of GetPositionAtUT() to simulate what the little chevrons in KSP are doing, but that should be a separate issue.

[Dunbaratu]

I expect to have the above to-do list implemented by the end of this weekend and up on my local fork for the two others (@marianoapp and @erendrake) to see. It's a big enough change that it should be scrutinized by the other people who would be affected by the changes. There's a LOT of little places in the code where the assumption that only vessels would need full information and bodies don't was built-in.

(One change I'm sure erendrake won't like to see is that I had to pass the SharedObjects "global" object to a lot more places because I need to calculate coordinate positions relative to the vessel the CPU running the code is in. But as per issue #107, I shouldn't just pass in a reference to the vessel, even when the vessel is the only part of shared I actually need.)

[baloan]

It would be great online help if an object can tell about its properties - and maybe even their values. Might look like this:

set var to Minmus.
print var.
<Orbitable class>
print var:props. // or var:dir or whatever you like best.
APOAPSIS: 150000m
PERIAPSIS: 153000m
BODY: Mun
PERIOD: 1287146s
INCLINATION: 6.49484deg
ECCENTRICITY: 0.5
SEMIMAJORAXIS: 75000
SEMIMINORAXIS: 35000
TRANSITION: ?

Now I know I can do:

print var:period.
1287146

It's cool because now many questions in the forum are easily answered and some pressure is removed from keeping the online doc up-todate (I'm NOT implying the online doc is obsolete or should be updated with delay). I found myself looking up the suffixes class in the codebase to find out about what's actually implemented - knowing that online doc lags behind.

[erendrake]

@baloan I agree completely, you should be able to interrogate these structures.

[Dunbaratu]

Perhaps accessed by the user typing a function call like so:

PRINT TYPE(MyVar). And get back something like this:

<Vector>
Suffix                   Get? Set?
----------------------   ----  ----
X                        yes  yes
Y                        yes  yes
Z                        yes  yes
MAG                      yes  yes
NORMALIZED               yes  no

etc. Doesn't print values but prints the whole user-accessible structure.

Implementation: I'd suggest that SpecialValue have an abstract property that requires all subclasses of it to "announce" their suffix terms publicly in some uniform format (a hash that maps suffix strings to structures that contain the suffix terms and the flags for get/set for example) that can be called from the implementation of the SUFFIXES function. (Then GetSuffix and SetSuffix could be modified to actually use the hashmap of suffix terms. Perhaps we make the list contain delegate method references for each suffix and we use them instead of GetSuffix and SetSuffix. It's a backward-compatible step toward having proper methods with just an string name overlay on top of them. Also, it would make it easier to help people like @solarliner trying to write editor IDEs because it puts all the terms in a cohesive format that can be looked at to find them (for syntax highlighting). The only ugly part would be the problem with suffix terms the class itself doesn't hardcode, like the STAGE and SHIP resource terms. If we can find an answer for those cases I'd say we should do this.

[erendrake]

This is exactly what I have been gearing up for. Its will be much easier to inject new suffixes through addons or print statuses and any number of actions. Want to talk about it tomorrow in the meeting? On Jun 28, 2014 12:47 PM, "Steven Mading" notifications@github.com wrote:

Perhaps accessed by the user typing a function call like so:

PRINT TYPE(MyVar). And get back something like this:

Suffix Get? Set?

X yes yes Y yes yes Z yes yes MAG yes yes NORMALIZED yes no

etc. Doesn't print values but prints the whole user-accessible structure.

Implementation: I'd suggest that SpecialValue have an abstract property that requires all subclasses of it to "announce" their suffix terms publicly in some uniform format (a hash that maps suffix strings to structures that contain the suffix terms and the flags for get/set for example) that can be called from the implementation of the SUFFIXES function. (Then GetSuffix and SetSuffix could be modified to actually use the hashmap of suffix terms. Perhaps we make the list contain delegate method references for each suffix and we use them instead of GetSuffix and SetSuffix. It's a backward-compatible step toward having proper methods with just an string name overlay on top of them. Also, it would make it easier to help people like @solarliner https://github.com/solarliner trying to write editor IDEs because it puts all the terms in a cohesive format that can be looked at to find them (for syntax highlighting). The only ugly part would be the problem with suffix t erms the class itself doesn't hardcode, like the STAGE and SHIP resource terms. If we can find an answer for those cases I'd say we should do this.

— Reply to this email directly or view it on GitHub https://github.com/KSP-KOS/KOS/issues/133#issuecomment-47435261.

[Dunbaratu]

Yeah lets talk it over tomorrow if possible. I am not 100% sure if I'll be around after all for tomorrow. Something may have come up but I won't know until tomorrow morning.