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
fix(Scripts/Spell) Rogue glyph of backstab (port from TrinityCore) #18846
base: master
Are you sure you want to change the base?
Conversation
Greetings. Thanks for the input. If you would be so kind as to please complete the pull request with the necessary data to be able to carry out the tests, the people who do it will be able to know what they have to test and the results they have to obtain from said tests. Thank you so much. |
add test case |
Please use the new spell registration method (you can check examples in that file) |
0.03f, // 3 points: ${($m1 + $b1*3 + 0.03 * $AP) * 6} damage over 12 secs | ||
0.03428571f, // 4 points: ${($m1 + $b1*4 + 0.03428571 * $AP) * 7} damage over 14 secs | ||
0.0375f // 5 points: ${($m1 + $b1*5 + 0.0375 * $AP) * 8} damage over 16 secs | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each call CalculateAmount creates an attackpowerPerCombo array. It's always the same. I would make it static
|
||
if (countMin < countMax) | ||
{ | ||
bonusDuration += 2000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is very controversial to modify a variable of another class by reference. It's just int. There will be no performance gain from such tricks.
new spell_rog_rupture(); | ||
new spell_rog_glyph_of_backstab_triggered(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont think you need to use old registry here. use RegisterSpellScript, or if they are tied together with an aurascript use RegisterSpellAndAuraScriptPair()
Changes Proposed:
This PR proposes changes to:
Issues Addressed:
SOURCE:
The changes have been validated through:
Tests Performed:
This PR has been:
How to Test the Changes:
Current Behaviour
Currently Glyph of Backstab makes the Rupture doesn't deal damage last 6 seconds before DOT debuff end.
Expected Blizzlike Behaviour
Glyph upgrades Backstab to prolongs DOT debuff of Rupture per 2 seconds each time Backstab used (up to 6 seconds).
Steps to reproduce the problem:
/stopattack
to game chat and observe the DOT ticks of Rupture (should deal damage every 2 seconds);Known Issues and TODO List:
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.
This fix made with @tagrim