[PATCH] Gamesetup - Optionally allow players to setup the game

[elexis]

It would be nice if players could pick their own civilizations without having to communicate it via chat to the host.

Since it is a delicate issue to balance the teams, the setting could be implemented using a dropdownlist, giving players different degrees of freedom (none, civs only, civs+teams, all gamesettings)).

---

There is an according TODO in gamesetup.js:

	// TODO: Shouldn't players be able to choose their own assignment?
	for (let i = 0; i < g_MaxPlayers; ++i)
	{
		Engine.GetGUIObjectByName("playerAssignment["+i+"]").hidden = true;
		Engine.GetGUIObjectByName("playerCiv["+i+"]").hidden = true;
		Engine.GetGUIObjectByName("playerTeam["+i+"]").hidden = true;
	}

(Previous releases had signficantly imbalanced civilizations, so no sane host would have allowed players to pick OP civs. With alpha 19, balance is significantly improved, so that this feature becomes actually useful).

---

The remaining TODOs are: server code:

• rewrite changeSetting message to send only the value and the name of the changed setting
• Forward the changeSetting message only to the host
• Only accept new settings message from host

gamesetup.js:

• When recieving a changesetting message from server, check if change is legit. If so send new settings to server

[Itms]

#2932 was a duplicate, with a less general aim.

I think this is an important issue.

[elexis]

This is also a requirement for #3556. It is probably not simple, as the the controller of the gamesetup must not assume that it can access the server directly via the ScriptInterface, but needs to send the settings via network.

[Imarok]

My solution would be: Adding a new scriptfunction that sends a message(like chatmessage or readymessage) with the new setings to the server. Make the server listen to it. If the server recieves the message he applies the new values and updates the client with his UpdateGameAttributes function.

Is this draft right/ok?

[Imarok]

WIP Patch: already implemented a test button to set the revealMap setting to false, that can be used by every player

[elexis]

PlayersCanChange -> GuestSettings ? Color & Civilization & Team => Color, Civilization and Team

[Imarok]

still WIP, changed PlayersCanChange to GuestSettings

[Imarok]

wip patch

[Imarok]

This patch works completley. One little bug is that the guestsettings setting sometimes resets. Also I need to cleanup the code to remove comments, warnings and follow the CodingConventions.

[Imarok]

Fixes the ticket. The additional loading state will be removed when elexis made his patch for gamesetup.js

[Imarok]

Removed SetNetworkGameAttributes as it is unneeded now, removed the TODO mentioned in the ticket description, extended initDropdowns and some cleanup

[Imarok]

Removed the additonal loading state and done some cleanup

[Imarok]

pass string as ref and reverted L74

[Imarok]

TODO:

• Let player A not cange player B's color (same with civ and team)
• Ensure the change was valid (maybe in a new ticket)

[Imarok]

Let player only change their own color/team/civ if guestsetting!="all", removed the CStr8->CStr changes

[Imarok]

removed the failed hunk and added guest_settings.json that wasn't included in the previous patch

[Imarok]

ChangeSettingMessage only sends changed setting and value, forward ChangeSettingMessage only to host, ignore every AssignPlayerMessage not coming from host, ignore every SetGameAttributesMessage not coming from host

[Imarok]

cpp part is finished. js part needs to wait for #3994

[Imarok]

reverted the renaming of SetNetworkGameAttributes

[Imarok]

Only the part of sending gameSetup and assignments over network

[Imarok]

removed a comment

[Imarok]

WIP Also handle StartNetworkGame, KickPlayer, SetNetworkPlayerStatus, ClearAllPlayerReady over network. Delete SetTurnLength ScriptFunction because it's unused.

[Imarok]

Still needs testing

[Imarok]

Forgot a ! in js

[Imarok]

Don't remove SetTurnLength. Add Transition for NMT_KICKED when syncing

[elexis]

Thanks for working on this big ticket and splitting the patch into smaller managable subsets!

Here my review of your patch that replaces the direct C++ access to the server with network interaction:

