Skip to content

fix(charactersheet): stop a failed enchant scan sticking for the session - #1993

Merged
EllesmereGaming merged 1 commit into
EllesmereGaming:mainfrom
dfrisone:fix/dk-runeforge-reads-as-missing-enchant
Sep 7, 2026
Merged

fix(charactersheet): stop a failed enchant scan sticking for the session#1993
EllesmereGaming merged 1 commit into
EllesmereGaming:mainfrom
dfrisone:fix/dk-runeforge-reads-as-missing-enchant

Conversation

@dfrisone

@dfrisone dfrisone commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Reported by @railgun (Blizz UI Enhanced, 9.1.6): the character sheet keeps
flagging a Death Knight's main hand as missing an enchant with the runeforge
applied.

What it is not

Traced live against an affected weapon first, because every part of the chain
looked correct on inspection:

  • the runeforge is in the item link's enchant field (ench: 6245), so the
    item:%d+:(%d+) parse is fine
  • the tooltip line is present and carries type 15
    (ItemEnchantmentPermanent), reading Enchanted: Rune of the Apocalypse --
    so _ENCHANT_LINE_TYPE matches on the first branch
  • TooltipUtil.SurfaceArgs no longer exists on this client, but lines now
    arrive pre-surfaced (leftText populated, args nil), so the guarded skip
    is harmless

Against live data the resolver returns the right answer. It just never gets
asked.

Root cause

EUI_GetEnchantText caches by enchant id and treats an empty result as a
cacheable answer:

local cached = _enchantNameCache[enchantID]
if cached ~= nil then return cached end   -- "" counts as a hit
...
_enchantNameCache[enchantID] = ""         -- written on every failed scan

A scan can legitimately come back empty because the item's data has not
arrived yet. That empty answer was then stored permanently, and since the
guard is ~= nil rather than a truthiness test, every later call returned it
without re-reading.

The sheet already re-runs on GET_ITEM_INFO_RECEIVED (QueueBetterItemsRefresh)
for exactly this situation -- the recovery path was built and correct, and the
cached miss silently threw it away.

That is why it reproduces for one player and not another: it depends purely on
whether the first scan happened before the item's data was ready.

Fix

Only successful lookups are cached. A miss returns "" uncached, so the next
refresh re-reads and the existing recovery path works as intended.

Death Knights feel it hardest because both weapons carry the same runeforge id,
so a single poisoned entry flags the pair -- but nothing about the bug is
class-specific; any enchant can hit it.

Testing

luac -p clean; house style check clean. Not click-tested against the original
report -- I could not reproduce locally (my own DK's first scan succeeds, so the
cache holds the good value). Worth having @railgun confirm, and worth noting
that /reload should have cleared it for them before this fix, which is itself
a decent check that this is the right cause.

Reported for Death Knights: the character sheet kept flagging the main
hand as missing an enchant with a runeforge applied.

EUI_GetEnchantText caches by enchant id and treated an empty result as a
cacheable answer, guarded by `cached ~= nil`. A scan that came back empty
because the item's data had not arrived yet was therefore stored, and every
later call returned it without re-reading. The sheet already re-runs on
GET_ITEM_INFO_RECEIVED for precisely that case, so the recovery path was
there and the cached miss discarded it.

Only successful lookups are cached now. Verified live: the item link carries
the runeforge (enchant id 6245), and the tooltip line for it is present with
type 15 (ItemEnchantmentPermanent) reading "Enchanted: Rune of the
Apocalypse" -- so once the scan is allowed to run again it resolves.

Hits Death Knights hardest because both weapons carry the same runeforge id,
so one poisoned entry flags the pair, but nothing about it is class-specific.
@EllesmereGaming
EllesmereGaming merged commit d219d81 into EllesmereGaming:main Sep 7, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants