
[BUG] "dispel" auraIdentifier needs more checks
Closed this issue · 14 comments
Issue description:
This check in Modules/AuraIndicators.lua
:
or (aura.isHarmful and aura.isRaid and "dispel" == auraIdentifier)
used to be sufficient to check for dispellable debuffs, but is no longer (at least on retail) as Blizzard have started to mark boss auras and priority auras the same way so they appear on their indicators.
Based on the Blizzard UI code[1], I think the correct check is now:
or ("dispel" == auraIdentifier and aura.isHarmful and aura.isRaid and not aura.isBossAura and not AuraUtil.IsPriorityDebuff(aura.spellId))
or
or ("dispel" == auraIdentifier and AuraUtil.ProcessAura(aura, true) == AuraUtil.AuraUpdateChangedType.Debuff)
This only works for retail, classic and classic_era don't have AuraUtil.ProcessAura
nor SpellIsPriorityAura
that it relies on. I don't play classic and don't know how to check anything there. I assume the current check is either working there or will continue to be wrong in the same way there if left.
[1] Interface/AddOns/Blizzard_FrameXMLUtil/Mainline/AuraUtil.lua
How to reproduce:
Set "dispel" as debuff type in an indicator, see it appear for things you can't dispel in dungeons and raids.
Technical info:
- Addon Version: 3.16.3
- Client: Retail
- Client Patch Version: 11.1.0
Additional context:
Screenshots:
N/A
Lua error log:
N/A
I think the faked up isRaid = true
from LibDispel for bleeds should pass this ok, but haven't worked through the logic.
I know last time you were keen to do the work yourself (understandable) so I've logged this early without testing any fixes myself. If you want me to submit a PR after I've tried it out let me know.
Thanks, I'll take a look. I don't actually play WoW currently, so my ability to test is limited. I'll see if I can log into a low level toon and replicate this though
Happening since the start of TWW I believe. I just assumed LibDispel was trash and causing the problem, now I finally got around to looking at it I see Blizzard have changed their logic for "display only dispellable debuffs". I'm still guessing that this the cause, maybe it's not. I believe I'm seeing "dispel" matching things I can't dispel though.
If you're not playing and/or maintaining ERF don't worry about it, I'm sure I can fix my own copy after a bit more debugging.
Interesting. This is good info, and I'm happy to help get the fix mainlined. The "isRaid" logic always seemed a bit suspicious to me, and it was super poorly documented, so it doesn't surprise me that they changed how that works. That said, if we can tease apart exactly what the issue is and get a fix in, I'm more than happy to facilitate it. I think I can sort out the logic for classic and classic_era as well, despite not having those installed.
So the leading theory right now is that the isRaid flag no longer only indicates if an item is dispellable, but now also reports as true on boss auras and priority auras? I'm still confused as to what isRaid is meant to do in that case lol. It seems like a super confused and convoluted flag that the game is setting lol.
This API documentation page still has "isRaid" listed as "Whether or not this aura meets the conditions of the RAID aura filter." And this page has the RAID filter listed as:
HELPFUL: Buffs filtered by the player's class, e.g. for Priests it will only return [Power Word: Fortitude] or [Renew].
HARMFUL: Certain debuffs that only show up on raid frames, e.g. most debuffs that are relevant in a raid context.Requires the HELPFUL or HARMFUL filter if using UnitAura / .GetAuraDataByIndex
This does not require you to be in a raid
Based on this, harmful doesn't seem to directly indicate "is dispellable" like we're using it, but rather is some abstract concept of "debuffs relevant in a raid context", which seems weirdly variable and non-specific.
Likewise, for your suggestions:
("dispel" == auraIdentifier and aura.isHarmful and aura.isRaid and not aura.isBossAura and not AuraUtil.IsPriorityDebuff(aura.spellId))
or
or ("dispel" == auraIdentifier and AuraUtil.ProcessAura(aura, true) == AuraUtil.AuraUpdateChangedType.Debuff)
Why would filtering out isBossAura=true and IsPriorityDebuff()=true help here? The implication here is that boss applied auras aren't dispellable, but we know that they are. So filtering out any auras applied by the boss seems like it would just hide necessary debuff information from the player. However, maybe I'm misunderstanding what the isBossAura flag is trying to indicate.
I'm wondering if the "dispel" keyword and the isRaid flag is maybe too inconsistent to use practically. Our old way was just to hard-specify the debuff types, i.e. curse, poison, magic, disease, and bleed (new with libdispel). And this method at least gave us predictable results. I'd love the "dispel" keyword to work the way we want it to, but it seems like they keep rug pulling us on how it works lol.
I made a couple of quick changes that 'probably' won't fix the issue, but should take into account a couple changes to return types from the wow API.
I'm not sure really about any of that. There's no official documentation for the AuraData and from observation the members do not necessarily reflect the names intuitively, so I don't want to argue about whether isBossAura makes sense or that kind of thing. The wiki data is just made by people like you and me doing reverse engineering on the Blizzard UI code.
We don't need to. because Blizzard already does want we want in a setting on the base frames, accessible in the UI for "display only dispellable debuffs". So I can't answer any of your "why would ..." questions except to say "because Blizzard does".
ERF can copy that logic – trying to understand it seems like a recipe for wasting time to me, given Blizzard make no assurances about what anything means or that they won't re-use flags for completely differnent purposes. Whether this is too fragile for you, I can't say. I'm happy to maintain my own fork of ERF forever, so in some sense it's no matter to me. It seems likely I'm the only user of "dispel" since nobody else has logged it as a problem.
Here is a pull-out of the code for CompactUnitFrame displaying only dispellable debuffs, bottom up (so things are defined before they are called). I haven't gone through the cascaded if statements to find the right magic, but I include it here in case you want to also. You can obviously see this yourself in the relevant blizzard UI code files.
function AuraUtil.ProcessAura(aura, displayOnlyDispellableDebuffs, ignoreBuffs, ignoreDebuffs, ignoreDispelDebuffs)
if aura == nil then
return AuraUtil.AuraUpdateChangedType.None;
end
if aura.isNameplateOnly then
return AuraUtil.AuraUpdateChangedType.None;
end
if aura.isBossAura and not aura.isRaid and not ignoreDebuffs then
aura.debuffType = aura.isHarmful and AuraUtil.UnitFrameDebuffType.BossDebuff or AuraUtil.UnitFrameDebuffType.BossBuff;
return AuraUtil.AuraUpdateChangedType.Debuff;
elseif aura.isHarmful and not aura.isRaid and not ignoreDebuffs then
if AuraUtil.IsPriorityDebuff(aura.spellId) then
aura.debuffType = AuraUtil.UnitFrameDebuffType.PriorityDebuff;
return AuraUtil.AuraUpdateChangedType.Debuff;
elseif not displayOnlyDispellableDebuffs and AuraUtil.ShouldDisplayDebuff(aura.sourceUnit, aura.spellId) then
aura.debuffType = AuraUtil.UnitFrameDebuffType.NonBossDebuff;
return AuraUtil.AuraUpdateChangedType.Debuff;
end
elseif aura.isHelpful and not ignoreBuffs and AuraUtil.ShouldDisplayBuff(aura.sourceUnit, aura.spellId, aura.canApplyAura) then
aura.isBuff = true;
return AuraUtil.AuraUpdateChangedType.Buff;
elseif aura.isHarmful and aura.isRaid then
if displayOnlyDispellableDebuffs and not ignoreDebuffs and not aura.isBossAura and AuraUtil.ShouldDisplayDebuff(aura.sourceUnit, aura.spellId) and not AuraUtil.IsPriorityDebuff(aura.spellId) then
aura.debuffType = AuraUtil.UnitFrameDebuffType.NonBossRaidDebuff;
return AuraUtil.AuraUpdateChangedType.Debuff;
elseif not ignoreDispelDebuffs and AuraUtil.DispellableDebuffTypes[aura.dispelName] ~= nil then
aura.debuffType = aura.isBossAura and AuraUtil.UnitFrameDebuffType.BossDebuff or AuraUtil.UnitFrameDebuffType.NonBossRaidDebuff;
return AuraUtil.AuraUpdateChangedType.Dispel;
end
end
return AuraUtil.AuraUpdateChangedType.None;
end
function CompactUnitFrame_ProcessAura(frame, aura, displayOnlyDispellableDebuffs, ignoreBuffs, ignoreDebuffs, ignoreDispelDebuffs)
local type = AuraUtil.ProcessAura(aura, displayOnlyDispellableDebuffs, ignoreBuffs, ignoreDebuffs, ignoreDispelDebuffs);
-- Can't dispell debuffs on pvp frames
if type == AuraUtil.AuraUpdateChangedType.Dispel and CompactUnitFrame_IsPvpFrame(frame) then
type = AuraUtil.AuraUpdateChangedType.Debuff;
end
return type;
end
local function HandleAura(aura)
local type = CompactUnitFrame_ProcessAura(frame, aura, displayOnlyDispellableDebuffs, ignoreBuffs, ignoreDebuffs, ignoreDispelDebuffs);
if type == AuraUtil.AuraUpdateChangedType.Debuff then
frame.debuffs[aura.auraInstanceID] = aura;
elseif type == AuraUtil.AuraUpdateChangedType.Buff then
frame.buffs[aura.auraInstanceID] = aura;
elseif type == AuraUtil.AuraUpdateChangedType.Dispel then
frame.debuffs[aura.auraInstanceID] = aura;
end
end
function CompactUnitFrame_ParseAllAuras(frame, displayOnlyDispellableDebuffs, ignoreBuffs, ignoreDebuffs, ignoreDispelDebuffs)
AuraUtil.ForEachAura(frame.displayedUnit, AuraUtil.CreateFilterString(AuraUtil.AuraFilters.Harmful), batchCount, HandleAura, usePackedAura);
AuraUtil.ForEachAura(frame.displayedUnit, AuraUtil.CreateFilterString(AuraUtil.AuraFilters.Helpful), batchCount, HandleAura, usePackedAura);
AuraUtil.ForEachAura(frame.displayedUnit, AuraUtil.CreateFilterString(AuraUtil.AuraFilters.Harmful, AuraUtil.AuraFilters.Raid), batchCount, HandleAura, usePackedAura);
end
function CompactUnitFrame_UpdateAurasInternal(frame, unitAuraUpdateInfo)
local displayOnlyDispellableDebuffs = CompactUnitFrame_GetOptionDisplayOnlyDispellableDebuffs(frame, frame.optionTable);
local ignoreBuffs = not frame.buffFrames or not frame.optionTable.displayBuffs or frame.maxBuffs == 0;
local displayDebuffs = CompactUnitFrame_GetOptionDisplayDebuffs(frame, frame.optionTable);
local ignoreDebuffs = not frame.debuffFrames or not displayDebuffs or frame.maxDebuffs == 0;
local ignoreDispelDebuffs = ignoreDebuffs or not frame.dispelDebuffFrames or not frame.optionTable.displayDispelDebuffs or frame.maxDispelDebuffs == 0;
CompactUnitFrame_ParseAllAuras(frame, displayOnlyDispellableDebuffs, ignoreBuffs, ignoreDebuffs, ignoreDispelDebuffs);
end
It's also possible Blizzard is also showing things incorrectly, I guess. Like I said I did almost no work on this yet.
Edit: curiously enough I'm 90% sure the Blizzard code has a bug, where in HandleAura
elseif type == AuraUtil.AuraUpdateChangedType.Dispel then
frame.debuffs[aura.auraInstanceID] = aura;
should read
elseif type == AuraUtil.AuraUpdateChangedType.Dispel then
frame.dispels[aura.dispelName][aura.auraInstanceID] = aura
this is papered over by the partial auraData update code doing the right thing.
FWIW I think you should probably ignore this until I can find a reproducible case.
Final update.
Found a repeatable case. Follower mode Cinderbrew Meadery, mobs in the entry room called "Venture Co. Pyromaniac" cast a magic debuff called "Erupting Inferno" (see below for post-processed AuraData).
This incorrectly matches on mage BUT Blizzard also incorrectly shows it.
So there is nothing we can do, the current logic is correct but doesn't always work. I don't know if this is a bug from Blizzard's end or they just define "dispellable" in some different way to what they did in the past.
I think you should probably move "dispel" to check LibDispel:IsDispellableByMe
aura = {
spellId=437956,
isBossAura=false,
duration=9,
expirationTime=578685.889,
isFromPlayerOrPlayerPet=false,
points={},
icon=236220,
nameplateShowPersonal=false,
dispelName="magic",
debuffType=4, -- This was added by AuraUtil.ProcessAura
nameplateShowAll=false,
auraInstanceID=39225,
timeMod=1,
isRaid=true,
isHarmful=true,
canApplyAura=false,
name="erupting inferno",
isHelpful=false,
applications=0,
isNameplateOnly=false,
sourceUnit="nameplate25",
isStealable=false
}
Can you test my latest patch? I took your advice and re-implemented the "dispel" wildcard using LibDispel to inform us of the player's dispellable debuff types. This should hopefully give you more consistency in what it is reporting. I've done a bit of testing and it doesn't error out and seems to work for me, but a more thorough test would be useful before I release a new version.
Tested with the follower dungeon aura from above, seems correct. Aura matches on class who can dispel it, does not match on class that can't.
Appreciate your time, sorry the bug info was ill-considered.