-
Notifications
You must be signed in to change notification settings - Fork 16
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
Update IAS definition #34
base: master
Are you sure you want to change the base?
Conversation
Was previously EAS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooof, indeed, heavy code.
|
||
public static float mps2Ias(float mps, Vessel v) | ||
{ | ||
float M = (float)(mps / v.speedOfSound); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For every division, sqrt and other possible NaN-poisoning operations, please check that we don't fall flat.
Vacuum or some other conditions may break the new code - you should probably handle it just in case.
float M = (float)(mps / v.speedOfSound); | ||
float gamma = (float) v.mainBody.atmosphereAdiabaticIndex; | ||
float pressureRatio; | ||
if (M <= 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the transition smooth/continuous? If not, why? And it may need a filter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it is. At M = 1 both subsonic and supersonic pressure equals to Pow((gamma + 1) / 2, gamma / (gamma - 1)). Ferram used a simplified version of the Rayleigh formula and achieved both simpleness and continuity.
|
||
float soundPres = Mathf.Pow((gamma + 1) / 2, gamma / (gamma - 1)); ; | ||
|
||
if (pressure <= soundPres) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same question: is the transition smooth/continuous?
m2 = mnew; | ||
pt1 = pt2; | ||
pt2 = rayleighPitotTubeStagPressureSuperSonic(m2, gamma) - pressure; | ||
} while (Mathf.Abs(m2 - m1) > machEpsilon); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's put a limit to iteration count and print Error to log if we reach it.
Frankly, not a fan of such heavy math in our fixedUpdate =(
Lot's of cycles for such a petty formularity.
Does the method always converge?
Can we replace it by some precalculated mesh interpolation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds bold but I suggest using IAS alone for setpoint. Here's my 2 cents:
- IRL aircrafts never knows their true speed against ground (and static air) hence IAS is their only choise to read & hold (along with mach at higher altitude, which is also calculated from IAS);
- As said before, IAS plays an important role in aerodynamic and maneuverability;
- All other speed modes in AA, can be converted to IAS with O(1) algorithms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1). I never intended to constrain AAP to what IRL aircraft knows. I used every easily-accessible information the game gave me.
2). ...in FAR. Maybe we should only use these formulas when FAR is installed? There already exists FAR-specific code in AAP, so that would be easy to code and definitely easy my mind.
3). Nah, let's keep it in kinematic speed. Simple is good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I came with another possible solution.
I've found a way to read IAS value directly from FAR so we may ditch the calculation code.
Then, I want to make another change: we determine the airspeed differencies from inside the Prograde Thrust Controller and ditch the need to call mps() method outside the class. So we can apply thrust controls without the need of the IAS reverse functions. It will also make PTC a StateController rather than a SISOController.
If FAR is not found, we can then use the good old EAS code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good! If you feel comfortable with runtime reflections (to call FAR code without depending on it), that would be the simplest solution, IMO.
This reverts commit 1a909ac.
This version of IAS def uses reflection to read IAS value from FAR API, and vanilla Vessel.indicatedAirSpeed() method for stock aerodynamic. Secant method is still required for IAS->mps conversions but only invoked when explicitly requested (and NOT per-frame). Prograde Thrust Controller refactored to allow two airspeed reference systems: kinematic-based (surface speed) and pressure-based (indicated airspeed). Also added option to turn off Speed Control if throttle full / cut out key is pressed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May cause bonus despair, but the problems are not that small to ignore.
} | ||
|
||
// Load ActiveVesselIAS method from FARAPI | ||
var activeVesselIASMethodInfo = ReflectionUtils.GetMethodInfo( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Active vessel only? What happens if you fly in formation and switch to another vessel? The autopilot of the first vessel must be autonomous.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another method that accept Vessel as a parameter is also included in the FAR API. I'll apply and test that one.
THB TBH I don't know AAP works on non-focused vessel. 😂
(wtf Apple autocorrections)
@@ -403,13 +628,16 @@ float solve_thrust_req(double required_thrust, float prev_throttle) | |||
|
|||
#region GUI | |||
|
|||
static readonly string[] spd_str_arr = new string[] { "OFF", "ms", "kts", "M", "ias", "kias" }; | |||
string[] spd_str_arr = new string[] { "m/s", "kts", "Mach", "IAS", "kIAS" }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spd_type_names
/// <summary> | ||
/// Base class of sub state controllers that is called by main state controllers | ||
/// </summary> | ||
public abstract class SubStateController : AutopilotModule |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need another base class. StateController is fine as base for ProgradeThrust...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be trouble only if it was sealed: then it would get caught into autopilot_module_types and registered as top-level autopilot.
/// If converting between systems, use convert() instead. | ||
/// </summary> | ||
/// <returns>Speed in m/s</returns> | ||
public float sameSystemMps() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
currentSystemMps
public static class SpeedTypeMethods | ||
{ | ||
// SpeedType classification | ||
public static bool isKinematicSystem(this SpeedType t) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if you would go one step further and not treat this as bool, the LoC would reduce. We actually already have three unit systems (where units in the same system are linearly convertible between eachother):
kinematic (mps, kts),
instrumental (ias, kias),
sound (mach)
Old code was treating kinematic mps as lingua franca and ran all conversions through it. That is why the old code was 'switch' and not 'if' based. Can you try and separate unit system to some tangible named entity? Either another (maybe, private) enum or class hierarchy...
Basically, my point is, current code looks a bit ugly because the pressure/kinematic condition is too prevalent in the interface and forces you to write a lot of if clauses. And you can't extend it to new unit systems easily. What if you wanted to support both IAS and EAS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
current code looks a bit ugly because the pressure/kinematic condition is too prevalent in the interface and forces you to write a lot of if clauses
Yes and I already have trouble naming functions. (e.g. "sameSystemMps" is used to avoid confusion like "currentSpeedInCurrentSystem"😂)
Actually this commit is meant to be a temporary fix and I'm still testing with it (in my actual gameplay :P). Currently it doesn't work well in IAS mode when climbing/descending (changing in dyn. pressure must be taken into account for acceleration prediction) and I'm still looking forward to another refactoring (v3 perhaps).
Please ignore my commit on the FLC branch. Changes on that branch is meaningless until we settle down a solution on this branch. (Whew great it seems that I "forgot" to commit them.)
Oops sorry I didn't know committing on my own branch will also wake this PR up. 🤣 So no document (yet) because I'm not actually ready to file changes to the public. |
Hi Boris!
I'm 2Fat2Fly on KSP forum (previously Fat_Bird).
First of all thanks for your amazing addon which is still the best among the autopilot plugins for KSP IMO!
I've picked up KSP again recently and now I'm comming back to try to solve several issues mentioned several years ago. I've split the changes to several branches to better describe them one by one. This being the first of them.
Indicated Airspeed (IAS) Definition Update
The issue
In the previous code, the "IAS" was defined using true airspeed (ground speed) against atmosphere density:
AtmosphereAutopilot/AtmosphereAutopilot/Modules/ProgradeThrustController.cs
Line 64 in 6d21fdb
Of which is actualy equivalent airspeed (EAS) by definition.
However, as quoted from the IAS Wikipedia page:
So I think it is necessary to tell the autopilot to stick to a certain IAS, instead of a certain EAS.
Solution
In the Prograde Thrust Controller module, I replicated the FARc's code to calculate IAS, as below:
https://github.com/T2Fat2Fly/AtmosphereAutopilot/blob/1a909acf1974c5f59ba366451969645d80ea1655/AtmosphereAutopilot/Modules/ProgradeThrustController.cs#L68
But since the code have nothing to do with FARc, it should work with stock aerodynamic as well.
Since the PTC module uses mps (ground speed) as setpoint for thrust control, a reverse function is also included, which involved secant method for the supersonic part. Luckily it seems not to have a significant performance hit in game.
Result
FAR stock aircraft FAR Firehound MS used as example. It seems that a good consistency with the FARc IAS is established, both subsonically and supersonically: