Robotics drift
gotmachine opened this issue · 10 comments
Issue summary
When using a Breaking Grounds DLC robotic part, all child parts are affected by non-recoverable position/rotation drift.
Specifically, child parts of robotic parts have their "pristine" (as defined in the editor) coordinates updated to the current in-physics coordinates, which can differ significantly when the vessel is under stress due to forces like gravity, atmospheric drag, engine and RCS thrust, etc.
- This mean every time the game re-apply the saved "not-so-pristine-anymore" coordinates (ie, when timewarping or unloading the vessel), the original parts position/rotation are definitely lost and the vessel is permanently deformed.
- The deformation gets worse over time, as every time the vessel goes in and out of physics, the in-physics forces apply a new deformation on top of initial one.
For the sake of clarity, two videos comparing the behavior of children parts of robotic parts and demonstrating the simplest way to reproduce that issue.
Expected behavior when timewarping (no robotic part)
The physics constraint is reverted and the parts "snap" back to their pristine positions :
2022-03-10.01-07-42-604.mp4
Robotics behavior when timewarping
The physics constraint doesn't revert, and the resulting deformation is now permanent :
2022-03-10.01-12-41-605.mp4
Root cause
This is caused by BaseServo.ApplyCoordsUpdate() calling recursively the Part.UpdateOrgPosAndRot() method on all children parts. This method update the Part.orgPos and Part.orgRot fields (which are the "pristine" persisted part coordinates relative to the vessel root part) according the the current Part.transform.position and Part.transform.rotation values.
BaseServo.ApplyCoordsUpdate() is called continuously while a servo is moving, and on various occasions during the start / load / save events.
Implemented fix
The current fix override the recursive call done in BaseServo.ApplyCoordsUpdate() to implement a custom logic. Instead of updating Part.orgPos and Part.orgRot based on the current in-physics part coordinates, it computes the servo induced translation/rotation offset, and apply that offset recursively to the current Part.orgPos and Part.orgRot values.
Shortcomings and potential issues
- The patch currently doesn't cover the other coordinate update done in
BaseServo.RecurseAttachNodeUpdate(). I haven't investigated as the current solution is satisfactory, and that code is probably fine as a result of the patch restoring pristine moving part coordinates prior to that method being called. - The "pristine" coordinates we are computing might get slowly out of sync with the in-physics ones due to compound FP precision inconsistencies between our computations and the Unity ones. This being said, stress testing show part positions are visually stable after several hours of continuous servo operation.
- Computing the pristine coordinates adds some computational overhead when servos are in motion, but this is relatively insignificant compared to what those modules do already.
- This is a behavior change that can have an adverse impact in some gameplay scenarios, especially while landed and under significant gravity. The stock "issue" also mean that all stress from gravity is "released" on a save/load or timewarp cycle. You get a deformed vessel, but it is now closer to being "at rest", which can help avoiding kraken events on scene load or when getting out of timewarp.
I have another potential solution. Just tell the game to remove all physics loads from any craft before it saves part positions. Less of a fix and more of removing the cause. I've been thinking of trying to make it myself, but I'm unsure how to go about making the mod.
This isn't a viable solution because :
- When the vessel is about to be saved, it's already too late to remove external forces. It would take hundreds of physics cycles for the joints to settle in a relatively steady state, but you have no way to delay the event.
- You can't really "remove all physics load" reliably. By definition, the rigidbody simulation is always unstable. Even when no external forces are acting on the vessel, the joints themselves are essentially "springs" that don't have a steady state. So even if you somehow manage to delay part position save events, this would only mitigate the issue, not eliminate it. In fact, it could make it even worse, because releasing joint stress suddenly is gonna cause a lot of part position shifting, and struts/autostruts can cause wild instabilities before everything eventually settle.
The only way to fix robotics drift is to save part positions/rotations according to the joints target pos/rot, instead of using the resulting pos/rot after physics. But this is far from easy, because while that information exists, it isn't directly exposed. There are multiple corner cases to consider : physicless parts don't have joints and robotics parts use additional internal joints configured uniquely for each robotics module type.
Theoretically, you would need to start from the root part, then walk down the part hierarchy, finding the connection type of every children (physicless, classic joint, robotics...), and recompute each part "pristine" root part offset according to the theoretical target. This is already not trivial.
The other difficulty is properly overriding the stock orgPos/orgRot updates and properly replacing it with that custom method, which is also a difficult problem, as there are many entry points to cover.
Also the parts saving thing other than scene changes wouldn’t really matter would it?
Yes it would. From the top of my mind, there are at least 3 cases to handle (ie, cases where the current orgPos/orgRot will be restored later) :
- Scene changes
- Game saves (either manual or auto-saves)
- Vessel packing (every time a vessel enter non-physics timewarp, or when a vessel leaves physics loading range)
There is no "clean" way to delay those events to let physics run for few seconds. Even if you could do that without side issues (very unlikely), you would have to patch every possible entry point from all code paths in the stock codebase, and you would likely have tons of cases where other mods would bypass your hooks.
In conclusion, given that this isn't really a solution (you would need to delay all those actions by several seconds for the physics to settle vaguely reliably, with no guarantee that this actually work), and that it would be a huge mess to implement, I don't see the point of attempting this. Not to mention that when "re-enabling" physics again after doing your thing, you would inevitably trigger kraken events. This is just a terrible idea.
The "proper" solution I described would also require a lot of messing around and implementation work, but at least it would actually solve the issue.
Fix implemented in KSPCF 1.9
Not 100% sure this isn't caused by another mod, but my console is flooded with NullReferenceExceptions
[EXC 10:33:34.877] NullReferenceException: Object reference not set to an instance of an object KSPCommunityFixes.BugFixes.RoboticsDrift+ServoInfo.PristineCoordsUpdate () (at <6d36e84722a243e9ba216b0d7c675f35>:0) KSPCommunityFixes.BugFixes.RoboticsDrift.BaseServo_RecurseCoordUpdate_Prefix (Expansions.Serenity.BaseServo __instance, Part p, UnityEngine.ConfigurableJoint ___servoJoint, UnityEngine.GameObject ___movingPartObject) (at <6d36e84722a243e9ba216b0d7c675f35>:0) (wrapper dynamic-method) Expansions.Serenity.BaseServo.Expansions.Serenity.BaseServo.RecurseCoordUpdate_Patch1(Expansions.Serenity.BaseServo,Part,Part) Expansions.Serenity.BaseServo.ApplyCoordsUpdate () (at <39c0323fb6b449a4aaf3465c00ed3c8d>:0) Expansions.Serenity.BaseServo.Update () (at <39c0323fb6b449a4aaf3465c00ed3c8d>:0) UnityEngine.DebugLogHandler:LogException(Exception, Object) ModuleManager.UnityLogHandle.InterceptLogHandler:LogException(Exception, Object) UnityEngine.Debug:CallOverridenDebugHandler(Exception, Object)
NOTE: I have KSPCF 1.12.2 installed.
I'm not sure if this is related or should be treated as its own issue. I have a rover attached to the bottom of my lander with a piston and a klaw. I saved the game with the piston partway extended so that the rover's wheels were touching the ground. When I loaded it, the klaw's relative position to the end of the piston was no longer correct so the rover was in the wrong position. It seems like on every save/load it gets further away.
persistent.txt
@JonnyOThan This is likely the same issue as the one someone just reported on the forums.
I changed something in KSPCF 1.12.2 to fix another bug in the RoboticsDrift patch involving rotation servos, but I forgot to check if that was working correctly with translation servos. Should hopefully be an easy fix. Sorry for the inconvenience.
Should be fixed in release 1.13.2
However, this won't correct the deformation that already happened on existing vessels, sorry for the inconvenience...
Well, glad it's fixed! I think I don't even need to pick the rover up again on this mission, so no inconvenience at all.