Merge lp:~widelands-dev/widelands/bug-1802629-territorial-crash into lp:widelands

Proposed by GunChleoc
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
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.

To post a comment you must log in.
Revision history for this message
bunnybot (widelandsofficial) wrote :

Continuous integration builds have changed state:

Travis build 4267. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/458731325.
Appveyor build 4061. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1802629_territorial_crash-4061.

Revision history for this message
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/scripting/lua_errors.cc:22] [string "scripting/win_conditions/territorial_function..."]:159: attempt to index a nil value (field '?')

review: Needs Fixing
8930. By GunChleoc

Fix crash where player does not exist any more.

Revision history for this message
GunChleoc (gunchleoc) wrote :

Should be fixed now

Revision history for this message
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.

Revision history for this message
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.

Revision history for this message
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

Revision history for this 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

8931. By hessenfarmer

fixed some bugs:
-score is now calculated for every player including defeated ones
-if only one player / team is left game ends

Revision history for this message
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.

review: Needs Resubmitting
Revision history for this message
GunChleoc (gunchleoc) wrote :

Code LGTM - we should test again to see if we can still reproduce a crash

review: Approve (code)
Revision history for this message
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.

Revision history for this message
GunChleoc (gunchleoc) wrote :

Tested and working, thanks!

@bunnybot merge

8932. By GunChleoc

Merged trunk.

Revision history for this message
bunnybot (widelandsofficial) wrote :

Continuous integration builds have changed state:

Travis build 4326. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/467359309.
Appveyor build 4121. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1802629_territorial_crash-4121.

Revision history for this message
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://travis-ci.org/widelands/widelands/builds/467359309.

8933. By Toni Förster

merged trunk

Revision history for this message
bunnybot (widelandsofficial) wrote :

Continuous integration builds have changed state:

Travis build 4367. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/472186642.
Appveyor build 4160. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1802629_territorial_crash-4160.

Revision history for this message
Toni Förster (stonerl) wrote :

@bunnybot merge force

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
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 -- last_winning_team = -1,
6 -- -- The currently winning player, if any. -1 means that no player is currently winning.
7 -- last_winning_player = -1,
8+-- -- The name of the currently winning player, if any. Empty means that no player is currently winning.
9+-- last_winning_player_name = "",
10 -- -- Remaining time in secs for victory by > 50% territory. Default value is also used to calculate whether to send a report to players.
11 -- remaining_time = 10,
12 -- -- Points by player
13@@ -98,6 +100,9 @@
14 -- TODO(GunChleoc): We want to be able to list multiple winners in case of a draw.
15 last_winning_team = -1,
16 last_winning_player = -1,
17+ -- We record the last winning player name here to prevent crashes with retrieving
18+ -- the player name when the player was just defeated a few ms ago
19+ last_winning_player_name = "",
20 remaining_time = 10,
21 all_player_points = {},
22 points = {}
23@@ -114,10 +119,7 @@
24 -- :arg wc_descname: String with the win condition's descname
25 -- :arg wc_version: Number with the win condition's descname
26 --
27-function calculate_territory_points(fields, players, wc_descname, wc_version)
28- -- A player might have been defeated since the last calculation
29- check_player_defeated(players, lost_game.title, lost_game.body, wc_descname, wc_version)
30-
31+function calculate_territory_points(fields, players)
32 local points = {} -- tracking points of teams and players without teams
33 local territory_was_kept = false
34
35@@ -140,6 +142,7 @@
36 winning_teams[teaminfo.team] = true
37 territory_points.last_winning_team = teaminfo.team
38 territory_points.last_winning_player = -1
39+ territory_points.last_winning_player_name = ""
40 else
41 winning_teams[teaminfo.team] = nil
42 end
43@@ -150,11 +153,12 @@
44 territory_was_kept = winning_players[playerinfo.number] ~= nil
45 winning_players[playerinfo.number] = true
46 territory_points.last_winning_player = playerinfo.number
47+ territory_points.last_winning_player_name = players[playerinfo.number].name
48 territory_points.last_winning_team = -1
49 else
50 winning_players[playerinfo.number] = nil
51 end
52- if teaminfo.team == 0 and players[playerinfo.number] ~= nil then
53+ if teaminfo.team == 0 then
54 points[#points + 1] = { players[playerinfo.number].name, playerinfo.points }
55 end
56 end
57@@ -248,7 +252,7 @@
58 if territory_points.last_winning_team >= 0 then
59 winner_name = team_str:format(territory_points.last_winning_team)
60 elseif territory_points.last_winning_player >= 0 then
61- winner_name = players[territory_points.last_winning_player].name
62+ winner_name = territory_points.last_winning_player_name
63 end
64 local remaining_minutes = math.max(0, math.floor(territory_points.remaining_time / 60))
65
66
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 -- Sleep 30 seconds == STATISTICS_SAMPLE_TIME
72 sleep(30000)
73
74+ -- A player might have been defeated since the last calculation
75+ check_player_defeated(plrs, lost_game.title, lost_game.body)
76+
77 -- Check if a player or team is a candidate and update variables
78- calculate_territory_points(fields, plrs, wc_descname, wc_version)
79+ calculate_territory_points(fields, wl.Game().players)
80
81 -- Do this stuff, if the game is over
82- if territory_points.remaining_time == 0 then
83- territory_game_over(fields, plrs, wc_descname, wc_version)
84+ if territory_points.remaining_time == 0 or count_factions(plrs) <= 1 then
85+ territory_game_over(fields, wl.Game().players, wc_descname, wc_version)
86 break
87 end
88
89
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 sleep(30000)
95
96 remaining_max_time = remaining_max_time - 30
97-
98+ -- A player might have been defeated since the last calculation
99+ check_player_defeated(plrs, lost_game.title, lost_game.body)
100 -- Check if a player or team is a candidate and update variables
101 -- Returns the names and points for the teams and players without a team
102- calculate_territory_points(fields, plrs, wc_descname, wc_version)
103+ calculate_territory_points(fields, wl.Game().players)
104
105 -- Game is over, do stuff after loop
106- if territory_points.remaining_time <= 0 or remaining_max_time <= 0 then break end
107+ if territory_points.remaining_time <= 0 or remaining_max_time <= 0 or count_factions(plrs) <= 1 then break end
108
109 -- at the beginning send remaining max time message only each 30 minutes
110 -- if only 30 minutes or less are left, send each 5 minutes
111@@ -97,6 +98,6 @@
112 end
113
114 -- Game has ended
115- territory_game_over(fields, plrs, wc_descname, wc_version)
116+ territory_game_over(fields, wl.Game().players, wc_descname, wc_version)
117 end
118 }

Subscribers

People subscribed via source and target branches

to status/vote changes: