MechJeb2

MechJeb2

4M Downloads

MJ (2.6.0 master) + RealismOverhaul, RealFuels: Engines w/Ullage requirements do not calculate node burn length, countdown correctly

DRVeyl opened this issue ยท 14 comments

commented

Running MJ 2.6.0 (master, from CKAN), RealismOverhaul latest release and its many required additional mods. Specifically with RealFuels and its support for engines requiring ullage.

If the engine requires ullage and the propellant state is not "Very Stable" then MJ will calculate an infinte burn time required, and thus an infinite burn countdown. This results in MJ automatically beginning to execute the next node as soon as it the autopilot is engaged, instead of determining the "correct" burn time and burn countdown if the engine were able to perform and waiting/warping accordingly.

If the fuel state is "Very Stable" or the engine does not require ullage, there is no issue.
If the engine requiring ullage is in any future stage and has not been activated, there is no issue.
If the engine requiring ullage is in a future stage but has been de-activated, there is no issue.

This can be re-created by launching a craft to orbit in RO. Activate a stage with an engine requiring ullage (such as the AJ-10 line). Turn off autopilot and create a maneuver node in the future. MJ "Maneuver Node Info" panel displays for "Node Burn Length" and "Node Burn Countdown" will read correct when the engine's fuel state is "Very Stable" and will read "Inf" when the fuel state is anything else. Link to screenshots: http://imgur.com/a/vTzAh User can switch between the two conditions indefinitely by iteratively ullaging and then allowing the fuel to unsettle.

Repeatable workaround: Disable the engine, create a new future stage, and move the engine to that future stage. This causes MJ to correctly calculate the burn duration and countdowns and behave as expected.

I don't know if this is better addressed in a RealFuels or MJ issue. I'm not sure if this is something in RO/RF (ModuleEnginesRF.cs?) causing the engine to have 0 thrust/ISP, if RO/RF is doing this only for engines in the current stage, or if MJ's calculation for future stages is different from its current-stage calculation. Did not see this in KSP 1.1.3, have always seen this behavior as of 1.2.2.

commented

No, this is definitely an MJ issue. I noticed it myself and am currently tracing it through the code. Once I find the problem I'll submit a pull request to fix it.

My observation is that it only occurs if the option to 'Prevent unstable ignition' is enabled. So an easier workaround is to:

  • Disable 'Prevent Unstable ignition' in Utilities (or in any custom window that you add the option to)
  • Execute the node after the 'Node Burn Countdown' displays a proper finite value
  • If Auto Warp is enabled you may now re-enable 'Prevent unstable ignition' if you want. (I have not observed the countdown to go back to 'Inf' if there is a warp in progress)
commented

@DRVeyl have you tried the latest dev version? Because I can't repro this any more after downloading the latest one: https://ksp.sarbian.com/jenkins/job/MechJeb2-Dev/

commented

There was a similar bug in the past that occurred whenever your current engine had not actually been fired. For example, you staged to a new engine on orbit, but didn't actually light it. When MJ was given the command to execute next node, it would instantly fire the engine because it didn't know how much thrust it had (having not seen it used) so it would try to light it for a quick test. Obviously didn't work well with RO engines though.

Not sure if it is the same bug rearing its head again or not, but that was what happened in the past.

commented

I'm still experiencing this

commented

Yeah I've seen it again since. The notion though that it's necessary to fire the engines to see how much thrust they have is pretty weird. The engines themselves already have that information and it's an easy matter to look that data up, even on derived engines like RF has.

Otherwise MJ couldn't display that information in the Vessel Info window. Or the Delta V window

commented

Here is that original issue in case it is related in any way:
#740

It might have had nothing to do with determining thrust, that was just my guess as to what was happening.

commented

As of build 689 (MechJeb2 v2.5.1.0 / vDev #689 Sarbian / v2.6.0.0), yes.
Just tried build 690. Behavior seems slightly different? I loaded the same craft that I used for my original test (in the screenshots.)

Activated engine with unstable propellent in the current stage: Burn time is inf
Shutdown engine with unstable propellent in the current stage: Burn time is correct. New?
Activated engine with unstable propellent in future stage: Burn time is inf
Shutdown engine with unstable propellent in future stage: Burn time is correct.

If the propellent is stable, the burn time is correct no matter the engine activation or stage.

I have not tried changing the 'Prevent Unstable Ignition' setting.

commented

@jwvanderbeck Looking at the fix involved, I'd have to say no. Mostly it seems to be a logic fix in determining when it's time to burn and modifying the simulation to be more thread friendly.

commented

@jwvanderbeck looking at this again and while I still don't think that specific change caused this, I DO think the particular bit of code you referenced IS responsible. What I think is happening here is that in one place, when ullage instability is detected it sets throttleLimit to 0. That in turn affects other calculations where it's ending up dividing by zero. Dividing 0.0D results in Infinity.

So when it's doing

if (halfBurnTime > 0 && timeToNode < halfBurnTime)
{
    ... stuff where it initiates burns
}

halfBurnTime is Infinity so it's always greater than 0 and timeToNode is always less than Infinity. (that last one is why I say that #740 's commit didn't cause this, because it was already doing the second check)

The problem is, I'm not sure what the solution is. I can think of a few but they revolve around NOT setting throttleLimit to 0, and that would mean that MJ would try to throttle up instead of using RCS. There's a reason why it's limiting it. So going that route means adding some other special handling later in MechJebModuleNodeExecutor? Maybe just before ThrustForDV()? Or IN that method?

Pinging @sarbian here: Do you have any ideas as to how you think it ought to be handled? I don't mind doing the code for it I'm just not sure what the right way to go is.

commented

Ok, I coded up some fixes and am doing a pull request. (I did one already but I screwed up and sent it to Master instead of Dev)

Summary: It checks if throttle is limited due to unstable ignition and applies a throttle limit multiplier based only on manual throttle limits but is still technically throttled to 0% so that the engine cannot fire. In other words it only affects the burn time countdown. It's still possible to end up with Infinity values but now it will probably be a PEBCAK issue. It'll still try to compensate by using node.UT - vesselState.time as an countdown timer but a manual throttle limit of 0% is the final authority. (though I did also make it turn the text red as a subtle warning)

commented

Pull request is here: #882

commented

this was probably more due to dd3a656

"targetThrottle" gets set by other mechjeb controllers (like ascentcontroller) and they will de-register themselves from the thrust controller but leave the targetThrottle at 1.0 -- the "prevent unstable ignitions" code is supposed to only engage if the crash is (trying to) really throttle up -- but the check there was see "targetThrottle > 1.0" even though mainThrottle was 0.0

With that fix while the node executor is not throttling up the ullage code in the throttle controller isn't exercised so the burntime calc should be okay before the burn now.

I think there would still be an issue though that as the node executor throttled up, then the limiter would kick on, then it would get 0.0 limited throttle and infinite burntime though until the propellant stabilized.

should probably put more of this logic in the throttle controller so that the node executor didn't have to know so much about ullage.... working on a patch...

commented

is anyone still seeing this after #882 / 2.6.0.0-694?

commented

@sarbian think this can be closed