Page MenuHomeWildfire Games

Refactor session lobby bot client code to use object orientation and remove duplication and hardcoding
ClosedPublic

Authored by elexis on Oct 20 2019, 7:41 PM.

Details

Summary

In the course of #5387, this patch transforms the session.js reportGame and sendLobbyPlayerlistUpdate function to use class syntax.
Additionally it moves the lobby code including globals to a separate session/lobby/ folder that can be deleted without further modification if someone wants to distribute 0ad without lobby code (refs D2284).

Unfortunately many bugs have been discovered during the implementation of this that can't be all addressed here.
This is mostly caused by duplicating code and then only one of the copies getting updates.

  • rP12914 introduced score counts, where the total score = economic score + military score + exploration score rP14098 introduced the lobby including rating code, added a field for totalScore in the database but didn't receive the value for that from session.js.
  • rP14752 added the totalScore sending, but it copy&pasted the function from the summary screen, so that future updates to the summary screen code would not get into this score value. The duplication cannot be removed here without introducing two random score functions to gui/common/ and I'd rather add comments that will be ignored by developers and broken again than introducing again incohesive code to that folder. rP14752 also missed to add the exploration score present in rP12914.
  • rP14098 added trailing commas to all reported values and added python code to remove them again. All of that for no apparent reason. This is not going to be fixed in this diff to keep this a GUI only diff.
  • rP14703 introduced counting of specific unit and building classes in the summary screen and StatisticsTracker. It modified the session.js reportGame function to hardcode unit and class names in both the StatisticsTracker AND session.js. This is not obvious at all that these have to be duplicates, it more appears like a selection of specific classes chosen in the lobby rating client code, but it's not. D2384 removes the duplication for the StatisticsTracker, this diff removes the duplication for the GUI.
  • rP16550 implemented capturing, rP18395 introduced capturing statistics. Due to the duplication effect, it added the buildingsCaptured property for the StatisticsTracker, but not for the reportGame function, so the same problem still exists here. This is not fixed in this patch as the bot needs to be updated for that, and with that the ratings db table would have to be migrated too.
  • rP16624 introduced ceasefire, added it to reportGame, making it being sent, but the bot never reads from it. Secondly it converted the time number to string while the timeElapsed is sent as a number. I suppose someone looking at the ratings database would rather be interested in the ceasefire time that was set in the matchsettings, rather than the remaining ceasefire time (as only resign and gaia can defeat someone during that time). So this is removed in this patch. It had been added apparently for consistency, and this makes one wonder whether one shouldnt just send the playerstate instead of copying and transforming every property individually... But I want to be conservative with this patch and finish the OOP rewrite before going down too many rabbitholes (which I already do as you see).
  • rP14703 broke the economic score by counting vegetarianFood twice. rP14752 copied the mistake to the rating reporter code. This was first noticed by causative in http://irclogs.wildfiregames.com/2016-06/2016-06-07-QuakeNet-%230ad-dev.log Notice that this double-counting of vegetarian food has been claimed to be a feature by both causative and later by temple too, because for all gathered resources one has 0 investment costs except for corrals where they are 50%, so the score ended up in showing the corral player to be twice as many resources available which was not the case due to the investment. rP19584 fixed the doublecounting for the statistics code but not for the rating reporter copy. It also added trade to the statistics economy score, but not to the rating reporter code. rP20543 changed the mechanism to reduce the number of resources gathered when training animals, but again not for the rating code. If only counting "net resources" as introduced by that commit is reasonable, then this should be renamed to such, both shown in the summary screen page and in the variable names. Because everyone looking at "gathered resources" will think it's amount of resources gathered. But one can actually get negative gathered resources and economy score if one trains sheep without gathering them. This behavior is not changed in this patch because unrelated, but that doesn't stop this issue from slowing down development of this patch. Ahhhh!
Test Plan

Display the players simulation state (F9, warn(uneval(Engine.GuiInterfaceCall("GetExtendedSimulationState"))) before or after the patch.
Display the report object using warn(uneval(..)) before the rating call.
Set Available to return true to test in singleplayer mode.
Compare that all values are the same as before, just in a different order, and notice that the order doesn't matter.

Event Timeline

elexis created this revision.Oct 20 2019, 7:41 PM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/993/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/478/display/redirect

elexis updated this revision to Diff 10188.Oct 20 2019, 7:48 PM

Remove ceasefire and thus Time.js and extendedSimState argument for InsertValues.

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/994/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/479/display/redirect

elexis updated the Trac tickets for this revision.Oct 21 2019, 4:28 PM
Stan added a subscriber: Stan.Oct 21 2019, 4:41 PM
Stan added inline comments.
binaries/data/mods/public/gui/session/lobby/LobbyGamelistReporter.js
56

.

binaries/data/mods/public/gui/session/lobby/LobbyRatingReport/Score.js
23

.

binaries/data/mods/public/gui/session/lobby/LobbyRatingReporter.js
28

.

34

.

46

Oversight, commented code.

binaries/data/mods/public/gui/session/lobby/LobbyService.js
9

GameList ?

binaries/data/mods/public/gui/session/messages.js
577

Typo.

binaries/data/mods/public/gui/summary/counters.js
130

.

Verification of the patch:

The rating report object without the patch:

gitprivate
{
	timeElapsed: 25000,
	playerStates: "defeated,won,",
	playerID: 1,
	matchID: "B52D904E2AD657E8",
	civs: "spart,cart,",
	teams: "-1,-1,",
	teamsLocked: "false",
	ceasefireActive: "false",
	ceasefireTimeRemaining: "0",
	mapName: "Acropolis Bay (2)",
	economyScore: "340,0,",
	militaryScore: "0,0,",
	totalScore: "340,0,",
	woodGathered: "240,0,",
	foodGathered: "100,0,",
	stoneGathered: "0,0,",
	metalGathered: "0,0,",
	woodUsed: "350,0,",
	foodUsed: "300,0,",
	stoneUsed: "0,0,",
	metalUsed: "0,0,",
	woodSold: "0,0,",
	foodSold: "0,0,",
	stoneSold: "0,0,",
	metalSold: "0,0,",
	woodBought: "0,0,",
	foodBought: "0,0,",
	stoneBought: "0,0,",
	metalBought: "0,0,",
	vegetarianFoodGathered: "0,0,",
	totalUnitsTrained: "5,0,",
	totalUnitsLost: "0,0,",
	enemytotalUnitsKilled: "0,0,",
	infantryUnitsTrained: "3,0,",
	infantryUnitsLost: "0,0,",
	enemyInfantryUnitsKilled: "0,0,",
	workerUnitsTrained: "4,0,",
	workerUnitsLost: "0,0,",
	enemyWorkerUnitsKilled: "0,0,",
	femaleCitizenUnitsTrained: "1,0,",
	femaleCitizenUnitsLost: "0,0,",
	enemyFemaleCitizenUnitsKilled: "0,0,",
	cavalryUnitsTrained: "1,0,",
	cavalryUnitsLost: "0,0,",
	enemyCavalryUnitsKilled: "0,0,",
	championUnitsTrained: "0,0,",
	championUnitsLost: "0,0,",
	enemyChampionUnitsKilled: "0,0,",
	heroUnitsTrained: "0,0,",
	heroUnitsLost: "0,0,",
	enemyHeroUnitsKilled: "0,0,",
	siegeUnitsTrained: "0,0,",
	siegeUnitsLost: "0,0,",
	enemySiegeUnitsKilled: "0,0,",
	shipUnitsTrained: "0,0,",
	shipUnitsLost: "0,0,",
	enemyShipUnitsKilled: "0,0,",
	traderUnitsTrained: "0,0,",
	traderUnitsLost: "0,0,",
	enemyTraderUnitsKilled: "0,0,",
	totalBuildingsConstructed: "1,0,",
	totalBuildingsLost: "0,0,",
	enemytotalBuildingsDestroyed: "0,0,",
	civCentreBuildingsConstructed: "0,0,",
	civCentreBuildingsLost: "0,0,",
	enemyCivCentreBuildingsDestroyed: "0,0,",
	houseBuildingsConstructed: "1,0,",
	houseBuildingsLost: "0,0,",
	enemyHouseBuildingsDestroyed: "0,0,",
	economicBuildingsConstructed: "0,0,",
	economicBuildingsLost: "0,0,",
	enemyEconomicBuildingsDestroyed: "0,0,",
	outpostBuildingsConstructed: "0,0,",
	outpostBuildingsLost: "0,0,",
	enemyOutpostBuildingsDestroyed: "0,0,",
	militaryBuildingsConstructed: "0,0,",
	militaryBuildingsLost: "0,0,",
	enemyMilitaryBuildingsDestroyed: "0,0,",
	fortressBuildingsConstructed: "0,0,",
	fortressBuildingsLost: "0,0,",
	enemyFortressBuildingsDestroyed: "0,0,",
	wonderBuildingsConstructed: "0,0,",
	wonderBuildingsLost: "0,0,",
	enemyWonderBuildingsDestroyed: "0,0,",
	tradeIncome: "0,0,",
	tributesSent: "0,0,",
	tributesReceived: "0,0,",
	treasuresCollected: "0,0,",
	lootCollected: "0,0,",
	percentMapExplored: "6,5,"
}

The rating report object after the patch:

{
	playerID: -1,
	matchID: "6CC2259D6D62B420",
	mapName: "Acropolis Bay (2)",
	timeElapsed: 32200,
	totalBuildingsConstructed: "2,0,",
	houseBuildingsConstructed: "1,0,",
	economicBuildingsConstructed: "0,0,",
	outpostBuildingsConstructed: "0,0,",
	militaryBuildingsConstructed: "1,0,",
	fortressBuildingsConstructed: "1,0,",
	civCentreBuildingsConstructed: "0,0,",
	wonderBuildingsConstructed: "0,0,",
	totalBuildingsLost: "0,0,",
	houseBuildingsLost: "0,0,",
	economicBuildingsLost: "0,0,",
	outpostBuildingsLost: "0,0,",
	militaryBuildingsLost: "0,0,",
	fortressBuildingsLost: "0,0,",
	civCentreBuildingsLost: "0,0,",
	wonderBuildingsLost: "0,0,",
	enemytotalBuildingsDestroyed: "0,0,",
	enemyHouseBuildingsDestroyed: "0,0,",
	enemyEconomicBuildingsDestroyed: "0,0,",
	enemyOutpostBuildingsDestroyed: "0,0,",
	enemyMilitaryBuildingsDestroyed: "0,0,",
	enemyFortressBuildingsDestroyed: "0,0,",
	enemyCivCentreBuildingsDestroyed: "0,0,",
	enemyWonderBuildingsDestroyed: "0,0,",
	tradeIncome: "0,0,",
	tributesSent: "0,0,",
	tributesReceived: "0,0,",
	treasuresCollected: "0,0,",
	lootCollected: "0,0,",
	percentMapExplored: "6,5,",
	playerStates: "defeated,won,",
	civs: "spart,athen,",
	teams: "-1,-1,",
	teamsLocked: "false",
	foodGathered: "0,0,",
	woodGathered: "540,0,",
	stoneGathered: "0,0,",
	metalGathered: "0,0,",
	foodUsed: "2500,0,",
	woodUsed: "1650,0,",
	stoneUsed: "2500,0,",
	metalUsed: "1500,0,",
	foodSold: "0,0,",
	woodSold: "0,0,",
	stoneSold: "0,0,",
	metalSold: "0,0,",
	foodBought: "0,0,",
	woodBought: "0,0,",
	stoneBought: "0,0,",
	metalBought: "0,0,",
	vegetarianFoodGathered: "0,0,",
	economyScore: "54,0,",
	militaryScore: "0,0,",
	totalScore: "114,50,",
	totalUnitsTrained: "20,0,",
	infantryUnitsTrained: "0,0,",
	workerUnitsTrained: "20,0,",
	femaleCitizenUnitsTrained: "20,0,",
	cavalryUnitsTrained: "0,0,",
	championUnitsTrained: "0,0,",
	heroUnitsTrained: "0,0,",
	siegeUnitsTrained: "0,0,",
	shipUnitsTrained: "0,0,",
	domesticUnitsTrained: "0,0,",
	traderUnitsTrained: "0,0,",
	totalUnitsLost: "0,0,",
	infantryUnitsLost: "0,0,",
	workerUnitsLost: "0,0,",
	femaleCitizenUnitsLost: "0,0,",
	cavalryUnitsLost: "0,0,",
	championUnitsLost: "0,0,",
	heroUnitsLost: "0,0,",
	siegeUnitsLost: "0,0,",
	shipUnitsLost: "0,0,",
	traderUnitsLost: "0,0,",
	enemytotalUnitsKilled: "0,0,",
	enemyInfantryUnitsKilled: "0,0,",
	enemyWorkerUnitsKilled: "0,0,",
	enemyFemaleCitizenUnitsKilled: "0,0,",
	enemyCavalryUnitsKilled: "0,0,",
	enemyChampionUnitsKilled: "0,0,",
	enemyHeroUnitsKilled: "0,0,",
	enemySiegeUnitsKilled: "0,0,",
	enemyShipUnitsKilled: "0,0,",
	enemyTraderUnitsKilled: "0,0,"
}

After alphebtic sort, we notice the diff:

--- /tmp/before.txt	2019-10-21 14:57:56.005856158 +0200
+++ /tmp/after.txt	2019-10-21 14:58:02.349189443 +0200
@@ -3,3 +3 @@
-cavalryUnitsTrained: "1,0,",
-ceasefireActive: "false",
-ceasefireTimeRemaining: "0",
+cavalryUnitsTrained: "0,0,",
@@ -10 +8,2 @@
-civs: "spart,cart,",
+civs: "spart,athen,",
+domesticUnitsTrained: "0,0,",
@@ -13 +12 @@
-economyScore: "340,0,",
+economyScore: "54,0,",
@@ -29 +28 @@
-enemyTraderUnitsKilled: "0,0,",
+enemyTraderUnitsKilled: "0,0,"
@@ -33 +32 @@
-femaleCitizenUnitsTrained: "1,0,",
+femaleCitizenUnitsTrained: "20,0,",
@@ -35 +34 @@
-foodGathered: "100,0,",
+foodGathered: "0,0,",
@@ -37,2 +36,2 @@
-foodUsed: "300,0,",
-fortressBuildingsConstructed: "0,0,",
+foodUsed: "2500,0,",
+fortressBuildingsConstructed: "1,0,",
@@ -45 +44 @@
-infantryUnitsTrained: "3,0,",
+infantryUnitsTrained: "0,0,",
@@ -48 +47 @@
-matchID: "B52D904E2AD657E8",
+matchID: "6CC2259D6D62B420",
@@ -52,2 +51,2 @@
-metalUsed: "0,0,",
-militaryBuildingsConstructed: "0,0,",
+metalUsed: "1500,0,",
+militaryBuildingsConstructed: "1,0,",
@@ -58,2 +57,2 @@
-percentMapExplored: "6,5,"
-playerID: 1,
+percentMapExplored: "6,5,",
+playerID: -1,
@@ -68 +67 @@
-stoneUsed: "0,0,",
+stoneUsed: "2500,0,",
@@ -71,2 +70,2 @@
-timeElapsed: 25000,
-totalBuildingsConstructed: "1,0,",
+timeElapsed: 32200,
+totalBuildingsConstructed: "2,0,",
@@ -74 +73 @@
-totalScore: "340,0,",
+totalScore: "114,50,",
@@ -76 +75 @@
-totalUnitsTrained: "5,0,",
+totalUnitsTrained: "20,0,",
@@ -87 +86 @@
-woodGathered: "240,0,",
+woodGathered: "540,0,",
@@ -89 +88 @@
-woodUsed: "350,0,",
+woodUsed: "1650,0,",
@@ -91 +90 @@
-workerUnitsTrained: "4,0,",}
+workerUnitsTrained: "20,0,",}

