[Widelands-dev] [Merge] lp:~widelands-dev/widelands/refactor_gameclient into lp:widelands
The proposal to merge lp:~widelands-dev/widelands/refactor_gameclient into lp:widelands has been updated. Status: Needs review => Merged For more details, see: https://code.launchpad.net/~widelands-dev/widelands/refactor_gameclient/+merge/366743 -- Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/refactor_gameclient. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/refactor_gameclient into lp:widelands
Review: Approve I have done some testing and it seems to work fine. I am not getting the double free you mentioned. @bunnybot merge -- https://code.launchpad.net/~widelands-dev/widelands/refactor_gameclient/+merge/366743 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/refactor_gameclient. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/refactor_gameclient into lp:widelands
Sorry, your recent changes slipped me by. Thanks for the reminder, I will do another review & testing. -- https://code.launchpad.net/~widelands-dev/widelands/refactor_gameclient/+merge/366743 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/refactor_gameclient. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/refactor_gameclient into lp:widelands
Now why can we nort merge this one? -- https://code.launchpad.net/~widelands-dev/widelands/refactor_gameclient/+merge/366743 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/refactor_gameclient. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/refactor_gameclient into lp:widelands
Continuous integration builds have changed state: Travis build 4891. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/528361519. Appveyor build 4672. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_refactor_gameclient-4672. -- https://code.launchpad.net/~widelands-dev/widelands/refactor_gameclient/+merge/366743 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/refactor_gameclient. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/refactor_gameclient into lp:widelands
Review: Resubmit Found that I lost a '!' while refactoring. Now a normal network game works as itendend. I assume that disconnect/double freee has always been there, I will create a followup bug for that one. When debugging I triggered a racecondition, I doubt we ever will see that i the wild. Gun: and all, please take anotheer look -- https://code.launchpad.net/~widelands-dev/widelands/refactor_gameclient/+merge/366743 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/refactor_gameclient. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/refactor_gameclient into lp:widelands
Using unique_ptr is unneeded, this is only a helper class neede to as long as some progress dialog is open, so using normal scope is ok. (We should actually cleanup that usage, some other time) The Problem is the owenership of UI::ProgressWindow* loader The disconnect code try to close the last 'modal' window. In case of the loader this is incorrect. Not sure how to cleanup tis messs by now -- https://code.launchpad.net/~widelands-dev/widelands/refactor_gameclient/+merge/366743 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/refactor_gameclient into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/refactor_gameclient into lp:widelands
Continuous integration builds have changed state: Travis build 4871. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/527824464. Appveyor build 4652. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_refactor_gameclient-4652. -- https://code.launchpad.net/~widelands-dev/widelands/refactor_gameclient/+merge/366743 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/refactor_gameclient into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/refactor_gameclient into lp:widelands
You could also do the game tips like this:
const std::string tribename = get_players_tribe();
assert(Widelands::tribe_exists(tribename));
std::unique_ptr tips(new GameTips(*loader, {"general_game",
"multiplayer", tribename}));
As to the ASan crash, I suspect that there is a problem with the lifetime of an
object caused by the pulling out of stuff into separate functions.
--
https://code.launchpad.net/~widelands-dev/widelands/refactor_gameclient/+merge/366743
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/refactor_gameclient into lp:widelands.
___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/refactor_gameclient into lp:widelands
Still get a heap-use-after-free related to std::unique_ptr loader_ui(new UI::ProgressWindow()); But this is maybe just a problem as the game crashed with disconnect(CLIENT_CRASHED, ) A have a deja vue around GameClientImpl.modal which is released twice. -- https://code.launchpad.net/~widelands-dev/widelands/refactor_gameclient/+merge/366743 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/refactor_gameclient into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/refactor_gameclient into lp:widelands
Found a first bug, still need some debugging -- https://code.launchpad.net/~widelands-dev/widelands/refactor_gameclient/+merge/366743 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/refactor_gameclient into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/refactor_gameclient into lp:widelands
* Used parent instead of This. * moved all Variables into Impl About that -2 magic: I got lost in finding the sematics of user/player etc. I added a TODO comment with our best guess by now. I am at the End of my vacation (sigh) so lets get this in before it starts lingering around for too long. -- https://code.launchpad.net/~widelands-dev/widelands/refactor_gameclient/+merge/366743 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/refactor_gameclient into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/refactor_gameclient into lp:widelands
Continuous integration builds have changed state: Travis build 4845. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/526796650. Appveyor build 4626. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_refactor_gameclient-4626. -- https://code.launchpad.net/~widelands-dev/widelands/refactor_gameclient/+merge/366743 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/refactor_gameclient into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/refactor_gameclient into lp:widelands
Hello Gun: * Adressed some comments * Get the tribe and check whether it exists before you push it -> I have no idea (yet) what this code does :-) * Playercommand is not for current player , not sure if this can normally happen lets wait If I ever see this, then we can make it an assert. * The This / this confusion is because I moved some fucntion into Impl, but they need the main class. I could use "outer" or some other name to avoid it. * send_player_command(Widelands::PlayerCommand* pc) making this a unique_ptr: * Will affect a lot of code * Some aspects of this code are not clear to me (e.g. Commnands a queued for networking) -> lets adress this in a seperate branch, I already regret I touched it :-) * I would leave "GameClientImpl* d" as is: * it is used very often * it is used in the local scope only -> no risc to loose control * commonly used Pattern in Widelands * d->settings.usernum == -2,-1, n seems to be used to define the "hello" handshake what about "kPreHello" and "kAfterHello" constants? Maybe we should move the two remaining variables to GameClientImpl, too? Please check the logic of the main switch statement, I changed some control flow for better readbility. I intend to do a similar refactoring with gamehost, too. -- https://code.launchpad.net/~widelands-dev/widelands/refactor_gameclient/+merge/366743 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/refactor_gameclient into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/refactor_gameclient into lp:widelands
uot;));
> + d->net->send(s);
> + try {
> + g_fs->fs_unlink(file_->filename);
> + } catch (const FileError& e) {
> + log("file error in GameClient::handle_packet:
> case NETCMD_FILE_PART: "
> + "%s\n",
> + e.what());
> + }
> + }
> + // Check file for validity
> + bool invalid = false;
> + if (d->settings.savegame) {
> + // Saved game check - does Widelands recognize the file
> as saved game?
> + Widelands::Game game;
> + try {
> + Widelands::GameLoader gl(file_->filename, game);
> + } catch (...) {
> + invalid = true;
> + }
> + } else {
> + // Map check - does Widelands recognize the file as map?
> + Widelands::Map map;
> + std::unique_ptr ml =
> map.get_correct_loader(file_->filename);
> + if (!ml)
> + invalid = true;
> + }
> + if (invalid) {
> + try {
> + g_fs->fs_unlink(file_->filename);
> + // Restore original file, if there was one
> before
> + if
> (g_fs->file_exists(backup_file_name(file_->filename)))
> +
> g_fs->fs_rename(backup_file_name(file_->filename), file_->filename);
> + } catch (const FileError& e) {
> + log("file error in GameClient::handle_packet:
> case NETCMD_FILE_PART: "
> + "%s\n",
> + e.what());
> + }
> + s.reset();
> + s.unsigned_8(NETCMD_CHAT);
> + s.string(_("/me checked the received file. Although md5
> check summing succeeded, "
> +"I can not handle the file."));
> + d->net->send(s);
> + }
> + }
> +}
> +
> +/**
> + *
> + */
> +void GameClient::handle_setting_tribes(RecvPacket& packet) {
> + d->settings.tribes.clear();
> + for (uint8_t i = packet.unsigned_8(); i; --i) {
> + Widelands::TribeBasicInfo info =
> Widelands::get_tribeinfo(packet.string());
> +
> + // Get initializations (we have to do this locally, for
> translations)
> + LuaInterface lua;
> + info.initializations.clear();
> + for (uint8_t j = packet.unsigned_8(); j > 0; --j) {
> + std::string const initialization_script =
> packet.string();
> + std::unique_ptr t =
> lua.run_script(initialization_script);
> + t->do_not_warn_about_unaccessed_keys();
> +
> info.initializations.push_back(Widelands::TribeBasicInfo::Initialization(
> + initialization_script,
> t->get_string("descname"), t->get_string("tooltip")));
> + }
> + d->settings.tribes.push_back(info);
> + }
> +}
> +
> +/**
> + *
> + */
> +void GameClient::handle_setting_allplayers(RecvPacket& packet) {
> + d->settings.players.resize(packet.unsigned_8());
> + for (uint8_t i = 0; i < d->settings.players.size(); ++i) {
> + receive_one_player(i, packet);
> + }
> + // Map changes are finished here
> +
> Notifications::publish(NoteGameSettings(NoteGameSettings::Action::kMap));
> +}
> +
> +/**
> + *
> + */
> +void GameClient::handle_playercommand(RecvPacket& packet) {
> + if (!d->game)
> + throw DisconnectException("PLAYERCMD_WO_GAME");
> +
> + int32_t const time = packet.signed_32();
> + Widelands::PlayerCommand& plcmd =
> *Widelands::PlayerCommand::deserialize(packet);
> + plcmd.set_duetime(time);
> + d->game->enqueue_command();
> + d->time.receive(time);
> +}
> +
> +/**
> + *
> + */
> +void GameClient::handle_syncrequest(RecvPacket& packet) {
> + if (!d->game)
> + throw DisconnectException("SYNCREQUEST_WO_GAME");
> + int32_t const time = packet.signed_32();
> + d->time.receive(time);
> + d->game->enqueue_command(new CmdNetCheckSync(time, [this] {
> sync_report_callback(); }));
> + d->game->report_sync_request();
> +}
> +
> +/**
> + *
> + */
> +void GameClient::handle_chat(RecvPacket& packet) {
> + ChatMessage c("");
> + c.playern = packet.signed_16();
> + c.sender = packet.string();
> + c.msg = packet.string();
> + if (packet.unsigned_8())
> + c.recipient = packet.string();
> + d->chatmessages.push_back(c);
> + Notifications::publish(c);
> +}
> +
> +/**
> + *
> + */
> +void GameClient::handle_system_message(RecvPacket& packet) {
> + const std::string code = packet.string();
> + const std::string arg1 = packet.string();
> + const std::string arg2 = packet.string();
> + const std::string arg3 = packet.string();
> + ChatMessage c(NetworkGamingMessages::get_message(code, arg1, arg2,
> arg3));
> + c.playern = UserSettings::none(); // == System message
> +//
> c.sender remains empty to indicate a system message
> + d->chatmessages.push_back(c);
> + Notifications::publish(c);
> +}
> +
> +/**
> + *
> + */
> +void GameClient::handle_desync(RecvPacket&) {
> + log("[Client] received NETCMD_INFO_DESYNC. Trying to salvage some "
> + "information for debugging.\n");
> + if (d->game) {
> + d->game->save_syncstream(true);
> + // We don't know our playernumber, so report as -1
> + d->game->report_desync(-1);
> + }
> +}
> +
> /**
> * Handle one packet received from the host.
> *
>
> === modified file 'src/network/gameclient.h'
> --- src/network/gameclient.h 2019-04-29 16:22:08 +
> +++ src/network/gameclient.h 2019-05-01 07:38:30 +
> @@ -125,8 +141,11 @@
> bool sendreason = true,
> bool showmsg = true);
>
> +
> + GameClientImpl* d;
Turn this into a unique_ptr
> +
> + /** File that is eventually transferred via the netowrk if not found at
> the other side */
netowrk -> network
> std::unique_ptr file_;
> - GameClientImpl* d;
> bool internet_;
> };
>
--
https://code.launchpad.net/~widelands-dev/widelands/refactor_gameclient/+merge/366743
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/refactor_gameclient into lp:widelands.
___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/refactor_gameclient into lp:widelands
The proposal to merge lp:~widelands-dev/widelands/refactor_gameclient into lp:widelands has been updated. Description changed to: * Refactor big switch statement in gameclient.cc * Refactor ::run() to improve readability * use send_player_command with Ptr instead of Reference to clarify ownership of PlayerCommand. * Added more comments Added some TODOs and empty comments where I did not know the details. There should be no functional change at all. For more details, see: https://code.launchpad.net/~widelands-dev/widelands/refactor_gameclient/+merge/366743 -- Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/refactor_gameclient into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/refactor_gameclient into lp:widelands
Klaus Halfmann has proposed merging
lp:~widelands-dev/widelands/refactor_gameclient into lp:widelands.
Commit message:
Refactor gameclient.h/.cc for better readability
Requested reviews:
Widelands Developers (widelands-dev)
For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/refactor_gameclient/+merge/366743
* Refactor big switch statement in gameclient.cc
* Refactor ::run() to improve readability
* use send_play_camd with Ptr instead of reference to clarify ownership of cmd.
* Added more comments
Added some TODOs and empty comments where I dodnt know the details.
There should be no functional change at all.
--
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/refactor_gameclient into lp:widelands.
=== modified file 'src/ai/defaultai.cc'
--- src/ai/defaultai.cc 2019-04-10 16:39:31 +
+++ src/ai/defaultai.cc 2019-05-01 07:38:30 +
@@ -6544,7 +6544,7 @@
const uint16_t new_target = std::max(default_target * multiplier / 10, 3);
assert(new_target > 1);
- game().send_player_command(*new Widelands::CmdSetWareTargetQuantity(
+ game().send_player_command(new Widelands::CmdSetWareTargetQuantity(
gametime, player_number(), observer->economy.serial(), id, new_target));
}
}
=== modified file 'src/logic/game.cc'
--- src/logic/game.cc 2019-04-26 16:52:39 +
+++ src/logic/game.cc 2019-05-01 07:38:30 +
@@ -712,7 +712,7 @@
* It takes the appropriate action, i.e. either add to the cmd_queue or send
* across the network.
*/
-void Game::send_player_command(PlayerCommand& pc) {
+void Game::send_player_command(PlayerCommand* pc) {
ctrl_->send_player_command(pc);
}
@@ -735,55 +735,55 @@
// we might want to make these inlines:
void Game::send_player_bulldoze(PlayerImmovable& pi, bool const recurse) {
- send_player_command(*new CmdBulldoze(get_gametime(), pi.owner().player_number(), pi, recurse));
+ send_player_command(new CmdBulldoze(get_gametime(), pi.owner().player_number(), pi, recurse));
}
void Game::send_player_dismantle(PlayerImmovable& pi) {
- send_player_command(*new CmdDismantleBuilding(get_gametime(), pi.owner().player_number(), pi));
+ send_player_command(new CmdDismantleBuilding(get_gametime(), pi.owner().player_number(), pi));
}
void Game::send_player_build(int32_t const pid, const Coords& coords, DescriptionIndex const id) {
assert(tribes().building_exists(id));
- send_player_command(*new CmdBuild(get_gametime(), pid, coords, id));
+ send_player_command(new CmdBuild(get_gametime(), pid, coords, id));
}
void Game::send_player_build_flag(int32_t const pid, const Coords& coords) {
- send_player_command(*new CmdBuildFlag(get_gametime(), pid, coords));
+ send_player_command(new CmdBuildFlag(get_gametime(), pid, coords));
}
void Game::send_player_build_road(int32_t pid, Path& path) {
- send_player_command(*new CmdBuildRoad(get_gametime(), pid, path));
+ send_player_command(new CmdBuildRoad(get_gametime(), pid, path));
}
void Game::send_player_flagaction(Flag& flag) {
- send_player_command(*new CmdFlagAction(get_gametime(), flag.owner().player_number(), flag));
+ send_player_command(new CmdFlagAction(get_gametime(), flag.owner().player_number(), flag));
}
void Game::send_player_start_stop_building(Building& building) {
send_player_command(
- *new CmdStartStopBuilding(get_gametime(), building.owner().player_number(), building));
+ new CmdStartStopBuilding(get_gametime(), building.owner().player_number(), building));
}
void Game::send_player_militarysite_set_soldier_preference(Building& building,
SoldierPreference my_preference) {
- send_player_command(*new CmdMilitarySiteSetSoldierPreference(
+ send_player_command(new CmdMilitarySiteSetSoldierPreference(
get_gametime(), building.owner().player_number(), building, my_preference));
}
void Game::send_player_start_or_cancel_expedition(Building& building) {
send_player_command(
- *new CmdStartOrCancelExpedition(get_gametime(), building.owner().player_number(), building));
+ new CmdStartOrCancelExpedition(get_gametime(), building.owner().player_number(), building));
}
void Game::send_player_enhance_building(Building& building, DescriptionIndex const id) {
assert(building.owner().tribe().has_building(id));
send_player_command(
- *new CmdEnhanceBuilding(get_gametime(), building.owner().player_number(), building, id));
+ new CmdEnhanceBuilding(get_gametime(), building.owner().player_number(), building, id));
}
void Game::send_player_evict_worker(Worker& worker) {
- send_player_command(*new CmdEvictWorker(get_gametime(), worker.owner().player_number(), worker));
+ send_player_command(new CmdEvictWorker(get_gametime(), worker.owner().player_number(), worker));
}
void Game::send_player_set_ware_priority(PlayerImmovable& imm,
@@ -791,14 +791,14
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/refactor_gameclient into lp:widelands
Regressiontest are ok: Ran 44 tests in 1377.106s Will try to do a network game, too. -- https://code.launchpad.net/~widelands-dev/widelands/refactor_gameclient/+merge/366743 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/refactor_gameclient into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
