prevent errors from nil DR icons #11

2021-08-01T04:27:03+02:00

AlexFolland commented

2021-08-01 04:27:03 +02:00

(Migrated from github.com)

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.

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 commented

2021-08-02 14:43:00 +02:00

(Migrated from github.com)

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.

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 commented

2021-08-02 15:05:26 +02:00

(Migrated from github.com)

For reference, the DRData-1.0 Gladdy is using is from the authors (Shadowed) git repo from 2008:
9c0cae86d7
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.

For reference, the DRData-1.0 Gladdy is using is from the authors (Shadowed) git repo from 2008: https://github.com/Shadowed/DRData-1.0/commit/9c0cae86d751b18615b239c2aadc949e9f8c27c2 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 commented

2021-08-02 20:29:33 +02:00

(Migrated from github.com)

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?".

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](https://github.com/wardz/DRList-1.0). 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 commented

2021-08-02 20:32:37 +02:00

(Migrated from github.com)

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

XiconQoo commented

2021-09-15 10:32:22 +02:00

(Migrated from github.com)

Fixed

AlexFolland commented

2021-09-15 20:46:31 +02:00

(Migrated from github.com)

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.

I still see DRData which isn't maintained for classic instead of [DRList](https://github.com/wardz/DRList-1.0) 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 commented

2021-09-27 16:44:52 +02:00

(Migrated from github.com)

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.

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 commented

2021-09-29 18:26:23 +02:00

(Migrated from github.com)

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.

I asked the author of DRList if the claim that it's not maintained for TBC is substantiated, [here](https://github.com/wardz/DRList-1.0/issues/4). 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 commented

2021-09-29 21:26:57 +02:00

(Migrated from github.com)

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:

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)
Have one Acc with some alliance character parked outside of shat.
Create all the classes as horde on the remaining accounts.
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.

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.

Pull request closed

Please reopen this pull request to perform a merge.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: xiconfjs/Gladdy-TBC#11