From 751cac062b184c2e347c9e5bbd36be42969a5793 Mon Sep 17 00:00:00 2001 From: grom Date: Fri, 16 Oct 2026 17:59:57 -0600 Subject: [PATCH] compat: C_Spell.GetSpellEffectMechanics, the per-effect sibling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `SuperCleveRoidMacros/ClassicAPI.lua:267` calls it unguarded, so its absence was a raise a session. It is not a second spelling of `GetSpellMechanicByID`, which this tree already answers: that one reads `Spell.dbc`'s spell-level `Mechanic`, and vanilla stores some mechanics on an *effect* instead. The consumer's own comment names the case — Rake's bleed is effect-level, so the spell-level column says 0 while this answers `{0, 15, 0}` — and a macro conditional asking whether its target is bleeding gets the wrong answer from the spell-level field alone. `SpellDisplay::effect_mechanic` was already parsed for the cast-immunity scan, so this is a field on the probe payload and a reader, not a new lookup. The three-way answer is its sibling's, for the sibling's reason: nothing at all for an id no spell owns, three zeros for a real spell that carries no mechanic anywhere, the ids otherwise — collapsing those two would reclassify crowd control as ordinary damage, which is the hand-kept table this function exists to replace. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SgstFWtn1RxEJ5kVrpgYwn --- crates/benilla-app/src/ui_script/lifecycle.rs | 1 + .../src/script/compat/classic_api/spell.rs | 91 +++++++++++++++++++ .../benilla-ui/src/script/compat/nampower.rs | 1 + crates/benilla-ui/src/script/model.rs | 5 + docs/ADDON_COMPAT.md | 2 +- 5 files changed, 99 insertions(+), 1 deletion(-) diff --git a/crates/benilla-app/src/ui_script/lifecycle.rs b/crates/benilla-app/src/ui_script/lifecycle.rs index 3ec904cf..0fe43c1a 100644 --- a/crates/benilla-app/src/ui_script/lifecycle.rs +++ b/crates/benilla-app/src/ui_script/lifecycle.rs @@ -197,6 +197,7 @@ pub(crate) fn install_spell_extras_probes( let d = displays.get(&spell_id)?; Some(benilla_ui::script::SpellDbcExtras { mechanic: d.mechanic, + effect_mechanics: d.effect_mechanic, mechanic_name: names .as_ref() .and_then(|n| n.name(d.mechanic)) diff --git a/crates/benilla-ui/src/script/compat/classic_api/spell.rs b/crates/benilla-ui/src/script/compat/classic_api/spell.rs index f728a9ea..fd8b4798 100644 --- a/crates/benilla-ui/src/script/compat/classic_api/spell.rs +++ b/crates/benilla-ui/src/script/compat/classic_api/spell.rs @@ -245,6 +245,39 @@ pub(super) fn install(lua: &Lua) -> mlua::Result { })?, )?; + // GetSpellEffectMechanics(spellID) → `{m1, m2, m3}`, the per-effect `EffectMechanic[3]` + // column, where `GetSpellMechanicByID` above reads only the spell-level `Mechanic` one. + // + // The two are not the same question and the consumer says why + // (`SuperCleveRoidMacros/ClassicAPI.lua:259-263`): "vanilla stores some mechanics on an + // effect instead — Rake's bleed is effect-level, so GetSpellMechanicByID returns 0 but this + // returns {0,15,0}". A macro conditional that asks whether its target is bleeding gets the + // wrong answer from the spell-level field alone. + // + // Same three-way answer as its sibling, for the same reason: **nothing** for an id no spell + // owns, and a table of three numbers otherwise, with `0` in a slot meaning that effect + // carries no mechanic. A table of zeros and no table at all must stay distinguishable. + t.set( + "GetSpellEffectMechanics", + lua.create_function(|lua, spell_id: Option| { + let Some(spell_id) = spell_id else { + return Ok(Value::Nil); + }; + let model = lua.app_data_ref::().expect("model app_data"); + let Some(probe) = model.spell_mechanic_probe.as_ref() else { + return Ok(Value::Nil); + }; + let Some(x) = probe(spell_id) else { + return Ok(Value::Nil); + }; + let out = lua.create_table()?; + for (i, m) in x.effect_mechanics.iter().enumerate() { + out.set(i + 1, *m)?; + } + Ok(Value::Table(out)) + })?, + )?; + // GetSpellPowerCost(spellID) → an **array of cost rows**, not a number: the consumer reads // `costs[1].cost` (`SuperCleveRoidMacros/Core.lua:670-672`), so the shape is a list even // though 1.12 has exactly one cost per spell — there is no second power type to report. @@ -609,6 +642,63 @@ mod tests { use crate::script::{SpellBookState, SpellSlotView, SpellTabView, UiScript}; /// `GetSpellMechanicByID`'s three-way answer, which is the consumer's own spec: nothing for an + /// `GetSpellEffectMechanics` reads the **per-effect** column, which is a different question + /// from its spell-level sibling and the reason the consumer calls both: vanilla stores some + /// mechanics on an effect instead, so a spell whose `Mechanic` is 0 can still carry one + /// (`SuperCleveRoidMacros/ClassicAPI.lua:259-263` names Rake's bleed, `{0, 15, 0}`). An id no + /// spell owns stays distinguishable from a spell whose three effects carry none. + #[test] + fn the_per_effect_mechanics_are_a_table_of_three_and_absent_for_an_unknown_spell() { + let mut s = UiScript::new().unwrap(); + s.set_spell_mechanic_probe(Box::new(move |id| match id { + // Rake: nothing at the spell level, a bleed on the second effect. + 1822 => Some(crate::script::SpellDbcExtras { + mechanic: 0, + mechanic_name: None, + effect_mechanics: [0, 15, 0], + mana_cost: 0, + power_type: 0, + duration_ms: 0, + icon_id: 0, + }), + // A real spell with no mechanic anywhere. + 133 => Some(crate::script::SpellDbcExtras { + mechanic: 0, + mechanic_name: None, + effect_mechanics: [0; 3], + mana_cost: 0, + power_type: 0, + duration_ms: 0, + icon_id: 0, + }), + _ => None, + })); + assert_eq!( + s.eval::>("return C_Spell.GetSpellEffectMechanics(1822)") + .unwrap(), + vec![0, 15, 0], + "the effect-level bleed the spell-level column does not carry" + ); + assert_eq!( + s.eval::>("return C_Spell.GetSpellEffectMechanics(133)") + .unwrap(), + vec![0, 0, 0], + "a real spell with no mechanic is three zeros, not nothing" + ); + assert!( + s.eval::>("return C_Spell.GetSpellEffectMechanics(999999)") + .unwrap() + .is_none(), + "an id no spell owns answers nothing at all" + ); + // And the spell-level getter still disagrees with it, which is the whole point. + assert_eq!( + s.eval::("local m = C_Spell.GetSpellMechanicByID(1822) return m") + .unwrap(), + 0 + ); + } + /// id no spell owns, `0` for a real spell with no mechanic, the id otherwise. It replaces a /// hand-kept spellID → mechanic table, so collapsing "unknown" into "none" would quietly /// reclassify crowd control as ordinary damage. @@ -618,6 +708,7 @@ mod tests { let extras = |mechanic: u32, name: Option<&str>, cost: u32| crate::script::SpellDbcExtras { mechanic, mechanic_name: name.map(str::to_string), + effect_mechanics: [0; 3], mana_cost: cost, power_type: 0, duration_ms: 0, diff --git a/crates/benilla-ui/src/script/compat/nampower.rs b/crates/benilla-ui/src/script/compat/nampower.rs index 49bb51e8..c72c0966 100644 --- a/crates/benilla-ui/src/script/compat/nampower.rs +++ b/crates/benilla-ui/src/script/compat/nampower.rs @@ -545,6 +545,7 @@ mod tests { let extras = |duration_ms: u32| crate::script::SpellDbcExtras { mechanic: 0, mechanic_name: None, + effect_mechanics: [0; 3], mana_cost: 0, power_type: 0, duration_ms, diff --git a/crates/benilla-ui/src/script/model.rs b/crates/benilla-ui/src/script/model.rs index e27bcf92..9b32af12 100644 --- a/crates/benilla-ui/src/script/model.rs +++ b/crates/benilla-ui/src/script/model.rs @@ -84,6 +84,11 @@ pub struct SpellDbcExtras { /// That mechanic's enUS `SpellMechanic.dbc` name, lower-case as the data holds it; `None` for /// mechanic 0 and for an id with no row. pub mechanic_name: Option, + /// `EffectMechanic[3]`, the per-effect column, where [`Self::mechanic`] is the spell-level + /// one. Vanilla puts some mechanics on an effect instead of the spell — Rake's bleed is the + /// consumer's own example — so a caller that reads only the spell-level field misses them. + /// `0` in a slot means that effect carries no mechanic. + pub effect_mechanics: [u32; 3], /// `manaCost` (column 32): the flat cost, in [`Self::power_type`]'s unit. pub mana_cost: u32, /// `powerType`: 0 mana, 1 rage, 3 energy — the unit [`Self::mana_cost`] is counted in. diff --git a/docs/ADDON_COMPAT.md b/docs/ADDON_COMPAT.md index aac847a3..199a5c04 100644 --- a/docs/ADDON_COMPAT.md +++ b/docs/ADDON_COMPAT.md @@ -388,7 +388,7 @@ with the load checked after each. | Secure-attribute click dispatch (`type1`/`type2`/click-cast) | **built** | `SetAttribute`/`GetAttribute` were built as a bare key/value store, with this file's own header calling that "honest" about not claiming modern WoW's restricted-execution subsystem. The line was in the wrong place: a frame that stores attributes and does nothing when clicked is not a documented limitation, it is an inert frame. **pfUI drives every one of its unit frames this way whenever Clique is absent** — which is the default — setting `type1="target"`, `type2="menu"` and `EnableClickCast()` and installing **no `OnClick` script at all** (`api/unitframes.lua:1320-1326`). So the player frame could not be clicked to target, right-clicked for its menu, or used for click-casting, and neither could any other unit frame. Dispatched in `frame.rs`'s `dispatch_attribute_click`, called from `button.rs` **after** the `OnClick` handler for the same reason the nameplate selection is — a frame may have both and a raising hook must not eat the action. Handles `target`, `menu` (through `ClassicAPI_ToggleUnitMenu`, the same entry pfUI's own Lua path calls, so the two routes open one menu), `focus`, `spell` (cast on the frame's `unit`, which is what click-casting means) and `macro`/`macrotext`, with `shift-`/`ctrl-`/`alt-` prefixes falling back to the unmodified binding as the reference's lookup does. **What is dispatched is the action, not the security model**: no taint, no protected call, no combat lockdown — none of which 1.12 has | | nampower's `*_SELF` aura edges | **built** | `BUFF_ADDED_SELF`/`BUFF_REMOVED_SELF`/`DEBUFF_ADDED_SELF`/`DEBUFF_REMOVED_SELF`, over the diff that already produced the `*_OTHER` pair — the player was **explicitly skipped** there, with a comment saying this half was unbuilt. Its helpful half is diffed separately because the halves fire different events. Why it matters: DoiteAuras reads the whole aura block **once**, at `PLAYER_ENTERING_WORLD`, and then relies entirely on these four (`UpdateAuras` is called nowhere else), so with none fired its table froze at login and every buff gained afterwards read as missing. Upstream has **eight**; the `BUFF_*_OTHER` pair is deliberately not produced — it would mean diffing the helpful half of every streamed unit on every edge for no reader — and `AuraEdge`'s three variants say so rather than leaving a dead arm firing events the gate must then allow. **Gate note**: routing the names through a `&str` parameter compiled and left `reference_ui.rs`'s event walker blind (it reads the literal at the call site), including to the two `*_OTHER` names already declared; the literals are back at the fire sites for that reason | | Aura feed ordered before the `PLAYER_ENTERING_WORLD` fire | **fixed** | `feed_auras` and `feed_units` both sat in `UnitFeed` with **no ordering between them**, and `feed_units` is what fires `PLAYER_ENTERING_WORLD`. Whether an addon reading the aura block in that handler saw it was down to Bevy's ambiguous order — which is why DoiteAuras worked on a fresh login and broke after a `/reload`. A new `UnitFeedDispatch` set wraps `feed_units` alone so a sibling feed can order itself ahead of the fire; `feed_auras` now does. Same reasoning `feed_units` already applied internally to the XP pair ("ahead of the `PLAYER_ENTERING_WORLD` fire so its first update reads it") | -| `C_Spell.GetSpellMechanicByID` / `GetSpellPowerCost` | **built** | One probe for both (`SpellDbcExtras`): same `Spell.dbc` row, same install site, and a caller asking for either has already paid the lookup. Its own probe rather than columns on `SpellLookup`, which has eleven construction sites with no answer for them. **`GetSpellMechanicByID` is three-way** and the consumer spells it out: nothing for an id no spell owns, `0` for a real spell with no mechanic, the id otherwise — it replaces a hand-kept spellID → mechanic table, so collapsing "unknown" into "none" would reclassify crowd control as plain damage. The enUS name is the second return, from `SpellMechanic.dbc` (which this tree already parsed). **`GetSpellPowerCost` answers a LIST** (`costs[1].cost`), one row, since 1.12 has a single cost per spell; it is the flat `manaCost` column **without** the reference's per-level and percentage terms (`GetPowerCost` `0x6e31b0`), which need the caster's level and max power — stated because it is the base cost a macro reader wants, not what a scaling spell would actually spend | +| `C_Spell.GetSpellMechanicByID` / `GetSpellEffectMechanics` / `GetSpellPowerCost` | **built** | One probe for all three (`SpellDbcExtras`): same `Spell.dbc` row, same install site, and a caller asking for either has already paid the lookup. Its own probe rather than columns on `SpellLookup`, which has eleven construction sites with no answer for them. **`GetSpellMechanicByID` is three-way** and the consumer spells it out: nothing for an id no spell owns, `0` for a real spell with no mechanic, the id otherwise — it replaces a hand-kept spellID → mechanic table, so collapsing "unknown" into "none" would reclassify crowd control as plain damage. The enUS name is the second return, from `SpellMechanic.dbc` (which this tree already parsed). **`GetSpellEffectMechanics` is the per-effect sibling**, `EffectMechanic[3]` as `{m1, m2, m3}`, and it is not the same question: vanilla puts some mechanics on an effect rather than on the spell, so Rake's bleed answers `{0, 15, 0}` where `GetSpellMechanicByID` answers 0 — a macro conditional asking whether its target is bleeding gets the wrong answer from the spell-level column alone. Same three-way shape (nothing for an unknown id, three zeros for a spell with no mechanic anywhere). `SpellDisplay` already parsed the column for the cast-immunity scan, so this cost a field on the probe payload and nothing else. Its absence was one raise a session from `SuperCleveRoidMacros/ClassicAPI.lua:267`, which calls it unguarded. **`GetSpellPowerCost` answers a LIST** (`costs[1].cost`), one row, since 1.12 has a single cost per spell; it is the flat `manaCost` column **without** the reference's per-level and percentage terms (`GetPowerCost` `0x6e31b0`), which need the caster's level and max power — stated because it is the base cost a macro reader wants, not what a scaling spell would actually spend | | nampower `GetUnitField` / `GetSpellRecField`, version 4.1.3 | **built** | `GetUnitField(unit, field)` is nampower v3+'s window onto the raw `UNIT_FIELD_AURA` block, and the reason it exists is that its slots are **stable**: every 1.12 aura getter walks the *compacted* list, so each index shifts the moment an earlier aura drops and nothing can be keyed on it across frames. Two fields: `"aura"` → slot → spell id, `"auraApplications"` → slot → **stacks minus one** (the wire's own encoding; pfUI reads it back as `(apps[slot] or 0) + 1`, so returning `stacks` would show every debuff one stack high). **Slots are reported 1-based** — debuffs 33..48, which both consumers loop literally — over benilla's 0-based wire index. Needed new state: `AuraState::slot`, since the VM's aura list was compacted and had dropped the raw index; filled at both app-side builders. Accepts a token or a raw GUID (pfUI passes `GetDebuffSlotMap(guid)`, DoiteAuras passes `"player"`), folding a guid back through `token_for_guid`. Reads the same filtered list every other getter does — hidden-from-aura-bar and tracking auras are already gone, which is what a debuff tracker wants, and the slot keying is unaffected because a dropped aura reads nil at its slot rather than shifting its neighbours. `GetSpellRecField(spellID, field)` is **partial on purpose**: `name` and `rank` are answered, `stackAmount`/`manaCost`/`spellIconID` are **nil rather than a plausible zero** — this client reaches no other column of that row, and a fabricated `manaCost` of 0 reads as "free", which is exactly the test DoiteAuras' Clearcasting rule turns on. `spellIconID` is not derivable at all: the icon is carried as an MPQ path, not as the numeric id that names it. With all three in place `GetNampowerVersion` moves 4.0.0 → **4.1.3**, which is the gate DoiteAuras gates on — bumped only *after* the machinery existed, per that constant's own standing rule not to claim what is absent. Measured: DoiteAuras goes from refusing to open to **0 load / 0 session errors**; pfUI's `libs/libdebuff.lua` ("GetUnitField Edition") stops falling back to manual slot shifting | | nampower's six spellbook/duration verbs | **built** | `GetSpellNameAndRankForId(id)`, `GetSpellIdForName(name)`, `GetSpellSlotTypeIdForName(name)` → `slot, bookType`, `IsPassiveSpell(index, bookType)`, `GetSpellDuration(id[, ignoreModifiers])` → ms, and `GetPlayerAuraDuration(auraSlot)` → `spellID, durationMs, expirationTimeMs` (`script/compat/nampower.rs`). Signatures are **not inferred**: `SuperCleveRoidMacros/NampowerAPI.lua` is a shipped, documented wrapper over this whole surface and states each one's arguments, returns and units, which is what these were built against. Why they matter beyond a missing-global warning: DoiteAuras takes each as a *fast path* and otherwise falls back to a 200-slot `GetSpellName` scan **per lookup, per tracked aura, per condition evaluation**. None is expressible in 1.12: that client walks a book by index and reads a name, but has no id for what it finds and no way back from a name to either, and it exposes a running aura's remaining time (`GetPlayerBuffTimeLeft`) but no way to ask a spell how long its aura lasts *before* casting it. The two name→X verbs answer the **highest rank known**, as `CastSpellByName` does. **The two slot conventions deliberately disagree**: `GetUnitField` is 1-based (debuffs 33..48) and `GetPlayerAuraDuration` is raw 0-based (0..31 buffs, 32..47 debuffs) — each is what its own consumers loop over, and matching them to each other would break both. `GetSpellDuration` reads `SpellDuration.dbc` through the same `ui_cast_events::aura_duration_ms` that answers nampower's `SPELL_START_*` `arg7`, so the event and the query cannot disagree; it keeps upstream's three-way answer (nil for no such spell, 0 for a spell whose length this client will not state, ms otherwise) because a consumer treats 0 as absent and falls back, while a number it trusts and draws as a timer. `ignoreModifiers` is accepted and ignored: it asks for the base duration, and the base is all this client has (`spell/mods.rs` stores the server's `SPELLMOD_DURATION` cells but has no reader) | | nampower `GetSpellIconTexture` / `GetItemSpell` / `IsSpellInRange` / `IsSpellUsable` / `GetAmmo` | **built** — this row replaces the `todo` it was written as | The last five of DoiteAuras' eleven probes, so all eleven now answer. `IsSpellUsable(id|name)` -> `usable, oom` and `IsSpellInRange(id|name[, unit])` read a live per-spell map `ui_action::state` pushes over the player's **whole book**, computed by the reference's own usable walk (`0x6e3d60`) and range verdict (`0x4e56f0`) — the same computation the action bar greys its buttons with, so a spell on a bar and the same spell asked for here cannot disagree. That feed is the reason these are not a forward of `IsUsableAction`, which is keyed by *action-bar slot*: a spell the player knows but has not placed on a bar has no slot to ask through. Swept at ~10 Hz (a cooldown-generation change jumps the queue) because a book is ~150 spells and the walk reads equipment and reagents per spell. Range is measured against the **current target**, as the bar's own light is, so naming any other unit answers nil rather than quoting the target's distance under another unit's name. `IsSpellUsable`'s absence was the one with teeth: DoiteAuras' `_SafeSpellUsable` returns `1, 0` without it, so every usability condition passed and an aura gated on one showed **always**. `GetSpellIconTexture(spellIconID)` needed the pair's other half built first — `SpellDisplay` now keeps `icon_id` beside the path it resolves to, and the catalog keeps `SpellIcon.dbc` itself, so `GetSpellRecField(id, "spellIconID")` answers a real row id and this resolves it back (verified end to end: spell 6673 -> icon 456 -> `Interface\Icons\Ability_Warrior_BattleShout`). `GetItemSpell(itemID)` answers the **Use:** spell only (wire triggers 0 and 5), never an equip or proc one, or every stat trinket would read as an activatable. `GetAmmo()` is the doll's slot 0, whose count the app already sums across quivers for the low-ammo alert. Signatures throughout are **not inferred**: `SuperCleveRoidMacros/NampowerAPI.lua` is a shipped wrapper documenting each one's arguments, returns and units |