Opened 8 years ago

Last modified 3 years ago

#3806 new enhancement

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

Reported by: elexis Owned by: Imarok
Priority: Should Have Milestone: Backlog
Component: UI – Game setup Keywords: patch
Cc: rogue-spectre Patch: Phab:D431

Description (last modified by 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

Attachments (25)

3806_player_gamesetup_v0.5.patch (16.8 KB ) - added by Imarok 8 years ago.
WIP Patch: already implemented a test button to set the revealMap setting to false, that can be used by every player
3806_player_gamesetup_v0.6.patch (17.8 KB ) - added by Imarok 8 years ago.
still WIP, changed PlayersCanChange to GuestSettings
3806_player_gamesetup_v0.8.patch (25.9 KB ) - added by Imarok 8 years ago.
wip patch
3806_player_gamesetup_v0.9.patch (31.7 KB ) - added by Imarok 8 years ago.
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.
3806_player_gamesetup_v1.1.patch (31.8 KB ) - added by Imarok 8 years ago.
Fixes the ticket. The additional loading state will be removed when elexis made his patch for gamesetup.js
3806_player_gamesetup_v1.2.patch (32.4 KB ) - added by Imarok 8 years ago.
Removed SetNetworkGameAttributes as it is unneeded now, removed the TODO mentioned in the ticket description, extended initDropdowns and some cleanup
3806_player_gamesetup_v1.4_rebased.patch (29.3 KB ) - added by Imarok 8 years ago.
Removed the additonal loading state and done some cleanup
3806_player_gamesetup_v1.5.patch (30.9 KB ) - added by Imarok 8 years ago.
pass string as ref and reverted L74
3806_player_gamesetup_v1.7.patch (30.4 KB ) - added by Imarok 8 years ago.
Let player only change their own color/team/civ if guestsetting!="all", removed the CStr8->CStr changes
3806_player_gamesetup_v1.7.1.patch (30.4 KB ) - added by Imarok 8 years ago.
removed the failed hunk and added guest_settings.json that wasn't included in the previous patch
3806_player_gamesetup_v2.0.patch (30.8 KB ) - added by Imarok 8 years ago.
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
3806_player_gamesetup_v2.3.patch (44.7 KB ) - added by Imarok 8 years ago.
cpp part is finished. js part needs to wait for #3994
3806_player_gamesetup_v2.4.patch (43.7 KB ) - added by Imarok 8 years ago.
reverted the renaming of SetNetworkGameAttributes
3556_dedServer_v0.2.patch (7.0 KB ) - added by Imarok 8 years ago.
Only the part of sending gameSetup and assignments over network
3556_dedServer_v1.0.patch (6.9 KB ) - added by Imarok 8 years ago.
removed a comment
3556_dedServer_v1.1.patch (14.5 KB ) - added by Imarok 8 years ago.
WIP Also handle StartNetworkGame, KickPlayer, SetNetworkPlayerStatus, ClearAllPlayerReady over network. Delete SetTurnLength ScriptFunction because it's unused.
3556_dedServer_v1.2.patch (15.6 KB ) - added by Imarok 8 years ago.
Still needs testing
3556_dedServer_v1.2.1.patch (15.6 KB ) - added by Imarok 8 years ago.
Forgot a ! in js
3556_dedServer_v1.3.1.patch (14.7 KB ) - added by Imarok 8 years ago.
Don't remove SetTurnLength. Add Transition for NMT_KICKED when syncing
3556_dedServer_v1.4.patch (18.5 KB ) - added by Imarok 8 years ago.
Add changes described in comment:10
3556_dedServer_v1.4.1.patch (18.6 KB ) - added by Imarok 8 years ago.
Fixed the ready message
3556_dedServer_v1.5.patch (18.8 KB ) - added by Imarok 8 years ago.
simplify ready message removed some if (!
3556_dedServer_v1.5.1.patch (18.7 KB ) - added by Imarok 8 years ago.
Rename SendKickPlayerMessage, Modify comment and remove the kick/ban LOGERROR
3556_dedServer_v1.5.2.patch (22.7 KB ) - added by elexis 8 years ago.
Tell me if that works on windows and I'll commit it.
3806_player_gamesetup_v2.4_rebased.patch (11.4 KB ) - added by Imarok 8 years ago.
rebased version of the cpp part (introduces CChangeSettingMessage)

Download all attachments as: .zip

Change History (49)

comment:1 by Itms, 8 years ago

Cc: rogue-spectre added
Priority: Nice to HaveShould Have

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

I think this is an important issue.

comment:2 by elexis, 8 years ago

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.

comment:3 by Imarok, 8 years ago

Owner: set to Imarok

comment:4 by Imarok, 8 years ago

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?

by Imarok, 8 years ago

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

comment:5 by elexis, 8 years ago

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

Last edited 8 years ago by elexis (previous) (diff)

by Imarok, 8 years ago

still WIP, changed PlayersCanChange to GuestSettings

by Imarok, 8 years ago

wip patch

by Imarok, 8 years ago

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.

by Imarok, 8 years ago

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

comment:6 by Imarok, 8 years ago

Keywords: review patch added
Milestone: BacklogAlpha 21

by Imarok, 8 years ago

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

by Imarok, 8 years ago

Removed the additonal loading state and done some cleanup

by Imarok, 8 years ago

pass string as ref and reverted L74

comment:7 by Imarok, 8 years ago

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)

by Imarok, 8 years ago

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

comment:8 by Imarok, 8 years ago

Description: modified (diff)
Keywords: review patch removed

by Imarok, 8 years ago

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

by Imarok, 8 years ago

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

comment:9 by Imarok, 8 years ago

Description: modified (diff)

by Imarok, 8 years ago

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

by Imarok, 8 years ago

reverted the renaming of SetNetworkGameAttributes

by Imarok, 8 years ago

Attachment: 3556_dedServer_v0.2.patch added

Only the part of sending gameSetup and assignments over network

by Imarok, 8 years ago

Attachment: 3556_dedServer_v1.0.patch added

removed a comment

by Imarok, 8 years ago

Attachment: 3556_dedServer_v1.1.patch added

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

by Imarok, 8 years ago

Attachment: 3556_dedServer_v1.2.patch added

Still needs testing

by Imarok, 8 years ago

Attachment: 3556_dedServer_v1.2.1.patch added

Forgot a ! in js

by Imarok, 8 years ago

Attachment: 3556_dedServer_v1.3.1.patch added

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

comment:10 by elexis, 8 years ago

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)

