Opened 12 years ago
Closed 8 years ago
#1459 closed enhancement (duplicate)
[PATCH] time-dependent statistics
Reported by: | mackwic | Owned by: | |
---|---|---|---|
Priority: | Nice to Have | Milestone: | |
Component: | UI & Simulation | Keywords: | patch |
Cc: | Patch: |
Description
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.
Attachments (1)
Change History (10)
by , 12 years ago
Attachment: | patch.diff added |
---|
follow-up: 2 comment:1 by , 12 years ago
follow-up: 6 comment:2 by , 12 years ago
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.
comment:3 by , 12 years ago
Keywords: | patch review added |
---|
follow-up: 5 comment:4 by , 12 years ago
Also thinking about the purpose of the patch: it seems strange that all these different StatisticsTracker
s 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.
comment:5 by , 12 years ago
Replying to historic_bruno:
Also thinking about the purpose of the patch: it seems strange that all these different
StatisticsTracker
s 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.
comment:6 by , 12 years ago
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 ?
comment:7 by , 12 years ago
Keywords: | review removed |
---|
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 :)
comment:8 by , 12 years ago
Milestone: | Alpha 11 → Backlog |
---|---|
Priority: | Should Have → Nice to Have |
Summary: | [Patch] time-dependent statistics → [PATCH] time-dependent statistics |
comment:9 by , 8 years ago
Milestone: | Backlog |
---|---|
Resolution: | → duplicate |
Status: | new → closed |
Duplicate of #3403. Closing this one as the other one has more comments and the patch there actually plots the stuff (though incorrectly atm).
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.