fix(CORE/Player): Apply equip effect auras with -1 duration

[NinjaPleezAC]

Enforces a -1 duration value to auras applied from equip effects.
Current reliance on the DurationEntry of the auras causes at least one aura to incorrectly expire.

Changes Proposed:

• Enforces a -1 duration value to auras applied from equip effects, rather than using the value from spellinfo->DurationEntry

Issues Addressed:

• Closes AzerothCore issue The Green Tower Shield - Proc Does Not Work #5649
• Closes [The Green Tower] Equip does not proc chromiecraft/chromiecraft#531
• May close other open or unreported issues

SOURCE:

To my knowledge, there exists no concise documentation that proves this behavior was previously Blizzlike, or that the proposed behavior is Blizzlike. Please read my writeup in the comments for the supporting argument for this change.

Tests Performed:

• Tested item "The Green Tower" is now correctly applying an aura with -1 duration, and not expiring before unequip.
• Regression tested several other known working auras from "Equip:" and "Set" effects to ensure they still worked as intended.

How to Test the Changes:

1. Create/Select a character level 36+
2. .additem 1204
3. Equip item "The Green Tower"
4. .list auras
5. Observe that the aura for The Green Tower has a duration and maxduration of -1
6. Enter combat and allow yourself to be damaged by melee until the aura correctly procs

Regression:

1. Repeat steps 2-5 above with any item which has an "Equip:" or "Set" effect
2. Perform the necessary steps to test that each regression item's aura is still correctly functioning in combat or as otherwise relevant.

Known Issues and TODO List:

• This change would create new issues IF: any item exists with an "Equip:" effect that is meant to expire a set duration after initially equipping the item.

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.

[NinjaPleezAC]

The regression scope for this change is quite large, so I'm providing an explanatory writeup here:

In my research into the above linked issues related to The Green Tower, it became apparent that the aura duration data for the associated spell in Spell.dbc was not the correct duration to use when applying the aura from an item.

Others came to similar conclusions here: #8745
and here: #5682

Their PRs were ultimately rejected with the reasoning that patching the Blizzlike data was not the correct solution, as AC would only accumulate tech debt without solving the underlying problem of how the data was used.

The core argument for this PR is that if the Spell.dbc is not the source of truth for the duration of auras applied on item equip, and if it should not be altered piecemeal in something like SpellMgr.cpp's ApplySpellFix, then the duration data must come from elsewhere. Lacking any other known source for the duration data, I believe Blizzard must have globally enforced a -1 duration while applying an "Equip:" aura, as this PR does.

This would mean that there may be several other equip auras which are currently failing due to having finite durations set in their spellinfo.

The regression scope of this change, combined with how new I am to the project, necessitate thorough testing of this PR beyond what I can test on my own. And if at any point an example is provided of an item with an "Equip:" effect which is intended to expire a set duration after the initial equip, this PR should fail as incomplete, as it would break those items.

[heyitsbench]

This kind of makes sense to me, as the tooltip for the on equip effect uses the duration index of the initial spell rather than the proc spell, thus forcing the need to have the duration index for the spell be thirty seconds in this case in the DBC, but Blizz forces an indefinite duration server-side. I'm not very familiar with the spell systems in general, so I'm no authority, but I definitely do see some logic behind why this issue happens. I do agree that this needs extensive testing though. 😛

[NinjaPleezAC]

Actually, at least in the case of The Green Tower, the duration of the secondary "thorns" effect is used in the tooltip of the item, so it would be functionally fine to change the data, but not responsible to do so. Other working equip effects I've checked have DurationEntry's of 0 or 21, which end up as -1 duration when parsed. My best guess is that the Blizz developer incorrectly entered a DurationEntry for this item which equates to a 30sec duration, but that wasn't a problem for them because that value was ignored on the backend.

[NinjaPleezAC]

Many hours of scouring data later...
I've come to the conclusion that this bug(?) only affects two items in the entire game: "The Green Tower", and "Shirt of Uber".
https://www.wowhead.com/wotlk/item=45280/shirt-of-uber

The latter is a Blizz-awarded PTR item specifically for the purpose of testing a heroic Argent raid.
Its aura would also have an "incorrectly" finite 3 hour duration, were it not for the item itself also expiring after 3 hours, thus making it an impossibility for it to be negatively impacted by this system, regardless.

This means a couple things: Since items with outlier equip effect durations are few, anything this PR might break should be apparent across a wide variety of items, decreasing the likelihood of hidden regression. But it also calls into question the reasoning for not accepting previous PRs for The Green Tower.

I agree with the original closing sentiment of the previous PRs that patchwork fixes to Blizzlike data should be avoided when an underlying systems-related issue may be the culprit; but in this case it does seem to be a 1-off (or 2-off) problem that might not warrant a code change.

I'll leave it to the maintainers to decide which course to take, but these seem to be our best options and I'd like us to move forward with one of the following:

1. Use the data changes proposed in previous PRs
2. Test and use this PR
3. Hardcode an exception case for The Green Tower to use a CastCustomSpell, allowing all other items to use the previously existing codeflow.

Option 3 is similar to what is done for exceptions in Unit::HandleProcTriggerSpell. More than a little gross, imo, but it might be a good middle ground between changing Blizzlike data and making a code change with this broad of a regression scope.

Thanks for coming to my TED talk. I've done way too much homework for a 1-line, 1-item fix. @_@

[Kitzunu]

Can't this be fixed with a SpellScript if it only applies for 2 spells, or rather an AuraScript? To avoid the need of CastCustomSpell

[NinjaPleezAC]

Entirely likely, yes. This PR initially went the direction it did because of the previous rejection reasons for other Green Tower fixes. The sentiment was that it shouldn't be a one-off fix. But the more I researched, the more it made sense for it to be exactly that. I'll need to study AuraScripts a bit but I'll gladly submit one if that's the preferred approach.

[Kitzunu]

yea as it is only an issue for 1 or maybe 2 spells, then it would probably be preffered

[avarishd]

I'm confused, a simple spell correction for the duration of the spell fixes this.
Any other spells (if any) with similar issues can be resolved in the same way, no point (in my opinion) to change core mechanic for 1 spell.

    // The Green Tower
    ApplySpellFix({ 18097 }, [](SpellInfo* spellInfo)
    {
        spellInfo->DurationEntry = sSpellDurationStore.LookupEntry(0);
    });

[Kitzunu]

no point (in my opinion) to change core mechanic for 1 spell.

Well if the core mechanics are wrong, then it should obviously be fixed. All SpellInfoCorrections are hackfixes.

[Nyeriah]

no point (in my opinion) to change core mechanic for 1 spell.

Well if the core mechanics are wrong, then it should obviously be fixed. All SpellInfoCorrections are hackfixes.

Inaccurate, blizzard does corrections server side as well (that we can’t know about, except for the fact the spells worked for them while they wouldn’t work as intended for us). There’s missing DBC data to be complemented sometimes. Later down the road Blizzard implemented hotfixes but right now serverside dbc corrections is the only thing we have at our disposal

[Kitzunu]

Sure. But often people tend to go straight to that route when there might be another issue. Which would classify it as a hack