Merge lp:~widelands-dev/widelands/bug-1802629-territorial-crash into lp:widelands
- bug-1802629-territorial-crash
- Merge into trunk
Status: | Merged | ||||||||
---|---|---|---|---|---|---|---|---|---|
Merged at revision: | 8954 | ||||||||
Proposed branch: | lp:~widelands-dev/widelands/bug-1802629-territorial-crash | ||||||||
Merge into: | lp:widelands | ||||||||
Diff against target: |
118 lines (+21/-13) 3 files modified
data/scripting/win_conditions/territorial_functions.lua (+10/-6) data/scripting/win_conditions/territorial_lord.lua (+6/-3) data/scripting/win_conditions/territorial_time.lua (+5/-4) |
||||||||
To merge this branch: | bzr merge lp:~widelands-dev/widelands/bug-1802629-territorial-crash | ||||||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
GunChleoc | code | Approve | |
hessenfarmer | Needs Resubmitting | ||
Review via email: mp+359227@code.launchpad.net |
Commit message
In territorial win conditions, check if player still exists before making it the winning player. Player will also win if there are no other players left.
Description of the change
bunnybot (widelandsofficial) wrote : | # |
hessenfarmer (stephan-lutz) wrote : | # |
Ok did some playtesting. Won a territorial lord match which was quite fine all messages were as excepted.
played a territorial time as well. Intentionally I lost the game around 3 hours. Game lost message was in the message box. with the message to be given at 3:30h I got the following error while only having 1 sentry left:
[../src/
GunChleoc (gunchleoc) wrote : | # |
Should be fixed now
hessenfarmer (stephan-lutz) wrote : | # |
ok. I will test this tonight.
But this I somehow weird, cause I wasn't anytime the winning player in the whole game which crashed.
hessenfarmer (stephan-lutz) wrote : | # |
good news is it doesn't crash anymore.
Bad news ia it doesn't end anymore (at least not after 4 hours).
I suspect the check for defeated player function in multiple routines removing all players from the players table.
perhaps we should remove them from all functions and see what happens.
hessenfarmer (stephan-lutz) wrote : | # |
I think I know what is going on why it doesn't end. The game over message is probably correctly triggered, but the message get sent only to the Players remaining in the players table. as we check for player defeated the defeated players are removed from the players table and don't get the message anymore.
so we could try to use wl.Game().players instead in the game over message
hessenfarmer (stephan-lutz) wrote : | # |
did some testing
I probably found a solution and will test it tonight.
If testing goes well I will push a new revision of the branch
hessenfarmer (stephan-lutz) wrote : | # |
ok I fixed all bugs in both win conditions.
the trick was to always calculate the score for all players and only report result for every player when the game is finished, while using the Game players (plrs) to count if only one opponent is left (same as autocrat)
this leads to the behaviour that always the game end time reported for all players losing the game, as it is only finally lost when it is over.
furthermore in a multiplayer setup it might be possible to win while defeated, but this is how the win condition should be in my eyes. If this is not wanted it could be changed easily.
GunChleoc (gunchleoc) wrote : | # |
Code LGTM - we should test again to see if we can still reproduce a crash
hessenfarmer (stephan-lutz) wrote : | # |
I have tested territorial time intensively
I won on crater (2 players) - everything ok.
I lost after 4 hours on crater - everything ok.
I lost by P2 winning - Game ended after p2 (AI) had more than 50% of the map plus 20 minutes.
I lost by loosing my HQ - Game ended due to just one player remaining
I tested even a 3 player setup on two frontiers I lost my HQ and the game ended after one of the AI players had more than 50% of the map plus 20 minutes.
The only thing in the current setup is that the Game status is reported at the end so my early loss in two frontiers was not reported. Theoretically it is possible as well to win while defeated by still holding more than 50 % of the map, unless there is only one player left.
I was not able to provoke a crash even though I removed most of the safety nets introduced in previous versions of this branch.
GunChleoc (gunchleoc) wrote : | # |
Tested and working, thanks!
@bunnybot merge
bunnybot (widelandsofficial) wrote : | # |
Continuous integration builds have changed state:
Travis build 4326. State: failed. Details: https:/
Appveyor build 4121. State: success. Details: https:/
bunnybot (widelandsofficial) wrote : | # |
Refusing to merge, since Travis is not green. Use @bunnybot merge force for merging anyways.
Travis build 4326. State: failed. Details: https:/
bunnybot (widelandsofficial) wrote : | # |
Continuous integration builds have changed state:
Travis build 4367. State: errored. Details: https:/
Appveyor build 4160. State: success. Details: https:/
Toni Förster (stonerl) wrote : | # |
@bunnybot merge force
Preview Diff
1 | === modified file 'data/scripting/win_conditions/territorial_functions.lua' | |||
2 | --- data/scripting/win_conditions/territorial_functions.lua 2018-10-16 06:53:20 +0000 | |||
3 | +++ data/scripting/win_conditions/territorial_functions.lua 2018-12-25 21:29:20 +0000 | |||
4 | @@ -86,6 +86,8 @@ | |||
5 | 86 | -- last_winning_team = -1, | 86 | -- last_winning_team = -1, |
6 | 87 | -- -- The currently winning player, if any. -1 means that no player is currently winning. | 87 | -- -- The currently winning player, if any. -1 means that no player is currently winning. |
7 | 88 | -- last_winning_player = -1, | 88 | -- last_winning_player = -1, |
8 | 89 | -- -- The name of the currently winning player, if any. Empty means that no player is currently winning. | ||
9 | 90 | -- last_winning_player_name = "", | ||
10 | 89 | -- -- Remaining time in secs for victory by > 50% territory. Default value is also used to calculate whether to send a report to players. | 91 | -- -- Remaining time in secs for victory by > 50% territory. Default value is also used to calculate whether to send a report to players. |
11 | 90 | -- remaining_time = 10, | 92 | -- remaining_time = 10, |
12 | 91 | -- -- Points by player | 93 | -- -- Points by player |
13 | @@ -98,6 +100,9 @@ | |||
14 | 98 | -- TODO(GunChleoc): We want to be able to list multiple winners in case of a draw. | 100 | -- TODO(GunChleoc): We want to be able to list multiple winners in case of a draw. |
15 | 99 | last_winning_team = -1, | 101 | last_winning_team = -1, |
16 | 100 | last_winning_player = -1, | 102 | last_winning_player = -1, |
17 | 103 | -- We record the last winning player name here to prevent crashes with retrieving | ||
18 | 104 | -- the player name when the player was just defeated a few ms ago | ||
19 | 105 | last_winning_player_name = "", | ||
20 | 101 | remaining_time = 10, | 106 | remaining_time = 10, |
21 | 102 | all_player_points = {}, | 107 | all_player_points = {}, |
22 | 103 | points = {} | 108 | points = {} |
23 | @@ -114,10 +119,7 @@ | |||
24 | 114 | -- :arg wc_descname: String with the win condition's descname | 119 | -- :arg wc_descname: String with the win condition's descname |
25 | 115 | -- :arg wc_version: Number with the win condition's descname | 120 | -- :arg wc_version: Number with the win condition's descname |
26 | 116 | -- | 121 | -- |
31 | 117 | function calculate_territory_points(fields, players, wc_descname, wc_version) | 122 | function calculate_territory_points(fields, players) |
28 | 118 | -- A player might have been defeated since the last calculation | ||
29 | 119 | check_player_defeated(players, lost_game.title, lost_game.body, wc_descname, wc_version) | ||
30 | 120 | |||
32 | 121 | local points = {} -- tracking points of teams and players without teams | 123 | local points = {} -- tracking points of teams and players without teams |
33 | 122 | local territory_was_kept = false | 124 | local territory_was_kept = false |
34 | 123 | 125 | ||
35 | @@ -140,6 +142,7 @@ | |||
36 | 140 | winning_teams[teaminfo.team] = true | 142 | winning_teams[teaminfo.team] = true |
37 | 141 | territory_points.last_winning_team = teaminfo.team | 143 | territory_points.last_winning_team = teaminfo.team |
38 | 142 | territory_points.last_winning_player = -1 | 144 | territory_points.last_winning_player = -1 |
39 | 145 | territory_points.last_winning_player_name = "" | ||
40 | 143 | else | 146 | else |
41 | 144 | winning_teams[teaminfo.team] = nil | 147 | winning_teams[teaminfo.team] = nil |
42 | 145 | end | 148 | end |
43 | @@ -150,11 +153,12 @@ | |||
44 | 150 | territory_was_kept = winning_players[playerinfo.number] ~= nil | 153 | territory_was_kept = winning_players[playerinfo.number] ~= nil |
45 | 151 | winning_players[playerinfo.number] = true | 154 | winning_players[playerinfo.number] = true |
46 | 152 | territory_points.last_winning_player = playerinfo.number | 155 | territory_points.last_winning_player = playerinfo.number |
47 | 156 | territory_points.last_winning_player_name = players[playerinfo.number].name | ||
48 | 153 | territory_points.last_winning_team = -1 | 157 | territory_points.last_winning_team = -1 |
49 | 154 | else | 158 | else |
50 | 155 | winning_players[playerinfo.number] = nil | 159 | winning_players[playerinfo.number] = nil |
51 | 156 | end | 160 | end |
53 | 157 | if teaminfo.team == 0 and players[playerinfo.number] ~= nil then | 161 | if teaminfo.team == 0 then |
54 | 158 | points[#points + 1] = { players[playerinfo.number].name, playerinfo.points } | 162 | points[#points + 1] = { players[playerinfo.number].name, playerinfo.points } |
55 | 159 | end | 163 | end |
56 | 160 | end | 164 | end |
57 | @@ -248,7 +252,7 @@ | |||
58 | 248 | if territory_points.last_winning_team >= 0 then | 252 | if territory_points.last_winning_team >= 0 then |
59 | 249 | winner_name = team_str:format(territory_points.last_winning_team) | 253 | winner_name = team_str:format(territory_points.last_winning_team) |
60 | 250 | elseif territory_points.last_winning_player >= 0 then | 254 | elseif territory_points.last_winning_player >= 0 then |
62 | 251 | winner_name = players[territory_points.last_winning_player].name | 255 | winner_name = territory_points.last_winning_player_name |
63 | 252 | end | 256 | end |
64 | 253 | local remaining_minutes = math.max(0, math.floor(territory_points.remaining_time / 60)) | 257 | local remaining_minutes = math.max(0, math.floor(territory_points.remaining_time / 60)) |
65 | 254 | 258 | ||
66 | 255 | 259 | ||
67 | === modified file 'data/scripting/win_conditions/territorial_lord.lua' | |||
68 | --- data/scripting/win_conditions/territorial_lord.lua 2018-09-28 18:05:24 +0000 | |||
69 | +++ data/scripting/win_conditions/territorial_lord.lua 2018-12-25 21:29:20 +0000 | |||
70 | @@ -59,12 +59,15 @@ | |||
71 | 59 | -- Sleep 30 seconds == STATISTICS_SAMPLE_TIME | 59 | -- Sleep 30 seconds == STATISTICS_SAMPLE_TIME |
72 | 60 | sleep(30000) | 60 | sleep(30000) |
73 | 61 | 61 | ||
74 | 62 | -- A player might have been defeated since the last calculation | ||
75 | 63 | check_player_defeated(plrs, lost_game.title, lost_game.body) | ||
76 | 64 | |||
77 | 62 | -- Check if a player or team is a candidate and update variables | 65 | -- Check if a player or team is a candidate and update variables |
79 | 63 | calculate_territory_points(fields, plrs, wc_descname, wc_version) | 66 | calculate_territory_points(fields, wl.Game().players) |
80 | 64 | 67 | ||
81 | 65 | -- Do this stuff, if the game is over | 68 | -- Do this stuff, if the game is over |
84 | 66 | if territory_points.remaining_time == 0 then | 69 | if territory_points.remaining_time == 0 or count_factions(plrs) <= 1 then |
85 | 67 | territory_game_over(fields, plrs, wc_descname, wc_version) | 70 | territory_game_over(fields, wl.Game().players, wc_descname, wc_version) |
86 | 68 | break | 71 | break |
87 | 69 | end | 72 | end |
88 | 70 | 73 | ||
89 | 71 | 74 | ||
90 | === modified file 'data/scripting/win_conditions/territorial_time.lua' | |||
91 | --- data/scripting/win_conditions/territorial_time.lua 2018-09-28 18:05:24 +0000 | |||
92 | +++ data/scripting/win_conditions/territorial_time.lua 2018-12-25 21:29:20 +0000 | |||
93 | @@ -78,13 +78,14 @@ | |||
94 | 78 | sleep(30000) | 78 | sleep(30000) |
95 | 79 | 79 | ||
96 | 80 | remaining_max_time = remaining_max_time - 30 | 80 | remaining_max_time = remaining_max_time - 30 |
98 | 81 | 81 | -- A player might have been defeated since the last calculation | |
99 | 82 | check_player_defeated(plrs, lost_game.title, lost_game.body) | ||
100 | 82 | -- Check if a player or team is a candidate and update variables | 83 | -- Check if a player or team is a candidate and update variables |
101 | 83 | -- Returns the names and points for the teams and players without a team | 84 | -- Returns the names and points for the teams and players without a team |
103 | 84 | calculate_territory_points(fields, plrs, wc_descname, wc_version) | 85 | calculate_territory_points(fields, wl.Game().players) |
104 | 85 | 86 | ||
105 | 86 | -- Game is over, do stuff after loop | 87 | -- Game is over, do stuff after loop |
107 | 87 | if territory_points.remaining_time <= 0 or remaining_max_time <= 0 then break end | 88 | if territory_points.remaining_time <= 0 or remaining_max_time <= 0 or count_factions(plrs) <= 1 then break end |
108 | 88 | 89 | ||
109 | 89 | -- at the beginning send remaining max time message only each 30 minutes | 90 | -- at the beginning send remaining max time message only each 30 minutes |
110 | 90 | -- if only 30 minutes or less are left, send each 5 minutes | 91 | -- if only 30 minutes or less are left, send each 5 minutes |
111 | @@ -97,6 +98,6 @@ | |||
112 | 97 | end | 98 | end |
113 | 98 | 99 | ||
114 | 99 | -- Game has ended | 100 | -- Game has ended |
116 | 100 | territory_game_over(fields, plrs, wc_descname, wc_version) | 101 | territory_game_over(fields, wl.Game().players, wc_descname, wc_version) |
117 | 101 | end | 102 | end |
118 | 102 | } | 103 | } |
Continuous integration builds have changed state:
Travis build 4267. State: passed. Details: https:/ /travis- ci.org/ widelands/ widelands/ builds/ 458731325. /ci.appveyor. com/project/ widelands- dev/widelands/ build/_ widelands_ dev_widelands_ bug_1802629_ territorial_ crash-4061.
Appveyor build 4061. State: success. Details: https:/