Nivekk / KOS

Fully programmable autopilot mod for KSP.
Other
80 stars 30 forks source link

lock steering to HEADING(a,b) is backward (or documentation is). #235

Open Dunbaratu opened 10 years ago

Dunbaratu commented 10 years ago

The readme claims this:

''HEADING (degreesFromNorth, pitchAboveHorizon)

Represents a heading that's relative to the body of influence.

SET X TO HEADING(45, 10). // Create a rotation facing northeast, 10 degrees above horizon''

But in reality it's the other way around when I try it. To go on a heading of 45 on the compass, 10 degrees pitched up, is really HEADING(10,45) not HEADING(45,10).

Either the readme has it backward, or the code has it backward.

I think the readme is right and the code is wrong, IMO, because it's more intuitive to have it in the same order around regardless of whether you say: HEADING A BY B or HEADING( A,B).

Right now if it's HEADING A BY B, then it's HEADING(B,A), which seems non-intuitive.

JoCRaM commented 10 years ago

while the documentation was correct and the implementation wrong, fixing it will break existing scripts.

This, unfortunately, should now be a documentation change not a code change.

a1270 commented 10 years ago

So people don't have to edit 4 lines of code? Decent text editors make that a single line of regex. Fixing a bug in code is worth having to add a note to the changelog.

Dunbaratu commented 10 years ago

By what logic is the claim being made that it was correct for the code to swap the order between using parentheses versus using the words?

I agree that making it correct is more important than making old code work. Backward compatibility is something to start worrying about probably after version 1.0 not before it.

But I don't agree that it's correct that the orders are different between the two different methods. What's the ratioanle for saying that swapping is the correct thing to do? When saying "HEADING .. BY ... " the order is opposite of using parenthesis (...,...) .. and there's no reason I see that it has to be that way.

If you have some argument for why putting the pitch first is the "right way", then be consistent about it and make it also be that way in the wordy syntax as well then. Change it to pitch by heading rather than heading by pitch.

I'm not saying it's more correct to put compass heading first than to put pitch first. I really don't care which way it is. But I do care that it stick with a single convention and is consistent with itself, regardless of which convention that is. EIther make pitch first in BOTH ways, or make heading first in BOTH ways. But don't have them swapped.

JoCRaM commented 10 years ago

I'm sorry,how do I use the "regex" function in the kOS editor?

a1270 commented 10 years ago

@JoCRaM, kOS editor? Do you mean the seemingly outofdate ide or internal editor? If you're using the internal one... Use http://notepad-plus-plus.org/ on the txt files in the archive. Kerbal Space Program\Plugins\PluginData\Archive\

You can do a swap by doing a regex replace with something like: (heading()(\d{2,3})(\,)(\d{2,3})()) in the find box and heading(\4,\2) in the replace. You then click replace all. Make sure you made backups of course. I don't know if this works on notepad++(don't use windows but once a year) but it's valid regex. Not the best regex but that is another matter.

JoCRaM commented 10 years ago

I mean the internal editor. My heading commands use nested parenthesis. Extensively. it'll be somewhat more complicated than that.

my newer scripts just SET pitch TO 90. SET yaw TO 0. SET roll TO 0. LOCK STEERING TO HEADING(pitch,yaw,roll).

and I adjust adjust the variables- they'll be easy enough to change, but some of the earlier stuff spans 4 or 5 lines per change

Dunbaratu commented 10 years ago

And like I said, I'm perfectly happy making the change in my preferred way instead so that HEADING(a,b) works the same as it always had but "HEADING compass BY pitch" is what has to change to :"HEADING pitch BY compass."

I just think the fact that the two are in disagreement with each other is messy and likely to confuse. Whichever has to change to match the other I'm fine with.

a1270 commented 10 years ago

I am somewhat reversing my option. HEADING x BY y is deprecated and is only for legacy scripts, no new scripts should use it. Thus it may be be wise just to adapt the docs. The question is what makes more sense. Probably was a be too strong with defending it. It was a bug and it got fixed it after all, fixing bugs is always means it's better. This will be easily resolved when Nivekk shows up.

JoCRaM commented 10 years ago

the 9.2.2 implementation of heading "matches" the argument order of pitch, yaw, roll) tat the R() operator uses - BUT applies those rotations in a different order.

it makes sense for the argument order to be yaw, pitch, roll - (as documented) - this is exactly what it does.

My suggestion is to create a new name for the function - say headed - which uses the yaw, pitch, roll as documented, and leave the heading function as an undocuented mapping to it. Such a change might also be useful to help prevent confusion between HEADING and HEADING()

Dunbaratu commented 10 years ago

"the 9.2.2 implementation of heading "matches" the argument order of…" No. The implantation of heading() does, but not the implementation of heading. (Those parentheses matter).

JoCRaM commented 10 years ago

[:]HEADING, HEADING ... BY, and HEADING() - are there anymoe overloads?