fix(cdm): never park a cooldown frame we failed to identify - #1210
Open
dfrisone wants to merge 4 commits into
Open
fix(cdm): never park a cooldown frame we failed to identify#1210dfrisone wants to merge 4 commits into
dfrisone wants to merge 4 commits into
Conversation
CollectAndReanchor's Phase 4 sweep named two states and handled two, but three arrive. Its own comment says "unclaimed (unrouted or ghost-bar routed)", and everything not in usedFrames is alpha-zeroed and parked at -10000. A frame that merely failed to RESOLVE lands in that bucket too. Resolution fails whenever GetCooldownViewerCooldownInfo returns nil for a cooldownID, which is what Blizzard does mid-rebuild: a zone-in, PvP talents activating, or a spec swap that just wiped the per-frame resolve memos. So a single transitional read parks most of the bar, nothing re-collects afterwards, and the CDM stays empty until a reload. The frames that survive are the ones whose fc memo was still valid and never re-resolved, which is why it presents as "everything gone except an icon or two". The same root has a second face. A frame that misses the sweep entirely sits at Blizzard's own Edit Mode position instead, which is the centre-screen scramble on top of the action bars. Record resolve failures separately and skip them in the sweep: unknown is not rejected, and we have no basis to park what we could not identify. Re-collect on a bounded 0.5/1.5/2.5s backoff so they claim as soon as the API answers, which makes recovery timing-independent rather than racing a fixed delay against the loading screen. Once the budget is spent, an id that still will not resolve is no longer plausibly transient and parks as before, so a stale pool entry cannot sit on screen forever. Any clean pass restores the budget.
Second report: the CDM was fine for the whole battleground and went blank on the way OUT. Nothing showing. The exit is the harder half of this transition and the previous commit's fixed retry budget does not cover it. Leaving a PvP instance deactivates the PvP talents, so PLAYER_ENTERING_WORLD runs ScheduleTalentRebuild, which wipes every frame's resolve memo (resolvedSid, cachedCdID) and re-collects against an API that is still transitional across the loading screen. The budget buys 0.5 + 1.5 + 2.5 seconds; a zone-out settle can outlast that, and when it does every frame parks at once and the CDM is empty until a reload. A fixed timer cannot be the answer here, so use a fact instead. fc.barKey records that we have claimed the frame before, and ScheduleTalentRebuild deliberately does not wipe it, so it survives the very rebuild that strips the resolve memos. A frame that was on a bar moments ago and is briefly unidentifiable is transient by definition. Exempt it from parking permanently rather than for a few seconds. This cannot strand a genuinely retired frame. One whose spell was unassigned or ghosted still RESOLVES; it just routes nowhere, so it never enters the unresolved set and takes the normal park path. The budget still bounds frames we have never claimed, which is where a dead cooldownID would show up.
Reporter also hit this by zoning into a battleground on one spec and swapping spec during the load. The barKey exemption already covers that case, and deliberately so: the spec-swap path is FullCDMRebuild's isFullWipe branch, which clears seven fc fields and, like ScheduleTalentRebuild, does not clear barKey. So every frame claimed before the swap is still vouched for while the API is transitional. Documented that at the wipe site, since it is now load-bearing and reads like an omission. The gap is cooldowns that are NEW to the spec being swapped TO. They have no barKey to vouch for them, so the 0.5/1.5/2.5s budget is all they have, and a swap during a loading screen can hand them a budget an earlier transition already spent. Reset it at both memo-wipe sites: a wipe is by definition the start of a fresh window of transient failures, so the pass that follows should get the full allowance.
Three rounds of fixes on this bug have been reasoned from "the icons disappeared", and that phrase covers three causes a screenshot cannot tell apart: Blizzard's pools are empty -> nothing to claim; the viewer never repopulated frames exist but are PARKED -> our claim pass failed and swept them offscreen frames exist and are CLAIMED -> bars are fine, something else hides them Only the middle one is what the last three commits address. If a report is actually the first, every claim-side fix is aimed at the wrong thing. /cdmwhy dumps per-viewer active/claimed/parked counts, probes GetCooldownViewerCooldownInfo live for each frame (the exact call whose nil return makes a frame unidentifiable mid-rebuild), lists each bar's icon count and alpha, and prints a verdict. Read-only, no new chunk locals, and zero cost until someone types it.
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
1 participant
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.
The report
Porting into a battleground left the Cooldown Manager holding one or two icons, out of combat, with no Lua errors, and only a
/reloadbrought it back. A second reporter saw the same thing on the way out of a battleground, and after swapping spec during a battleground load.Cause
CollectAndReanchor's Phase 4 sweep names two states and handles two, but three arrive. Its own comment says it:-- CD/utility frame: unclaimed (unrouted or ghost-bar routed).Everything not in
usedFramesis alpha-zeroed and parked at -10000. A frame that merely failed to resolve lands in that same bucket.Resolution fails whenever
C_CooldownViewer.GetCooldownViewerCooldownInforeturns nil for a cooldownID, which is exactly what Blizzard does mid-rebuild: a zone-in, PvP talents activating or deactivating, or a spec swap. At the collect site the failure had noelseat all, so the frame was silently dropped, never enteredusedFrames, and got parked.The survivors are the frames whose
fcmemo was still valid (fc.resolvedSid and fc.cachedCdID == cdID) and so never re-resolved. That is why it presents as "everything gone except an icon or two".The same root has a second face: a frame that misses the sweep sits at Blizzard's own Edit Mode position instead, which is the centre-screen scramble over the action bars that one reporter caught on video.
Fix
Record resolve failures separately and skip them in the sweep. Unknown is not rejected, and there is no basis to park what could not be identified. A bounded 0.5 / 1.5 / 2.5s backoff re-collects so the frames claim as soon as the API answers, which makes recovery timing-independent rather than racing a fixed delay against the loading screen.
A frame we have claimed before never parks on a failed resolve.
fc.barKeyrecords that we claimed it, and neither memo-wipe path (ScheduleTalentRebuildnorFullCDMRebuild'sisFullWipe) clears it, so it survives the very rebuild that strips identification. A frame that was on a bar moments ago and is briefly unidentifiable is transient by definition. This is what covers the zone-out case, where a loading-screen settle can outlast any fixed budget.This cannot strand a retired frame: one whose spell was unassigned or ghosted still resolves and simply routes nowhere, so it never enters the unresolved set and takes the normal park path.
The budget refills at every memo wipe, since a wipe is by definition the start of a fresh window of transient failures. That matters for cooldowns new to the spec being swapped to, which have no
barKeyto vouch for them.Once the budget is spent, a never-claimed frame that still will not resolve is no longer plausibly transient and parks as before, so a stale pool entry cannot sit on screen forever. Any clean pass restores the budget.
/cdmwhyThe last commit adds a read-only diagnostic, in the same spirit as
/eablagand/cdmbb. "The CDM is empty" has three causes a screenshot cannot separate: Blizzard's pools are empty, the frames are parked, or they are claimed and something else hides them. Only the middle one is what this PR addresses, and three rounds of this bug were reasoned without being able to tell them apart./cdmwhydumps per-viewer active/claimed/parked counts, probes the resolve call live, lists each bar's icon count and alpha, and prints a verdict. Zero cost until typed, and easy to drop from this PR if you would rather it landed separately.Notes
ns, the frame set on the existing_scratch_*pattern.FullCDMRebuildnow carries a comment explaining whybarKeyis deliberately absent from the list, since that omission is load-bearing and would otherwise read like an oversight.luac -pclean.Testing
Confirmed in game: battleground entry, exit, and a spec swap during a battleground load, with no breakage.