dfherr / LibThreatClassic2

A library for multi-target threat tracking in WoW Classic. Successor of ThreatClassic-1.0
GNU Lesser General Public License v2.1
16 stars 13 forks source link

Incorrect threat modifiers (from e.g. Tranquil Air Totem) #8

Closed seppurt closed 4 years ago

seppurt commented 4 years ago

Currently, the threat modifiers from things like Tranquil Air Totem https://classic.wowhead.com/spell=25909/tranquil-air are not being calculated at all. In the .gif below, it can be seen how a warrior in Battle Stance (0.8 base threat modifier):

TranquilAirThreatBug

Expected math, with Battle Stance and Tranquil Air threat modifiers: 246 + 106 * 0.8 * 0.8 = 313.84

Obviously 331 is not equal to 313.84. More specifically, the threat modifier for the 106 damage attack is (according to the addon's visual presentation): (331-246) / 0.8 = 106.25

Meaning the threat modifier for the 106 damage attack is only 0.8, which comes from Battle Stance.

Lines 181-186 in ThreatClassModuleCore.lua show that Tranquil Air (and other things like Salvation etc.) is at least implemented in some capacity, but still does not function correctly:

-- Tranquil Air
    [25909] = function(self, action)
        if action == "exist" then
            self:AddBuffThreatMultiplier(0.8)
        end
    end,

To add to this, the shaman talent Enhancing Totems (both rank 1 and 2, spell ID's 16259 and 16295 respectively) boosts the threat reduction for Tranquil Air Totem. This is not the case in the code, but is evident in the following sources:

Hence, the addon should be updated to take these talents into account when calculating the AddBuffThreatMultiplier for Tranquil Air, as posted above.

dfherr commented 4 years ago

I just verified it again and paladin blessings are working correctly. As tranquil air totem is implemented in the same way, maybe it's an issue related to totems? Can you check what /run print(UnitAura("player", 1)) returns for you? (you might have to change 1 to 2,3... to find the tranquil air totem, depending on which buff it is).

The talent unfortunately can't be implemented in a reasonable manner. It's impossible to get the talents of another player, so the only option would be to make shamans proactively send other players that they have this talent and manage the state of shamans, etc.. This is a nightmare to implement and I won't do this sorry.

seppurt commented 4 years ago

Re-did the same type of test, and included the command you posted. See the result in the gif below.

Tranquil Air bug

Math: (374-312) / 0.8 = 77.5 compare it to 78 dmg done. Thus, only the Berserker Stance threat modifier of 0.8 active. No extra threat reduction from Tranquil Air totem.

This is the output of the command, both before and after the attack in the gif (which indicates I did indeed have the Tranquil Air aura active): Tranquil Air 136013 0 nil 0 0 party1 false false 25909 false false true false 1

dfherr commented 4 years ago

I can only guess here. Have to test that myself ingame. Which is going to be hard, as I'm playing alliance :D

Maybe this helps debugging the issue:

My best guess is that the aura is detected, but the threat multiplier is not recalculated.

seppurt commented 4 years ago

Good ideas on how to debug!

After some more testing, experimenting with killing a mob while having Tranquil Air active on the player and then killing a mob while not having Tranquil Air active on the player, here are my results:

It appears as though the function call to update thhreat modifier needs to be done more dynamically/more often. Perhaps on application/loss of buff?

dfherr commented 4 years ago

It appears as though the function call to update thhreat modifier needs to be done more dynamically/more often. Perhaps on application/loss of buff?

It updates the threat modifier on 3 occassions:

It is checking gaining/losing buffs here: https://github.com/dfherr/LibThreatClassic2/blob/master/ThreatClassModuleCore.lua#L547-L576

The call to calcBuffMods on line 571, if the aura matches one of the tracked auras (in BuffHandlers) is triggering this recalculation. Apparently this function fails for totems.

You could help debugging this by activating the debug mode here: https://github.com/dfherr/LibThreatClassic2/blob/master/LibThreatClassic2.lua#L199

By default, this will send the debug output to your 5th chat window. You can change this here: https://github.com/dfherr/LibThreatClassic2/blob/master/LibThreatClassic2.lua#L211 (e.g. make it ChatFrame1)

You also might want to temporarily increase the revision number here by 1 https://github.com/dfherr/LibThreatClassic2/blob/master/LibThreatClassic2.lua#L91

This should make sure the debug-active library is loaded and not any of the other installed ones.

You should see these 2 debug outputs, if tranquil totem would be working: https://github.com/dfherr/LibThreatClassic2/blob/master/ThreatClassModuleCore.lua#L553 https://github.com/dfherr/LibThreatClassic2/blob/master/ThreatClassModuleCore.lua#L563

seppurt commented 4 years ago

Thank you for the clear instructions.

I setup debug mode and got that part working fine. After exiting combat, the correct Tranquil Air modifier (barring Shaman talents) is set. Meaning, I get Set buffThreatMultipliers to 0.8 after killing a mob with Tranq Air totem active, and Set buffThreatMultipliers to 1 after killing a mob without Tranq Air totem active.

However, I do not get any updates (both out of combat or during combat) of Tranq Air totem application/deactivation. Thus, not triggering

You should see these 2 debug outputs, if tranquil totem would be working:
https://github.com/dfherr/LibThreatClassic2/blob/master/ThreatClassModuleCore.lua#L553
https://github.com/dfherr/LibThreatClassic2/blob/master/ThreatClassModuleCore.lua#L563

Would you mind testing if you get those debug outputs in your Paladin testing when getting Blessing of Salvation applied to you? And also if you experience the same behavior as I posted with the modifier being updated after combat (effectively meaning: the first mob you go for after getting Salv, you will not see the correct threat modifier)?

seppurt commented 4 years ago

I managed to get help from some alliance friends to try out the behavior for Salvation. Here are the results:

My conclusion is that removal of buffs/auras does not call upon the "update threat modifier". Additionally, applying Salvation does update dynamically, however Tranquil Air totem aura does not.

Does this give you any ideas of how to treat the following two issues:

  1. Buff/Aura removal is not handled correctly. No update of threat modifier is performed, unless leaving combat.
  2. Tranquil Air totem aura gain (or removal) does not seem to call upon the "update threat modifier" function.
dfherr commented 4 years ago

Thanks for the new info. I didn't have time yet to dig into this sorry. Looks like aura handling for totems is different from other buffs.

That aura removal out of combat is not tracked is kind of working as expected as the threat tracking is deactivated out of combat. Maybe a good change would be to not stop tracking out of combat in instances.

Dyaxler commented 4 years ago

It appears as though the function call to update thhreat modifier needs to be done more dynamically/more often. Perhaps on application/loss of buff?

It updates the threat modifier on 3 occassions:

when the class module is enabled (usually on login or when you join a party)
Leaving combat (this is PLAYER_REGEN_ENABLED)
when a tracked aura is applied (debuff or buff). Tranquil totem and Paladin blessings are tracked auras.

PallyPower already has a feature to automatically allow "wiping off" Salvation on tanks. The workflow would be to hit Warriors with Greater Blessing of Salvation and then come back and hit Warrior Tanks with a Blessing of Wisdom. This should kick off the threat modifier function. Just instruct your tanks to allow Paladins to wipe off Salvation and not use something like Weak Auras to automatically click it off...

Can we get an ETA on when this fix will be applied?

mikekochis commented 4 years ago

I'd say updating the threat modifier from pally buffs on a more dynamic basis makes sense (ie, a 4th "tank" that helps snag adds in Nefarion ph1, but then gets buffed with a salv to DPS on Nefarion in ph2).

dfherr commented 4 years ago

I'd say updating the threat modifier from pally buffs on a more dynamic basis makes sense (ie, a 4th "tank" that helps snag adds in Nefarion ph1, but then gets buffed with a salv to DPS on Nefarion in ph2).

I'm already dynamically updating the buffs, but there are two different mechanics at play here.

The totem buffs are completely ignored by the combatlog and thus the threatlib isn't getting an event to update the threat modifiers. I recently got some valueably extra input and finally had time to get some testing with a shaman myself (kinda hard as an alliance player ;)) and got the data I needed to evaluate this clearly.

The salvation mechanic is troublesome, because the lib checkes for SPELL_AURA_REMOVED events, but these are called AFTER the buff is removed and then trying to resolve the spellid (thanks blizz for removing that from the combatlog) fails for anyone but a paladin (because only paladins have salvation in their spellbook. This applies for manually clicking it away AND getting it overbuffed. The issue is corrected, when leaving combat or getting any other buff that has buff handling attached.

What I'm planning todo (release probably this weekend):

The last one will trigger unnecessariy load, but it shouldn't be too bad and is the only way to reliably track losing salvation.

dfherr commented 4 years ago

I added buffmod tracking with UNIT_AURA in the latest commit https://github.com/dfherr/LibThreatClassic2/commit/99d3dc239c6791383c782327eccfc4f855bf995a

This should correctly track totems, but might impact performance.

dfherr commented 4 years ago

Just tested the performance in yesterdays raid and didn't notice any relevant impact (although with decent hardware). So this will be finally fixed in the next release (weekend).

Orvisky commented 4 years ago

@dfherr can you maybe also add feature to be able to get value of buffThreatMultipliers and debuffThreatMultipliers from outside? To make it available e.g. in ClassicThreatMeter2 or TinyThreat. Thank you. To be sure what value we have and maybe make it viisble on screen in frame.

dfherr commented 4 years ago

The modifiers are available in the debug output in your 4th chat window, when you enable /tc2 debug. They print everytime they get calculated (with the new logic everytime you gain or lose any buff/debuff). It's debug information afterall and nothing else, so I think it's fine there it is.

You can also probably get this value yourself using a weakaura, by getting LibThreatClassic2 via libstub, when loading the "Player-R12" (or whatever the current revision number is) module via ace and then printing passiveThreatModifiers.

Dyaxler commented 4 years ago

@dfherr I know this issue is closed but you still haven't released a new version yet. Once that happens your fixes to threat still have to trickle down into Details and ThreatClassic2...

We have a ton of raiders who are freaking out about their threat being accurate. I've manually patched both addons with your fixes and they work beautify so in the mean time they are appeased.

ETA on revision 12?