[PATCH] Require that all users confirm game settings before host can start game.

[scythetwirler]

We should have some kind of confirmation screen that all players accept the current configuration of map, civs, teams etc.

This will probably also involve including some kind of kicking capability for the host so that clients that want to abuse the system by rejecting all settings every time and preventing the game from starting can be dealt with.

This confirmation should not inhibit the client's ability to actually view the settings that they are about to confirm/reject.

[scythetwirler]

Something's not right with the image scaling of the checkboxes (they get cut off) and I'm not too familiar with that part of the code. Any ideas? EDIT: fixed. Thanks historicbruno!

[scythetwirler]

Side effect: deletes the player name column because it wouldn't fit otherwise.

[Josh]

We shouldn't be communicating important data using hidden chat messages, we have an existing system for sending that data. I've started working on extending the C++ API to supports a per-user "status".

[scythetwirler]

Uploaded a new version that uses the network C++ API.

[Josh]

The patch is well coded, but I think the separation between ReadyData and PlayerAssignments is synthetic. In most occasions where g_PlayerAssignments would be accessed or changed, g_ReadyData would be as well. Separating them allows more surface for bugs to appear and creates repetitive logic. In the worst case, combining the objects and messages results in the ReadyData field being unused sometimes. I really dislike asking people to go rewrite working code, but I think the benefits are worthwhile in this case. If I remember correctly, a great computer science professor at Stanford said something like, "the mark of a true programmer is not in how well the code works, but in how well others can read and understand it."

[scythetwirler]

ReadyData and PlayerAssignments have been merged in this patch.

[scythetwirler]

There's some remnants of the checkbox GUI code commented out that I'll take out if this current GUI is accepted.

[scythetwirler]

TODO: Convert the changes to be internationalization-friendly (also make an updated diff after the internationalization commit).

[scythetwirler]

I'd like some people to help test this, as it's difficult to test multiplayer by myself.

[sanderd17]

Line 172 of gameSetup.xml has a small mistake in it (325 instead of 315 px). Not your fault, but it will be easier if you fix it (no conflicts).

[scythetwirler]

Replying to sanderd17:

Line 172 of gameSetup.xml has a small mistake in it (325 instead of 315 px). Not your fault, but it will be easier if you fix it (no conflicts).

Fixed in this version :)

[leper]

gamesetup.js:

• L452: Could just be if(a && b) instead of if (a) if (b).
• L903: The check for the automatically added " ($number)" string is a hack. (Having a player with a nick with a "(" in it will break it)
• L1254 and L1268: No need to add trailing whitespace.
• L1562: Make it else if (a && b)
• L1575: Same, also move the comment to a new line.
• L1673: Use if(!a) continue.
• L1695: Missing semicolon.

NetHost.h:

• Why is it an i32?

NetServer.cpp:

• L688: Useless braces.
• Switch the parameters for ReadyPlayer().

[scythetwirler]

Enrique's gear icon.

[scythetwirler]

Enrique's gear icon

[scythetwirler]

Replying to leper:

gamesetup.js:

• L452: Could just be if(a && b) instead of if (a) if (b).
• L903: The check for the automatically added " ($number)" string is a hack. (Having a player with a nick with a "(" in it will break it)
• L1254 and L1268: No need to add trailing whitespace.
• L1562: Make it else if (a && b)
• L1575: Same, also move the comment to a new line.
• L1673: Use if(!a) continue.
• L1695: Missing semicolon.

NetHost.h:

• Why is it an i32?

NetServer.cpp:

• L688: Useless braces.

Thanks for the review. These have been resolved.

Replying to leper:

• Switch the parameters for ReadyPlayer().

I'm not sure what you mean? If you meant swapping the order, I just followed the same order AssignPlayer had.

[scythetwirler]

On hold until #2482 is resolved.

[scythetwirler]

There's a bug I need to iron out.

[scythetwirler]

Bug fixed and order of ReadyPlayer parameters swapped.

[scythetwirler]

Any objections to committing?

[scythetwirler]

Updated with i18n.

[Josh]

I haven't gotten the patch working locally yet, but I can't tell how you are supposed to change your ready status from that screenshot?

[scythetwirler]

That's from the host's point of view; from a client's perspective, the start game button switches between "I'm ready!" and "I'm not ready".

[Josh]

Hmm, I thought we were going for something similar to that checkbox-style system you showed earlier, but I guess this could work.

Also please remove the asterisk before the ready messages. It's inconsistent on the gamesetup screen.

Another thing that wasn't caused by your patch, but could you colorize the other uses of the player name on the gamesetup screen? (for example the player leave/join messages in your screenshot)

[scythetwirler]

In 15006:

Implements ready status into gamesetup. Fixes #2447.

[scythetwirler]

In 15033:

Don't display I'm ready in single player setup. Refs #2447.

[elexis]

In 19396:

Update ready state immediately when receiving the message.

Differential Revision: ​https://code.wildfiregames.com/D304
Reviewed By: Vladislav
Refs #2447