Vessel orientation broken when unloaded or when in inverse rotation

[NathanKell]

Due to looking into RO's packed-rotation code, I ended up looking at KSP itself and found these issues:

• KSP does not keep vessel orientation fixed when in inverse rotation. If you orbit below the inverse rot threshold, create a maneuver, align with it, and warp, you'll see you start to drift off the node.
• KSP attempts to store orientation for vessels through save/load (or vessel switching outside the loaded bubble, which a save/load on the same scene) by storing in the protovessel the vessel's orientation as surface-relative with respect to the mainbody. However, it only does that storage when a vessel unloads, which means that a vessel that is not loaded at the time of flightstate save will have a busted orientation if the vessel switched SOIs or if the planetarium was ever rotating (i.e. the active vessel was under inverse rotation) after that vessel was unloaded.
• Obviously vessel orientation for unloaded vessels when the active vessel is under the inverse rot threshold gets very busted, even regardless of a save-load, because the entire universe is rotating but the vessel's orientation isn't rotated to match.

Suggested fixes:

• Ignore the inverse rotation issue for unpacked vessels. Bodies rotate slowly enough that the deviation when not in warp is minor (and most probably SAS or MJ or something is engaged to keep the vessel aligned with anything fixed but meaningful like a maneuver burn vector or normal/antinormal), and orientation error is consistent across the loaded bubble. Fixing this would involve messing with objects that have live rigidbodies, which seems like a bad risk/reward ratio.
• For packed vessels (regardless of whether they are loaded), as part of orbit updating in vessel precalculate also set their rotation. This would involve preserving orientation in a fixed frame (i.e. converting orientation in unityspace to Planetarium Zup space) and saving it when a vessel packs or unloads. Protovessel's rotation field could be used for this as long as it is converted to surface-relative rotation prior to flightstate save and back to Planetarium Zup-space rotation on flightstate load. Rather than doing this workaround to Protovessel, a VesselModule could be used to hold this data, but that's less clean (and less performant). Finally, storing rotation as surface-relative (i.e. fixing the bug where KSP doesn't update it for unloaded vessels as part of BackupVessel) would fix some issues but not solve others, and should be discarded.

Reapplying an invariant rotation (rotated to whatever the current localspace coordinate system is) whenever a vessel is packed--including for unloaded vessels!--should solve (a) the inverse-rotation issue for loaded but packed vessels, (b) the inverse-rotation issue for unloaded vessels, (c) SOI transitions for unloaded vessels, and (d) unloaded vessels not having their current rotations saved as part of BackupVessel

[NathanKell]

This implementation is now correct (in RO): https://github.com/KSP-RO/RealismOverhaul/commit/0e7c715bb15ad7d737aa928eb6a7d5794e89b55e

[gotmachine]

I'm trying to wrap my head around this, so I will try to just reformulate, let me know if there is something I didn't get right. So I see 3 somewhat separate issues :

1. The rotation persistence mechanism consist in persisting ProtoVessel.rotation, equal to Vessel.srfRelRotation, defined by : Quaternion.Inverse(vessel.mainBody.bodyTransform.rotation) * vessel.transform.rotation. That persistence mechanism doesn't work because :
   • bodyTransform.rotation isn't a fixed surface relative reference when inverse rotation is active, making the persisted value useless garbage as soon as inverse rotation has been engaged after that value has been persisted.
   • An unloaded vessel can change SOI, making that main body relative persisted value useless garbage in that case too.
2. Being under inverse rotation isn't corrected for on the pos/rot of the parts of loaded vessels (packed or not), resulting in an orientation drift over time.
3. There is no continuous correction of the inverse rotation drift for unloaded vessels either. KSP never touch the rotation of unloaded vessels (neither vessel.transform.rotation nor protovessel.rotation), beside setting calling vessel.transform.rotation = vessel.orbit.referenceBody.bodyTransform.rotation * protoVessel.rotation once on ProtoVessel.Load() when the vessel is instantiated on a full game reload.

The solutions would be :

1. Persist a planetarium zup space rotation instead of a main body surface relative rotation
2. When under inverse rotation mode, continuously reposition all parts in every loaded vessel to correct for the inverse rotation induced drift.
3. When under inverse rotation mode, continuously rotate the transform of all unloaded vessels according to the persisted planetarium zup-relative rotation value.

Difficulties / implementation :

1. Repurposing the ProtoVessel.rotation field to store a zup-relative rotation would require patches on ProtoVessel(Vessel VesselRef, bool preCreate) (where the value is set) and ProtoVessel.Load() (where the value is copied to vessel.transform.rotation). That seems like something sensible, but would cause weird side effects to any code relying on that field being a unity world rotation. From a quick github search, this would at the very least break the LMP/DMP multiplayer mods, possibly a few others. The ProtoVessel objects lifecycle is almost impossible to track, so storing the value in a Dictionary<ProtoVessel, Quaternion> would be a memory leak mess. That leaves us with the VesselModule option which is quite overkill, or leveraging the weird Dictionary<string, KSPParseable> vesselStateValues "random data store" field in ProtoVessel, which has the drawback of being quite inefficient to read from.
2. Counteracting the inverse rotation drift on loaded vessels sounds like a potential mess.
   • For packed vessels, it sounds like this could be patched relatively safely in OrbitDriver.updateFromParameters(), by updating vesselTransform.rotation prior to the Vessel.SetPosition() call.
   • For unpacked vessels, AFAIK we have no general entry point. The floating origin isn't necessarily running every frame, adding another transform update of all rigidbodies would have a quite noticeable performance impact, and the potential compatibility issues of doing this are quite high...
3. Patching OrbitDriver.updateFromParameters(), to update vesselTransform.rotation should also take care of the unloaded vessels case.

In any case, setting vesselTransform.rotation for packed and unloaded vessels is something that stock never do past initial instantiation, but a bunch of mods are definitely doing it, so overwriting the value continuously with our own "source of truth" will surely cause a lot of issues.

A safer alternative would to be to apply the inverse rotation induced rotation offset to the current vesselTransform.rotation value, but given how small these offsets are likely to be, and the fact that we will be applying them on a 32 bit floats backed quaternion, I fear there will be significant FP precision induced drift over time, but this should probably be tested. Applying an offset also mean we should be extra careful to not miss a vessel or apply it multiple times to the same vessel, which will likely prove tricky if we want to reuse the existing stock SetPosition() calls in OrbitDriver and FloatingOrigin to avoid the performance overhead of calling it again in a component of our own.

[NathanKell]

Yeah, it's complex indeed! Your reformulation looks broadly right based on my current understanding; here are a couple additions/modifications:

1. It's worth pointing out that it's less "when inverse rotation is active" and more "whenever Planetarium's inverse rotation angle changes". This could be because the active vessel has gone below the inverse rotation threshold altitude, which is to say or it could be because flight was switched to an unloaded vessel (Planetarium's Awake sets the inverse rotation angle to 0) or the scene was switched to space center (which puts the home body in inverse rotation).
2. Position is fine, only rotation is an issue. (Position is corrected for packed vessels because their position is set every frame by orbitdriver; it's correct on unpacked vessels because the forces applied to them take inverse rotation into account).
3. This is true. However this is also a problem for unloaded vessels when saving/loading when Planetarium is directly rotating too: because the rotation field of the protovesel is not actually updated during save for unloaded vessels, even if Planetarium is not inversely rotating, the body the vessel is orbiting is rotating, which means protovessel.rotation no longer matches vessel.srfRelRotation.

On solutions and difficulties:

1. It is fine to store rotation as surface-relative, but if it is surface-relative it must be updated every frame (at the end of frame, i.e. post-SOI-transition, and post any-other-mods* ).
2. Packed vessels, whether loaded or unloaded, can be treated alike, just using vessel.SetRotation. So long as this is done as part of Precalculate, it should not interfere with other mods, because it will run before their FixedUpdates. (Principia is a slightly special case, because it hijacks precalculate in a different way, so coordination with egg is probably needed, or just force-disabling the patch as unnecessary so long as Principia is installed).

I agree that it's likely somewhat dangerous to straightforwardly patch orbitdriver because updateFromParameters might be called multiple times. Patching precalculate however seems safer not least because a mod that digs deeper would, I would have thought, replaced that with its own subclass (which is why I left the methods virtual--I'm not entirely sure why egg didn't do that).

I also agree float precision will bite us if we're applying per-frame inverse rotation changes at low warp, which is why I tend towards the approach of storing a fixed orientation and setting based on that each frame (and if at end of frame it differs, storing the new orientation).

[gotmachine]

Okay, did a bunch of experimentation trying to hijack updateFromParameters(), tracking vessels rotation with a QuaternionD independently or storing a fixed orientation, but this is becoming quite a fragile mess and I'm facing weird issues, and in both cases I need the "and if at end of frame it differs, storing the new orientation" part, but I'm starting to doubt this can be done reliably. Or maybe this was just a bad day. Will try to poke at this another time.

[NathanKell]

Did the hijacking in RO since we can basically guarantee that it's safe in that ecosystem (since we disable in the presence of Principia, which is the only other relevant supported mod).

Per discord I'm not really sure what the correct stock-ecosystem approach is though because as you say there's a whole bunch of stuff that can be happening during the frame if you can't rely on yourself to be the canonical source of vessel orientation (a github search for .setrotation vessel path:*.cs turns up more than enough hits, alas).