TweakScale/TweakScale

`PartModule.OnLoad` **is not** called with `node` as null on Flight Scene!

Lisias opened this issue · 1 comments

Well, embarrassing enough, this is a detail that I just let pass trough all these years - if one day I was aware of this one, I forgot about:

On Editor Scene, the PartModule.OnLoad(ConfigNode) callback is called twice when loading a craft:

  1. Once, when instancing the Part from the prefab, and ConfigNode is null on this call
  2. Another while loading the respective information from the craft file.

But on flight scene it is called only once, with the ConfigNode filled. And so a semantical bug I let pass trough (I was depending on the prefab call happening) blew on my face.

TL;DR

Fix the "MiniUpgradePipeline" to do not access any ScalingEngine data (this.scaler).

The Gory Details

So, TweakScale initialises some internal data twice on its life cycle, and onde of this internal data is the Scale Engine itself. While on "prefab mode" load, the ScaleEngine is short circuited to give only default results and ignore scaling: just enough to keep KSP happy while calling some internal interfaces, as IPartCostModifier. The ScalerEngine is instantiated on the SetupPrefab, or in the Setup methods respectively, and OnLoad method choose which one to call based on the contents of the ConfigNode thingy.

And this is where I botched the thing: the "MiniUpgradePipeline" stunt I pulled out of my hat depended of a working ScaleEngine to do its work due a silly oversight of mine. So, the ExecuteMyUpgradePipeline method should only be called after the SetupPrefab or Setup ones.

BUT by calling Setup before I would be tampering with the "MiniUpgradePipeline" work, and I probably did that mistake sometime in the past to allow launching the damned thing on Flight Scene - without being fully aware of the consequences (late night coding, being pissed off of being forced to diagnose another bug induced by 3rd party, just a bad day full of brain farts, whatever).

Now that I revised the code and realised (again) the mistake of calling Setup before the "MiniUpgradePipeline", I left it exposed because it was depending on a ScaleEngine to get a silly information to do its job. This crap didn't blew on my face on Editor time because Editor always call OnLoad twice, and by the time "MiniUpgradePipeline" is called, the old prefab one was still in memory. On Flight Scene, it was not and… KABOOM.

We have here a really textbook case of data coupling: different components without semantical relationship depending of something from each other. Or plain coding stupidity.

There're still some coupling on the Scale Engine internal data around, but the components are semantically related to it, so at least for now this stunt is passing trough on the rest of the code - but, really, this is something that I should fix sooner or later for the sake of my own mental healthiness.

But this is a fight for another day.

Stupidity fixed on commit b73eb39