Boss icon is unstable during combat
Nukme opened this issue ยท 10 comments
version: 6.21.11b
style: Neon
problem:
The boss icon(skull) could disappear during combat. I first met this problem when pulling Vectis and Fetid in Uldir a few weeks ago, and I tested with world bosses and boss-level dummy this week to confirm it.
When out of combat, there's nothing wrong. The skull icon would show up no matter how you toggle the nameplate or move camera direction. But during combat, if somehow the nameplate of the boss is refreshed or regenerated, there's a chance that the boss icon would disappear.
I've reproduced the problem in the following ways:
i. phased into a new realm with a live boss-fight.
ii. toggle nameplate on and off during a boss-fight.
iii. move camera direction so that the nameplates leaves and re-enters the screen during a boss-fight.
Hi, I think I might be able to explain the bug.
I've been thinking about the fix and been confused by the fact that neither UpdateIndicator_Standard() nor UpdateIndicator_Level() actually does anything about isBoss. Although by comparing unit.isBoss to unitcache.isBoss does prevent the disappearance, it just doesn't seem right. So I change the code back and put some debug trackers (print()) and here is what I find.
When the disappearance happens, the status of 4 flags:
unit.level = -1
unitcache.level = nil
unit.isBoss = nil
unitcache.isBoss = nil
By comparing unit.isBoss to unitcache.isBoss, what it actually does is to prevent UpdateIndicator_Level() from hiding the boss-icon, which is not the root of the bug.
The code registers game-event 'UNIT_HEALTH_FREQUENT' and binds the function OnHealthUpdate() to it, which calls ProcessUnitChanges() that calls UpdateIndicator_Standard() to setup icons. When the nameplate is toggled on, OnShowNameplate() is triggered, which clears unit and unitcache. And if it is closely followed by game-event 'UNIT_HEALTH_FREQUENT', a OnHealthUpdate() with empty parameter tables will be executed, which leads to nil isBoss and the disappearance. And that explains why when out of combat everything is fine but in combat it could happen by chance.
So, I think the appropriate fix should be adding one line to UpdateUnitCondition() as
...
function UpdateUnitCondition(plate, unitid)
UpdateReferences(plate)
unit.level = UnitEffectiveLevel(unitid)
unit.isBoss = unit.level == -1
...
which acts as a fail-safe check outside UpdateUnitIdentity(), and should work well along with the comparison fix.
Setting isBoss in two different functions make even less sense than the current fix.
By comparing unit.isBoss to unitcache.isBoss, what it actually does is to prevent UpdateIndicator_Level() from hiding the boss-icon, which is not the root of the bug.
This assessment is just plain wrong. We use a or operator not an and, which means it does hide the icon because the level is different from cache. It is there so that the level function gets run, again, once isBoss has been set.
If it bothers you that it does hide the icon, for at most a frame, you could check if isBoss is nil in the level function and not change the skullicon if it is, because this should never be nil if it the information has actually been gotten.
If anything should be moved it is probably where level is set, because I can't really find a reason for this being updated everytime the units health is changed. But at the same time I don't really want to mess a with something that isn't broken, just because I can't see the reasoning for it.
Hmmm, I see where I make a mistake. Thanks for the explaining.
But I insist that isBoss being nil is a flaw, and the auto-adjusting perspective of hiding&showing skull-icon should not being the purpose of level function in the first place. I originally thought that since unit.level is set twice, it is only natural to check isBoss twice. Indeed it's a better way to move level check and all the information gathering so that we don't need two functions to setup icons. But like you said, it's an overdo.
Again, thanks for your time!
unit.level isn't set twice though.
And since isBoss is determined by level, the level function handling that isn't a far stretch.
I definitely agree that the current method might not be the most optimal. And most likely the root cause of the issue is actually that these two values have a relationship, but are set by different functions. It is just that the most optimal solution might have unforeseen consequences when you don't know the purpose behind certain things.
Closing this as it is now part of the latest release.
I have an update that may help locate the bug.
I change line 656 of TidyPlatesCore.lua (6.21.11b) to
if unit.isBoss and style.skullicon.show then visual.level:Hide(); visual.skullicon:Show() else visual.skullicon:Hide(); print(unit.level); print(unit.isBoss) end
and when the skull-icon disappears, it prints '-1' and 'nil'.
I haven't figured out how the isBoss flag turns nil between toggles but there seems to be an easy fix by changing condition from unit.isBoss to unit.level==-1.
While I cannot reproduce this on my end, I think there is something I can do to try and prevent this from happening, assuming I'm right about why it might be happening. Which is that it has something to do with the level being reported incorrectly when it's checked if the unit is a boss, but not when getting the units level a function later.
I'll include it in the next release and hopefully that solves this.
It is weird that isBoss is 'nil' as that would mean the function hasn't been run at all. If that is the case then isElite and isRare should be 'nil' as well. And the only difference between those are that isBoss is the only one that doesn't compare itself to the 'unitcache', but rather checks if level is different from the unitcache.
So getting it to compare the unitcache for isBoss as well as level in the function 'UpdateIndicator_Standard' should resolve the issue.
As to what is happening is probably that 'OnHealthUpdate' gets triggered before the 'OnUpdateNameplate' is done. This causes the icons etc. to be set up by that function even though they don't have the necessary information yet.
'OnUpdateNameplate' then attempts to do its own setup of the icons, but only updates the ones that are different when compared to the 'unitcache'. And since it compares the units level when checking the cache and not isBoss, and the level is updated by the 'OnHealthUpdate' function, that is a value that is unchanged, so it doesn't update.