kakysha / HonorSpy

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

Issue with latest release - column values #171

Closed HolteUnderscore closed 2 years ago

HolteUnderscore commented 2 years ago

I think your formula is a bit awry on the latest update pushed yesterday. The EstTodayHonor column contains a value that is adding what you actually got today to the known honour. Thus when it calculates for EstWeekHonor it looks like you have twice as much as you do have.

teelolws commented 2 years ago

If you patched the addon midweek there might be some inconsistencies until the next reset. Let me know if its still wrong after doing some pvp after the reset.

teelolws commented 2 years ago

Okay I think it wasn't resetting properly for people in a specific situation: people who hadn't patched the addon before reset today, and patched the addon after reset today before logging in. Unfortunate timing. I've fixed it in this: https://github.com/kakysha/HonorSpy/pull/172/commits/03cf4c32eabbc352e15972c77a241c385848c6ee but I can't backdate it to fix for anyone who already patched and logged in today. Also need @kakysha to accept the PR!

Slivo-fr commented 2 years ago

I got something really looking like this issue on latest update today

image

As you can see, the estWeekHonor is just the estToday doubled

Slivo-fr commented 2 years ago

This guy has a "failed blizzard update" hence the estToday but the value is still doubled

image

teelolws commented 2 years ago

That guy is either running an outdated version or he updated the addon halfway through the week. There isn't much I can do about someone updating at weird times.

Slivo-fr commented 2 years ago

I had him install it yesterday so it would reset properly, isn't this enough ?

teelolws commented 2 years ago

It'll be right next week. OR you have the EU issue. From the front page:

Reset Day Is Wrong: I'm playing on EU server, but HonorSpy resets on Tuesday instead of Wednesday: its because your client thinks you are in US. Check your WoW Classic Folder > WTF > Config file, it should have SET portal "EU", not "US".

Slivo-fr commented 2 years ago

I don't think the reset day is wrong, it's just blizz not updating honor/rank as it often happens on Wednesday! This is why there is still honor registered this week. This is not the issue however.

It'll be right next week

Does it means that new user may have doubled value on their first week of use ?

teelolws commented 2 years ago

Does it means that new user may have doubled value on their first week of use ?

Its not a "new user" problem. Your friend has used HonorSpy before. Maybe a long time ago.

Slivo-fr commented 2 years ago

Allright !

Slivo-fr commented 2 years ago

I still have the issue this week :/

image

EDIT : And my config.wtf is fine

teelolws commented 2 years ago

Did you log into that character last week after updating the addon?

Slivo-fr commented 2 years ago

Yes! I installed the addon last week, before the previous week reset on wednesday.

Installed on march 1st, reset was on march 2nd, played on the 2nd to 8th of march week, new reset was yesterday on march 9th

Slivo-fr commented 2 years ago

This seems to display proper values : https://github.com/Slivo-fr/HonorSpy/commit/0d7d24316d13d526ab14d6b91b91f31c06bc6493

image

I can pull request if you wish so

teelolws commented 2 years ago

You've changed GUI.lua (which I hadn't touched). I'll need to check that your implementation is consistent with how original_honor and estimated_honor are intended to be used. What does the Honor tab look like with your changes?

Slivo-fr commented 2 years ago

No changes, I only touched the main window table display part. Honor tab shouldn't be affected

teelolws commented 2 years ago

The problem exists in the Honor tab. It says This Week Honor 1750 (1750) when it should say This Week Honor 1750 (0).

Slivo-fr commented 2 years ago

I don't think so. As far as I understand the code, it's intended for the week honor to include the blizzard week values and the estimated honor of the day.

teelolws commented 2 years ago

No. 1750 is what Blizzard has calculated you have already earned before today, this week. If you haven't done any PVP today then it should be 0 in brackets.

Slivo-fr commented 2 years ago

I disagree with that.

Today should be zero, but the week value should equal the blizz value at daily reset. Then when you start gaining honor, it increase both today and this week value.

Maybe @kakysha could help us there

teelolws commented 2 years ago

Er yeah something is definitely wrong here. 218 was the estimated honor yesterday, which has been corrected to 204 by Blizzard.

image

teelolws commented 2 years ago

Okay, I agree with your changes in https://github.com/Slivo-fr/HonorSpy/commit/0d7d24316d13d526ab14d6b91b91f31c06bc6493

estHonor and thisWeekHonor do not function the way I expected them. They're different to orignal_honor and estimated_honor

Go ahead and PR it.

Slivo-fr commented 2 years ago

Done ! :)

I think I'll have to handle conflicts after first merge

teelolws commented 2 years ago

Should just make them the same PR.

Slivo-fr commented 2 years ago

Would avoid the conflicts but that's two separate issues, I prefer each having their own merge request ! I'll be quick with the conflict so we don't waste time :)

Slivo-fr commented 2 years ago

This does raise a question however, do we really need the EstTodayHonor column in the GUI ? Because estWeekHonor will now includes it and should be enough itself imo You would have known honor for the blizz value and EstWeekHonor for the player estimated value of the week, today estimated just being the week estimation minus blizz value

Slivo-fr commented 2 years ago

I does look like there is still something a bit odd somewhere and I get estimated values that are smaller than the blizz value

image

Slivo-fr commented 2 years ago

I look like it happens when the blizz value gets updated without the estimation being updated. I'd say it's not a GUI issue, I could however set it to display 0 if it's negative.

What do you think ?

Edit : did that in the #190

teelolws commented 2 years ago

Yeah it'll be when you've inspected that player, but haven't gotten an estimation update from them. Probably because they turned HonorSpy off.

if (tonumber(estHonor) ~= nil) then and if (tonumber(estHonor)) then is the same code? I also might have to fix the sorting function

Slivo-fr commented 2 years ago

if (tonumber(estHonor) ~= nil) then and if (tonumber(estHonor)) then is the same code?

Kinda, but the nil check is more clean imo