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 HG-3 and HG-3-SL #1609

Merged
merged 36 commits into from
Jul 14, 2017
Merged

Add HG-3 and HG-3-SL #1609

merged 36 commits into from
Jul 14, 2017

Conversation

Ash19256
Copy link
Contributor

Adding two historical engines that IRL were stepping stones between the J-2 and the RS-25 aka Space Shuttle Main Engine.

Copy link
Contributor

@raidernick raidernick left a comment

Choose a reason for hiding this comment

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

when doing a +part you must use BEFORE not FOR

@@ -167,6 +167,107 @@
}
engineType = J2
}
// HG-3
+PART[SXTLT80]:FOR[RealismOverhaul]
Copy link
Contributor

Choose a reason for hiding this comment

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

should be changed to +PART[SXTLT80]:BEFORE[RealismOverhaul]

engineType = HG3
}
// HG-3-SL
+PART[SXTLT80]:FOR[RealismOverhaul]
Copy link
Contributor

Choose a reason for hiding this comment

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

change to +PART[SXTLT80]:BEFORE[RealismOverhaul]

Incorporated raidernick's change requests.
@Ash19256
Copy link
Contributor Author

Changes made

@PhineasFreak
Copy link
Contributor

BTW, the TweakScale stuff for the engine parts is not required anymore, as we have a global patch to take care of that: https://github.com/KSP-RO/RealismOverhaul/blob/master/GameData/RealismOverhaul/RO_Engines.cfg#L17-L25

@Ash19256
Copy link
Contributor Author

Acknowledged, removed it to ensure there was no interference with the global patch.

@name=HG-3
%RSSROConfig = True
{
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Module was deleted in 90d1cb6 but you forgot the brackets

@name=HG-3-SL
%RSSROConfig = True
{
}
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

Cleaning up the changed modules.
@Ash19256
Copy link
Contributor Author

Thank you for pointing that out.

Realized that considering the shared heritage of the J-2 and HG-3, it didn't make sense that the HG-3 wouldn't be able to throttle as deeply.
@Ash19256
Copy link
Contributor Author

Well, now that I know that engine configs are a thing, would it be a good idea for me to add the HG-3 and HG-3-SL to the J-2 series engine configs? The HG-3 and HG-3-SL were derived from the J-2, and have higher performance.

@NathanKell
Copy link
Member

NathanKell commented Apr 27, 2017 via email

@Ash19256
Copy link
Contributor Author

Created HG-3 engine and HG-3 Config.

name = ModuleEngineConfigs
type = ModuleEngines
configuration = J-2
@configuration:NEEDS[RP-0] = J-2-200klbf
Copy link
Member

Choose a reason for hiding this comment

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

configuration needs changing to HG-3

and the @configuration:NEEDS line can probably go

@SirKeplan
Copy link
Member

Would it make sense to give the upper stage variant extra ignitions? considering it was considered for use on the Saturn MLV's modified S-IVB stage.

Correcting the configuration and removing a nonsensical requirement.
Add temporary HG-3 plume effect.
@Ash19256
Copy link
Contributor Author

Alright, so that should fix the Test Flight thing. Also, added a temporary Real Plume config courtesy of Cosmonaut Crash on YouTube. I'll just get this all compiled and off to him for more testing prior to an official merging of this pull request.

@Ash19256
Copy link
Contributor Author

@PhineasFreak is this good now? It has a plume config (albeit not necessarily the most fitting one for the vacuum variant), and the TestFlight stuff should be working.

@PhineasFreak
Copy link
Contributor

PhineasFreak commented May 28, 2017

@Ash19256 the ModuleEnginesRF fields are not needed anymore (they are patched automatically) but otherwise it is good!

Removing unneeded ModuleEnginesRF patches.
@Ash19256
Copy link
Contributor Author

Ash19256 commented Jun 5, 2017

Fixed that. Looks like this is ready for merging otherwise.

@NathanKell
Copy link
Member

Does this already config the SHIP version? https://github.com/Saabstory88/RO_Extended

@Ash19256
Copy link
Contributor Author

Ash19256 commented Jul 7, 2017

Uh.. no. I'll get on that right away.

Copy link
Contributor

@PhineasFreak PhineasFreak left a comment

Choose a reason for hiding this comment

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

You could migrate these two to be part of the global engine config. That way you won't have to worry about copying and pasting these lines between different parts.

@Ash19256
Copy link
Contributor Author

Ash19256 commented Jul 7, 2017

Right, that should add SHIP compatibility to this pull request. I'll see what I can do about testing it, but otherwise it should be fine.
EDIT: @PhineasFreak that's pretty much what I did, actually.

@PhineasFreak
Copy link
Contributor

PhineasFreak commented Jul 7, 2017

@Ash19256 disregard my initial comment, it did not work as i expected to. I was talking about these two lines:

https://github.com/Ash19256/RealismOverhaul/blob/42907f86c9535468f44278523bf085c789f373d5/GameData/RealismOverhaul/RO_SuggestedMods/SHIP/Engines.cfg#L18-L19

@Ash19256
Copy link
Contributor Author

Ash19256 commented Jul 7, 2017

Oh, okay. I wasn't sure which lines you were talking about. Those are still in the SHIP/Engines.cfg file.

@PhineasFreak
Copy link
Contributor

Yea, it was a bit of a brainfart...

@Ash19256
Copy link
Contributor Author

Ash19256 commented Jul 10, 2017

Out of curiosity, is there a tutorial or something for making a RP-0 PR for something like this? Is it something that I should only do after this PR gets approved/merged or what? Also, is there a How To guide type thing someone could link me to if it's okay to make a pull request to add this into the RP-0 tech tree at this time?

EDIT: Or is the fact that RP-0 flags them as "Non RP-0" a fail on my part, because the configs are missing something that they should have?

Realized that because the SHIP HG-3 parts are already existing parts, the @name field is probably rather irrelevant.
@NathanKell
Copy link
Member

RP-0 needs to be made aware, yeah--best bet is to talk to @pap1723 about how to integrate them into the new tree, since the methodology has changed significantly from the current release build's way.

@Ash19256
Copy link
Contributor Author

So should I just wait for @pap1723 to make an appearance?

@pap1723
Copy link
Contributor

pap1723 commented Jul 10, 2017

I have appeared!

@Ash19256 What that means is that the parts were not priced and place on the Tech Tree. So RP-0 will automatically flag them as Non-RP0. Do you have real-world pricing on these parts?

@Ash19256
Copy link
Contributor Author

@pap1723 Unfortunately no, I've only been able to find 3 sources, none of which list a unit cost. Considering the HG-3 and it's seal level variant were never actually built IRL (it was, as far as the sources I've found have indicated, an on-paper project only, with no prototypes built), that's not really surprising. Considering the time period and other engines available at the time, I'd imagine that it'd be probably somewhere in the same price range (probably around 5-10% more expensive) than the J-2S, which my sources indicate it's a contemporary of. Take into account the higher chamber pressure making manufacturing more expensive, and making it cost more than the roughly similarly sized J-2S makes sense to me. But ultimately, the price would be a best guesstimate, simply because no IRL price for the engines can be found.

@NathanKell
Copy link
Member

If memory serves the HG-3 was, like, halfway between J-2 and RS-25*, so I'd suggest a price somewhere between them rather than so close to J-2, maybe?

*RS-25 was developed using a lot of the techniques used for HG-3, IIRC.

@Ash19256
Copy link
Contributor Author

Ash19256 commented Jul 13, 2017

Yeah, that sounds vaguely right. Although, based on it's capabilities, I'd lean more towards the J-2 side of things than the RS-25 side of things, mostly because the engine only produces around 27% more thrust for the same fuel consumption. That being said, it's ultimately up to @pap1723 and the others, so I'll go along with whatever they decide the price should be. I'll just need to know what to change in the configs to make them not get flagged as non RP-0.

Credit to Cosmonaut Crash on YouTube for these updates.
@Ash19256
Copy link
Contributor Author

Right, with this recent update this PR should be compatible with the SHIP mod. All that's needed now is to figure out RP-0 compatibility.

@NathanKell
Copy link
Member

Great! RP-0 stuff is done on RP-0 side so that's all set! :)

@NathanKell NathanKell merged commit 0b6b9cb into KSP-RO:master Jul 14, 2017
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

9 participants