Principia node execution is erroneous with a multi-manœuvre flight plan
eggrobin opened this issue · 2 comments
MechJeb Version
Dev ≥ 958.
KSP Version
1.9.1 or 1.8.1.
Description
Principia node execution is incorrect on a flight plan with multiple manœuvres; the executor fails to detect the end of the first manœuvre, and reorients itself to the second without shutting down the engine.
Replication Case
Any Principia flight plan with more than one manœuvre, both with nonzero Δv, and with some coasting time between the manœuvres, will lead to this misbehaviour.
KSP.log
Irrelevant.
Discord discussion with @lamont-granquist
[01:47] egg: @lamont-granquist: when the Principia node jumps to the future, the current manœuvre is done, and burning must stop; does the implementation do that, or does it keep burning regardless of a change in the initial time?
[01:48] lamont: i did not write the implementation
[01:48] egg: you reviewed it though :-p
[01:51] lamont: i don't really understand what it does yet
[01:51] egg: the condition (!double.IsInfinity(halfBurnTime) && halfBurnTime > 0 && timeToNode <= 0.0225) || timeToNode is a bit mystifying
[01:51] lamont: yep
[01:51] lamont: i was looking at that
[01:52] lamont: i think that is "when the principia node jumps to the future"?
[01:53] lamont: or not because that's the start of execution
[01:53] lamont: it seems to be using the same shutdown principle as normal nodes, and AFAIK that shouldn't work
[01:53] egg: yeah
[01:53] egg: it happens to work when there is only one manœuvre, because then we just kill the node
[01:54] lpg: even that will only work well with engines that have instant ignition
[01:54] egg: so I guess around line 145 if the node starts in the future you need to untrigger it?
[01:54] lamont: yeah if you kill the node then the node executor shuts down
[01:54] lamont: no, because that's the entry condition
[01:55] egg: is there a mechanism to kill the eggsecutor?
[01:56] lamont:MechJeb2/MechJeb2/MechJebModuleNodeExecutor.cs
Lines 102 to 125 in 333745d
[01:56] lamont: that is its normal method of exiting execution
[01:57] egg: OK so for one principia node that should be "if the node is in the future, untrigger & abort"
[01:57] egg: and if you wanted an "execute the whole flight plan" command, that would simply be "if the node is in the future, untrigger"
[01:57] egg: (and it will retrigger itself on the next node)
[01:58] lamont: i sort of think the behavior of ONE_NODE needs to be the principia behavior if princpia is loaded
[01:58] egg: (executing the whole flight plan in open loop is questionable, but might be useful for simple cases I suppose)
[01:58] lamont: and you can fall into the normal ALL_NODES handling then to handle multiple principia nodes
[01:59] egg: nah, normal all_nodes will fail just as much as normal one_node
[01:59] egg: by virtue of never entering the if statement you linked
[01:59] egg: because dVLeft will always be the whole Δv of the next node
[02:00] lamont: no but if you change 102 to be principia-aware and look at maneuver-node-in-the-future that takes care of it
[02:00] egg: ah, right
[02:00] egg: node-in-the-future is a harmless condition for stock too
[02:01] lamont: not really though? you can fly past a node in stock and still execute it because stock is insane.
[02:01] egg: oh right you start before the node in stock
[02:01] egg: so things start with the node in the future
[02:01] lamont: yeah and then it burns down weirdly and i think it can absolutely wind up in the past under certain circumstances
[02:02] egg: so then, line 102 needs to check node-in-the-future if Principia, and 108 needs to work for one pnode
[02:02] egg: and then it kind of falls into place I thnik?
[02:02] lamont: yeah
[02:02] lamont:
[02:02] lamont: i'm a bit busy right this second tho
[02:03] egg: hah
The change in 958 also seems to have broken something in the normal stock node execution in the Rendezvous Autopilot where the nodes are now planned but no longer execute.
