widelands-dev team mailing list archive

[Notabilis <notabilis27@xxxxxx>]

Notabilis has proposed merging lp:~widelands-dev/widelands/bug-1434875-net-client-disconnect into lp:widelands.

Commit message:
Adding a dialog for the host when the connection to a client is lost. Allows the host to select whether to replace the client with an AI or to exit.

Requested reviews:
  Widelands Developers (widelands-dev)
Related bugs:
  Bug #1434875 in widelands: "Multiplayer network problems go unnoticed"
  https://bugs.launchpad.net/widelands/+bug/1434875

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1434875-net-client-disconnect/+merge/354531

This is not ready for merge yet!

When a client disconnects (either on purpose or due to network errors), the game is paused, saved, and a dialog is displayed to the host. There, the host can select whether to replace the client with a Normal/Weak/Very Weak/Empty AI or to exit the game.
No dialog is shown, if the leaving player ...
- is only an observer
- has already lost. In that case the player is replaced by the Empty AI
- has won the game. In that case the player is replaced by the Normal AI

So far, this branch seems to work fine in my testcases and could be merged.
However, ASAN is complaining about access to already freed memory when the host closes the game with Alt+F4 while the dialog is shown. Strangely, there is no complain if the "really exit game" confirmation dialog has been shown but aborted.
I wasn't able to figure out what is happening there. So if someone has an idea what is causing the problem, please tell me or fix it yourself. Apart from this memory bug, the branch is ready for review and testing.
-- 
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1434875-net-client-disconnect into lp:widelands.

=== modified file 'src/network/gamehost.cc'
--- src/network/gamehost.cc	2018-04-07 16:59:00 +0000
+++ src/network/gamehost.cc	2018-09-09 17:19:05 +0000
@@ -31,6 +31,7 @@
 #endif
 
 #include "ai/computer_player.h"
+#include "ai/defaultai.h"
 #include "base/i18n.h"
 #include "base/md5.h"
 #include "base/warning.h"
@@ -62,6 +63,7 @@
 #include "ui_basic/progresswindow.h"
 #include "ui_fsmenu/launch_mpg.h"
 #include "wlapplication.h"
+#include "wui/game_client_disconnected.h"
 #include "wui/game_tips.h"
 #include "wui/interactive_player.h"
 #include "wui/interactive_spectator.h"
@@ -560,6 +562,25 @@
 	                                ->instantiate(*d->game, p));
 }
 
