EsreverWoW / LibThreatClassic

A library for multi-target threat tracking in WoW Classic (1.13.2).
GNU Lesser General Public License v2.1
8 stars 9 forks source link

Lib uses improper syncing in BGs #21

Open MysticalOS opened 4 years ago

MysticalOS commented 4 years ago
function ThreatLib:GroupDistribution()
    if GetNumGroupMembers() > 0 and IsInRaid() then
        return "RAID"
    else
        return "PARTY"
    end
end

is flawed, it should be

function ThreatLib:GroupDistribution()
    if IsInGroup(2) then
        return "INSTANCE_CHAT"
    elseif GetNumGroupMembers() > 0 and IsInRaid() then
        return "RAID"
    else
        return "PARTY"
    end
end

This will repair this lib causing "not in a raid" errors spamming screen when in a BG with any addon using this lib

MysticalOS commented 4 years ago

Unil this is fixed, I actually removed lib from DBM to avoid it affecting users experience in BGs.

I don't want to use the forked version someone else posted either because that kind of fragmentation is not good for project. especially a new name new sync channel. the best classic threat mod is by same author (es) so the broken lib stays in circulation always, you have two libs in some cases installed on peoples computers.

the only proper way to fix this and other issues, is someone who's active to finda way to reach es and get added to this repo, or at very least an official fork continues using same name and sycnc channel, with a higher version, so the higher version overwrites discontinued one.

That's the only way forward, this lib updated, or replacement overwrites it, doesn't exist a long side it.

dfherr commented 4 years ago

@MysticalOS es is unrechable and inactive for 3 months.

This addon has a completely broken LibStub implementation: https://github.com/EsreverWoW/LibThreatClassic/blob/master/ThreatClassic-1.0.lua#L59-L77

It doesn't pass it's minor version to LibStub (LibStub.minors[MINOR] = MINOR should be LibStub.minors[MAJOR] = MINOR) and ignores already loaded versions and just recreates itself over it. So as long as this version is in circulation with the same namespace, the actual used version probably depends on loading order.

I not only started https://github.com/dfherr/LibThreatClassic2 , but also updated the threat meter https://github.com/dfherr/ClassicThreatMeter2

Imho only good way forward is letting this die completely and switching to a new namespace and communication channel. Especially if you update in DBM and Details updates too, this will be widespread pretty fast.

MysticalOS commented 4 years ago

sadly fan updates are not permitted on curse, classic threat meter 2 can't go to curse. and most I know use that over details, i guess it varies guild to guild, whichever one people adopted first.

which is why i felt backwards compat was essencial. so I'm still kind fo waiting to see what happens, especially since both you and cannon planned to do forks, unless you're both planning to contribute to that one?

dfherr commented 4 years ago

@cannonpalms forked, but didn't do a single code change in 20 days, so I got tired of waiting and took over. I'm happy to accept any help on my repo tho, already merged a PR and fixed nearly all of the issues mentioned here. I'm just having a hard time with the broken NPC modules right now (for fixing ony and ragnaros) and the apparent issue that the classic SPELL_CAST unfiltered combat event always returns 0 as a spellid (how did you fix that in DMB?).

I'm planning some substantial updates to the stand-alone threadmeter, so maybe I can get it accepted as a new addon using the lib.

I'll add your distribution channel suggestions tomorrow. I'm currently in the middle of the npc module work.

I would have kept this backward compatible, but the broken LibStub implementation and no check on always using the newest available versions kind of makes this pointless, especially because this is packed into multiple other addons and has a ton of bugs.

MysticalOS commented 4 years ago

there is no way to get spellID from a casting unit, period. blizzard didn't want spell ranks detectable. Basically DBM and most mods just check the bit band or GUID to determine if it's caster they want, but only spell Name for what's being cast.

if you get the thread lib working without all the unnessesary ace libraries that'd be biggest imrpovement, after fixing accuracy of course.

dfherr commented 4 years ago

And how do you deal with spell name localization? Just a translated list for every spell and language? 😨

This is the first time I'm developing a WoW addon apart from minor changes/bug fixes. What would be the improvement from removing all the ace stuff? From what I've seen so far it's used pretty heavily (module creation & loading, communication and probably more). So removing it wouldn't be an easy task.

dfherr commented 4 years ago

This feature request was added tho https://github.com/EsreverWoW/LibThreatClassic/issues/3

Here: https://github.com/dfherr/LibThreatClassic2/blob/master/LibThreatClassic2.lua#L720-L774

so at least 1 addon has to actively register to the lib for it to start listening to combat log events. otherwise it unregisteres from the combat_log_events. Implementation is not ideal, but the best I could do with the given structure with reasonable effort.

dfherr commented 4 years ago
function ThreatLib:GroupDistribution()
  if IsInGroup(2) then
      return "INSTANCE_CHAT"
  elseif GetNumGroupMembers() > 0 and IsInRaid() then
      return "RAID"
  else
      return "PARTY"
  end
end

this is fixed https://github.com/dfherr/LibThreatClassic2/commit/20d6d9622e2369269fb01e53564787bc398f6d4f

michaelnpsp commented 4 years ago

Just a translated list for every spell and language?

You can use getspellinfo function to get the localized spell name:

local spells={ [GetSpellInfo(169)] = 169, [GetSpellInfo(58)]=58, } Fix: Sorry, the correct function is: GetSpellInfo.