BreakBB / ExtendedCharacterStats

Extended Character Stats: A WoW Classic addon
MIT License
7 stars 18 forks source link

Add Classic Era version of Suppression (5-point talent) #322

Closed RA80533 closed 1 month ago

RA80533 commented 4 months ago

Suppression in Wrath is a 3-point talent that adds bonus spell hit by 1/2/3%. In Classic Era, it is a 5-point talent that adds bonus spell hit by 2/4/6/8/10% for affliction spells.

This PR adds a condition to check which version of the should be used in calculating bonus spell hit. The effects of the talent on bonus spell hit is brought in line with how shadow priest's Shadow Focus is handled:

https://github.com/BreakBB/ExtendedCharacterStats/blob/82027f987da9b025e4432525ec192ca02b2ccf1b/Modules/Data/SpellHit.lua#L53-L59

Fixes #321

RA80533 commented 4 months ago

It looks like 0770681 undid this exact change as a fix. This is likely because the talent in Classic Era only increases bonus spell hit for affliction spells. The fix was not applied to the other talents that increase bonus spell hit in the same way:

BreakBB commented 4 months ago

Hey @RA80533

Thanks for taking the time to look into this! However I am not sure if it is a good idea to re-add Suppression for Classic Era. I am not playing Era anymore, so will Warlocks always run this talent?

You are right about the other classes, though 🤔

BreakBB commented 1 month ago

As explained in #328 ECS will not take the Suppression talent into account because it only affects Affliction spells. Overall ECS in inconsistent about this, as similar talents are still taken into account though.