OverRefunding on Stored Parts (I think)
Lisias opened this issue · 21 comments
Fellow Kerbonaut Krazy1 reported a bug on Forum.
Then when I recovered, it gave me over :funds:160K for a :funds:120K ship. Using latest version. Nothing on the ship was modded. I have TweakScale installed but everything was stock size. KSP 1.11.2.
Second test: big rocket in VAB::funds:425792 and recovered :funds:468823... which listed 96607 for Refunding. So that just does not add up at all. This rocket did have a few Tweaked parts.
(The first problem was fixed on commit eecabb1 )
S&D the damn bug.
Well, that's interesting...
I made this vessel, exactly like the one on the reporter's machine:
and cheated my funds to be barely enough to afford it:
By launching the thing, I got 760 Funds left on the wallet:
However, on recovering the craft, I got twice the intended refunding!!
The intended Refunding is 70 Funds, but I got 140 instead!!!
Craft file: Test OverRefunding.craft.zip
I think I know what's happening - the Recovering process is correctly recovering the funds of Stored parts!
Jesus Christ, how many ways of doing the same freaking thing were implemented on KSP? It's no surprise the code base is so terribly bugged, they implemented the same feature many times, each one on a different way - and once something changes, God knows how may different code that was doing the same thing need to be fixed - not to mention tested!
Theoretically, the solution is simple - once the part is stored inside an Inventory Part, the Refunding must be withdrawn, and once it is removed from it, reapplied.
But... Nothing is "simple" on KSP, we just don't know how much dirty is under this carpet...
A VERY NAÏVE work around was implemented on commit cb233e4
it didn't worked.
Whatever ir happening inside KSP, it's still broken. But on a new way, because the Refunding Resource IS BEING COUNTED TWICE.
I will shove the 0.1.0.4 release on the wild anyway. The hack it's harmless (perhaps it would be a fix for what appears FMRS over refunding on automatic recoveries? This can stick...). This issue remains open.
Implementing a messy hack over the messy hack - if by any means someone counts resources the right way, this hack will counter-act the Refunding using the iPartCostModifier.
Now I need to understand why in hell a Cargo Part stored on a Inventory Part is being counted TWICE on the refunding process. (not to mention why there's a refunding at first place, but I had an idea on this - parts loose resources when being stored, right?)
Found something interesting. Using a debug compile of Refunding, I realised that ModuleInventoryPart implemented IPartCostModifier, and it's returning a cost of 35Funds (look for CalculateModulesCost below):
[LOG 22:46:13.775] [KSP-Recall-Refunding] TRACE: Recalculate kerbalEVAfemale (Valentina Kerman):FFF9DD2C
[LOG 22:46:13.775] [KSP-Recall-Refunding] TRACE: CalculateModulesCost(kerbalEVAfemale,ModuleInventoryPart) => 35
[LOG 22:46:13.775] [KSP-Recall-Refunding] TRACE: CalculateModulesCost(kerbalEVAfemale,Refunding) => 0
[LOG 22:46:13.775] [KSP-Recall-Refunding] TRACE: Recalculate Results originalCost: 0.0; resourceCosts:0.0; wrongCost:0.0; rightCost:35.0; fix:35.0 ;
[LOG 22:46:13.775] [KSP-Recall-Refunding] TRACE: UpdateResource kerbalEVAfemale:FFF9DD2C
[LOG 22:46:13.775] [KSP-Recall-Refunding] TRACE: Test OverRefunding-Part:FFF9DD2C is not stackable
[LOG 22:46:13.776] [KSP-Recall-Refunding] TRACE: Before PartResource 0 1 1
[LOG 22:46:13.776] [KSP-Recall-Refunding] TRACE: After PartResource 35 0 1
[LOG 22:46:13.776] [KSP-Recall-Refunding] TRACE: Recalculate kerbalEVA (Lulan Kerman):FFF9D762
[LOG 22:46:13.776] [KSP-Recall-Refunding] TRACE: CalculateModulesCost(kerbalEVA,ModuleInventoryPart) => 35
[LOG 22:46:13.776] [KSP-Recall-Refunding] TRACE: CalculateModulesCost(kerbalEVA,Refunding) => 0
[LOG 22:46:13.776] [KSP-Recall-Refunding] TRACE: Recalculate Results originalCost: 0.0; resourceCosts:0.0; wrongCost:0.0; rightCost:35.0; fix:35.0 ;
[LOG 22:46:13.776] [KSP-Recall-Refunding] TRACE: UpdateResource kerbalEVA:FFF9D762
Since Refunding tacitly assume that the IPartCostModifier is always ignored, it adds that 35Funds into the costFix variable (that will be used to create the RefundingResource later). Since we have two Kerbals on the vessel, we have two Refundings of 35 Funds, ergo 70 Funds.
So we now have the explanation from where came that 70 Refunding.
Now we need to find from where the others 70 Funds came. Logic suggests that each part that the Kerbal's IPartCostModifier is being honored if and only if the Kerbal has a Resource on him. This would explain the 140 Funds on the recovery of the Vessel used for this test.
I'm an idiot. O plain forgot to execute the Test Case without KSP-Recall installed - I was working without a bencmark.... #facePalm
Well, so this is the very same test realised above, without Refunding.
HA!!!! It's yet another bug on KSP!!!!!
Resources on storage are being counted twice. That extra 70 is from KSP itself!
So Refunding need to detected these situations somehow, and DEFUND the value of the Resources to get the correct value back on recovering.
Rushing 0.1.0.6 for testings on the Field. If everything goes fine, I will push it into Mainstream (Curseforge and Spacedock)
Well, new debugging session. I took the same craft from the previous test, and added Mk0 Fuel tanks inside one of the SEQ:
And on recovering, I got only that extra 70 Funds of the last attempt, so the Mk0 was accounted correctly.
Craft: Test OverRefunding II.craft.zip
One less hypothetical problem to worry about.
Was reported on Forum a misbehaviour using the Weather Analyser.
No noticeable mishbehaviour. So the problem reported is 100% on Refunding.
The devil is on the details...
I realised something interesting - late night is not exactly the better time of the day for debugging...
I had assumed that the extra 70 Funds were coming from the EVA JetPack from the two Kerbonauts, but the true is that... WE DON'T KNOW from where this Funds is coming from.
Checking the test craft on a stock KSP, I got this:
So, let's munch some numbers:
So, it's not a surprise I was not being able to get rid of that extra 70 Funds, and ended up chasing ghosts on the ProtoPartSnapshot. This other bug on KSP is deep on the code on some hack did in the past, and not on counting Resources!!
I ran some more tests.
I launched the vehicle with only one Kerbal, and got a 35F over-refunding.
Then I added an extra command-chair, and launched it with 3 Kerbals, and got 105F over-refunding. Appears to be consistent.
Then I launched it again with only one Kerbal, but getting rid of the chute and of the JetPack. No over-refunding. Adding more Kerbals, as long they are without chutes or jetpacks gave no over-refunding neither.
Found a pattern.
Launching the craft with a single Kerbal with jetpack but without chute gave me back that nameless Resource with a cost of 25F, and a 25F over-refunding.
Launching the craft with a single Kerbal without jetpack but with chute gave me back that nameless Resource with a cost of 10F, and a 10F over-refunding.
Further testings confirmed the over-refunding being applied on a per Kerbal basis.
25F and 10F are the prices for the JetPack and Chute, respectively.
That nameless Resource is a marker with the price of the equipment on the Kerbal, and it's multiplied by the number of the Kerbals on the vessel. [not quite, but near...]
So I launched the craft with 3 Kerbals, one with chute only, other with JetPack, and the third without any equipment.
I got a refunding of 35F, what I expected. That weird nameless Resource happened only once, with a 10F price.
By adding anything else to a Kerbal, I got the total price of the itens over-refunded. By example, if I shove a Small Work Lamp and a EVA Fuel Cylinder to a Kerbal, I got a over-refund of 75F, the combined price of the two itens.
I finally understood. Squad added the ModuleInventoryPart stunt but didn't got rid of the older code that was handling Kerbal's inventory.
Oukey, one good new. The Over-Refunding only affects Kerbals on Command Seats, not on Crew Cabins. Meno male.
And I have it fixed. :)
Rework on Commit: d8626ae
[LOG 13:25:17.730] [KSP-Recall-Refunding] TRACE: Recalculate kerbalEVAfemale (Valentina Kerman):FFF9A7D2
[LOG 13:25:17.730] [KSP-Recall-Refunding] TRACE: This part is a Kerbal with ModuleInventoryPart. Deducting 75
[LOG 13:25:17.730] [KSP-Recall-Refunding] TRACE: Recalculate Results originalCost: 0.0; resourceCosts:0.0; wrongCost:0.0; rightCost:0.0; fix:-75.0 ;
[LOG 13:25:17.730] [KSP-Recall-Refunding] TRACE: UpdateResource kerbalEVAfemale:FFF9A7D2
[LOG 13:25:17.730] [KSP-Recall-Refunding] TRACE: Test OverRefunding IV-kerbalEVAfemale (Valentina Kerman):FFF9A7D2 is not stackable
[LOG 13:25:17.731] [KSP-Recall-Refunding] TRACE: Before PartResource 0 1 1
[LOG 13:25:17.731] [KSP-Recall-Refunding] TRACE: After PartResource -75 0 1
[LOG 13:25:17.731] [KSP-Recall-Refunding] TRACE: Recalculate kerbalEVA (Lulan Kerman):FFF9A1DE
[LOG 13:25:17.731] [KSP-Recall-Refunding] TRACE: This part is a Kerbal with ModuleInventoryPart. Deducting 0
[LOG 13:25:17.731] [KSP-Recall-Refunding] TRACE: Recalculate Results originalCost: 0.0; resourceCosts:0.0; wrongCost:0.0; rightCost:0.0; fix:0.0 ;
[LOG 13:25:17.731] [KSP-Recall-Refunding] TRACE: UpdateResource kerbalEVA:FFF9A1DE
[LOG 13:25:17.731] [KSP-Recall-Refunding] TRACE: Test OverRefunding IV-kerbalEVA (Lulan Kerman):FFF9A1DE is not stackable
[LOG 13:25:17.731] [KSP-Recall-Refunding] TRACE: Before PartResource 0 1 1
[LOG 13:25:17.731] [KSP-Recall-Refunding] TRACE: After PartResource 0 0 1
Note the entry [LOG 13:25:17.730] [KSP-Recall-Refunding] TRACE: This part is a Kerbal with ModuleInventoryPart. Deducting 75 :)
Oh, well... Almost perfect. :P
I have a rounding error or something stealing one Fund on a Test Craft. :D
Craft: Rounding Error Test I.craft.zip
I'm ran out of time again (and for a mile this time). I will leave things as is for now, unless someone finds something really nasty.
