Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
High level overview
4 new patches related to attitude control stuff :
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.Related changes :
New modding patch : BaseFieldListUseFieldHost, allow
BaseField
and related features (PAW controls, persistence...) to work when a customBaseField
is added to aBaseFieldList
(ie, aPart
orPartModule
) with ahost
instance other than theBaseFieldList
owner. Allow to dynamically add fields defined in other classes to aPart
orPartModule
. This patch is used to implement extra fields for the RCSLimiter patch.TODO
ITorqueProvider.GetPotentialTorque()
accordingly.Possibly :
GetPotentialTorque()
caching for ModuleRCS ?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" tweakableModuleRCS
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 forModuleControlSurface
and especially forModuleGimbal
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 yawpos
is the actuation induced torque for a positive FlightCtrlState (pitch = 1, roll, = 1 yaw = 1) control requestneg
is the actuation induced torque for a negative FlightCtrlState (pitch = -1, roll, = -1 yaw = -1) control requestModuleReactionWheel
, which is the most simple and straightforward and doesn't leave any room for interpretation.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.
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 totalavailable 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.
FixedUpdate()
, including the controlstate callbacks like
Vessel.OnFlyByWire
orVessel.On*AutopilotUpdate
. Calling them from theUpdate()
loop will resultin 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 :Review of how the stock implementations are handled in the modding ecosystem
pos
orneg
, it assume a negative value from either of theVector3
is a negative torque component (ie, ifpos.x
orneg.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.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.ModuleControlSurface
and consequently has a customGetPotentialTorque()
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()
andModuleLiftingSurface.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
andminlinearAlignement
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 customBaseField
is added to aBaseFieldList
(ie, aPart
orPartModule
) with a host instance other than theBaseFieldList
owner. This allow to dynamically add fields defined in another class to aPart
orPartModule
and to benefit from all the associated KSP sugar :Part
/PartModule
hosting theBaseFieldList
The whole thing seems actually designed with such a scenario in mind, but for some reason some
BaseField
andBaseFieldList
methods are using the
BaseFieldList.host
instance instead of theBaseField.host
instance (as for whyBaseFieldList
has ahost
at all, I've no idea and this seems to be a design oversight). There is little to no consistency in whichhost
reference is used, they are even sometimes mixed in the same method. For example,
BaseFieldList.Load()
usesBaseFieldList.host
in its main body, then callsBaseFieldList.SetOriginalValue()
which is relying onBaseField.host
.Changing every place where a
host
reference is acquired to ensure theBaseField.host
reference is used allow to use a customhost
instance, and shouldn't result in any behavior change. This being said, the stock code can theoretically allow a pluginto instantiate a custom
BaseField
with anull
host and have it kinda functional if that field is only used toSetValue()
/Getvalue()
and as long as the field isn't persistent and doesn't have any associatedUI_Control
. This feels like an extremelyimprobable scenario, so this is probably fine.