fix(Core/Spell): Recklessness + Juggernaut causes non-critical mortal…

[EricksOliveira]

… strike/slam hits

Fix removes Juggernaut after Mortal Strike/Slam critical hit

Changes Proposed:

This PR proposes changes to:

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

Issues Addressed:

• Closes 💰50$ Bounty - [Warrior] Recklessness + Juggernaut causes non-critical mortal strike/slam hits to occur #20741
• Closes Recklessness + Juggernaut causes non critical mortal strike / slam hits to occur chromiecraft/chromiecraft#5042

SOURCE:

The changes have been validated through:

• [x ] Live research (checked on live servers, e.g Classic WotLK, Retail, etc.)
• Sniffs (remember to share them with the open source community!)
• [x ] 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:

• [x ] 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.

How to Test the Changes:

• [x ] 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. .aura 1719 and .aura 65156
2. Cast Mortal Strike and Slam
3. Note the crit hit and whether Juggernaut and Recklessness are removed correctly.

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.

[Kitzunu]

1. we use english
2. Just no. Do not ever hardcode raw values
3. Try to fix the issue in a spellscript

[EricksOliveira]

Sorry. Ok

[EricksOliveira]

If anyone has any ideas that they can share, I am available. I tried to make this change only in Spell_Warrior.cpp but without success, the critical does not happen. Buff Juggernaut always outperforms Buff Recklessness.

[vrachv]

I found the following solution:
Change the priority of the Recklessness spell higher than Juggernaut

ApplySpellFix({ 1719 }, [](SpellInfo* spellInfo)
{
    spellInfo->SpellPriority = 51;
});

For spell_warr_slam and spell_warr_mortal_strike, add:

void HandleOnHit()
{
    if (Unit* caster = GetCaster())
    {
        if (caster->HasAura(SPELL_WARRIOR_JUGGERNAUT_CRIT_BONUS_BUFF))
        {
            caster->RemoveAura(SPELL_WARRIOR_JUGGERNAUT_CRIT_BONUS_BUFF);
        }
    }
}

You also need to add spell_warr_mortal_strike to the database.

It works, but its a hack and I dont like it. I simply elevated Recklessness by increasing its priority, and I forcibly remove Juggernaut.

Currently, code that calculates auras does not working for multiple auras with charges working together:

if (mod->ownerAura->IsUsingCharges())
{
    if (!chargedMod || (chargedMod->ownerAura->GetSpellInfo()->SpellPriority < mod->ownerAura->GetSpellInfo()->SpellPriority))
    {
        chargedMod = mod;
    }

    continue;
}

In my opinion, it would be best to allow some auras to trigger in any case, but I'm not sure how this would affect the system globally since I've only been studying this project for a few days.

[EricksOliveira]

From what I understand, Juggernaut surpasses Recklessness and for some reason there is a Critical Limit of 100%, if it exceeds this value, there is a return where one of the buffs is disabled so as not to exceed this value. And it seems that Juggernaut surpasses Recklessness. In short, Recklessness is disabled, according to the dbc. @UltraNix #13498

// xinef: do not allow to consume more than one 100% crit increasing spell if (mod->op == SPELLMOD_CRITICAL_CHANCE && totalflat >= 100) { return; }

[vrachv]

From what I understand, Juggernaut surpasses Recklessness and for some reason there is a Critical Limit of 100%, if it exceeds this value, there is a return where one of the buffs is disabled so as not to exceed this value. And it seems that Juggernaut surpasses Recklessness. In short, Recklessness is disabled, according to the dbc. @UltraNix #13498

// xinef: do not allow to consume more than one 100% crit increasing spell if (mod->op == SPELLMOD_CRITICAL_CHANCE && totalflat >= 100) { return; }

No, you understand it incorrectly. return occurs only if a critical hit is ALREADY guaranteed.

Since both buffs are consumable, only one—the one with higher priority based on SpellPriority—is used, and the check mod->op == SPELLMOD_CRITICAL_CHANCE && totalflat >= 100 is not related to this.

[EricksOliveira]

Pelo que entendi, Juggernaut supera Recklessness e por algum motivo existe um Limite Crítico de 100%, se ultrapassar esse valor, há um retorno onde um dos buffs é desabilitado para não ultrapassar esse valor. E parece que Juggernaut supera Recklessness. Resumindo, Recklessness é desabilitado, segundo o dbc.@UltraNix #13498
// xinef: do not allow to consume more than one 100% crit increasing spell if (mod->op == SPELLMOD_CRITICAL_CHANCE && totalflat >= 100) { return; }

Não, você entendeu errado. O retorno ocorre somente se um acerto crítico JÁ estiver garantido.

Como ambos os buffs são consumíveis, apenas um — aquele com maior prioridade com base na SpellPriority — é usado, e o teste mod->op == SPELLMOD_CRITICAL_CHANCE && totalflat >= 100não está relacionado a isso.

Sorry. I misunderstood then. Thanks for the reply.