[PATCH] time-dependent statistics

[mackwic]

I wrote a patch for StatisticsTracker?.js, it count basically the same things but store it in an array, and reset in a new structure every 30 seconds (we can easily change that).

As I'm not familiar with the GUI and don't know how to plot (generating pictures on the fly ?), I let an other making that and let the old structure for backward compatibility.

[mackwic]

Hmmmm, this patch looks very ugly.
I'm sorry, don't know why it rewrite all as there is still a lot of old code.

[historic_bruno]

Replying to mackwic:

I'm sorry, don't know why it rewrite all as there is still a lot of old code.

Could it be a line ending issue?

We never explicitly use Object, it would be better if you just used {}. Also InitTracker doesn't seem to depend on or modify the StatisticsTracker state, so it could just as easily be a global function instead.

StatisticsTracker is actually associated with a player entity, not the global SYSTEM_ENTITY, so you'll want to use this.entity instead when setting the timer interval.

switch/case has unconventional formatting, see our coding conventions.

[historic_bruno]

Also thinking about the purpose of the patch: it seems strange that all these different StatisticsTrackers will have essentially independent timers so it would be hard to graph or compare data for multiple players. What if instead we had something like a global StatisticsAggregator system component that used the timer and called each per-player StatisticsTracker on the interval and stored their data in its own data structure (and possibly clears their old values if we want incremental data). That could be more efficient anyway since there would only be one timer and callback.

I don't see us wanting e.g. different timer intervals per player, so I think that could work.

[mackwic]

Replying to historic_bruno:

Also thinking about the purpose of the patch: it seems strange that all these different StatisticsTrackers will have essentially independent timers so it would be hard to graph or compare data for multiple players.

True, the StatisticAggregator? solution is better. I'll work on it.

I'm sorry, don't know why it rewrite all as there is still a lot of old code.

Could it be a line ending issue?

It's a possibility, I didn't check that.

[mackwic]

Replying to historic_bruno:

We never explicitly use Object, it would be better if you just used {}.

Object.freeze return a copy of an object, with this copy immutable, it's different that using new Object(). Why don't using it when we can ?

[historic_bruno]

A few other comments. Some of the switch/case blocks can be simplified. In IncreaseResourceGatheredCounter(), it should be enough to do this:

this.timeStats[this.currTime].Resources.Gathered[type] += amount;

instead of handling each case of type separately. It's OK if the types are lower case (even if inconsistent with other stats) because that's how they're referenced in other scripts. So it's more important to be consistent across several scripts than to have one object with consistent properties ;)

And similarly for KilledEntity()/LostEntity() there's no need to handle each case of unit rank:

++this.timeStats[this.currTime].Units.Lost[cmpLostEntityIdentity.GetRank()];

There should be no other/default case, if someone adds a new rank, they'll need to update the statistics trackers too :)