Do not let AI issue new orders after loading a game

[Morlic-fo]

This PR suggests one way to resolve #1837.

By storing the last played turn in the AIstate, we can detect if we already issued orders for a given turn (i.e. after loading the game). By default, the AI will not try to issue orders a second time in that case but use the orders in the savegame instead.

As discussed in #1837, for debugging work it is often useful to be able to load a savegame and replay the turn to check the effect of code changes. For this purpose, the flag replay_turn_after_load can be set in the AI config file.

[Dilvish-fo]

Here's my initial thoughts-- did you get around to looking into my request from that Issue,

Would you doublecheck if the use of the turn-begin autosave avoids this [primary] problem?

(I'll reiterate something I stated there, though, that even if the turn-begin autosave can solve the primary problem, there can still be some value to other handling like this.)

If the answer to that question is yes, then I would think that to add some real value this PR would need to include the discard-previous-orders aspect mentioned in the comments (either done automatically when replaying, or possibly subject to an additional option).

If the answer to that question was no (which would somewhat surprise me), then I suppose I'd see this add value even without the discard-previous-orders treatment, although I think I'd still prefer the discard to at least be an an option (and I'd probably want to understand better just why the turn-begin autosave did not actually solve the primary problem).

[Dilvish-fo]

Hmm, and it just struck me, I guess we don't currently have client-side support for just discarding a client's current orders (at least not that I saw). Although orders, at least some, have some ability to be undone. I'm not sure they all do, and that would probably be a messy process. We'd probably need to be able to split out the Orders().ApplyOrders(); step from AIClientApp.cpp to be separately triggered by the AI upon a savegame load (so the AI could simply not trigger them on its universe copy if it was going to discard them). @geoffthemedio any objections to that?

Also, the AI changes its own state about fleets and ships as it joins and splits those, so it would need to keep a copy of its own 'turn-start-state' to revert to in the even that it discarded orders.

It seems to me that during the steps of splitting fleets and whatnot, the client might request new object IDs from the server, and if those respective orders get thrown out then those object IDs would be wasted, but no actual harm should come from that. @geoffthemedio does that sound right to you?

that's all the extra issues that has come to mind so far...

[Dilvish-fo]

Hmm, looking into the bit about getting new object IDs for splitting fleets, it is looking to me (and seeming familiar) that we have changed that process so that there is no longer an advance request for the server to generate the object ID -- the client and server now each generate IDs as they process orders, done in a way so that they stay in sync but without letting any client know which other client is generating which other IDs. So just more reason not to need to worry about throwing away orders that had involved generating object IDs.

[Morlic-fo]

Would you doublecheck if the use of the turn-begin autosave avoids this [primary] problem?

Sorry for not mentioning this. Autosaves at beginning of the turn still seem to be after the AI python scripts. When I load a beginning of the turn autosave with this PR, I will see at the logs that the AI already played its turn.

Now that you mention it again, I forgot to doublecheck if the AI orders are actually also in that savegame or if there is some weird behavior where the AIstate is from after the turn but the orders are empty. I should probably also double-check what happens later in the game when AI turn take in the order of some seconds and if that influences the behavior (Is the AIClient even multithreaded?)

Also, the AI changes its own state about fleets and ships as it joins and splits those, so it would need to keep a copy of its own 'turn-start-state' to revert to in the even that it discarded orders.

Yes, we discussed this as option and consequence in the original issue. I figured that while the preferably solution would be clearing the issued orders (possibly optionally), it seems quite more complex and error-prone. What I came up with in this PR seemed simple and efficient enough as a stop-gap solution which could maybe even be included in the next release. Note I added a TODO in the relevant code pointing out the cleaner solution.

Unless you think we can get up the cleaner solution soon, I think the solution in this PR is better than the current state and easily reverted if something better comes along (but as mentioned above, I'll need to verify the correct behavior with autosaves at turn start more thoroughly).

[Dilvish-fo]

Looks pretty good to me, just a couple minor-ish requested changes

[Dilvish-fo]

please add check_bool to the import from option_tools, and then this line would be

    if not check_bool(get_option_dict().get('replay_turn_after_load', 'false')):

[Dilvish-fo]

Autosaves at beginning of the turn still seem to be after the AI python scripts. When I load a beginning of the turn autosave with this PR, I will see at the logs that the AI already played its turn.

OK, thanks. Looking into this, it appears to me that the 'turn start' autosave is triggered by the Human Client when it receives the TurnUpdate, which will have also at the same time triggered the AIs to generateOrders(). I think I have an idea on a modest change to this process which would allow the turn-start savegame to use the AIState prior to the AI order generation, which would be helpful for our development/debugging process. I've opened Issue #2108 for that and will try to get on that soon.

[Dilvish-fo]

Please add 'new' after 'Aborting'
Somehow this got left out of my review.

[Morlic-fo]

Fixed the missing check_bool call and missing "new" in logging statement.

[Dilvish-fo]

I think we should cherry pick this for 0.4.8. Although we've downgraded #1794 to optional after changing the related logging to warn level for the 0.4.8 branch, there is still some underlying bugginess going on, and the changes of this PR should make it so that by default those are at least reduced (and perhaps avoided entirely).

[Vezzra]

I think we should cherry pick this for 0.4.8.

Yep, it's a bugfix, as such it should go into 0.4.8. Done.