ReRootPreserveSurfaceAttach does not reconnect the child attachnode to the parent part

[JonnyOThan]

Ends up causing https://github.com/JonnyOThan/TweakScale/issues/56

Normally the attachedPart would be set in ReverseSrfNodeDirection

However, disabling the patch leaves the attachnode position in the wrong place - so the patch is good, just missing a step.

I'm not sure if this has any other possible negative effects, but it does persist across a save/load of the craft file.

[JonnyOThan]

I'm actually starting to question the utility of this patch in the first place...Maybe the stock code needs to be fixed but moving the surface attach node does seem to make sense to me. Are you sure it doesn't affect partjoint positioning? I'll investigate a bit.

At the very least, if I re-root to a surface-attached part and then pick up the other part and try to re-attach it, you can get some strange behaviors because it will change orientation. But of course as you mentioned in the original issue it's also very strange to have different instances of the same part that have different surface attach points.

[JonnyOThan]

OK after reading through #142 again I agree with your conclusions - but I'm still concerned about the partjoint positioning in flight. Will need to check.

[gotmachine]

Good catch on the the patch failing to set attachedPart on the child node.

Indeed, it seems logical that docking could result in an in-flight hierarchy inversion, in which case the absence of a surface attach node repositioning logic would likely cause some weirdness with joints positions. Arguably, the stock code just mess that up even more, so removing it (as the patch currently does) can't hurt.

I decided not to try to fix the stock logic for a few reasons :

• I'm lazy
• It's a high risk / low reward modification
• At least in the editor, I feel what the stock code is trying to do isn't actually a good idea from an UX perspective. It tries to handle some unavoidable inconsistency in behavior by introducing another one, and only managed to introduce bugs and to break a somewhat reasonable assumptions that attach nodes are supposed to stay where they are defined in the part config/prefab.

I guess if the in flight issue with joints is confirmed, we could try to put something together there, but I feel like even so, this isn't really worth the trouble. It's not like joint behavior wasn't an unpredictable mess anyway.

[JonnyOThan]

OK so it looks like there's a bug here, but it's not quite what I was expecting. The position seems OK, but the stiffness (which comes from node size) is wrong:

Repro steps:

1. Start a new craft
2. Place a big heavy fuel tank
3. Place another one surface-attached on the left side (i.e. on the opposite side of where the srf attach node is). both tanks should have the same orientation. If they have some kind of greeble you should be able to tell
4. re-root to the second tank
5. repeat step 3, so that you end up with 3 tanks in a line, in the same orientation and the middle one is the root
6. place some launch clamps on the center tank so it doesn't move
7. launch the craft

[JonnyOThan]

Oh no....so I just realized that the tests in the above comment were done on the dev branch of KSPCF (not including the changes I had already made to address this issue). When the parent part is set correctly, the stiffness is correct but the node position is wrong (which is the problem I was expecting, but I don't know why this only happens when the parent is set!)

Notice how both tanks tilt to the right (because the joint on the tank on the left is in the wrong place)

[JonnyOThan]

I have a hunch....

The stock AttachNode.ReverseSrfNodeDirection function (and ChangeSrfNodePosition) only modifes the position member of the AttachNode - not the originalPosition. So far I've been pretty confused about why these two fields both exist, because most code seems to try to keep them in sync. But this might be a good reason why they might be different. If you then had a way to make the position snap back to the originalPosition if the user picks up the part, it should probably resolve the problem, and I wonder if that's what they actually intended. One terrible problem though is that the originalPosition seems to never get persisted to craft files. BUT we could probably maybe almost use the srfAttachNode from the prefab....except for all the code that could potentially modify it per-part (ModulePartVariants, TweakScale, who knows what else).

I think the ideal design is to move the attachNode's position, leave the originalPosition where it was, and then set position = originalPosition when picking up the part. But I think we'd need to persist originalPosition somewhere in the craft file, because otherwise I don't think there's any good way to reconstruct it when saving/loading in the editor. Note that for vessels saved in flight, we don't really need to persist the originalPosition. This might have some impact on EVA construction, but I think at that point the player can deal with the weirdness of parts that have a srfAttachNode that is different from the prefab.

alternatively we can go the other way - keep position and originalPosition in sync, and then find some other way to persist where the PartJoint should be created. That seems a little scarier to me - for one, vessels in flight would still have to deal with it.

[JonnyOThan]

Interesting....so the only place where the originalPosition gets used is when mirroring parts. And I missed one piece of info earlier - it DOES get persisted to the craft file, but only for the attachNodes list, not the srfAttachNode.

[JonnyOThan]

I've been banging my head against this for a while. Every bug I fix (and there are a lot of obvious bugs) just causes more problems.

I'm starting to be convinced that your original patch actually IS the correct design. But I wonder...when you need to flip the surface attach node, could you maybe just clone it and stick it into the normal attachNode list instead. So the parent effectively has two surface attach nodes - the normal one which never moves and the cloned one which points to its old parent (now child). This seems way more consistent because the current design leads to weird contradictions in attachRules etc. I don't know what's going to break if we try it...

[gotmachine]

when you need to flip the surface attach node, could you maybe just clone it and stick it into the normal attachNode list instead

Even if you can make it seemingly work, that feels quite hacky to me.

I haven't reviewed this in detail, but if the part is defined as surface attached (Part.attachMode = AttachModes.SRF_ATTACH), it is a reasonable assumption that the node to look for is defined in Part.srfAttachNode, not in the Part.attachNodes list. And in reverse, it is a reasonable assumption that Part.attachNodes only contain stack nodes. I have some doubts that "coming out of nowhere" extra node will be treated gracefully by the various stock and modded dynamic node switchers, and by the various pieces of code checking nodes for a reason or another...

I'm not sure what the side effects could be in practice, but this feels quite messy and risky, which lead to the question : what is actual benefit of this ?

I'm a bit lost about the state of this issue : with your previous fix to the original patch, what are exactly the remaining issues ?

It seems as expected, there are inconsistencies with joint placement in flight, but outside of that ?