UmbraSpaceIndustries / MKS

MKS/OKS Colonization Systems
Other
221 stars 151 forks source link

FSfuelSwitch module in ranger type warehouse bug when using small (<1) volumes. #1031

Open paul23-git opened 7 years ago

paul23-git commented 7 years ago

While writing a mod that combines TAC&USI I was working on the containers, specifically the Ranger_ISM.cfg file. I noticed a peculiar bug: while not game breaking and irrelevant for the main workings it prevents modding, and more importantly: it is part of probably quite a large issue.

The part is capable of expanding after launch: at which point it holds 100 times as many resources. However this isn't the case for a part with different settings, ie setting the "Mulch" amount to 0.2 expanding it gives it suddenly 2000 mulch (factor 10000) - worse: collapsing again reduces it only to 20(!). So it seems it isn't using a simple multiplication?

during construction: image

While deployed: image

And retracted again: image

jd284 commented 7 years ago

It's not FSFuelSwitch, it's the USI_Animation module that causes this, for any resource with an inflated capacity that's smaller than the inflation multiplier, i.e. a deflated capacity less than 1 (see lines 480 and 502 of USIAnimation.cs). To make it work properly that check would need to be moved ahead of the loop, or at least make it set a flag. Then it'd work as long as a resource with larger capacity comes first.

paul23-git commented 7 years ago

That's silly. Looking over the code the line: "if (res.maxAmount < inflatedMultiplier)" seems "wrong": the line seems to indicate that it checks "if we are actually inflated or not". However it does so by checking resource amounts vs a mulitplier (as you said). Any "fix" like you suggested above (just doing it for the first resource instead of every), has unwanted side effects. The solution is actually really simple: Add an "bool isInflated" property to the class, and use that. About 3 lines of code I think? (Oh might have to check the whole file for such tests).

jd284 commented 7 years ago

That would break all existing ships though. Unless you make a save converter...

paul23-git commented 7 years ago

Well even that isn't hard - given I have not written any mod for ksp but it just requires a simple thing:

on-save-load:

if (not_initialized(isInflated)) {
    if (res.maxAmount < inflatedMultiplier) {
        [KSPField] bool isInflated = false;
    } else {
        [KSPField] bool isInflated = true;
    } 
}

Given I do not know the exact syntax, but it would just require an event on save loading, and then checking if it already has the required isInflated in the save - if it doesn't it uses once the old-way of checking to make initialize the isInflated.

EDIT: isDeployed works exactly this way. And one could add an extra in the same way isDeployed works. (Reuse of isDeployed is not the recommended course of action, since one might foresee a module that can both be deployed and be inflated in the future).