fix(Core/Spells): weapon damage based magic abilities gain too much effect from spell aura % damage increase

[Macs-Account]

Changes Proposed:

This PR proposes changes to:

• Core (units, players, creatures, game systems).
• Scripts (bosses, spell scripts, creature scripts).
• Database (SAI, creatures, etc).

Issues Addressed:

• Closes Auras of type 79 - SPELL_AURA_MOD_DAMAGE_PERCENT_DONE double dip on weapon damage % based magic abilities #22220

SOURCE:

The changes have been validated through:

• Live research (checked on live servers, e.g Classic WotLK, Retail, etc.)
• Sniffs (remember to share them with the open source community!)
• Video evidence, knowledge databases or other public sources (e.g forums, Wowhead, etc.)
• The changes promoted by this pull request come partially or entirely from another project (cherry-pick). Cherry-picks must be committed using the proper --author tag in order to be accepted, thus crediting the original authors, unless otherwise unable to be found

Tests Performed:

This PR has been:

• Tested in-game by the author.
• Tested in-game by other community members/someone else other than the author/has been live on production servers.
• This pull request requires further testing and may have edge cases to be tested.

Methodology
List of SpellIds to use while testing this
49143 - Frost Strike (Magic damage based on both Weapon % damage and flat damage)
45902 - Blood Strike (Physical damage based on both Weapon % damage and flat damage)
45477 - Icy Touch (Magic damage dealing spell)
53209 - Chimera Shot (Magic damage based on Weapon % damage only)

48266 - Blood presence for damage buff

To test, get an average damage of a spell without the aura (for spells that have damage ranges). Write it down as X
Apply a % damage buff to yourself. Write it down as Y
Get an average damage of a spell with the aura active (for spells that have damage ranges). Write that down as Z

Z/X should equal very close to Y (within statistical variation)

For ease of testing you can optionally turn up the damage % buff on blood presence or make a custom spell with a really high value of aura name 79

How to Test the Changes:

• This pull request can be tested by following the reproduction steps provided in the linked issue
• This pull request requires further testing. Provide steps to test your changes. If it requires any specific setup e.g multiple players please specify it as well.

1. Without this change, go to a target dummy and use a magic weapon damage ability like Frost Strike, and a physical weapon damage ability like blood strike
2. Gain a aura name 79 Percent damage modifier (such as blood presence)
3. Cast those spells again and calculate how much extra damage was actually added to both spells from blood presence (Your physical spell will work as expected, the magic spell will gain way too much benefit)
4. With this change, repeat the above steps

Known Issues and TODO List:

• [ ]
• [ ]

How to Test AzerothCore PRs

When a PR is ready to be tested, it will be marked as [WAITING TO BE TESTED].

You can help by testing PRs and writing your feedback here on the PR's page on GitHub. Follow the instructions here:

http://www.azerothcore.org/wiki/How-to-test-a-PR

REMEMBER: when testing a PR that changes something generic (i.e. a part of code that handles more than one specific thing), the tester should not only check that the PR does its job (e.g. fixing spell XXX) but especially check that the PR does not cause any regression (i.e. introducing new bugs).

For example: if a PR fixes spell X by changing a part of code that handles spells X, Y, and Z, we should not only test X, but we should test Y and Z as well.

[Tereneckla]

I'd say the problem lies in the implementation of MeleeDamageBonusDone, where it doesn't apply %damage auras when the spell only is physical, but still applies auras that mod physical to other spells

azerothcore-wotlk/src/server/game/Entities/Unit/Unit.cpp

Lines 13187 to 13208 in 8d9636e

	// mods for SPELL_SCHOOL_MASK_NORMAL are already factored in base melee damage calculation
	if (!(damageSchoolMask & SPELL_SCHOOL_MASK_NORMAL))
	{
	// Some spells don't benefit from pct done mods
	AuraEffectList const& mModDamagePercentDone = GetAuraEffectsByType(SPELL_AURA_MOD_DAMAGE_PERCENT_DONE);
	for (AuraEffectList::const_iterator i = mModDamagePercentDone.begin(); i != mModDamagePercentDone.end(); ++i)
	{
	if (!spellProto || (spellProto->ValidateAttribute6SpellDamageMods(this, *i, false) &&
	sScriptMgr->IsNeedModMeleeDamagePercent(this, *i, DoneTotalMod, spellProto)))
	{
	if (((*i)->GetMiscValue() & damageSchoolMask))
	{
	if ((*i)->GetSpellInfo()->EquippedItemClass == -1)
	AddPct(DoneTotalMod, (*i)->GetAmount());
	else if (!(*i)->GetSpellInfo()->HasAttribute(SPELL_ATTR5_AURA_AFFECTS_NOT_JUST_REQ_EQUIPPED_ITEM) && ((*i)->GetSpellInfo()->EquippedItemSubClassMask == 0))
	AddPct(DoneTotalMod, (*i)->GetAmount());
	else if (ToPlayer() && ToPlayer()->HasItemFitToSpellRequirements((*i)->GetSpellInfo()))
	AddPct(DoneTotalMod, (*i)->GetAmount());
	}
	}
	}
	}

