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

Core/Spell: Fix Devastate + Glyph of Sunder armor stacking #14785

Closed
wants to merge 1 commit into from

Conversation

pjasicek
Copy link
Contributor

Fixes #14633

@jackpoz
Copy link
Member

jackpoz commented Nov 8, 2015

anyone would like to test the behavior with and without this PR and provide feedback ?

@ghost
Copy link

ghost commented Nov 8, 2015

@ikir83 should be one of the most interested in testing this, he opened issue #14633.
Just to point out some relevant info to those who may have forgotten about it:
http://wow.gamepedia.com/Devastate Level 26 Protection warrior ability (6.2.2)
http://wotlk.openwow.com/spell=20243 Devastate: Protection warrior level 50 ability (3.3.5)
http://wow.gamepedia.com/Glyph_of_Devastate Requires Level 50
http://wotlk.openwow.com/item=43415 Glyph of Devastate
http://wow.gamepedia.com/Sunder_Armor Protection Warrior Level 10 ability
http://wotlk.openwow.com/spell=7386/sunder-armor/#see-also-ability
http://wow.gamepedia.com/Glyph_of_Sunder_Armor Requires Level 25 (4.1.0)
http://wotlk.openwow.com/item=43427/glyph-of-sunder-armor Requires Level 15(3.3.5)

@jackpoz jackpoz self-assigned this Dec 17, 2015
@jackpoz
Copy link
Member

jackpoz commented Jan 3, 2016

2 months and no feedback

@ghost
Copy link

ghost commented Jan 3, 2016

I would have tried to test it, but I suck at testing player classes I don't know very well from playing online.
Unfortunately, there seems to be no activity from the OP since he posted this PR and @ikir83 has not been active since 2015-09-14 (he would stand the most to gain from testing it).

@jackpoz
Copy link
Member

jackpoz commented Jan 4, 2016

@pjasicek could you explain more in detail your PR ? The description "Fixes #14633" doesn't really explain anything and it's unclear why the previous "cast the spell only if target doesn't have the aura and then modify the number of stacks" isn't good anymore.
53a7a7b was the previous change to that code.

@jackpoz
Copy link
Member

jackpoz commented Jan 5, 2016

Please ask a TrinityCore member to reopen this Pull Request once you provide the requested feedback. There is no need to open a new Pull Request with the same changes.

Opening a Pull Request without a clear description about all the changes doesn't really help us reviewing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants