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

Fix LR91 ECMs. #700

Merged
merged 1 commit into from Jun 23, 2017
Merged

Fix LR91 ECMs. #700

merged 1 commit into from Jun 23, 2017

Conversation

ahmedcharles
Copy link
Contributor

No description provided.

@rsparkyc rsparkyc requested a review from NathanKell June 19, 2017 14:49
@rsparkyc
Copy link
Member

@NathanKell I know you fixed some of these before, and you seem to be the expert on such things, want to take a look?

@NathanKell
Copy link
Member

So, the FASA large LR91 skips the first config, and the miniLR91 only has the first config (IIRC). That's why they use subtractor not the usual multiplier. What was the issue encountered?

@ahmedcharles
Copy link
Contributor Author

At a high level, the ECM configs don't mention the FASAGeminiLR91Mini at all. It seems to have the same stats as the FASAGeminiLR91, but I didn't look at the engine configs themselves. I assumed they were the same, so I figured they should work the same as other parts that are the same engine with different looks.

@NathanKell
Copy link
Member

The ECMs for it were written when the -3 was only on the Mini, and the -5 and above were only on the regular. THat's why (as you can see in the original file) the Mini counts as a subtractor for the others, but not vice versa.

If both the mini and the regular now have all the same config options, then they should be treated identically and there should be three ECMs (one per part), each of which has multiplier 0 for the other two copies.

@ahmedcharles
Copy link
Contributor Author

As far as I can tell, the Mini and the regular have the same engine configs, because they both have engine = LR91 and the configs are the same there. So, I assume that means that this change is correct?

@ahmedcharles
Copy link
Contributor Author

@NathanKell Does this look good?

@NathanKell
Copy link
Member

Yup!

@NathanKell NathanKell merged commit 74995e2 into KSP-RO:master Jun 23, 2017
NathanKell added a commit that referenced this pull request Jun 23, 2017
@ahmedcharles ahmedcharles deleted the lr91ecm branch June 24, 2017 20:16
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.

None yet

3 participants