kakysha / HonorSpy

World of Warcraft: Classic HonorSpy addon
57 stars 50 forks source link

Honorspy randomly reset estimated honor #221

Closed Slivo-fr closed 1 year ago

Slivo-fr commented 2 years ago

In some undefined edge case, honorspy re-set the estimated honor to the actual blizzard "this week" value This can lead to large difference in real honor if it happens in the middle of a PVP night.

I have a specific case I can describe here that will probably not help solving it but should help understanding what I'm explaining. I had a player aiming to stack his honor that was going to take a plagueland tower : He had 106634 honor and had few HK during the day. When he gained the tower honor, his honor felt back to 106299 (blizz week value + tower honor, HK honor was lost)

Here is a screen of his character sheet after the tower

image

I can't find out how and why yet, still investigating

Slivo-fr commented 2 years ago

I've been able to debug a bit, here is what I found.

At HonorSpy:CheckNeedReset I had this print:

    local _, thisWeekHonor = GetPVPThisWeekStats()
    if (HonorSpy.db.char.original_honor ~= thisWeekHonor) or (HonorSpy.db.char.last_reset ~= must_reset_on) then
        print("HonorSpy:CheckNeedReset", "reset", skipUpdate, HonorSpy.db.char.estimated_honor)
        print(HonorSpy.db.char.original_honor, thisWeekHonor, HonorSpy.db.char.last_reset, must_reset_on)

And when it happened to me the output was (line 2 and 3)

[16:51:56] HonorSpy:CheckNeedReset skipUpdate nil 80759
[16:51:56] HonorSpy:CheckNeedReset reset nil 80759
[16:51:56] 49218 0 1656486000 1656486000
[16:51:56] HonorSpy:CheckNeedReset2 reset nil 0
[16:51:56] HonorSpy:CheckNeedReset skipUpdate nil 0

The player thisWeekHonor was 0 at the moment. Could the GetPVPThisWeekStats return a wrong value in some edge cases ? The point is I also had this debug print :

function HonorSpy:CheckNeedReset(skipUpdate)

    print("HonorSpy:CheckNeedReset", "skipUpdate", skipUpdate, HonorSpy.db.char.estimated_honor)

    if (not skipUpdate) then
        HonorSpy:UpdatePlayerData(function() HonorSpy:CheckNeedReset(true) end)
    end

And as you can see on the output above, HonorSpy:CheckNeedReset is only called once with skipupdate true, without a skipupdate nil first and I can't find any other call to HonorSpy:CheckNeedReset(true) in the code outside HonorSpy:CheckNeedReset function.

image

Slivo-fr commented 2 years ago

If GetPVPThisWeekStats is the culprit here, we can't control the faulty behavior. In this case I'm considering a timed call back (5-10s ?) to checkneedreset with a "doublecheck" boolean when the returned value is 0 so it does confirm a second time the actual value and doesn't reset on the first missreturn

Any opinion about this workaround ?

kakysha commented 2 years ago

We can try that, maybe you can test it for some time locally first and see if GetPVPThisWeekStats is the actual cause?

Slivo-fr commented 2 years ago

I'm definitely debugging more on my side before even putting up a merge request yeah I'll keep you updated

Slivo-fr commented 1 year ago

I unable to reproduce this issue for a long time now. It was quite common when I first opened this thread. My fix should be functionnal and ready but I didn't had the chance to test it properly because of that Maybe something changed on blizzard end or in the context somehow ?

I will keep my debug/test local version. Maybe I can open a draft merge request with my WIP fix and we let it be so it's there if it's ever needed later on ?

kakysha commented 1 year ago

Sure, let's keep it for history

Slivo-fr commented 1 year ago

Happened to a guildmate lately, so it's not fully gone