Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
seppurt opened this issue Jan 5, 2020 · 16 comments
Closed

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

seppurt opened this issue Jan 5, 2020 · 16 comments
Labels
bug Something isn't working

Comments

@seppurt
Copy link

seppurt commented Jan 5, 2020

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):

  • Has 246 threat on the mob, before attacking
  • Attacks for 106 damage
  • Has 331 threat on the mob after attacking

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 dfherr added the bug Something isn't working label Jan 5, 2020
@dfherr
Copy link
Owner

dfherr commented Jan 6, 2020

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
Copy link
Author

seppurt commented Jan 6, 2020

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
Copy link
Owner

dfherr commented Jan 6, 2020

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:

  • Was the aura active before beginning combat or after?
  • What happens, when you lose and gain the aura during combat (e.g. the shaman puts another air totem and then switches back to tranquil air)?
  • Try killing one monster, leave combat (i.e. you can eat) and then fight a second monster without ever losing the buff. Do you have the threat modifier active then?

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

@seppurt
Copy link
Author

seppurt commented Jan 6, 2020

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:

  • If you kill a mob while being affected by Tranquil Air, the next mob you fight (after dropping combat) will count the player as having Tranquil Air active. This is regardless if Tranquil Air is actually up or not on this subsequent mob (applied either pre-combat or mid-combat with the subsequent mob). This will be the case for the entire combat duration with the subsequent mob.

  • If you kill a mob while not being affected by Tranquil Air, the next mob you fight (after dropping combat) will not count the player as having Tranquil Air active. This is regardless if Tranquil Air is actually up or not on this subsequent mob (applied either pre-combat or mid-combat with the subsequent mob). This will be the case for the entire combat duration with the subsequent mob.

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
Copy link
Owner

dfherr commented Jan 6, 2020

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.

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
Copy link
Author

seppurt commented Jan 6, 2020

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
Copy link
Author

seppurt commented Jan 16, 2020

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

  • Applying Blessing of Salvation works as expected, both in combat and out of combat.
  • Threat modifier is checked when exiting combat, just like described in my Tranquil Air post above, and is saved from one combat to the next.
  • Clicking Salvation off does not call upon the function to update threat modifier, neither in combat nor out of combat. Thus, exiting combat with Salvation buff active -> leaving combat -> clicking Salvation off -> entering combat with a new mob yields a threat modifier that has Salvation active.

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
Copy link
Owner

dfherr commented Jan 18, 2020

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
Copy link

Dyaxler commented Apr 17, 2020

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
Copy link

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
Copy link
Owner

dfherr commented Apr 21, 2020

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):

  • check for buffs whenever an air totem is cast
  • check for buffs whenever entering combat (currently only when leaving combat...)
  • check for buffs whenever an aura got removed that could not be resolved (spellid 0)

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
Copy link
Owner

dfherr commented Apr 21, 2020

I added buffmod tracking with UNIT_AURA in the latest commit 99d3dc2

This should correctly track totems, but might impact performance.

@dfherr
Copy link
Owner

dfherr commented Apr 22, 2020

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
Copy link

Orvisky commented Apr 28, 2020

@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
Copy link
Owner

dfherr commented Apr 28, 2020

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
Copy link

Dyaxler commented Apr 30, 2020

@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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants