awls99 / Random-Hearthstone-Toy-Continued

A continuation of Hemco's random Hearth toy
MIT License
0 stars 1 forks source link

Is there a reason the In Combat check is commented out? #9

Closed teelolws closed 1 year ago

teelolws commented 1 year ago

https://github.com/awls99/Random-Hearthstone-Toy-Continued/blob/main/RandomHearthToy.lua#L84

I suspect commenting out this line pre-dates you getting the code from the previous author? I don't know why the line is commented out, but having it commented out means the player will get an Lua error shortly after logging in if they login or /reload while in combat.

teelolws commented 1 year ago

Ok yeah it was Hemco. The addon was basically rewritten for v2.1: https://www.curseforge.com/wow/addons/random-hearthstone-toy/files/3154640 Compare to the previous version where the code was completely different: https://www.curseforge.com/wow/addons/random-hearthstone-toy/files/3141038

That line has been commented out since it was written, and I don't see a reason why. Should un-comment it, so that login error goes away.

awls99 commented 1 year ago

Good catch, thank you! There might be something to it, so I'll run some experiments when I'm off work later. Looking at the code I think it might have been that the author didn't want the function to do both things (check combat and update), but I'm speculating , there's a combat check in one of the places this function is used but not in the other, I suspect they thought people wouldn't get into combat within 10s of login in 🤔

teelolws commented 1 year ago

Yeah. I don't usually enter combat within 10s of login, but sometimes I already am in combat when I login. Following a disconnect, following a relog, etc.

awls99 commented 1 year ago

I couldn't get a notable difference while experimenting with or without that if commented out, here's what I ended up testing:

  1. auto attack dummies
  2. reload
  3. spam hs as soon as back from loading screen while still in combat
  4. note results
  5. stop combat
  6. spam hs button
  7. note results
  8. back in combat, attempt to HS
  9. note results

On both cases, 4 - nothing happened 7 - takes a second or so to hs to work

8 however had a difference code as is: hs works and changes if you cancel cast. adding the check: hs work, ICON changes, but actual hs being used stays the same

Also, I'm still not getting any lua errors in either cases, which I find odd.

Did you try it?

teelolws commented 1 year ago

Did you try it?

I've been uncommenting it to make the error go away for months. Have no had any problems.

Yeah it might use the wrong hearthstone to the icon, but... so what /shrug

awls99 commented 1 year ago

guess it might work for other people that complained about lua errors on curse forge, I'll push it.