Ticket #42325

texai crash when loading game for second time

Open Date: 2021-05-20 05:25 Last Update: 2021-05-29 09:53

Reporter:
Owner:
Type:
Status:
Closed
Component:
MileStone:
Priority:
4
Severity:
5 - Medium
Resolution:
Fixed
File:
3

Details

Start the server with tex as default ai, load a game, then change your mind and load a different game (or the same game again)

5: AI*1 now under tex AI (1)
5: AI*2 now under tex AI (2)
5: AI*3 now under tex AI (3)
5: AI*4 now under tex AI (4)
5: AI*5 now under tex AI (5)
5: AI*5 no longer under threaded AI (4)
5: AI*4 no longer under threaded AI (3)
5: AI*3 no longer under threaded AI (2)
5: AI*2 no longer under threaded AI (1)
5: AI*1 no longer under threaded AI (0)
5: Hammurabi now under tex AI (1)
5: Cofresí now under tex AI (2)
5: Ur-Nanshe no longer under threaded AI (1)       <--- this is a human player
5: Hammurabi no longer under threaded AI (0)
5: Cofresí no longer under threaded AI (-1)

When a game is being deallocated, to load a new one, there's CALL_PLR_AI_FUNC(lost_control) from server_remove_player in plrhand.c

But that calls lost_control for the ai_type linked to the player, whether the player was actually AI-controlled or not.

Ticket History (3/13 Histories)

2021-05-20 05:25 Updated by: andrewmcg
  • New Ticket "texai crash when loading game for second time" created
2021-05-20 05:35 Updated by: andrewmcg
Comment

Patch checks in texai_control_lost() whether the player was actually AI controlled or not. We can't just check the flag, because player_set_under_human_control sets that to human before calling lost_control() for the AI. So it creates a list of players.

Being able to check in tex ai means we can gradually change it to ignore calls made to the AI for human-controlled players, until we can gain confidence that those calls aren't needed and fix it in CALL_PLR_AI_FUNC for all calls on all AIs (which would certainly break a lot of stuff now).

I'm not sure about the merits of keeping texai state data in the exthrai object, as opposed to extending the ai_type->private object. I guess the argument is it's easier to ensure that accesses are thread safe if it's not exposed outside of texaiplayer.c (but the new controlled_players list is only accessed from the main thread anyway)

2021-05-20 05:45 Updated by: cazfi
Comment

Shouldn't "lost_control" be opposite of "gained_control", and as such be called (only) when player formerly under AI control is no longer under AI control?

2021-05-20 15:48 Updated by: andrewmcg
Comment

Reply To cazfi

Shouldn't "lost_control" be opposite of "gained_control", and as such be called (only) when player formerly under AI control is no longer under AI control?

Yes, but all the CALL_PLR_AI_FUNC calls should be called only for players under AI control. My thinking is if I work on tex ai to make sure it works correctly that way, then we'll be that much closer to understanding what needs to be done to classic ai, and then we can fix CALL_PLR_AI_FUNC.

Adding the controlled_players list to texai is not the only way, but it's sensible information for the AI module to have -- really I think AI needs to keep a lot more state to be more effective.

If you do want a simpler fix, I'd say move set_as_human() to the end of player_set_under_human_control, rather than the beginning,and then texai_control_lost can just check is_human() instead of needing the list. It maybe makes more sense to call lost_control() as the last thing before switching the flag on the player, rather than doing it after.

2021-05-20 19:24 Updated by: cazfi
Comment

Changing CALL_PLR_AI_FUNC() to be called only for ai-controlled players is a future API change, but isn't the fact that lost_control is called when control was not actually lost a bug even under current API?

2021-05-27 02:20 Updated by: andrewmcg
Comment

OK, here's a patch to just guard it from calling

2021-05-27 05:51 Updated by: cazfi
Comment

The patch does not apply to S2_6 that has no tex. Is that branch affected by the general API bug? If yes, can you make a patch?

2021-05-27 17:23 Updated by: andrewmcg
Comment

It does affect S2_6 if enable-ai-static=threaded . This fixes it but I think there are a lot of other problems if you do that.

2021-05-27 17:35 Updated by: cazfi
  • Resolution Update from None to Accepted
  • Milestone Update from (None) to 2.6.5 (closed)
  • Component Update from (None) to AI
2021-05-29 09:53 Updated by: cazfi
  • Status Update from Open to Closed
  • Owner Update from (None) to cazfi
  • Resolution Update from Accepted to Fixed

Attachment File List

Edit

You are not logged in. I you are not logged in, your comment will be treated as an anonymous post. » Login