Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

KSP 1.11.x is borking the recovering of costs from scaled parts. #12

Closed
Lisias opened this issue Mar 1, 2021 · 30 comments
Closed

KSP 1.11.x is borking the recovering of costs from scaled parts. #12

Lisias opened this issue Mar 1, 2021 · 30 comments
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@Lisias
Copy link
Contributor

Lisias commented Mar 1, 2021

Fellow Kerbonaut firethorn6 have a misbehaviour on his rig - as from KSP 1.11.x, the refunding at vessels recover is borked!

Evidencest:
screenshot7

A MK1 Pod and a Flea scaled to 2.8M (as anything bigger would not be allowed to launch on a early career). 2848 Funds.

I launched and immediately recovered it, to get the full refund:

  • 588
  • 1304
  • 12
  • 944

And this totalled 2848 Funds. So it's working, at least on KSP 1.4.5.

So I redid the test on 1.7.3:
screenshot11
screenshot12

And it works too. Right now I cleared something wrong on TweakScale, now I need to check if something changed on KSP itself, perhaps on 1.8.1?
screenshot61

Nops. Worked fine. Just for the sake of completude, I did the test on KSP 1.10.1:
screenshot63

And again fine.

Well, gone to KSP 1.11.1 (lots of thing changed on this one, perhaps?)
screenshot55

Bingo. It's something on KSP 1.11.1. :( Interesting enough, the cost of the vessel is now 2883 (probably due the new Inventory system)

@Lisias Lisias self-assigned this Mar 1, 2021
@Lisias
Copy link
Contributor Author

Lisias commented Mar 1, 2021

The cost of the part is borked, not the cost of the resources.

I really hope Squad didn't decided to fetch the values from the PREFAB instead of asking the living (soon to be dead) part's module for the info. This will break not only TweakScale, but any Add'On that change the cost from the part.

It may be a change on the life cycle of the part's module too - this will be troublesome, but at least is fixable. I think.

@Lisias
Copy link
Contributor Author

Lisias commented Mar 1, 2021

Tried to change the IPartCostModifier.GetModuleCostChangeWhen from FIXED to

       ModifierChangeWhen IPartCostModifier.GetModuleCostChangeWhen()
       {
           return ModifierChangeWhen.CONSTANTLY;
       }

No dice. :(

@Lisias
Copy link
Contributor Author

Lisias commented Mar 1, 2021

Well, this is the log from KSP 1.10.1 when recovering the craft:

[LOG 17:58:10.588] [Untitled Space Craft]: ground contact! - error. Moving Vessel  down -0.232m
[LOG 17:58:10.589] Unpacking Untitled Space Craft
[LOG 17:58:10.686] [UIMasterController]: ShowUI
[LOG 17:58:23.884] [TweakScale] TRACE: IPartCostModifier.GetModuleCost mk1pod.v2:FFFA49B4 IsScaled? False
[LOG 17:58:23.884] [TweakScale] TRACE: IPartCostModifier.GetModuleCost mk1pod.v2:FFFA49B4 0
[LOG 17:58:23.885] [TweakScale] TRACE: OnSave mk1pod.v2:FFFA49B4
[LOG 17:58:23.885] [TweakScale] TRACE: IPartCostModifier.GetModuleCost solidBooster.sm.v2:FFFA4956 IsScaled? True
[LOG 17:58:23.885] [TweakScale] TRACE: Get Module Cost solidBooster.sm.v2:FFFA4956
[LOG 17:58:23.886] [TweakScale] TRACE: Module Cost without resources solidBooster.sm.v2:FFFA4956 1103.77331542969
[LOG 17:58:23.886] [TweakScale] TRACE: Module Cost *WITH* resources solidBooster.sm.v2:FFFA4956 2047.88506541407
[LOG 17:58:23.886] [TweakScale] TRACE: IPartCostModifier.GetModuleCost solidBooster.sm.v2:FFFA4956 2047.885
[LOG 17:58:23.887] [TweakScale] TRACE: OnSave solidBooster.sm.v2:FFFA4956
[LOG 17:58:23.887] Flight State Captured
[LOG 17:58:23.888] Saving Achievements Tree...
[LOG 17:58:23.889] [MessageSystem] Save Messages
[LOG 17:58:23.898] Game State Saved to saves/career_TS/persistent
[LOG 17:58:23.903] [UIMasterController]: HideUI
[LOG 17:58:23.919] [HighLogic]: =========================== Scene Change : From FLIGHT to SPACECENTER (Async) =====================
[LOG 17:58:24.442] KbApp.OnDestroy Planet Parameters
[LOG 17:58:24.444] [UIApp] OnDestroy: KSPedia
[LOG 17:58:24.471] [UIApp] OnDestroy: ActionGroupsApp
[LOG 17:58:24.471] KbApp.OnDestroy Planet Info
[LOG 17:58:24.473] [TweakScale] TRACE: OnDestroy mk1pod.v2:FFFA49B4
[LOG 17:58:24.473] [TweakScale] TRACE: VariantPartScaler.Destroy mk1pod.v2:FFFA49B4
[LOG 17:58:24.478] KbApp.OnDestroy NullName
[LOG 17:58:24.478] KbApp.OnDestroy Vessel Info
[LOG 17:58:24.479] [UIApp] OnDestroy: ResourceDisplay
[LOG 17:58:24.480] [UIApp] OnDestroy: Contracts
[LOG 17:58:24.480] [UIApp] OnDestroy: DeltaVApp
[LOG 17:58:24.480] [UIApp] OnDestroy: CurrencyWidgetsApp
[LOG 17:58:24.480] KbApp.OnDestroy Planet Resources
[LOG 17:58:24.483] [TweakScale] TRACE: OnDestroy solidBooster.sm.v2:FFFA4956
[LOG 17:58:24.484] [TweakScale] TRACE: VariantPartScaler.Destroy solidBooster.sm.v2:FFFA4956

And this is the same code running on KSP 1.11.1 (1.11.0 also behaves like this):

[LOG 18:15:28.315] [Untitled Space Craft]: ground contact! - error. Moving Vessel  down -0.232m
[LOG 18:15:28.316] Unpacking Untitled Space Craft
[LOG 18:15:28.419] [UIMasterController]: ShowUI
[LOG 18:15:31.813] [TweakScale] TRACE: IPartCostModifier.GetModuleCost mk1pod.v2:FFF96AE0 IsScaled? False
[LOG 18:15:31.813] [TweakScale] TRACE: IPartCostModifier.GetModuleCost mk1pod.v2:FFF96AE0 0
[LOG 18:15:31.813] [TweakScale] TRACE: OnSave mk1pod.v2:FFF96AE0
[LOG 18:15:31.813] [TweakScale] TRACE: IPartCostModifier.GetModuleCost solidBooster.sm.v2:FFF96A82 IsScaled? True
[LOG 18:15:31.813] [TweakScale] TRACE: Get Module Cost solidBooster.sm.v2:FFF96A82
[LOG 18:15:31.813] [TweakScale] TRACE: Module Cost without resources solidBooster.sm.v2:FFF96A82 1103.77355957031
[LOG 18:15:31.813] [TweakScale] TRACE: Module Cost *WITH* resources solidBooster.sm.v2:FFF96A82 2047.8854117409
[LOG 18:15:31.813] [TweakScale] TRACE: IPartCostModifier.GetModuleCost solidBooster.sm.v2:FFF96A82 2047.885
[LOG 18:15:31.814] [TweakScale] TRACE: OnSave solidBooster.sm.v2:FFF96A82
[LOG 18:15:31.814] Flight State Captured
[LOG 18:15:31.814] Saving Achievements Tree...
[LOG 18:15:31.815] [MessageSystem] Save Messages
[LOG 18:15:31.826] Game State Saved to saves/career_TS/persistent
[LOG 18:15:31.831] [UIMasterController]: HideUI
[LOG 18:15:31.842] [HighLogic]: =========================== Scene Change : From FLIGHT to SPACECENTER (Async) =====================
[LOG 18:15:32.324] [UIApp] OnDestroy: KSPedia
[LOG 18:15:32.324] ScaleModList: listSize 41 maxListSize 264
[LOG 18:15:32.326] [UIApp] OnDestroy: ActionGroupsApp
[LOG 18:15:32.326] ScaleModList: listSize 41 maxListSize 264
[LOG 18:15:32.327] KbApp.OnDestroy NullName
[LOG 18:15:32.327] [TweakScale] TRACE: OnDestroy solidBooster.sm.v2:FFF96A82
[LOG 18:15:32.327] [TweakScale] TRACE: VariantPartScaler.Destroy solidBooster.sm.v2:FFF96A82
[LOG 18:15:32.329] KbApp.OnDestroy Planet Resources
[LOG 18:15:32.329] KbApp.OnDestroy Unowned Info
[LOG 18:15:32.330] KbApp.OnDestroy Vessel Crew

As we can see, the CostModifier if being reported correctly on both KSP versions, but on KSP 1.11.1, the value is not being used. My guess is that the PREFAB values are being used now for the total cost of the part, and when subtracting the value of the resources, we have a negative value!!

@Lisias
Copy link
Contributor Author

Lisias commented Mar 1, 2021

This is the recovery funds from an unscaled Flea:

screenshot57

The recovery funds for the part it's 116F, while for the solid fuel is 84F. What confirms the total cost of 200F.

Now, 200 - 944 (the refund of the fuel on the scaled Fleat), we have -744F - exactly the value being "recovered" by KSP 1.11.1.

So, TweakScale is toast. It's another hardcoded change inside the KSP guts, and I don't see how to fix this without reverse engineering - breaking Forum Rules

@DarthPointer
Copy link

Hi @Lisias!

I guess I'm also a victim of this shit happening. So I will have to anounce 1.11 being uncompatible.

Could you contact me if you find a solution?

@Lisias
Copy link
Contributor Author

Lisias commented Mar 2, 2021

Hi @Lisias!

Hi!

I guess I'm also a victim of this shit happening. So I will have to anounce 1.11 being uncompatible.

So do I. :/

Could you contact me if you find a solution?

Yes. Keep an eye on this issue, I'm working on the problem.

However, this is not a problem on TweakScale - to tell you the true, this is not even a problem because there's no solution from our side, Add'On developers. The fact is that some dev literally rendered the interface IPartCostModifier useless by ignoring it and using the prefab total cost instead - and there's no way out of the mess other than rewriting the KSP guts doing this stunt.

What I'm trying to do is to cook a workaround on KSP Recall that would try to "cheat" back the lost fundings on Vessel Recovery. It will be a hell of a ugly hack, but theoretically it will work - and it will work for everybody, not only for TweakScale.

My problem now is managing to make this hack to work - there're some key values I need to mangle on the living craft that, if by bad luck Squad is also squashing them from the prefab, will render the workaround impossible to implement.

On this case, I will have no other option as to reverse engineer KSP and apply a hotpatch using Harmony - and this will render the trick not publishable on Forum (due some draconic rules on modding)

@DarthPointer
Copy link

DarthPointer commented Mar 2, 2021

However, this is not a problem on TweakScale - to tell you the true, this is not even a problem because there's no solution from our side, Add'On developers. The fact is that some dev literally rendered the interface IPartCostModifier useless by ignoring it and using the prefab total cost instead - and there's no way out of the mess other than rewriting the KSP guts doing this stunt.

I use this interface on my own. And going to test it with 1.11 rn before I accuse SqUaD of breaking the things in my mod's thread.

I guess this cheating is pretty possible if you remember current balance and can access the vessel being stll an actual Vessel when the button is hit. However I do not like the idea of attempting to override recovery funds in a f'ing engine reliability&maintenace mod, taken that my module returns extra cost depending on baseCost and what other cost modifiers say.

It took me pretty much time to make the thing work for 1.8-1.10.

Upd: taken that, should you make the fix as a separate mod so we only add a dependecy and dont get bothered with problems that can occur when multiple mods are trying to fix the problem?

@Lisias
Copy link
Contributor Author

Lisias commented Mar 2, 2021

I use this interface on my own. And going to test it with 1.11 rn before I accuse SqUaD of breaking the things in my mod's thread.

"Test, don't trust!" - it's the way to go. By all means, prove me wrong - I will be happy, believe me! :)

I guess this cheating is pretty possible if you remember current balance and can access the vessel being stll an actual Vessel when the button is hit. However I do not like the idea of attempting to override recovery funds in a f'ing engine reliability&maintenace mod, taken that my module returns extra cost depending on baseCost and what other cost modifiers say.

Don't work. Squad squashed the recovery funds by using the prefab data, nothing you could do on the Module will have any effect.

I'm trying a (Forum rules compliant) workaround on this.

Upd: taken that, should you make the fix as a separate mod so we only add a dependecy and dont get bothered with problems that can occur when multiple mods are trying to fix the problem?

Yes. This issue will be moved to KSP-Recall, I will fix this there - as the fix will work for everybody. I'm keeping this here to avoid confusion, as I already spammed this link everywhere.

@Lisias
Copy link
Contributor Author

Lisias commented Mar 2, 2021

News from the front:

the "mechanism" is working perfectly:

[LOG 13:43:25.706] [KSP-Recall-Refunding] TRACE: Recalculate mk1pod.v2:FFFA3FA4
[LOG 13:43:25.706] [KSP-Recall-Refunding] TRACE: Recalculate Results originalCost: 612.0; resourceCosts:12.0; wrongCost:600.0; rightCost:600.0; fix:0.0 ;
[LOG 13:43:25.706] [KSP-Recall-Refunding] TRACE: Recalculate solidBooster.sm.v2:FFFA37E8
[LOG 13:43:25.706] [KSP-Recall-Refunding] TRACE: Recalculate Results originalCost: 284.0; resourceCosts:944.1; wrongCost:-660.1; rightCost:1387.8; fix:2047.9 ;

But the prefab squashing had bit me again in another place. Now trying a workaround to make this workaround to work...

@Lisias
Copy link
Contributor Author

Lisias commented Mar 2, 2021

And going to test it with 1.11 rn before I accuse SqUaD of breaking the things in my mod's thread.

In time. This is not an accusation. It's the declaration of a fact. :)

I had tested this issue on KSP 1.4.5, 1.5.1, 1.6.1, 1.7.3, 1.8.1, 1.9.1, 1.10.1 and finally 1.11.0 and 1.11.1 . Only the 1.11.x presented the problem, with all the others elements (Machine, OS Version, TweakScale and ModuleManager) being exactly the same.

So the only change between the tests are the KSP version being tested. And since there's only code from Squad on KSP, the logical conclusion is evident. ;)

@DarthPointer
Copy link

"Test, don't trust!" - it's the way to go. By all means, prove me wrong - I will be happy, believe me! :)

Then I have bad news.

Or good, taken that they proove your model.

Issue has been successfully reproduced for PayToPlay mod, KSP 1.11.1, recovery to KCT storage. My balance has changed.
Same actions but with KSP 1.10.1 do not reproduce this.

@Lisias
Copy link
Contributor Author

Lisias commented Mar 2, 2021

News From the Front:

I found ANOTHER FSCKIG BUG on this mess.

The way I tried to recover the funds was by adding a new Resource, called unsurprisingly Refunding.

RESOURCE_DEFINITION
{
	name = RefundingForKSP111x
	displayName = Refunding
	abbreviation = rF
	density = 0
	unitCost = 1
	hsp = 0
	flowMode = NO_FLOW
	transfer = NONE
	isTweakable = false
	RESOURCE_DRAIN_DEFINITION
	{
		isDrainable = false
		showDrainFX = false
	}
}

And then I created a module to control it.

@PART[*]:NEEDS[KSPRECALL-REFUNDING]
{
	%MODULE[Refunding]
	{
		active = True
	}

	%RESOURCE[RefundingForKSP111x]
	{
		amount = 0
		maxAmount = 999999
		isTweakable = false
		hideFlow = true
	}
}

Problem. Even by stating that the default amount of the Resource IS ZERO, the freaking game is calculating the cost of the Resource by using the maxAmount, DAMNIT.

screenshot58

This COMPLETELY SCREWED UP the whole cost system - you need to add the costs of all the resources to the Part's price, even when it's empty. Thinking a bit about it, I should not had been caught with my pants down on this, as I had to deal with this too once on TweakScale.

Well... I still have two options to try to make this work this way. It will add more ugliness to an already ugly kludge, but I want to keep my interventions on the game engine at minimum.

@DarthPointer
Copy link

DarthPointer commented Mar 2, 2021

@Lisias this one is not really a bug. Or is. Don't know, it is not 1.11-specific at least.

For example, if you MM-patch a regular fuel tank part to have more or less fuel, its full cost is not affected.

This means we can actually reduce recovered part cost posting some empty resource into it. If negative resource costs apply "correctly" then we will be able to increase the cost too.

@Lisias
Copy link
Contributor Author

Lisias commented Mar 2, 2021

@Lisias this one is not really a bug. Or is. Don't know, it is not 1.11-specific at least.

Agreed. It's more an anti-feature than a bug on a feature. :) Need to cool my head a bit.

This means we can actually reduce recovered part cost posting some empty resource into it. If negative resource costs apply "correctly" then we will be able to increase the cost too.

Don't work, as the prefab data is injected on the part on launch. You would need to create a new Part for each possible combination.

However... While bashing my head on the wall in frustration after trying to mangling the part's living data, I got an even crazier idea, currently under test.

Cross your fingers.

@Lisias
Copy link
Contributor Author

Lisias commented Mar 3, 2021

News from the Front

My last not so dirty trick failed.

My worst problem is not calculating the "cost fix" value, neither applying them. The problem is to avoid screwing up the part and vessel value on editor.

My initial attempt was to create a resource with 999.999 as maxAmount, density = 0 and unitCost = 1 - and then I tried to set the resource's amount with the cost fix (and then mangle the part with IPartCostModifier - that are honoured on the Editor - to keep things tight). It worked, but the part's cost on the Editor gone completely nuts (because everything became 999.999 Funds more expensive on the Editor - besides you being able to buy them at current price because that 999.999 was deducted due the zero Resource amount.

Oh well, at least I demonstrated that the stunt have teeth.

Then I tried a dirtier trick. I can't use Resource's amount and maxAmount due the Editor problem, but what about mangling the PartResourceDefinition's unitCost? Initially the unitCost would be set to 0 (with both Resource's amount and maxAmount set to 1), so the Editor would calculate the Resource's cost to zero, that subtracted from the part's cost is still the part's cost. Then, on launch, I would set the resource's info with a runtime, custom made PartResourceDefinition that is essentially a clone from the original, but with the unitCost set to 1.0.

It worked at runtime.

screenshot61
screenshot62

The Refunding "resource" is also scaled by TweakScale, so the unitCost is divided by the current maxAmount to compensate - on unscaled parts, maxAmount is 1 anyway.

But....

screenshot63

The prefab's Resource Definition ended up overruling my stunt once the craft is recovered - @$%@$$#!%!!!!! >:-/

I had run out of "not that dirty" options. My remaining option is to go nuclear - I will brute force my way out of the problem using Introspection. It's what TweakScale does anyway, but I was trying to avoid using this trick. TweakScale is already nosy enough, I didn't wanted a new nosy add'on screwing up other people's business. :D

@Lisias
Copy link
Contributor Author

Lisias commented Mar 3, 2021

Almost there! :)

