Ralgathor / LibClassicSwingTimerAPI

GNU General Public License v3.0
4 stars 5 forks source link

Fix druid weapon speeds when casting spells that unshift them #45

Closed Korbrawr closed 1 year ago

Korbrawr commented 1 year ago

Implements the proposed solution for #44 This waits for the spellcast event to resolve before calling unit:SwingStart, so an accurate swing speed and expiration time can be sent with SWING_TIMER_START after druids cast a spell which unshifts them.

Korbrawr commented 1 year ago

@Ralgathor I realize this PR may look naive, but this is the best solution I see. Every spell currently listed in reset_swing_spells will unshift a druid - however, if we were to add the spells which reset swing timer and don't unshift, I don't believe it would do a significant amount of harm to have the SwingStart be evaluated at the next frame.

Ralgathor commented 1 year ago

@Korbrawr I tested the case describe on your PR. The change it's deeper that expected.

When I have implementated the logic the game API was firing UNIT_ATTACK_SPEED events whener a druid is changing form. Regarding that I implemented a logic to ignore this UNIT_ATTACK_SPEED events in this case.

Following my test the game API it's no longer firing this UNIT_ATTACK_SPEED events when druid change form. That make this UNIT_ATTACK_SPEED ignoring logic obsolete but it impact the precise case you describe. I'm going to diging more into this to see if you proposed solution is the best to implement or if I can find a more abstract way to do do this.

Korbrawr commented 1 year ago

@Ralgathor Thank you for checking this. When I test this ingame, I do see UNIT_ATTACK_SPEED being fired when druids change forms - however, the arguments being passed to lib:UNIT_ATTACK_SPEED are slightly different than what's expected. The first argument passed is the event name "UNIT_ATTACK_SPEED", while the function expects the unitGUID. This leads to the function being unable to find a unit as seen here. Can't find unit

Upon checking the documentation for Frame:RegisterUnitEvent, when an event is triggered, the event name is always passed first, while the unitGUID will be the second argument. This aligns with what we're seeing right now.

Modifying the arguments to UNIT_ATTACK_SPEED seems to have fixed the issue without the need for any other change, I'll update the PR shortly.

Korbrawr commented 1 year ago

I had to make one small adjustment, if the swing timer is reset by casting a spell that reset it, we don't want to skip the next attack speed update, so I unset unit.skipNextAttackSpeedUpdate in that case

.An easy way to test the issues and the fix is to go into cat form, and then try casting Mark of the Wild immediately after melee'ing. In the previous version, you will see the cat form timer progress until the end of the bar and stay there for as long as it needs for the caster auto attack to go off. With the fix, you will see an accurate caster swing timer.

Ralgathor commented 1 year ago

@Korbrawr

Good catch ! Indeed the issue is link to the Frame:RegisterUnitEvent that pass the event name first. I surprised no one notify that earlier as it impact all UNIT_ATTACK_SPEED event for all class. Even me that use this lib for my Shaman swing timer...