Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Addon update for Trajectories v2.4.0 #2747

Merged
merged 10 commits into from
Nov 22, 2020
Merged

Conversation

PiezPiedPy
Copy link
Contributor

@PiezPiedPy PiezPiedPy commented Aug 17, 2020

An update to the addons for Trajectories, bringing in support for new API functionality in Trajectories v2.4.0.

See Issue #2263 - A few missing features for Trajectories Addon.

New Suffix's have been added as follows:

IsVerTwoFour() returns true if the installed version of Trajectories is >= 2.4.0
GetVersionMajor, GetVersionMinor, GetVersionPatch return version information.
GetTarget() returns the GeoCoordinates of the set target.
ClearTarget() clears the target.
ResetDescentProfile(ScalarValue aoa) resets all descent profile nodes to the passed angle (in degrees).
DescentAngles returns or sets all the angles of the descent profile nodes via a list of values (in degrees).
DescentModes returns or sets all the modes of the descent profile nodes via a list of booleans.
DescentGrades returns or sets all the grades of the descent profile nodes via a list of booleans.

Documentation has also been updated to include the new changes.

@nuggreat
Copy link

To match with almost all other angles in kOS it would be better if it took/returned the angles in degrees and not radians. As a result of kOS working with degrees is all most all cases if a degree to radian conversion is needed for a given API call it is much more covenant for the users if that is done automatically on the C# side of things as apposed to requiring the users to do the conversion.

@PiezPiedPy
Copy link
Contributor Author

I don't use kOS myself and was unaware that kOS used degrees for most things, I'll add conversion to the addon as Trajectories uses radians internally.

@konstantinua00
Copy link

Is it possible to change IsVerTwoFour() to... something more extendable into the future?
Maybe function that returns version as single number, or major-minor-bugfix trio of functions?

@PiezPiedPy
Copy link
Contributor Author

PiezPiedPy commented Aug 18, 2020

@konstantinua00 There is already a 'GetVersion()' suffix that returns the installed version number as a string.
There are methods in the Trajectories API that return major, minor, and patch as integers so I will add them to this PR later today 😉

tbh there is little reason to check the version integers, for functionality you only need to know if the correct version for the suffixes you want to call is installed, hence why the suffixes that return a Boolean are better suited. I suggest you have a look at the kOS addon docs for Trajectories.

http://ksp-kos.github.io/KOS_DOC/addons/Trajectories.html

var lng = Utils.DegreeFix(body.GetLongitude(worldImpactPos), -180);
Vector3d worldImpactPos = (Vector3d)impactVect + body.position;
double lat = body.GetLatitude(worldImpactPos);
double lng = Utils.DegreeFix(body.GetLongitude(worldImpactPos), -180);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, thank you for this change.
The "var, go look up method elsewhere to see what type it really is" design pattern is one of the most annoying misfeatures of C#. I call it "IDE-think" - people think "well, the IDE tells me the type so I don't have to state it", which is a PITA when reading it somewhere other than an IDE, like on github diff output.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only place I consider it good is when calling a constructor, because there the constructor's name IS the type so it's already clear from that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to agree, I rarely use var, usually only when throwing code together quick, once I'm happy with how the code works I will change the var's to the actual types.

@Dunbaratu
Copy link
Member

@PiezPiedPy I am considering taking the risk of merging this without me testing it, and taking your word for it that it's good. I don't normally use Trajectories and you are the expert here on it. Nothing in the code looks bad or out of place and I had a read through it. Are you okay with that?

@PiezPiedPy
Copy link
Contributor Author

@Dunbaratu I have no issue with a merge and have already tested so there should be no problems, but those pesky bugs always find a way 😜.

@Dunbaratu Dunbaratu merged commit 0f70ef1 into KSP-KOS:develop Nov 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants