grom/benilla-octo
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

compat: the usable pair tells the truth — 1/0 from nampower's verb, the walk behind C_Spell's

Browse Source 
Two defects in one pair of verbs, both found by reading what grom's addons do with what we
answer rather than from a report. A DLL whose source is not to hand still has its contract
written down: in the addons written against it.

**`IsSpellUsable(id|name)` answers two numbers.** Ours gave 1/nil for the first — transcribed
from `IsUsableAction`'s own convention — while already giving 0/1 for the second. Four
readings in the corpus say otherwise, and `AlternativeClassicActionBars/Button.lua:1907` is
the clearest: `elseif usable and usable ~= 0 then` is not a line anyone writes against a
function that answers 1/nil. `SuperCleveRoidMacros/Core.lua:1035` labels its own branch
"IsSpellUsable returned 0 = wrong stance/form, not enough power type", reading the value
straight out of `pcall(IsSpellUsable, …)`. And the one with teeth: DoiteAuras'
`_SafeSpellUsable` keeps a verdict only `if ok and u ~= nil` (`DoiteConditions.lua:970`), so
a nil sent it down a fallback chain ending at `:1022`'s "treat as usable so icon doesn't die
forever" — **every** Usable-mode aura lit on cooldown alone, stance, rage, range and reagents
ignored. Answering the wrong shape and not answering at all land in the same place.

0 is truthy in Lua, so that was checked before changing it: no caller tests the value for
truth unguarded, and the rest compare `== 1`. The DLL's string pool publishes the signature
and the name-lookup error but not the return shape, and the five mirrors of the fork's
`SCRIPTS.md` I could reach are each behind a Gitea login or 404 — the corpus was the better
source. A name it cannot resolve raises there, which is why every caller wraps the call in
`pcall`; ours answers `nil, nil`, and all four callers treat those identically.

**`C_Spell.IsSpellUsable` was computing the wrong thing and raising on its caller's
argument.** It was approximated as "known and not on cooldown" — the one reason upstream does
*not* count (its own list is "hasn't learned it, lacks mana or reagents, reactive conditions
haven't been met") and none of the ones it does. It now reads the same per-spell map the
global does, the reference's usable walk (`0x6e3d60`), which agrees about cooldowns: "a
running cooldown never greys here" (`spell/usable.rs` leg 11), leaving `GetSpellCooldown` the
question about cooldowns.

That makes it the honest answer for a reactive proc, which is exactly what its only caller
asks: Overpower greys on the caster's combo-point byte (leg 5, `PLAYER_FIELD_BYTES` byte 1,
the byte the server sets when the proc banks), so the walk answers it inside the proc window
and nowhere else, while "off cooldown" showed it nearly always. pfUI's reactive indicator
(`modules/nampower.lua:130`) passes a **name** against a `u32` signature, which mlua refuses
for a non-numeric string — a raise once per frame while the indicator was switched on.
`spell_ident` is shared now, so a name resolves through the book as the global's does.

Both returns there stay booleans, unlike the global's: the caller hands the first straight to
`SetShown`, where a numeric 0 would be truthy and show every icon forever. Two conventions,
each its own caller's, written down where they live.

Tests: the global's pair is asserted as `1|0` and `0|1`, with the callers' own accept test
(`pcall` + `u ~= nil`) quoted as an assertion; `C_Spell`'s is asserted over Overpower outside
and inside the proc window with nothing about cooldowns changing, by id and by name, plus the
not-in-book case answering false rather than raising.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SgstFWtn1RxEJ5kVrpgYwn
This commit is contained in:
grom
2026-11-01 13:11:00 -07:00
co-authored by Claude Opus 5
parent 315ebe9138
commit e646b5a678
3 changed files with 123 additions and 25 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/benilla-ui/src/script/compat/classic_api/spell.rs
+83 -12
View File
@@ -415,22 +415,36 @@ pub(super) fn install(lua: &Lua) -> mlua::Result<Table> {
})?,
)?;
// IsSpellUsable(spellID): approximated as "known and not on cooldown" — modern usability also
// weighs resource cost (mana/rage/energy) and range, neither tracked per spell id here.
// IsSpellUsable(spellID|name) -> isUsable, insufficientPower, off the live per-spell map
// `ui_action::state` pushes: the reference's own usable walk (`0x6e3d60`), the same answer the
// action bar greys its buttons with. Upstream's own list of what makes a spell unusable is
// "hasn't learned it, lacks mana or reagents, reactive conditions haven't been met" — not
// cooldown, and the walk agrees ("a running cooldown never greys here",
// `spell/usable.rs` leg 11), so `GetSpellCooldown` stays the question about cooldowns.
//
// This was approximated as "known and not on cooldown", which is the one thing upstream does
// *not* count and none of the things it does. For a reactive ability it inverted the answer:
// Overpower greys on the caster's combo-point byte (leg 5, `PLAYER_FIELD_BYTES` byte 1 — the
// byte the server sets when the proc banks), so the walk answers it only inside the proc
// window, while "off cooldown" showed it nearly always.
//
// pfUI's reactive indicator is the caller and it passes a **name**:
// `C_Spell.IsSpellUsable(icon.spellName)` (`modules/nampower.lua:130`, for Overpower, Revenge,
// Execute, Riposte, Mongoose Bite and Counterattack), against a signature of `u32` that raised
// on it once per frame while the indicator was switched on. It hands the answer straight to
// `SetShown`, so both returns stay booleans as upstream's are — a numeric 0 is truthy in Lua
// and would show every icon forever.
t.set(
"IsSpellUsable",
lua.create_function(|lua, spell_id: u32| {
lua.create_function(|lua, ident: Option<mlua::Value>| {
let model = lua.app_data_ref::<Model>().expect("model app_data");
let now: f64 = lua.globals().get("__benilla_now").unwrap_or(0.0);
let Some(s) =
find_slot(&model, spell_id).and_then(|(slot, bank)| book_slot(&model, slot, bank))
else {
return Ok(false);
let Some(id) = super::super::nampower::spell_ident(&model, ident.as_ref()) else {
return Ok((false, false));
};
let off_cooldown = s.cooldown.is_none_or(|(start_ms, duration_ms, enabled)| {
!enabled || (start_ms as f64 / 1000.0) + f64::from(duration_ms) / 1000.0 <= now
});
Ok(off_cooldown)
Ok(model
.spell_usable
.get(&id)
.map_or((false, false), |u| (u.usable, u.out_of_power)))
})?,
)?;
@@ -694,6 +708,63 @@ pub(super) fn install_globals(lua: &Lua, t: &Table) -> mlua::Result<()> {
mod tests {
use crate::script::{SpellBookState, SpellSlotView, SpellTabView, UiScript};
/// `C_Spell.IsSpellUsable` answers the usable walk rather than "off cooldown", and takes the
/// name pfUI's reactive indicator passes it (`modules/nampower.lua:130`). Overpower is the case
/// that separates the two: the walk greys it outside its proc window, which is the whole point
/// of a reactive indicator, while "off cooldown" showed it nearly always.
#[test]
fn is_spell_usable_reads_the_usable_walk_and_takes_a_name() {
let mut s = UiScript::new().unwrap();
let slot = |spell_id: u32, name: &str| SpellSlotView {
spell_id,
name: name.into(),
rank: Some("Rank 1".into()),
..Default::default()
};
s.set_spellbook(SpellBookState {
tabs: Vec::new(),
slots: vec![slot(7384, "Overpower"), slot(6572, "Revenge")],
});
let u = |usable, out_of_power| crate::script::SpellUsable {
usable,
out_of_power,
in_range: None,
};
// Outside the proc window, and on no cooldown at all.
s.set_spell_usable(
[(7384, u(false, false)), (6572, u(false, true))]
.into_iter()
.collect(),
);
assert!(!s
.eval::<bool>("return C_Spell.IsSpellUsable(7384)")
.unwrap());
assert!(
!s.eval::<bool>("return C_Spell.IsSpellUsable(\"Overpower\")")
.unwrap(),
"the caller passes a name, and it used to raise on one"
);
let pair: String = s
.eval(
"local a, b = C_Spell.IsSpellUsable(6572) \
return tostring(a) .. \"|\" .. tostring(b)",
)
.unwrap();
assert_eq!(
pair, "false|true",
"both returns are booleans, which is what `SetShown` needs"
);
// The proc banks: the walk flips, with nothing about cooldowns changing.
s.set_spell_usable([(7384, u(true, false))].into_iter().collect());
assert!(s
.eval::<bool>("return C_Spell.IsSpellUsable(\"Overpower\")")
.unwrap());
// A spell the book does not carry answers false rather than raising.
assert!(!s
.eval::<bool>("return C_Spell.IsSpellUsable(\"Mongoose Bite\")")
.unwrap());
}
/// `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
crates/benilla-ui/src/script/compat/nampower.rs
+38 -11
View File
@@ -76,7 +76,7 @@ fn book_slots(
/// nampower's `spellIdentifier`: a spell id, or a name resolved through the player's own book at
/// its highest known rank (the same rule [`GetSpellIdForName`] follows).
fn spell_ident(model: &crate::script::Model, v: Option<&mlua::Value>) -> Option<u32> {
pub(super) fn spell_ident(model: &crate::script::Model, v: Option<&mlua::Value>) -> Option<u32> {
match v? {
mlua::Value::Integer(i) => u32::try_from(*i).ok(),
mlua::Value::Number(n) => {
@@ -1549,7 +1549,8 @@ pub(in crate::script) fn install(lua: &Lua) -> mlua::Result<()> {
//
// Why these two matter more than a missing-global warning: DoiteAuras' `_SafeSpellUsable`
// returns `1, 0` — usable, not out of power — when `IsSpellUsable` is absent, so every
// usability condition passed and an aura gated on one showed **always**.
// usability condition passed and an aura gated on one showed **always**. Answering with the
// wrong *shape* lands in the same place, which is what the return comment below is about.
lua.globals().set(
"IsSpellUsable",
lua.create_function(|lua, ident: Option<mlua::Value>| {
@@ -1565,9 +1566,23 @@ pub(in crate::script) fn install(lua: &Lua) -> mlua::Result<()> {
}) else {
return Ok((None, None));
};
// 1/nil, never a boolean: `_SafeSpellUsable` hands the pair straight back to callers
// that test it numerically.
Ok((found.0.then_some(1u32), Some(u32::from(found.1))))
// **1/0, and never nil for a spell the book knows.** Its callers switch on the
// number, and one refuses a nil outright: `SuperCleveRoidMacros/Core.lua:1035`
// has a branch labelled "IsSpellUsable returned 0 = wrong stance/form, not enough
// power type", reading the value straight out of `pcall(IsSpellUsable, …)`, and
// DoiteAuras' `_SafeSpellUsable` accepts an answer only `if ok and u ~= nil`
// (`DoiteConditions.lua:970`). Handed nil it walked its fallback chain to
// "treat as usable so icon doesn't die forever" (`:1022`), so **every** Usable-mode
// aura lit on cooldown alone, with stance, rage, range and reagents ignored. 0 is
// truthy in Lua, which would matter if a caller tested the value for truth; no caller
// in the corpus does so unguarded: `AlternativeClassicActionBars/Button.lua:1907`
// writes `elseif usable and usable ~= 0 then`, which is a line nobody writes against
// a function that answers 1/nil — it is the clearest statement of the convention in
// the corpus, and the rest compare `== 1`.
//
// The second return was already 0/1 rather than `IsUsableAction`'s 1/nil, so this
// makes the pair one convention instead of two.
Ok((Some(u32::from(found.0)), Some(u32::from(found.1))))
})?,
)?;
@@ -3234,10 +3249,11 @@ mod tests {
.unwrap());
}
/// `IsSpellUsable` answers the feed's pair as 1/nil and 1/0, never booleans: DoiteAuras'
/// `_SafeSpellUsable` hands both straight back to callers that test them numerically. A spell
/// outside the player's own book answers nothing at all, which is upstream's stated contract
/// and the signal the caller's fallback waits for.
/// `IsSpellUsable` answers the feed's pair as two numbers, 1/0 either side, never booleans
/// and never nil for a spell the book knows — `0` is the answer the DLL's own callers switch
/// on, and a nil is one of them (DoiteAuras) walking a fallback chain that ends in "usable".
/// A spell outside the player's own book answers nothing at all, which is upstream's stated
/// contract and the signal the caller's fallback waits for.
#[test]
fn spell_usable_answers_the_live_pair_and_nil_outside_the_book() {
let mut s = UiScript::new().unwrap();
@@ -3253,11 +3269,22 @@ mod tests {
.collect(),
);
assert_eq!(s.eval::<u32>("return IsSpellUsable(6673)").unwrap(), 1);
let fine: String = s
.eval("local a, b = IsSpellUsable(6673) return tostring(a) .. \"|\" .. tostring(b)")
.unwrap();
assert_eq!(fine, "1|0", "usable, and not for want of power");
let oom: String = s
.eval("local a, b = IsSpellUsable(20252) return tostring(a) .. \"|\" .. tostring(b)")
.unwrap();
assert_eq!(oom, "nil|1", "unusable, and the power gate is why");
assert_eq!(oom, "0|1", "unusable, and the power gate is why");
// The gate both callers put the answer through, quoted: DoiteAuras' `_SafeSpellUsable`
// keeps a verdict only `if ok and u ~= nil` (`DoiteConditions.lua:970`), and a nil sent
// it down a chain ending at "treat as usable so icon doesn't die forever".
assert!(
s.eval::<bool>("local ok, u = pcall(IsSpellUsable, 20252) return ok and u ~= nil")
.unwrap(),
"the callers' own accept test refuses our answer for an unusable spell"
);
// A name resolves through the book, highest rank first — rank 2 of Battle Shout.
assert!(s
.eval::<bool>("return IsSpellUsable(\"Battle Shout\") == nil")
docs/ADDON_COMPAT.md
+2 -2
View File
File diff suppressed because one or more lines are too long