by Imarok, 8 years ago

Attachment: 3556_dedServer_v1.4.patch added

Add changes described in comment:10

by Imarok, 8 years ago

Attachment: 3556_dedServer_v1.4.1.patch added

Fixed the ready message

comment:11 by elexis, 8 years ago

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.

by Imarok, 8 years ago

Attachment: 3556_dedServer_v1.5.patch added

simplify ready message removed some if (!

by Imarok, 8 years ago

Attachment: 3556_dedServer_v1.5.1.patch added

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

by elexis, 8 years ago

Attachment: 3556_dedServer_v1.5.2.patch added

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

comment:12 by elexis, 8 years ago

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.

comment:13 by elexis, 8 years ago

Keywords: patch added
Summary: Gamesetup - Optionally allow players to setup the game[PATCH] Gamesetup - Optionally allow players to setup the game

Thanks, that's a big step forward!

by Imarok, 8 years ago

rebased version of the cpp part (introduces CChangeSettingMessage)

comment:14 by Andy Alt, 8 years ago

Keywords: review added

comment:15 by Imarok, 8 years ago

Keywords: review removed

comment:16 by elexis, 8 years ago

Milestone: Alpha 21Backlog

Backlogging due to lack of progress.

comment:17 by elexis, 7 years ago

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

comment:18 by elexis, 7 years ago

Description: modified (diff)
Patch: Phab:D431

comment:19 by elexis, 7 years ago

Milestone: BacklogAlpha 23

Would be really awesome to finish this for the upcoming release

comment:20 by elexis, 6 years ago

Milestone: Alpha 23Backlog

comment:21 by Imarok, 5 years ago

Component: UI & SimulationGame setup

Move tickets to Game Setup as UI & Simulation got some sub components.

comment:22 by elexis, 4 years ago

In 23374:

Gamesetup class rewrite, fixes #5322, refs #5387.

  • Decouples settings logically which in turn allows fixing many problems arising from previous coupling.
  • Fixes the persist-match-settings feature, refs #2963, refs #3049.
  • Improves performance of the matchsetup by rebuilding GUI objects only when necessary.

Provides groundwork for:

  • UI to control per-player handicap, such as StartingResources, PopulationCap, StartingTechnologies, DisabledTechnologies, DisabledTemplates, ..., refs #812.
  • Map specific settings (onMapChange event), refs #4838.
  • Chat notifications announcing which settings changed, refs D1195,
  • Multiple controllers setting up the game (since setting types can check for permissions in onUpdateGameAttributes without the need for a new data model or a second gamesetup data network message type), refs #3806, subsequently dedicated server, refs #3556.
  • MapBrowser (MapCache, MapTypes, onUpdateGameAttributes interface), refs D1703 and D1777,
  • Multiplayer saved games (decoupling and setting dependent unique logic), refs #1088.

Refs https://wildfiregames.com/forum/index.php?/topic/20787-paid-development-2016/ https://wildfiregames.com/forum/index.php?/topic/20789-paid-development-2016/

Enable maps to restrict setting values:

  • If a map specifies an AI or Civs for a playerslot, the controller can't assign a player/other AI or Civ to that slot, refs #3049, #3013.

Fix per player StartingResources, PopulationCap, StartingTechnologies, DisabledTechnologies, DisabledTemplates following rP12756, refs #812, fixes #4504. Use this for DisabledTechnologies on Polar Sea.

Persist user settings for Skirmish maps:

  • All user chosen settings are persisted when changing the selected map or maptype, except where the selected map overwrites the setting value and except for Scenario maps which still use the default value where the map doesn't specify the setting value.
  • Tickets relating to that Skirmish mapchange user setting persistance:
    • Selecting a map doesn't change the selected civilizations, fixes #3120 (together with r23279 removing map specified Civs).
    • Selecting a map type doesn't reset the selected settings, fixes #5372.
    • Selecting a map doesn't change the selected victory conditions, unless the map specifies those, refs #4661, #3209. (Atlas still writes VictoryConditions to every map.)
    • Consume the player color palette from Skirmish maps, refs rP17040 / #1580. Preserve the selected playercolors when switching the Skirmish/Random map by chosing the most similar colors if the map comes with a different palette.

Rated games:

  • Hide and disable Rated game setting unless there are exactly two players, fixes #3950, supersedes D2117.
  • Display conspicuous warning if the game is rated, so players are perfectly aware.

Autostarted games:

  • Allow using the gamesetup page to autostart matches with arbitrary maps, not only this one tutorial, as reported in D194 and rP19599, refs D11.

Networking:

  • Keep gamesetup page open after disconnect, allowing players to read chat messages indicating why the host stopped the server, fixes #4114.
  • The message subscription system allows new and mod settings to run custom logic on arbitrary setting changes (most importantly on map change). This removes hardcoded logic restrictions from the gamesetup option unification rewrite in rP19504/D322, refs #3994, such as the hardcoding of setting references in selectMap to biomes from rP20115/D852 and the difficulty from rP20760/D1189, RelicDuration, WonderDuration, LastManStanding, RegicideGarrison, TriggerScripts, CircularMap, Garrison, DisabledTemplates.

Checkboxes:

  • Display values of disabled checkboxes with Yes/No labels, fixes D2349, reviewed by nani.

Clean g_GameAttributes of invalid values and gamesetup GUI temporaries, refs #3049, #3883:

MapCache:

  • Refactor to MapCache class, store maps of all types and use it in the replaymenu, lobby and session as well.

SettingTabsPanel:

  • Remove hardcodings and coupling of the SettingTabsPanel with biomes/difficulties/chat UI from D1027/rP20945.

GamesetupPage.xml:

  • Restructure the page to use hierarchical object organization (topPanel, centerPanel, centerLeftPanel, bottomPanel, centerCenterPanel, centerRightPanel, bottomLeftPanel, bottomRightPanel), allowing to deduplicate object position margins and size math and ease navigation.

New defaults:

  • Check LockedTeams default in multiplayer (not only rated games).
  • Persist the rated game setting instead of defaulting to true when restarting a match, which often lead to unintentional rated games when rehosting.
  • 60 FPS in menus since they are animated

Autocomplete sorting fixed (playernames should be completed first).
Refactoring encompasses the one proposed in Polakrity and bb D1651.

Differential Revision: https://code.wildfiregames.com/D2483
Tested by: nani
Discussed with:

Emojis by: asterix, Imarok, fpre, nani, Krinkle, Stan, Angen, Freagarach

comment:23 by Imarok, 3 years ago

refs r25077:

Separate Game Settings from the GUI Gamesetup

Split the gamesetup in two: the 'GameAttributes' part into gamesettings/ and the GUI/Presentation part in gamesetup/. This makes it easier to separate the presentation from the data.

The immediate benefit is that campaigns & autostart scripts don't need to load the gamesetup folder at all. This also makes it much easier for any modder that would want to to change the GameSetup itself.

Each 'game attribute' is given a unique class extending GameSetting (with a few exceptions), in charge of:

'Serializing' to the JSON-compatible 'InitAttributes' format, which is used for persisted settings, network synchronization, map script settings, hotloading. Deserializing from the same format. Watching for settings it depends on (such that e.g. unexploring the map also unreveals it).

The GUI controls remain in charge of presenting the state accurately, however they now directly subscribe to changes of the GameSettings for update. The updating logic in general has been lightened on the GUI side, making it more straightforward to know when something will update, and reducing un-necessary computations (in theory - in practice, I believe the gamesetup was already fairly good about this). The 'Controller' class of the gamesetup have also been lightened, since more responsibility now lies with GameSettings. In particular, this include code to actually launch a game.

In general the GameSettings class is permissive - the GUI gamesetup has tighter restriction for what the player can/cannot modify. This is intended to give flexibility for campaign maps, which may want to change arbitrary settings.

Further work would be useful, non-exhaustively:

the setting of default values remains messy. They currently exist somethings in GameSettings, sometimes in the GUI gamesetup, and in the simulation itself (in case attributes are not set). the availability and 'lockedness' of settings remains a work-in-progress. some attributes, like disabled technologies, should probably be removed and triggers used instead. the Handling of AI and player-specific data could be improved. settings Persistence should follow its own path - not all settings are worth persisting. GAIA settings are added simulation-side but not in the GUI, which is confusing.

Thanks langbart & Freagarach for testing.

Follows the gamesetup rewrite in rP23374.

Refs #3049

Differential Revision: https://code.wildfiregames.com/D3243

comment:24 by wraitii, 3 years ago

In 25099:

MP: don't enforce game init attributes synchronization in PREGAME.

The NetServer stored a complete copy of the game Init Attributes, which it sent to new clients on updates from the controller. This worked well, but prevents incremental updates and other unrelated messages from being sent.

This changes the system so that:

  • in PREGAME state, the server does not update its copy of the game init attributes
  • the server forwards game setup messages from the controller to all clients
  • Joining clients get a full copy of the Settings, when joining, from the controller (this is a js-driven behaviour - other situations might not need do it).
  • Make the StartNetworkGame message take a copy of the final init attributes, to ensure synchronization (and simplify some logic).

In practice, this:

  • makes it possible to send different types of gamesetup messages (this introduces two: a regular update and the full 'initial-update' for new clients).
  • moves some C++ hardcoding into JS - here in essence the PREGAME server state is now init-attributes-agnostic.
  • does not change much for readiness control - the server already needed to force a change at game start to set random elements.

Note that the loading page is currently still receiving the 'local' game attributes, which assumes that all clients are correctly synchronized (they should be).

Refs #3806, #3049

Differential Revision: https://code.wildfiregames.com/D3714

Note: See TracTickets for help on using tickets.