+void GameHost::start_ai_for(uint8_t playernumber, const std::string& ai) {
+	assert(d->game->get_player(playernumber + 1)->get_ai().empty());
+	assert(d->game->get_player(playernumber + 1)->get_ai()
+			== d->settings.players.at(playernumber).ai);
+	// Inform all players about the change
+	// Has to be done at first in this method since the calls later on overwrite players[].name
+	send_system_message_code(
+		"CLIENT_X_REPLACED_WITH",
+		d->settings.players.at(playernumber).name,
+		ComputerPlayer::get_implementation(ai)->descname);
+	set_player_ai(playernumber, ai, false);
+	d->game->get_player(playernumber + 1)->set_ai(ai);
+	// Activate the ai
+	init_computer_player(playernumber + 1);
+	set_player_state(playernumber, PlayerSettings::State::kComputer);
+	assert(d->game->get_player(playernumber + 1)->get_ai()
+			== d->settings.players.at(playernumber).ai);
+}
+
 void GameHost::init_computer_players() {
 	const Widelands::PlayerNumber nr_players = d->game->map().get_nrplayers();
 	iterate_players_existing_novar(p, nr_players, *d->game) {
@@ -2254,8 +2275,7 @@
 	}
 
 	set_player_state(number, PlayerSettings::State::kOpen);
-	if (d->game)
-		init_computer_player(number + 1);
+	// Don't replace player with AI, let host choose what to do
 }
 
 void GameHost::disconnect_client(uint32_t const number,
@@ -2266,6 +2286,37 @@
 
 	Client& client = d->clients.at(number);
 
+	// If the client is linked to a player and it is the client that closes the connection
+	// and the game has already started ...
+	if (client.playernum != UserSettings::none() && reason != "SERVER_LEFT" && d->game != nullptr) {
+		// And the client hasn't lost/won yet ...
+		if (d->settings.users.at(client.usernum).result == Widelands::PlayerEndResult::kUndefined) {
+			// And the window isn't visible yet ...
+			if (!client_disconnected_.window) {
+				// Show a window and ask the host player what to do with the tribe of the leaving client
+
+				if (!forced_pause()) {
+					force_pause();
+				}
+
+				WLApplication::emergency_save(*d->game);
+
+				new GameClientDisconnected(d->game->get_ipl(), client_disconnected_, this);
+			}
+		// Client was active but is a winner of the game: Replace with normal AI
+		} else if (d->settings.users.at(client.usernum).result
+						== Widelands::PlayerEndResult::kWon) {
+			start_ai_for(client.playernum, DefaultAI::normal_impl.name);
+		// Client was active but has lost or gave up: Replace with empty AI
+		} else {
+			assert(d->settings.users.at(client.usernum).result
+							== Widelands::PlayerEndResult::kLost
+					|| d->settings.users.at(client.usernum).result
+							== Widelands::PlayerEndResult::kResigned);
+			start_ai_for(client.playernum, "empty");
+		}
+	}
+
 	// If the client was completely connected before the disconnect, free the
 	// user settings and send changes to the clients
 	if (client.usernum >= 0) {

=== modified file 'src/network/gamehost.h'
--- src/network/gamehost.h	2018-04-07 16:59:00 +0000
+++ src/network/gamehost.h	2018-09-09 17:19:05 +0000
@@ -26,6 +26,7 @@
 #include "logic/widelands.h"
 #include "network/nethost_interface.h"
 #include "network/network.h"
+#include "ui_basic/unique_window.h"
 
 struct ChatMessage;
 struct GameHostImpl;
@@ -79,6 +80,7 @@
 	void set_player_shared(PlayerSlot number, Widelands::PlayerNumber shared);
 	void switch_to_player(uint32_t user, uint8_t number);
 	void set_win_condition_script(const std::string& wc);
+	void start_ai_for(uint8_t playernumber, const std::string& ai);
 
 	// just visible stuff for the select mapmenu
 	void set_multiplayer_game_settings();
@@ -158,6 +160,8 @@
 	GameHostImpl* d;
 	bool internet_;
 	bool forced_pause_;
+
+	UI::UniqueWindow::Registry client_disconnected_;
 };
 
 #endif  // end of include guard: WL_NETWORK_GAMEHOST_H

=== modified file 'src/network/network_gaming_messages.cc'
--- src/network/network_gaming_messages.cc	2018-04-07 16:59:00 +0000
+++ src/network/network_gaming_messages.cc	2018-09-09 17:19:05 +0000
@@ -128,6 +128,7 @@
 	ngmessages["MALFORMED_COMMANDS"] = _("Client sent malformed commands: %s");
 	ngmessages["SOMETHING_WRONG"] = _("Something went wrong: %s");
 	ngmessages["CLIENT_X_LEFT_GAME"] = _("%1$s has left the game (%2$s)");
+	ngmessages["CLIENT_X_REPLACED_WITH"] = _("%1$s has been replaced with %2$s");
 	ngmessages["UNKNOWN_LEFT_GAME"] = _("Unknown user has left the game (%s)");
 	ngmessages["SYNCREQUEST_WO_GAME"] =
 	   _("Server sent a SYNCREQUEST even though no game is running.");

=== modified file 'src/ui_basic/unique_window.cc'
--- src/ui_basic/unique_window.cc	2018-04-27 06:11:05 +0000
+++ src/ui_basic/unique_window.cc	2018-09-09 17:19:05 +0000
@@ -37,14 +37,15 @@
 	if (!window) {
 		open_window();
 	} else {
-		if (window->is_minimal())
+		if (window->is_minimal()) {
 			window->restore();
+		}
 		window->move_to_top();
 	}
 }
 
 /**
- * Destroys the window, if it eixsts.
+ * Destroys the window, if it exists.
 */
 void UniqueWindow::Registry::destroy() {
 	if (window) {

=== modified file 'src/wui/CMakeLists.txt'
--- src/wui/CMakeLists.txt	2018-05-16 05:30:22 +0000
+++ src/wui/CMakeLists.txt	2018-09-09 17:19:05 +0000
@@ -175,6 +175,8 @@
     encyclopedia_window.h
     fieldaction.cc
     fieldaction.h
+    game_client_disconnected.cc
+    game_client_disconnected.h
     game_debug_ui.cc
     game_debug_ui.h
     game_statistics_menu.cc

=== added file 'src/wui/game_client_disconnected.cc'
--- src/wui/game_client_disconnected.cc	1970-01-01 00:00:00 +0000
+++ src/wui/game_client_disconnected.cc	2018-09-09 17:19:05 +0000
@@ -0,0 +1,197 @@
+/*
+ * Copyright (C) 2002-2018 by the Widelands Development Team
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ *
+ */
+
+#include "wui/game_client_disconnected.h"
+
+#include <boost/bind.hpp>
+#include <boost/format.hpp>
+
+#include "ai/computer_player.h"
+#include "ai/defaultai.h"
+#include "base/i18n.h"
+#include "network/gamehost.h"
+#include "ui_basic/messagebox.h"
+#include "wui/interactive_gamebase.h"
+
+namespace {
+
+const int32_t width = 256;
+const int32_t margin = 10;
+const int32_t vspacing = 5;
+const uint32_t vgap = 3;
+
+/**
+ * Small helper class for a "Really exit game?" message box.
+ */
+class GameClientDisconnectedExitConfirmBox : public UI::WLMessageBox {
+public:
+	// TODO(GunChleoc): Arabic: Buttons need more height for Arabic
+	GameClientDisconnectedExitConfirmBox(GameClientDisconnected* gcd, InteractiveGameBase* gb)
+	   : UI::WLMessageBox(gcd->get_parent(),
+	                      /** TRANSLATORS: Window label when "Exit game" has been pressed */
+	                      _("Exit Game Confirmation"),
+	                      _("Are you sure you wish to exit this game?"),
+	                      MBoxType::kOkCancel),
+	     igb_(gb), gcd_(gcd) {
+	}
+
+	void clicked_ok() override {
+		igb_->end_modal<UI::Panel::Returncodes>(UI::Panel::Returncodes::kBack);
+	}
+
+	void clicked_back() override {
+		cancel();
+		die();
+		// Make parent window visible again so player can use another option
+		gcd_->set_visible(true);
+	}
+
+private:
+	InteractiveGameBase* igb_;
+	GameClientDisconnected* gcd_;
+};
+
+} // end anonymous namespace
+
+GameClientDisconnected::GameClientDisconnected(InteractiveGameBase* gb,
+                                 UI::UniqueWindow::Registry& registry,
+                                 GameHost* host)
+	: UI::UniqueWindow(gb, "client_disconnected", &registry, 2 * margin + width, 0,
+						/** TRANSLATORS: Window label */
+						_("Client got disconnected")),
+		igb_(gb),
+		host_(host),
+		box_(this, margin, margin, UI::Box::Vertical, width, get_h() - 2 * margin, vspacing),
+		box_h_(&box_, margin, margin, UI::Box::Horizontal, width, 35, vspacing),
+		text_(&box_,
+			0,
+			0,
+			width,
+			10, // automatic height
+			UI::PanelStyle::kWui,
+			_("A player disconnected from the game. An automatic save game has been created. "
+				"Do you want to continue playing?"),
+			UI::Align::kLeft,
+			UI::MultilineTextarea::ScrollMode::kNoScrolling),
+		continue_(&box_h_,
+			"continue_game",
+			0,
+			0,
+			width - 35 - vspacing,
+			35,
+			UI::ButtonStyle::kWuiMenu,
+			/** TRANSLATORS: Button text */
+			_("Continue game"),
+			/** TRANSLATORS: Button tooltip */
+			_("Continue game with...")),
+		type_dropdown_(&box_h_,
+			width - 50, // x
+			0, // y
+			60, // width of selection box
+			800, // height of selection box, shrinks automatically
+			35, // width/height of button
+			/** TRANSLATORS: Dropdown tooltip to select the AI difficulty,
+				entries have the form "Replace with Normal AI" */
+			_("Continue game and"),
+			UI::DropdownType::kPictorial,
+			UI::PanelStyle::kWui),
+		exit_game_(&box_,
+			"exit_game",
+			0,
+			0,
+			width,
+			35,
+			UI::ButtonStyle::kWuiMenu,
+			g_gr->images().get("images/wui/menus/menu_exit_game.png"),
+			/** TRANSLATORS: Button tooltip */
+			_("Exit Game")) {
+
+	box_.add(&text_);
+	box_.add_space(vgap);
+	box_h_.add(&continue_);
+	box_h_.add(&type_dropdown_);
+	box_.add(&box_h_);
+	box_.add(&exit_game_);
+	box_.set_size(width, text_.get_h() + vgap + box_h_.get_h() + exit_game_.get_h() + 3 * vspacing);
+	set_inner_size(get_inner_w(), box_.get_h() + 2 * margin);
+
+	continue_.sigclicked.connect(
+	   boost::bind(&GameClientDisconnected::clicked_continue, boost::ref(*this)));
+	exit_game_.sigclicked.connect(
+	   boost::bind(&GameClientDisconnected::clicked_exit_game, boost::ref(*this)));
+
+	// Add all AI types
+	for (const auto* impl : ComputerPlayer::get_implementations()) {
+		type_dropdown_.add((boost::format(_("Replace with %s")) % impl->descname).str(),
+							impl->name.c_str(),
+							g_gr->images().get(impl->icon_filename), false,
+							(boost::format(_("Replace with %s")) % impl->descname).str());
+	}
+
+	// Set default mode to normal AI
+	type_dropdown_.select(DefaultAI::normal_impl.name.c_str());
+
+	if (get_usedefaultpos())
+		center_to_parent();
+}
+
+void GameClientDisconnected::die() {
+
+	if (host_->forced_pause()) {
+		host_->end_forced_pause();
+	}
+	if (is_visible()) {
+		// Dialog aborted, default to the old behavior and add a normal AI
+		set_ai(DefaultAI::normal_impl.name);
+	}
+	UI::UniqueWindow::die();
+}
+
+void GameClientDisconnected::clicked_continue() {
+	assert(type_dropdown_.has_selection());
+
+	const std::string selection = type_dropdown_.get_selected();
+	assert(selection != "");
+
+	set_ai(selection);
+	// Visibility works as a hint that the window was closed by a button click
+	set_visible(false);
+
+	die();
+}
+
+void GameClientDisconnected::clicked_exit_game() {
+	if (SDL_GetModState() & KMOD_CTRL) {
+		igb_->end_modal<UI::Panel::Returncodes>(UI::Panel::Returncodes::kBack);
+	} else {
+		new GameClientDisconnectedExitConfirmBox(this, igb_);
+		set_visible(false);
+	}
+}
+
+void GameClientDisconnected::set_ai(const std::string& ai) {
+	const std::string ai_descr = ComputerPlayer::get_implementation(ai)->descname;
+	for (size_t i = 0; i < host_->settings().players.size(); i++) {
+		if (host_->settings().players.at(i).state != PlayerSettings::State::kOpen
+				|| !igb_->game().get_player(i + 1)->get_ai().empty()) {
+			continue;
+		}
+		host_->start_ai_for(i, ai);
+	}
+}

=== added file 'src/wui/game_client_disconnected.h'
--- src/wui/game_client_disconnected.h	1970-01-01 00:00:00 +0000
+++ src/wui/game_client_disconnected.h	2018-09-09 17:19:05 +0000
@@ -0,0 +1,61 @@
+/*
+ * Copyright (C) 2002-2018 by the Widelands Development Team
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ *
+ */
+
+#ifndef WL_WUI_GAME_CLIENT_DISCONNECTED_H
+#define WL_WUI_GAME_CLIENT_DISCONNECTED_H
+
+#include "ui_basic/box.h"
+#include "ui_basic/button.h"
+#include "ui_basic/dropdown.h"
+#include "ui_basic/multilinetextarea.h"
+#include "ui_basic/unique_window.h"
+
+class GameHost;
+class InteractiveGameBase;
+
+/**
+ * Dialog that offers to replace a player with an AI or exit the game.
+ * Even when the text only talks about a single player, actually all recently disconnected
+ * players will be replaced by the taken choice.
+ */
+struct GameClientDisconnected : public UI::UniqueWindow {
+	GameClientDisconnected(InteractiveGameBase*,
+	                UI::UniqueWindow::Registry&,
+	                GameHost*);
+
+	void die() override;
+
+private:
+	void clicked_continue();
+	void clicked_exit_game();
+
+	void set_ai(const std::string& ai);
+
+	InteractiveGameBase* igb_;
+	GameHost* host_;
+
+	UI::Box box_;
+	UI::Box box_h_;
+	UI::MultilineTextarea text_;
+	UI::Button continue_;
+	UI::Dropdown<std::string> type_dropdown_;
+	UI::Button exit_game_;
+};
+
+#endif  // end of include guard: WL_WUI_GAME_CLIENT_DISCONNECTED_H