Attachment Points are not being scaled (or being reset) after changing the Variant
Lisias opened this issue · 47 comments
As reported by Fellow Kerbonaut Krazy1 on Forum:
Having trouble with attachment nodes moving after changing variant. Simple test:
- place coupla
- attach FL-T200 on bottom
- Tweak to 2.5m
- remove tank... see top bottom nodes are correct... reattach
- Change tank to orange/ gray variant
- 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.
Test Session for KSP 1.4.3.
Environment
- KSP 1.4.3
- KSPe 2.5.4.2
- KSP-Recall 0.5.0.0
- Module Manager /L 4.2.3.3
- ModuleManagerWatchDog 1.1.1.2
- TweakScale Beta Futurely 2.5.0.63 (at commit 4d2f756)
Session
- Resume Saved Game called
test-ts - Go to VAB
- Start a new craft with the Mk1 Command Pod
- Put a Rockomax Jumbo 64 Fuel Tank bellow it
- Shrink it to 1.125m
- Change the variant to the orange one
- Clone it (Alt+LeftClick) and put below the original one
7.1 You should get this image:
- Clone the subassembly rooted by the first fuel tank
8.1 You should have this image:
9 Attach it radially with a 4 summetry/
9.1 You should have this image:
9.2 The mater subassembly node sizes will be reset to the default during the attachment, but once it's attached the nodes will be sized the correct size. This is the Editor trying to screw with us.
9.3. Check that all fuel tanks have the same fuel amount.
- Save the damned thing as Untitled Space Craft
- Click New
- Load it back
- Checl if everypart has its attributes right (size, fuel, attachment nodes size and position)
13.1 Take any new part from the Part Manager, the nodes will be highlighted:
- Clone the subassembly starting from the first fuel tank again
- You will be seeing something like this:
- Attach it at the bottom of the stack.
15.1 Like this:
- Save it, New, Load it back. You need to have the same craft back.
- Clone the subassembly starting at the first fuel tank again.
- Attach it radiall wth 4 symmetry
18.1 You will get this image:
18.2 Some fuel tanks will be clipped inside the parent's subassembly. - Save it, New, Load it.
19.1 You need to have the same craft back. - Launch the damned thing into the launch pad.
20.1 Check the craft again. - Quit KSP on the spot.
Evidences
KSP.log: KSP.log- Savegame: test-ts.zip
Test Session for KSP 1.7.3.
Environment
Slightly different from 1.4.3, I forgot to update MM/L . I will no redo that Test Session again just for it... 🤓
- KSP 1.7.3
- KSPe 2.5.4.2
- KSP-Recall 0.5.0.0
- Module Manager /L 4.2.3.4
- ModuleManagerWatchDog 1.1.1.2
- TweakScale Beta Futurely 2.5.0.63 (at commit 4d2f756)
Session
Exactly the same steps.
Evidences
KSP.log: KSP.log- Savegane: test-ts.zip
Test Session for KSP 1.12.5.
Environment
- KSP 1.12.5
- KSPe 2.5.4.2
- KSP-Recall 0.5.0.0
- Module Manager /L 4.2.3.4
- ModuleManagerWatchDog 1.1.1.2
- TweakScale Beta Futurely 2.5.0.63 (at commit 4d2f756)
Session
Found a problem while cloning a complex subassembly!
The surface attachments are being screwed.
This is something I will check after sleep, it's time to close the shop.
Final notes before closing shop:
- KSP 1.8.1 does not presented the problem depicted on the last comment.
- KSP 1.11.2 DID presented the problem
- My KSP 1.9.1 was accidentally destroyed. Could not test it.
- KSP 1.10.1 DID presented the problem
Without checking 1.9.1, I can't be absolutely sure about the source of the problem - but I have a good guess where the problem started and what it's about. The DelayedRescale stunt needs to reset the surface attachment notes too from KSP 1.9.x and forward.
Oh, well. Vida que segue. 😃
All of them using the following environment:
- KSPe 2.5.4.2
- KSP-Recall 0.5.0.0
- Module Manager /L 4.2.3.4
- ModuleManagerWatchDog 1.1.1.2
- TweakScale Beta Futurely 2.5.0.63 (at commit 4d2f756)
HA! I was wrong, but I was also right!
Yes, the surface attachment are being screwed but not during the scaling or first upate, but on Cloning!
The SubAssemblies are working fine even on 1.12.5, if the problem was what I was thinking it was, it would get screwed when using subseemblies!
Corollary: merging crafts is also working as is.
The current problem is only while doing alt+click on a subassembly with scaled parts with variants.
Oukey, I had dug KSP 1.9.1 from my archives, installed it and finally made it to run.
Hint to my future self (damn, I had forgot about it - wasted my entire afternoon due this crap! 😠 )
xattr -dr com.apple.quarantine <KSP-ROOT>/KSP.app
As expected, it behave exactly like 1.10.2 - what establish the 1.9 series as the starting point of this problem.
I also did regression tests on KSP-Recall and TweakScale on 1.9.1 since December 2021, to detect exactly when I screwed the pooch on this one. All KSP-Recall releases, including the latest, worked fine. TS up to 2.4.6.8 too.
2.4.7.6 borked the Alt+Click even worse than Beta, but 2.4.7.0 worked fine. So it's something I did between them. Working on it.
2.4.7.2 was the last release where the Alt+Click worked fine on 1.9.1.
So I screwed the pooch on 2.4.7.3.
Now I have something to work with.
Found the trigger. It was commit e60645b , meant to fix this exact issue!!! (KRAP!!!).
Something on 1.9.1 editor apparently is not being initialized in time for TweakScale anymore (i.e., the attachment nodes), or perhaps the data is being overwritten by something between the initialization and the time TS needs it. (KRAP!!!)
KSP became a huge mess since the transition to Unity 2019, some things appears to be left behind in this transition in the same sense some things were left behind while transitioning to Unity 2017, see this commit: 34b0a41 where I reimplement the FindAttachNodeByPart to restore a method that were always returning null. (sigh).
Since this commit (e60645b) fixed this for KSP < 1.9.0, then the problem should be reworked only on KSP 1.9.0 and newer. I will not even try to understand the underlying issue, I will just forward-port the change can call it a day.
Hummm... We have two different problems. Similar, but not identical misbehaviours on legacy and beta.
This Hummm... We have two different problems. Similar, but not identical misbehaviours on legacy and beta.
Commit 31deffe worked around the problem on the legacy branch but did nothing on the beta.
Meno male. I can kick 2.4.8.0 trough the door.
Test session for KSP 1.12.5
Environment
- KSP 1.12.5
- KSPe 2.5.4.2
- KSP-Recall 0.5.0.1
- Module Manager /L 4.2.3.4
- ModuleManagerWatchDog 1.1.1.2
- TweakScale Legacy Futurely 2.4.8.0 (at commit 47fa35c)
Session
Exactly the same steps from #307 (comment)
Evidences
I'm rolling back the change. Besides kinda fixing part of the problem, it created another one worst.
Now I really think there's some other thread/coroutine doing something by our backs, undoing partly what's TweakScale is doing.
Well... Couldn't reproduce this on KSP 1.4.3 using the current beta development branch. 😃
Let's see 1.12.5...
On the "bright" side, I reproduced the problem on 1.12.5. Let's see the lowest KSP in which the problem still persists.
On KSP 1.7.3 the attachment nodes are fine, but the Part is displaced after changing the Variant. 😒
However, this is a problem that IIRC I introduced on the beta after refactoring it. I surely lost the "sweat spot" where things were happening all right.
As a matter of fact, I'm getting a glimpse of what may be happening now.
Confirming, this problem is still present from KSP 1.8.0 and forward.
Now I have (again) something to work with
Oh, yeah, baby... KSP'e Editor at its best... 😠
NOW I have hard evidences that Editor is, indeed, screwing me up. The fix for the problem was doing exactly what I was doing before - but after skipping 2 frames on a co-routine.
TweakScale's algorithm may not be the prettiest, but it's working. The problem is not TweakScale. (sigh).
Anyway, commit bfe2ec4 fixed this.
On a side note, the commit 7eddc87 is a clutch needed to allow 2.4.8.0 (where the refactoring wasn't made, and where a lot of clutches are still lingering) to be pushed - and so I can focus on the Beta again.
From the code:
// Note to my future self!
//
// This is not a fix, is a hell of a clutch to get rid of the #307 problem on the legacy branch
// https://github.com/TweakScale/TweakScale/issues/307
//
// The fact is that the legacy branch (this one) still have a lot of gambiarras que are bitting my arse badly. The beta
// branch was refactored for a reason.
//
// Since this worked on beta, it's a proper fix, and not working here is due the clutches that still linger. Porting the beta
// gradually as I was doing while #307 wasn't fixed is not feasible on the long run, and it is not viable anymore on the short.
// So this clutch end up being the less worst of the short term fixes I'm able to do now to push 2.4.8.0 ahead and focus
// on finishing Beta and promoting it to mainstream.
//
// The consequences of this clutch is that variants that change resouces and other physical characteristics will not be properly
// handled, and some features may be left unscaled after the variant is applyed. Eventually a full Scaling will be aplied on
// the craft (by undo, by saving and loading, or by launching) so this is not a show stopper, but it will be annoying for sure.
//
// Corolarry: I need to promote beta to the mainstream ASAP.
Crap. I need to reopen this as I didn't checked if I'm correctly handling KSP-Recall with this delayed frames stunt. Probably yes, but I need to be sure - and I will not have the time to do it today, so a TweakScale release will be made only during the week and I need to be sure to do not forget it.
Additionally... This ended up being not my fault after all, and so wasn't a bug but a regression caused by someone else!!! 😄
I'm reopening this because KSP 1.4.3 got screwed by these changes, and this means that I had fixed the problem on the wrong place.
Historically, I had success on fixing the problem on the KSP release that created them, because all the later KSPs were built over that mistakes. Fixing the problem on the latest KSP release usually means that I'm brute forcing my way, ignoring the root cause of the problem (what potentially screws up 3rd parties, the problem that triggered the whole beta branch ordeal)
OUkey. Commit 4d2f756 fixed KSP 1.4.3 and, very probably, everything after it.
Now I will proceed with a full test session for KSP 1.4.3, 1.7.3 and 1.12.5 (the main workhorses at this time), publishing evidences for future references. Had I did this above, I would had information to diagnose why things appeared to work before...
Oukey. All the test cases from this issue (and some others, as the subassemblies) passed with flying colours. 😀
Dude, there is something running behind the scenes royally screwing up with me. Some process, thread or co-routine is (intentionally or not) being run on every OnShipModified (I'm guessing) while TweakScale does it's magic - undoing some of the changes.
TweakScale (legacy) managed to walk into the mess by redoing things as every undoing was detected, a somewhat less then ideal approach, but given the knowledge I had at the time, a feat by itself.
TweakScale (beta) walked away from such gambiarras and, so, lost the "sweet spot" where things managed to work by accident.
The hint for this crap came when KSPCF accidentally broke KSP-Recall ( see net-lisias-ksp/KSP-Recall#73 ). Analyzing the logs, and since I have access for the source code for both artefacts, I managed to pinpoit the cause - a brittle equilibrium between concurrent routines (co-routines) that KSPCF broke, moving the sweet spot and, so, screwing KSP-Recall that was relying on it to work.
That hinted me that probably something similar could be happening now - after all, since KSP 1.4.0 times the Editor is known to brute force its way into the parts, screwing 3rd parties. My best guess at this point is the code that handles PartModuleVariant - by obvious reasons! 😉
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.
Fixed on commit ad9b8db
This one worths some digging, see next posts for details.
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
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:
- the absence (or unreliability) of
prefabdata needed to do the job ; and/or - 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!
Reopening, as I need to fix this again but without triggering yet another Editor bug (see #314 for the consequences).
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 anabsolutemoving!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.
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.
Oukey, finally understood the problem. I screwed up the fix for #261 on this commit:
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 #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...
This issue is blocked by #319
The commit af45cc5 properly handles the situation - it was a mishap on the cloning process all the time!
Task #319 was finished. Unblocking this.
Reopening, as this problem is happening again on 1.4.3.
it worth to mention that had preserved the test bed used on the test session on this comment: #307 (comment)
Frankly, this is getting ridiculous.
But now the problem is happening when loading the craft file. Only loaded crafts are getting screwed this time.
It's a different use case. I confirmed that the thing is working while changing the variant, as expected, but borked only after saving the craft and loading it back.
But, and again, this damned thing was working last time I checked it, including loading crafts - just found some old notes on the local working directory.
note: an mesh I'm working on a partset I'm toying is borking the PartLoader on KSP 1.7.3, but it was being loaded allright 3 months ago when I last worked on them.
OW KRAP 💩
Oukey. Regression tests.
The problem was reproduced on the following KSP versions:
- 1.4.3
But worked fine on:
- 1.4.5
- 1.5.1
- 1.6.1
- 1.7.3
Oh, well.. Meno male, and I already now why its happening - it's due the changed on the Editor's ModulePartVariant handling that started to happen on 1.4.4.
Since the last working TS on 1.4.3 (today, because the lastest was working last time I checked) is the 2.4.7.3, and I used the latest KSP-Recall on all the tests, this is, really, related to TweakScale.
At least, it's only on 1.4.3.
For the records, 1.4.3 1.4.1** was the first release supporting Making History.
** As a matter of fact, 1.4.1 (the brief) and 1.4.2 were plagued by some bugs. 1.4.3 ended up being the de facto "first" release properly supporting MH, to the point I don't bother to test anything from 1.4.0 to 1.4.2 anymore!
reworked AGAIN on commit xxxxxxx
Scrap that. I over reacted, revisiting the solution. I will almost surely ditch the whole commit due the mess.
Ok, now some evidences - these evidences was what saved my sorry arse this time, removing from my shoulders the burden of trying to debug code that was known to had worked fine before. :)
This craft file is the same used on both testes below on the same 1.4.3 test bed: Test-Issue-307-Again.craft.zip
TS from 2.4.7.4 to 2.4.8.6
Everything was working fine, except by reloading a craft on Editor (no problems were found anywhere else). The attachment nodes are reset to the original position, so scaling up parts had them inside and scaled down parts had them outside, as can be shown on this image
It's important to note that you need to save and then reload the craft file to trigger the problem. The craft file itself is intact, the problem happens after loading the file.
Log : KSP.log
Creating and after saving, but before loading
And now after clicking in New and then on Loading the craft back
Note the attachment node way downthere - but, interesting enough, it's correctly scaled.
With this fix applied:
Log: KSP.log
Regression Tests were executed on the following KSP releases:
-
1.3.1
- Craft File: test-ts-307-again.zip
- Log: KSP.log
-
1.4.5
- FAILED
-
1.5.1
- Craft File: test-td-307-again.zip
- Log: KSP.log
-
Test Suite aborted.
AW KRAP. I borked 1.4.5. At least 1.5.1 (and probably later ones) is still working, so at least I compartmentalized the crap.
Since I will have to revisit this issue, I will check the others after fixing 1.4.5
I will take a nap, and then I will check what I did wrongly on the Scale.PartDB.144 thingy.
Oukey, the problem is that I overreacted when I detected that #328 is still kicking on the 1.4.x series (but is still fixed on everything else). I can live with that for while, I really doubt there's anyone else besides me playing on it.
So, here we go again:
- 1.3.1
- Savegame: KSP.log
- Log: test-ts-307-again.zip
- 1.4.3
- Savegame: test-ts-307-again.zip
- Log: KSP.log
- #328 kicked on this one
- 1.4.5
- Savegame: test-ts-307-again.zip
- Log: KSP.log
- #328 kicked on this one
- 1.5.1
- Savegame: test-td-307-again.zip
- Log: KSP.log
- 1.6.1
- Savegame: test-ts-307-again.zip
- Log: KSP.log
- 1.7.3
- Savegame: test-ts-307-again.zip
- Log: KSP.log
- 1.8.1
- Savegame: test-ts-307-again.zip
- Log: KSP.log
- 1.9.1
- Savegame: test-ts-307-again.zip
- Log: KSP.log
- 1.10.1
- Savegame: test-ts-307-again.zip
- Log: KSP.log
- 1.11.2
- Savegame: test-ts-307-again.zip
- Log: KSP.log
- 1.12.5
- Savegame: test-ts-307-again.zip
- Log: KSP.log
Tests made for the https://github.com/TweakScale/TweakScale/tree/dev/legacy/2.4.8.8 deliverable.