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

[GunChleoc]

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.

[bunnybot (widelandsofficial)]

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.

[hessenfarmer (stephan-lutz)]

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

[lp:~widelands-dev/widelands/bug-1802629-territorial-crash]

8930. By GunChleoc on 2018-11-26

Fix crash where player does not exist any more.

[GunChleoc (gunchleoc)]

Should be fixed now

[hessenfarmer (stephan-lutz)]

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)]

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)]

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)]

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

[lp:~widelands-dev/widelands/bug-1802629-territorial-crash]

8931. By hessenfarmer on 2018-12-03

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

[hessenfarmer (stephan-lutz)]

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

[GunChleoc (gunchleoc)]

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

review: Approve (code)

[hessenfarmer (stephan-lutz)]

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)]

Tested and working, thanks!

@bunnybot merge

[lp:~widelands-dev/widelands/bug-1802629-territorial-crash]

8932. By GunChleoc on 2018-12-12

Merged trunk.

[bunnybot (widelandsofficial)]

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.

[bunnybot (widelandsofficial)]

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.

[lp:~widelands-dev/widelands/bug-1802629-territorial-crash]

8933. By Toni Förster on 2018-12-25

merged trunk

[bunnybot (widelandsofficial)]

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.

[Toni Förster (stonerl)]

@bunnybot merge force

=== modified file 'data/scripting/win_conditions/territorial_functions.lua'
--- data/scripting/win_conditions/territorial_functions.lua	2018-10-16 06:53:20 +0000
+++ data/scripting/win_conditions/territorial_functions.lua	2018-12-25 21:29:20 +0000
@@ -86,6 +86,8 @@
 --          last_winning_team = -1,
 --          -- The currently winning player, if any. -1 means that no player is currently winning.
 --          last_winning_player = -1,
+--          -- The name of the currently winning player, if any. Empty means that no player is currently winning.
+--          last_winning_player_name = "",
 --          -- Remaining time in secs for victory by > 50% territory. Default value is also used to calculate whether to send a report to players.
 --          remaining_time = 10,
 --          -- Points by player
@@ -98,6 +100,9 @@
    -- TODO(GunChleoc): We want to be able to list multiple winners in case of a draw.
    last_winning_team = -1,
    last_winning_player = -1,
+   -- We record the last winning player name here to prevent crashes with retrieving
+   -- the player name when the player was just defeated a few ms ago
+   last_winning_player_name = "",
    remaining_time = 10,
    all_player_points = {},
    points = {}
@@ -114,10 +119,7 @@
 --    :arg wc_descname: String with the win condition's descname
 --    :arg wc_version: Number with the win condition's descname
 --
-function calculate_territory_points(fields, players, wc_descname, wc_version)
-   -- A player might have been defeated since the last calculation
-   check_player_defeated(players, lost_game.title, lost_game.body, wc_descname, wc_version)
-
+function calculate_territory_points(fields, players)
    local points = {} -- tracking points of teams and players without teams
    local territory_was_kept = false
 
@@ -140,6 +142,7 @@
             winning_teams[teaminfo.team] = true
             territory_points.last_winning_team = teaminfo.team
             territory_points.last_winning_player = -1
+            territory_points.last_winning_player_name = ""
          else
             winning_teams[teaminfo.team] = nil
          end
@@ -150,11 +153,12 @@
             territory_was_kept = winning_players[playerinfo.number] ~= nil
             winning_players[playerinfo.number] = true
             territory_points.last_winning_player = playerinfo.number
+            territory_points.last_winning_player_name = players[playerinfo.number].name
             territory_points.last_winning_team = -1
          else
             winning_players[playerinfo.number] = nil
          end
-         if teaminfo.team == 0 and players[playerinfo.number] ~= nil then
+         if teaminfo.team == 0 then
             points[#points + 1] = { players[playerinfo.number].name, playerinfo.points }
          end
       end
@@ -248,7 +252,7 @@
    if territory_points.last_winning_team >= 0 then
       winner_name = team_str:format(territory_points.last_winning_team)
    elseif territory_points.last_winning_player >= 0 then
-      winner_name = players[territory_points.last_winning_player].name
+      winner_name = territory_points.last_winning_player_name
    end
    local remaining_minutes = math.max(0, math.floor(territory_points.remaining_time / 60))
 

=== modified file 'data/scripting/win_conditions/territorial_lord.lua'
--- data/scripting/win_conditions/territorial_lord.lua	2018-09-28 18:05:24 +0000
+++ data/scripting/win_conditions/territorial_lord.lua	2018-12-25 21:29:20 +0000
@@ -59,12 +59,15 @@
          -- Sleep 30 seconds == STATISTICS_SAMPLE_TIME
          sleep(30000)
 
+         -- A player might have been defeated since the last calculation
+         check_player_defeated(plrs, lost_game.title, lost_game.body)
+
          -- Check if a player or team is a candidate and update variables
-         calculate_territory_points(fields, plrs, wc_descname, wc_version)
+         calculate_territory_points(fields, wl.Game().players)
 
          -- Do this stuff, if the game is over
-         if territory_points.remaining_time == 0 then
-            territory_game_over(fields, plrs, wc_descname, wc_version)
+         if territory_points.remaining_time == 0 or count_factions(plrs) <= 1 then
+            territory_game_over(fields, wl.Game().players, wc_descname, wc_version)
             break
          end
 

=== modified file 'data/scripting/win_conditions/territorial_time.lua'
--- data/scripting/win_conditions/territorial_time.lua	2018-09-28 18:05:24 +0000
+++ data/scripting/win_conditions/territorial_time.lua	2018-12-25 21:29:20 +0000
@@ -78,13 +78,14 @@
          sleep(30000)
 
          remaining_max_time = remaining_max_time - 30
-
+         -- A player might have been defeated since the last calculation
+         check_player_defeated(plrs, lost_game.title, lost_game.body)
          -- Check if a player or team is a candidate and update variables
          -- Returns the names and points for the teams and players without a team
-         calculate_territory_points(fields, plrs, wc_descname, wc_version)
+         calculate_territory_points(fields, wl.Game().players)
 
          -- Game is over, do stuff after loop
-         if territory_points.remaining_time <= 0 or remaining_max_time <= 0 then break end
+         if territory_points.remaining_time <= 0 or remaining_max_time <= 0 or count_factions(plrs) <= 1 then break end
 
          -- at the beginning send remaining max time message only each 30 minutes
          -- if only 30 minutes or less are left, send each 5 minutes
@@ -97,6 +98,6 @@
       end
 
       -- Game has ended
-      territory_game_over(fields, plrs, wc_descname, wc_version)
+      territory_game_over(fields, wl.Game().players, wc_descname, wc_version)
    end
 }