screenshot64

I forgot to zero the Resource's amount, and so I ended up nullifying the costfix, i.e., I counter-counter-break the feature. :D

@Lisias
Copy link
Contributor Author

Lisias commented Mar 3, 2021

Things didn't worked exactly as I had planned.... :/

screenshot65

The trick worked as I intended, but I misjudged the resulting effect.

Some more reflection abuse is on the way....

@Lisias
Copy link
Contributor Author

Lisias commented Mar 3, 2021

Datum perficiemus munus

screenshot66

Now I will move this issue to KSP Recall, and will publish a Beta Release for public evaluation.

I don't trust this stunt yet - a lot of tests need to be done (by me and also on the field) in order to declare this kludge something safe.

@Lisias Lisias transferred this issue from TweakScale/TweakScale Mar 3, 2021
@Lisias
Copy link
Contributor Author

Lisias commented Mar 3, 2021

Slightly better.

screenshot67

The end result is still the same, but I further abused my way on it in order to prevent changing two fields on the Summary. Now the (improper) values are kept, making easier to track what I really did.

@Lisias Lisias added the enhancement New feature or request label Mar 3, 2021
@Lisias Lisias added this to the 0.0.7.0 milestone Mar 3, 2021
Lisias added a commit that referenced this issue Mar 3, 2021
@Lisias
Copy link
Contributor Author

Lisias commented Mar 3, 2021

How to check if the KSP Recall stunt is installed on your 1.11.x rig:

  1. Look for this Log Entry on the KSP.log:
[LOG 04:11:02.071] [KSP_Recall] Version 0.0.7.0 /L *BETA* DEBUG running on KSP 1.11.1
  1. Observe the Resouces Widget on flight, look for the Refunding one:
    screenshot71

  2. Look for the Refunding Resource when recovering the craft:
    screenshot72

Lisias added a commit that referenced this issue Mar 3, 2021
…behaviour for everybody, and not only for add'ons that works on Editor (and TweakScale).

#12
@Lisias
Copy link
Contributor Author

Lisias commented Mar 3, 2021

Some add'ons as PayToPlay are being ignored by Refunding. The reason is that Refunding, on its first prototype, updates itself on Launch and from there, it needs someone to "tell him" to update itself.

The solution worked fine with TweakScale (surprised? :P ) but it's cumbersome for add'ons that constantly update the part's cost, exactly the case for PayToPlay.

A better solution is to monitor when the craft is being recovered and update itself on that time, what guarantee the most updated data to be "refunded".

This is a testrun for PayToPlay, where depreciation is being applied to the Flea engine for being used:
screenshot73

The U.I. will be still "weird", with the +- on front of the Refunding "resource", but it will do as is. This is (hopefully) a temporary hack, after all.

