Attachment Points are not being scaled (or being reset) after changing the Variant

[Lisias]

As reported by Fellow Kerbonaut Krazy1 on Forum:

Having trouble with attachment nodes moving after changing variant. Simple test:

1. place coupla
2. attach FL-T200 on bottom
3. Tweak to 2.5m
4. remove tank... see top bottom nodes are correct... reattach
5. Change tank to orange/ gray variant
6. remove tank... nodes moved inside tank

If I change the variant first and then resize it, it's OK.

KSP 1.12.5, TS 2.4.7.1 and lots of mods.

I reproduced it on my 1.12.5 ACP test bed, ruling out 3rd parties influence on the problem. So it's something on TweakScale or perhaps KSP-Recall - but I need to do regression tests on previous KSP versions to be sure it's not a yet new bug on KSP 1.12.x.

[Lisias]

I reproduced it down to KSP 1.4.3 - way before all of that crap that plagued KSP later.

It's something on TweakScale, I'm probably forgetting to do something on the VariantPartScaler.

[Lisias]

Fixed on commit ad9b8db

This one worths some digging, see next posts for details.

[Lisias]

This is how I "fixed" the problem:

		protected override void DoScale()
 		{
 			base.DoScale();
- 			this.ScalePart(true, false);     <— I removed this line
+ 			this.ScalePart(true, true);      <— I added this line
 			this.ScaleDragCubes(false);
 			this.OnChange();
 		}

The ScalePart has the following signature:

		/// <summary>
		/// Updates properties that change linearly with scale.
		/// </summary>
		/// <param name="moveParts">Whether or not to move attached parts.</param>
		/// <param name="absolute">Whether to use absolute or relative scaling.</param>
		protected void ScalePart(bool moveParts, bool absolute)

So, what I changed is to tell the ScalePart to reposition the AttachmentNodes (all of them) in absolute mode, instead of calculating the displacement from the current positions.

I really never understood why this option ever existed at all, it always puzzled me why the original author decided to calculate the new positions from the current ones, instead of getting the original values from prefab at once. But I kept following suit all these years because, well, the damned thing was working and without some background history (or a need) it's not wise to change things just because you find them "weird".

Anyway, I decided to track down this damn thing downto the root. TweakScale was heavily refactored on all these years, and so this tracking can be somwhat cumbersome - and my decision to create specialized DLLs for each KSP version, besides hugelly improving the versioning and tracking of KSP features and changes, didn't exactly helped me on this task. :)

Anyway, I tracked down the first time this method, ScalePart was created to this commit: 073f313 when the UpdateByWidth method was renamed to ScalePart. The reason was "refactoring" - one of many times this happened. :)

The method's signature and function didn't changed, only the name. So from now on I will dig for the UpdateByWidth thing.

So I kept digging and found this commit 8f2a9f6 where the UpdateByWidth method changed signature

-        private void UpdateByWidth(bool moveParts)
+        private void UpdateByWidth(bool moveParts, bool absolute)

And we found the birth of the bool absolute thingy, and the reason was "Fixed erroneous placement of nodes after duplicating parts" :), and the commit date is 2014-12-24 (Christmas Eve!!!)

Oh, boy, I had fought with it for some time too. :D

This commit happened when the KSP version on the wild was the 0.90, released 9 days before the commit. And logic says that this probably ws needed due a change from the previous KSP version, 0.25 .

The 0.90 Change Log, however, doesn't gave me any concrete hint about the need for the change:

Summary
Editor overhaul
Kerbal experience and professions (Pilot, engineer, scientist)
Extra contracts (Integration of Fine Print)
Biomes on every planet and moon
MK3 overhaul
Upgradable facilities (in Career)
Organization of parts, including custom filters
Tutorial and help

But I would bet (based on my previous experiences on this field) that it was something that changed due the "Editor overhaul" task. :)

Well, this was fun. Really - I like to dig on the commit history to see why things were done the way they were done. As George Santayana said once:

Those who cannot remember the past are condemned to repeat it.

Guess what? He was right. :D

[Lisias]

For the records: this is not intended to be a "finger pointing" fest. This is a learning journey, I wanted to know why things developed this way, and I did! :)

My best guess at this moment (as, as usual, my window for toying with KSP is closing again) is that the relative scalings and movings were due:

1. the absence (or unreliability) of prefab data needed to do the job ; and/or
2. the importing of already tested code from another game where there was not prefab

Knowing as I do now why things are what they are (probably), I'm reasonably sure that I should remove all the "relative" calculations after all (i.e., everything should scale and mode with absolute as true). There's no need for them nowadays, and they are only haunting me with unexpected collateral effects as I refactor and implement new things on TweakScale!

[Lisias]

Reopening, as I need to fix this again but without triggering yet another Editor bug (see #314 for the consequences).

[Lisias]

Quoting myself from #314 (comment) :

NOW I understand why that was needed. When the part have ModulePartVariants, the needed dada is not available at that precise moment for an absolute moving!

What I fail to understand is why in hell things work on Flight Scene so - if I'm doing something wrong, so I'm doing something wrong and I'm doing it all the time.

Anyway, Editor is screwed and this is not going to change. I need to cope with the shit somehow.

[Lisias]

Commit b746ac1 fixes this issue, but created another one: the Cloning process is now screwed.

This suggests that the cloning support is flawed, and need to be redone.

[Lisias]

Oukey, finally understood the problem. I screwed up the fix for #261 on this commit:

dbe87fb

Reverting it fixed the problem for KSP <= 1.7.3, but it persisted slightly different on KSP >= 1.8 . Issue #261 ended up in regression too for KSP >= 1.8, and this strongly suggests that:

• This issue is a duplicate of Misbehaviour (again) while scaling parts with VARIANT #261
• I should not had removed the 1.8 Support DLL on commit 8b344a0#diff-406cf1d632e5bd3df5e55e5a0a49a0117eaaacc27e4ccbf4dbe8bf7dbc00490a

So many things changed since removing the 1.8 DLL that it's probably a better idea to recreate it from scratch...

[Lisias]

This issue is blocked by #319

[Lisias]

The commit af45cc5 properly handles the situation - it was a mishap on the cloning process all the time!

[Lisias]

Task #319 was finished. Unblocking this.