Allow specifying gaia settings in maps, and do so in the maps.
AbandonedPublic
Actions

Authored by • leper on Jul 13 2017, 12:46 AM.

Details

Reviewers

elexis
Sandarac

Group Reviewers

Restricted Owners Package	(Owns No Changed Paths)
Restricted Owners Package	(Owns No Changed Paths)

Trac Tickets

#3886
#3998

Summary

This is WIP, but I'd like a few comments/suggestions on the gamesetup changes.

NOTE: I just include one map in this, since arc seems to fail uploading the diff where I change all maps. (Add gaia, some cleanup of things that are specified in the defaults.)

TODO: make this and D642 a new map xml version.
TODO: Add a script to update maps. (Also for D642, see P59)
TODO: Actually just have {} instead of { "Civ": "gaia" } since that is the default that gets used.
TODO: Make Atlas emit {} for gaia instead of null
TODO: cleanup the gui changes.
TODO: Remove debugging code (warns/commented out code), fix TODOs

Test Plan

Test that starting maps and the gamesetup still works as before.

Add another player template, possibly with a new custom component that does something (eg warn something), specify it as the civ in the map, start that map, see that it works.

Diff Detail

Repository

rP 0 A.D. Public Repository

Branch

gaia_playerattr

Lint

Lint Skipped

Unit

Unit Tests Skipped

Build Status

Buildable 2599
Build 4402: Vulcan Build (Windows)	Jenkins
Build 4401: Vulcan Build	Jenkins
Build 4400: arc lint + arc unit

Event Timeline

• leper created this revision.Jul 13 2017, 12:46 AM

Owners added a subscriber: Restricted Owners Package.Jul 13 2017, 12:46 AM

• leper edited the test plan for this revision. (Show Details)Jul 13 2017, 12:49 AM

• leper updated the Trac tickets for this revision.

Build has FAILED

Link to build: http://jw:8080/job/phabricator/1741/
See console output for more information: http://jw:8080/job/phabricator/1741/console

Harbormaster failed remote builds in B2599: Diff 2903!Jul 13 2017, 3:45 AM

Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/simulation/helpers/Player.js
|  81| »   »   let·cmpPlayer·=·QueryPlayerIDInterface(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'cmpPlayer' is already declared in the upper scope.

binaries/data/mods/public/simulation/helpers/Player.js
| 174| »   »   »   let·cmpPlayer·=·QueryPlayerIDInterface(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'cmpPlayer' is already declared in the upper scope.

binaries/data/mods/public/simulation/helpers/Player.js
|  68| »   »   var·civ·=·getSetting(playerData,·playerDefaults,·i,·"Civ");
|    | [NORMAL] JSHintBear:
|    | 'civ' is already defined.

binaries/data/mods/public/simulation/helpers/Player.js
|  69| »   »   var·template·=·cmpTemplateManager.TemplateExists("special/player_"+civ)·?·"special/player_"+civ·:·"special/player";
|    | [NORMAL] JSHintBear:
|    | 'template' is already defined.

binaries/data/mods/public/simulation/helpers/Player.js
|  70| »   »   var·entID·=·cmpPlayerManager.GetPlayerByID(i);
|    | [NORMAL] JSHintBear:
|    | 'entID' is already defined.

binaries/data/mods/public/simulation/helpers/Player.js
|  79| »   for·(var·i·=·0;·i·<·numPlayers;·++i)
|    | [NORMAL] JSHintBear:
|    | 'i' is already defined.

binaries/data/mods/public/simulation/helpers/Player.js
|  89| »   »   if·(i·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/simulation/helpers/Player.js
| 141| »   »   »   for·(var·j·=·0;·j·<·numPlayers;·++j)
|    | [NORMAL] JSHintBear:
|    | 'j' is already defined.

binaries/data/mods/public/simulation/helpers/Player.js
| 172| »   »   for·(var·i·=·0;·i·<·numPlayers;·++i)
|    | [NORMAL] JSHintBear:
|    | 'i' is already defined.
|    | [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
| 318| 318| /**
| 319| 319|  * Index of the GUI timer.
| 320| 320|  */
| 321|    |-var g_GameStanzaTimer = undefined;
|    | 321|+var g_GameStanzaTimer;
| 322| 322| 
| 323| 323| /**
| 324| 324|  * Only send a lobby update if something actually changed.
|    | [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
|1148|1148| 
|1149|1149| function verticallyDistributeGUIObjects(parent, objectHeight, ignore)
|1150|1150| {
|1151|    |-	let yPos = undefined;
|    |1151|+	let yPos;
|1152|1152| 
|1153|1153| 	let parentObject = Engine.GetGUIObjectByName(parent);
|1154|1154| 	for (let child of parentObject.children)

binaries/data/mods/public/gui/gamesetup/gamesetup.js
| 696| »   »   »   let·pData·=·playerData.find(pData·=>·sameColor(g_PlayerColorPickerList[selectedIdx],·pData.Color));
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'pData' is already declared in the upper scope.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1604| »   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
|2061| »   »   for·(let·guid·in·g_PlayerAssignments)
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'guid' is already declared in the upper scope.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|2061| »   »   for·(let·guid·in·g_PlayerAssignments)
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'guid' is already declared in the upper scope.

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

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

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

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

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1324| »   »   i!=0·&&·Object.keys(g_PlayerAssignments).every(guid·=>·g_PlayerAssignments[guid].player·!=·i)
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with '0'.

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

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

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

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1638| »   »   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
|1639| »   »   »   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
|1811| »   »   if·(i·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1819| »   »   »   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
|1833| »   »   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
|1866| »   »   »   if·(i·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

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

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

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

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

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

Every n'th place adding a -1 and every n'th +1st place removing a -1 will be fun to review.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
432	Arbitrary design decision from rP9336
487	Maybe the above default could be moved here by changing `g_OptionOrderInit`. (`reloadMapFilterList` only depends on the map type and should ideally remain independent of the number of players.)

Sandarac awarded a token.Jul 14 2017, 10:28 AM

• leper mentioned this in D989: template organization: treasures, ruins, and the plane (1/5).Oct 29 2017, 3:31 AM

It is a good patch. If the TODOs and debug code is removed and more -1 +1 exceptions removed, it can be committed.

The idea is to remove any gaia exceptions, assumptions where possible, minimizing the complexity (while incidentally gaining more control over gaia settings).
Adding gaia dropdown and returning true from`hidden` of g_PlayerDropdowns[dropdown] for gaia should remove any +1 and -1.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
1003	Compensating a -1 in one function with a +1 in the other function was indeed what we wanted to fight.
1494	`&&`. A `civ != "gaia"` check rather than a `i != 0` check would remove one more unneeded assumption in the code and reveal meaning. (I'm not taking any responsibility for this function, it should be deleted for the most part without losing the feature.)
1528	yay
1537	civ == "gaia"?

This revision now requires changes to proceed.Dec 19 2017, 5:37 PM

Feel free to take over, I guess here you're unlikely to break things you don't care to understand.

In D744#46379, @leper wrote:

Feel free to take over, I guess here you're unlikely to break things you don't care to understand.

Could also tell me that I'm incapable of understanding even if I was to care. ¯\_(ツ)_/¯
One way to kill a constructive conversation is to insult people instead of discussing the subject matter.
Given that this patch consists of mostly questions as to why the code is the way it is without having looked up where it came from, perhaps that accusation applies to the author.
Because you haven't specified the purpose neither in the Summary (which has the purpose of stating the purpose of a patch), nor the references D642 and P59, you leave it as either guesswork or as a task for the reviewer to do the actual feature design, and then complain if one didn't guess correctly what you might have had in mind.

In case it isn't clear, I'm writing the following to document the design decisions of the code for people looking it up, not to get a constructive or any discussion going since both of these facets are impossible now for the reason how you responded and not responded in cases like these and for the reason that you were treated like always being in the right until suddenly getting instabanned instead of getting the disagreed statements and conflicts addressed while remaining on topic, from the beginning when they arose, which is an equal failure that I still wait for to be rectified on a place that I call elsewhere too since some similar experience.

There are three use cases for maps specifying Gaia I could discover while working with the gamesetup and maps:
(1) obtain consistent maps, since some of them don't specify gaia while others specify gaia as null
(2) maps modifying Gaia player properties
(3) gamesetup controls modifying Gaia player properties

A realistic example for (2) and (3) may be StartingTechnologies, PlayerColor or new modifiable properties used by mods that only relate to Gaia.

Map consistency is actually cleaner done by deleting gaia in maps, because it means that one doesn't have to insert an i != 0 exception to about every place that deals with PlayerData in the gamesetup.
Maps and Gamesetup influencing gaia PlayerData properties while having all maps consistent can only be done by having all maps specify Gaia.
Whether an i != 0 exception would make sense in every place is something that should be deduced from a feature design, not from a list of implementation TODOs and the lack of a post describing why the patch exists to begin with.
For (2) and (3) and some selected player properties and relatable conversations, I can already fathom the answer "but it doesnt even make sense to allow players / map makers to modify that property of gaia" (notice the lack of deduction in there as well). In that case there is either no purpose in letting maps specify Gaia or the absent feature description fails to distinguish which properties are a use case to be modified by maps and gamesetup.

I just wanted to state that D2483 removes the misleading default playercount from D322 marked in one of the TODO questions in the diff, but I also won't let this accusation remain unrefuted nor will I participate in accusations without arguments, so there you go.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
432	Slice is performed to obtain the subset, 4 instead of all 8 players. I can only guess that 4 was deemed more appropriate in rP9336 than 8 players by default since its the median (1 too few, 8 too many). The code should only be performed for random maps, not for skirmish maps, since those come with their own PlayerData. This wrong behavior introduced amongst others in rP13938, apparently due to copy&paste from the random map case from rP9336 without examining the purpose of the code, fixed in D2483.