Stats.cs: The big spring cleanup (?)

Topics: Rawr.Base
Developer
Feb 23, 2010 at 11:46 AM

Stats is an object that sees extensive use in all of rawr.  It gets frequent updates, and there's a lot of stale bits in it popping up.

A lot of the stats in there are totally obsolete, and yet, models still make use of them.  Stats.bloodlust is probably the most obvious one of this.  While this may at one point in time has served the purpose of taking care of the bloodlust/heroism buff, it no longer does so.  Bloodlust/heroism today is just coded as an OnUse trigger with a PhysicalHaste, Meleehaste and Spellhaste specialeffect.  I talked about this with Orlangur last night and it seems he already took this one out and done some more cleanup already as well.

Once rawr is fully loaded and you have an empty character , there's a whopping 13000ish different stat objects 'alive'.  Each time you add a new stat to Stats, this effectively means rawr consumes 52K extra memory. Weeding out the old obsolete/unused stats does make a lot of sense.  It also means it makes sense trying to be a bit more conservative on memory use.

Quite a few of the variables in Stats are currently intermediate variables used by a particular model to conveniently pass around a calculated value (a talent turned into a stat for example) from one part of the model to the next.  A nice idea, but it has a few problems.
1) By adding this variable in stats.cs, you're adding weight to EVERY instance of a stat object.
2) The variable you created may get picked up by another model under the assumption it's a stat that can come from an item/enchant/buff. Especially if the name have been chosen rather generically.  Example: The stat 'AverageAgility' is listed under the category "Feral". It's not used anymore (likely an inermediate variable for an old talent or set bonus?). Both hunter and rogue are using it. Probably because when the model devs had a look over the various stats, they figured they needed it.
3) It increases the list of possible values listed in the item editor. And it likely won't even have any impact if a user does fill one in, even in a right module. Because that model will only set this intermediate but not use a potential value already there.

 

I.m.o.  the ideal situation would be that Stats.cs only has those stats that can come from Items, Enchants and Buffs.  To accomodate for these intermediate variables, Stats.cs can be change to provide for model specific values.  Maintaining those extra variables is then the sole responsability of each model meaning less chance of clutter and overall lower memory usage (which should translate into some performance gain as well).  It also means that stale intermadiates are more likely to get removed.  Right now, when you stop using a variable, you probably don't immediately think about removing that variable in stats.cs.  If some other model made a reference to it (in error), you're just going to let it be, perpetuating the problem.

Probably the easiest way would be to have a string/float dictionary added to stats.cs

Stats.cs:

             public Dictionary<string,float> ModelStats = new Dictionary<string,float>();

In your model.

           stats.ModelStats["MyStats1"] = <calculation>;
           stats.ModelStats["MyStats2"] = <calculation>;
           float Something = stats.ModelStats["MyStats1"] + stats.ModelStats["MyStats2"];

But there's other possibilities (such as an array and using an enum in this array).
The clone, multiplication, averaging etc could be made to also account for these model vars, but I'm not sure that's even needed.  Model devs should know best how they expect their model specific intermediate/helper variables to get used.

 

Good idea, Bad idea ?
           

 

 

 

Feb 23, 2010 at 1:17 PM

Some thoughts:
- the problem is, currently Stats are designed to allow + and - operators
- Stat objects have a short lifespan, they are only used to calculate CharacterCalculations, so they are just garbage collected when needed and memory should not be an issue here
- at present, a number of different stats is really huge and it's really hard to extract stats your module needs to handle from them all
- some stats are duplicated like having a proc as a special effect granting some stats and at the same time as a custom "blablablaProc" stat that is handled separately in some calculations 

Developer
Feb 23, 2010 at 3:02 PM

I know Stats has operators. I even mentioned this in the post, I just didn't refer to the + and - ones...  It shouldn't necessarily be a problem, you can enumerate all the stats in the dictionary and add them to the other dictionary.   But as I wrote...  "I don't know if this is needed".  I'm not sure the stats objects that are used to pass those extra vars around are indeed also used in this way.  If yes, then the operators need to account for this dictionary, if not, then they don't "have to" (but still could) just to be complete.

Most stats objects have a short lifespan. Around 13000 of them stay alive for the duration of rawr. All the enchants, buffs, their special effects, cached items and their special effects etc... 

 

 

Coordinator
Feb 23, 2010 at 5:34 PM

We definitely can audit it and remove the unused stats. The memory usage isn't really the significant problem, it's the CPU load on working with them that's the issue. Because of the indexes that are used under the hood, it doesn't actually cost much (anything?) to have unused or rarely used stats in there. Kavan's most familiar with that system, so can comment further, I'm sure.

Developer
Feb 23, 2010 at 10:34 PM

workign with 1 stat object won't change much indeed... but 13000 stats objects each using that bit more memory, means a bigger overall memory usage.  THat usually translates to some performance loss due to L1/L2 cache misses, more paging needed etc.  Not a huge performance factor, but trimming 1Mb or so (what I'm guessing at)  should make a small change at least.

Coordinator
Feb 24, 2010 at 3:01 AM
Edited Feb 24, 2010 at 3:01 AM

Regardless of impact on performance or otherwise Stats should ideally be used only for things coming from gear/enchants/buffs. As far as I'm concerned talents have no place in there. If some models are using Stats to store intermediate variables in it that needs serious audit. Ideally from the point where you accumulate all stats on character the Stats would be considered read only.

Coordinator
Feb 24, 2010 at 3:05 AM

Aye, agreed. I may be a little bit guilty of that myself; I think there are a couple talents I may translate to Stats properties, that aren't otherwise on gear/enchants/buffs (like reduced energy cost on abilities). Do you reinspect the talents in GetCharacterCalculations, for that sort of thing, Kavan?

Coordinator
Feb 24, 2010 at 4:33 AM

For talents that modify abilities I usually check them in the ability class when constructing data for that ability.

Feb 25, 2010 at 4:02 PM
Edited Feb 25, 2010 at 4:22 PM

