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 RealFuels support #4

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add RealFuels support #4

wants to merge 2 commits into from

Conversation

krissi
Copy link

@krissi krissi commented Jun 1, 2017

No description provided.

@blackliner
Copy link
Owner

Thanks for the PR. I am not using RF, did you test this and nwhat does it do? Why mass * 1000?

Copy link
Owner

@blackliner blackliner left a comment

Choose a reason for hiding this comment

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

It would still be missing the RealBattery module for the tanks, a SC only tank would make no sense, since SC can't flow to another part. How will the RealBattery module be added?

@Starwaster
Copy link

I replied to your post on the KSP forums but I'll also leave this here so you have a record as part of this PR:

Real Fuels lets the player control what resources are assigned to a 'tank' part. (tank can be a loose term; in this case it would be like a battery)

The thing that determines what a given tank can hold is the TANK_DEFINITION. The patch in that pull request lets any tank that could have electric charge (basically adding batteries to the part) also add StoredCharge.

As you note in the PR comments, by itself it seems to do nothing. The player could configure tank parts with StoredCharge but if, as you say, your mod / resource definition won't allow for StoredCharge to actually flow into the part then it's probably pretty useless. Nothing else in there adds your mods modules to the part. (the kinds of parts that this would affect in RF would be plane fuselages, service modules and electric propulsion tank parts (Xenon tanks that also contain batteries for ion engines)

About the mass / utilization... that looks really screwy because the end result is that one StoredCharge would require a 'tank' mass (really battery mass) of 2.89 metric tons due to the math involved. Mass is the tank mass for one volume unit and utilization is how volume unit of the resource will fit in one tank volume unit... EC had a utilization of 1000 and mass of 0.00289 so... yeah, it's going to end up as 2.89 tons per volume unit of SC.... I am not familiar with your mod and what kind of power density your batteries (capacitors?) have so I could be wrong but that seems screwy to me.

@krissi
Copy link
Author

krissi commented Sep 7, 2017

I somewhat messed up the testing, not sure what happened there. I just added the missing module but did not fix the incorrect mass for now.
For reference: the 10kg RealBattery stores 18.0 EC and 1.8 StoredCharge.

Unfortunately it seems that there is more to to than just taping the parts together. RealBattery uses the part mass for calculations (RealBattery.cs#L344) and there is a null-ref when no StoredCharge is present (RealBattery.cs#L437). The presence of StoredCharge should also be involved when displaying the battery stats and in the "LoadMaster"-mechanism.

Thats a bigger change than I thought, unfortunately I do neither have the time nor the development environment at the moment. So feel free to close this PR or leave it open for the future and/or someone else.

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