KSP-KOS / KOS

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

Suggestion: make suffixes that return velocity as a vector, not a structure #2489

Open pand5461 opened 5 years ago

pand5461 commented 5 years ago

Right now, one has to use Velocity:orbit and Velocity:surface to get the velocities in the inertial and rotating reference frames, respectively. This has two issues:

The proposal is to change the kOS interface to explicit :srfvelocity and :obtvelocity suffixes that simply return vectors, and introduce obtvelocityat(object, time) and srfvelocityat(object, time) functions that also both simply return vectors.

Dunbaratu commented 5 years ago

I don't understand this claim:

position literally is orbitable:position and velocity literally is orbitable:velocity. As in velocity is a shorthand for ship:velocity, and ship is a Vessel, which is an orbitable.

orbitable:velocity is the abstraction that works the same for a vessel or a body, and in both cases it returns a type that holds both the surface and orbital variants. In the surface variant the parent body's rotation is subtracted from the velocity so you get your velocity relative to the spot on the parent body under you, and in the orbit variant it isn't.

I don't see the supposed difference you're talking about. Ship:velocity:surface does the same thing that Mun:velocity:surface does (velocity relative to a spot on Kerbin, the SOI body that both are orbiting assuming the ship is currently in Kerbin's SOI.) because both Ship and Mun are of type orbitable, and the velocity suffix comes from that and does the same thing for both of them.

I don't see the inconsistency you are claiming exists.

pand5461 commented 5 years ago

I'll clarify: for any flight prediction, one needs an initial position and an initial velocity as vectors. The former is one suffix away from an object reference, the latter is two suffixes away. Is there a really good reason to do that vs. to have one-suffix aliases for the orbital and surface velocities, just like there are prograde and srfprograde directions?

The inconsistency is worse for positionat() and velocityat(). I have the "cannot use * on Scalar and OrbitableVelocity" more frequently than I'd like.

I understand that the structure mirrors the underlying code structure. But is such mirroring necessary?

Dunbaratu commented 5 years ago

Is there a really good reason to do that

The fact that the orbital versus the surface gives a different answer is only true of the velocity, not the position. If I did add two different suffixes for ship:position:surface and ship:position:orbit, just for "consistency", they would return the same answer as each other anyway. That's why velocity has two types and position has one. If you are calling that "inconsistent", well then that "inconsistency" comes from the very definition of what those things are.

Yes, it is possible to add shortcut suffixes that skip the middle step, for convenience. (Not for "consistency", just for convenience of trimming things down.)

I understand that the structure mirrors the underlying code structure. But is such mirroring necessary?

They underlying code structure is that way to mirror the physical data these things represent (where there are two velocity frames but only one position frame). The positions are the same whether orbital or surface, because they aren't measured with respect to the planet's axes in the first place. Rotating the planet only changes the velocity. In order for there to have been such a thing as :position:surface and actually have it differ from :position:orbit depending on whether the planet rotation is taken into account, that would require position:surface to have been some kind of polar coordianate (which is essentially what LATLNG+Altitude is giving you).

pand5461 commented 5 years ago

The fact that the orbital versus the surface gives a different answer is only true of the velocity, not the position.

I do not understand this part. Say, there is a satellite in an elliptic orbit not synchronized with the planet's rotation. It's logical to say that its orbital position is the same at each periapsis pass. What do you mean by "the orbital versus surface gives the same answer for position" in that case?

Position in a specific (rotating or non-rotating) coordinate frame can be expressed as a tuple. The velocity in the same frame is a tuple showing how fast the position tuple changes. Going from one reference frame to another changes both position and velocity tuples in a consistent way.

There is a difference between position and velocity transformations in the fact that positions only rotate whereas velocities rotate and shift. In that sense, we are indeed not interested whether the frame rotates or not when we have to transform the position, we only need the current orientation of the axes. Of course, when one changes from one inertial frame to another, both position and velocity tuples only rotate.

For velocity, what kOS outputs as "surface" and "orbital" velocities are in fact "rotated and shifted" and "rotated" tuples representing the velocity in the "prime" coordinate system. If you look carefully, you can see that velocity:orbit does not change as one would expect a "true" orbital velocity would, i.e. dV/dt != -mu*R/|R|^3 when the ship is below the rotating-to-inertial frame transition (the difference is quite noticeable, about 0.6 m/s^2 for a circular 100 km LKO). That's because the velocity is expressed in a new inertial coordinate system each tick.

Dunbaratu commented 5 years ago

None of that has anything to do with what you said before in this issue.

There are two different issues:

1 - You don't like having to go through a separate second structure to get to the orbital versus surface values for velocity and would prefer them to just be two differently named suffixes. You also claimed this somehow constitutes an inconsistency.

2 - Regardless of how many suffixes you have to go through to get to it, you don't like that the orbital velocity and position is in KSP's native coordinate system instead of one that is fixed to the universe (such as what it would be like if Xaxis = solarprimevector all the time.)

I asked what the inconsistency was about -1-, and you answered as if you had been talking about -2-, which you never brought up in these comments until just now, and truly has nothing to do with -1- at all.

When I mentioned how position doesn't change with frame and velocity does, that was in regards to -1- to defend why the chain to get to the information is different for position versus velocity which is the thing you were talking about. In the current system, the position doesn't change because the current system isn't doing -2-. Until now, you didn't mention -2- in this issue at all.

That's why I asked for what the inconsistency was. If you were describing something supposedly inconsistent, then what you were talking about couldn't have possibly been what you said it was.

It's relevant because had I done -1- like you asked for, which is a minor quick edit, and it hadn't satisfied you because you really wanted -2-, it could have taken a month or two for the next release before you would realize what you wanted isn't what you got.

To do -1- is simple and quick. It won't give you -2- though, or fix any of what you're mentioning as inconsistent. To get -2- requires a big refactor to make sure I've caught everywhere that a KSP value about position, velocity, or accelleration can be accessed by the script with get or set and run all of them through a conversion. What I don't want to do is end up making a mistake in that and only converting some of the vectors but not all of them. Now that would be an inconsistent nightmare. Thus why such a refactor requires being careful - probably trying to invent a system that makes it impossible to mistakenly miss a spot in the code.

Dunbaratu commented 5 years ago

Also, making the position fixed in space so it matches the velocity being fixed in space (which would be needed to truly have position and velocity consistent in the same reference frame) would be a massive problem for a lot of already written scripts that presume V(0,0,0) is the ship. (i.e. they can assume that target:position:mag is the distance to the target right now, an assumption that is totally wrong if v(0,0,0) is fixed in space rather than moving with the ship.)

The ugly fact that position uses an origin that moves along with the ship, while velocity assumes the ship is in motion, and thus they are not the same reference frame, is baked into KSP at a deep level because it's how they dealt with the fact that floating-point numbers get imprecise at larger magnitude values. Fixing the origin point for position in space would mean putting the ship at large positions like V(1000000000,1000000000,1000000000) and at numbers like those, 32-bit floats aren't very precise. (The game itself uses 64-bit floats for its purposes, but Unity uses 32-bit when talking to the graphics card, so at some point these numbers have to be converted down into 32-bit to be handed off to the graphics card that calculates collisions, and it gives a lot of false positives for colliding parts when the magnitudes are that big and thus imprecise. That's where the Space Kraken came from, and it's why the position is in a reference frame that moves with the ship even though that's not consistent with the reference frame they use for velocity where the velocity vector says the ship is moving at 2380 m/s.

pand5461 commented 5 years ago

There are two different issues:

I'm talking about (1), as fixing (2) consistently is a huge change affecting everything about steering, flight prediction etc.

The inconsistency I'm seeing in naming is that (a) orbital and surface velocities are packed into a structure whilst positions in inertial and rotating frames are not discriminated at all, and srf/obt prograde are two different suffixes and (b) Orbitable:orbit and OrbitableVelocity:orbit suffixes return different datatypes.

That often causes little inconvenience when one only uses object:velocity. However, it gives rise to expressions like object:orbit:velocity:orbit which is just ugly, and it makes similarly-looking functions positionat() and velocityat() return different datatypes. I don't know how large is a portion of users who uses velocityat(), but in my case it's done exclusively for passing the result into an orbital solver, so that the need of additional :orbit suffix to the velocity feels superfluous.

On the consistency between the velocity and the rate of change in the position, I'm OK with the idiom that the "velocity" is actually the rate at which object:position - body:position changes. As such, just having coordinate axes that either consistently rotate or consistently stay fixed is fine to me. But that's another issue entirely.

Dunbaratu commented 5 years ago

I do not agree at all that -1-(a) is a naming inconsistency. More on that later.

But about -1-(b) I agree. The two types of direction quaternion are accessed through two different suffixes that each return a different quaternion, while the two types of velocity vector are accessed through one suffix that in turn returns a tuple of two vectors. That is the only inconsistency I see that has anything to do with naming. Both for Directions and Velocities you have two values, one for surface and one for orbital, but the means to access those two values is different. But to be fair, this is NOT the "inconsistency" you were talking about when you opened this issue. You were talking about -1-(a), a discrepancy between position and velocity, rather than between direction and velocity. That's why I brought up issue -2-, as it's the real cause of that discrepancy that you were first mentioning.

The reason I don't agree with you about (a) is because the naming difference was the effect of the difference between how velocity and position are handled, not the cause of it. The fact that there only is one kind of position offered but two kinds of velocity offered is why velocity was given that extra suffix to get at the specific one, and position was not. The proposed change this issue asked for does not get rid of this discrepancy at all. All it does it transform it into a slightly differently named version of the exact same discrepancy. Instead of "Why does velocity have an additional suffix distinguishing orbital vs surface and position does not?" it would become "Why are there two velocity suffixes called srfvelocity and obtvelocity when there's only one suffix called position, with no srfposition or obtposition?"

Thus my view that the "naming" inconsistency you labeled as (a) (and the one that this issue was talking about originally), is actually just an effect of issue -2-, the more difficult refactor. Before velocity and position can have similar naming conventions, first there would have to actually be two different kinds of position to report just like there's two kinds of velocity. As long as there's only one kind of position and two kinds of velocity, the "naming" inconsistency has nothing to do with naming, and can't be fixed entirely with naming alone. (Or to put it another way, as long as there is still only a single kind of position, but a tuple of multiple kinds of velocity, then it's actually correct for it not to work the same way velocity does because the actual information itself doesn't have the same hierarchy.)

But now you bring up issue -1-b, which you didn't mention at first. That is a legitimate reason to want the change you propose. Because unlike with position, with directions there actually are indeed two different types the script can access, orbital versus surface, for both directions and velocities, and so those should be able to be accessed similarly.