[PATCH] Additional counters for summary screen

[fcxSanya]

It would be good to have more counters on summary screen, here is some ideas from Mythos_Ruler's forum post (http://www.wildfiregames.com/forum/index.php?showtopic=13683&view=findpost&p=213723):

• Total Resources
• Deforestation
• Kill/Death Ratio
• Temples
• Special Buildings
• Warships Destroyed/Warships Lost
• Number of Super Units
• Heroes Killed
• Favorite Military Unit
• Most Bloodthirsty Hero
• Avg Lifespan

For implementation details see ticket #638 and corresponding changeset r8576, which contains already implemented counters for summary screen. Most related files are:

• binaries/data/mods/public/simulation/components/StatisticsTracker.js - simulation component, which keep track of statistic counters;
• binaries/data/mods/public/gui/summary/summary.xml - summary page layout;
• binaries/data/mods/public/gui/summary/summary.js - scripts for summary.xml.

[Ross Bearman]

The above attachment adds in a field for total resources gathered, as well as one for the favourite unit (most commonly trained.)

The favourite unit mechanism stores the number of times each unit has been trained in an associative array and displays the unit with the highest value when the summary screen is displayed.

[Ross Bearman]

I'm currently working on a number of other enhancements to the summary screen, so this should be held-off on patching for now.

[Ross Bearman]

I've updated the patch introducing the following changes:

• Unit count split into army (infantry and cavalry), navy (warships) and workers (female citizens and citizen soldiers)
• Kill/death ratios for army and navy
• Favourite units for army and navy
• Temples and special buildings count
• Workers trained counts
• Total for gathered resources
• Loot counts for all resources

[Ross Bearman]

May be identical to the last patch, but I noticed one of the files was unsaved, so just to be on the safe side I've recreated the patch..

[Philip Taylor]

Replying to Ross Bearman:

I'm currently working on a number of other enhancements to the summary screen, so this should be held-off on patching for now.

Should this still be held off on?

A few trivial comments:

CalculateFavouriteSoldier and CalculateFavouriteWarship -- probably would be nice to reduce code duplication by having a single function instead of two, and pass in either this.soldierTypeCount or this.warshipTypeCount as an argument.

CalculateKillDeathRatio -- assuming the goal is to return a value with 3 decimal places, it may be better to use "(kills/deaths).toFixed(3)" (so it'll consistently return "1.234", "1.000" (not "1") etc).

Nitpicking on formatting:

this.soldierTypeCount = new Object();

The usual convention (well, my convention at least) is to do

this.soldierTypeCount = {};

instead.

if(cmpUnitIdentity) (plus similar cases) -- should have space before ( for consistency with the rest of the code.

this.soldierTypeCount[name]++ (plus similar cases) -- should have ; at end of statement.

I can't help feeling that summary.js could do with some functions or loops or something, rather than big chunks of repeated layout code. But that's not a new problem and probably doesn't need to be fixed here if it's a pain :-)

[Ross Bearman]

Replying to Philip:

Replying to Ross Bearman:

I'm currently working on a number of other enhancements to the summary screen, so this should be held-off on patching for now.

Should this still be held off on?

The enhancements are mostly refactoring the code to lessen duplication and improve readability/maintainability, so it's probably not worth merging it at this point, as long as the XML changes aren't going to be an issue (which seems unlikely). I posted the patch mainly to check that I wasn't going off on a completely wrong direction or missing anything crucial.

A few trivial comments:

CalculateFavouriteSoldier and CalculateFavouriteWarship -- probably would be nice to reduce code duplication by having a single function instead of two, and pass in either this.soldierTypeCount or this.warshipTypeCount as an argument.

I'd overlooked this in my refactoring, will fix that in the updated patch.

CalculateKillDeathRatio -- assuming the goal is to return a value with 3 decimal places, it may be better to use "(kills/deaths).toFixed(3)" (so it'll consistently return "1.234", "1.000" (not "1") etc).

This seems like a good idea. I've not often had to do this in JavaScript and I had some recollection of issues with toFixed(), probably some old browser issues.

Nitpicking on formatting:

this.soldierTypeCount = new Object();

The usual convention (well, my convention at least) is to do

this.soldierTypeCount = {};

instead.

As the type counts are associative arrays (which use properties), none of the array functionality is used. I imagine it makes very little practical difference, but many JavaScript conventions recommend this way. ​Google Style Guide for example.

if(cmpUnitIdentity) (plus similar cases) -- should have space before ( for consistency with the rest of the code.

this.soldierTypeCount[name]++ (plus similar cases) -- should have ; at end of statement.

Noted; I've noticed different styles throughout the JS files, so I wasn't sure what was best to use and there doesn't seem to be a coding conventions article for JavaScript on the wiki. Missing semi-colons are the bane of my life, I should probably set something up to complain at me if they're missing, if only for stylistic reasons.

I can't help feeling that summary.js could do with some functions or loops or something, rather than big chunks of repeated layout code. But that's not a new problem and probably doesn't need to be fixed here if it's a pain :-)

As explained above, I'm currently slowly refactoring this to reduce the amount of duplication, just taking the time to make sure changes I make don't cause any new problems.

Thanks for taking the time to review!

[Philip Taylor]

Replying to Ross Bearman:

This seems like a good idea. I've not often had to do this in JavaScript and I had some recollection of issues with toFixed(), probably some old browser issues.

Hmm, ​sounds like it has problems in IE <= 8, but our JS engine is from Firefox 4.0 so we fortunately don't have to care about compatibility :-)

The usual convention (well, my convention at least) is to do this.soldierTypeCount = {}; instead.

As the type counts are associative arrays (which use properties), none of the array functionality is used.

{} is semantically equivalent to new Object(). Arrays would be [] instead, which I wasn't suggesting.

(...well, not exactly equivalent, since somebody could redefine the global Object to be something different. But equivalent apart from that.)

(That style guide is interesting - don't think I've seen it before. Fortunately it ​suggests what I suggested for object literals too :-) )

Noted; I've noticed different styles throughout the JS files, so I wasn't sure what was best to use and there doesn't seem to be a coding conventions article for JavaScript on the wiki.

Yeah, we really ought to document some standard, it's a bit ad-hoc at the moment. I think we're mostly consistent in including semicolons after a ​Statement (except after a "Foo.prototype.Bar = function() { ... };" which gets frequently forgotten), so anything else should probably be considered a (trivial) style bug.

Missing semi-colons are the bane of my life, I should probably set something up to complain at me if they're missing, if only for stylistic reasons.

Might be nice if there's some tool to encourage consistency - otherwise I can always complain at people ;-)

As explained above, I'm currently slowly refactoring this to reduce the amount of duplication, just taking the time to make sure changes I make don't cause any new problems.

Okay, sounds great!

[Ross Bearman]

Replying to Philip:

{} is semantically equivalent to new Object(). Arrays would be [] instead, which I wasn't suggesting.

Ugh, shouldn't have written this reply at one in the morning; as {} is used for array declaration in many languages my brain short circuited that. Thanks for pointing it out.

Noted; I've noticed different styles throughout the JS files, so I wasn't sure what was best to use and there doesn't seem to be a coding conventions article for JavaScript on the wiki.

Yeah, we really ought to document some standard, it's a bit ad-hoc at the moment. I think we're mostly consistent in including semicolons after a ​Statement (except after a "Foo.prototype.Bar = function() { ... };" which gets frequently forgotten), so anything else should probably be considered a (trivial) style bug.

Aye, I thought I'd cleared up all the missing semi-colons, but apparently I wasn't that thorough.

Missing semi-colons are the bane of my life, I should probably set something up to complain at me if they're missing, if only for stylistic reasons.

Might be nice if there's some tool to encourage consistency - otherwise I can always complain at people ;-)

With Git I have pre-commit hooks set up to sanity check files, for example running JS files through ​Lint, syntax check PHP, etc. Not sure how possible something like this is with SVN, haven't really used it in anger before.

[historic_bruno]

Patch was work in progress and now has too many conflicts with the trunk to be trivially applied, removing review keyword.

[sanderd17]

As you don't use the formatting functions in other methods, I'd define those functions inside the method. And save some global variables that way. With locally defined functions, you can also use local variables, so it would save another function argument too.

In JS, the [] and the . can both be used in many cases, but we prefer the . notation for fixed names, and the [] notation for names dependent on a variable. So there are some you might want to change.

The comment on the function you made to increase the counters can also be better.

[Bajter]

This patch applies changes to layout and adds counters to the summary screen. The changes are further described on the forums here: http://www.wildfiregames.com/forum/index.php?showtopic=17914#entry280413

[Bajter]

Patch contains changes that were described on IRC and here: http://www.wildfiregames.com/forum/index.php?showtopic=17914#entry280413, it also contains changes that make this ticket: ​​http://trac.wildfiregames.com/ticket/2268 not needed anymore.

[sanderd17]

Patch is good enough for inclusion IMO.

[leper]

In session.js you have 3 loops over unitsClasses and 3 over buildingsClasses, make two loops out of all of those. We also have spaces after most comments (would be nice to keep that in line with the rest of the codebase)

Some of the captionSomething() functions have another function of the same name in them that could be replaced by a single assignment in the outer function. A comment for the infinity symbol for the kill ratio would be nice.

StatisticsTracker.prototype.CounterIncrement could use some spaces for the comment.

Did you test games with 4 teams and 3 or 4 players without a team? How does that look?

[sanderd17]

One thing I forgot to test, and it certainly causes problems when playing agains someone who doesn't have the patch applied, is if the stats are send correctly to the lobby bot.

Is someone willing to apply the patch and test with me? Or test with someone else and report if it works or not.

[Grzes Kabza]

I've tested it with Bajter and fixed some errors in session.js but we still have some errors.

Our test configuration: Me: Clean 0AD (without patch) - hosting the game through lobby Bajter: 0AD with patch - joining my game.

Warnings:

​https://dl.dropboxusercontent.com/u/51423444/warning.jpg

Errors:

​https://dl.dropboxusercontent.com/u/51423444/error.jpg

Despite the error we were able to play and counters looks good. I didn't have teams displayed, Bajter had.

[sanderd17]

Only the result of when you have two people playing with the patch applied counts. As people are expected to be on the same revision. Did you test that case?

[Grzes Kabza]

Yes. We've tested that case. Only mentioned warning shows up.

[sanderd17]

Cleaned up the object reported to the lobby, so the lobby bot can be adapted. This is the object returned now: ​http://pastebin.com/QSjzzTWU

[sanderd17]

Updated the patch to the current SVN + added some more keys to the lobby report on scythetwirler's request.

These is the current returned object: ​http://pastebin.com/hsJvmj6m

[Josh]

I have some spare time today, you can just go ahead and commit it; I'll fix any breakages.

[sanderd17]

In 14703:

Better summary screen. Fixes #686. Patch by Bajter and Kabzerek

[scythetwirler]

In 14752:

Reformat the database to accept the additional summary counters as well as adding a couple summary counters. Also deletes unnecessary welcome message and deletes whitespace. Refs #686.

[elexis]

In 23086:

Remove hardcoding and quadruplication of the StatisticsTracker unit and building classes following rP14703, refs #686.

Amongst other issues encountered in the lobby ranking session statistics reporting, refs #5387, D2385, rP14098.
Correct test case from rP21250/D1305.

Differential Revision: ​https://code.wildfiregames.com/D2384
Comments By: Freagarach

[elexis]

In 23087:

Refactor session lobby bot client code to use object orientation, refs #5387.

Adapt rating score for exploration score and add comment to keep calculateExplorationScore and calculateScoreTotal in sync following rP14752 (following the formula in rP12914 and rP14098 being otherwise in sync with the summary screen measures), refs #686.
Adapt rating score for captured entities and add comment to keep calculateMilitaryScore in sync following rP18395, refs #3216, rP16550.
Adapt rating score for trade and vegetarian food and add comment to keep calculateEconomyScore in sync following rP19584/D494 and rP20543/D1052, refs #3948.
Remove ceasefire sending from the client from rP16624 since the bot doesn't record it and since ceasefire time is expected to be 0 after a 1v1 was decided, refs #2749.
Resolve column value computation fragmentation and remove unneeded intermediary object encoding following rP14703, refs #686.
Remove hardcoding/duplication of unit and structure classes and send domesticUnitsTrained too, refs rP23086/D2384.
Move session code including the two globals to a separate folder so as to maximize separation of and ease distribution without lobby code.
Migrate useless ugly trailing commas from rP14098, to be removed in a patch modifying the bot code.

Differential Revision: ​https://code.wildfiregames.com/D2385