diff --git a/crates/benilla-ui/src/script/compat/nampower.rs b/crates/benilla-ui/src/script/compat/nampower.rs index 94071ae1..095b67ad 100644 --- a/crates/benilla-ui/src/script/compat/nampower.rs +++ b/crates/benilla-ui/src/script/compat/nampower.rs @@ -984,27 +984,31 @@ pub(in crate::script) fn install(lua: &Lua) -> mlua::Result<()> { })?, )?; - // `GetItemIconTexture(itemId)` (v2.14+): the item's inventory icon as an extensionless - // `Interface\Icons\…` path, the sibling of `GetSpellIconTexture` above. Two hops, because an - // item template carries a *display* id and not an icon: the cache row's `displayInfoID`, then - // `ItemDisplayInfo`'s own `Icon` column through `Model::item_icon_probe`. + // `GetItemIconTexture(displayInfoId)`: the icon of an `ItemDisplayInfo` row, as an + // extensionless `Interface\Icons\…` path. The exact sibling of `GetSpellIconTexture` above — + // **both take a raw DBC row id, not the id of the thing that points at the row.** // - // An item the cache has never seen raises "Item not found in DBCache", exactly as the other - // `item_record` verbs here do — the caller's `pcall`-or-nil-check is the same either way, and - // the ask is recorded so the next call can answer. A known item whose display row is missing - // is **nil**: that is a gap in the data, not an error in the call. + // The argument is the `displayInfoID` an item's cache row carries, which a caller reaches with + // `GetItemStatsField(itemId, "displayInfoID")`; the DLL's own usage string says so + // (`Usage: GetItemIconTexture(displayInfoId)`), and `SuperCleveRoidMacros` keeps the two hops + // apart in exactly that shape: `API.GetItemIconTexture(displayInfoId)` is a pass-through, and + // the item-keyed convenience is a *different* wrapper above it, + // `API.GetItemIconTextureByItemId(itemId)`, which looks the display id up first + // (`NampowerAPI.lua:2648` and `:2665`). + // + // An id with no row is nil — a gap in the data, not an error in the call. Nothing is asked of + // the item cache here at all, so there is no "Item not found in DBCache" raise and no stat ask + // recorded: a display id is not an item id, and treating one as the other is how this verb + // read until the DLL's usage strings were compared against it. lua.globals().set( "GetItemIconTexture", - lua.create_function(|lua, item_id: Option| { - let display_id = item_record(lua, item_id)?.display_info_id; + lua.create_function(|lua, display_id: Option| { let model = lua .app_data_ref::() .expect("model app_data"); - Ok(model - .item_icon_probe - .as_ref() - .filter(|_| display_id != 0) - .and_then(|probe| probe(display_id))) + Ok(display_id + .filter(|&id| id != 0) + .and_then(|id| model.item_icon_probe.as_ref().and_then(|probe| probe(id)))) })?, )?; @@ -1272,47 +1276,48 @@ mod tests { ); } - /// `GetItemIconTexture` takes the two hops an item icon needs — the cache row's display id, - /// then `ItemDisplayInfo` — and tells a data gap apart from an unknown item: a known item - /// whose display row is missing answers nil, an item the cache has never seen raises, as the - /// other `item_record` verbs here do. + /// `GetItemIconTexture(displayInfoId)` reads an `ItemDisplayInfo` row id, **not** an item id: + /// the DLL's own usage string says so, and the addon layer keeps the item-keyed hop in a + /// separate wrapper of its own (`API.GetItemIconTextureByItemId`). An id with no row is nil. + /// + /// The literals matter: `7777` is a display id the probe answers for, and `123` is an item id + /// whose display id is `7777`. Passing the *item* id must answer nothing — that is the whole + /// bug this test pins, and it is why the item cache is populated here at all. #[test] - fn get_item_icon_texture_resolves_through_the_display_id() { + fn get_item_icon_texture_reads_a_display_row_not_an_item() { let mut s = UiScript::new().unwrap(); s.set_item_icon_probe(Box::new(|display_id| { (display_id == 7777).then(|| "Interface\\Icons\\INV_Sword_04".to_string()) })); - // Nothing cached: the same raise every item verb here makes. - assert!(s - .eval::>("return GetItemIconTexture(123)") - .is_err()); + s.set_item_template( + 123, + crate::script::ItemTemplateView { + display_info_id: 7777, + ..Default::default() + }, + ); - for (id, display) in [(123u32, 7777u32), (124, 4242), (125, 0)] { - s.set_item_template( - id, - crate::script::ItemTemplateView { - display_info_id: display, - ..Default::default() - }, + assert_eq!( + s.eval::>("return GetItemIconTexture(7777)") + .unwrap(), + Some("Interface\\Icons\\INV_Sword_04".to_string()), + "the display row id is the argument" + ); + // The item id that points *at* that row is not the argument, and asking with one answers + // nothing rather than quietly resolving it: `GetItemIconTextureByItemId` is the caller's + // own hop, and an item cache is not consulted here at all. + for call in [ + "GetItemIconTexture(123)", + "GetItemIconTexture(4242)", + "GetItemIconTexture(0)", + "GetItemIconTexture()", + ] { + assert_eq!( + s.eval::>(&format!("return {call}")).unwrap(), + None, + "{call}" ); } - assert_eq!( - s.eval::>("return GetItemIconTexture(123)") - .unwrap(), - Some("Interface\\Icons\\INV_Sword_04".to_string()) - ); - assert_eq!( - s.eval::>("return GetItemIconTexture(124)") - .unwrap(), - None, - "known item, missing display row: a data gap, not an error" - ); - assert_eq!( - s.eval::>("return GetItemIconTexture(125)") - .unwrap(), - None, - "display id 0 is not a row" - ); } /// nampower's three posture reads and `IsAuraHidden`, in the shapes its own doc lines give: diff --git a/docs/ADDON_COMPAT.md b/docs/ADDON_COMPAT.md index 4c0872c3..15f3010d 100644 --- a/docs/ADDON_COMPAT.md +++ b/docs/ADDON_COMPAT.md @@ -487,6 +487,59 @@ the events `SPELL_FAILED_SELF`, `SPELL_QUEUE_EVENT`, `UNIT_DIED`, `UNIT_HEALTH_G called is not proof an addon needs it — most sit behind an `if GetTrinkets then` — so the next step is reading each call site, not implementing the list. +## The registration table — a complete list, where the pointer filter gives a candidate list + +`scripts/dll-surface.py` above answers "what might this DLL publish". For `nampower.dll` there is a +**better** question available, and it has an exact answer: the names it registers sit **contiguously +in the string pool, in registration order, bracketed by the DLL's own log lines**. + +``` +$ strings -a -t x nampower.dll | awk '{a=strtonum("0x" $1); if (a>=0x7c6a8 && a<=0x7cb50) print}' + 7c6a8 Registering Custom Lua functions + 7c6cc QueueSpellByName + 7c6e0 CastSpellByNameNoQueue + ... + 7cad0 ExecuteCustomLuaFile + 7cae8 GetQuestLogQuestIds + 7cafc GetQuestDialogQuestId + 7cb14 Registering Glue Lua functions + 7cb34 EncryptPassword + 7cb44 EncryptedServerLogin +``` + +That is the whole Lua surface: **60 functions plus 2 glue-screen ones**, no inference. Immediately +before it, the same way, is the **event** table (`0x7c198`–`0x7c658`, after the `Added custom event +code:` line): 59 names, from `SPELL_QUEUE_EVENT` to `SPELL_DISPEL_BY_OTHER`. Immediately before +*that*, after `Registering/Loading CVars`, the CVar names. + +Why this works where the pointer filter only narrows: a compiler emits one string literal per +`lua_register(L, "", fn)` call and pools them in source order, so a single registration +function's worth of names lands in one contiguous run. The log line that opens the run is the +giveaway — it is the literal immediately before the first name, because it is printed immediately +before the first call. **Look for the log line, then read forward until the strings stop looking +like identifiers.** Nothing else was needed, and it is strictly better evidence than "the image +points at this string": a statically linked library's `BlueValues` cannot land inside the run. + +Two other things fall out of the same pool, and both corrected code that was already written: + +- **Usage strings are the argument contract.** `strings -a nampower.dll | grep '^Usage:'` returns 44 + of them, each the exact spelling the DLL prints when a call's arguments are wrong — e.g. `Usage: + GetSpellModifiers(spellId, modifierType) - modifierType: 0=DAMAGE, 1=DURATION, …` enumerates all + 26 modifier constants, and `Usage: GetSpellRangeData(rangeIndex)` says that verb takes a + `SpellRange.dbc` **row index** and not a spell id, which this file previously had wrong. +- **`GetItemIconTexture` took the wrong argument here, and the usage string is how that was found.** + `Usage: GetItemIconTexture(displayInfoId)` — an `ItemDisplayInfo` row id, the exact sibling of + `GetSpellIconTexture(spellIconId)`. Ours resolved an *item* id through the cache to its display id + first, which is a different function: upstream's addon layer keeps that hop in a wrapper of its + own (`NampowerAPI.lua:2648` is the pass-through, `:2665` is `GetItemIconTextureByItemId`). Fixed, + with the test now pinning that an item id answers **nothing**. + +Three of the four field-name blocks are in the pool too, in table order, which is how +`GetCastInfo`'s ten fields and the cooldown table's twenty were read without guessing: `castId`, +`spellId`, `guid`, `castType`, `castStartS`, `castEndS`, `castRemainingMs`, `castDurationMs`, +`gcdEndS`, `gcdRemainingMs` at `0xa8854`, then `isOnCooldown` … `isOnGcdCategoryCooldown` at +`0xa88d4`. + ## The DLL work list, name by name — `scripts/dll-surface.py`'s output, worked The script above enumerates what the DLLs publish; this is what came of reading each name's call @@ -499,7 +552,7 @@ is to define the global. Read a call site before implementing: the first pass of 25 names as "unguarded" that were doc comments and table keys inside that registry, and pfUI's `GetCastInfo` is its own file-local function (`modules/nameplates.lua:105`), not nampower's. -**Done (16).** `PlayerIsMoving`, `PlayerIsRooted`, `PlayerIsSwimming`, `IsAuraHidden`, +**Done (22 as of the round that wrote this, and the Built row below is the live count).** `PlayerIsMoving`, `PlayerIsRooted`, `PlayerIsSwimming`, `IsAuraHidden`, `GetItemIconTexture`, `GetTrinkets`, `FindPlayerItemSlot`, `GetBagItem`, `GetItemIdCooldown`, `UseItemIdOrName`, `CancelPlayerAuraSpellId`, `CancelPlayerAuraSlot`, `CastSpellNoQueue`, `CastSpellByNameNoQueue` (nampower), `GetQuestIDForLogIndex`, `GetQuestLinkForLogIndex` (SuperWoW). @@ -526,7 +579,7 @@ because it is in the same family — it would flush our own `WoWCombatLog.txt`, |---|---| | `GetCastInfo()` | Its table is `castId, spellId, guid, castType, castStartS, castEndS, castRemainingMs, castDurationMs, gcdEndS, gcdRemainingMs` (`NampowerAPI.lua:1859`). `PendingCast` (`spell/inflight.rs`) carries a spell id, a deadline and a guard bit — no start instant, no channel state, no GCD end. The consumer **divides by these** (`API.GetGCDRemainingMs`), so a table of three real fields and seven zeros is worse than the nil its own fallback already handles correctly. Needs `SMSG_SPELL_START`'s cast time kept as a start/end pair, a channel flag, and the GCD end the action bar already computes but does not publish | | `GetCurrentCastingInfo()` | `castId, visId, autoId, casting, channeling, onswing, autoattack`. Same gap: `model.casting` is one bool that explicitly excludes channels, and `visId`/`autoId`/`onswing` are queue state this client has none of | -| `GetSpellRangeData(spellId)` | `SpellRange.dbc` is not on the spell probe — `IsSpellInRange` answers the *verdict* from the live usable map, not the yards. One more probe beside `install_spell_extras_probes` | +| `GetSpellRangeData(rangeIndex)` | A `SpellRange.dbc` **row index**, per the DLL's own usage string — not a spell id, which is how this row read until the usage strings were compared against it. `IsSpellInRange` answers the *verdict* from the live usable map, never the yards, so the table is genuinely absent. One more probe beside `install_spell_extras_probes` | | `GetSpellModifiers(spellId)` | `spell/mods.rs` stores the server's `SPELLMOD_*` cells but publishes no reader | | `GetUnitData(unit)` | The whole descriptor as a table, where `GetUnitField` answers one field. Needs a decision about which fields: the real one returns GUID-typed columns (charm, summon, charmedBy) this client's field reader does not type | | `GetTrinketCooldown()` | Equipped-slot cooldowns. `container_cooldowns` is keyed `(bag, slot)`; the equipped slots are not in it | @@ -538,10 +591,49 @@ index, rank)` is left out for a narrower reason: `LearnTalent` here learns the * reaching a named rank means queueing the pair once per missing rank, and whether the server accepts that burst is not something this client can check before sending. -**Events (7), all already tracked in the nampower section above**: `SPELL_CAST_EVENT`, -`SPELL_QUEUE_EVENT`, `SPELL_START_SELF`, `SPELL_GO_SELF`, `SPELL_FAILED_SELF`, `UNIT_DIED`, -`UNIT_HEALTH_GUID`. The first two are queue events; the rest want columns the start/go rows above -already name as the blocker. +### The whole registered list, accounted for + +With the registration table read (the section above), the surface is no longer a count of what a +heuristic flagged. It is **62 names**, every one of which is now in exactly one of these buckets. +`comm -23` against `dump_globals` is the check, and it is reproducible: + +``` +strings -a -t x nampower.dll \ + | awk '{a=strtonum("0x" $1); if (a>=0x7c6cc && a<=0x7cb50) {$1=""; print substr($0,2)}}' \ + | grep -E '^[A-Za-z_][A-Za-z0-9_]*$' | sort -u > /tmp/np +cargo run -q -p benilla-ui --example dump_globals | cut -f1 | sort -u > /tmp/ours +comm -23 /tmp/np /tmp/ours +``` + +| Bucket | Count | Names | +|---|---|---| +| **Built** | 32 | `CancelPlayerAuraSlot`, `CancelPlayerAuraSpellId`, `CastSpellByName`, `CastSpellByNameNoQueue`, `CastSpellNoQueue`, `FindPlayerItemSlot`, `GetBagItem`, `GetEquippedItem`, `GetEquippedItems`, `GetItemIconTexture`, `GetItemIdCooldown`, `GetItemLevel`, `GetItemStats`, `GetItemStatsField`, `GetNampowerVersion`, `GetPlayerAuraDuration`, `GetSpellDuration`, `GetSpellIconTexture`, `GetSpellIdForName`, `GetSpellNameAndRankForId`, `GetSpellRecField`, `GetSpellSlotTypeIdForName`, `GetTrinkets`, `GetUnitField`, `GetUnitGUID`, `IsAuraHidden`, `IsSpellInRange`, `IsSpellUsable`, `PlayerIsMoving`, `PlayerIsRooted`, `PlayerIsSwimming`, `SetMouseoverUnit`, `UseItemIdOrName` | +| **Withheld pending grom's say-so** | 8 | `WriteCustomFile`, `ReadCustomFile`, `CustomFileExists`, `ImportFile`, `ExportFile`, `ExecuteCustomLuaFile`, `StartItemExport` (writes a file of its own), `EncryptPassword` + `EncryptedServerLogin` | +| **Not ours to add** | 4 | `QueueSpellByName`, `QueueScript`, `ChannelStopCastingNextTick`, `LearnTalentRank` | +| **To build, in corpus-call order** | 18 | `GetCurrentCastingInfo` (25 call sites outside the API layer), `GetCastInfo` (20), `GetSpellPower` (7), `GetBagItems` (7), `GetSpellModifiers` (6), `DisenchantAll` (5), `GetSpellIdCooldown` (3), then `GetSpellRec`, `GetTrinketCooldown`, `UseTrinket`, `GetUnitData`, `GetSpellRangeData`, `CombatLogFlush`, `GetRaidTargets`, `SetLocalRaidTargetIndex`, `GetQuestLogQuestIds`, `GetQuestDialogQuestId` (0 each — present for completeness, not need) | + +The two at the top of that last bucket are worth the ordering note: `GetCurrentCastingInfo` and +`GetCastInfo` are the **most-called nampower functions in the whole corpus**, and one of +`GetCurrentCastingInfo`'s sites (`SuperCleveRoidMacros/Core.lua:94`) calls it with no existence +guard at all, so on this client that path raises today. They are also coupled: `Core.lua:4899` +registers `SPELL_CAST_EVENT` **gated on `GetCurrentCastingInfo` existing**, so defining the function +without firing the event would move that addon from "never registered" to "registered and silent", +which is worse. Implement the pair with the event. + +### The event table, accounted for + +The same read gives **59 event names** (`0x7c198`–`0x7c658`). Their argument lists are upstream's +`EVENTS.md`, and for the cast family pfUI's `libs/libdebuff.lua:909-945` corroborates every column +from the consumer side — it destructures `arg2`=spellId, `arg3`=casterGuid, `arg4`=targetGuid, +`arg6`=castTime, `arg7`=channel duration, which is exactly the documented order. + +| Bucket | Names | +|---|---| +| **Fired** | `BUFF_ADDED_SELF`, `BUFF_REMOVED_SELF`, `DEBUFF_ADDED_SELF`, `DEBUFF_REMOVED_SELF`, `DEBUFF_ADDED_OTHER`, `DEBUFF_REMOVED_OTHER`, `AURA_CAST_ON_SELF`, `AURA_CAST_ON_OTHER` (`ui_aura.rs`, `ui_cast_events.rs`) | +| **Deliberately absent** | `BUFF_ADDED_OTHER`/`BUFF_REMOVED_OTHER` — diffing the helpful half of every streamed unit, which nothing registers (the `AuraEdge` doc says so at the enum); `SPELL_QUEUE_EVENT` — the queue; `SPELL_DELAYED_OTHER` — upstream's own note is that the server only sends the packet to the affected player, so it never fires there either | +| **To fire, off packets already parsed** | `SPELL_START_SELF`/`_OTHER`, `SPELL_GO_SELF`/`_OTHER`, `SPELL_FAILED_SELF`/`_OTHER`, `SPELL_DELAYED_SELF`, `SPELL_CHANNEL_START`, `SPELL_CHANNEL_UPDATE`, `SPELL_CAST_EVENT` — all of these are edges `ui_cast_events.rs`'s one queue already carries for `UNIT_CASTEVENT`; what each needs beyond today's row is named in that file | +| **To fire, needs its own producer** | `UNIT_DIED`, the thirteen `UNIT_*_GUID`/`PLAYER_GUILD_UPDATE_GUID` rows (one shape: `guid, isPlayer, isTarget, isMouseover, isPet, partyIndex, raidIndex`, gated by the `NP_EnableUnitEvents*` CVars already registered), `UNIT_COMBAT_GUID`, `SPELL_DAMAGE_EVENT_SELF`/`_OTHER`, `SPELL_HEAL_BY_SELF`/`_BY_OTHER`/`_ON_SELF`, `SPELL_ENERGIZE_*`, `SPELL_MISS_SELF`/`_OTHER`, `SPELL_DISPEL_BY_*`, `DAMAGE_SHIELD_*`, `ENVIRONMENTAL_DMG_*`, `AUTO_ATTACK_SELF`/`_OTHER`, `BUFF_UPDATE_DURATION_SELF`, `DEBUFF_UPDATE_DURATION_SELF`, `KEY_DOWN`/`KEY_UP` | + **ClassicAPI's 70** are the remaining bulk and are tracked in `docs/classic-api-functions.md` rather than here. The same caution applies twice over: its 999 pointed-at names include a great deal