Paths

Table of Contentst

Optionally assign only buddies to unused playerslots
ClosedPublic
Actions

Authored by elexis on May 23 2017, 9:01 PM.

Details

Reviewers

ffffffff
scythetwirler

Commits

rP19666: Option to prevent joining clients from becoming automatically assigned to…

Summary

When hosting a game, one often wants to play only with some buddies, not with anyone who joins.
But since everyone is assigned automatically to an unused playerslot, unwanted players will be disappointed or get the wrong impression if they become unassigned if the host was briefly afk and didn't unassign immediately.

With D209 players can select buddies in the lobby list. This feature can be used to add a new option that changes the mechanism to only assign joining players to unused slots if they are buddies.
This way some of these discussions are prevented and a lot of configuration work for the controller can be saved.

Test Plan

Either edit your user.cfg or join the lobby with two accounts and make one of these a buddy.
Enable the option. Host a game. Join without lobby, via IP 127.0.0.1.
Since you can chose the nickname freely, you can test whether it works as advertized.

Diff Detail

Repository

rP 0 A.D. Public Repository

Branch

/ps/trunk

Lint

Lint OK

Unit

No Unit Test Coverage

Build Status

Buildable 1973
Build 3159: Vulcan Build (Windows)	Jenkins
Build 3158: Vulcan Build	Jenkins
Build 3157: arc lint + arc unit

Event Timeline

elexis created this revision.May 23 2017, 9:01 PM

Sandarac added a subscriber: Sandarac.May 23 2017, 9:15 PM

Sandarac added inline comments.

binaries/data/config/default.cfg
380	Notice how every other config option in this file is `foobar`, not `boo_bar`?
binaries/data/mods/public/gui/gamesetup/gamesetup.js
1292	Useless comment.
1293	Wrong indentation.

