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

Add TransferWindowPlanner - Fork #9049

Merged
merged 3 commits into from
Mar 27, 2022
Merged

Add TransferWindowPlanner - Fork #9049

merged 3 commits into from
Mar 27, 2022

Conversation

Nazfib
Copy link
Contributor

@Nazfib Nazfib commented Mar 27, 2022

I forked TWP to make it useful for transfers between planets that have non-zero inclination (by specifying which inclined parking orbit should be used). This is especially important in RSS, but also in stock it can reduce Δv requirements by several hundred m/s for some transfers.
In the process, I updated it for 1.12.

It would be great to have on CKAN for easier installation, but I want it clear to users that it is a fork and not the original mod by TriggerAu.

Relates to Nazfib/TransferWindowPlanner#1.


https://github.com/Nazfib/TransferWindowPlanner

I forked TWP to make it useful for transfers between planets that have non-zero inclination (by specifying which inclined parking orbit should be used). This is especially important in RSS, but also in stock it can reduce Δv requirements by several hundred m/s for some transfers. 
In the process, I updated it for 1.12.

It would be great to have on CKAN for easier installation, but I want it clear to users that it is a fork and not the original mod by TriggerAu.
@HebaruSan
Copy link
Member

HebaruSan commented Mar 27, 2022

Hi @Nazfib, thanks for the submission! I had a question and a suggestion regarding adopting this mod...

  • Have you considered submitting a pull request to have your changes integrated into the original fork? It looks like @TriggerAu isn't doing much active maintenance there anymore, but you could at least give him a chance to say "no" or not respond. (Or he might even invite you to officially take over maintenance, which would make it easy for users of the existing fork to find yours by simply upgrading in CKAN.)
  • If you're planning to support this fork for a while, it would be a good idea to start a thread on the KSP forum so your users can contact you in case they have bug reports or other requests. Otherwise the original fork's thread is likely to get confused by cross-traffic between the two forks, and the original author will get pinged with notifications.

Otherwise this looks fine, but I'll hold off on merging for now in case we want to make changes regarding the above points.

@Nazfib
Copy link
Contributor Author

Nazfib commented Mar 27, 2022

Hi,
@NathanKell tried to contact @TriggerAu on my behalf a while ago, but got no response. Since then, the changes are getting so large (a complete overhaul of the build system for one, a planned full overhaul of the internals for another), that PR'ing it upstream will probably not happen anymore.

As for a thread on the forums, I'm not active there, so I'm likely to not see any responses. This basically started when I saw many people having trouble with interplanetary transfers on the KSP-RO discord, and I knew how to change TWP to make it easier, and it all snowballed a bit from there... any possible support I could give will have to happen there or in the issues on GitHub I'm afraid.

@HebaruSan
Copy link
Member

HebaruSan commented Mar 27, 2022

Ahh, OK. What do you think about setting the resources.homepage property to the KSP-RO Discord, if that's the best way to reach you? We have a few mods set up like that already.

@HebaruSan
Copy link
Member

Looks good to me, thanks for the submission!

@HebaruSan HebaruSan merged commit f95570e into KSP-CKAN:master Mar 27, 2022
@TriggerAu
Copy link
Contributor

Hey both, You only have to reach out and I am around. I have been struggling with time between KSP2 and my other job, but have been trying to keep on top of things yeah.

Didn't notice any questions from Nathan, nor PRs against TWP except yalovs one that I need to get in. Did I miss a PR somewhere?

@TriggerAu
Copy link
Contributor

Kk, seeing the note about comms in the source patch, interesting I dont have a recent message from Nathan. I dont see any issues with the fork being diffn, etc, but also haven't seen any PR requests for this to the upstream repo yet. Ill have a dig around on the weekend and see if anything stands out.

@NathanKell
Copy link
Contributor

@TriggerAu That's super weird, The forum claims the PM I sent back in Feb was sent so I'm not sure what's going on. :(

As to PRs, I added this ( TriggerAu/TransferWindowPlanner#60 ) back in October when I PM'd you then (and you responded), but no action occurred and I didn't hear back when I responded to that thread in Feb, thus I figured you were gone (or KSP2'd) for good.

Glad to see you are around though! And sorry for the confusion.

@NathanKell
Copy link
Contributor

Here's the text of the forum PM (with your last reply from Oct) in case the forum ate it.

TriggerAu
TriggerAu 3,790
Replied: October 1, 2021

Cool. Ill check it out

Quote
NathanKell
NathanKell 5,968
Replied: February 26

Hi! Hope things are well with you!!

Just checking in--do you care if the RSS/RO optimized version of TWP gets put on CKAN? That will also give us a 1.10+ compatible version we can rec (for RO users) on CKAN, since your TWP is version-locked to 1.7. (It would be named to be clear it's a fork.)

And as for KAC--I updated my PR to support 1.12 as well. KAC is such a great mod, it'd be really nice if it was installable through CKAN again! :)

@TriggerAu
Copy link
Contributor

I got all me bits and bobs sorted, so v 1.8.0.0 shoudl update next time CKAN does a scan etc.

im not 100% how this is expected to show up in CKAN - can check tomorrow Im sure :) . Will it show up as "TWP" and "TWP-RO Fork" or its expected to have some mechanism whereby it only presents to people with RO installed or some such?

@NathanKell
Copy link
Contributor

NathanKell commented Apr 11, 2022 via email

@TriggerAu
Copy link
Contributor

Maybe it some detail in the description so its a it clear its not two copies, or whats diffn or something. People done good work to create the fork, people should use it if they need it more than just pick the default one

@HebaruSan
Copy link
Member

The new fork has "Fork" in the name and an extra author:

image

@Nazfib, you can differentiate the descriptions by editing this section of your repo on GitHub:

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants