KSPModdingLibs / KSPCommunityFixes

Community patches for bugs in the KSP codebase
49 stars 17 forks source link

Axis Groups have unsynchronized update interval, leading to erroneous averaging in KALs #129

Open HB-Stratos opened 1 year ago

HB-Stratos commented 1 year ago

KAL controllers in KSP have a rather useful property: if two KAL controllers simultaneously affect the same target value, their requested states get averaged. So if you have one input KAL requesting a play position of 5 on the output KAL, but a second input KAL demands 0, the output KAL play position will be 2.5. Note that this only works if both KALs are actually affecting the same value simultaneously, in other words they either need to have their play position actively moved, or be playing (even 0% play speed works).

The issue is that when the same setup (two KALs affecting the same output KAL) is attempted with one of those input KALs being assigned to an axis group (e.g. throttle), the average is incorrect, and jitters.

Here, the left KAL is set to a play position of zero. Note that it is proxied through the KAL below it as a playing KAL cannot have it's play position edited by a human. However, other KALs can. So only the bottom KAL is the one actually playing, while the top one is a proxy for human testing. On the other side there is a KAL of which the play position is affected by the throttle axis group. Both this throttle KAL and the playing Human KAL affect the play position of the output KAL linearly, so their average should be 2.5 with the human KAL set to zero, and the throttle at full. However, the average leans far more towards the throttle side, and jitters around, with frequency and amplitude seemingly affected by the amount of lag present. screenshot1047 My theory for why this happens is that the throttle axis group affects it's taget KAL at a lower frequency than the normally playing Human KAL, but that when a throttle update happens, it is overriding and ignores the averaging that should happen. Interestingly, it does not matter whether the throttle KAL is playing or not for this issue to occur.

Demo Craft: Axis Group Jitter Demo.craft.txt Usage: Load it in the hangar. You can see that both the proxying KAL on the left and the throttle KAL are playing. If you use the Human Input KAL to modify the play position of the proxying KAL, you can see the output update to reflect the new average between the Throttle and Human Input KAL. In the hangar there are no axis groups are in effect and everything works as expected. Spawn the Craft in and trottle up. As soon as you throttle up the needle should move to the average of zero (default value of the Human Input KAL) and the throttle position. However, you will see that the needle jitters and overshoots the middle point of 2.5 by quite a bit.

gotmachine commented 1 year ago

Ok, I only gave a quick look, but that issue has at least 2 causes.

First part of the issue is that when you add an axis group, that axis group start counting as an additional input toward the average calculation, so instead of doing (0 + 5) / 2, it does (0 + 5) / 3.

But as you noticed, on top of that, there is also some jittering. The reason is that axis group update are propagated in FixedUpdate (running at 50 ups), while the robotic controller logic runs from Update (running at whatever your framerate is). So depending on how the two cycles are interleaved, you sometimes end up with the extra axis group being registered 0, 1, or more times, which will end up in a random final position of 2.5, 1.66, 1.25, etc.

TBH, I don't have the courage to try to fix this. My guess would be that the axis groups shouldn't be ending being accounted for here, but maybe not and this is part of the design, and this side effect just wasn't accounted for in the whole thing design.

For future reference and anybody interested in fixing this, the place where the averaging is done is in RoboticControllerManager. UpdateFieldValues(). That method is in charge of processing a collection of AxisUpdate instances, and the "bad denominator" is already in that data (the values field), so one should likely look where the AxisUpdate.values field is populated and follow the breadcrumbs from there.

HB-Stratos commented 1 year ago

Hmmm interesting, cool to have someone with insight into the code have a look at this. the /3 averaging explains a lot of the weirdness and I think it'll save me a part when I correct my concorde reverse thrust and gate. Too bad I can't get rid of the jitter. Thinking about it, the KALs should really run in FixedUpdate too. I have run into issues with complex KAL machinery before where lag broke the system or caused glitches that should not be able to happen.