Fix hero-talent spell icons reverting to their base spell on zone change - #1266
Open
dfrisone wants to merge 1 commit into
Open
Fix hero-talent spell icons reverting to their base spell on zone change#1266dfrisone wants to merge 1 commit into
dfrisone wants to merge 1 commit into
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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._cdIconHealskips its repaint when a per-button texture memo equals the live texture:fd.lastIconTexwas written in exactly one place in the codebase: inside_cdIconHealitself. 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
GetActionTexturebriefly reports the BASE spell, andForceButtonRefreshpaints it without stamping. The memo and the icon then disagree, in the one direction that does cause a wrong skip:_cdIconHealstamps the memo with Black Arrow's texture. Icon correct.ForceButtonRefreshpaints Kill Shot's texture, memo untouched. Icon and memo now diverge.SPELL_UPDATE_ICONfires_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
ForceButtonRefreshnow stamps the memo with the texture it paints, restoring the invariant that the memo matches what is actually on the icon.GetActionTexturewas 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_cdIconHealalready 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
_cdIconHealsee 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
_cdIconHealcomment, 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
_cdIconHealcomment builds the memo on "the texture fileID is ... never secret, per the API docs", whileForceButtonRefreshsays to gate onHasActionand 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_cdIconHealhas 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):
Screenshots
N/A, no visual design change. The fix stops an icon from rendering the wrong spell's art.
Checklist
ForceButtonRefreshcall, no new API call (the existingGetActionTextureis hoisted to a local), no allocationHookScript/hooksecurefunconly, neverSetScripton Blizzard frames -- the buttons are EUI's ownEABButtonframes and the memo lives in EUI's ownns._eabFDside table