#686 closed enhancement (fixed)
[PATCH] Additional counters for summary screen
Reported by: | fcxSanya | Owned by: | Bajter |
---|---|---|---|
Priority: | Nice to Have | Milestone: | Alpha 16 |
Component: | UI & Simulation | Keywords: | patch |
Cc: | Patch: |
Description (last modified by )
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.
Attachments (6)
Change History (39)
comment:1 by , 13 years ago
Milestone: | Backlog → Alpha 4 |
---|
comment:2 by , 13 years ago
Priority: | minor → major |
---|
comment:3 by , 13 years ago
Milestone: | Alpha 4 → Alpha 5 |
---|
comment:4 by , 13 years ago
Milestone: | Alpha 5 → Alpha 6 |
---|
comment:5 by , 13 years ago
Description: | modified (diff) |
---|---|
Keywords: | simple added |
Milestone: | Alpha 6 → Backlog |
Priority: | Should Have → Nice to Have |
comment:6 by , 13 years ago
Summary: | Additional couters for summary screen → Additional counters for summary screen |
---|
comment:7 by , 13 years ago
Description: | modified (diff) |
---|
comment:8 by , 13 years ago
Keywords: | review added |
---|
comment:9 by , 13 years ago
Keywords: | patch added |
---|---|
Milestone: | Backlog → Alpha 7 |
Summary: | Additional counters for summary screen → [PATCH] Additional counters for summary screen |
follow-up: 12 comment:10 by , 13 years ago
Milestone: | Alpha 7 → Backlog |
---|
I'm currently working on a number of other enhancements to the summary screen, so this should be held-off on patching for now.
comment:11 by , 13 years ago
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
by , 13 years ago
Attachment: | summary_screen_improvements.patch added |
---|
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..
follow-up: 13 comment:12 by , 13 years ago
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 :-)
follow-up: 14 comment:13 by , 13 years ago
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
andCalculateFavouriteWarship
-- probably would be nice to reduce code duplication by having a single function instead of two, and pass in eitherthis.soldierTypeCount
orthis.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!
follow-up: 15 comment:14 by , 13 years ago
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!
comment:15 by , 13 years ago
Replying to Philip:
{}
is semantically equivalent tonew 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.
comment:16 by , 12 years ago
Keywords: | review removed |
---|
Patch was work in progress and now has too many conflicts with the trunk to be trivially applied, removing review keyword.
comment:17 by , 12 years ago
Keywords: | simple, patch → patch, simple |
---|
comment:18 by , 10 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:19 by , 10 years ago
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.
by , 10 years ago
Attachment: | summary_screen_counters.patch added |
---|
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
by , 10 years ago
Attachment: | summary_screen_counters_teams.patch added |
---|
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.
comment:20 by , 10 years ago
Keywords: | review added; simple removed |
---|---|
Milestone: | Backlog → Alpha 16 |
Patch is good enough for inclusion IMO.
comment:21 by , 10 years ago
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?
comment:22 by , 10 years ago
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.
by , 10 years ago
Attachment: | summary_screen_counters_teams_24XII.patch added |
---|
comment:23 by , 10 years ago
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.
comment:24 by , 10 years ago
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?
by , 10 years ago
Attachment: | summary_screen_better_lobby_reporting.diff added |
---|
comment:26 by , 10 years ago
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
by , 10 years ago
Attachment: | summary_screen_better_lobby_reporting.2.diff added |
---|
comment:27 by , 10 years ago
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
comment:28 by , 10 years ago
I have some spare time today, you can just go ahead and commit it; I'll fix any breakages.
comment:30 by , 10 years ago
Keywords: | review removed |
---|
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.