Notice that this is the case due to two different games being played.

I observer that the new code has domesticUnitsTrained which the previous one didn't have.

The "before" snapshot was actually done with the git mirror using r23084 and the after one with rP23086.

I would say it doesnt matter if the excess data is sent (see earlier excess ceasefire properties), and doing so means not adding yet another "ignore domestic units" check for the Units.js code.

Notice the trailing commas are part only of the strings that represent arrays, and of all arrays.
Notice that this encoding actually implies a bug if a mapname or other property that is a string contains a comma (bug in the bot code).

Updated the lobby server OS, found mistakes in D1661, answered a personal data request, started the a24 bot and resigned two games with the elexis2 and elexis3 brothers to confirm the patch experimentally.

binaries/data/mods/public/gui/session/lobby/LobbyRatingReport/Buildings.js
22

This would be a 2D loop instead of 3 1D loops if the enemy thing wouldn't have a different order.

binaries/data/mods/public/gui/session/lobby/LobbyRatingReport/Score.js
38

capturing statistics can be added here, albeit not being added to the buildings and units statistics until modifying the botcode.

45

exploration score missing

binaries/data/mods/public/gui/session/lobby/LobbyRatingReporter.js
2

The wording is wrong, because there is the extends keyword for inheritance in JS and this doesnt use inheritance.

binaries/data/mods/public/gui/session/session.js
157

[] -> Set change missed in rP23081

1228

What was really unfortunate for this function is the fragmentation.

For example if you want to work with the unit statistics, you have some arrays at the top, then logic about everything except units, then again a loop about units, then everything except units again, then units again etc.

With the revised code there is one file containing one class about units, and everything else in that folder can be ignored by the reader / author

1265

The hardcoding of these classnames was removed in the StatisticsTracker / moved to templates in rP23086 / D2384,
and this hardcoding and copy is removed by iterating over the keys of whatever StatisticsTracker hands over.

This shoudl be correct, because sending more or less data didn't break the bot judging by the commits mentioned in the summary.

binaries/data/mods/public/gui/session/session.xml
12

I know, I know, capitals inconsistent, bad, very bad, but if I was to not intend to make everything consistent in the GUI in the long run I wasnt writing these diffs. The classnames are capitalized, the filenames meet the classnames, the foldernames meet the filenames. So those subfolders will become capitalized too.

Notice that wiki:Coding_Conventions says:

Primarily, try to match the style of the functions that you're editing (assuming it's at least self-consistent and not too bizarre), in order to avoid making it less self-consistent.
Secondly, try to match the style of the files that you're editing.
Then, try to match the style of the other code in the subdirectory you're editing.

and that clearly has "Secondly" coming before "Then", so in fact every item the session/lobby subfolder is capitalized, hence this must be the correct way to implement it, especially when considering the planned run.

binaries/data/mods/public/gui/summary/counters.js
125

I will add a copy of this comment to all four functions affected, one copy for every instance where someone broke it because of updating only one of the duplicate functions.

Using gui/common/score/Score.js would also be a way to only include it in two GUi pages, but I dislike that too.

Code that only makes sense in two GUI pages shouldn't be in a folder called "common".

If it was just in the simulation, it would be unified already. But we dont want it in the simulation, because we want to avoid the performance and memory cost; secondly there is also Single Point of Truth pattern enforcing that data cant become inconsistent (the score not fitting the player statistics). So meh + not now.

We also can't go through the GUIinterface because the score computation also works when the simulation doesnt exist (replay menu).
(globalscripts even a worse choice)

126

Adding comment because calculateEconomyScore was broken by

138

Adding a comment to calculateMilitaryScore because it was broken by capturing rP18395 (refs rP16550)

This revision was not accepted when it landed; it landed in state Needs Review.Oct 21 2019, 5:01 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Owners added a subscriber: Restricted Owners Package.Oct 21 2019, 5:01 PM