[patch/dev][8635] bonus damage handling

[Auntie Mangos]

if i understood you correctly, with this patch, Ranged and melee based attacks wont benefit from spell power and magic based attacks will not benefit from attack power.

if that is the case, wouldn't that screw paladin's damage calculation completely?

i mean, as patch 3.x most paladins damage dealing abilities benefit from AP and SP.

in ie:

Seal of Command, witch is Melee based, benefits from spell power.

Judgements, Exorcisms, and Consecration are "magic" based abilities and they benefit from attack power. Yesterday 07:00 PM

There are a lot of magic based spells that profit from attackpower. Mostly for melee classes that use also magic spells (Paladin, Hunter). But that is not the generic case. For those cases we have the spell_bonus_data table (ok, an entry for ranged attackpower is missing currently (hunter) but that should not be a problem to implement).

The Seal of Command proc is the only weapon based ability that profits from spell power (as far as I know). As this is an weapon based spell that uses Effect:WeaponDmg you anyway have to handle this seperatly (it is a part of the base damage, so you have to add it before apllying any percentage damage mods, what is already done in Effect:WeaponDmg) as it was already discussed here: http://getmangos.eu/community/viewtopic.php?id=6388

If you know any other weapon based spells tell me. But as there are anyway very less non - physical, weapon based spells, I think there aren't any more.

[pasdVn]

As I have already proclaimed in other threads, I think that the spell bonus damage calculation depending on the school of the spell is incorrect. I tried to make the the bonus damage calculation dependent on the damage class. That means weapon based Spells (SPELL_DAMAGE_CLASS_RANGED, SPELL_DAMAGE_CLASS_MELEE) are all handeled by MeleeDamageBonus() while "magical" spells (SPELL_DAMAGE_CLASS_MAGIC, SPELL_DAMAGE_CLASS_NONE) are handeled in SpellDamageBonus().

This system has some advantages:

• * we can handle all bonus damage of "magical" spells in SpellBonusDamage() as we did before, but don't have to concern about any weapon dependent auras
  * we can handle all spells, that have anything to do with your weapon in MeleeDamageBonus(), including checks for weapon dependend auras, enhanced school checks, etc
  

So in the end everything should be a bit more clear and better to understand.

Some examples, I want to reach with that patch:

• * "Arcane Shot" or "Explosive Shot" won't benefit from spell damage
  * all hunter stings and shots that cause non physical damage are affected by "Ranged Weapon Specialization"
  

Problems is sometimes to find generic rules that say when such a wepon using spell benefits from a damage mod (mostly percentage mods for wepon based spells, but sometimes also flat mods). After doing a lot of researches and thinking about I think it's the best to check if the aura's schoolmask fits with the spells school (that's nothing new) AND with weapons school (always physical for players for now) the spell is using.

Again an example:

• * you have any aura that increases you fire dmage by x% (or adds Y fire damage) and you are using "Explosive Shot" => won't be affected
  

For damage taken auras we don't have this problem. There a normal school check belongs as I could find out.

All changes affect also DOT's, of course, meaning, that e.g. auras increasing your damage with a one handed weapon, will also increase the damage of a dot, that was applied with a one handed wepon etc..

Some of you will notice, that there are just shots and stings as examples now (in my previous versions I also talked about "Seal of Command"). The reason is, that I found out (or am at least the oppinion), that weapon damage based spells (that measns spells using SPELL_EFFECT_WEAPON_DAMAGE, SPELL_EFFECT_WEAPON_DAMAGE_NOSCHOOL, SPELL_EFFECT_NORMALIZED_WEAPON_DMG or SPELL_EFFECT_WEAPON_PERCENT_DAMAGE) should be not handled with those school checks.

Those spells just take your current autohit or ranged damage (where a lot of bonus damag is already included) and change the damage class, no matter if the the bonus damage that are applied to weapon damage fit to the new damage class (this seems to be also quite logical, if you read the tooltips). This is done already correctly in the code and does not need any changes. I just want to mention that, because previously I wrote other things in this place.

Some more things I worked out:

• * bonus damage calculation for physical Effect:SchoolDmg spells (this is missing currently completely!!!) as nearly all hunter pet spells, some feral druid spells, or e.g. "Steady shot", this will probably the change you will notice the most when playing
  * a lot of cleanup in MeleeDamageBonus() and CalculateSpellDamage() (including adding the spell crit bonus damage calculation to SpellCriticalDamageBonus()), that is why that patch looks quite huge
  * implementation of two global helper functions for more often used code
  * add pet's bonus damage value to MeleeDamageBonus() (I checked the correctness on an official server)
  

Update 29.08.09:

The Patch V3: http://github.com/pasdVn/mangos/commit/fe8908b88b1a4f5797bc7b863a54b12b5c086503

pasdVn

[Sephiroth1983]

This patch is really interesting and needed, thanks for working on this complex patch.

[hikikomori18]

I hope the giant "change", doesn't hurt its chances to be reviewed and commited. I know its gonna be a long review.

[pasdVn]

As I said I rather expect this patch to be reviewed soon, but more to show up a more correct damage calculation (or at least a way that is more close to the calculation on official servers).

Sometimes it is maybe faster to write an own patch than reviewing another one^^

[rechapa79]

if i understood you correctly, with this patch, Ranged and melee based attacks wont benefit from spell power and magic based attacks will not benefit from attack power.

if that is the case, wouldn't that screw paladin's damage calculation completely?

i mean, as patch 3.x most paladins damage dealing abilities benefit from AP and SP.

in ie:

Seal of Command, witch is Melee based, benefits from spell power.

Judgements, Exorcisms, and Consecration are "magic" based abilities and they benefit from attack power.

[The_Game_Master]

Yes spell bonus damage is incorrect. But i think the formula is somewhere in between CLASS and SCHOOL damage calculation.

[oc_redfox]

This is one of those patches that makes you raise your eyebrow...

Very interesting conclusion =/

This way seems more generic than the old one...

[The_Game_Master]

Any news about this? Looks very promicing. In the curent state it could only lead to a crash. So come on, a review... something.

[emsy]

If you know any other weapon based spells tell me. But as there are anyway very less non - physical, weapon based spells, I think there aren't any more.

http://www.wowhead.com/?spell=60103

[hikikomori18]

http://www.wowhead.com/?spell=60103

School Fire

[pasdVn]

Ah, very nice. This spell should not profit from spellpower directly. Only via Flametongue (the dumm effect). So it complys with this system :-)

[oc_redfox]

Tested it but reverted, causes to many crashes...

[The_Game_Master]

Tested it but reverted, causes to many crashes...

Likewise, there seems to be a small difference tough. Hunter's dps semmes sligthly changed.

Alltough it's nothing wrong with the code, i think it has some deeper implications that are not added in it.

[pasdVn]

I worked again on that patch

I decided to remove the idea with SPELL_ATTR_EX5_WEAPON_DMG_MOD_ALL_DAMAGE for now, because it has nothing to with the original idea.

Updates:

• * crash fix (hope there won't be any problems now)
  * a lot of cleanup in MeleeDamageBonus(), looks better now :-)
  * a bit more cleanup in CalculateSpelldmage() (moved crit bonus calculation for weapon based spells to SpellCriticalDamageBonus() )
  * moved some scripted effects to MeleeDamageBonus()
  

Would be nice if some of you guys could test it again. I'm also looking forward to any kind of comment or discussion.

Patch in first post.

Edit: One more thing to mention: The changes/cleanup in CalculateSpellDamage() and SpellCriticalDamageBonus() also fix a wrong handling of SPELL_AURA_MOD_ATTACKER_RANGED_CRIT_DAMAGE, SPELL_AURA_MOD_ATTACKER_MELEE_CRIT_DAMAGE, SPELL_AURA_MOD_CRIT_DAMAGE_BONUS_MELEE, SPELL_AURA_MOD_CRIT_PERCENT_VERSUS. As you can see in the code, currently the whole damage (that means the damage + critbonus) is multiplied with the factors of this auras, what is incorrect, imho. Correct handling is, to multiply only the critbonus with those factors.

[przemratajczak]

is this project died?

[Shad00w]

Hunter's have a really small dmg and dps.. We must fix this This patch its a very important for hunter's.

[JordanMckeag]

Anyone else review it

I ain't had any crashes from it yet

[pasdVn]

Thank you for your interest :-)

I just want to clarify some things:

• * project is more alive than ever :-P
  * this project - in fact - has nothing to do with balancing any classes, it's just a try to implement a correct math of bonus damage calculation, that should be as near as possible to the real system, that is used on official servers. The only reason, that I mostly use hunter spells as examples is, that I know this class quite well, and that there are a lot of hunter spells, that have interesting spelleffect/dmgclass/dmgschool combinations, that can be used to generalize the logic behind the calculation. (besides I don't think this patch will increase hunter dps significant, because it is more a "nerf" for all non physical dmg shots/stings)
  * originally, I don't wanted this patch to be fully reviewed, but in the meantime, now that I invested quite a lot of work and research, I have to admint, that I would pleased if any dev is interested in this patch^^, but not just now, because patch is still not fully correct, as I found out
  

What is still incorrect:

• * I quote myself here from my first post:
  

you have skilled "two handed weapon specialization" (http://www.wowhead.com/?spell=20113) and using "Seal of Command" (the proc) => "Seal of Command" proc won't be affected because it is holy damage

This is wrong in two points:

* any damageSchool depending "damagedone" auras are already included in the the normal "white damage" part of effect::WeaponDamage spells (as Seal of Command) , so changing anything in MeleeDamageBonus() won't change anything at all to this (I don't know what went on in my head when writing this^^)

* I think my current idea ( the school of the "damage done aura" has to fit to the spells school and the weapon's school) is anyway wrong for spells with effect::WeaponDamage, I think those spells should really deal (as a lot of tooltips of such spells say) "the weapon's damage as holy/fire... [whatever] damage". This means that my current provided patch is correct again^^

• * damage done flat modifieres should be scaled in MeleeDamageBonus() (->melee/ranged spells) the same way as in SpellDamageBonus() (-> magical spells), that means for example the well known 42,86% for instant spells. I checked that myself on an offi server, so that information should be really reliable ;-)
  * the "petbonusdamage" is also added to melee/ranged spells of pets (hunter as well as warlock), I also checked that myself on an offi server, to convince myself of it's correctness (I admit that this will really be a huge improvement for hunter pets)
  

I will provide an updated patch within the next days.

[16roman16]

Are some progress?

[przemratajczak]

i test this patch since 2 weekes, and dont see any performance lost or other problems.

Base ranged dmg (white) before and after patch does not changed (it is properly counted using formula from wowwiki), but overall dmg including skills was fixed, especialy for hunters who was most weakend before applying. If there are any questions about behaviour of this patch please ask

[pasdVn]

Thank you for feedback

Updated patch and description in first post with the changes I talked about.

[przemratajczak]

i forgot to add that probably related to this patch issue. SpellMgr.h is missing Unit.h include. I've got error in Debian (gcc 4.3+) compilation (missing attack types definition or something like that don't remember now). In VC every thing was all right.

Will test new version and give feedback.

edit:

are you sure you pushed changes to damagebonus branch because last commit date is still august 7th

[pasdVn]

Yes, it is the correct one. I think git uses the wrong date somehow, beacuse i rebased my whole branch and just added the newe things to this patch (via git --amend).

And yes, you are right. In fact an include is missing there. VC++ seems to interpret all enums as global!? Don't know. I moved the enum WeaponAttackType to SharedDefines.h. Just including Unit.h to SpellMgr.h is not the correct way, as there are no similar includes in SpellMgr.h and WeaponAttacktype is anyway used in several classes (so it is shared^^).

Thank you for hint. First Post Updated.

[ApoC]

Hi, I looked to this patch and made some changes. Biggest one is coding style clenup but I also improved/renamed some methods and fixed few typos. You can track my progress on:

Github: git://github.com/apoc/mangos.git damagebonus