Executing section Default...
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (no-undef-init):
|    | It's not necessary to initialize 'g_GameStanzaTimer' to undefined.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.js
| 303| 303| /**
| 304| 304|  * Index of the GUI timer.
| 305| 305|  */
| 306|    |-var g_GameStanzaTimer = undefined;
|    | 306|+var g_GameStanzaTimer;
| 307| 307| 
| 308| 308| /**
| 309| 309|  * Only send a lobby update if something actually changed.
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before 'name'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.js
| 889| 889| 				name =
| 890| 890| 					'[color="' +
| 891| 891| 					g_ReadyData[assignedGUID ? g_PlayerAssignments[assignedGUID].status : 2].color +
| 892|    |-					'"]' +  name + '[/color]';
|    | 892|+					'"]' + name + '[/color]';
| 893| 893| 
| 894| 894| 			return name;
| 895| 895| 		},
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before 'attribs'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.js
| 927| 927| 
| 928| 928| 	g_IsNetworked = attribs.type != "offline";
| 929| 929| 	g_IsController = attribs.type != "client";
| 930|    |-	g_IsTutorial = attribs.tutorial &&  attribs.tutorial == true;
|    | 930|+	g_IsTutorial = attribs.tutorial && attribs.tutorial == true;
| 931| 931| 	g_ServerName = attribs.serverName;
| 932| 932| 	g_ServerPort = attribs.serverPort;
| 933| 933| 
|    | [NORMAL] ESLintBear (no-undef-init):
|    | It's not necessary to initialize 'yPos' to undefined.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1127|1127| 
|1128|1128| function verticallyDistributeGUIObjects(parent, objectHeight, ignore)
|1129|1129| {
|1130|    |-	let yPos = undefined;
|    |1130|+	let yPos;
|1131|1131| 
|1132|1132| 	let parentObject = Engine.GetGUIObjectByName(parent);
|1133|1133| 	for (let child of parentObject.children)
|    | [NORMAL] ESLintBear (no-extra-boolean-cast):
|    | Redundant double negation.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1231|1231| 
|1232|1232| 	g_GameAttributes = message.data;
|1233|1233| 
|1234|    |-	if (!!g_GameAttributes.settings.RatingEnabled)
|    |1234|+	if (g_GameAttributes.settings.RatingEnabled)
|1235|1235| 	{
|1236|1236| 		g_GameAttributes.settings.CheatsEnabled = false;
|1237|1237| 		g_GameAttributes.settings.LockTeams = true;

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1560| »   while·(g_IsNetworked)
|    | [NORMAL] ESLintBear (no-unmodified-loop-condition):
|    | 'g_IsNetworked' is not modified in this loop.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
| 306| var·g_GameStanzaTimer·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'g_GameStanzaTimer' to 'undefined'.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
| 930| »   g_IsTutorial·=·attribs.tutorial·&&··attribs.tutorial·==·true;
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with 'true'.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1130| »   let·yPos·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'yPos' to 'undefined'.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1373| »   »   let·filterID·=·g_MapFilterList.id.findIndex(id·=>·id·==·g_GameAttributes.mapFilter);
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1542| »   if·(g_LoadingState·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1594| »   »   if·(playerData.some((pData,·j)·=>·i·!=·j·&&·sameColor(playerData[i].Color,·pData.Color)))
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1595| »   »   »   playerData[i].Color·=·g_PlayerColorPickerList.find(color·=>·playerData.every(pData·=>·!sameColor(color,·pData.Color)));
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1772| »   »   »   chosenCiv·=·pickRandom(Object.keys(g_CivData).filter(civ·=>·g_CivData[civ].Culture·==·culture));
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1786| »   »   let·usedName·=·g_GameAttributes.settings.PlayerData.filter(pData·=>·pData.Name·&&·pData.Name.indexOf(chosenName)·!==·-1).length;
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|2219| »   if·(g_GameStanzaTimer·!=·undefined)
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with 'undefined'.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|2256| »   if·(g_GameStanzaTimer·!=·undefined)
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with 'undefined'.
Executing section Python...
Executing section Perl...

http://jw:8080/job/phabricator_lint/24/ for more details.

Harbormaster failed remote builds in B1928: Diff 2158!May 23 2017, 11:50 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/1332/ for more details.

Im waiting for ur replies to the comments

How about sortin Teamplayers in Summary by Total Score numbers and showing them 1,2,3,4th place?

elexis marked 3 inline comments as done.May 24 2017, 3:46 PM

elexis added inline comments.

binaries/data/config/default.cfg
380	This happens to me each time I add an option. -.-
binaries/data/mods/public/gui/gamesetup/gamesetup.js
1292	I'd lead to documenting this too much than too few, since iirc I introduced a bug that had to be fixed with D50 and caused me to post a code-travolta meme, so I'd rather avoid that with a comment.
1293	Guess this came from the lint patch I had let it apply when uploading with arc

Fix space and underscore

(We really should have an option to mark players as buddies when they join the gamesetup. This is something which could be added to the network dialog.)

Not sure this is the right way to implement this. Other games often have a "spectate" button and a "join" button, with the join button (and/or the spectate button) settable to require a password by the host. IMO this seems like like over-use of the buddy system (and makes it harder to reason through).

In D546#22312, @scythetwirler wrote:

Not sure this is the right way to implement this.

Which this exactly?

Other games often have a "spectate" button and a "join" button, with the join button (and/or the spectate button) settable to require a password by the host.

Giving the client the choice whether he wants to play is a must have, but orthogonal to the purpose of this patch.

IMO this seems like like over-use of the buddy system (and makes it harder to reason through).

If that refers to the non-lobbied gamesetup, yes, I just said that it doesn't conflict with it.
If that refers to the lobbied gamesetup, please refer to reasoning stated.

This proposal is not about giving the client but the host control over the gamesetup by removing repetitive gamesetup controls.
, who should be able to decide who he wants to play with.
This has become especially problematic since now 40 clients can join the gamesetup.

Example:

The host want to play a game with two friends who are online and potentially other people who he plays regularly with if they join until the game starts
Besides the two intended players, one or two handful of clients join that the host doesn't want to play with. Yet the current gamesetup assigns them as players.
For each unwanted player, the host needs at least 3 clicks to remove it:
- one click onto the dropdown
- one scrollbar interaction (mousewheel or clicking on the scrollbar)
- one click on the "Unassigned" item Which makes about 10-20 timewasting clicks already up to this point.

Notice the players must be unassigned immediately, otherwise an expectation that the assigned player can actually play builds up with each minute.
So the host is pressed for time even to execute the pointless clicks.

With the patch applied, he not only saves the pointless clicks to remove unwanted players, he can even go afk for a minute or two and keep that last slot
until the last one joined.

This repeats itself not every numerous times per gamesetup, but every single game.

If the buddy list was abolished, then we should still add the proposed gamesetup option, not as a dropdown with 3 choices but one with 2 choices (assign joining clients to unused playerslots or not).
However adding the third option seems nice to have, it will save even more clicks (assigning the wanted ones will be saved too).

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/1342/ for more details.

Executing section Default...
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (no-undef-init):
|    | It's not necessary to initialize 'g_GameStanzaTimer' to undefined.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.js
| 303| 303| /**
| 304| 304|  * Index of the GUI timer.
| 305| 305|  */
| 306|    |-var g_GameStanzaTimer = undefined;
|    | 306|+var g_GameStanzaTimer;
| 307| 307| 
| 308| 308| /**
| 309| 309|  * Only send a lobby update if something actually changed.
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before 'name'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.js
| 889| 889| 				name =
| 890| 890| 					'[color="' +
| 891| 891| 					g_ReadyData[assignedGUID ? g_PlayerAssignments[assignedGUID].status : 2].color +
| 892|    |-					'"]' +  name + '[/color]';
|    | 892|+					'"]' + name + '[/color]';
| 893| 893| 
| 894| 894| 			return name;
| 895| 895| 		},
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before 'attribs'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.js
| 927| 927| 
| 928| 928| 	g_IsNetworked = attribs.type != "offline";
| 929| 929| 	g_IsController = attribs.type != "client";
| 930|    |-	g_IsTutorial = attribs.tutorial &&  attribs.tutorial == true;
|    | 930|+	g_IsTutorial = attribs.tutorial && attribs.tutorial == true;
| 931| 931| 	g_ServerName = attribs.serverName;
| 932| 932| 	g_ServerPort = attribs.serverPort;
| 933| 933| 
|    | [NORMAL] ESLintBear (no-undef-init):
|    | It's not necessary to initialize 'yPos' to undefined.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1127|1127| 
|1128|1128| function verticallyDistributeGUIObjects(parent, objectHeight, ignore)
|1129|1129| {
|1130|    |-	let yPos = undefined;
|    |1130|+	let yPos;
|1131|1131| 
|1132|1132| 	let parentObject = Engine.GetGUIObjectByName(parent);
|1133|1133| 	for (let child of parentObject.children)
|    | [NORMAL] ESLintBear (no-extra-boolean-cast):
|    | Redundant double negation.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1231|1231| 
|1232|1232| 	g_GameAttributes = message.data;
|1233|1233| 
|1234|    |-	if (!!g_GameAttributes.settings.RatingEnabled)
|    |1234|+	if (g_GameAttributes.settings.RatingEnabled)
|1235|1235| 	{
|1236|1236| 		g_GameAttributes.settings.CheatsEnabled = false;
|1237|1237| 		g_GameAttributes.settings.LockTeams = true;

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1560| »   while·(g_IsNetworked)
|    | [NORMAL] ESLintBear (no-unmodified-loop-condition):
|    | 'g_IsNetworked' is not modified in this loop.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
| 306| var·g_GameStanzaTimer·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'g_GameStanzaTimer' to 'undefined'.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
| 930| »   g_IsTutorial·=·attribs.tutorial·&&··attribs.tutorial·==·true;
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with 'true'.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1130| »   let·yPos·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'yPos' to 'undefined'.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1373| »   »   let·filterID·=·g_MapFilterList.id.findIndex(id·=>·id·==·g_GameAttributes.mapFilter);
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1542| »   if·(g_LoadingState·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1594| »   »   if·(playerData.some((pData,·j)·=>·i·!=·j·&&·sameColor(playerData[i].Color,·pData.Color)))
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1595| »   »   »   playerData[i].Color·=·g_PlayerColorPickerList.find(color·=>·playerData.every(pData·=>·!sameColor(color,·pData.Color)));
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1772| »   »   »   chosenCiv·=·pickRandom(Object.keys(g_CivData).filter(civ·=>·g_CivData[civ].Culture·==·culture));
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1786| »   »   let·usedName·=·g_GameAttributes.settings.PlayerData.filter(pData·=>·pData.Name·&&·pData.Name.indexOf(chosenName)·!==·-1).length;
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|2219| »   if·(g_GameStanzaTimer·!=·undefined)
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with 'undefined'.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|2256| »   if·(g_GameStanzaTimer·!=·undefined)
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with 'undefined'.
Executing section Python...
Executing section Perl...

http://jw:8080/job/phabricator_lint/34/ for more details.

Harbormaster failed remote builds in B1941: Diff 2171!May 24 2017, 11:57 PM

sadly i cant test at moment

no host

Imarok awarded a token.May 25 2017, 8:41 PM

In D546#22312, @scythetwirler wrote:

IMO this seems like like over-use of the buddy system (and makes it harder to reason through).

Imho it's a nice feature which brings the buddy feature to be really usefull now.
(I think nothing speaks against password protection and/or a spectate button, additionally to this patch)

Couple reasons:

Yes, it might reduce the number of clicks for the host, but it creates confusion on the part of the client, who has no idea whether the host has set him as buddy.
This feature seems like it would also be hard to explain to hosts. Where would you put a tooltip or something that says "if you buddy someone, they will be automatically assigned upon joining"?
Possible solution: Add a list of unassigned players to the mp gamesetup screen somewhere visible so we don't have to /list too much. Default everyone to unassigned, buddy or not, upon joining. (Besides, isn't the chance that all your buddies arrive in team order is fairly slim that would require some player swapping anyway?) This also doesn't give joining players the impression that they are assigned. Further down the line I think it's planned to have clients be able to assign themselves and select their own civs so this "gamesetup lobby" approach seems to fit in well.

In D546#22494, @scythetwirler wrote:

Couple reasons:

Yes, it might reduce the number of clicks for the host, but it creates confusion on the part of the client, who has no idea whether the host has set him as buddy.

Does it matter though? He can still ask to become assigned like the other clients who become observers if all slots are assigned.

This feature seems like it would also be hard to explain to hosts. Where would you put a tooltip or something that says "if you buddy someone, they will be automatically assigned upon joining"?

That tooltip is in the setting that enables the change.

Possible solution: Add a list of unassigned players to the mp gamesetup screen somewhere visible so we don't have to /list too much.

The host has this list when he clicks on playerdropdowns as usual. The network dialog is also going to provide such a list to everyone (was the one feature I wanted to implement for a21 but didn't.)

Default everyone to unassigned, buddy or not, upon joining.

Yes, that can be a third option (everyone, players, noone)

Besides, isn't the chance that all your buddies arrive in team order is fairly slim that would require some player swapping anyway?

Yes, but we still avoid the issues above, the fact that the host has to unassign all players instantly until everyone who is intended to play has joined

This also doesn't give joining players the impression that they are assigned

If they are buddies and that setting is set, yes.

Further down the line I think it's planned to have clients be able to assign themselves and select their own civs so this "gamesetup lobby" approach seems to fit in well.

Doesn't conflict, as it's still the host/controller that decides how much freedom he gives the clients. If players can assign themselves, then this setting can be ignored.

very good
works!
delete underscore in options.json !

binaries/data/mods/public/gui/options/options.json
296	remove underscore

After discussion on IRC, we agreed to also add an "unassign by default" option.

This revision now requires changes to proceed.May 25 2017, 10:03 PM

Make that a choice between three options.
Increase the width of options page by 10px left and 10px right and use those 20px for the general settings column, so that at least the english string fits.

Executing section Default...
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (no-undef-init):
|    | It's not necessary to initialize 'g_GameStanzaTimer' to undefined.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.js
| 303| 303| /**
| 304| 304|  * Index of the GUI timer.
| 305| 305|  */
| 306|    |-var g_GameStanzaTimer = undefined;
|    | 306|+var g_GameStanzaTimer;
| 307| 307| 
| 308| 308| /**
| 309| 309|  * Only send a lobby update if something actually changed.
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before 'name'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.js
| 889| 889| 				name =
| 890| 890| 					'[color="' +
| 891| 891| 					g_ReadyData[assignedGUID ? g_PlayerAssignments[assignedGUID].status : 2].color +
| 892|    |-					'"]' +  name + '[/color]';
|    | 892|+					'"]' + name + '[/color]';
| 893| 893| 
| 894| 894| 			return name;
| 895| 895| 		},
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before 'attribs'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.js
| 927| 927| 
| 928| 928| 	g_IsNetworked = attribs.type != "offline";
| 929| 929| 	g_IsController = attribs.type != "client";
| 930|    |-	g_IsTutorial = attribs.tutorial &&  attribs.tutorial == true;
|    | 930|+	g_IsTutorial = attribs.tutorial && attribs.tutorial == true;
| 931| 931| 	g_ServerName = attribs.serverName;
| 932| 932| 	g_ServerPort = attribs.serverPort;
| 933| 933| 
|    | [NORMAL] ESLintBear (no-undef-init):
|    | It's not necessary to initialize 'yPos' to undefined.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1127|1127| 
|1128|1128| function verticallyDistributeGUIObjects(parent, objectHeight, ignore)
|1129|1129| {
|1130|    |-	let yPos = undefined;
|    |1130|+	let yPos;
|1131|1131| 
|1132|1132| 	let parentObject = Engine.GetGUIObjectByName(parent);
|1133|1133| 	for (let child of parentObject.children)
|    | [NORMAL] ESLintBear (no-extra-boolean-cast):
|    | Redundant double negation.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1231|1231| 
|1232|1232| 	g_GameAttributes = message.data;
|1233|1233| 
|1234|    |-	if (!!g_GameAttributes.settings.RatingEnabled)
|    |1234|+	if (g_GameAttributes.settings.RatingEnabled)
|1235|1235| 	{
|1236|1236| 		g_GameAttributes.settings.CheatsEnabled = false;
|1237|1237| 		g_GameAttributes.settings.LockTeams = true;

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1562| »   while·(g_IsNetworked)
|    | [NORMAL] ESLintBear (no-unmodified-loop-condition):
|    | 'g_IsNetworked' is not modified in this loop.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
| 306| var·g_GameStanzaTimer·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'g_GameStanzaTimer' to 'undefined'.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
| 930| »   g_IsTutorial·=·attribs.tutorial·&&··attribs.tutorial·==·true;
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with 'true'.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1130| »   let·yPos·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'yPos' to 'undefined'.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1375| »   »   let·filterID·=·g_MapFilterList.id.findIndex(id·=>·id·==·g_GameAttributes.mapFilter);
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1544| »   if·(g_LoadingState·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1596| »   »   if·(playerData.some((pData,·j)·=>·i·!=·j·&&·sameColor(playerData[i].Color,·pData.Color)))
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1597| »   »   »   playerData[i].Color·=·g_PlayerColorPickerList.find(color·=>·playerData.every(pData·=>·!sameColor(color,·pData.Color)));
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1774| »   »   »   chosenCiv·=·pickRandom(Object.keys(g_CivData).filter(civ·=>·g_CivData[civ].Culture·==·culture));
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1788| »   »   let·usedName·=·g_GameAttributes.settings.PlayerData.filter(pData·=>·pData.Name·&&·pData.Name.indexOf(chosenName)·!==·-1).length;
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|2221| »   if·(g_GameStanzaTimer·!=·undefined)
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with 'undefined'.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|2258| »   if·(g_GameStanzaTimer·!=·undefined)
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with 'undefined'.
Executing section Python...
Executing section Perl...

http://jw:8080/job/phabricator_lint/51/ for more details.

Harbormaster failed remote builds in B1973: Diff 2210!May 26 2017, 5:51 AM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/1365/ for more details.

In terms of use for a typical user, wouldn't we also want this setting in the gamesetup windows? I'd imagine that the options page on the main menu would be the global default while gamesetup could hold match/this instance specific settings. e.g. if we want to set assigned by default and find that too many irrelevant players are joining, we could switch the setting from within gamesetup so we don't have to re-host to change it.

scythetwirler added inline comments.May 26 2017, 6:44 PM

binaries/data/mods/public/gui/gamesetup/gamesetup.js
1298	Indent?
binaries/data/mods/public/gui/options/options.json
89	Noone->No one

That "More Options" dialog only contains simulation relevant settings. I would advise against adding an option there that is not related to the actual matchsettings (like the late observer flag was once before it was moved to the options dialog).
It would require a new sub-page Hosting-Settings in which we could show other settings too (like the late observer setting or the dropdown proposed to allow the host providing privileges to other clients).
This page will be worth it once we have more than one dropdown in that page, right?
(The "Spectate" button shouldn't be part of either, just a new button besides Ready and Cancel)

good

Thx for the remarks fpre, scythetwirler and Imarok.