Changes to support the Trajectories v2.0.0 to v2.2.0 API by PiezPiedPy · Pull Request #2220 · KSP-KOS/KOS

PiezPiedPy · 2018-01-26T21:32:13Z

Hi, I'm a Collaborator for the Trajectories mod and we have made changes to the API which will be included in the soon to be released v2.2.0

https://github.com/neuoy/KSPTrajectories/blob/master/Plugin/API.cs

I thought it would be wise to provide you with a pull-request, with changes needed to help stop any problems arising in future releases of kOS, when used with Trajectories.

Changes for v2.0.0 are:

Changes to Trajectories addon classes to support the v2.0.0 release of Trajectories.
New API function added HasTarget.

Changes for v2.2.0 are:

Changes to Trajectories addon classes to support the v2.2.0 release of Trajectories.
New API functions added TimeTillImpact.
For the Descent Profile Prograde and Retrograde.
And some for version checking GetVersion IsVerTwo IsVerTwoTwo.

There are also some changes that you need to make regarding Trajectories that are not correct in the kOS Documentation:

SetTarget can be used regardless of a predicted trajectory calculated or not and thus a check for HasImpact is not required but an active vessel check is still required.
PlannedVector and CorrectedVector require a valid target to be set before calling otherwise they will throw an exception, the Trajectories:API methods PlannedDirection and CorrectedDirection will return a null in such cases.

Changes to Trajectories addon classes to support the v2.0.0 release of Trajectories. Added the new Trajectories:API method HasTarget.

PiezPiedPy · 2018-01-26T21:39:02Z

Issue #2214 should also be fixed with the Trajectories v2.0.0 release

Dunbaratu · 2018-01-28T18:52:05Z

Will these proposed changes only work with Trajectories v2.0.0, or will they also work with the current Trajectories release? (i.e. if we merge this PR, does that mean we should wait for Trajectories v2.0.0 before we let the public see this PR?)

PiezPiedPy · 2018-02-15T07:27:30Z

@Dunbaratu
Sorry I have not got back to you sooner, GitHub did not notify me of your post. 😠

The changes in this PR will only work with Trajectories v2 which I'm going to release later Today.

Also regrettably Trajectories v2 will not work with the current release of kOS due to the API method name changes that where made 😞

parkerbossier · 2018-03-22T03:14:55Z

I can confirm that Trajectories is working. I've got this PR locally merged into master 06ef52a, and I correctly see ADDONS:TR:AVAILABLE, ADDONS:TR:HASIMPACT, and ADDONS:TR:IMPACTPOS.

Dunbaratu · 2018-03-30T06:42:09Z

src/kOS/AddOns/Trajectories/TRWrapper.cs

-            trGetImpactPosition = trajectoriesAPIType.GetMethod("getImpactPosition");
+            trGetImpactPosition = trajectoriesAPIType.GetMethod("GetImpactPosition");
            if (trGetImpactPosition == null)
            {


I'd like to attempt to make this work regardless of which version of Trajectories is installed.
Would it be a bad thing if I changed this to look for both spellings and use whichever one it finds?
That will make life easier for me later if I choose to make a back-port for RO compatibility.

Sounds like a good idea, only bad thing although not a big issue is anyone using an older Trajectories version will not gain the error checking in the newer API.
With the new method HasTarget you could just return a message stating something like, 'This function only available in Trajectories v2.x.x'
On a side note I've also created a branch of Trajectoriesv2.x.x for KSP1.3 (not built a release yet but tests have been positive so a release is imminent).

Thinking about it I am going to add a new method to the Trajectories API which returns its version number.

I can make the changes later today if you wish and add them to this PR.

I don't think older users will get the error checking, but at least it would still work as well as it did before before the error checking was present, i.e. using new kOS with old Trajectories wouldn't break. By the way I was looking at this myself overnight and was going to send a PR back to you on top of your PR.

I'll wait till you send your PR. 😉
you could use the new API method to check the version if you wish, I've called it GetVersion returns a string i.e. 2.1.0
Also now going to add GetVersionMajor , GetVersionMinor and GetVersionPatch that return an integer value.
Obviously a check for a version earlier than v2.x.x will be just to find if the Trajectories addon is available and if GetVersion exists.
Commit will be later today on Trajectories/master I'll let you know when I commit so you can have a look at the code.

I had a few questions about the API but it's probably faster to PR back to you what I have and let you handle it than keep asking questions and doing it myself. Let me make the PR. It's not big.

So HasTarget didn't exist in older Trajectories versions? That answers one of the questions I had (did you just happen to add the suffix to kOS it because it was forgotten before, or because it truly was new).

HasTarget is new to v2.x.x and does not exist in the older versions.

Is your branch "Trajectories-v2.0.0-API-update" based on an older version of our develop? When I try to PR back to you I see a very big list of diffs, far more than the few I made. It makes sense why it would be (I let this PR sit for a bit). But I think you'd be best off first updating your branch to the most recent develop before looking at my PR to you, that way you'll only see the relevant diffs I just made.

The PR to your branch has been made.

Thanks, will have a look soon 😉

…te' into trajectories_both_versions

This probably still needs more changes - giving it back to PiezPiedPy.

…0.0-API-update

…versions

Trajectories both versions

PiezPiedPy · 2018-03-31T01:46:31Z

@Dunbaratu Hi
Can you tell me what return type I need to use for a Double with suffixes, I've used DoubleValue but it seems it is not defined, I tried to look but got lost lol.
AddSuffix("TIMETILLIMPACT", new Suffix<DoubleValue>(TimeTillImpact, "Remaining time until Impact in seconds."));

hvacengi · 2018-03-31T02:49:07Z

@PiezPiedPy ScalarValue.Create(val) to create any instance of a scalar. Internally we treat doubles and scalars differently, but they identical to users, therefore the constructor is protected.

Dunbaratu · 2018-03-31T05:58:46Z

@PiezPiedPy, basically in all these sorts of AddSuffix lines in the code, suffixes that return doubles or ints should be implemented as new Suffix<ScalarValue>(...) rather than <ScalarDoubleValue> or <ScalarIntValue>. Even when you know for sure that it will definitely be a double or definitely be an int, still package it as the generic type "ScalarValue" for the sake of the script reading it.

PiezPiedPy · 2018-04-01T21:33:18Z

Is there any way to simulate a C# property { get; set; } with a single Suffix rather than having to define two Suffixes.

This is what I have done for the new API property ProgradeEntry { get; set; }
get returns true, false or null if no active vessel
AddSuffix("ISPROGRADE", new Suffix<BooleanValue>(IsPrograde, "Check whether the descent profile is set to Prograde."));

set is allways true, does not need an input arg
AddSuffix("SETPROGRADE", new NoArgsVoidSuffix(SetPrograde, "Sets the descent profile to Prograde."));

Also done the same for RetrogradeEntry

* Changes to Trajectories addon classes to support the v2.2.0 release of Trajectories. * New API functions added `TimeTillImpact`. * For the Descent Profile `IsPrograde` `IsRetrograde` `SetPrograde` `SetRetrograde`. * And some for version checking `GetVersion` `IsVerTwo` `IsVerTwoTwo`.

PiezPiedPy · 2018-04-02T00:00:58Z

I'm all done here, also added what was needed for the v2.2.0 API additions.

New API functions added TimeTillImpact.
For the Descent Profile IsPrograde IsRetrograde SetPrograde SetRetrograde.
And some for version checking GetVersion IsVerTwo IsVerTwoTwo.

I also checked that all versions of Trajectories will work with this merge. 😉
There will be a release of v2.2.0 for KSP 1.4.2 and 1.3.1 very soon.

https://github.com/neuoy/KSPTrajectories/blob/master/Plugin/API.cs if you need/want to have a nose about.

Dunbaratu · 2018-04-02T10:24:49Z

@PiezPiedPy - Look for places in our code where the phrase "new SetSuffix" appears. That should show you what you want, a suffix that can both get and set at the same time. (We don't have set-only suffixes, so the type SetSuffix presumes the suffix is also a getter even though the name of the type doesn't phrase it that way.)

The constructor typically looks like this:

SetSuffix( getter delegate, setter delegate, "text description")

(There's also a ClampSetSuffix where it will clamp the user provided value to a range before the setter sees it.)

Would you like to use this to combine IsPrograde and SetPrograde together into one suffix? It sounds like this was what you wanted but couldn't find the way to do it. (Same for IsRetrograde and SetRetrograde.)

On another note, this does make me feel a lack in kerboscript is the lack of an enum type. Because obviously you don't want both retrograde and prograde to be true at the same time, so having two independent Booleans for them seems wrong, but I admit there isn't really a good way around it without adding enums to kerboscript.

PiezPiedPy · 2018-04-02T13:07:57Z

@Dunbaratu Thanks for the help, I will have a play and see what happens, hopefully with the info I can successfully combine IsPrograde and SetPrograde into one suffix. (Same with IsRetrograde and SetRetrograde.)

`IsPrograde` and `SetPrograde` are now `Prograde`. `IsRetrograde` and `SetRetrograde` are now `Retrograde`.

PiezPiedPy · 2018-04-02T13:59:14Z

Finally done, you can merge whenever you feel the urge 😉

Dunbaratu

I will be adding the user documentation changes to go with this before I merge it. I don't want to ask you to go learning the doc subsystem too, so I'll do that part on my own before I merge it.
(i.e. I'll be editing the file doc/source/addons/Trajectories.rst and adding that edit to my branch before I merge this to develop.)

To help manage the user docs and keep them updated, I made this rule a couple of years ago: All merges into develop must combine both the code changes and user doc changes together so they form a single atomic merge. In other words, in develop, the *.cs files and the *.rst files are not allowed to temporarily disagree with each other.

This went a long way to cleaning up the docs. The docs used to have a lot of disagreement with reality back when we were manually updating them just before each release rather than editing them piecemeal along with the code changes.

Dunbaratu · 2018-04-04T05:09:59Z

src/kOS/AddOns/Trajectories/TRWrapper.cs

+        public static bool IsVerTwo { get; private set; } = false;
+
+        public static bool IsVerTwoTwo { get; private set; } = false;
+


The Monodevelop that ships with Unity 2017.1.3 (the Unity version KSP 1.4.2 is officially built with) doesn't understand this C# syntax (having an initializer for a property). It's stuck on a slightly older version of the C# language spec from before this was a thing. To make it work, I'll edit these lines to have the property backed by a field, and initialize the field.

Making sure things compile with the older C# supported by the Monodevelop that Unity comes with is important to me because that's the only option people developing on Linux have at the moment. Eventually, Unity says they will move to a system where they use VSCode and the commandline compiler of Mono as the official Linux developer solution, but in the current version KSP is using, that hasn't happened yet.

Dunbaratu · 2018-04-04T05:12:48Z

src/kOS/AddOns/Trajectories/TRWrapper.cs

+                IsVerTwo = true;
+                IsVerTwoTwo = true;
+                SafeHouse.Logger.Log("Checking Trajectories version: API.GetVersion returned version: v" + GetVersion);
+            }


This looks like a good way to solve the version check problem where there wasn't a version API beforehand.

Dunbaratu · 2018-04-04T08:00:13Z

@PiezPiedPy - As I was writing the docs for this I realized there seems to be a lack of an important feature in the API:

Once a script calls Addons:Trajectories:SETTARGET() I can't figure out how it could un-set it again.

I was just going to add a suffix myself to this PR that does it. But when I looked at the Trajectories code on Github, I can't find anything in Trajectories.API that would null out the Trajectories.Target.Body or Trajectories.Target.WorldPosition, which is I think how you unset the target from what I can see.

Of course, I could bypass Trajectoreis.API and just call Trajectories.Target.Set() directly, but that feels like it would defeat the point of having the API class.

Dunbaratu · 2018-04-04T09:25:19Z

There's a new PR to your branch you can look at. Sorry, for constantly throwing this back - I swear I'm ready to merge it now, if you approve of the edits I did to the user docs and verify that I didn't say anything wrong in them.

PR review changes.

PiezPiedPy · 2018-04-04T14:31:08Z

@Dunbaratu Checked your PR and the docs look fine so I've merged your PR.

Re: The clearing of the Target, well spotted I missed that one 🙄. I will add it onto my To-do list and include it when I code the methods for setting the Descent Profile via the API, I wont be including them in this PR though but I will create a new one when the time comes.

Dunbaratu · 2018-04-06T04:52:42Z

@PiezPiedPy - There's a few things to add to maybe the next update to the API, but they shouldn't prevent the merging of this PR, so I made a separate issue for them, #2263.
If you are planning to add them to your TODO list, you can track them at that issue.

Dunbaratu · 2018-04-13T23:59:35Z

Also fixes #2254 which is a duplicate issue.

Changes to support Trajectories v2.0.0 API

364b956

Changes to Trajectories addon classes to support the v2.0.0 release of Trajectories. Added the new Trajectories:API method HasTarget.

PiezPiedPy mentioned this pull request Jan 26, 2018

ADDONS:TR:PLANNEDVEC is broken #2214

Closed

This was referenced Feb 24, 2018

KOS Cannot use Trajectories. neuoy/KSPTrajectories#131

Closed

kOS and any mod that uses the Trajectories v2.0.0 to v2.2.0 API neuoy/KSPTrajectories#133

Open

hvacengi added this to the v1.1.6.0 milestone Mar 10, 2018

Dunbaratu reviewed Mar 30, 2018

View reviewed changes

Dunbaratu and others added 6 commits March 30, 2018 06:18

Merge remote-tracking branch 'PiezPiedPy/Trajectories-v2.0.0-API-upda…

c609030

…te' into trajectories_both_versions

Started work for both old and new Trajectories.

f2b35a7

This probably still needs more changes - giving it back to PiezPiedPy.

Merge remote-tracking branch 'upstream/develop' into Trajectories-v2.…

8b15dd6

…0.0-API-update

Merge branch 'Trajectories-v2.0.0-API-update' into trajectories_both_…

a49d3dd

…versions

Merge pull request #1 from Dunbaratu/trajectories_both_versions

37fa1e6

Trajectories both versions

.gitignore: Visual Studio user specific files.

e5b5340

PiezPiedPy changed the title ~~Changes to support the Trajectories v2.0.0 API~~ Changes to support the Trajectories v2.2.0 API Apr 2, 2018

Combined suffixes IsPrograde and SetPrograde same with Retrograde.

4a23047

`IsPrograde` and `SetPrograde` are now `Prograde`. `IsRetrograde` and `SetRetrograde` are now `Retrograde`.

PiezPiedPy changed the title ~~Changes to support the Trajectories v2.2.0 API~~ Changes to support the Trajectories v2.0.0 to v2.2.0 API Apr 2, 2018

PiezPiedPy mentioned this pull request Apr 2, 2018

[Implemented] Make GUI attitude settings settable in the API neuoy/KSPTrajectories#82

Closed

Dunbaratu reviewed Apr 4, 2018

View reviewed changes

Dunbaratu added 3 commits April 4, 2018 04:02

PR review changes.

ebb6a72

Added some version clarifications to user docs.

eb6a7f3

Missed a spot where version needed in the docs

e314151

Merge pull request #2 from Dunbaratu/trajectories_merge

f98ba72

PR review changes.

Dunbaratu mentioned this pull request Apr 6, 2018

A few missing features for Trajectories Addon #2263

Closed

Dunbaratu merged commit f98ba72 into KSP-KOS:develop Apr 6, 2018

PiezPiedPy deleted the Trajectories-v2.0.0-API-update branch April 13, 2018 21:09

Dunbaratu mentioned this pull request Apr 14, 2018

Trajectories wrapper doesn't work with 2.0.0 #2254

Closed

PiezPiedPy mentioned this pull request Jul 22, 2018

ADDONS:TR:AVAILABLE returns false even when TR addon is installed #2303

Closed

PiezPiedPy mentioned this pull request Sep 30, 2018

Added API for kRPC neuoy/KSPTrajectories#153

Closed

PiezPiedPy mentioned this pull request Dec 14, 2018

Trajectories detection not working? #2379

Closed

		public static bool IsVerTwo { get; private set; } = false;

		public static bool IsVerTwoTwo { get; private set; } = false;

Conversation

PiezPiedPy commented Jan 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PiezPiedPy commented Jan 26, 2018

Uh oh!

Dunbaratu commented Jan 28, 2018

Uh oh!

PiezPiedPy commented Feb 15, 2018

Uh oh!

parkerbossier commented Mar 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dunbaratu Mar 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PiezPiedPy commented Mar 31, 2018

Uh oh!

hvacengi commented Mar 31, 2018

Uh oh!

Dunbaratu commented Mar 31, 2018

Uh oh!

PiezPiedPy commented Apr 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PiezPiedPy commented Apr 2, 2018

Uh oh!

Dunbaratu commented Apr 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PiezPiedPy commented Apr 2, 2018

Uh oh!

PiezPiedPy commented Apr 2, 2018

Uh oh!

Dunbaratu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dunbaratu commented Apr 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Dunbaratu commented Apr 4, 2018

Uh oh!

PiezPiedPy commented Apr 4, 2018

Uh oh!

Dunbaratu commented Apr 6, 2018

Uh oh!

Dunbaratu commented Apr 13, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

PiezPiedPy commented Jan 26, 2018 •

edited

Loading

parkerbossier commented Mar 22, 2018 •

edited

Loading

Dunbaratu Mar 30, 2018 •

edited

Loading

PiezPiedPy commented Apr 1, 2018 •

edited

Loading

Dunbaratu commented Apr 2, 2018 •

edited

Loading

Dunbaratu commented Apr 4, 2018 •

edited

Loading