12.1: performance pass, alpha-14 fixes, Important Debuffs on by default - #224
Open
Krathe82 wants to merge 40 commits into
Open
12.1: performance pass, alpha-14 fixes, Important Debuffs on by default#224Krathe82 wants to merge 40 commits into
Krathe82 wants to merge 40 commits into
Conversation
Opening Edit Mode used to blank every DF aura row. We hid them, because the
alternative was watching Blizzard's sample icons render in our rows -- correct
against build 68412, where the rows had no other defence.
68569 changed the shape of the problem. CustomAuraContainer was folded onto
ManagedAuraContainer, so parsing runs through GetAuraSources(), which returns
the Edit Mode source list whenever the container's own useEditModeSource flag
is set. Exactly one thing sets it:
ManagedAuraContainerPrivateMixin:OnAuraDataProviderSwitch(useReal)
self:SetUseEditModeSource(not useReal)
ResetAuraDataProvider carries no HasRestrictions in the generated docs, unlike
its neighbours SetGroupBuffVisualAlerts and SetHiddenGroupBuffs, and test mode
already drives it. So our containers do not need to ignore the switch -- we
hand the real provider straight back and every container, ours and Blizzard's,
returns to the real source. No teardown, no button pools recreated.
THE TRADE, and it is global: Blizzard's own Edit Mode preview loses its sample
auras, so their buff frame shows your real ones while you position them. Same
class of trade DF test mode already makes in the other direction; the
alternative is our rows flashing.
Order, each step load-bearing:
1. PARK synchronously inside the dispatch. AURA_DATA_PROVIDER_SWITCH is
synchronous, so nothing is drawn between the containers' handler and ours
whichever runs first -- the sample icons are never painted at all.
2. RESET one frame later, never inline. An inline reset nests a dispatch
inside this one, and any container the outer fake dispatch had not yet
visited would take fake AFTER our reset and strand on the sample source.
3. Fall back to REBIRTH only if the reset is unavailable or fails: a
container built after a switch never receives it and useEditModeSource
initialises false, so a fresh one is on the real source by construction.
Test mode sets _ownsProviderSwitch around its own switches, so its curated
preview is exempt from all of this.
DEAD END, kept deliberately: unregistering AURA_DATA_PROVIDER_SWITCH on the
container is refused BY DESIGN, not by accident.
Enum.ForbiddenAspect.EventRegistrations restricts modifying an object's
registered events; every Register* method ADDS that aspect and UnregisterEvent
CHECKS it, and Blizzard's OnLoad_Intrinsic registers the event from secure
code -- so the container carries the aspect from birth and no ordering helps.
The probe stays, run once and never retried, so a build that relaxes the rule
gets noticed instead of assumed away.
Squashed from 11 commits: the deafening attempt above, a rebuild-everything
mechanism that the provider hand-back replaced, an instrumented experiment
into whether the client dispatches a re-entrant switch NESTED or QUEUED, and
the refinements those made unnecessary.
Two independent things want test frames on screen -- the USER (test panel,
/df test, toolbar) and UNLOCK (which needs frames to have something to drag) --
and one boolean could not tell them apart.
Unlock snapshotted DF.testMode; lock trusted the snapshot. Anything that
changed test mode DURING the unlock session left it stale, and it failed both
ways. Field-reported, both orders:
test on -> unlock -> close the panel -> lock
snapshot says true, so lock keeps a preview nobody asked for. Stuck on
until you toggle test mode or enter combat.
unlock -> enable test -> lock
snapshot says false, so lock kills a preview you had just turned on.
The "Cannot disable test mode while frames are unlocked" error existed only to
stop the snapshot going stale -- and it never even covered the toolbar button,
which closes the PANEL without going through ToggleTestMode. That was the
reported repro: the button looked like it disabled test mode, and nothing
refused it.
Replaced with ownership (TestMode/Shim.lua, resident because unlock/lock are):
frames show while ANY owner wants them. Unlock claims on unlock and releases on
lock; the user claims via the panel and the toggle. Lock releases only unlock's
claim, so a preview you asked for survives it and one you did not disappears
with it. Both reported orders come out right without special-casing, and the
refusal is gone.
Show*/Hide*TestFrames stay the only thing that moves frames, so everything
downstream -- PinnedFrames' testModeActive and its mover chrome, the aura
containers, the animation driver -- keeps working without knowing ownership
exists. Paths that hide directly drop their claims so no owner outlives the
frames.
The UI consequences, all of them fallout from the model rather than separate
features:
- The panel's toggle reflects the USER's claim, not whether a preview is on
screen. Those differ while unlocked, and driving it from the preview made
a legitimate click read as doing nothing.
- Turning it off while unlocked says why the frames stay and what will clear
them. The old code refused outright, which was at least feedback; silent
correct behaviour is worse to use.
- Both buttons perform one action -- turn the preview off and close the
panel -- and report the same message and the same state, whichever one you
click.
- The toolbar glyph tracks the preview rather than whether the panel is
open, since the panel can be closed with a preview still running.
Leaves the now-unused refusal locale key in place: removing it across every
locale belongs with the pending locale audit, not here.
Squashed from 4 commits (the model, then three rounds of UI follow-up from
testing it).
Toggling test mode in raid mode said nothing, while party printed "Test mode enabled/disabled." Pre-existing asymmetry, not from the ownership work -- ShowRaidTestFrames and HideRaidTestFrames never had the Say at all. Neither even took `silent`, although Panel.lua has been calling HideRaidTestFrames(true) with a "-- silent" comment for a while; that argument has quietly done nothing. Both halves now take it and honour it, so the mode-switch and Click-Casting-tab paths stay quiet as they intended and the user-driven ones speak up.
/edit left DF half-dressed. The game closes DandersFramesGUI and DandersFramesTestPanel for us -- both happen to be in UISpecialFrames -- but the position panel is not, so it stayed up, and behind it the grid, the movers and the test preview kept running. Two grids overlapping, and fake party frames sitting on top of Blizzard's own editor. The real gap is that DF had no Edit Mode integration at all: the two windows that did close were closing by accident of UISpecialFrames membership, not because anything handled Edit Mode. Locking is the right response and needed nothing new -- LockFrames and LockRaidFrames already hide the position panel, grid, movers and pinned drag chrome, and release unlock's test claim. Dropping the user's claim as well means no preview survives into Edit Mode; that also hands the real aura data provider back, which keeps Edit Mode's sample auras out of our rows. Not restored on exit, deliberately. Blizzard's Edit Mode does not put anyone else's windows back either, and silently re-unlocking frames under someone who has just rearranged their UI is worse than making them click Unlock.
Held until confirmed in game, per the usual rule. Krathe has now run the nine-step raid sequence from the original report and it no longer sticks. Four entries: the stuck/self-disabling preview (and the refusal message going away with it), the two test buttons becoming one control, raid mode finally confirming enable/disable in chat, and DF standing down when Blizzard's Edit Mode opens. Written from what the user sees rather than the mechanism -- "two things want test frames" reads as an explanation, not an excuse, and the reported symptom (stuck on, or switched off under you) is what people will search the changelog for.
Reported by Rytiou, with a repro for each half. ImportFilterPayload took the decoded name verbatim, and FindContentMatch compares SPELLS only -- so an import whose name already existed was not examined by anything. You got two rows reading the same thing, holding different spell lists, and no way to tell from the Buffs page which one you had just ticked. Worse through "Import as Copy", where the option that promises a distinguishable copy produced a row identical in name AND spells. Filter names are not unique and deliberately stay that way -- New, Rename and Duplicate all let you pick freely, and people have deliberately similar names saved. The actual defect is narrower: import was the ONE path that committed a name without showing it to you. Duplicate has always pre-filled "<name> copy" and prompted. So import now asks, but only when the name would clash: a clean import is still a single paste, and a clashing one gets Duplicate's treatment with a suggestion that does not collide. Both entry points -- the plain import and "Import as Copy" -- go through it. The prompt is chained; popups are a singleton, and the copy route opens it from inside another popup's handler. Includes its changelog entry (squashed from 2 commits).
placedStructSig carried includeSig(map) -- the tracked spell-ID whitelist -- so changing which spells an indicator watches forced Handle:Rebuild. That is a teardown+recreate, and teardown can only Hide(): WoW never destroys frames. Every such edit therefore stranded the container plus a 10-frame batch per group permanently (AddAuraGroup always calls CreateFrameBatch with FrameCreationBatchSize = 10, before maxFrameCount is even applied). A profile switch measured 29.95 MiB / 157 ms in one frame at party size, and none of it came back. The map is live-mutable: it becomes config.candidateFilters, which SetAuraGroupCandidateFilters mutates in place. applyGroupTuning already says so in its own comment -- "include/excludeSpellIDs live in the TUNING signature, not the struct one". The filter-group path has worked this way since Wave 1; the placed-indicator path was the outlier. Split it to match: placedStructSig keeps only create-only properties, the new placedTuningSig carries the map, and the call site gains the tuning branch (ApplyTuning off a fresh config, testEntries swapped so a test-mode rebuild previews the new selection). buildPlacedConfig has the same shape as buildFilterGroupConfig -- plain string filter, config-wide candidateFilters, max = 1, no sort -- so the proven pattern transfers unchanged. Alpha stays inside the cosmetic branch: it is already part of coSig, and this block runs per indicator per tick.
Same split as the placed icon/square path: the tracked spell-ID map comes out of barStructSig / alertCompanionStructSig and rides the shared placedTuningSig, so a selection edit is an ApplyTuning instead of a teardown+recreate that permanently strands the container and its 10-frame batches. Both are mode = "row" configs carrying testEntries and a config-wide candidateFilters, so applyGroupTuning handles them exactly as it already handles the filter-group path. The companion's steady-state early-out now compares the tuning sig as well, or a selection edit would be skipped entirely rather than applied. Remaining sites (placed/frame-level missing-buff, overlay tint, health mirror, border) are mode = "overlay"/"missing", where applyGroupTuning early-returns. Those need the slot-side setters wired up first and are left alone here.
RESOLVERS.group is the single biggest allocator in the addon -- 38% of a follower-dungeon trace and 43.6% of a boss trace, #1 in both -- and almost all of it was scratch. Every call built a fresh `parts` table plus a fresh 14-field element table PER ITEM, from config that only changes when the user edits it. Both now reuse module-level tables. Safe because groups cannot nest: the Text Designer's Add Item picker passes excludeKey = "group", so RESOLVERS[typeKey] can never route back into this resolver and it is never re-entered. No resolver retains its elem argument either -- they only read fields, and applyNameTrunc reads just nameLength/truncateMode. `parts` never escapes: it is appended, iterated, and the function returns a plain string. Every field of the shared element table is assigned on every iteration, nil included, so nothing leaks from one item to the next. Also guarded the four DF:Debug calls with DF:DebugActive("TD"). Arguments are evaluated by the caller, so the unguarded per-call one allocated two strings via tostring() on every tick even with the trace switched off. Matches the existing guard pattern in ClickCasting.
pcall(function() ... end) allocates a fresh closure every call purely to wrap one method call. In the build and tuning loops that is per group (or per slot) per container per unit frame, on the paths that already dominate every rebuild trace. pcall(fn, args...) does the same job and allocates nothing; the protection is identical, since these calls are guarded because AddAuraGroup and friends assert. Converted the five that sit inside per-key loops: AddAuraGroup (test pin groups and the record loop), AddAuraSlot, the slot SetAllPoints, the three setters in applyGroupTuning, and SetAuraGroupLayout in applyLayout. Left the once-per- container ones alone (SetEnabled, SetUnit) and the Hide/Show bounces, which are two statements and cannot take this form. Also recorded the real cost shape found in the Blizzard source while here: SetAuraGroupMaxFrameCount and SetAuraGroupSortMethod only MarkDirty and those flags coalesce into a single ProcessDirtyFlags next OnUpdate, so they are near free at any group count -- but SetAuraGroupCandidateFilters runs an immediate UpdateAllAuras per call with no equality guard, so N groups cost N full updates. There is no batch setter, so the only lever there is declaring fewer groups.
applyGroupTuning bailed out for both "overlay" and "missing" with no explanation -- unlike the test-mode guard directly below it, which is documented. Reading it against the Blizzard source, the overlay bail is scope rather than a constraint: overlay declares AuraSLOTs, so groupKeys is empty and the GROUP setters genuinely have nothing to act on, but SetAuraSlotCandidateFilters / SetAuraSlotSortMethod exist and do the same job per slot. Overlay now takes a slot branch. The cfByKey derivation is shared unchanged -- build() keys slots and groups identically (rec.key or positional "df<i>") -- as is the identity-gate re-derivation, which overlay previously never reached. A slot is one button, so there is no maxFrameCount or layout to push; sort still matters because it decides which aura wins the slot. MISSING stays on the Rebuild path, now with a comment saying why: its layout-push inversion is load-bearing and hard-won (the badge only clears the clip window because one blank button's layout CELL pushes it out), and a live candidateFilters swap changes precisely which buttons exist. Enabling that blind is not worth it. Inert until the Factory's overlay sites move the spell map out of their struct sigs -- they still Rebuild today, so nothing reaches this path yet. Committed separately so the engine change is bisectable on its own.
Activates the slot-tuning path added in 10d6048e. The tracked spell-ID map comes
out of the struct sig at all four overlay sites -- health mirror (both the flat
tint and filled mirror branches), background tint, border, and the Text Designer
mirror host -- and rides placedTuningSig instead, so a selection edit stops
teardown-and-recreating a container whose frames can never be reclaimed.
What stays structural is unchanged and deliberate: wholeBar picks a different
config builder, drawAbove changes the create-time level, and the pool filter
string still binds at build.
Two hazards handled rather than assumed away:
* The filled-mirror branch must NOT clear frame.dfADHealthMirror on a tuning
pass. The create and rebuild branches clear it because the slot is torn down
and onBar re-stashes the new StatusBar; a tuning pass keeps the same slot and
the same bar, so clearing it would strand the reference with nothing to
re-stash.
* The mirror-host site has a fourth branch (the TD-teardown host recovery), so
the tuning check is inserted as another link in that elseif chain rather than
folding the tail into an else. That preserves its one-action-per-pass shape;
the sync runs every tick, so a second pending change lands on the next one.
The two MISSING-mode sites keep their maps in the struct sig on purpose --
applyGroupTuning still returns early for that mode (see 10d6048e).
…acks resolveAppearance is DanderBot#2 in every combat trace (13.9% dungeon, 16.1% boss) and allocates up to four tables per call: two `or {}` fallbacks, the white colour fallback, and the returned table. The class-colour branch in UpdateOne called it a SECOND time for the same elem and globalDefaults -- a deterministic function with identical inputs -- purely to read one alpha. applyAppearance had already resolved exactly that and its result is still live in scope (it is passed to mirrorElement further down). Reuse it and the whole second resolve disappears for every class-coloured element, every tick. The two `or {}` fallbacks and the white colour now point at module-level shared tables. Verified safe: every use in TextDesigner reads these fields and nothing writes them, and mirrorElement reads app.font/fontSize/outline immediately rather than retaining the table. ☠ Deliberately NOT sharing the returned appearance table. Before the change above two results were live simultaneously -- the outer one bound for mirrorElement and the inner class-colour one -- so a single scratch table would have aliased and corrupted. That trap is gone now that the inner call is, but one aliasing hazard in a function is enough reason not to introduce a second for one table per call.
DF:SafeSetFont is one of the hottest functions in the addon and PreloadFont ran on every single call -- 5.7% of a boss trace's allocation, 15% combined with SafeSetFont itself, and the largest anonymous-CPU cluster in the login trace. PreloadFont exists only to force WoW to load a font file. A path needs that once: the file does not unload and a path IS the file, so the mapping never changes. Now memoised per path. Keyed on ATTEMPTED rather than succeeded, which preserves the old behaviour exactly -- the pcall result was discarded, so a failed load was never retried before either. Checked that neither fontFamilies wipe (ClearFontCache, RegisterFontAlphabetSupport) needs to reset it: both exist to rebuild families with updated ALPHABET support, which says nothing about whether a file is loaded. The memo stays valid across both. Also dropped the two per-call closures: PreloadFont's pcall takes the pcall(fn, args...) form, and the SetFontObject pair is hoisted to a file-local so its pcall can too. That one is two statements -- the GameFontNormal set first is deliberate -- so it needed a named helper rather than a bare method reference.
BuildSpec calls k(suffix) 37 times and each one concatenated prefix .. suffix
fresh, so every call allocated 37 strings. It shows up in both trace families --
2.7% of a boss run's allocation and 2.6% of a live rebuild's -- because it runs
per border per restyle and again per border per rebuild.
The built keys are now cached per prefix and reused. prefix .. suffix is
deterministic, so a cached key can never go stale.
Checked the memo cannot grow without bound, which is the thing that would turn
this from a fix into a leak: every call site passes a literal from a small fixed
set ("", frame, pet, buff, debuff, defensiveIcon, resourceBar, targetedList,
missingBuffIcon, personalTargetedSpell, personalTargetedSpellImportant), and the
two indirect sites (borderSpec.prefix, a pass-through prefix) resolve to the same
literals. Nothing derives a prefix per frame, per aura or per indicator.
k() stays a closure because it now captures the per-prefix memo, so this trades
37 allocations per call for 1. Left as a closure deliberately rather than
rewriting all 37 call sites to a different form for one more allocation.
TargetedList_LayoutBars called TargetedList_ApplyBarAppearance for every active bar on every render. That function is pure config -- everything it writes derives from db alone, never from the spell, the bar's state or the clock -- so nothing it did could differ between two renders with unchanged settings. It was 86% of all Border:Apply allocation in a combat trace (BuildSpec + Apply plus three or four SafeSetFont calls, per bar, per tick). Gated on targetedListLayoutVersion. That counter already existed at the top of the file and DF:UpdateTargetedListLayout already incremented it -- but nothing read it, so the mechanism was there and unused. It is the right gate because that hook is the single funnel for every settings change: the Options pages call it, preset application calls it, and a profile switch reaches it through Position.lua. A bumped version means "config changed", which is exactly when this work is needed. The hook's own comment already described the intended design -- appearance and content applied at acquisition, re-applied on settings change -- so the per-render call was the anomaly, not the mechanism. ☠ TargetedList_ResetBar now clears the stamp on release, immediately after the _lastTexturePath it already cleared for the same reason. Without that a recycled bar would skip the whole function and never get its status-bar texture reset, since forcing that re-set is precisely why _lastTexturePath is nil'd there. Colour-picker drag is unaffected: LightweightUpdate*Color write the widgets directly and deliberately bypass this path.
DataSource.Live allocated a fresh table plus a setmetatable on every call, and it
is called once per frame per Text Designer update -- 520 KB in a boss trace, and
100% of that function's allocation.
The wrapper carries nothing but the frame and its unit, and it is now reused per
frame with both fields re-stamped on every call, so a retargeted or recycled frame
still resolves against the correct unit.
Checked the two things that would have made this unsafe:
* No LiveSource method writes to self -- they are all pure reads -- so a reused
instance cannot carry stale per-update state.
* The instance does not escape the update. There is exactly one call site
(Render.lua) and it passes the source straight into Render:UpdateFrame, which
only calls methods on it and never stores it.
Falls back to a one-off instance when there is no frame to hang the cache off.
Render:UpdateFrame built a fresh liveIds table on every call to work out which FontStrings belong to deleted elements. It runs per frame per update and was 475 KB of a boss trace. Now a module-level scratch, matching enabledScratch a few lines above which already does exactly this in the same function. It needs its own table rather than sharing that one: enabledScratch holds only the ENABLED ids, this holds ALL of them. Safe against re-entry: the table is wiped immediately before it is filled and consumed straight after, and nothing between those points can call back into UpdateFrame -- the fill loop is a plain ipairs and the consume loop only calls FontString:Hide().
…ilters With the Blizzard category filters enabled (rather than Show All), the Important Debuffs highlight silently did nothing for a large share of the auras it should have covered. Reported as "boss/role/priority seems to be missing one of them" -- some important debuffs highlighted, some did not. Cause was a precedence inversion. The bossrole and priority records carried neg(true, true, true), so they EXCLUDED anything dispellable, CC-flagged or raid-flagged. That made the important categories the lowest precedence of the five: a priority debuff also carrying the RAID token was pushed out of the styled priority record and into the unstyled raid one. Most boss and priority debuffs in group content do carry RAID, hence "some highlight, some don't". Show All mode was never affected because it builds its exclusivity a completely different way. Fixed by making category mode match the mode that was already correct. Show All (the ALL-mode block earlier in this file) declares the important records FIRST with no negation and subtracts them from the rest via candidateFilter flags (isBossOrRoleAura = false, isPriorityAura = false). Category mode now does the same: bossrole and priority claim their auras first, and the cc / raid / dispel records subtract whichever important records were actually declared. Verified against the Blizzard source that a false candidate filter is an explicit negation rather than "absent" -- Blizzard_AuraContainerUtil.lua guards each flag with `~= nil` and then compares for equality, so false means "must NOT be". Exclusivity is still complete, so nothing double-renders: every non-important record subtracts both important flags, and the existing dispel > CC > raid token ordering between them is untouched. Users with none of boss/role/priority enabled get an empty subtraction table, which collapses to the same nil candidateFilters as before -- no change at all for them. Also dropped neg()'s now-dead excludeRaid parameter. Raid is the last token record and the important records no longer negate anything, so no call site could ever pass it true again. ⚠ This moves auras between rows, not just their styling: a raid-flagged priority debuff now renders in the priority record instead of the raid one, and therefore also leads the row. That is the documented intent of "important debuffs lead the row", but it is visible beyond the highlight itself.
Test mode declared one AuraGroup per preview slot, and every AddAuraGroup eagerly creates FrameCreationBatchSize (10) button frames BEFORE maxFrameCount is applied. So a 10-icon preview cost 10 groups x 10 frames = 100 button frames per container, per unit frame. Measured at 40 test frames that was the single largest allocation anywhere in the addon -- ~530 MB across four toggles, 65% of the test-mode trace -- and the re-measure after the rest of this batch showed it unmoved at -1%, because nothing so far had touched the group COUNT. Krathe: "it still feels very slow building out those 40 frames." It was. Now two groups: the styled slot keeps its own (so declaration order still pins it to position 1 -- the whole point of the original split, one styled icon leading a row of plain ones), and every remaining slot shares one group. That is 20 frames instead of maxCount x 10. _makeInitializeFrame gains seqStart: with it set, a group numbers its own buttons sequentially instead of taking a fixed index or the handle-wide creation counter. The shared counter cannot serve here because Blizzard creates a whole batch per group, so it runs well past the preview's slot range. Overshoot is harmless -- _paintTestSlot already wraps the index modulo the pool size, and buttons past maxFrameCount are never displayed. ⚠ TRADE-OFF, stated plainly: inside the shared group the flow assigns auras to buttons in the CONTAINER's order, not creation order, so the plain entries may appear in a different order than the curated pool lists them. It is deterministic (test mode declares no sort, so the same samples land the same way every build) and every entry is still shown. The "Lightning Shield mid-row" failure the per-slot shape was built to prevent was about the STYLED entry drifting, which declaration order still guarantees, and the mismatched-tooltip half is moot since tooltips are now forced off in test mode. Checked nothing else constructs or matches the old dfTest<N> keys. Updated two comments that described the retired per-slot design.
Scale, size and position of the settings window were stored in db.party, so they travelled with the profile. A new profile is born from PartyDefaults, which meant creating one silently reset guiScale to 1 -- but only for the readers that re-read the db. The Test Mode and Unlock windows do that on every OnShow, so they snapped to 100% while the already-open settings window kept the old scale, and only moving the UI Scale slider (the single writer) put them back in step. ExportCategories has always declared these keys local-only 'machine/window state, not profile content'. Storing them account-wide on DandersFramesDB_v2.windowState is what enforces that: a profile switch or a fresh profile can no longer move, resize or rescale the window. Seeded once at login from the active profile, then stripped from every profile (both modes -- RaidDefaults is copied from PartyDefaults, so each profile carried a dead db.raid set too). Same shape as the languageOverride per-profile -> per-character move already in this block.
The dispatcher's unknown-word fallback loads the companion and retries, so tools that register their slashes only on load (icons, colorhook, atlas, auraexp, memtest) still answer before /df has been opened. But 'recognised' was read from DebugSlashBySub, which only RegisterDebugSlash fills. The 38 commands registered through RegisterDebugSub -- every hand-written branch in this addon, zorder and auradata and ppdump among them -- were invisible to that lookup, read as unknown, and pulled in ~3 MB of settings UI before running code that never needed it. One diagnostic on a fresh login gave back the whole saving of splitting the addon in two. Worst for the probes that exist to measure memory: loading the companion perturbs exactly what they report. RegisterDebugSub already sees every name and already builds a set one line below for the dev flag; record the names the same way and test that set before the load. Registering IS the gate, so a branch added later is covered the day it is written. Verified no sub() name collides with a companion-registered slash, so the set cannot swallow a word that genuinely needs the load. The sub() branches that do need the companion (profiler and three others) call EnsureOptionsLoaded themselves, at the point of need.
…gns none UnitGroupRolesAssigned answers 'what role did the GROUP assign', not 'what does this player do'. It returns NONE solo, in the open world, and inside a delve until something forces an assignment. ShouldShowResourceBar mapped that NONE straight onto DAMAGER, so a Holy Paladin standing in Silvermoon was gated by the DPS checkbox: ticking Healers did nothing, and ticking DPS showed the bar for every spec regardless of Show in Solo Mode. In a delve the role only resolved after a spec change. Two copies of the correct fallback already existed (frame border colour, role border colour) with near-identical comments. Promoted to DF:GetUnitRole and routed all three call sites through it. Resolution only, no policy: it can still return NONE, and callers keep deciding what that means -- other units expose no public spec API, so the NONE arm stays for them. Border.lua / Core.lua are behaviour-preserving: GetUnitRole returns nil for a nil or non-existent unit, exactly what their old guard did.
In a delve Brann makes IsInGroup() true but no role is ever assigned, so UnitGroupRolesAssigned stays NONE and UpdateRoleIcon's NONE arm hid the icon for the whole run -- until a spec change made the game assign one, which is exactly what the reporter saw. Resolved through DF:GetUnitRole, but gated on being in a group. Solo the question is not being asked: resolving there would put a permanent role icon on your own frame in the open world, where there has never been one. So this is deliberately NOT the resource bar's policy, which wants the spec role everywhere. The split lives in one place, DF:GetRoleIconRole. ☠ The dirty check in ProcessRoleUpdate had to move with it. That cache decides whether UpdateRoleIcon runs at all; keyed on the raw assignment while the icon displays a resolved role, the resolution is unreachable -- across a delve the raw value never leaves NONE, the cache sees no change, and the icon is never asked to redraw. Both sides now call GetRoleIconRole, so the cache key and the drawn value cannot drift. Left alone: Headers.lua's role SORT still reads the raw assignment. Sorting by the assigned role is its own question and changing it is a separate decision.
The three highlight frames were pinned at owner level +9 / +10 / +11 -- offsets written when a unit frame's children topped out around +4 and anything above the health bar counted as on top. The frame-level rework made every DF element an ABSOLUTE offset from the unit frame, and the stack now runs resource bar 20, contentOverlay 25, status icons 30, missing buff 35, aura rows 40, defensive 65 (verified against Config, not the layer-map doc). At +10 the hover highlight was under all of it. 'The resource bar draws over the hover highlight' is the shallowest instance of that, not the whole of it. Safe to raise because every mode draws at the frame PERIMETER -- SOLID, CORNERS and DASHED are edge lines, GLOW is an edge glow -- so none of this can obscure the name or health text. Relative order is preserved: aggro < hover < selection. 82/84 rather than 78/80: 65 is the highest default and a row's art sits above its baseline by +12 or +14 depending on the element, so the real ceiling is 79. The layer-map doc disagrees with itself on that figure, so these clear it rather than landing on the boundary, where one off-by-one would silently re-bury the highlight. Still far under the movers' absolute 100. Also re-applies strata and level on the REUSE path. The level is absolute and derived from the owner's at the moment it is set, so a highlight created before the owner's level changed kept a stale one for the rest of the session. ⚠ NOT yet verified in game -- needs /df debug zorder against a frame with defensive icons and auras up.
Drag-to-reorder only dropped where you aimed at 100% UI Scale. Reported for Role Priority and Group Display Order; it is the same defect in six widgets, and it is on Live too. GetCursorPosition returns a value you divide by a frame's EFFECTIVE scale to get that frame's coordinates. These divided by UIParent's -- but every one of them then compares against frame:GetTop() / GetLeft(), which is already in the frame's own space, and the settings window carries the user's UI Scale on top of UIParent's. So the two operands were in different coordinate systems, off by exactly that factor: at 140% the cursor reads 40% further from the screen edge than it is, and the drop lands somewhere else. Above and below 100% both break, in opposite directions. Not a new idiom -- the colour picker and the Click Casting keybind popup already divide by their own frame's scale. These sites just did not. DF.GUI:CursorPos(frame) now holds it in one place. It takes the frame deliberately rather than assuming, because the exception is real: the Aura Designer's drag ghost is PARENTED AND ANCHORED to UIParent, so UIParent's scale is the correct divisor there and that site is left alone. 17 sites: the four reorder widgets in Controls.lua (role, class, group, highlight roster), the Nicknames row reorder, and CreateRangeSlider -- which had it on the X axis, so grabbing a handle picked the wrong one and clicking the track jumped to the wrong value.
resolveAppearance is DanderBot#2 in every combat trace and ran once per element per frame per tick. ac55fb8 shared its three fallback tables, leaving exactly one allocation per call -- and that one was 55.7% of all remaining steady-state allocation, because of how often it runs. ac55fb8 declined to share the return table, correctly at the time: the class-colour branch re-resolved the same element just to read one alpha while the outer result was still bound for mirrorElement, so two results were live at once and a shared table would have aliased them silently. That commit removed the second call. The hazard went with it. Verified before sharing rather than trusting that: applyAppearance is the only caller of resolveAppearance and updateOne the only caller of applyAppearance; resolveAppearance is a file-local so nothing outside Render.lua can reach it; mirrorElement reads app.font/fontSize/outline at the point of use and retains nothing; and all five fields are reassigned every call so nothing stale carries over. The invariant and what would break it are written at the declaration.
Reported as an imported profile arriving with raid frames in columns instead of rows, reproducible on every import. The import was never at fault: the value transferred correctly and the post-import refresh re-synced over it. 3b912fb0 added growDirection and growthAnchor to general_frame's Copy/Sync/ Reset prefix list, as part of an audit for keys no section owned. That commit already carried the argument against it, for the 14 raid* layout keys it deliberately left out: per-mode keys that BOTH pages edit must not be owned by one list, because the same list drives Copy, Sync and Reset, so owning them lets party's value overwrite raid's. growDirection is exactly that case and escaped the exclusion only because it is not spelled "raid...". Consequence went well beyond import: with Sync on, raid could never hold a different growth direction from party at all -- any change was overwritten on the next refresh. ☠ Nearly invisible by construction. The sync copies every key under these prefixes, but the two modes agree on almost all of them, so the only evidence is the one key where a user's party and raid genuinely differ. Diffing an imported profile shows 34 of 35 layout keys matching and one wrong, which reads like a single dropped key rather than a rule firing on everything. The raid dropdown inverts the labels for this key (HORIZONTAL reads "Columns" there, "Rows" on party and raid-flat) -- these were never one setting. Audited the rest of that commit and its sibling de06a13: the other seven additions (permanentMover, combatIcon, directBuff, directDebuff, defensiveBar, useFrameSort, and the Fading renames) are all keys only ONE page edits, so they carry no cross-mode hazard and stay.
DF:SafeSetFont was 40.9% of all boss-fight allocation and 12% of trash, on two
allocations per call -- and it runs once per FontString per frame, twice over
for any element carrying an AD mirror.
1. outline:match("^SHADOW;(.*)$") -- string.match WITH A CAPTURE allocates the
captured substring every call. The parse is a pure function of the raw
outline string and those strings come from a handful of dropdown options, so
it memoises once and never goes stale. The SLUG concat caches with it, since
that variant derives from the parsed flags alone; toggling fontSlug just
selects between two precomputed strings.
2. FontFamilyKey built a four-part concat to PROBE the family cache, so it ran
on every hit, not just on a miss. Memoised through a nested table: three
small tables per new (font, outline, size, shadow) combination and nothing
after. The key stays a string, so fontFamilies is still string-keyed and the
broken-family eviction in SafeSetFont still finds its entry.
Four steady-state allocators, 16.9% of trash-fight allocation between them. GetDurationColorBreakpoints (6.6%) rebuilt the whole ladder per aura per tick -- a list table, a table per breakpoint, and a colorToHex string per breakpoint -- from account-wide config that only changes on a Colours-page edit. Memoised per scale. Verified all five callers only read it before sharing the list. Keyed on the resolved def table so an unknown scale shares TEXT_SECONDS' entry rather than duplicating it. Invalidation rides DF:InvalidateDurationFormatters, which every breakpoint edit already fires and whose comment already made it mandatory -- and PERCENT now caches too, where the signature memo only ever covered SECONDS. subSig (2.1%) allocated a keys table and a parts table per call, inside syncPlacedPool -- a walk whose whole purpose is to be allocation-free. ☠ It RECURSES, so the scratch is PER DEPTH: a single shared pair would let a nested call wipe its caller's half-built list. Depth is hard-bounded at 4 by the existing guard, so one pair per level is sufficient. placedBorderRawSig (4.8%) got a single shared scratch -- it cannot recurse, only reaching colSig and ppSigToken, neither of which calls back into it. colSig (3.5%) built a four-element array just to tconcat it. A chained concat compiles to one concat over a register range, so the result string is now the only allocation.
…nged 22.4% of trash-fight allocation and 9.7% of boss. It is target-switch driven, so it re-ran the full hide-everything-then-re-apply pass on every target change even when the resulting art was identical. Deferred once already because an early-out here is not the two-line change it looks like. Four hazards, all handled: 1. ☠ THREE IMPLICIT INPUTS beyond the eight parameters. ch:GetEffectiveScale() (thickness and inset are pixel-snapped against it), ch:GetWidth()/GetHeight() (CORNERS derives cornerLen from them), and db.pixelPerfect. Miss one and the art goes stale with nothing on screen to explain it. The scale read for the signature is the SAME local the snapping uses, so what is cached and what is applied cannot disagree. 2. ☠ AN EXTERNAL WRITER. DF:LightweightUpdateHighlight writes size, points and colour straight onto the four line textures during a slider drag, bypassing this function entirely. It now invalidates first, or the next full update would see a matching signature and leave the drag values in place forever. 3. ☠ THE ANIMATOR. The three not-wanted branches call SelectionAnimator_Remove. Turning an ANIMATED highlight off and back on with identical settings would have early-outed and never re-added it -- an animation that silently stops. All three invalidate. 4. Fields are compared individually rather than through a string key: building a key would allocate exactly what the early-out exists to avoid. Swept for other writers of topLine/bottomLine/leftLine/rightLine outside Highlights.lua -- LightweightUpdateHighlight is the only one.
395af09f memoised the 37 db-key strings but left one allocation: k captured memo and prefix, so a closure was constructed on every call. BuildSpec runs per bordered element per tick -- 4.6% of trash allocation and 4.3% of boss. ☠ The upvalues are shared, safe only because BuildSpec cannot re-enter. Checked its body rather than assuming: it reaches DF:GetClassColor, DF:GetTestUnitData, DF:GetUnitRole and the Border:Resolve* helpers, none of which call BuildSpec. Recorded at the declaration, because if a resolver ever does build a spec this starts reading another prefix's keys and produces a wrong border with nothing at the call site to explain it. Also noted while measuring, not a change: DF:GetDB shows at 3.5% of trash CPU but is two branches and a table index -- that is Perfy's ~1us per-call instrumentation, not real work. Same caution applies to any tiny function high in the CPU list; optimising those would be optimising the profiler.
Two per-call allocators on the proc/animation paths -- 3.0% of trash allocation
and 5.6% of boss.
C_Texture.GetAtlasInfo RETURNS A FRESH TABLE every call, and setupProcGlow asked
for two of them per call. Both atlases are compile-time constants, so the info is
fetched once and shared. Resolved lazily, not at file scope, because C_Texture
may not be up when this file loads -- and the latch only sets once the API
actually answers, or a nil before the texture system is ready would cache "no
atlas" for the session and the proc glow would never draw again.
Safe to share: swept _procAtlas / _procStartAtlas and every consumer only reads
(.file, and stepProcFlipbook's frame maths).
Seven sites wrote readColor(anim.color or { ... }) with the table inline, so any
animation with no colour set built a throwaway on every driver tick. Hoisted to
two shared constants. readColor only reads its argument and nothing retains it.
A raid header keeps its whole child set alive and hands units out as the roster fills, so outside a full group most children carry no unit at all. UpdateAllDispelOverlays walks IterateAllFrames unfiltered, so every one of those empty slots got a full six-slot CustomAuraContainer stood up for it -- hidden, unable to ever match a dispellable aura, and (because Create defaults a nil unit to "player") pointed at the wrong unit. Measured at login while solo: 41 containers built, 40 of them empty raid slots. That is 10.8 MB of the 14.1 MB the addon spent standing containers up, plus 3.3 MB of BindDispelCarriers closures -- ~76% of container construction for frames nobody can see. The aura rows never paid this because they come through FullFrameRefresh, which already returns on a unitless frame; every other IterateAllFrames consumer (StatusIcons x3, Headers, Core, FindFrameByUnit) gates the same way. This was the only unguarded one. Existing handles are deliberately left alone: the roster churns, and SetUnit re-targets a live one for free when a unit lands in the slot.
Two independent construction costs. bindNative ran twice per BUTTON: a pcall wrapper closure, and a fresh textColor table whenever a colour curve is configured. It was the single largest profile-switch item at 24.9 MB. The colour pair now caches on the config by spec identity -- the same pattern _dfDurBind two lines above already used, invalidated for free by the fresh style table a structural Rebuild hands over -- and the pcall passes its args instead of capturing them, matching the AddAuraGroup call in build(). SetTestMode's rebuildAll walked every handle. A container genuinely has to be rebuilt across a test transition (the two shapes declare different groups and groups can never be removed), but that only ever applied to the handles that RENDER the preview. Live frames are hidden for the whole session by SetTestModeStateDrivers and their containers are built deaf, so they ignore the provider bounce and sit on real data untouched from entry to exit -- rebuilding them into test shape and back bought nothing. Provenance is resolved once at Create by a bounded walk up the anchor's parent chain for dfIsTestFrame, which TestFramePool and CreatePlayerTestFrame both stamp at frame creation, long before any row is driven onto them. Note the entry pass is NOT redundant, despite reading that way: the test frame pool is created once behind testFramePoolInitialized and its frames are only hidden on exit, so from the second entry onward the test handles are present and do need the rebuild.
AddAuraGroup hardcodes a frame batch and creates it BEFORE maxFrameCount is applied, so a group that can only ever show one icon still paid for a whole batch. AddAuraSlot creates exactly one frame and builds the same comparator from sortMethod/sortDirection, so a max=1 group and a slot pick the same winner. Seven Aura Designer configurations showed a single icon and created a batch each. The engine grows a singleSlot row mode -- one AddAuraSlot pinned to the container's flow anchor, carrying the row's padding so the duration strip keeps its reservation -- and those seven opt into it. Falls back to the group path if a config claims singleSlot with anything other than exactly one filter.
Reviewing the pass end to end turned up four things it had broken or left half-done: - Highlights: ch:Show() sat BELOW the _hlSig early-out, so a container whose signature was unchanged never got shown again -- the selection border vanished after leaving Test Mode. - Single-slot rows dropped the duration-strip reservation, so a row with duration text lost its bottom padding. - Border latched the proc atlas on the first of two atlas lookups, so a missing second atlas left the latch claiming resolved. - The test-mode bounce re-enabled every handle it had disabled, including live ones, undoing the rebuild scoping on the way back out.
Every widget hung off a dispel slot button -- the overlay frame, its border StatusBars, the bound carriers -- is a descendant of the secret aura button, and the client can turn that subtree forbidden when it reclaims or re-initialises the slot. btn.dfDispelWidget was never cleared on any path, so the stash outlived the art and ApplyOverlayLayout's first touch (borderLeft:ClearAllPoints) threw on every state-changing refresh. The throw aborted StyleGameMainSlot, so the whole overlay died on that frame, not just the border geometry. Reported on alpha 14 at 32x after a dungeon fight and 98x on a single Show Overlay For dropdown -- same frames, once per refresh, so it persists rather than clearing itself. Same defect class as bug #1004 (the health mirror, 999x in a follower dungeon); Frames/Core.lua already carries this guard for that consumer. StyleDispelSlots now probes the stashed art before anything paints it and, on failure, drops every stash on the button and re-runs DispelSlotSecureInit to rebuild and re-bind. One retry per button, latched, so art that comes back untouchable degrades to a skip with a single debug line instead of rebuilding every pass; the latch clears as soon as a probe passes, so a later reclaim is still recoverable. The probe is read-only and touches borderLeft -- the object that actually throws -- rather than IsForbidden(), which inside the secret container can hand back a secret on healthy art and would read as "skip" for everyone. NOT verified in game: needs the #1004 repro (follower dungeon, roster churn) to confirm the rebuilt art is touchable and the overlay comes back.
The v5 lane rebuilds the aura row anyway, so the "don't change a look every user already has" reason for shipping it off no longer applies. Party and raid both pick it up -- RaidDefaults is derived from PartyDefaults and the key is not in RAID_DEFAULT_OVERRIDES. Only the shipped default moves. Profiles are a deep copy of the defaults taken at creation (Core/Profile.lua:59), so existing profiles keep their stored false; this changes new profiles only.
The duplicate-icon known issue ('a debuff that is both Priority and Boss/Role
can show one icon per matching filter') no longer describes the code. The
priority record subtracts boss/role via its own candidateFilters flag, and the
crowd-control, raid and dispel records subtract both -- every record is mutually
exclusive by construction, in category mode and in Show All alike.
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.
Everything on the 12.1 lane since
v5.0.0-alpha.14: a profiling pass across the addon, the fixes that came out of alpha testing, and one default change.40 commits, 30 files, +2115 / −446.
Performance
The bulk of the diff. Every change is the same shape — reuse work instead of redoing it — and each was measured before and after against the same scenario rather than reasoned about.
Aura rows tune in place. Changing a filter, sort order or icon cap used to tear the container down and rebuild it. The Aura Designer's placed indicators, bar and expiry-alert companions, overlay indicators, and the engine's overlay containers now push the change through the slot setters instead.
Strings and tables that were being rebuilt every frame are now cached. Border spec keys, the font outline parse and family cache key, the Text Designer's appearance resolver (which was also being called twice and throwing one result away), its Live data source, and the duration ladder the aura and Aura Designer signature builders walk.
The highlight pass skips when nothing that affects it changed, instead of restyling on every refresh.
Test mode was giving each preview slot its own aura group. A group eagerly creates a batch of frames before
maxFrameCountis applied, so a preview showing five icons paid for far more than five. Rows that can only ever show one icon now useAddAuraSlot, which creates exactly one frame and builds the same comparator fromsortMethod/sortDirection— so a max=1 group and a slot pick the same winner. Seven Aura Designer configurations were in that position. Leaving test mode also stopped rebuilding every live handle alongside the preview ones.The dispel overlay was building containers onto empty raid slots. A raid header keeps its whole child set alive and hands units out as the roster fills, so outside a full group most children carry no unit. Login built 85 containers; 45 were dispel overlays and 40 of those sat on empty slots — hidden, unable to ever show a dispellable aura, and pointed at the wrong unit because a nil unit defaults to
player. Now 45 containers with none wasted; secure slot inits fell 270 → 30.Measured
Three builds, five scenarios each, folded from full traces. Indexed against the shipping 4.10 build:
Roughly half the drop came from the port and half from this pass.
Before and after the pass — both on 12.1, both on the
Testprofile, MB and ms:12.1 against the live build — both on the
Defaultprofile, which is byte-identical across the two lanes:These are addon-side Lua only. The profiler cannot see Blizzard's C++ container work, and 12.1 deliberately moves aura work there while 4.x did the equivalent in Lua where every byte is counted. So this is half the addon-side Lua cost, not half the total work. It is still the number that matters for hitching — Lua allocation is what drives garbage collection.
Two other caveats worth carrying: the
Defaultprofile exercises the core frame path only (no Aura Designer indicators, no Targeted List, no proc glow, on either lane), and part of the 12.1-vs-retail gap is the load-on-demand split and the border and font memoisation rather than the container port — those are portable back to 4.x.Fixes
/df debugdiagnostics for the main addon no longer load the settings addon.Default change
Important Debuffs is now on by default. The v5 lane rebuilds the aura row anyway, so the "don't change a look every user already has" reason for shipping it off no longer applies. Only the shipped default moves — profiles are a deep copy of the defaults taken at creation, so existing profiles keep their stored value and this affects new profiles only.
Changelog
Covered the work above, and removed one Known Issue that is no longer true:
The priority record subtracts boss/role through its own
candidateFiltersflag, and the crowd-control, raid and dispel records subtract both. Every record is mutually exclusive by construction now, in category mode and in Show All alike.The other five Known Issues were each checked against the code and all still hold — the overlapping dispel type icons are still a documented trade-off in
StyleGameTypeIconSlot, the Aura Designer's colour-by-cover mechanism is unchanged, and the settings-window border derivation was not touched by the hit-testing fix.Testing status
Verified in game: the perf work (five scenarios, five passes each, plus a same-profile comparison against the 4.10 build), test mode, and the Edit Mode behaviour.
Not yet verified in game: