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

Added Trajectories addon #1603

Closed
wants to merge 41 commits into from
Closed

Added Trajectories addon #1603

wants to merge 41 commits into from

Conversation

CalebJ2
Copy link
Contributor

@CalebJ2 CalebJ2 commented Apr 17, 2016

Fixes #687

Adds an addon that accesses the Trajectories mod via reflection. Provides the following suffixes:
addons:TR:available: True if the addon was able to access all the things it needs in the Trajectories mod.
addons:TR:hasImpact: True if Trajectories has found an impact position for the current vessel.
addons:TR:impactPos: GeoCoordinates of the impact prediction.

Notes & potential discussion points:

  • Works with the current Trajectories version (1.4.6). Future Trajectories updates may break this addon, in which case addons:TR:available will return false.
  • Only works on the "Active Vessel" because Trajectories is hard coded that way as far as I can tell.
  • In the documentation I link to one of my scripts on github as an example. Not sure if that's against a policy or something.
  • I haven't actually tested the documentation. Hopefully it works.

@tomekpiotrowski
Copy link
Member

Any chance Trajectories will provide some kind of an API in the future? Can we send a pull request? We use reflection sometimes if we have to, but obviously it's better to avoid it.

@CalebJ2
Copy link
Contributor Author

CalebJ2 commented Apr 18, 2016

I found this a while ago that said Trajectories used to have an API but they gave up on maintaining it, so I doubt they would really want to add it back.
Trajectories is on github so I guess we could send a pull request. I'll send Youen (Trajectories author) a message on the ksp forums and ask what he thinks.

…ontrol-C.

It was falsifying the notion of whether or not the system is at
global scope when it sees a "local" directive, because it
saw the leftover remains of the previous run on the stack.  That
error cascaded into other problems causing it to end up seeing
the old leftover version of the delegate that was still in the
global variable from last time.
@CalebJ2
Copy link
Contributor Author

CalebJ2 commented Apr 21, 2016

Youen said he would be fine with adding an API so I will work on that. I might wait until he updates Trajectories to 1.1.

@tomekpiotrowski
Copy link
Member

That's great :) Keep us posted.

@tomekpiotrowski
Copy link
Member

tomekpiotrowski commented May 8, 2016

I'd be willing to merge this if you 1) updated it and solved all the conflicts 2) tested it with KSP 1.1.2. 3) Added a note somewhere in the documentation that Trajectories has no official API and kOS support might break in future versions.

And added more access to Trajectories API
Turns on trajectories auto updating. Needed for (currently pending)
trajectories api to work.
@CalebJ2
Copy link
Contributor Author

CalebJ2 commented Jun 17, 2016

Ok so I submitted a pull request to Trajectories with an API a while ago and recently fixed an issue he had with it, so I'm expecting it to get merged fairly soon.

Hopefully I didn't mess things up with #1664 too much by adding another commit.

I don't think I'll be able to do more programming until the end of the summer. :( If someone else can see this through that would be great.

This needs to be updated to latest version of kOS, and the docs section saying there is no API (or something like that) needs to be removed.

@hvacengi
Copy link
Member

No worries, we'll make it work. It's on the list of things to make sure we get addressed shortly anyways.

@CalebJ2
Copy link
Contributor Author

CalebJ2 commented Sep 6, 2016

My API changes have been merged into Trajectories so it now has an official API.

This addon works with that API, but needs to be updated to the latest kOS version.

@hvacengi hvacengi self-assigned this Sep 20, 2016
@hvacengi hvacengi added this to the v1.0.1 milestone Sep 20, 2016
@hvacengi
Copy link
Member

Adding a reference to the API PR: neuoy/KSPTrajectories#67

@hvacengi
Copy link
Member

Found an issues in the Trajectories source (see neuoy/KSPTrajectories#74) so I'm going to put this on hold until I hear back about how to proceed.

I have a number of review changes that I've made to both the code and the documentation. These changes are currently stored in my local repository, rather than as comments to the PR mostly because I was just about ready to merge it with my edits. I'll hold off merging.

For the time being, I am planning on merging as a rebase, because it makes it easier for me to identify the files directly edited by this PR (as opposed to those edited by by other merged commits). This is a potential issue for repository history however, so I'm open to suggestions about alternative paths.

@hvacengi hvacengi added the Not Ready Author is warning Reveiwer not to merge this PR yet. More edits are expected. label Sep 20, 2016
@hvacengi hvacengi added Not Ready Author is warning Reveiwer not to merge this PR yet. More edits are expected. and removed Not Ready Author is warning Reveiwer not to merge this PR yet. More edits are expected. labels Oct 10, 2016
@hvacengi
Copy link
Member

While the issues have been fixed in the Trajectories repository, they are not yet incorporated into a release. I'm holding off on merging my review branch so that we don't release a new feature for which users need to know how to compile trajectories. I'll publish my working branch a little later tonight just so that anyone who finds this and wants to make it work can use the most recent version, include my edits to suffix names.

@PBscoots
Copy link

@hvacengi which branch of yours has your working branch changes? I can't seem to find it. I will download it and do a compile and test of both Trajectories and KOS for this addon. Hopefully neuoy makes a rev release soon so we can get this finished.

@hvacengi
Copy link
Member

@PBscoots I just pushed it to https://github.com/hvacengi/KOS/tree/pull-review/1603

Be warned however, there will probably be a rebase coming before I push, and I will definitely modify the history far enough to change my "WIP" commit into a legit commit.

@PBscoots
Copy link

@hvacengi am I able/(should I) to add commits to your pull-review/1603 to add in the orbit and impact velocity bits?

@PBscoots
Copy link

@hvacengi I am going to add some commits to your 1603 branch to add in those other parts. My commits on the trajectories side have been implemented and he released an update for 1.2. I have tested it with KOS and available, hasimpact, impactpos, correctedvec, plannedvec all work. Im not quite sure what to do with settarget to test it.
Anyway, maybe sometime in the next few days I will add my bits, test them, then commit to your branch. Then you can finish your rebase after that. If thats not how you think I should do it, let me know

@hvacengi hvacengi removed the Not Ready Author is warning Reveiwer not to merge this PR yet. More edits are expected. label Oct 26, 2016
@hvacengi
Copy link
Member

I merged this via rebase, and apparently didn't close the original PR. Closing now.

@hvacengi
Copy link
Member

hvacengi commented Nov 1, 2016

@PBscoots if you have other suffixes/features you would like to add, feel free to submit them as a pull request against the develop branch at this point

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.

Trajectories API integration
9 participants