rgd87 / LibClassicDurations

Provides duration expiration time for common auras via combat log
32 stars 14 forks source link

Commit „Fixed tbc missing spell error“ breaks the lib on classic #75

Open Nils89 opened 3 years ago

Nils89 commented 3 years ago

With the commit "Fixed tbc missing spell error" the lib always returns nil for durations

d87 commented 3 years ago

Classic what? BC or Vanilla? If BC then the whole lib is disabled there, because you don't need it. And if on vanilla then i don't see how https://github.com/rgd87/LibClassicDurations/compare/1.64...master any of these changes could break anything And I don't want to resub just to find out for sure

Nils89 commented 3 years ago

I mean vanilla. Strange is that I have embedded version 64 and Weak Auras has now with there latest update 66. And I do not get any duration back. Only if I raise the 64 to 67 anything is working fine

mrbuds commented 3 years ago

I can assist if something needs to be fixed but so far no one gave a good step by step example to reproduce their issue, and the lib is working perfectly fine in weakauras using version 66 on classic

Nils89 commented 3 years ago

Have some more people reported that issue?

Current state: Gw2 uses version 64 which works fine and we got the timers.

But with the latest WA and version 66 we got no timers from the lib.

So in that case throbbing from WA is loaded (higher number)

cont1nuity commented 3 years ago

And I don't want to resub just to find out for sure Any way I could support the investigation?

From my tests:

Using the following atm:


local LCD = LibStub:GetLibrary("LibClassicDurations", true)
if LCD then
    LCD:Register(Plater)
    LCD.RegisterCallback(Plater, "UNIT_BUFF", function(event, unit)end)
    UnitAura = LCD.UnitAuraWithBuffs
end
Nils89 commented 3 years ago

Same for gw2

d87 commented 3 years ago

jfc, I guess i'll just rollback to 64. Now that WOW_PROJECT_BURNING_CRUSADE_CLASSIC is here lib is going to be disabled in BC regardless of the changes i made at the start of beta EDIT: Done

mrbuds commented 3 years ago

Thanks for the rollback but we will need an increase of the minor version to be > 66 to make sure this is the loaded version of the lib, then we can publish a new release of weakauras

cont1nuity commented 3 years ago

Increasing the minor version actually triggers the error. I copied v64 to WA libs and did a version increase there -> issue is back.

d87 commented 3 years ago

Nothing was broken in 66, it was just disabling itself in BC. The issue most likely stems from reference to library and its functions being obtained during load time by GW2/Plater, instead of PLAYER_LOGIN when the highest version has been loaded overwriting the embedded ones. And these functions maybe had some closures to locals in them, idk. But if it's such an issue then how come it never came up before after updates.

Nils89 commented 3 years ago

Can you try version 70?

In gw2 we set the Version local to 70 and this works fine

cont1nuity commented 3 years ago

Can you try version 70?

In gw2 we set the Version local to 70 and this works fine

The number is not the issue. It is purely about local references to the LCD calls, e.g. UnitAuraWithBuffs. The old local references are no longer working when a newer version is loaded.

cont1nuity commented 3 years ago

Maybe something like this to keep the wrapper in tact could help?

if not lib.UnitAuraWrapper then
    function lib.UnitAuraWrapper(unit, ...)
        return lib.FillInDuration(unit, UnitAura(unit, ...))
    end
end

Edit: This seems to fix the issue, but requires a new version rollout on all affected addons.

d87 commented 3 years ago

Short term solutions: a) WA goes back to 64 and nothing gets overwritten anymore, all good b) You initialize the lib after all addons are loaded

Long term i'll investigate this and especially if i'll be doing updates. But the future of the lib is hazy. If blizzard are going to use single client for both classic BC and Vanilla, there's a chance they'll just lift the restriction and go with TBC UnitAura

cont1nuity commented 3 years ago

The following appears to work with the scenario as well:

if not lib.UnitAuraWithBuffs then
    function lib.UnitAuraWithBuffs(unit, index, filter)
        return lib.UnitAuraDirect(unit, index, filter)
    end
end
cont1nuity commented 3 years ago

Short term solutions: a) WA goes back to 64 and nothing gets overwritten anymore, all good b) You initialize the lib after all addons are loaded

I could live with this. Requires changes in all related addons, as far as I can tell: GW2_UI, Plater and WA.

Long term i'll investigate this and especially if i'll be doing updates. But the future of the lib is hazy. If blizzard are going to use single client for both classic BC and Vanilla, there's a chance they'll just lift the restriction and go with TBC UnitAura

Yes, I agree... Makes motivation to fix things even lower.

d87 commented 3 years ago

Made a proper fix, turns out the issue was only present when you had either standalone LCD or PallyPower installed. Also initializing during load time should be fine now

Xruptor commented 3 years ago

Thanks for keeping this addon updated! Both my addons xanDebuffTimers and xanBuffTimers utilize this wonderful library! Having it around is a godsend for the classic servers to track buff/debuff durations since it's not built in. Have been using this library since it was committed. Thanks a ton!