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

Fix ravager axe not properly proccing deep wounds, weapon enchants et… #2162

Open
wants to merge 3 commits into
base: development
Choose a base branch
from

Conversation

scottnice
Copy link
Contributor

@scottnice scottnice commented Aug 14, 2023

🍰 Pullrequest

Fix ravager axe not properly proccing deep wounds, weapon enchants etc on bladestorm hits, fixed ravager axe issue where channeling would stop when target died, extra attacks generated while channeling will now be applied after channelling is complete, this matches behaviour of classic. Fix issue with ravager bladestorm where its whirlwind hits could not reproc blade storm.

Proof

Windfury procs during bladestorm are stored and released after channelling is complete:
https://www.wowhead.com/classic/item=7717/ravager#comments:id=3160508
https://www.youtube.com/watch?v=XEZpNdxlV1A

Issues

How2Test

Create a character with ravager on a shaman and a warrior. Test with windfury and sweeping strikes.
Ravager whirlwind procs also do not consume sweeping strikes matching the functionality in the linked youtube video from Classic SoM

Todo / Checklist

  • Fix bladestorm and sweeping strikes
  • Fix White space issues

@Wall-core
Copy link
Contributor

the spell_template data is taken directly from the client dbc and is blizzlike. If absolutely necessary, this would be handled under spell_effect_mod or spell_mod but would be better resolved in the core as it's best to avoid adding mods.

@scottnice
Copy link
Contributor Author

scottnice commented Aug 14, 2023

Thanks Wall, I'll update it to use the spell_mod table for now but I'll also look to see if there's a way to handle this in the core.

@scottnice scottnice marked this pull request as draft August 14, 2023 13:29
@scottnice
Copy link
Contributor Author

@Wall-core @ratkosrb it's possible the fields changed between versions. Because according to this commit 3f91713 at one point the bladestorm ability had the SPELL_ATTR_EX2_TRIGGERED_CAN_TRIGGER_PROC flag set, and this behavior has now moved over to the SPELL_ATTR_EX3_NOT_A_PROC flag which is not set on these spells. If the same migration in the code happened in the database one would expect the SPELL_ATTR_EX3_NOT_A_PROC flag to be set on the Bladestorm and its Whirlwind proc.

@ratkosrb
Copy link
Contributor

Both the attributes assigned to spells you see in spell_template, and the naming of attributes in the source code comes from Blizzard. None of these should be changed.

@scottnice scottnice force-pushed the fix-ravager-axe-Bug-1786 branch 3 times, most recently from 066497b to e80ab96 Compare August 15, 2023 02:16
@scottnice scottnice marked this pull request as ready for review August 15, 2023 02:17
…c on bladestorm hits, fixed ravager axe issue where channeling would stop when target died, extra attacks generated while channeling will now be applied after channelling is complete, this matches behaviour of classic. Fix issue with ravager bladestorm where its whirlwind hits could not reproc blade storm.
@scottnice
Copy link
Contributor Author

@Wall-core @ratkosrb Updated to use spell_mod table and made some small code changes to support proper functioning of Ravager without impacting other abilities.

@scottnice scottnice marked this pull request as draft August 15, 2023 12:54
@scottnice scottnice marked this pull request as ready for review August 16, 2023 04:53
@scottnice
Copy link
Contributor Author

@ratkosrb some of this code can be replaced with overrides via the spell mod table. Let me know which path you prefer and I can update the pull request accordingly.

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

Successfully merging this pull request may close these issues.

Ravager Axe🐛 [Bug]
3 participants