Skip to content

Fix hero-talent spell icons reverting to their base spell on zone change - #1266

Open
dfrisone wants to merge 1 commit into
EllesmereGaming:mainfrom
dfrisone:ab-override-icon-memo
Open

Fix hero-talent spell icons reverting to their base spell on zone change#1266
dfrisone wants to merge 1 commit into
EllesmereGaming:mainfrom
dfrisone:ab-override-icon-memo

Conversation

@dfrisone

@dfrisone dfrisone commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes hero-talent spell icons reverting to their base spell's icon when you enter or leave an instance.

Reported symptom: on a Dark Ranger, Black Arrow renders as Kill Shot after zoning; same for Rider of the Apocalypse's Death Charge showing as Death's Advance. Pressing the button sometimes repaints it, usually not. Confirmed not to occur on Blizzard's action bars, and the Cooldown Manager is unaffected.

Root cause

ns._cdIconHeal skips its repaint when a per-button texture memo equals the live texture:

local tex = GetActionTexture(action)
local fd = EFD(btn)
if fd.lastIconTex ~= tex then
    fd.lastIconTex = tex
    ...paint...
end

fd.lastIconTex was written in exactly one place in the codebase: inside _cdIconHeal itself. Every other painter left it alone. The comment documented that as deliberate, reasoning that unstamped painters "always paint the CURRENT texture, so a stale memo can only cause one redundant repaint here -- never a wrong skip".

That reasoning holds only while CURRENT means the texture the memo already holds. A spell override resolves late across a zone change, so GetActionTexture briefly reports the BASE spell, and ForceButtonRefresh paints it without stamping. The memo and the icon then disagree, in the one direction that does cause a wrong skip:

  1. Hero talent live: _cdIconHeal stamps the memo with Black Arrow's texture. Icon correct.
  2. Zone change, override not yet resolved: ForceButtonRefresh paints Kill Shot's texture, memo untouched. Icon and memo now diverge.
  3. Override resolves, SPELL_UPDATE_ICON fires _cdIconHeal, which computes Black Arrow's texture, finds it equal to the stale memo, and skips.

So the event whose entire purpose is repainting an override change is the one the stale memo disables. The icon stays wrong until some other path repaints that slot, which is why pressing the button occasionally cleared it.

This also explains the reporter's other observations: Blizzard's bars have no such memo and simply repaint, the Cooldown Manager is a separate module, and only override spells are affected because an override is the only thing that changes a slot's texture without changing the slot, which is the memo's entire premise.

Fix

ForceButtonRefresh now stamps the memo with the texture it paints, restoring the invariant that the memo matches what is actually on the icon. GetActionTexture was already being called there, so it is hoisted into a local rather than called twice: no new API call, and no new handling of the texture value beyond the assignment _cdIconHeal already performed.

The two assisted-combat painters are deliberately left unstamped, and the comment now says so. They paint the SUGGESTED spell's texture rather than the action's, so stamping that would make _cdIconHeal see a mismatch and repaint with the action texture, clobbering the One Button Assist icon. A blanket "stamp every painter" change would regress OBA.

I also rewrote the _cdIconHeal comment, since it asserted the old write-behind reasoning as fact and would invite someone to remove the stamp as redundant.

One thing left alone, flagging for you

The file contradicts itself on whether the action texture can be secret. The _cdIconHeal comment builds the memo on "the texture fileID is ... never secret, per the API docs", while ForceButtonRefresh says to gate on HasAction and never on "the texture value, which can be secret". Both cannot be right, and the memo's design rests on the first.

This PR does not depend on which is correct and changes nothing about secret handling, deliberately: if that note is there on purpose, it should be your call rather than an assumption from me. If the second comment is the accurate one, the ~= comparison inside _cdIconHeal has a separate issue that predates this bug and is worth a look on its own.

How was it tested?

Live retail. Reproduced the stale icon across instance enter/leave on an overriding hero-talent spell, applied the branch, and confirmed the icon now stays correct in both directions.

For anyone verifying, this scan reports any button whose painted texture has drifted from what its slot reports (silence means all icons match; run it outside an instance, since the texture can come back secret inside):

/run for s=1,180 do local b=_G["EABButton"..s] local i=b and (b.icon or b.Icon) local a=b and b:GetAttribute("action") local y=a and GetActionTexture(a) if i and y and i:GetTexture()~=y then print("MISMATCH",s) end end

Screenshots

N/A, no visual design change. The fix stops an icon from rendering the wrong spell's art.

Checklist

  • New settings default OFF (no behavior change without opt-in) -- N/A, no new settings; correctness fix to an existing paint path
  • Zero cost while disabled: no events registered, no polling, no hooks doing work, no frames built -- N/A, no new feature; no new events, hooks or frames
  • Cheap while enabled: event-driven (no polling, no timer-based logic, no per-frame allocations) -- one table field write per ForceButtonRefresh call, no new API call (the existing GetActionTexture is hoisted to a local), no allocation
  • No writes onto Blizzard-owned frames (weak-table pattern used); HookScript/hooksecurefunc only, never SetScript on Blizzard frames -- the buttons are EUI's own EABButton frames and the memo lives in EUI's own ns._eabFD side table
  • Tested in-game, works on live retail. Not exercised on the 12.1 PTR client; the change is a local hoist plus one field assignment on an existing path, with no API surface that differs between clients.
Scared Baby GIF

ns._cdIconHeal skips its repaint when its per-button texture memo equals
the live texture. The memo was write-behind: only _cdIconHeal itself ever
stamped it, on the reasoning that other painters always paint the current
texture, so a stale memo could cost a redundant repaint but never a wrong
skip.

That holds only while "current" is the texture the memo already holds. A
spell override can resolve late -- zoning into or out of an instance on a
hero talent briefly reports Black Arrow as Kill Shot, Death Charge as
Death's Advance -- so ForceButtonRefresh painted the BASE texture while
the memo still held the override's. The SPELL_UPDATE_ICON heal that
follows, whose whole purpose is repainting an override change, then
compared equal and skipped, leaving the base icon up until something else
repainted the slot (pressing the button sometimes did).

Stamp the memo in ForceButtonRefresh with the texture it paints, so the
memo always matches what is on the icon. The assisted-combat painters are
deliberately left unstamped: they paint the SUGGESTED spell's texture
rather than the action's, and stamping that would make the heal clobber
the One Button Assist icon.
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.

1 participant