Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Attitude control overhaul #118

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Conversation

gotmachine
Copy link
Contributor

@gotmachine gotmachine commented Jan 30, 2023

High level overview

4 new patches related to attitude control stuff :

  • GetPotentialTorqueFixes [Bugfix] : Complete rewrite of the stock ITorqueProvider.GetPotentialTorque() implementations for the reaction wheels, rcs, gimbal and control surface stock partmodules. Fix various issues with the stock implementations, ranging from "minor" to "completely broken". Also add in-flight available torque readouts in the PAW for gimbals and control surfaces.
  • NoLiftInSpace [Perf] : Disable control surfaces updates and actuation when the part isn't submerged or in an atmosphere
  • RCSLimiter [QoL] : Add two extra tweakables in the RCS module "Actuation Toggles", giving the ability to define a separate angle threshold for linear and rotation actuation. This allow to optimize efficiency of multi-nozzle RCS parts that are impossible to fine-tune with only the actuation toggles. This also add a potential torque/force readout to the actuation toggles PAW items, both in editor and in flight.
  • BetterSAS [QoL] : Slightly improves the stock SAS precision and implement an optional alternate attitude controller called "PreciseController" (derived from the MechJeb PID controller), more stable and precise for in-space operations but not well suited for atmospheric operations.

Related changes :

  • Added an EditorPhysics support patch that provide in-editor data about CoM and reference part, as well as the state of the stock delta-v app situation selector. Used by the RCSLimiter patch to provide in-editor torque readouts.
  • Removed the ReactionWheelsPotentialTorque patch (superseded by the GetPotentialTorqueFixes patch)

New modding patch : BaseFieldListUseFieldHost, allow BaseField and related features (PAW controls, persistence...) to work when a custom BaseField is added to a BaseFieldList (ie, a Part or PartModule) with a host instance other than the BaseFieldList owner. Allow to dynamically add fields defined in other classes to a Part or PartModule. This patch is used to implement extra fields for the RCSLimiter patch.

TODO

  • The "PreciseController" alternative PID controller is just not reliable enough in atmospheric conditions. It should likely be auto-disabled based on a dynamic pressure threshold.
  • Coordination with MechJeb, kOS and TCA so they are made aware of the changes and adjust their usage of ITorqueProvider.GetPotentialTorque() accordingly.
  • Some beta-testing before pushing this to the masses

Possibly :

  • Implement GetPotentialTorque() caching for ModuleRCS ?
  • The gimbal / control surface PAW torque readouts could be made available in the editor (like the RCS ones).

In depth patches documentation

GetPotentialTorqueFixes

This patch is a rewrite of the stock implementations for ITorqueProvider.GetPotentialTorque(out Vector3 pos, out Vector3 neg).
All 4 of the stock implementations have various issues and are generally giving unreliable (not to say plain wrong) results.
Those issues (and reimplementation details) are further commented in code, but to summarize :

  • ModuleReactionWheel is mostly ok, its only issue is to ignore the state of "authority limiter" tweakable
  • ModuleRCS is giving entirely random results, and the stcok implementation just doesn't make any sense. Note that compared to other custom implementations (MechJeb, TCA, kOS), the KSPCF implementation account for the RCS module control scheme thrust clamping and the actual thrust power (instead of the theoretical maximum).
  • ModuleGimbal results are somewhat coherent, but their magnitude for pitch/yaw is wrong. They are underestimated for CoM-aligned engines and vastly overestimated for engines placed off-CoM-center.
  • ModuleControlSurface results are generally unreliable. Beside the fact that they can be randomly negative, the magnitude is usually wrong and inconsistent. Depending on part placement relative to the CoM, they can return zero available torque or being vastly overestimated. They also don't account for drag induced torque, and are almost entirely borked when a control surface is in the deployed state.

Note that the KSPCF GetPotentialTorque() implementations for ModuleControlSurface and especially for ModuleGimbal are more computationally intensive that the stock ones. Profiling a stock Dynawing with RCS enabled during ascent show a ~30% degradation when summing the vessel total available torque (~250 frames median : 0.31ms vs 0.24ms, frame time : 1.81% vs 1.46% ). Overall this feels acceptable, but this is still is a non-negligible impact that will likely be noticeable in some cases (ie,
atmospheric flight with a large vessel having many gimbaling engines and control surfaces). The implementations are pretty naive and could probably be vastly optimized by someone with a better understanding than me of the underlying math and physics. To mitigate the general overhead of those methods, a caching mechanism is implemented for the most performance intensive modules (gimbals and control surface). The general idea is to avoid recomputing the available torque unless there is a significant change in input parameters. Limited profiling show a significant reduction of the average per-frame cost, well below the stock level.

The KSPCF implementations follow these conventions :

  • x is pitch, y is roll, z is yaw
  • pos is the actuation induced torque for a positive FlightCtrlState (pitch = 1, roll, = 1 yaw = 1) control request
  • neg is the actuation induced torque for a negative FlightCtrlState (pitch = -1, roll, = -1 yaw = -1) control request
  • These conventions are taken from the output of the the stock implementation for ModuleReactionWheel, which is the most simple and straightforward and doesn't leave any room for interpretation.
  • Contrary to the stock implementations, values are strictly the actuation induced torque (ie, the torque difference
    between the neutral state and the actuated state). Especially in the case of ModuleGimbal, the stock implementation
    returns the actuation torque plus the eventual "structural" torque due to an eventual CoM/CoT misalignement.
  • Positive values mean actuation will induce a torque in the desired direction. Negatives values mean that actuation will
    induce a torque in the opposite direction. For example, a negative pos.x value mean that for a positive roll actuation
    (ctrlState.roll = 1), the torque provider will produce a torque inducing a negative roll, essentially reducing the total
    available torque in that direction. This can notably happen with the stock aero control surfaces, due to their control
    scheme being only based on their relative position/orientation to the vessel CoM and ignoring other factors like AoA.
  • Like the stock implementations, they will give reliable results only if called from FixedUpdate(), including the control
    state callbacks like Vessel.OnFlyByWire or Vessel.On*AutopilotUpdate. Calling them from the Update() loop will result
    in an out-of-sync CoM position being used, producing garbage results.

So in the context of the KSPCF patch, a correct implementation of a GetVesselPotentialTorque() method is :

foreach (ITorqueProvider torqueProvider)
{
  torqueProvider.GetPotentialTorque(out Vector3 pos, out Vector3 neg);
  vesselPosTorque += pos;
  vesselNegTorque += neg;
}
if (vesselPosTorque.x < 0f) vesselPosTorque.x = 0f;
if (vesselPosTorque.y < 0f) vesselPosTorque.y = 0f;
if (vesselPosTorque.z < 0f) vesselPosTorque.z = 0f;
if (vesselNegTorque.x < 0f) vesselNegTorque.x = 0f;
if (vesselNegTorque.y < 0f) vesselNegTorque.y = 0f;
if (vesselNegTorque.z < 0f) vesselNegTorque.z = 0f;

Review of how the stock implementations are handled in the modding ecosystem

  • It seems Mechjeb doesn't care about a value being from pos or neg, it assume a negative value from either of the Vector3 is a negative torque component (ie, if pos.x or neg.x is negative, it add that as negative available torque around x), see code ref 1, code ref 2. As it is, since MechJeb doesn't care for pos/neg and only consider the max, the patches will result in wrong values, but arguably since it reimplement RCS they will only be "different kind of wrong" for control surfaces and gimbals, and probably "less wrong" overall.
  • kOS assume that the absolute value should be used. (side note : kOS reimplements ModuleReactionWheel.GetPotentialTorque() to get around the authority limiter bug), code ref. The patches should apply mostly alright for kOS, at the exception of occasional negative values for gimbals and control surfaces being treated as positive, resulting in a higher available torque than what it should.
  • TCA doesn't seem aware of the possibility of negative values, it assume they are positive : code ref. The patches should more or less work for TCA, at the exception of negative gimbal/control surfaces values being treated incorrectly and the reaction wheels authority limiter being applied twice.
  • Atmospheric Autopilot replace the stock module implementation by its own and doesn't use the interface at all : code ref
  • FAR implements a replacement for ModuleControlSurface and consequently has a custom GetPotentialTorque() implementation. It seems that it will always return positive "pos" values and negative "neg" values : code ref, which doesn't align with the stock convention.

NoLiftInSpace

This is straightforward patch : it prevent ModuleControlSurface.FixedUpdate() and ModuleLiftingSurface.FixedUpdate() from running when the part is neither in atmo or submerged, preventing some expensive calculations from running and pointless actuation from happening. It also take care of putting control surface to their neutral position for visual consistency.

RCSLimiter

This patch implements two extra tweakables in the RCS module "Actuation Toggles" Part Action Window.

By default, ModuleRCS will scale down the actuation level of each nozzle depending on how far the thrust force is from the "ideal" angle for a given actuation request (unless the "always full action" toggle is enabled).

This patch gives the ability to define a separate angle threshold for linear and rotation actuation.
If the resulting angle from a nozzle thrust force is below that threshold, that nozzle won't fire at all instead of firing at a reduced level. This allow to optimize efficiency, especially in the case of multi-nozzle RCS parts that are impossible to fine-tune with only the actuation toggles.

The default angle limits can be defined in the ModuleRCS / ModuleRCSFX configuration by adding minRotationAlignement and minlinearAlignement fields (value in degrees). If they aren't defined, they default to 90° (no limit, behavior similar to stock).

To make RCS tweaking easier, the patch also add a potential torque/force readout to the actuation toggles PAW items. In the editor, the actuation orientation is defined by the first found command module, starting from the root part (matching the command module that will be selected as the control point when launching the ship). The readout also takes the RCS module ISP curve into account, and uses the currently selected body and state (sea level/altitude/vacuum) of the stock DeltaV app.

The modification to the RCS control scheme is taken into account by the custom KSPCF ModuleRCS.GetPotentialTorque() implementation. As of writing, all mods reimplement their own version of that method, and all of them are ignoring the stock control scheme anyway, so the behavior change introduced in this patch won't make a significant difference in most cases.
Note that RCSBuildAid tries to simulate the stock control scheme, but its implementation doesn't reproduce stock behavior correctly, which is why its torque readout doesn't always match the KSPCF one.

BetterSAS

This patch implement a small tweak to the stock PID attitude controller in how it takes the GetPotentialTorque() results into account to switch from the acceleration to the neutral and coasting phases. This notably prevent it from massively overshooting its target orientation when the vessel has asymmetrical torque capabilities. An example of such a vessel is the stock Dynawing which has very asymmetrical RCS torque capabilities.

The patch also implement an alternative attitude controller more or less copypasted from the MechJeb implementation. The user can switch between controllers with an additional button on the command modules PAW. This PID controller is much more precise and very well suited for in-space operations, reducing RCS fuel consumption massively compared to the stock implementation. However, it can struggle to reach stability, especially when massive external forces are involved, which is usually the case in atmospheric flight situations.

BaseFieldListUseFieldHost

This patch allow BaseField and associated features (PAW controls, persistence, etc) to work when a custom BaseField is added to a BaseFieldList (ie, a Part or PartModule) with a host instance other than the BaseFieldList owner. This allow to dynamically add fields defined in another class to a Part or PartModule and to benefit from all the associated KSP sugar :

  • PAW UI controls
  • Value and symmetry events
  • Automatic persistence on the Part/PartModule hosting the BaseFieldList

The whole thing seems actually designed with such a scenario in mind, but for some reason some BaseField and BaseFieldList
methods are using the BaseFieldList.host instance instead of the BaseField.host instance (as for why BaseFieldList has a
host at all, I've no idea and this seems to be a design oversight). There is little to no consistency in which host
reference is used, they are even sometimes mixed in the same method. For example, BaseFieldList.Load() uses BaseFieldList.host in its main body, then calls BaseFieldList.SetOriginalValue() which is relying on BaseField.host.

Changing every place where a host reference is acquired to ensure the BaseField.host reference is used allow to use a custom
host instance, and shouldn't result in any behavior change. This being said, the stock code can theoretically allow a plugin
to instantiate a custom BaseField with a null host and have it kinda functional if that field is only used to SetValue() /
Getvalue() and as long as the field isn't persistent and doesn't have any associated UI_Control. This feels like an extremely
improbable scenario, so this is probably fine.

- [BugFix] GetPotentialTorqueFixes : Rewrite of the stock ITorqueProvider.GetPotentialTorque() implementations for the reaction wheels, rcs, gimbal and control surface stock partmodules. Fix various issues with the stock implementations, ranging from "minor" to "completely broken". Also add in-flight available torque readouts in the PAW for gimbals and control surfaces.
- [Perf] NoLiftInSpace : Disable control surfaces updates and actuation when the part isn't submerged or in an atmosphere
- [QoL] RCSLimiter : Add two extra tweakables in the RCS module "Actuation Toggles", giving the ability to define a separate angle threshold for linear and rotation actuation. This allow to optimize efficiency of multi-nozzle RCS parts that are impossible to fine-tune with only the actuation toggles. This also add a potential torque/force readout to the actuation toggles PAW items, both in editor and in flight.
- [QoL] BetterSAS : Slightly improves the stock SAS precision and implement an optional alternate attitude controller called "PreciseController" (derived from the MechJeb PID controller), more stable and precise for in-space operations but not well suited for atmospheric operations.
- Added an EditorPhysics support class that provide in-editor data about CoM and reference part, as well as the state of the stock delta-v app situation selector. Used by the RCSLimiter patch to provide in-editor torque readouts.

New modding patch : BaseFieldListUseFieldHost, allow BaseField and related features (PAW controls, persistence...) to work when a custom BaseField is added to a BaseFieldList (ie, a Part or PartModule) with a host instance other than the BaseFieldList owner. Allow to dynamically add fields defined in other classes to a Part or PartModule.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

1 participant