diff --git a/docs/ADDON_COMPAT.md b/docs/ADDON_COMPAT.md index 29d5d8e2..b0133d81 100644 --- a/docs/ADDON_COMPAT.md +++ b/docs/ADDON_COMPAT.md @@ -4156,3 +4156,207 @@ raising does not stop the registry walking the rest. `securecallfunction`, `secu takes a global's name), `issecure` and `forceinsecure` now live beside `seterrorhandler` in `stdlib.rs`, with failures going to `geterrorhandler()` exactly as the reference's own secure dispatch does — so an addon bug still reaches the log rather than being swallowed. Back to 86/87. + +## 2026-11-05: five defects from one play session, and what each turned out to be + +### A saved-variables file full of `nil` is a file that deletes the addon it belongs to + +grom: *"I'd also like to get advanced trade skill window2 working … I can't even get number 2 to +load right now."* His log had three runtime errors from it, all the same shape: + +``` +AdvancedTradeSkillWindow2.lua:844: attempt to index global `ATSW_Profession' (a nil value) +AdvancedTradeSkillWindow2.lua:5931: attempt to index global `ATSW_Bags' (a nil value) +AdvancedTradeSkillWindow2.lua:5990: attempt to index global `ATSW_Bank' (a nil value) +``` + +All three are assigned `{}` **at file scope** — lines 294, 5929 and 5986 — and the chunk ran: the +loader logged all five files, and the traceback has `ATSW_OnEvent` (line 2504) calling +`ATSW_ShowTrainingPoints` (4269), both past the first of them. So the tables existed and were +taken away, and `benilla-config/saved/AdvancedTradeSkillWindow2.lua` says by what: **19 of its 24 +declared names stood at `NAME = nil`.** + +An addon's saved file is executed **after** its own Lua (`0x51f4b5`, "after the files set their +defaults"), which is what makes a `nil` line destructive rather than merely useless — in a fresh VM +a global is already nil, so the line carries nothing and can only delete. And once written it is +self-sustaining: the nil it forces is what the next shutdown reads back and writes again, so one +bad write is permanent. The reference does not write them, which is why no real `SavedVariables` +file has ever had a `= nil` line in it. + +Two halves, because the first alone would still cost a session: the per-addon writer skips a nil +global (the **flat** `RegisterForSave` file still writes them — it is read *before* FrameXML, where +a nil line and an absent one mean the same thing), and the loader drops `NAME = nil` lines a +previous build already wrote, matched narrowly against that addon's own `## SavedVariables` set. It +is not one addon: his install carried poisoned files for **five** — ATSW2 (20 lines), BigWigs (4), +ImprovedErrorFrame (2), MasterTradeSkills (1), SuperCleveRoidMacros (1). + +With it fixed, every one of ATSW2's 24 variables is a table at the first handler, and +`ATSW_ShowTrainingPoints()` — the function that raised fifteen times — returns. + +### A pfUI skin that was skinned as far as line 63 + +`pfUI-addonskinner`'s skins run at `VARIABLES_LOADED`, and most of ATSW2 does not exist then: +`ATSW_CreateListButtons` builds the recipe rows, the tool buttons and the reagent buttons, and +`ATSW_Show` calls it behind an `InitializedOnShow` flag — so the first time `ATSWReagent1` exists +is the first time the player opens a profession. The skin's own line 64 is +`local size = btn:GetHeight()` over that button, `addonskinner:Skin` pcalls each skin, and so the +raise was swallowed and **everything below that line was never applied**: the scrollbars, both +dropdowns, the four popups, the Custom Sorting window — and the profession tabs, which are version +2's one addition over version 1 and the thing grom actually asked about. + +The body is now a function behind `if not ATSWReagent1 then return end`, re-applied from a +`hooksecurefunc` on `ATSW_CreateListButtons`; the hook is installed *before* the first call, +because that call is inside the skinner's pcall and a raise there would take the registration with +it. The tabs get pfUI's own skill-line-tab treatment, the one the spellbook's column already uses +(`pfUI/skins/blizzard/spellbook.lua:42-59`): strip the vanilla `SpellBook-SkillLineTab` plate, skin +the button around its own normal texture (which is the profession's icon), inset the texcoords, and +stack the column at the border's gap. Re-run on `ATSW_SelectTab` and `ATSW_AttachTabsTo`, because +`ATSW_RefreshTabs` re-textures a tab on every refresh and the column is re-anchored when the window +changes page. + +Measured before the first open: 8 tabs, 0 skinned. After: **8 of 8 skinned**, and +`ATSWReagent1`, `ATSWCSFrame`, `ATSWShoppingListFrame`, `ATSWConfigFrame`, `ATSWReagentsFrame`, +`ATSWSearchHelpFrame`, `ATSWBuyNecessaryFrame` and `ATSWRankFrame` all carry their backdrop. + +### The player arrow was the one piece of the map left outside the canvas + +grom: *"my player arrow goes to the wrong spot at any zoom level, this needs to be accurate all of +the time."* + +`WorldMapFrame_OnLoad` builds the arrow with `CreateWorldMapArrowFrame(WorldMapFrame)` +(`WorldMapFrame.lua:19`), so its parent is the **window**, while `MapCanvas.lua` reparents +everything it is anchored against into the canvas. `SetPoint`'s offsets are in the positioned +frame's own units, so the offsets the stock update computes — `playerX * GetWidth()`, times +`WorldMapDetailFrame:GetScale()`, which `SeatOnCanvas` pins at 1 because the zoom lives one level +up on `ScrollContainer.Child` — are short by exactly the canvas zoom. Measured on a 1600x900 +harness with the view centred on the player: exact at zoom 1, **250.5 x 501.0 screen px out at +zoom 2**. + +`WorldMapPlayer` — the invisible mouseover rect for the same spot, set from the same two FrameXML +lines — has always been right, because 1.12's XML nests it under `WorldMapButton`, which **is** in +the canvas. That makes it the oracle the test uses rather than a recomputed expectation, and the +new test walks zoom 1, 1.5, 2.5 and 4 asserting the two agree within a pixel. + +Not reparented like the detail frame and the button: the arrow is a size-less `Model` whose rect is +the arrow file's bounding box, so inside the canvas it would grow with the zoom, and there is no +frame scale that fixes the size without undoing the position — both read the same effective scale. +The cost of staying outside is clipping, hence an explicit bounds test, which is a hide the stock +code never needed because a map that cannot pan cannot put the player off-screen. + +That test is arithmetic on the controller's own normalized scroll, and the art's size is read off +`WorldMapButton` rather than the detail frame, because **this runs every frame the map is open**: +the detail frame is `SetAllPoints` to the canvas so its width is derived, `GetLeft` on a dirty tree +measures 30-370 us against 0.42 us settled, and a pan writes the scroll every frame. + +While in there: `InstantPanAndZoom` set `Child:SetScale(scale)` from its raw argument and +`GetCanvasScale` clamps to the zoom ladder, so a scale past the ladder's ceiling drew the art at +one scale while every extent, pan clamp and pin placement was computed for another. It clamps +first now. + +### The map window fell out of its own stratum every time it opened + +grom: *"the map needs to be on a higher level as I can see my unitframe over it."* + +`WorldMapFrame.xml:5` declares `frameStrata="FULLSCREEN"`. Measured on his install: **FULLSCREEN +with no addons, MEDIUM at level 0 with his** — the bottom of the same bucket pfUI's unit frames +live in (`pfUI/api/unitframes.lua:413`). Five visible frames drew over the map rect, and every +MEDIUM frame tied with it. + +No addon lowers it. A `WOW_WATCH_STRATA=` instrument (new, beside `SetFrameStrata` and +`SetParent`, printing the Lua caller) caught one `SetFrameStrata("FULLSCREEN")` from the loader and +then the real culprit: `WorldMapFrame:SetParent(UIParent)`, from +`WorldMapFrame_Minimize` (`WorldMapFrame.lua:183`) by way of `WorldMapFrame_OnEvent` — the realm's +own FrameXML, on the open path. A reparent forces the moved frame into its new parent's stratum +(`0x76ab5a`, pinned by `script::tests::setparent` off the binary), and `UIParent` is MEDIUM. + +The reparent is right and stays; the window's own declared stratum is re-asserted instead, on +`SeatLevels` — which already re-seats the frame levels on every show for the same kind of reason — +plus hooks on `WorldMapFrame_Minimize`/`_Maximize`, since the minimize runs while the window is +already up. After: FULLSCREEN, and **0 visible frames over the map**. + +### Fade the map while moving + +grom: *"It would also be nice if the whole map window went to 80% transparentcy when the player is +moving."* `mapFadeWhileMoving` is the percent, 0 off, default 80, with a slider on the Options +window's Interface page; 1.12 has no such behaviour, because 1.12's map cannot be read while +walking either way. + +Written as a **factor** of whatever alpha the window already has, not as an absolute, because that +alpha belongs to somebody else: pfUI sets it from its own saved config and offers shift-wheel over +the map to change it (`pfUI/modules/map.lua:67`). The base is re-read whenever the live alpha is +not the one this last wrote, which is the signature of another writer — so their setting is what +gets faded rather than what gets overwritten, and closing the map restores it. + +`GetSpeed()` is the movement test: 1.12 has no "is the player moving" verb, and that one is ours, +always present, and 0 standing still by construction — the same move flags the engine answers +`IsSwimming` and `IsMounted` from, so it cannot disagree with them about the same frame. + +### `Dismount` and `CancelShapeshiftForm`, because our own port calls them + +`pfUI/modules/autoshift.lua:45` answers `ERR_CANT_INTERACT_SHAPESHIFTED` with +`if IsMounted() then Dismount() else CancelShapeshiftForm() end`, and grom's log carries +`attempt to call global 'Dismount' (a nil value)` from exactly there — a port written against verbs +this client did not have. + +Both were on `compat/classic_api/state.rs`'s not-built list for a reason that turned out to be +answerable: each needs the **spell id** behind a state the engine reports as a flag +(`UNIT_FIELD_MOUNTDISPLAYID`) or an index (`SelfMovement::shapeshift_form`), and the player's aura +list carries spell ids and nothing about what each aura *is*. The missing half was one derivation — +`EffectApplyAuraName` 78 (`SPELL_AURA_MOUNTED`) and 36 (`SPELL_AURA_MOD_SHAPESHIFT`), now on +`AuraState` beside `channeled`, which is derived the same way. Each verb is then the aura it names +cancelled by id, which is what the reference's own right-click on the buff does. + +Not gated on `aura::cancel_authorized`, unlike `CancelPlayerBuff`: that gate is the reference's +check inside *that* verb, the server makes its own decision about a cancel it is sent, and a mount +whose aura is not flagged `AFLAG_CANCELABLE` would otherwise be undismountable with no way to tell +why. + +### SoftSpoken read the same zone line over itself, and the cut it spends was a frame late + +grom: *"softspoken was repeating zone readouts over and over unless I manually removed them from +queue."* + +1.12 has no verb that stops a playing sound, so every voice-over addon uses the one primitive there +is: `SetCVar("MasterSoundEffects", 0)` then `..., 1)`, whose *enabling* edge walks the live instance +list and stops each node (`0x457500` → `0x7a6570` → `0x7a67a0`). SoftSpoken's +`SoundUtils:StopSound` is exactly those two lines, and its `SoundQueue` spends them twice over: to +duck the game's NPC dialogue while it speaks, and — this report — to stop its **own** clip before +replaying one, which `StopKeeping` does and comments on ("it replays from the start when Advance +next reaches it, since the client has no seek"). + +We had made the toggle stop things, and then had to exempt `PlaySoundFile` audio from it because +stopping that silenced SoftSpoken outright: the addon spends the toggle and then plays its own +file, so a cut that reached files took the very line the toggle made room for. That exemption is +why `StopKeeping` stopped nothing, and the doc for it said as much — *"if the overlapping-replay +symptom returns, this is why."* It returned. + +The ordering was the thing, not the exemption. The reference stops **inside** the second `SetCVar`, +between the addon's two calls; ours was armed by the `CvarChanged` observer, which fires at a +command flush a frame after the Lua pass that wrote it — by which time `drain_ui_sounds` has +already started the new clip. So the cut now rides the **sound queue** as a `SoundRequest::CutEffects` +pushed from the one CVar store that sees both the old and the new value, and the drain spends it in +call order: cut, then play. That path stops files; the deferred observer path still spares them, +because a cut that late can only hit the new one. Both answers are in +`kit::stopped_by_reenable`, and both were paid for by a report. + +### What the map's own frame cost measures, with nothing found + +grom: *"The map is also running pretty laggy even with the addon off."* Measured, and I cannot +reproduce it. `ui-worldmap` with a driver panning and zooming the canvas every frame: + +| | mean | p50 | p95 | p99 | max | +|---|---|---|---|---|---| +| 1100x800, no addons | 2.90 | 2.90 | 3.24 | 3.47 | 3.81 | +| 1100x800, his addons | 6.41 | 5.27 | 6.92 | 11.06 | 491.75 | +| 2560x1440, his addons | 5.43 | 5.30 | 6.21 | 6.71 | 21.04 | + +The UI pass is 2.9 ms of the 1440p frame over 3,806 quads (convert 854 us, Lua tick 734, resolve +657, extract 381, mesh 235), one layout solve a frame, and 0 glyph-sheet resets. The 491 ms outlier +is a single `pipeline compiled LIVE` — a shader lane the warm menagerie has no arm for — and not +the map. + +His own session agrees: 11 `frame hitch:` lines in an hour, every one of them at world entry +(`quest_markers`, `redress_player_looks`, `terrain_stream`, `glue_booth`, `lava_light`), the three +largest being the loading screens, and **zero** live pipeline compiles. So the thing to pin down +next is which "laggy" it is — a sustained cost, or motion trailing the input, which would be the +zoom ease's 0.29 s half-life rather than a frame cost.