So if we change L13197, we can filter out all auras that mod physical

               // mods for SPELL_SCHOOL_MASK_NORMAL still are already factored in base melee damage calculation
                int32 bonusSchoolMask = (*i)->GetMiscValue();
                if (!(bonusSchoolMask & SPELL_SCHOOL_MASK_NORMAL) && (bonusSchoolMask & damageSchoolMask))

It seems like CalculateDamage and MeleeDamageBonusDone always are called together, so maybe it also could be worth it to remove the % mods from CalculateDamage altogether not worth it

[Macs-Account]

Thank you for the code change suggestion, applying it certainly put things on the right track.
However, After reverting my code change and using yours the damage dealt by frost strike now does just slightly too little damage based on what I would expect after gaining a percentage damage boost, whereas
icy touch (plain spell)
blood strike (physical damage with flat bonus)
shadow bolt (plain spell)
chimera shot (magic damage based on weapon damage with no flat damage)
deal exactly what I expect.

If I manually add the flat damage bonus of frost strike * damage modifier to the actual damage of frost strike I get the exact value I would expect though

So the flat damage portion of frost strike is now not gaining the bonus damage when it should

I'll try to figure it out but I am a bit new at modifying any core code on this project so I will be a little slow, and would certainly accept help

[Tereneckla]

@Macs-Account Can you check this out? I am not really happy about adding the Phys schoolmask if it is a %Weapon Spell, but it does work.

https://github.com/Tereneckla/azerothcore-wotlk/tree/ele-pct-weapon-spells

[Macs-Account]

Yea, I can see the reasoning behind that but that's misrepresenting the data, which I could see leading to problems down the road.

I haven't had time to sit down and trace out the code path of how every effect on the spells get processed, but if I had to guess any spell that has weapon % damage on it has the entire spell processed through the same means, so the weapon % and flat school damage components both get sent down the same method chain.

If that's the case the solution is probably to have the school damage effect portion of the ability get treated as a spell.

If this is getting too complicated, my original commit of just having the addPct bool be true if physical and false if not did result in the correct total damage being calculated. That might give a good clue as to the issue as well.

[Tereneckla]

Yeah, adding the phys mask was a misguided late evening addition by me. I thought these spells should benefit from phys modifiers as they scale of physical damage (weapon damage), but they don't. I rectified that in the branch I linked.

Your initial solution still is valid, I generalized it a bit. It probably is the version we should go with as it is slightly faster. My branch cut out the usage of precalculated values

Did some tests, and my version is ~6% slower. 4731ns vs 4455ns

    bool const isPhysical = (m_spellSchoolMask & SPELL_SCHOOL_MASK_NORMAL);
    if (isPhysical && (fixed_bonus || spell_bonus))
    {
        UnitMods unitMod;
        switch (m_attackType)
        {
            default:
            case BASE_ATTACK:
                unitMod = UNIT_MOD_DAMAGE_MAINHAND;
                break;
            case OFF_ATTACK:
                unitMod = UNIT_MOD_DAMAGE_OFFHAND;
                break;
            case RANGED_ATTACK:
                unitMod = UNIT_MOD_DAMAGE_RANGED;
                break;
        }
        float weapon_total_pct = m_caster->GetModifierValue(unitMod, TOTAL_PCT);
        fixed_bonus = int32(fixed_bonus * weapon_total_pct);
        spell_bonus = int32(spell_bonus * weapon_total_pct);
    }

    int32 weaponDamage = 0;
    // Dancing Rune Weapon
    if (m_caster->GetEntry() == 27893)
    {
        if (Unit* owner = m_caster->GetOwner())
            weaponDamage = owner->CalculateDamage(m_attackType, normalized, isPhysical);
    }
    else
    {
        weaponDamage = m_caster->CalculateDamage(m_attackType, normalized, isPhysical);
    }

[Macs-Account]

I tested that and it seemed to work properly with the variety of spells I was using to test. I will adjust this commit.

Thank you!

[sogladev]

I tested that and it seemed to work properly with the variety of spells I was using to test. I will adjust this commit.

Could you update the original issue under "Steps to reproduce the problem" or this PR under "Tests Performed" with a list of spell IDs and auras to test this?

[Macs-Account]

@sogladev Done! Added to this PR

[Macs-Account]

@sogladev I am new to this process, is there something I need to do at this point for this PR or is anything left done by the team?

[sogladev]

@sogladev I am new to this process, is there something I need to do at this point for this PR or is anything left done by the team?

Nothing needs to be done on your end. The label "To Be Merged" means it will get merged soon^tm.

If you want more details, the process is described on the wiki here:
https://www.azerothcore.org/wiki/faq https://www.azerothcore.org/wiki/merge-process