add orbit constructor and velocityat/positionat #1660
Conversation
adds a constructor for Orbit (OrbitInfo internally) structures so that users can construct their own orbit objects. this lets a function that can build a hohmann transfer node to the mun's orbit also be reused to transfer to a keosynchronous orbit. because the orbit structure has no associated orbitable body it is not possible to use positionat(orbitable) and velocityat(orbitable) functions, so therefore added positionat and velocityat functions to the orbit object (APIs that do not take into account maneuver nodes and are documented as such). docs updates and kerboscript tests included.
|
🚧 don't whack merge yet. i still at least need to debug the code to fixup the argument of periapsis. also i'm not wild about using strings for bodies in the interested in getting feedback though. |
|
All bodies are bound variables, so there should be no issue with setting the parameter to a body type, and then the bare word |
|
yeah its just that nothing else in |
|
I'll look at it close in the morning, but you should be able to cast |
|
yeah it definitely doesn't define that method. and i don't quite grok what the code there is doing... |
|
(i'm also going to go watch some GoT soonish and check it out tomorrow...) |
|
this fixes the API but the tests are uncovering some bugginess... still 🚧 don't merge yet. |
|
two issues:
|
|
The problem seems to be the inverse rotation that gets applied to the ship below a certain altitude. If orbits are constructed when the ship is above the The ArgPeA fixup also seems to be working fine, but the LAN is throwing NaN for perfectly |
|
so right now this takes body-centric coordinates. it should probably be modified to take ship-local coordinates. that would make this work: then it also has a path forwards to fixing the inverse rotation problem... |
changed to ship-local from body-centric, docs updated
plus minor doc fix
|
@hvacengi i think i'm satisfied enough with this to whack merge. I fixed up the LAN. I also noticed that the ArgumentOfPeriapsis fix that mechjeb does has a radians/degrees bug in the Math.acos() branch. I doubt that what we set for these values matters at all, but it will be better for kOS users since otherwise they'd need to work around "NaN on the stack" issues which is not delightful (and would troll anyone trying to do a perfectly circular and/or perfectly equatorial orbit, which is a pretty likely use case--by setting them real--if fake--values then we can avoid those problems). I talked a bit with NathanKell and there might be fixes coming in KSP itself for this issue, so I don't think its worth spending a ton of time on it (and if KSP fixes it, then IsNaN will be false and the code will just be dead code, which is tech debt but shouldn't hurt anyone). I switched the API to use ship-local coordinates instead of body-centric. That makes it consistent with the rest of the kOS API. It makes it easy to pull of x, v values off your current orbit, perturb them, then feed them into the constructor to get a new orbit. That feels like the right shape of the API. I documented how to go from body-centric coordinates to ship-local. Eventually I think this is also the right approach to fixing the problem below. There still is the one bug which I just punted and documented as a FIXME. I think that's fair because MechJeb and everything else has had this bug and I can't find a code solution anywhere. I don't think it comes up too often, but it just happened to troll me hard because I was doing testing while sitting on the launchpad. By switching the API to be ship-local we eventually should be able to fix the ship-local to body-centric conversion that happens here (both vectors need to be rotated in addition to the position being translated by body.position). This is likely a global problem with the kOS codebase and should have a core routine that does the conversion properly taking into account the inverse rotation. Since that issue hasn't come up before, again, I don't think its a particularly big deal and was mostly an artifact of how i chose to run the unit tests. |
| To create an orbit based on the position (r) and velocity (v) from the center of the body a translation is required: | ||
|
|
||
| SET o TO ORBIT( r + body:position, v, body, TIME:SECONDS ). | ||
|
|
There was a problem hiding this comment.
I think if we are going to provide a way to build an Orbit object directly from parameters without a vessel, we should probably also have a second version of this constructor that does it by Keplerian parameters rather than by position/velocity. I'm thinking of the contracts in which you are told to get a satellite into a specific orbit, and you are told the parameters of that orbit. It might be useful for people to be able to construct that orbit object from those parameters.
|
@lamont-granquist : I'm doing the review and merge for this. Do you mind if I swap the order of the args and put the body first in your I'd do the work to edit your kerboscript example to match. My reason is I want to also add a constructor based on inclincation,ecc, etc, and for that one I want to default mean anomaly at epoch and epoch time to zero if the user leaves them off... but to default args they have to go last... and it would look very weird to end with My only reason for asking is that I don't know how much you've been using this code already in your own work, and if I made this change you'd have to edit your scripts to match. |
|
@Dunbaratu i'm only using it in one place so far, i can swap the args around easy. |
|
the only concern i had around the keplerian parameters version was giving the user better access to stuff like the epoch, but it sounds like you're thinking about that... |
|
I only just had the epiphany yesterday to finally understand what the heck SQUAD was doing with Firstly, "epoch" usually implies something universal. The notion that you can pick a different one per orbit sort of defies the definition of the term as it is used everywhere else in computer programming. That was one barrier to my understanding. I didn't expose it because I didn't want to mess with something I didn't understand. Secondly, the realisation that the ONLY meaning "epoch" is used for here is in conjunction with "meanAnomalyAtEpoch" was needed. That effectively the pair of fields together represent a single item of information. And that's why I never understood what they were for before because that is utterly unnecessary for what the purpose of the fields is. If their only purpose is to have a concept of where the object is in the orbit at any given time T, and if you are free to fiddle with the epoch per orbit at your leisure because it's not a real "epoch", then why bother with meanAnomalyAtEpoch at all? Its an utterly pointless (and very indirect when the orbit is not circular) way to answer that question when you could just as easily have picked a different epoch timestamp. Just pick a time at which the orbit was at periapsis (effectively making meanAnomalyAtEpoch zero for all cases so you don't have to store it). I'm reluctant to expose that to kerboscript because it's just so utterly unnecessary for it to be that complex, and I have no idea what SQUAD was thinking there. Either fix the meanAnomalyAtEpoch permanently to zero, and vary the epoch to get the timing you need, or go the other way and fix the epoch so the word "epoch" really means what it says, and then only vary the meanAnomalyAtEpoch (or better yet just use an offset from epoch in seconds - the time at which this orbit hits Pe). |
|
I think someone mentioned that "epochs" for orbits was probably a way to avoid the kraken by reducing floating point overflow? The picture that i have in my head is that when the numbers start getting too big KSP notices it and rewrites the "epoch" to get the times back closer to zero (then somewhere along the line those times are getting multiplied by something and getting rather big and losing precision if you don't do that) |
|
Keeping the floating point numbers low still doesn't require BOTH epoch AND meanAnomalyAtEpoch. Just edit Epoch to be the time of the most recent Pe rather than the time of a Pe that happened 50 orbits ago, and keep meanAnomalyAtEpoch zero. |
|
shrug dunno. generally in any large codebase there are $REASONS that are probably due to some derp someone made 4 years ago that has never been important enough to un-derp... agreed, though, that kerboscript users should hopefully not need to get exposed to that... that's one reason why i looked at the keplerian constructor first, but rejected it and implemented the position/velocity constructor instead (although MJ prefers the latter as well, and it turns out to be a bit easier to perturb an orbit with a burn using it). |
|
I'm thinking of just exposing one number "time of a periapsis" - pick some timestamp and that is when this orbit has hit Pe at some point. Then under the hood I can calculate an Epoch and meanAnomalyAtEpochthat would be like that. |
|
It would be a breaking change because it would get rid of |
|
sigh. okay so I have seen examples of people using meananomalyatepoch and reverse engineering what the epoch is, so I can't remove it without breaking things. Fine, I guess I have to expose the ugliness to the scripters, but I'll try to document it as "you don't need to know how meananomalyatepoch works if you just set it to zero and then set the epoch to the time of most recent periapsis." |
|
I figured both - two suffixes : etatoperiapsis (obviously a time in the future because its just an offset, not a universal time), and periapsistime (the universal time of previous Pe). I'm not entirely comfortable with giving an ETA time to a constructor. It just feels wrong to have it depend on when the constructor got run. It feels more right to have the constructor use a universal timestamp, in which case we should expose that as a member suffix too (thus the periapsistime mentioned above) |
|
FWIW, i kinda think there should be stuff like trueanomlyat, meananomalyat, eta:asendingnode, eta:descendingnode -- even though we've all had to write those -- i think it'd make it quicker to get to usefully launching rockets... |
|
On a purely administrative note: This discussion makes it clear to me that I'm going well beyond the scope of the small little changes to the code that a mere PR review should be allowed to do. At this point I'm taking about implementing something big enough that it really should be PR reviewed by someone else other than me, and not snuck through under the radar in my review of Lamont's PR. Therefore I think it makes sense to split this idea off into a second PR to come later on. I'll review and merge Lamont's PR with the limited scope of just implementing the pos/vel constructor only. The addition of a second constructor, and these other suffixes to OrbitInfo, can be done in a second PR by me that someone else can review. |
|
I'll only make one last "off topic" comment before we move this to another issue: I waver back and forth on giving that much predictive information. I honestly feel like I have a much better understanding of orbital mechanics because I had to figure out how to do the calculations, rather than just being handed the values. I understand that not everyone uses kOS because they want to know the math. But I feel like ascending nodes especially would make it too easy. Those values would not be useful for orbital intercepts, only for adjusting your own inclination while maintaining the same LAN. Orbital intercepts are a fairly advanced topic, and it may be harder for users to transition if they were handed the values when adjusting inclination on it's own. Of course, the opposite may also prove true where they have an easier time by incrementally learning the orbital mechanics. Like I said, I can't seem to come to a final position on the issue. As far as I'm concerned, there's no reason to implement both mean and true anomalies, since they translate to the same information. I'd be inclined to only give true anomaly values instead of mean, since you can solve for mean anomaly without having using a numeric solver. |
|
I'm merging this without the Keplerian parameters constructor for now. However, I am still flipping the order of the arguments to put |
|
👍 |
|
@lamont-granquist - the kerboscript test you included with this PR doesn't really work. It tries to use the "exit." command, that doesn't exist. Was that meant to be a deliberate crash only when the test fails? |
|
I'm not sure what's going on yet, but when I try your test script, @lamont-granquist, I end up with an x0 and an x1 vector that are totally different from each other, and this makes the script abort. |
|
Its the problem with the inverse rotation that gets applied when you're too low in the atmosphere. If you try it multiple times sometimes it nails it, sometimes it fails. I don't think its a huge issue to block shipping since most people will do their orbital computations when they're in orbit. I've ping'd NathanKell about it and been refered to taniwha and have sent off a question about inverse rotations to try to get some clarification and fix the bug anyway -- but its a hell of a yak to block shipping on -- and mechjeb has had the same bug, probably for years.... |
|
Well then we probably need a new kerboscript test for it. I don't want a test program that makes success and fail both look like fails. (i.e. it should be a program that doesn't do things that depend on this behavior and therefore just works either way, regardless of this problem.) |
If you take another look at my screenshot image, and look at the KER readouts for Apoapsis and Periapsis, you can see that I was in orbit when I ran it. Pe = 80km. I just tried again, raising my orbit to Pe=666,040m and Ap=929,338m first, and then re-running. I got the same general sort of problem. I don't know exactly what the problem is, but maybe the orbits being perfectly circular is part of it - it might be making the math for "where is the periapsis of this orbit" really swingy and arbitrary. |
|
Okay I think I have the answer. It's got nothing to do with the altitude you perform the test at. 1 - How long was it since the game last updated the epochs of orbits. Note that coming out of time warp seems to reset them. 2 - How high is your IPU? You have a very very LONG list of code that keeps taking sample data like Sadly, I don't see a good fix. Making an orbit from state vectors, when those state vectors describe an exactly circular obit so precisely that it's unlikely natural piloting would ever achieve it, and the orbit only stores its time-based positioning as "angle since pe" (using two values to do so, but still that's basically what it's doing), means it's always going to be a swingy prediction. So I guess we can merge it, with the caveat that it's going to give really messy results sometimes if you managed to achieve a perfectly circular orbit. |
|
@lamont-granquist - Please look at my edited branch of this that I just pushed to my fork, at: https://github.com/Dunbaratu/KOS-1/tree/merging_1660 And let me know if my edits meet with your approval, especially my edits to the user docs. This is the version I propose to merge into develop, if you're okay with the edits I did. (Edit: okay so it's not that easy to see the diffs, because I also have all the recent develop changes in my branch too, and your branch from May doesn't have them.) |
|
All I see is d86dd6e? So if the problem is just unnaturally perfectly circular orbits then all my yapping in the comment is wrong and it should just be deleted. We should probably perturb the orbit in the test a bit to make it not circular so that it passes (probably also make it not perfectly equatorial as well). Was I getting better test results because I had a faster (desktop) computer? I can try it on my laptop and see if it fails. I can work on storing time:seconds and ship:position. I wonder if the experimentals that have the fixes for NaN LAN + ArgApo stuff will help just make the perfectly-circular-orbit problem go away? The problem is that KEOsynch was literally the first thing I wanted to do with this, and that is a perfectly circular, perfectly equatorial orbit if you get the math correct -- and I suspect others will want to do that as well.... |
|
(lemme work on it a bit today and see if I can get it in better shape) |
|
Yeah it may be possible to make the problem go away by deliberately choosing some standard epoch and meananomalyatepoch when first calling KSP's Orbit() constructor, and THEN calling UpdateFromStateVectors. That might help freeze a known Pe so the KSP API doesn't have to make a random guess at one. I didn't have time to test any of my theories out. You should definitely try with a noncircular orbit and see if it is more reliably giving the same answers. The only reason I didn't adjust your test to do that is that I saw too many places it depended on a circular orbit for the test to pass. It would probably take a newly written test script from scratch to really test my above hypotheses out (that being that noncircular orbits work just fine and the problem only happens on circular ones). It may be worth your time to confirm that that's true first before doing anything else. I only suspected that was the case. I didn't have time to test if that was really the problem. |
|
@lamont-granquist do you want to do more tries with this now that KSP 1.1.3 claims it did things to the way Pe is dealt with in circular orbits? |
|
yeah past couple days have been bad for me wanting to hack on any code outside of work, i'll try to get to this on 1.1.3 now... i did get my laptop setup to build develop, so should be able to test fairly easily... |
|
first test looks a lot better, no weird random rotation. i'm backing out the IsNaN hacks to see if those are necessary now. i'm getting some failures, but i'm suspecting that its more related to how i wrote the test per your point 2 above re: IPU and such.. |
|
@Dunbaratu that last commit backs out all the perfectly-circular-perfectly-equatorial hacks that seem to be no longer necessary in 1.1.3 after that i'm getting perfectly stable testing, even without addressing my poorly performing kerboscripting (and with IPU of 200) |
|
so, i gave it another shot and it decided to start failing again.... |
|
So what is the status on this? Were you planning on changing it some more or should I give what you have so far a look and see if I can come up with something? |
|
i think we do need to remove the hacky bits now -- at least you should merge that last commit and/or just remove that code from your branch and test. |
|
there's still the inverse rotation problem, i've got confirmation from taniwha that its probably the root cause. i don't have a code solution. i should probably try to replicate the inverse rotation problem without perfectly-circular/perfectly-equatorial in the mix to make sure that its got nothing to do with that. |
|
i'm a little bit confused that you hit the problem in orbit, but maybe you were still low enough. and in 1.1.3 the inverse rotation altitude got dropped some if i recall the changelog correctly. |
Wait, so are you saying that when you said:
That you were talking about a completely different kind of failing than I was? Because I was never talking about the problem of running it while you're on the launchpad, or in atmosphere. That was never the case I was testing. I was talking about what's shown in the picture: already in orbit, and the behavior seems to be either fixed, or caused by, time warp resetting something. If I'm getting the problem, and I time warp a bit, then drop from time warp and try again, I stop getting the problem. If I'm not getting the problem, and I time warp a bit and drop from time warp, sometimes I start getting the problem again. Nothing to do with inverse rotations, I think, but something to do with resetting where the notion of the orbit's Pe is. I haven't tried since 1.1.3 because I thought when you said:
That you were already talking about the same thing I was seeing, so I waited to see what you'd find. But if you hadn't ever been able to recreate the problem I'd had, then I should have been doing my own testing all along on this. |
|
yeah, i haven't tested your problem, you should be testing that. i'm just still seeing the launchpad / inverse-rotation issue. |
Having wrote my own lexicon-based custom orbit object, allow me to say: Yes please, though I have another idea on how this can be represented (like #1699 but for anomalies -- the only requirement for conversion is eccentricity). Going from mean anomaly to eccentric anomaly (and thus to true anomaly) requires trial and error possibly via a newton solver. Having tightly looping math run in "native" code rather than the kOS VM would be helpful from a performance perspective. (I say this as someone who has tried to port Alexmoon's Lambert solver to kOS and saw a test run of 100 start times x 100 time of flights take 15 minutes .. for what my browser did in a few seconds) If you need help with any of the math aspects of this, please ping me. I suspect most is available as built in KSP code or orbit extensions as used by MJ, but let me know. |
adds a constructor for Orbit (OrbitInfo internally) structures so
that users can construct their own orbit objects.
this lets a function that can build a hohmann transfer node to the
mun's orbit also be reused to transfer to a keosynchronous orbit.
because the orbit structure has no associated orbitable body it is
not possible to use positionat(orbitable) and velocityat(orbitable)
functions, so therefore added positionat and velocityat functions
to the orbit object (APIs that do not take into account maneuver
nodes and are documented as such).
docs updates and kerboscript tests included.