I cleaned up the usage of Stats.cs in Rogue, it now only has set bonuses and the normal things from enchants/gear/buffs in it, all talent info has been pulled out.
If anyone encounters anything Rogue related that doesn't belong there let me know or just delete it, break Rogue and I'll fix it (just don't do the latter right before a release :) ).

Feb 25, 2010 at 6:56 PM

Suggestion for set bonuses: instead of having separate 2pc and 4pc stats that are really just bools masquerading as floats (either you have the set bonus or you don't), can't we just have a DPSWarr_T10 = 1 on each tier item, and then have set bonus buffs added by inspecting your equipment stats?  Currently we have DPSWarr_2T10 = float and DPSWarr_4T10 = float

That would cut down the number of set bonus stats 50% (which can be a big deal when you consider that all stats objects have a stat for Warrior 2T7, Mage 2T7, Feral 2T7, ..., Warrior 4T7, Mage 4T7, Feral 4T7, ..., Warrior 2T8, ..., Warrior 4T10, Mage 4T10, Feral 4T10.

Coordinator
Feb 25, 2010 at 7:08 PM

No, don't do that. Set bonuses are stats, and have values associated with them, which are frequently used elsewhere as well Stats properties should be named what they do, not where they come from.

Developer
Feb 25, 2010 at 8:25 PM

With the set bonuses that are not generic (like +AP) and are more specific (like -2 sec MS CD or +1 Sudden Death Proc effect on Proc) it's easier to determine where these things come from and how to deal with them, and eventually how to get rid of them (when the current tiers become out of date in the next expansion due to the new Tiers).

Coordinator
Feb 25, 2010 at 8:53 PM

Nope, please use descriptions of what the stat does, not it's source. That promotes code readability and maintainability.

Developer
Feb 25, 2010 at 9:12 PM

I don't speak for the other classes but here's the list of DPSWarr set bonues as found in the code, they do both:

BonusWarrior_T7_4P_RageProc,
BonusWarrior_T8_2P_HasteProc,
BonusWarrior_T8_4P_MSBTCritIncrease,
BonusWarrior_T9_2P_Crit,
BonusWarrior_T9_2P_ArP,
BonusWarrior_T9_4P_SLHSCritIncrease,
BonusWarrior_T10_2P_DWAPProc,
BonusWarrior_T10_4P_BSSDProcChange,
BonusWarrior_PvP_4P_InterceptCDReduc,

Developer
Feb 25, 2010 at 9:57 PM

Would something like this be acceptable?

        /// <summary>
        /// Tier 8 4-piece bonus
        /// </summary>
        [System.ComponentModel.DefaultValueAttribute(0f)]
        [DisplayName("ShoR Block Value")]
        [Category("ProtPaladin")]
        public float ShieldOfRighteousnessBlockValue
        {
            get { return _rawAdditiveData[(int)AdditiveStat.ShieldOfRighteousnessBlockValue]; }
            set { _rawAdditiveData[(int)AdditiveStat.ShieldOfRighteousnessBlockValue] = value; }
        }

This satisfies that the property name describes what it does, but you can still reference where it came from by simply hovering over the property.  I like the idea of being able to reference what custom stat came from where as well.

Coordinator
Feb 25, 2010 at 10:50 PM

Roncli's example looks perfect.

Coordinator
Feb 25, 2010 at 11:05 PM

I don't know, I kind of prefer Mage2T10 over inventing a name for "Your Hot Streak, Missile Barrage, and Brain Freeze talents also grant you 12% haste for 5 sec when the effect of the talent is consumed."

Developer
Feb 26, 2010 at 12:12 AM

MageProcUseHaste? :)

I know what you're saying, though.  Some of the ProtPaladin ones are pretty long.  I don't mind it too much, though.

Developer
Feb 26, 2010 at 12:16 AM
Kavan wrote:

I don't know, I kind of prefer Mage2T10 over inventing a name for "Your Hot Streak, Missile Barrage, and Brain Freeze talents also grant you 12% haste for 5 sec when the effect of the talent is consumed."

There's always 'YHSMBaBFtagy12Perchf5secwtetic'

Coordinator
Feb 26, 2010 at 4:41 AM

HasteOnMageSpecProc?

...Or just make it into a SpecialEffect, with a trigger of HS/MB/BF?

Developer
Feb 26, 2010 at 6:59 AM

Personally I prefer Priest_T10_4pc over PriestMindFlayCastDurationAndTickIntervalDecrease. Also blizzard are known to change set bonuses from time to time.

Coordinator
Feb 26, 2010 at 7:19 AM

Come on, quit being obtuse guys. You don't have to name it overly long shit like that. MindFlayTickOffset.

Developer
Feb 26, 2010 at 11:43 AM

My 5 cents worth.

My main problem I'm having with it now, is that when I was working on the Retribution relevancy modelling and passed over stats.cs several times. I continuously had to ask myself. "is this useful for a ret?". And that wasn't always easy to answer.

Part of that problem is that not all class/model specific stats are listed under a "Added by Rawr.<Model>".  so you get to a stat called "AverageAgility". And the problem starts.  Agility is usefull, so I need to add this right ?  Several models did, but when you do some more investigation, you find it's not even used at all anymore. It's probably a leftover from an old feral setbonus. BaseAgillity, ok I want agility, I even want base agility.  but again that's not what this stat does, it's a helper variable for the Dodge calculation.

In this particular case. The names itself wouldn't have been a problem if it had been listed under "Added by Rawr.feral".  And to a further extent, how a model names their setbonus variables/members.  doesn't really matter either since it's largely hidden for everyone but that model.  Just be aware that you do always provide a usefull "name" that will be visible in the item editor.  And in that respect, a user will probably have a better time understanding "2T10 mage bonus" than "%haste when consuming talent procs".  Set bonusses are a bit of a weird thing... you're not likely to find these bonusses on anything other than set tiers.  They're stats, but they aren't as generic a stat like strength, haste...

 

Of course, 'hiding' everything in a "Added by Rawr.<model>" region still is no excuse for overloading stats with stuff that shouldn't be there. ;-)    I agree that stats.cs should be restricted to only those stats that are needed in buffs/items/enchants.