[9893-9898][Patch] Spell Bonus Coefficient Calculation Refactoring

[Auntie Mangos]

What bug does the patch fix? What features does the patch add?

• * Move default coefficient calculation (when no spell_bonus_data entry is used) to its own method in SpellMgr, CalculateDefaultCoefficient. This default coefficient calculation is basically the block of code that Unit::SpellHealingBonus and Unit::SpellDamageBonus had in common.
  * Calculate coefficients for spells in loading spell_bonus_data using CalculateDefaultCoefficient and compare it with what the entry in spell_bonus_data is. When the difference is small, output a DB error (maybe an outDebug could be used instead of outErrorDb, but I just wanted a fast way to copy and paste the output ). I chose the threshold of 0.02f(2%) arbitrarily, not sure if there is a better number to use here or a different method of determining which entries are more needed than others.
  * Do not add coefficient to healing part of life drain effects/auras, it is already considered when calculating the damage
  * Moved the CastingTime declaration
  

  uint32 CastingTime = !IsChanneledSpell(spellProto) ? GetSpellCastTime(spellProto) : GetSpellDuration(spellProto);

  from Bonus calculations to GetCastingTimeForBonus.
  * Subtract 5% from coefficients per "additional effect" as stated, instead of doing CastingTime -= 175 (which is only correct when the spell has a cast time of 3.5).
  * Minor changes:
  * The comment that reads "// Pet damage" is actually used for non-pet creatures
  * Typo: Bonusess -> Bonuses
  

For which repository revision was the patch created?

8284

Is there a thread in the bug report section or at lighthouse? If yes, please add a link to the thread.

http://getmangos.eu/community/viewtopic.php?id=6733 <- Not really a bug report, but I'm hoping this will make checking fixes like these easier.

Who has been writing this patch? Please include either forum user names or email addresses.

Myself

One thing that confused me were the lines introduced in this commit:

   // Not apply this to creature casted spells with casttime==0
   if(CastingTime==0 && GetTypeId()==TYPEID_UNIT && !((Creature*)this)->isPet())
       return 3500;

Is there a reason why 3500 is returned for instant cast spells but not others? My assumption was that it was supposed to make creatures get a coefficient 1.0f when they had "pseudo" spell power from auras, but this fact seems to have been lost in the commit that introduced spell_bonus_data. I removed these lines in my patch (and replaced them with a default coeff of 1.0f), but if someone could affirm (or disprove) my assumption, that would be nice.

Patch

If calculating and comparing the coefficients on load is seen as unnecessary work, here are the entries that were printed out so that perhaps a one-time SQL patch can be used. These are all the spells with a difference less than two percent of the calculated value, or spells that don't even match the wiki.

Entry   DamageType      DB Value    CalcValue   Difference
2912	`direct_bonus`	1.000000	1.000000	0.000000
139	    `dot_bonus`    	0.376000	0.376000	0.000000
603	    `dot_bonus`    	2.000000	2.000000	0.000000
30451	`direct_bonus`	0.714300	0.714286	0.000014
29722	`direct_bonus`	0.714300	0.714286	0.000014
2136	`direct_bonus`	0.428600	0.428571	0.000029
2948	`direct_bonus`	0.428600	0.428571	0.000029
25912	`direct_bonus`	0.428600	0.428571	0.000029
51505	`direct_bonus`	0.571400	0.571429	0.000029
5676	`direct_bonus`	0.428600	0.428571	0.000029
17877	`direct_bonus`	0.428600	0.428571	0.000029
774	    `dot_bonus`    	0.376040	0.376000	0.000040
689	    `dot_bonus`    	0.142800	0.142857	0.000057  ^-- Values above here definitely do not need spell_bonus_data
5570	`dot_bonus`    	0.127000	0.126667	0.000333  <-- Both wrong , Insect Swarm should should have 20% dot bonus
11426	`direct_bonus`	0.805300	0.805714	0.000414
34861	`direct_bonus`	0.402000	0.402857	0.000857
8092	`direct_bonus`	0.429600	0.428571	0.001029
32379	`direct_bonus`	0.429600	0.428571	0.001029
32546	`direct_bonus`	0.806800	0.805714	0.001086
19236	`direct_bonus`	0.806800	0.805714	0.001086
2061	`direct_bonus`	0.806800	0.805714	0.001086
17	    `direct_bonus`	0.806800	0.805714	0.001086
33110	`direct_bonus`	0.806800	0.805714	0.001086
421	`    direct_bonus`	0.570000	0.571429	0.001429  <-- Wiki seems to have rounded this value
1449	`direct_bonus`	0.212800	0.214286	0.001486
2060	`direct_bonus`	1.613500	1.611429	0.002071
8004	`direct_bonus`	0.808200	0.805714	0.002486
1064	`direct_bonus`	1.340000	1.342857	0.002857  <-- Wiki seems to have rounded this value
596	    `direct_bonus`	0.808600	0.805714	0.002886  <-- Typo?
6789	`direct_bonus`	0.220000	0.214286	0.005714  <-- My calculated value is closer to wiki value than spell_bonus_data's
14914	`dot_bonus`    	0.024000	0.029962	0.005962
26573	`dot_bonus`    	0.040000	0.033333	0.006667
44457	`direct_bonus`	0.400000	0.407143	0.007143
348	    `direct_bonus`	0.200000	0.207714	0.007714
30283	`direct_bonus`	0.195000	0.203429	0.008429
44457	`dot_bonus`    	0.200000	0.190000	0.010000
8921	`dot_bonus`    	0.130000	0.116629	0.013371
589	    `dot_bonus`    	0.182900	0.200000	0.017100

30108	`dot_bonus`    	0.240000	0.200000	0.040000  <-- Duration has been decreased from 18 to 15
172	    `dot_bonus`    	0.300000	0.200000	0.100000  <-- My calculated value is closer to wiki value than spell_bonus_data's (exact same in fact)

42208	`direct_bonus`	0.095200	0.214286	0.119086  <-- Both incorrect (calculation doesn't know that this is a spell triggered from a channeled spell). This is an example of something that strictly *needs* an entry.

EDIT: I just noticed that the coefficient calculation for DoT/hybrid spells seems a bit off.

EDIT2: Ok, fixed a typo in DoT calculation and made the effects counter less generic; it was penalizing DoTs for very weird effects, like Rejuvenation's mod rating effect which doesn't actually do anything.

EDIT3: I think there is a more accurate method of calculating coefficient penalties for AoE spells with "additional effects." I note that Shadow Fury, Dragon's Brath, Frost nova, Typhoon seem to take *(0.95^2) penalty to coefficients, while Cone of Cold only has a *0.95 penalty.

EDIT4: Small update to this patch to correctly cover the aforementioned spells... Basically make the calculation less generic and more correct . I didn't update the above table, however, because it's still pretty much the same spells that either need changes or need to be removed.

[imblizzspy]

So much effort was put into this and so little feedback, it's kinda hard to test thou.

btw, nos4r2zod you're amazing. Big kudos for making things more generic and simplified. MaNGOS spell system is one of the bottlenecks

[sk1pp3r]

Does it fix outdated spell coefficients?

[VladimirMangos]

Suggested way cleanup and fixing spell bonus coeffs claculation added (in fact after manual recreating at current sources with some addition problem fixing)

in [9893-9898]. Thank you

Does it fix outdated spell coefficients?

It' can't fix opudated value (you need manually check data for find porblems as before) but patch let add to table less data.