Apparently, OnCopy parts is misbehaving on Parts with Variants
Lisias opened this issue · 16 comments
As the title says.
Take any chain of parts, scale everything up and change the Variant of a scaled up part. See how the OnCopy is failing (use the Alt-Click thingy):
It's about a year since the last time I really dealt with scaling, so let's go again on a debugging fest.
The problem is not on applying the Variant.
The following craft is a command module and two FL-TX1800 tanks. With Chain Scaling, the command module was scaled up two times, to 3.75:
But Alt+Clicking the top Fuel Tank and attaching it in radial mirroring, everything works fine:
Restoring the initial state, I changed the variant and Alt+Click it again and redid the radial mirroring:
And now things are borked.
To be sure, I restarted from scratch, but this time I set the variant first before scaling up.
And I got the same result. Applying the variant it's innocent, it's the OnCopy the one borking on us. The nodes are correctly rescaled no matter if you apply the variant before of after scaling:
Further testings suggest that the OnCopy mechanism is working as expected, but something else after calling the OnCopy is reseting the attached part position using the prefab value. Again. :(
So, in the end, this is not a bug on TweakScale - but another misbehaviour on KSP.
Yep, it was exactly that. Something on KSP was screwing up with the Part's position.
Fixed on commit 9685c75
Hummm.. Not quite. There's still something left to be tacked down!
This appears to be on the OnCopy, because the mirroring (the part on the right) is working as expected (it's mirroring the original including the mishaps)
Again, by detaching the part and reattaching it, things get fine - so it's the MoveParts code the problem now. I'm using it out of its original context - what means that I can't just change it, or I will break it when used inside its context.
Oukey, so it's regression test fest. After installing older versions of TweakScale, I determined that I messed up something on TweakScale Beta 2.5.0.26.
So whatever I made wrong, was did between this tag and this one. About 7 commits to investigate.
Surprisingly, it was not the refactoring the cause of the mess! It was supporting changing Variants after scaling!! This regression is on Beta for a whole year - what means I should spend more time playing with Beta. I would get caught this one if I was playing Beta as I used to do….
Nope, it was not the MoveParts.
It's the OnCopy the problem - I think this is not handling correctly the ModulePartVariant. I just detected that my minimalistic approach was causing inconsistent behaviour:
- sometimes it works fine
- sometimes it slightly displaces the attached part
- sometimes is hugely displaces the attached part
But the behaviour is deterministic - the part is (mis)placed always on the same place given a Scale.
Since the code that works fine on the original is borking on the duplicate, it's reasonable to conclude that the duplication process is borking somewhere - same code only produces different results using different source data (unless we are randomising things, of course).
On an educated guess, I think the ModulePartVariant is not being duplicated as expected (if at all). Even by trying to scale the part from scratch (at is done when loading the part from a config) things is not working, what corroborates my current hypothesis.
Oukey, evidences:
Scaling one point to 2.5 (original is 1.875) and duplicating the fuel tanks:
Scaling one more point to 3.75 (it works by "accident", as it appears):
Scaling one more point to 5M:
Right now, I think my best line of action is to research the source code of older TweakScales where this thing (apparently) worked to see exactly how I managed to to that at that time. I think I got lucky...
Found it. THIS is the commit where the misbehaviour started:
Now I need to understand WHY IN HELL these two lines (I think) are causing this damage on OnCopy:
if (isAttachedParent) {
offset = -offset;
this.part.transform.Translate(offset, this.part.transform);
On a second thought, the problem could be this one:
Testings is on the works
It's definitively line 564. Changing it to:
Vector3 offset = (currentBaseNodesWithSameId[0].position - previousBaseNodesWithSameId[0].position)l;
"Solved" this problem, but I wrote that line for a reason (that I forgot what was), so I need to thoughtfully test the whole Shebang searching for side effects. Apparently it didn't affected the part moving itself - but, again, I had wrote that line for a reason last year, it must had solved something.
Oh well, more and more tests….
Comments in code are a beautiful thing, lol. Be nice to you future self. Comment generously
Comments in code are a beautiful thing, lol. Be nice to you future self. Comment generously
At that time, I didn't saw a reason for documenting a mere scaling up math statement. :) The offending part of that line would scale the offset from the default scale to the current one.
At that time, I had reasons to believe the problem were the UpgradePipeline overruling me (as I was thinking now), and so I documented the problem where I thought the problem was: 645cca7
Right now, I still think it's some code on KSP doing something unexpected. Perhaps the UpgradePipeline cold be involved too, because the unexpected nasty effect of that line was being worked around by saving and loading the craft file - what means that whatever that line was doing, it was innocuous at best, because its consequences were not being persisted on the craft file, what means that it was being recalculated on OnLoad, and since the craft behaved after OnLoad, that line is screwing up things.
It's embarrassing, but in the end this is just bad code. I just don't understand, yet, why it's bad code and this is the reason I didn't pushed this change yet. But the root cause of this problem, indeed, it's really simple: bad code.
Just remembered what I was intending to do with that fscking line of code: to fix issue #86 !! :)
Weird enough, this is the fix for this problem: d5a80c7
The reason this was screwing me up is beyound me, it must be buried inside KSP guts somewhere. All I could detect is that by removing this line, that frankly is not doing any fancy, KSP stopped screwing me up during the OnCopy.
I think that the positioning of the attached part can't overrun a threshold by some reason - otherwise a KSP internal code would misbehave. Without reverse engineering, we would never know the true reason, however.
In a way or another, the damned problem is fixed.
Issue #110 was affected by the fix for this one.