search

net-lisias-ksp / KSP-Recall

Recall for KSP blunders, screw ups and borks.
GNU General Public License v2.0
25 stars 2 forks source link

Weird Misbehaviour on Interstellar Technologies. #60

Closed Lisias closed 1 year ago

Lisias commented 1 year ago

Fellow Kerbonaut GoAhead's Space Agency is being stollen by a bug somewhat related to refunding.

See:

Lisias commented 1 year ago

First things first: A minimal KSP test bed where the problem can be reproduced is:

MM used is my personal fork, and it's on the latest release. Recall, MMWD and TweakScale are the latest published releases.

CRP and WarpPlying are the ones from the KSPIE+1.29.6+for+KSP+1.8.1.zip file I downloaded from SpaceDock.

InterstellarTechonologies are from the Interstellar_Technologies_-_A_KSPI-E_Expansion-0.4.1.zip file I downloaded from the same place.

Lisias commented 1 year ago

Test 1: KSPIE is working for me.

  1. Screenshot of the craft on Editor, note the Funds. screenshot11

  2. Screenshot of the craft after being launched, note the Funds screenshot12

  3. Screenshot of the craft after being recovered, note the Funds. screenshot13

Craft File: rbug-kspie.craft.zip

Whole Savegame: bug.zip

Lisias commented 1 year ago

Now, on the very same test bed, on the very same savegame:

  1. Screenshot of the craft on Editor, note the Funds. screenshot11

  2. Screenshot of the craft after being launched, note the Funds screenshot12

  3. Screenshot of the craft after being recovered, note the Funds. screenshot13

And vóilà, problem reproduced!

Craft file: rbug.craft.zip

Whoe savegame: bug.zip

Lisias commented 1 year ago

HA! FOUND THE PROBLEM! :)

It's due using floats and doubles to handling currency!

I can't fix this, because KSP itself is using floats for this task, so I have my hands tied. What I can do, however, is to mitigate the problem by using decimal internally and, so, preventing Refunding to aggravate the problem.

https://forum.kerbalspaceprogram.com/index.php?/topic/192048-143/&do=findComment&comment=4241876

Lisias commented 1 year ago

This issue now is a task to rework Refunding to use decimals.

It was not considered a bug, because KSP itself is using floats and so there's nothing I can do to really fix the issue.

Lisias commented 1 year ago

Worked around on commit https://github.com/net-lisias-ksp/KSP-Recall/commit/cfd6ee72d502e4a0606dcfc47b95be3adcc545e4 .

Switching all currency computations to decimal (and saving the original cost as string just in case) didn't really solved the problem, because in the end KSP calls us using the IPartCostModifier where the result is squashed into a single - and, so, we can have again inaccuracies due the float roundings.

However, at least our own computations are now rounding free, and so Refunding is not piling up inaccuracies itself, so the ending result is better (or less worse), minimizing the loss on the recoveries.

Lisias commented 1 year ago

Well, it didn't worked as expected on one of the GoAhead's craft files.

https://forum.kerbalspaceprogram.com/index.php?/topic/192048-143/&do=findComment&comment=4242594

Reopening for reevaluation.

Lisias commented 1 year ago

Well… Before banging my head on the nearest wall, I need some confirmations first.

  1. KSP itself has, indeed, a float inaccuracy problem when dealing with currency?
  2. KSPIE itself also has it?
  3. Refunding is helping or screwing up on the problem?

So I decided to hack my way on my old and faithful KSP 1.4.3 test bed. This release is, essentially, one of the most stable KSP I ever played.

The next posts will tackle down my questions on the subject on it.

Lisias commented 1 year ago

KSP itself has, indeed, a float inaccuracy problem when dealing with currency?

TL;DR: YES.

On KSP 1.4.3, I built a test bed as follows:

savegame: bug-float-cost.zip

[CommunityResourcePack]
[CommunityTechTree]
[ModuleManager]
[ModuleManagerWatchDog]
[Squad]
[SquadExpansion]
[TweakScale]
[WarpPlugin]
    [Localization]
    [Parts]
        [Electrical]
            [MuonCatFusionReactor2]
    [Patches]
    [ResourceConfigs]
    [Resources]
[net.lisias.ksp]
    [VesselMover]
000_Hyperspace.dll
000_KSPe
000_KSPe.dll
666_ModuleManagerWatchDog.dll
999_KSP-Recall
999_Scale_Redist.dll
ModuleManager.dll

It's essentially my standard test bed setup, plus some directories from KSPIE+1.29.6+for+KSP+1.8.1.zip. Note that WarpPlugin was mercilessly butchered - I'm tailoring it to fit on a minimal installation on a pretty deprecated KSP version.

These were the results:

  1. On VAB, open test -1 craft and launch it. Note the funds. screenshot92
  2. After launching, check the funds again:3. screenshot93
  3. Recover the thing, check the funds again.4. screenshot94 screenshot95

We lost 145 Funds. And this is on KSP 1.4.3, where almost none of the bugs KSP-Recall aim to work around happens and, so, no Refunding stunt to mess things up.

So, yea, Stock KSP is screwing up recovering Funds if the parts are expensive (or cheap) enough due float inaccuracies.

Lisias commented 1 year ago

KSPIE itself also has it?

Yes. Checking the source code for WarpPlugin, I found:

  1. InertialConfinementReactor extends InterstellarInertialConfinementReactor.
  2. InterstellarInertialConfinementReactor extends InterstellarFusionReactor
  3. InterstellarFusionReactor extends InterstellarReactor
  4. InterstellarReactor implements IPartCostModifier.
  5. IPartCostModifier uses float on its method GetModuleCost, that are realized on the InterstellarReactor class.

Checking the class atributes mentioned on this realization, almost all of them are doubles what makes things slightly less worse - but since the resulting value is still squashed into a single, we have the inaccuracy problem for sure.

Now I need to know if if affects the sample craft, so I reproduced the test rig on KSP 1.8.1, the minimum KSP WarpPlugin's Assembly works with the benefit of having way less bugs than newer KSPs (Refunding is not used on it neither). I reproduced the same test bed used on KSP 1.4.3, but this time I included the DLLs on the Plugins folder:

[CommunityResourcePack]
[CommunityTechTree]
[ModuleManager]
[ModuleManagerWatchDog]
[Squad]
[SquadExpansion]
[TweakScale]
[WarpPlugin]
    [Parts]
        [Electrical]
            [MuonCatFusionReactor2]
    <All the other directories and files>
[net.lisias.ksp]
    [VesselMover]
000_Hyperspace.dll
000_KSPe
000_KSPe.dll
666_ModuleManagerWatchDog.dll
999_KSP-Recall
999_Scale_Redist.dll
Interstellar_Redist.dll
ModuleManager.dll

Exactly the same thing from the previous test.

savegame: bug-float-cost.zip

  1. On VAB, open test -1 craft and launch it. Note the funds. screenshot72
  2. After launching, check the funds again. screenshot73
  3. Recover the thing, check the funds again. screenshot74 screenshot75

Again, we have some inaccuracies - we lost 779 Funds this time.

Curiously, the thing is somewhat cheaper on KSP 1.8.1, but I impute that to the absence of the WarpPlugin's PartModules on KSP 1.4.3.

Lisias commented 1 year ago

Refunding is helping or screwing up on the problem?

TL;DR: It's helping. Perhaps a bit too much. :)

Now I reproduced the very same test bed on 1.11.2, the first KSP where Refunding started to be needed. I'm using the current latest, 0.3.0.11.

savegame: bug-float-cost.zip

  1. On VAB, open test -1 craft and launch it. Note the funds. screenshot26
  2. After launching, check the funds again. screenshot27
  3. Recover the thing, check the funds again. screenshot28 screenshot29

Now we have again a small error, but this time in user's favor. 61 extra Funds.

well, the stunt I pulled on 0.3.0.11 worked for sure! :)

Lisias commented 1 year ago

Now I'm waiting for GoAhed to send me his savegame, so I can have a real world problem to use as benchmark and check if a workaround I'm cooking will be effective.

https://forum.kerbalspaceprogram.com/index.php?/topic/192048-143/&do=findComment&comment=4242807

Lisias commented 1 year ago

Dude sent me the savegame, and I was surprised on realising that KSP is handling Funds (more or less) correctly. The Funding class is using double.

Why in HELL IPartCostModifier is using float is beyound me.

Lisias commented 1 year ago

Oukey, apparently KSP is, indeed, doing its currency computations on double and so the root problem for this issue is IPartCostModifier still using float.

This make at least a nasty workaround feasible.

Lisias commented 1 year ago

Well, this is why the original hack didn't worked for GoAhead: some parts once scaled end up absurdly expensive, and these parts end up being squashed on IPartCostModifier (due the absurd conversion to float).

With many parts being squashed this way, the losses pile up - and they are significant, to the point to derail the savegame.

This cannot be properly fixed without a new IPartPreciseCostModifier interface where GetModuleCost would handle double instead (I prefer decimal, by the way).

So, no matter what we do, we will have inaccuracies . POINT. It's just impossible to represent the correct value using float.

What we can do is to choose if such inaccuracies will harm or benefit the user. Since the harmful inaccuracy effectively prevent the user from recovering its costs from a launch, the less worst solution is to err to their benefit, i.e., since I can't give the dude the right amount do the rounding, I will refund them the next ceiling if the rounded value, instead of the floor.

The user will be over-refunded with this stunt, so yeah… Kinda of cheating. But it's the best I can do from a PartModule.

This is implementend by commits https://github.com/net-lisias-ksp/KSP-Recall/commit/d18a8e77bfa5b5c1fad4de70eb8b2d5273a53756 and https://github.com/net-lisias-ksp/KSP-Recall/commit/4fc9471ca3175ca9ebee078b8c8f27100e1ccbdc .

Lisias commented 1 year ago

The hack I implement is simple: I applied a Gauss-Jacobi to find the next float capable value of the ideal refunding by adding the difference between the ideal and the "effective" amounts until the effective amount is no less then the ideal.

Dirty, but effective, trick.

Lisias commented 1 year ago

Proof of the fix. Using the very same test bed I specified in the last test session.

savegame: bug-float-cost.zip

We are handling with absurd values here - many times the Earth's PIB! :) Any discrepancies would not be noticed by the U.I. (except by the recovery dialog, if you know the exact amount on the wallet before launching).

  1. Pre launch screenshot30
  2. Lanched screenshot31
  3. Recovering screenshot32 screenshot33 screenshot34

Finally, on the persistent.sfs file:

Before the launch:

        SCENARIO
        {
                name = Funding
                scene = 7, 8, 5, 6
                funds = 120000000000000
        }

After the recovery:

        SCENARIO
        {
                name = Funding
                scene = 7, 8, 5, 6
                funds = 1200000391017787
        }

So, yeah, we have some serious over-refunding here, 39,1017,787 to be exact - about 39.1B Funds. But given the huge values involved on this absurdity called test-4.craft, I think it's an acceptable.. "unloss".

Obviously, affected way cheaper parts will have way less over-redunding - it's the whole meaning of the Gauss-Jacobi stunt I applied above.

Lisias commented 1 year ago

As a final note: a better solution is possible, but not relying only on a PartModule.

Each Refunding module can save in an attribute the diff between the ideal and the effective cost, and then someone handling a GameEvents.OnVesselRecoveryRequested could run over all the Refundind modules of a craft and add back these diffs directly into the Agency's Wallet using the double capable Funding class.

However, this would make the process less auditable, less transparent, and it will add yet another source for problems - and I already had my share with Refunding.

Unless someone gets annoyed by the current solution, I plan to call a day on this.

Lisias commented 1 year ago

Oh! I almost forgot!

An entry is added on the KSP.log when the issue is detected and worked around:

[LOG 18:44:34.889] [KSP-Recall.Refunding] WARNING: Your refunding was squashed by `IPartCostModifier` and was mangled to prevent losses ( see https://github.com/net-lisias-ksp/KSP-Recall/issues/60 ). Ideal value:146476434579000.000000 ; hack used instead:1.464765E+14
Lisias commented 1 year ago

Humm… That logging is insufficient. Commit https://github.com/net-lisias-ksp/KSP-Recall/commit/a15ec59f80300805f3ad134f523afacd60bbda76 works it.

[LOG 19:59:36.553] [KSP-Recall.Refunding] WARNING: Your refunding for test -4-XSMainHullReactor:FFF5463C was squashed by `IPartCostModifier` and was mangled to prevent losses ( see https://github.com/net-lisias-ksp/KSP-Recall/issues/60 ). Ideal value:178488510000.0000000000000 ; hack used instead:1.784885E+11

Much better.

Lisias commented 1 year ago

For the sake of completude, I just confirmed that parts those cost doesn't get screwed when converted into float are not affected by this stunt (no warnings being logged for then, so no differences between the effective and ideal costs).

Lisias commented 1 year ago

I just couldn't hold myself. :)

screenshot30

Lisias commented 1 year ago

Note to my future self: I misdiagnosed a mistake on the code for #62 and ended up thinking I had a regression on this issue and so changed the milestone before reopening it.

I was wrong, no need to rework it, so I restored the original milestone.

Lisias commented 1 year ago

I was wrong on being wrong! :D

KSPIE parts scaled up all-right, it's when scaling them down the problem happens!!

We didn't detected this misbehaviour before because by default, KSPIE don't scale down things.

KSPIE Extended, however, adds patches to scaling things down and so the problem is triggered!!!

Lisias commented 1 year ago

I'm suspecting this is not something to be tacked down on KSP-Recall. I think I will need to write a Companion for KSPIE to handle this situation.

Note to myself: reach FreeThinker and talk about moving all TweakScale support to a Companion, freeing him from the hassle. Right now, I'm probably the only crazy-arse crazy enough to do such a job…. :P

Lisias commented 1 year ago

Oukey, I need more focusing and less multitasking.

There were never a problem on KSPIE, it was a pretty stupid mistake of mine on a very basic code. (sigh).

The problem I had misdiagnosed was fixed on commit https://github.com/net-lisias-ksp/KSP-Recall/commit/ee11eabff8495ee9fe1dab5f51e8656dda1982f8