Weird Misbehaviour on Interstellar Technologies.

Question

Weird Misbehaviour on Interstellar Technologies.

Lisias opened this issue 2 years ago · 26 comments

Lisias commented 2 years ago

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

See:

Lisias · Answer 1 · 2023-02-24T23:02:24.000Z

Test 1: KSPIE is working for me.

Screenshot of the craft on Editor, note the Funds.
Screenshot of the craft after being launched, note the Funds
Screenshot of the craft after being recovered, note the Funds.

Craft File: rbug-kspie.craft.zip

Whole Savegame: bug.zip

Lisias · Answer 2 · 2023-02-24T23:06:34.000Z

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

Screenshot of the craft on Editor, note the Funds.
Screenshot of the craft after being launched, note the Funds
Screenshot of the craft after being recovered, note the Funds.

And vóilà, problem reproduced!

Craft file:
rbug.craft.zip

Whoe savegame: bug.zip

Lisias · Answer 3 · 2023-02-24T22:45:00.000Z

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

[000_KSPe]
[CommunityResourcePack]
[InterstellarTechnologies]
[ModuleManager]
[ModuleManagerWatchDog]
[Squad]
[SquadExpansion]
[TweakScale]
[WarpPlugin]
000_KSPe.dll
666_ModuleManagerWatchDog.dll
999_KSP-Recall
999_Scale_Redist.dll
Interstellar_Redist.dll
ModuleManager.dll

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 · Answer 4 · 2023-02-25T17:36:33.000Z

Worked around on commit cfd6ee7 .

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 · Answer 5 · 2023-02-25T11:52:14.000Z

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 · Answer 6 · 2023-02-25T11:53:14.000Z

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 · Answer 7 · 2023-02-25T21:03:45.000Z

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 · Answer 8 · 2023-02-25T23:40:49.000Z

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:

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

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 · Answer 9 · 2023-02-25T23:30:58.000Z

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

KSP itself has, indeed, a float inaccuracy problem when dealing with currency?
KSPIE itself also has it?
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 · Answer 10 · 2023-02-26T01:04:51.000Z

KSPIE itself also has it?

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

InertialConfinementReactor extends InterstellarInertialConfinementReactor.
InterstellarInertialConfinementReactor extends InterstellarFusionReactor
InterstellarFusionReactor extends InterstellarReactor
InterstellarReactor implements IPartCostModifier.
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

On VAB, open test -1 craft and launch it. Note the funds.
After launching, check the funds again.
Recover the thing, check the funds again.

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 · Answer 11 · 2023-02-26T01:32:44.000Z

`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

On VAB, open test -1 craft and launch it. Note the funds.
After launching, check the funds again.
Recover the thing, check the funds again.

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 · Answer 12 · 2023-02-26T03:00:00.000Z

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 · Answer 13 · 2023-02-26T15:46:42.000Z

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 · Answer 14 · 2023-02-26T21:06:31.000Z

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 · Answer 15 · 2023-02-26T21:28:43.000Z

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 d18a8e7 and 4fc9471 .

Lisias · Answer 16 · 2023-02-26T21:32:42.000Z

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 · Answer 17 · 2023-02-26T21:59:35.000Z

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 · Answer 18 · 2023-02-26T21:53:47.000Z

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).

Pre launch
Lanched
Recovering

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 · Answer 19 · 2023-02-26T22:05:14.000Z

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 · Answer 20 · 2023-02-26T23:00:34.000Z

Humm… That logging is insufficient. Commit a15ec59 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 · Answer 21 · 2023-02-26T23:07:31.000Z

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 · Answer 22 · 2023-02-27T02:34:55.000Z

I just couldn't hold myself. :)

Lisias · Answer 23 · 2023-03-25T05:09:17.000Z

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 · Answer 24 · 2023-03-25T18:58:53.000Z

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 · Answer 25 · 2023-03-25T19:00:51.000Z

~~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 · Answer 26 · 2023-03-25T22:59:48.000Z

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 ee11eab

Share to

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

KSPIE itself also has it?

Refunding is helping or screwing up on the problem?

`Refunding` is helping or screwing up on the problem?