• Seems to work on linux here, testing briefly with two instances.
• m_GUIDToAssign -> m_GUID.
• Use ENSURE for the sake of consistency (and cross fingers that for example hotloading session.js won't set g_IsNetworked to false triggering the breakpoint for example).
• Use CStr instead of std::string.
• Since the code doesn't care about the return value of the KickPlayer functions anymore the return value should be changed from bool to void too.
• Comment for ClearAllPlayerReady in the header of the NetClient missing
• The new ready message type is not needed. There already is a ready message type which can be reused to reset the ready state of others. The code could reject the message if a client that isn't the controller tries to reset the state of someone else. However calling SetPlayerReady once for every player when wanting to clear the ready state of all clients causes the sending of a playerassignment message for each client (even for observers). Better use a new message type ClearReady.
• The patch leaves the two ClearAllPlayerReady() of the server unused and copies the CNetServerWorker one, while it should just call that if an authorized client wants to reset ready.
• Since r8511, the CNetServer runs in the main thread, while the CNetServerWorker runs in a separate thread, so that it can respond to messages even when the main thread is dying. Thus the CNetServer contains almost no code (50 lines vs. 1450 lines of the server worker). The CNetServer functions only push the data from the GUI to the worker and don't do anything else. Kicking (#3241) violates this principle and should be made coherent (not necessarily in this patch). Since the code aims to not interact with the server from the GUI anymore, some of these functions became unused and should be removed, for example AssignPlayer (others are still used by the autostart / commandline options)

[Imarok]

Add changes described in comment:10

[Imarok]

Fixed the ready message

[elexis]

Bug: The startgame button is disabled for the host, even if there are no other clients or if they are all ready. Without the patch, the client of the host joins the server, appears as not-ready and then is set to ready right afterwards. With your patch, it just remains not-ready. The reason seems to be gamesetup using emptystring as the GUID for the localhost (yay gamesetup). Came up with that, but it results in an unstoppable readyspam:

@@ -1103,14 +1096,76 @@ bool CNetServerWorker::OnReady(void* con
 	CNetServerSession* session = (CNetServerSession*)context;
 	CNetServerWorker& server = session->GetServer();
 
 	CReadyMessage* message = (CReadyMessage*)event->GetParamRef();
 
-	message->m_GUID = session->GetGUID();
+	if (session->GetGUID() == server.m_HostGUID)
+		server.SetPlayerReady(message->m_GUID, message->m_Status);
+	else
+	{
+		message->m_GUID = session->GetGUID();
+		server.Broadcast(message);
+	}
 
-	server.Broadcast(message);
+	return true;

There may be further tedious related fallacies, but couldn't test yet due to that bug.

• mainlog.html contains all NetMessages send and received by the server, maybe that helps debugging. (At least when greping the file it does)

• Your proposed network protocol change seems to fulfil the requirements for having multiple controllers. (If there are multiple controllers, the game still shouldn't be started without the consent of all other clients. The ready/startgame button would have the startgame-functionality when all other clients are ready and the ready-functionality otherwise. This distinction will be done in the gamesetup script, not inside the server.)

• if (session->GetGUID() != server.m_HostGUID) -> what did I say about the if (!no) else pattern?
• You nuked the second CReadyMessage message, but you should also merge SendReadyMessage and SendSetPlayerStatusMessage since they do the same thing.

[Imarok]

simplify ready message removed some if (!

[Imarok]

Rename SendKickPlayerMessage, Modify comment and remove the kick/ban LOGERROR

[elexis]

Tell me if that works on windows and I'll commit it.

[elexis]

In 18322:

Major network cleanup. Patch by Imarok.

Access the server from the client only, not from the GUI (except for autostarted games).
Thereby lay the foundation for clients to setup the game (refs #3806) and dedicated hosting (refs #3556).
Doesn't transfer nor remove the SetTurnLength showcase from r7936.

[elexis]

Thanks, that's a big step forward!

[Imarok]

rebased version of the cpp part (introduces CChangeSettingMessage)

[elexis]

Backlogging due to lack of progress.

[elexis]

In 19504:

Unify gamesetup option handling, fixes #3994, refs #3049.

Removes a lot of duplication and ugly GUI handling code with very similar, yet different code paths.
Move the setting specific logic to the functions of that setting and keep the universal logic in global atomic functions.
Make gamesetup.xml agnostic of all gamesetup settings.

Adding a new gamesetup option now only requires adding one hunk with the titles and values and one line in g_OptionOrderGUI.
Opens up the possibility to iterate over all settings, refs #3806, #3883.

Allow starting singleplayer games in observermode with only AIs assigned, fixes #4078.
Autocomplete translations of all setting titles and selected title values like playernames and victory conditions.

Transfer the AI difficulty when swapping with a player.
Move logic from onTick to the GUI handling functions.
Change some global consts to var, so that contributors become invited to change them from a different place.
Add missing startGameButton tooltip translation.

Differential Revision: ​https://code.wildfiregames.com/D322
Reviewed By: wraitii
Some strings reviewed By: leper