Fix implemented on commit 06255bb

@Lisias
Copy link
Contributor Author

Lisias commented Mar 3, 2021

A new release is on the wild.

https://github.com/net-lisias-ksp/KSP-Recall/releases/tag/PRE-RELEASE%2F0.0.7.1

I will keep this Issue opened while this stunt is on tests.

@DarthPointer
Copy link

It is time I tell you I have lied. P2P does not update cost constantly, it actually changes cost max once during flight (to nullify cost). However there is no way to detect it outside P2P. And I guess at this point the fix covers most the cases if not all.

@Lisias
Copy link
Contributor Author

Lisias commented Mar 4, 2021

Fellow Kerbonaut firethorn6 reported a new misbehaviour, at this moment not known to be related to Refunding or not:

By bulding a craft using a stayputnik and a thoroughbred scaled to 10 meters, we are being charged above the craft cost on launching!

The stunt costs 576.300F, and on the screenshot below I have 600kF on my wallet:
screenshot73

The refunding stunt itself is working fine, the full costs of the vessel is being recovered:
screenshot74

But note on the screenshot above that my wallet is now 576.300F, the price of the craft - what means I had no money left!!!

The price being charged on launch is bigger than the craft's price - on this case, I paid 23700F more for it.

So I redid the test, but with way more money than needed, so see exactly how much I'm being charged for this:

screenshot76

And my wallet, after launch, came to... WHAT A HELL??? Zero Funds again?

screenshot77

Oukey, I need to reassess this situation...

@Lisias
Copy link
Contributor Author

Lisias commented Mar 4, 2021

New test. A way cheaper craft, 5M thoroughbred costing to launch 72.300F, and with 1M on the wallet.

screenshot78

After launch, my wallet gone down to 864.699F:
screenshot79

So the launch cost me 1.000.000 - 864.699 = 135.301F !!!!

Hummm.... I had an idea...

@Lisias
Copy link
Contributor Author

Lisias commented Mar 4, 2021

Found the reason.

[LOG 22:50:43.334] [KSP-Recall-Refunding] TRACE: Recalculate Results originalCost: 13800.0; resourceCosts:38400.0; wrongCost:-24600.0; rightCost:38400.0; fix:63000.0 ;

The costfix for this craft is 63.000F, what added to the nominal 23.700F gives the 135.301F charged on launch.

What's happening is that the charge happens on the Flight Scene, and by that point I already had initialised the Funding internal data.

So the solution is to do not initialise Refunding until the last moment it's needed...

@Lisias
Copy link
Contributor Author

Lisias commented Mar 4, 2021

Fixed!

screenshot80
screenshot81
screenshot82

Commit 6a0f157

@Lisias
Copy link
Contributor Author

Lisias commented Mar 5, 2021

With the 0.0.7.3 release publsihed, I consider this issue closed/fixed

@Lisias Lisias closed this as completed Mar 5, 2021
@gotmachine
Copy link

gotmachine commented Mar 22, 2021

This is a terrible, hacky and ugly solution for a very easy to solve problem.

The mistake in KSP 1.11 happens in the stock Funding.onVesselRecoveryProcessing() callback.
When implementing recovery of cargo parts, they somehow missed to call the original refund method with the includeModuleCosts arg set to true.

When includeModuleCosts is true, ShipConstruction.GetPartCosts() grab the ProtoPartSnapshot.moduleCosts value, itself set through the Part.GetModuleCosts() method (itself iterating on all IPartCostModifier modules to compute the result) that was called when the vessel was last unloaded, when the ProtoPartSnapshot was created

They are now calling only :

ShipConstruction.GetPartCosts(protoPartSnapshot, includeModuleCosts: false, availablePart, out var dryCost, out var fuelCost);

which causes the ProtoPartSnapshot.moduleCosts variable to be ignored when calculating refunding.

The solution is to create a KSPAddon that subscribe to the GameEvents.onVesselRecoveryProcessing event, adding a callback that look something like that (totally untested) :

private void onVesselRecoveryProcessing(ProtoVessel pv, KSP.UI.Screens.MissionRecoveryDialog mrDialog, float recoveryScore)
{
	if (pv == null)
	{
		return;
	}

	float modulesRecoveredFunds = 0f;

	foreach (ProtoPartSnapshot protoPartSnapshot in pv.GetAllProtoPartsIncludingCargo())
	{
		modulesRecoveredFunds += protoPartSnapshot.moduleCosts;
	}

	modulesRecoveredFunds *= recoveryScore;

	if (mrDialog != null && mrDialog.fundsEarned > 0f)
	{
		mrDialog.fundsEarned += modulesRecoveredFunds; // might not work if this is called before the stock callback, but this is UI only anyway.
	}

	Funding.Instance.AddFunds(modulesRecoveredFunds, TransactionReasons.VesselRecovery);
}

You can eventually get fancy and create a dummy PartWidget widget to the MissionRecoveryDialog to give an additional UI feedback.

Also, you should probably check the KSP version since this only apply to KSP 1.11, you can do something like that :

Version KSPVersion = new Version(Versioning.version_major, Versioning.version_minor, Versioning.Revision);

Version MinVersion = new Version(1, 11, 0);
Version MaxVersion = new Version(1, 11, 9);

if (KSPVersion >= MinVersion && KSPVersion <= MaxVersion)
{
	// Suscribe to GameEvents.onVesselRecoveryProcessing
}

@Lisias
Copy link
Contributor Author

Lisias commented Mar 22, 2021

This is a terrible, hacky and ugly solution for a very easy to solve problem.

The mistake in KSP 1.11 happens in the stock Funding.onVesselRecoveryProcessing() callback.
When implementing recovery of cargo parts, they somehow missed to call the original refund method with the includeModuleCosts arg set to true.

When includeModuleCosts is true, ShipConstruction.GetPartCosts() grab the ProtoPartSnapshot.moduleCosts value, itself set through the Part.GetModuleCosts() method (itself iterating on all IPartCostModifier modules to compute the result) that was called when the vessel was last unloaded, when the ProtoPartSnapshot was created

They are now calling only :

ShipConstruction.GetPartCosts(protoPartSnapshot, includeModuleCosts: false, availablePart, out var dryCost, out var fuelCost);

which causes the ProtoPartSnapshot.moduleCosts variable to be ignored when calculating refunding.

The solution is to create a KSPAddon that subscribe to the GameEvents.onVesselRecoveryProcessing event, adding a callback that look something like that (totally untested) :

private void onVesselRecoveryProcessing(ProtoVessel pv, KSP.UI.Screens.MissionRecoveryDialog mrDialog, float recoveryScore)
{
	if (pv == null)
	{
		return;
	}

	float modulesRecoveredFunds = 0f;

	foreach (ProtoPartSnapshot protoPartSnapshot in pv.GetAllProtoPartsIncludingCargo())
	{
		modulesRecoveredFunds += protoPartSnapshot.moduleCosts;
	}

	modulesRecoveredFunds *= recoveryScore;

	if (mrDialog != null && mrDialog.fundsEarned > 0f)
	{
		mrDialog.fundsEarned += modulesRecoveredFunds; // might not work if this is called before the stock callback, but this is UI only anyway.
	}

	Funding.Instance.AddFunds(modulesRecoveredFunds, TransactionReasons.VesselRecovery);
}

You can eventually get fancy and create a dummy PartWidget widget to the MissionRecoveryDialog to give an additional UI feedback.

Also, you should probably check the KSP version since this only apply to KSP 1.11, you can do something like that :

Version KSPVersion = new Version(Versioning.version_major, Versioning.version_minor, Versioning.Revision);

Version MinVersion = new Version(1, 11, 0);
Version MaxVersion = new Version(1, 11, 9);

if (KSPVersion >= MinVersion && KSPVersion <= MaxVersion)
{
	// Suscribe to GameEvents.onVesselRecoveryProcessing
}

There's no need to check for the KSP version on the Module, as they are injected selectively by the patch.

See the files:
https://github.com/net-lisias-ksp/KSP-Recall/blob/master/GameData/999_KSP-Recall/patches/refunding.cfg

'KSPRECALL-REFUNDING' is registered on ModuleManager only when the patch is needed - the decision of being applied doesn't belongs to the Module, but to some other code where better strategic decisions can be made.

@net-lisias-ksp net-lisias-ksp locked and limited conversation to collaborators Mar 24, 2023
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants