PVG coast fixes

[Nazfib]

This fixes two small issues that make coasts not work.

The first is that coasts complete immediately, caused by a sign error in MechJebModulePVGGlueBall.SetTarget. This causes the coast time-to-go to become 0 in the first guidance update during the coast.

The second issue causes PVG to immediately transition to FINISHED upon completion of the coast when CoastBeforeFlag is set.

The cause can be found in MechJebModuleGuidanceController.HandleTerminal. Suppose that vessel.currentStage == _ascentSettings.CoastStage, CoastBeforeFlag is set, and we're currently at the exact point where the coast ends. In the first tick after ignition, and before the next PVG optimizer update, IndexForKSPStage(vessel.currentStage) will return the index of the coast phase. The time-to-go in the coast phase is less than 10 s, and guidance will switch to terminal mode. Then, the Solution.TerminalGuidanceSatisfied call will compare the current state with that expected at the end of the coast, it will return true, and guidance will end.

The fix is to make IndexForKSPStage always return the index of the phase in which the stage is burning, not the coast phase (if that occurs first). Then, at ignition, time-to-go is the burn time of the stage and guidance will not switch to terminal mode.

The only other current use of this method is in MechJebModulePVGGlueBall.SetTarget. Here, it is used for a check to block the optimizer during staging events. Without the change, this blocks the optimizer for a few seconds around the end of the coast and ignition of the stage; with the change, it keeps running.

[lamont-granquist]

missed the e-mails about these, haven't had a chance to look at them yet, been taking a little mechjeb break after the last PR i got done, but they look very interesting.

[lamont-granquist]

I merged the first one via a cherry-pick to dev, it looks obviously correct (GH is being silly and not de-duplicating your commit though)

I need to think through the second one.

[Nazfib]

I merged the first one via a cherry-pick to dev, it looks obviously correct (GH is being silly and not de-duplicating your commit though)

I need to think through the second one.

Fair enough, it is a tricky issue. For reference, a fairly basic RP-1 Atlas-Centaur (the first stage is setup with separate tanks for PVG - let me know if you want a craft file) exhibits the problem. The setup is as follows:
The Centaur engines are in the same stage as the decoupler. "Coast stage" is set to the stage with the Centaur engines and the decoupler in it. "Fixed coast" is set to 10 s. "Coast before" is checked.

(With the first bug-fix, but without the second) on first-stage burnout, the throttle is set to zero and the decoupler is staged; the vessel coasts for 10 seconds; then PVG immediately goes to FINISHED.

With the second bug fixed, the Centaur engines ignite and PVG inserts correctly into orbit.

[lamont-granquist]

So from reading the code I'm thinking this bit in HandleTerminal looks kind of wrong to me:

            int solutionIndex = Solution.IndexForKSPStage(vessel.currentStage);
            if (solutionIndex < 0)
            {
                Done();
                return;
            }

I'm not sure that should be calling Done() and I think that may be even more problematic because if that code is hit for a KSP stage which isn't part of the Solution (stage with only decouplers or some solid ullage motor or whatever it is that makes the DeltaV window show a zero) then that code will finish the ascent right there.

That may explain some complaints that people have about it terminating in the middle of flight which I've found puzzling.

...

Oh I see, we've still got an issue though because if the Solution hasn't been updated by the optimizer and the next stage after the coast is the optimized coast we still need to do the terminal guidance check for that stage as well, but we ask for the kspStage and we get back the coast stage, and the coast stage is done. We're still carrying forwards that coast stage because we haven't gotten a new update, but we're looping through the whole list.

It might be better to fix that to loop only over stages which are in the future.

I think that'll work without any tick-to-tick problems because we fly coasts off of the Solution.

My only concern with that would be with something like residuals where we can be actually burning on a stage in the past long after the Solution has gone to the next stage (although with Coasts we always shut down the engine with ground-commanded shutdown before the coast now and waste the residuals and we always terminate the Coast based on the Solution so we know the Solution has accurate start/end times around a Coast).... I'm just trying to think through the implications of maybe doing this more widely in the Solution class.

I think we should probably either do that, or else just rename the method to be IndexForBurningKSPStage() or something. Gonna probably have to noodle on it a bit more though.

[lamont-granquist]

Okay I merged a slightly modified version of the CoastBefore fix and introduced a flag to control the behavior so that it preserves the suspension of the optimizer before the end of the coast, which I think I still want.

Let me know if I messed that up, I didn't actually test it.