Summary screen

[fcxSanya]

According to the GUI_-_Achievements the summary screen should contain following statistics:

(citation start)

5.2.2.1. Military

• 1. Units trained, 2. enemy units killed, and 3. player units killed.
• 4. Buildings constructed, 5) enemy buildings destroyed, and 6) player buildings destroyed.
• 7. Number of Civ Centres built (provinces claimed).
• 8. Number of enemy Civ Centres destroyed.

5.2.2.2. Economy

• Resources Gathered: 1. Food, 2. Wood, 3. Stone, 4. Ore.
• 5. Tribute paid.
• 6. Tribute received.
• 7. Resources traded.
• 8. Resources spent.

5.2.2.3. Research

• 1. Number of Techs researched.
• 2. % of available techs researched.
• 3. Time advanced to Town.
• 4. Time advanced to City.

5.2.2.4. Score

(citation end)

[fcxSanya]

In the attached patch implemented totals for:

• time elapsed
• units trained
• units lost
• buildings constructed
• buildings lost
• civ centres build
• resources gathered

[fcxSanya]

Also there is the forum topic about summary page UI design:

http://www.wildfiregames.com/forum/index.php?showtopic=13683

[fcxSanya]

I remove "review" keyword for now, because I want to create a new version of patch with updated UI design and fixed statistics.

[fcxSanya]

Updated patch contains new UI (like in screenshots in the forum topic) and addition totals for "enemy units killed", "enemy buildings destroyed", "enemy civ centres destroyed" and "vegetarian ratio" :)

[Philip Taylor]

Looks good! A few suggestions:

I think it makes more sense for the Looter component to belong to units (i.e. like most standard components), rather than being a system component. That would let us control which units support looting (by adding <Looter> only in the appropriate unit templates), and let us use different parameters for different units (maybe some are 50% more effective at looting, maybe some only get XP and don't loot any resources, etc).

The statistics-tracking code in Player.js seems like a self-contained concept, and mostly independent of the rest of the code there. I like components to be small and self-contained (since it makes them easier to read and understand - that was the original goal of splitting everything up into components), so I'd suggest creating a new component ("StatisticsTracker" perhaps?) and adding it to templates/special/players.xml (so all player entities have that component alongside Player), and moving the statistics code into there, if that's not going to be a problem.

---

function /* string */ timeToString(/* int */ time /* in milliseconds */)
{

That kind of annotation is not common in our code, or in JS in general, and I think it looks a bit weird. It may be cleaner and more conventional to use ​Doxygen-style comments when you want to describe types, like:

/**
 * @param time Time period in milliseconds (integer)
 * @return String representing time period
 */
function timeToString(time)

or something similar to that.

---

var colourString = "colour: " + playerState.colour.r * 255 + " " + playerState.colour.g * 255 + " " + playerState.colour.b * 255;

I think the GUI expects colour components to be integers, so this should probably be Math.floor(playerState.colour.r * 255) etc.

---

var cmpKillerOwnership = Engine.QueryInterface(killerEntity, IID_Ownership); 
var cmpPlayerManager = Engine.QueryInterface(SYSTEM_ENTITY, IID_PlayerManager); 
var killerPlayerEntityId = cmpPlayerManager.GetPlayerByID(cmpKillerOwnership.GetOwner()); 
var cmpKillerPlayer = Engine.QueryInterface(killerPlayerEntityId, IID_Player); 

Instead of this, use

var cmpKillerPlayer = QueryOwnerInterface(killerEntity, IID_Player);

(defined in simulation/helpers/Player.js) which should be identical but shorter. (There's the same pattern in TrainingQueue.js.)

The second group of changes in test_GuiInterface.js look wrong - it should be a comma-separated list of properties, not a semicolon-separated list of statements.

[fcxSanya]

Updated version of patch, should include all changes, what Philip suggest.

[Philip Taylor]

Attack.js:

/** 
 * Ñalled when some units kills something (another unit, building, animal etc) 

Should be "Called".

summary.xml: "Civ centres build" should be "Civ centres built", for correct grammar. (Also in StatisticsTracker.js, civCentresBuild should be civCentresBuilt).

I think TargetKilled should do an "if (cmpKillerPlayerStatisticsTracker)" etc test before using those components. It seems very unlikely than a unit won't have a valid owner with a statistics tracker, but I'm not certain it's impossible. So QueryOwnerInterface might perhaps return null, and it's better to test for that in order to avoid a null reference exception. (The general principle is that components should make as few assumptions about any other components as possible - that should make them more robust to unexpected edge cases, particularly when the other components are later modified (by us or by modders).)

Looter.js shouldn't have <a:component type='system'/> added, since it's not a system component in this version of the patch.

There's an error when running the tests ("IID_Timer is not defined").

Otherwise it looks great :-) . Are you able to commit to SVN yourself, or should I do that?

[fcxSanya]

Thanks for review.

Are you able to commit to SVN yourself, or should I do that?

Yes, I can commit it by myself. I plan to fix and commit it tonight (after 17:00 UTC), I will notify you in IRC.

[fcxSanya]

(In [8576]) Summary screen. Closes #638.