Misplacing parts when chain scaling clones.
Lisias opened this issue · 12 comments
Fellow Kerbonaut Krazy1 found another one.
In a nutshell:
-
drop Mk2 long fuel tank for root. Surface attach same tank to top.
-
translate it up and center on the root part so they appear to be stacked vertically.
-
Critical step: use Alt-LMB to copy the top tank and node-attach it on the back.
-
chain scale the root to 1.875 m. The last part is offset vertically.
-
Note if you did not Alt-LMB copy at "critical step" but instead took a new part from the library, then this does not happen... there is no offset, it looks correct.
Notes: Sounds like something TS needs to do about KSP-Recall, perhaps?
NOPE!!! :D
Today I did another thingy that I should had done way before: I tested 1.3.1 with TweakScale 2.3.7, the last one compiled for 1.3.1 - and I could reproduce the problem the same!
It's something happening since forever, this use case never worked at all.
Note to myself: not everything is my fault, after all! I do things right now and then!! :P
Anyway, I was really chasing my tail on this one. It's something that TweakScale never did right, or it's something bugged on KSP since 1.3.1 (perhaps the OnCopy
handling).
I think it's time to give KSP 1.2.2 a try just for the lulz. :)
I reproduced the problem on KSP 1.4.3. Exactly the same problem.
Same TweakScale, same Recall.
This is "good" news, it means that almost surely it's something I'm doing wrong, something I'm not doing right, or a new and consistent misbehaviour that will be fixed for everybody at the same time.
FIxing the part's position after scaling up, and then chain-scaling everything down again also presents the misbehaviour, but "inverted" - the displacement is now upwards.
Knowing the TweakScale code as I know, this suggests that the calculation of the attach node offset of the affected part is being "contaminated" somehow when the part is a clone. Such "contamination" doesn't happen on new parts using prefab
data.
This appears to rule out TweakScale's scaling code from the problem. [edit] Nope… See below…
I don't think this is a missing use-case on Recall itself… Or it is? Perhaps a race condition, since both TweakScale and Recall handles the OnCopy
event? [edit] There was a missing use case on Recall, but it is unrelated to this problem
Oukey. KSP-Recall is missing an use case, but it's unrelated to this problem: by implementing the use-case, I found no change on the misbehaviour.
Well… Then I did something that I should had already done at first place: checked KSP 1.3.1. And the problem is there too.
That's settle the matter: I messed up something on the TweakScale code for sure. KSP 1.3.1 has none of the problems I'm fighting since forever on Editor.
I'm considering, right now, that perhaps TweakScale should "normalize" the parts on attachment of cloned parts, so they would mimic the attributes of a new part from the Palette. This would avoid the need to mangle on a already stablished code (almost a decade old), not to mention making it easier to handle these parts for everybody.
I don't think I should do it on Recall, however. This is not a bug on Editor, and since it affects 1.3.x (and I'm betting everything downto the first KSP release to allow cloning parts), it would demand spreading Recall to these versions (down to 1.2.2 at least), something I'm pretty reticent on doing - I wanted to retire Recall once KSP's bugs would be fixed (how naive I was), not to spread the damned thing into everything as a pandemic.
Instrumenting the PartModule.OnCopy
and TweakScale.OnEditorAttach
callbacks, I got:
For a cloned part:
[LOG 13:45:21.162] [TweakScale] TRACE: orgPos, attPos, attPos0 (0.0, 0.0, -1.4) (0.0, 0.0, 0.6) (0.0, 0.0, -2.0)
[LOG 13:45:21.162] [TweakScale] TRACE: orgRot, attRotation, attRotation0 (0.0, 0.0, 0.0, -1.0) (0.0, 0.7, 0.0, 0.7) (0.0, 0.0, 0.0, -1.0)
For a new part:
[LOG 13:45:24.659] [TweakScale] TRACE: orgPos, attPos, attPos0 (0.0, 0.0, -1.4) (0.0, 0.0, 0.6) (0.0, -3.8, 0.0)
[LOG 13:45:24.659] [TweakScale] TRACE: orgRot, attRotation, attRotation0 (0.0, 0.0, 0.0, -1.0) (0.0, 0.0, 0.0, 1.0) (0.0, 0.0, 0.0, -1.0)
In the end, it's all about a missing use case on TweakScale - it assumes that every new attached part will get the default, prefab values for these attribute, clearly a mistake.
The fix is straightforward.
I'm an idiot. The diagnostic was under my nose all the time: if saving and loading the craft doesn't fixes the problem, all I need to do is to create a new similar craft without the problem (i.e., getting the 3rd MK2 tank from the Palette instead of cloning it) and compare them with KDIFF!!!
With the thing happening on 1.3.1, KDIFF will easily spot the differences.
Here, the KDIFF between the two sample crafts:
The attpos
appears to be the problem.
Craft files (both): Untitled Space Crafts.zip
Now I need to understand why the attpos
is being screwed when cloned. There's a chance we found yet a new bug on KSP's Editor…[edit: nope, just a use case that no one thought of before]
This is what I'm thinking it's happening:
-
When we clone a part that was displaced from the "ideal" attachment point, the new
attPos0
is calculated from the currentattPos
, instead of the ideal one specified on the prefab. -
When we take a new part from the Palette, the
attPos
is fetched directly from the prefab, and so theattPos0
is calculated in a more orthodox manner. -
The scaling code is getting confused because it uses the
prefab
as reference when recalculating positions, and gets screwed because the cloned part is using another position.