TestFlightFailure_IgnitionFail never occurs

[Starstrider42]

While I have seen all other engine/gimbal failures at least once in testing, I'm unable to trigger an ignition failure.

The bug can be reproduced in KSP 1.2.2, with ModuleManager 2.7.6, TestFlight 1.8.0.1, and this config file instead of the default stock ones. Small changes to the config, such as adding/removing a weight for IgnitionFail (it should be absent, right?), fleshing out baseIgnitionChance, or reducing the ignition chance to 0.1, do not change the behavior. I started a sandbox game, cheated the Orbiter 1A into orbit, and alternated between full and zero throttle at various intervals. Eventually an engine would shut down, but they never failed to ignite.

I see no IgnitionFail output in the log, even with debug logging enabled, so from comparison to TestFlightFailure_IgnitionFail.cs I suspect TestFlight is not detecting the ignitions in the first place.

[Starstrider42]

I've tracked down the problem: TestFlightFailure_IgnitionFail.OnUpdate queries TestFlightFailure_IgnitionFail.TestFlightEnabled to see if it should run. For the default configuration (i.e., just the internal part name), this always returns false. Ditto for a configuration that's just an alias name, as used by RO.

Unfortunately, I have a very limited understanding of TestFlight configurations, so I'm not sure how to proceed. Does TestFlightUtil.EvaluateQuery need to handle single-word configs? Should TestFlightFailureBase.Configuration always evaluate to a qualifier = value query, resolving aliases as needed? Are TestFlightFailure_Engine's and IgnitionFail's redefinitions of TestFlightEnabled obsolete? @jwvanderbeck, @NathanKell, or @anxcon, do you know what the best (least disruptive) fix is?

[jwvanderbeck]

The query system is.. complicated... and I can barely remember all the intricacies. As I recall though while at one time the "configuration" property was optional, it is now required. It should default to the kspPartName though according to this commit f9b4da1:

• CHANGE: Internally, any empty configuration string is coerced into kspPartname = squadFoo using the new interop value and the parsed part name.

Maybe try explicitly setting it in the config and see if it makes a difference? Maybe the default handling of a blank property is broken?

[Starstrider42]

Oh dear, TestFlightEnabled is a catch-22. Configuring as follows (abridged for clarity):

MODULE
{
	name = TestFlightCore
	configuration = kspPartName = liquidEngine3:LV-909
}
MODULE
{
	name = TestFlightFailure_IgnitionFail
	configuration = kspPartName = liquidEngine3:LV-909
}

lets TestFlightUtil.EvaluateQuery pass, but TestFlightEnabled = false because the failure module does not have a core. On the other hand,

MODULE
{
	name = TestFlightCore
	configuration = kspPartName = liquidEngine3:LV-909
}
MODULE
{
	name = TestFlightFailure_IgnitionFail
	configuration = LV-909
}

can find the core, but TestFlightUtil.EvaluateQuery fails. The second config is similar to RO's usage, so I'd assume it's what is supposed to be supported. Behavior is unchanged if I make the alias the same as the part name.

[jwvanderbeck]

You want to alias it. Set up an alias in the core, then use that alias in the other modules. For some reason that doesn't seem to be in the docs however, so let me see if I can find it.

[Starstrider42]

Isn't that my second example? It fails because EvaluateQuery tries to parse the alias as a query.

[jwvanderbeck]

Yeah it is sorry didn't look at it right.. Getting late here.

[jwvanderbeck]

Your second example looks correct to me and the system is universal across modules, so it seems odd it would be breaking in just this one module. Maybe it isn't binding to the engine?

[Starstrider42]

I think the binding is fine -- IgnitionFail has a non-null engines, though I haven't actually checked the list contents.

It almost makes sense that only this module breaks. TestFlightFailure_Engine and TestFlightFailure_IgnitionFail are the only modules that override TestFlightEnabled to use EvaluateQuery. These are also the only calls to EvaluateQuery outside of TestFlightCore.ActiveConfiguration. In addition, IgnitionFail is the only engine failure module to explicitly test TestFlightEnabled (it's also tested in TestFlightUtil.GetFailureModules, but C# can be weirdly inconsistent about polymorphism).

EDIT: weirdness confirmed. In TestFlightFailure_EnginePerformanceLoss, TestFlightUtil.GetFailureModules calls TestFlightFailureBase.TestFlightEnabled rather than TestFlightFailure_Engine.TestFlightEnabled. So the latter isn't actually used anywhere!

So if I modify TestFlightFailure_Engine.TestFlightEnabled and TestFlightFailure_IgnitionFail.TestFlightEnabled to call core.ActiveConfiguration where they now use EvaluateQuery, should that break anything?

[jwvanderbeck]

The performance loss, thrust loss, and shutdown modules are all good and work regularly. So I would use them as a basis to determine. Its possible IgnitionFail never got updated to the newer system.

[jwvanderbeck]

O"k looking at those other three, none of them override anything in the specific failure. They leave the Enabled check to the base engine module. so it looks like the overriding of that check in ignition fail is old and needs to be removed. Basically make it work the same as the other three failure modules.

[jwvanderbeck]

Hmm actually might be more complicated than what I just said. I need to look into it a bit more. IgnitionFail is kind of a special failure as it isn't a called failure that can occur at any time based on reliability rolls. It is instead a triggered failure that can only occur at specific times, IE when you go to ignite the engine. That is why it is set up differently.

[jwvanderbeck]

So try this. Remove the override of TestFlightEnabled that is in TestFlightFailure_IgnitionFail and see how that works. It should be that simple really.

[Starstrider42]

That change alone doesn't fix it; it IgnitionFail just calls TestFlightFailure_Engine.TestFlightEnabled, which has the same problem. I went ahead and also replaced the call to EvaluateQuery (in TestFlightFailure_Engine) and it seems to work, though I'd want to test it more thoroughly for side effects.

[jwvanderbeck]

That doesn't make sense because all the other engine related failures work fine and they use the one in the engine module.

[Starstrider42]

No, they don't. I verified with debug statements that the normal failure checks call TestFlightFailureBase.TestFlightEnabled.

[Starstrider42]

Aha, TestFlightEnabled was never declared virtual. It looks like the other top-level modules follow the same pattern of masking instead of overriding properties. Opened as #173, as this is a potentially far-reaching question.

[ts826848]

Looks like something may still be broken after the fix. I get ignition failures now, but well over 3/4 of my attempted launches of an Aerobee result in ignition failures, which feels way too high to me.