Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

prevent errors from nil DR icons #11

Closed
wants to merge 2 commits into from

Conversation

AlexFolland
Copy link

Gladdy didn't work at all for me, and it was due to this error-prone code. I've fixed it in my fork here and this pull request should help fix it upstream.

I honestly don't understand how that code could have worked for anyone else in TBC Classic, since the entire addon couldn't initialize without this fix.

Thanks for the addon.

@XiconQoo
Copy link
Owner

XiconQoo commented Aug 2, 2021

Thanks for your PR. I will consider it with more information.

I assume some other of your addons uses the DRData lib and most possibly a wrong version that is not suited for TBC at all.
This would also affect the whole Diminishing module and display wrong DRs!

nil icons would be the result since there would be spells in the DRData lib, which don't exist in TBC.
Thus select(3, GetSpellInfo(k)) would return nil...

Can you check your addons and tell me, which one uses DRData?

Gladdy standalone loads just fine without your fix.

@XiconQoo
Copy link
Owner

XiconQoo commented Aug 2, 2021

For reference, the DRData-1.0 Gladdy is using is from the authors (Shadowed) git repo from 2008:
Shadowed/DRData-1.0@9c0cae8
That is nowhere to be found on curseforge or wowace as a dowload option. The earliest version there is for WotLK - rev 1001. I am using revision 793.

That alone is a big red flag that some other addon loads a later version of DRData-1.0, which it most possibly shouldn't.

@AlexFolland
Copy link
Author

AlexFolland commented Aug 2, 2021

I did have a "DRData-1.0" folder installed directly with timestamp 2020-06-06, I guess for something else that needed it but didn't include it. I've deleted that folder and now I get no prints from Gladdy from my changes and the addon works as expected.

That being said, while grepping for "DRData" in my addons folder, I found a change note from TellMeWhen showing that the author of that addon switched from "DRData" to "DRList", which is a fork of DRData that is being actively maintained. I suggest switching Gladdy to use DRList.

As for this pull request, I still think personally that it's better to prevent the whole addon from breaking mysteriously, but I admit that even these new prints don't explain it clearly. I have an idea. Maybe I can improve the prints to say "Do you have a conflicting DRData copy being loaded?".

@AlexFolland
Copy link
Author

I've pushed a little change that should improve the error message, to help anyone who's seeing these prints.

@XiconQoo
Copy link
Owner

Fixed

@XiconQoo XiconQoo closed this Sep 15, 2021
@AlexFolland
Copy link
Author

I still see DRData which isn't maintained for classic instead of DRList which is maintained. I realize this pull request doesn't change that, but I was thinking it had served as a reminder to switch to DRList.

@XiconQoo
Copy link
Owner

DRList might be maintained for Retail but not for TBC I am afraid. There are some categories just plain wrong. So I maintain DRData myself without the interference from outside.
I renamed the LibStub init. There won't be anymore conflicts with any DRData out there anymore.

@AlexFolland
Copy link
Author

I asked the author of DRList if the claim that it's not maintained for TBC is substantiated, here. The author of DRList explained that it is still maintained for TBC and planned for WotLK as well. Here's their exact response:

The addon is still maintained for Classic, TBC, Retail and eventually WotLK. If you find a missing spell or incorrect category feel free to submit an issue ticket or a pull request and I'll fix it asap. If nothing gets reported, nothing gets fixed. (There's so many spells, its not possible for me to manually test every single spell for every single expansion & class. So any issue tickets are extremely helpful)

Some special DR categories where the ability is supposed to only DR with itself and nothing else, such as unstable_affliction, death_coil, freezing_trap, scatter_shot, mind_control are still included but unconfirmed in the lib. I haven't been able to test these in TBC sadly, and there's no user reports available. The rest of the DR categories works fine in TBC.

It's best to not duplicate work on libraries with multiple forks. Why not work together and make sure DRList is working well instead of maintaining your own fork of DRData? Several other addons use DRList anyway.

@XiconQoo
Copy link
Owner

XiconQoo commented Sep 29, 2021

Allright. Good point.

I can already confirm, freezetrap is on the same DR as Sap and Polymorph. Repentance as well. This is different from the private servers, where freezetrap and repentance had their own respective DR category.

Take a look here: https://silentshadows.net/tbc-diminishing-returns-list/

Since you apparently play the game I need you to confirm the DR-categories. This can only be done by testing. Here is how I would do it:

  1. You and a buddy having 2-3 PTR accounts each or you having 4-6. (Multiple PTR-accounts can be easily created with one B-Net account)
  2. Have one Acc with some alliance character parked outside of shat.
  3. Create all the classes as horde on the remaining accounts.
  4. Test all the combinations of DR-Categories and spells on DRList/DRData with the remaining accounts.

Alternatively if you want to test that within the same faction, you could just go to nagrand arena or blades edges arena. Everyone not in the same group is hostile.

I can't do it. First, I don't play the game. Second, I don't have the time to test all combinations by myself...

As the author of DRList mentioned: We are dependant on reports on wrong stuff and actual input from testing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants