TBC: Swing Timer megathread ticket
TheSorm opened this issue · 34 comments
Description
To get this started, we discussed some issues in: #3205 allready.
I have now found the genrall bug inside the swing timer that caused problems that made people belive that some haste changes are snapshoted at the beginn of a swing.
Original Version WeakAuras/GenericTrigger.lua 1593:
if lastSwingMain then
if mainSpeedNew ~= mainSpeed then
timer:CancelTimer(mainTimer)
local multiplier = mainSpeedNew / mainSpeed
local timeLeft = (lastSwingMain + swingDurationMain - GetTime()) * multiplier
swingDurationMain = mainSpeedNew
mainTimer = timer:ScheduleTimerFixed(swingEnd, timeLeft, "main")
end
end
Fixed Version:
if lastSwingMain then
if mainSpeedNew ~= mainSpeed then
timer:CancelTimer(mainTimer)
local multiplier = mainSpeedNew / mainSpeed
local timeLeft = (lastSwingMain + swingDurationMain - GetTime()) * multiplier
swingDurationMain = mainSpeedNew
mainSwingOffset = (lastSwingMain + swingDurationMain) - (GetTime() + timeLeft)
mainTimer = timer:ScheduleTimerFixed(swingEnd, timeLeft, "main")
end
end
That dose also mean that this WeakAuras/GenericTrigger.lua 1661:
if not WeakAuras.IsBCC() then
swingTimerFrame:RegisterUnitEvent("UNIT_ATTACK_SPEED", "player");
end
was not a propper solution and needs to be changed back to:
swingTimerFrame:RegisterUnitEvent("UNIT_ATTACK_SPEED", "player");
To boil down why the swing timer was not working correctly under attack speed changes mid swing:
The code to set the ScheduleTimerFixed was working correctly. It calculated the right time when the timer should expire.
E.g the swing was at 30% before the attack speed changed, the swing is also at 30% after the attack speed changes.
The error results from what the WeakAuras.GetSwingTimerInfo returns.
It return the swing duration, the time the next swing will land and some other stuff.
Now the problem is that the time that the next swing hits is calculated by lastSwingMain + swingDurationMain - mainSwingOffset while mainSwingOffset is 0, that leads to a wrong calculation of when the next swing should hit. Because its calculating it with the attack speed that was modified during the swing as if the new attack speed was allready present at the begin of the swing.
Because of that, using mainSwingOffset to correct the time the next swing lands works.
If more information is needed or you want more evedince i can give that to you but that will take some time.
But what i stated is an obvious bug and i showed how to fix it.
So all in all what has to be done is reenabling the UNIT_ATTACK_SPEED event for BC, fixe the bug, look into the off hand and see if there is something simelar.
CATION
My fix is more of a prove of concept because when you parry a mob in the same swing as your attack speed changes, mainSwingOffset is overwritten which can lead to problems, so there is probably a better fix to this issue. Probably it is better to change how WeakAuras.GetSwingTimerInfo(hand) gets/calculates the time of the next swing.
Example
Abacus of Violent Odds before the fix:
https://streamable.com/bv4pw8
Abacus of Violent Odds after the fix:
https://streamable.com/pvgz6m
WeakAuras Version
WeakAuras 3.5 and 3.6
World of Warcraft Flavor
The Burning Crusade
Tested with only WeakAuras
- Yes
- No
Lua Error
No response
Reproduction Steps
- Use anything (except Seal of the Crusader) to change your attack speed mid swing
- Look at how the Swing timer will either be done to early or to late
Last Good Version
No response
Screenshots
No response
Export String
No response
Thanks for taking the time to investigate the issues.
I encourage you to open a pull request with the fixes you have, you can find information on how to get started at https://github.com/WeakAuras/WeakAuras2/blob/main/CONTRIBUTING.md if you need more assistance feel free to join Weakaura's discord and request the alpha role in the bot channel for dev channels.
I just applied what TheSorm posted here to "GenericTrigger" and so far those changes look good, i tested Bloodlust and Divine Shield - swingtimer works fine!
As i said, that it will probabyl take a while until i get time to do so, but yes when i get the time I will do it.
This change seems to introduce a bug into TBC wherein a Raptor Strike registered immediately after a ranged Auto Shot does not start the melee swing timer. If the hunter waits for their auto to be ready and then Raptor Strikes, the melee swing timer is started as expected.
I cannot reproduce with WA versions before this change.
This change seems to introduce a bug into TBC wherein a Raptor Strike registered immediately after a ranged Auto Shot does not start the melee swing timer. If the hunter waits for their auto to be ready and then Raptor Strikes, the melee swing timer is started as expected.
I cannot reproduce with WA versions before this change.
This is too hard for me to reproduce your issue, i need you to test with the last version of weakauras, test if you can reproduce, then test again after removing those 2 lines
like that
After that, i need another test with this change :
-- check next frame
swingTimerFrame:SetScript("OnUpdate", function(self, elapsed)
if isAttacking then
swingStart("main")
swingTriggerUpdate()
end
self:SetScript("OnUpdate", nil)
end)
@TheSorm @Bakamaka11 @Jovarask can you test this #3255 ?
Nice that you are working on the seal issue :) The problem with the current fix you made is, if I activate SotC and some attack speed changing effect in the same swing, it will not be shown correctly. I think the best fix would be to track SotC as you do but instead of using it to make the hole swing being "snapshoted" one could calculate the right value of UnitAttackSpeed. Meaning as long as SotC is active and no swing happend yet, UnitAttackSpeed must be decided by the right amount to calculate the real value. I think a good idea would be to write a wrapper for UnitAttackSpeed that is used instead of the real function that is allways returning the real attack speed
Should i remove the commit and wait for your fix or leave it as it is and wait for your fix ?
I guess, as long as it is working like that, it should be fine to ceep it. I mean getting the general fix out for now (even tho there are still things we don't really know why they don't work) should be priority :) to fix all the rest we would probably need a lot of data and as long as no one wants to create it, nothing furser will be done
Update
9bee769 fixed the attack speed update issue
Seal of the Crusader snapshot special case didn't make it, there is no nice solution, for a proper fix we need a proper API, i suggest you to ask nicely Blizzard's dev to improve their API with swing timer support.
Reminder for @caccavale, we need your help if you want the raptor strike issue fixed
Note, wtlk-classic may force Blizzard to clean up the behavior when they tie ranged and melee swings together.
It also may very well make it worse.
IMO, #3178 is weird and obfuscates your actual swing state so that someone could avoid putting an and target exists trigger in their WAs. Like MrBuds said people would say, I think it should be reverted.
Sorry for not responding; I may take a stab at writing a work around.
So it requires entering a dungeon to test? Can you post logs + videos of it happening?
Sword spec from Arms Warriors can still mess with their swing timer.
What I've found is that I cant make it happen on the ptr vs dummies but on both versions if I enter a dungeon I can reliably make it happen. For the issue to occur the sword spec has to proc off a MS or WW used directly after a slam. Just hamstringing the target or something on its own wouldn't trigger it.
It's possible that the mob needs to survive the MS or WW for it to happen but not 100% sure on that one. I'd recommend dungeons like scholo or strat if anyone wants to try to reproduce it just to make sure.
I don't know what exactly it requires to happen. I just know that when I was on Shattrah on the PTR it was working fine but when I tested on live while farming Dark Runes in Scholo it would happen consistently. Initially I tried to enter scholo on the PTR to test in the same scenario but didn't have the key so I went to Stratholme instead and that made it so the early resets would happen.
I never recall having it happen to me when I was practicing slam against the immortal guys in Blasted Lands, but I cant say for certain. I would just recommend testing in a dungeon against mobs you wont kill with the attacks instantly because that was the method I found. Spent the rest of my time trying to figure out if it was connected to an addon or anything in my WoW folder.
Here's a video of it happening. The blue swing timer above my health is the addon WeaponSwingTimer which doesn't have the issue. I've reproduced this issue without any other addons installed and with a fresh installation of WoW.
@mrbuds
I can reproduce the raptor strike not triggering main hand timer. In TBCC, typically hunters set CVar autoRangedCombat to 0 for melee weaving. So when I casted an auto shot and move into melee range, it's not gonna toggle on melee auto attacks. Now when I hit raptor strike, it got casted instantly at the moment isAttack is probably false.
I have tested and can confirm both changes in this comment works for me.
Indeed, but we added the isAttacking check for a reason: #3178, thus we can't remove that.
You might want to verify by adding a print to the UNIT_SPELLCAST_SUCCEEDED branch which outputs isAttacking.
We should get a PLAYER_ENTER_COMBAT event when the auto-attack toggles on. I think the solution might be adding a swingTimerStart to that event. Unless someone can think of a reason why that wouldn't work.
@TheSorm That addon has an incompatible license, so we can't look at the code. Nor are we interested in investing even more time into the swing timer code.
The hunter issue should be fixed with the change buds has done. The paladin issue is essentially unfixable.
Apart from that there are no confirmed bugs with a good description.
We should get a PLAYER_ENTER_COMBAT event when the auto-attack toggles on.
When i switch target in the middle of a swing, both PLAYER_LEAVE_COMBAT & PLAYER_ENTER_COMBAT trigger on same frame, if i target my previous target before end of previously calculated swing (which triggers both events a second time) the swing hit as it was calculated. So starting swing on PLAYER_ENTER_COMBAT doesn't work here.
Maybe everyone working on this should check out https://github.com/watchyoursixx/WeaponSwingTimer-SixxFix
I heared that a lot of things are working correctly there so maybe check out the code or get in touch with the dev
Well, that sounds indeed like another case of blizzard making it impossible to figure out. We should simply add a warning to the swing timer noting that it's impossible to do correctly.
What more info do you need on the warrior slam bug? I can try different things out if you want.
I allready explained how the paladine issue is fixable #3234 (comment) . The problem is currently that finding the remainding issues (And there are some) is super hard, would require collecting a lot of data from a lot of szenarios.
We have investigated the paladin issue, and it's not something we will fix.
The api simply gives out wrong information. It' a bug in World of Warcraft, which we choose to not work-around. We don't work-around it, because it's impossible to do so in a reasonable amount of code. If you disagree, try writing the code necessary to do it.
In the long run there are a few scenarios for this trigger
- We leave it in "maintenance mode" (current situation), contribution are still welcome but no huge change.
- An external lib that replicate what a swing timer API should do is built and is shared by multiple addons (like https://github.com/rgd87/LibClassicDurations and https://github.com/rgd87/LibClassicCasterino by @rgd87 and a few contributors) and WeakAuras swing timer internal code is changed to use it. I don't know if that is something that can be of interest for @watchyoursixx
- Blizzard decide to implement a swing timer API, a ticket was made at their intention at Stanzilla/WoWUIBugs#161
- We disable the trigger (may happen for wtlk-classic)
Hey @mrbuds we did some more testing.
Turns out the swing timer is wrong whenever for example you drop more than one haste effect in one swing. (There are propably more cases when the current methode fails)
Currently:
local multiplier = mainSpeedNew / mainSpeed
local timeLeft = (lastSwingMain + swingDurationMain - now) * multiplier
swingDurationMain = mainSpeedNew
We need something like that:
local multiplier = mainSpeed / mainSpeedNew
local timeLeft = (timeOfNextAutoAttack - now) * multiplier
timeOfNextAutoAttack = now + timeLeft;
(Requires saving the timeOfNextAutoAttack globaly)
The code is not tested or anything, but the problem should be easely understandable looking at the 2 snippets.
The first sniped fails when two haste buffs are removed in the same swing, it fails on removal of the second buff.
To test the error and a potential fix, best is to use DST on the PTR, when it procs, pop abacus and sinse they have the same duration they should fall of in the same swing
Nope that is wrong:
swingDurationMain = mainSpeedNew
So for example if you are dropping haste 2 times, the swingDurationMain will not be calculated correctly sinse its just set to the new attack speed.
You can also just test what i explained, even if (as i also stated) my code is not the 100% solution, the problem is still there :)
all you changed is the multiplier.
timeOfNextAutoAttack IS the same as lastwing+duration
PR welcome
Haha yea will probably not happen, we stumbled over this while working on our ret sim for tbc, jus thought you guys maybe want to know about this ^^
all you changed is the multiplier. timeOfNextAutoAttack IS the same as lastwing+duration
While that intuitively makes sense, it is actually incorrect. Think about it - last swing is a static value as soon as it is set, so if your attack speed gets modified twice in one swing (increase or decrease both work) one of the modifications is partially ignored, the time span for which it was active essentially does not get tracked.
Time of next swing will be equal to last swing + duration for if your attack speed gets modified up to one time during that swing, no more. Meanwhile, if you track the time of next swing individually, you can assess this issue. That would involve setting the time of next swing equal to now + unitattackspeed("player") when the bar gets created, and then modifying it with the code that TheSorm linked when your attack speed is modified (the UNIT_ATTACK_SPEED event I suppose).
Testing on the PTR, the current implementation was consistently 100-180 ms off the actual swing time. The code provided reduced that to 30-40 ms, which is within the margin of error given batching and rounding errors. You can test it for yourself by outputting values in the swingTimerCheck function of GenericTrigger.lua and comparing them to output values of your swings' timestamps
As mentioned earlier, no one in the team is interested in working on this anymore, @TheSorm doesn't seem to want to contribute, but if you do @tasosgretsistas, when you will have a tested and working solution, this link will guide you on how to make a pull request (PR) to get that fixed for everyone https://github.com/WeakAuras/WeakAuras2/blob/main/CONTRIBUTING.md