diff --git a/crates/benilla-app/src/cvars.rs b/crates/benilla-app/src/cvars.rs index 27e1107f..15c2371a 100644 --- a/crates/benilla-app/src/cvars.rs +++ b/crates/benilla-app/src/cvars.rs @@ -1420,14 +1420,10 @@ pub(crate) const REGISTERED: &[Registered] = &[ ours( "NP_PreventMountingWhenBuffCapped", "1", - "nampower addon-compat, STORED ONLY, and this one is a real gap rather than a shrug: \ - upstream defaults it ON, so a real install refuses a mount cast at 32 buffs instead of \ - letting the server eat it. Everything it needs is here — `SPELL_AURA_MOUNTED` in the \ - spell's `effect_apply_aura`, all 32 positive aura slots occupied, \ - `unit_mount_display_id() == 0` — but the test belongs inside \ - `spell::cast_send::send_spell_cast`, which already takes 22 parameters and none of them \ - is the player's own descriptor. It also refuses a press, so it wants a live test rather \ - than being bolted on", + "nampower addon-compat, LIVE: a mount cast with all 32 positive aura slots taken is \ + refused before the commit instead of being sent for the server to accept and apply \ + nothing — see crate::spell::np_press::PressRules::mount_refusal, which also exempts a \ + dismount, the case upstream's own settings label calls \"(breaks dismount)\"", ), ours( "NP_EnableEnhancedTooltips", @@ -5081,10 +5077,6 @@ mod tests { "NP_PreventRightClickPvPAttack", "NampowerSettings panel writes it; no hook in the right-click interact path", ), - ( - "NP_PreventMountingWhenBuffCapped", - "NampowerSettings panel writes it; the cast path has no refusal for it yet", - ), ( "NP_EnableEnhancedTooltips", "NampowerSettings panel writes it; upstream's tooltip lines are not authored here", diff --git a/crates/benilla-app/src/spell/cast_send.rs b/crates/benilla-app/src/spell/cast_send.rs index 531ec7c1..cc48360b 100644 --- a/crates/benilla-app/src/spell/cast_send.rs +++ b/crates/benilla-app/src/spell/cast_send.rs @@ -86,6 +86,9 @@ pub(crate) struct CastLadder<'w, 's> { pub(crate) auto_repeat: ResMut<'w, AutoRepeatActive>, pub(crate) trade_skill_opens: ResMut<'w, crate::ui_tradeskill::TradeSkillOpens>, pub(crate) ground: ResMut<'w, super::targeting::SpellTargeting>, + /// nampower's press-side rules (`np_press.rs`): read-only, so every caster surface can + /// carry it without two of them ever reaching it at once. + pub(crate) np: Res<'w, super::np_press::PressRules>, /// nampower's `SPELL_CAST_EVENT` rows and the live cast's own columns /// (`crate::ui_self_cast`): every commit below records one, which is also where /// `GetCastInfo` learns a cast's type and target. @@ -273,6 +276,7 @@ impl CastLadder<'_, '_> { &mut self.trade_skill_opens, &mut self.ground, &mut self.self_casts, + &self.np, ); } } @@ -326,6 +330,7 @@ fn send_spell_cast( trade_skill_opens: &mut crate::ui_tradeskill::TradeSkillOpens, ground: &mut super::targeting::SpellTargeting, self_casts: &mut crate::ui_self_cast::SelfCastSends, + np: &super::np_press::PressRules, ) { let now = Instant::now(); let def = spells.and_then(|s| s.catalog.get(spell_id)); @@ -566,6 +571,22 @@ fn send_spell_cast( cast_errors.push_local(spell_id, 0x39); return; } + // nampower's `NP_PreventMountingWhenBuffCapped`, which its own log line calls "Blocking mount + // spell due to buff cap": a mount cast with all 32 positive aura slots taken is refused + // here rather than sent for the server to accept and then drop. `np_press.rs` carries the + // whole rule, including why the dismount case is exempt where upstream's is not. + // + // **Silently**, which is upstream's own behaviour and not a shortcut: its report is a line in + // its debug log, and 1.12's `SpellCastResult` has no code for "too many auras", so the only + // error line available would be a sentence about something else. What the player gets instead + // is the global cooldown and the packet both unspent — which is the whole point, since the + // alternative is a press that looks like it worked and did nothing. + if np.mount_refusal(def, ctx.rel.self_store) { + debug!( + "ui_action: mount cast {spell_id} refused locally — every buff slot is taken (NP_PreventMountingWhenBuffCapped)" + ); + return; + } // The water leg (`0x609d39–0x609d7b`): 0x50 "Cannot use while swimming" for mounts, Travel // Form and food, 0x58 "Can only use while swimming" for Aquatic Form. Local, because vmangos's // CheckCast has no such arm and would grant Aquatic Form on land. @@ -826,6 +847,7 @@ mod tests { world.init_resource::(); world.init_resource::(); world.init_resource::(); + world.init_resource::(); // A bare World has no message storage until it is asked for. world.init_resource::>(); world.init_resource::>(); @@ -848,6 +870,103 @@ mod tests { .expect("the ladder runs as a one-shot system"); } + /// nampower's `NP_PreventMountingWhenBuffCapped` through the whole ladder: a mount press with + /// all 32 buff slots taken spends neither the packet nor the global cooldown, and the row off + /// or one slot free sends it as before. + /// + /// The rule itself is tested in `np_press.rs`; what this adds is that the rung is reached and + /// that it sits **above** the commit, which is the only thing that makes it worth having — a + /// refusal below `start_gcd` would save nothing. + #[test] + fn a_mount_press_at_the_buff_cap_spends_neither_the_packet_nor_the_gcd() { + use crate::net::ObjectStore; + use benilla_formats::SpellDisplay; + use benilla_protocol::ObjectFields; + const MOUNT: u32 = 458; + + // `buffs` occupied positive slots: the id a dword from field 47, the AURAFLAGS liveness + // nibble eight to a dword from 95. Without the nibble every slot reads empty, so a + // fixture that wrote ids alone would leave the cap unreached and pass vacuously. + let occupied = |buffs: u16| { + let mut pairs: Vec<(u16, u32)> = Vec::new(); + let mut flags = [0u32; 4]; + for slot in 0..buffs { + pairs.push((47 + slot, 5916 + u32::from(slot))); + flags[usize::from(slot / 8)] |= 0xb << ((slot % 8) * 4); + } + for (i, f) in flags.iter().enumerate() { + #[allow(clippy::cast_possible_truncation)] // four dwords + pairs.push((95 + i as u16, *f)); + } + pairs + }; + + let press = |enabled: bool, buffs: u16| -> (bool, bool) { + let (mut world, rx) = world(); + world.insert_resource(crate::spell::np_press::PressRules::for_tests(enabled)); + let mut spells = crate::ui_action::Spells::empty_for_tests(); + spells.catalog = benilla_formats::SpellCatalog::from_displays( + [( + MOUNT, + SpellDisplay { + effect_apply_aura: [crate::spell::np_press::SPELL_AURA_MOUNTED, 0, 0], + // A real mount carries a GCD, which is the thing a refusal saves. + start_recovery_ms: 1_500, + start_recovery_category: 133, + ..Default::default() + }, + )] + .into_iter() + .collect(), + ); + world.insert_resource(spells); + let pairs = occupied(buffs); + let store: &'static ObjectStore = + Box::leak(Box::new(ObjectStore(ObjectFields::from_pairs(&pairs)))); + // The fixture is only worth anything if it really reaches the cap. + assert_eq!( + store.0.unit_auras().filter(|a| a.is_helpful()).count(), + usize::from(buffs), + "the descriptor fixture did not plant the buffs it claims" + ); + world + .run_system_once(move |mut ladder: CastLadder| { + let base = ctx(); + let with_store = cast_target::CastContext { + rel: cast_target::TargetRelations { + self_store: Some(store), + ..base.rel + }, + ..base + }; + ladder.send(MOUNT, &with_store, CastCommit::Spell); + }) + .expect("the ladder runs as a one-shot system"); + let sent = rx.try_recv().is_ok(); + let gcd = world + .resource::() + .gcd(Instant::now()) + .is_some(); + (sent, gcd) + }; + + assert_eq!( + press(true, 32), + (false, false), + "capped and the row on: no packet, and no global cooldown either" + ); + assert_eq!( + press(true, 31), + (true, true), + "one free slot and the aura lands — the press is ordinary" + ); + assert_eq!( + press(false, 32), + (true, true), + "the row off refuses nothing, which is what a player who unticked it asked for" + ); + } + /// TryCast's dead rung (`0x6e4c49`): a corpse and a ghost (health 1, the `PLAYER_FLAGS` bit) /// are refused with 0x13 and no packet, after the profession intercept; `Attributes & /// 0x800000` waives it, and the Attack button's own entry takes the same rung. diff --git a/crates/benilla-app/src/spell/mod.rs b/crates/benilla-app/src/spell/mod.rs index c7b9edad..828364bf 100644 --- a/crates/benilla-app/src/spell/mod.rs +++ b/crates/benilla-app/src/spell/mod.rs @@ -18,6 +18,7 @@ pub(crate) mod disenchant; mod inflight; mod mods; pub(crate) mod net; +pub(crate) mod np_press; pub(crate) mod queue; pub(crate) mod targeting; pub(crate) mod usable; @@ -60,6 +61,10 @@ impl Plugin for SpellPlugin { .init_resource::() .init_resource::() .add_observer(queue::on_cvar) + // nampower's press side: the rules that decide what a press does when the client + // would refuse it (`np_press.rs`). + .init_resource::() + .add_observer(np_press::on_cvar) .init_resource::() .init_resource::() .init_resource::() diff --git a/crates/benilla-app/src/spell/np_press.rs b/crates/benilla-app/src/spell/np_press.rs new file mode 100644 index 00000000..a84f1e0e --- /dev/null +++ b/crates/benilla-app/src/spell/np_press.rs @@ -0,0 +1,238 @@ +//! nampower's press side: the rules that sit at the top of the one cast path and decide what a +//! keypress does when the client would refuse it. +//! +//! This is the half of upstream's queue that `queue.rs` deliberately left out — the half that +//! intercepts **every ordinary press** rather than waiting for Lua to ask. It lives in its own +//! module because its inputs are not the queue's: a press is admitted or refused by the player's +//! own CVar set, and those rows are read nowhere else. +//! +//! Everything here is a pure function over its inputs, with the resource below as the only state, +//! so each rule can be tested without a cast path to run it through. The wiring is +//! `cast_send::send_spell_cast`, which is where the reference's own refusals live. + +use benilla_formats::SpellDisplay; +use bevy::prelude::*; + +use crate::net::ObjectStore; + +/// `EffectApplyAuraName` 78, `SPELL_AURA_MOUNTED`: the aura every mount spell applies, and the +/// only thing that distinguishes one from any other summon. +/// +/// Checked against this realm's own `Spell.dbc` by +/// [`tests::the_mount_aura_is_what_this_realms_mount_spells_actually_carry`] rather than taken on +/// trust from mangos' header — a wrong number here makes the refusal simply never fire, which no +/// amount of play distinguishes from the rule being off. +pub(crate) const SPELL_AURA_MOUNTED: u32 = 78; + +/// nampower's press-side CVar rows, applied by [`on_cvar`]. +/// +/// The registry fires that observer at boot with each registered default, so no field is read +/// before it has been seeded. +#[derive(Resource, Default)] +pub(crate) struct PressRules { + /// `NP_PreventMountingWhenBuffCapped`. + prevent_mounting_buff_capped: bool, +} + +/// `NP_PreventMountingWhenBuffCapped`. +pub(crate) const CVAR_PREVENT_MOUNTING: &str = "NP_PreventMountingWhenBuffCapped"; + +/// Apply a change of any press-side row. +pub(crate) fn on_cvar(ev: On, mut rules: ResMut) { + if ev.is(CVAR_PREVENT_MOUNTING) { + rules.prevent_mounting_buff_capped = ev.flag(); + } +} + +impl PressRules { + /// The resource as the registry would have seeded it, for a test that runs the whole ladder + /// rather than one rule. Test-only so [`on_cvar`] stays the only writer in the app. + #[cfg(test)] + pub(crate) fn for_tests(prevent_mounting_buff_capped: bool) -> Self { + Self { + prevent_mounting_buff_capped, + } + } + + /// `NP_PreventMountingWhenBuffCapped`: whether a mount press at the buff cap is refused. + pub(crate) fn mount_refusal( + &self, + spell: Option<&SpellDisplay>, + self_store: Option<&ObjectStore>, + ) -> bool { + self.prevent_mounting_buff_capped && mount_would_be_eaten(spell, self_store) + } +} + +/// Whether this press is a mount cast the server would accept and then drop on the floor. +/// +/// **Why the client has to be the one to notice.** A mount spell's whole effect is its +/// `SPELL_AURA_MOUNTED` aura, and an aura needs a free slot in the positive half of +/// `UNIT_FIELD_AURA` — of which there are exactly +/// [`UNIT_AURA_POSITIVE_SLOTS`](benilla_protocol::messages::UNIT_AURA_POSITIVE_SLOTS). With all 32 +/// taken, vmangos accepts the cast, runs the cooldown and the global cooldown, and applies +/// nothing: the player stands still having spent a press and a GCD, with no error line to read, +/// because from the server's point of view nothing failed. +/// +/// So this refuses before the commit, which is the only place the GCD and the packet can still be +/// saved. +/// +/// **The one deviation, and upstream names the bug itself.** nampower's own settings panel labels +/// this row "Preventing mounting due to buff cap (breaks dismount)" — in 1.12 you dismount by +/// casting the mount spell again, so a blanket block strands a buff-capped player on their mount. +/// The exemption below is that parenthetical: a press while +/// `UNIT_FIELD_MOUNTDISPLAYID` is live is a dismount and always allowed. It costs one descriptor +/// read and removes the only way this rule can make things worse. +fn mount_would_be_eaten(spell: Option<&SpellDisplay>, self_store: Option<&ObjectStore>) -> bool { + let Some((d, store)) = spell.zip(self_store) else { + // An uncatalogued spell, or no descriptor yet: nothing to be sure about, so nothing is + // refused. Every rule in this module fails open. + return false; + }; + if !d.effect_apply_aura.contains(&SPELL_AURA_MOUNTED) { + return false; + } + // The dismount exemption. + if store.0.unit_mount_display_id() != 0 { + return false; + } + free_buff_slots(store) == 0 +} + +/// How many of the player's 32 positive aura slots are empty. +fn free_buff_slots(store: &ObjectStore) -> usize { + let occupied = store.0.unit_auras().filter(|a| a.is_helpful()).count(); + usize::from(benilla_protocol::messages::UNIT_AURA_POSITIVE_SLOTS).saturating_sub(occupied) +} + +#[cfg(test)] +mod tests { + use super::*; + + use benilla_protocol::messages::{UNIT_AURA_POSITIVE_SLOTS, UNIT_AURA_SLOTS}; + use benilla_protocol::ObjectFields; + + /// A mount spell: aura 78 on the effect nampower reads. + fn mount() -> SpellDisplay { + SpellDisplay { + effect_apply_aura: [SPELL_AURA_MOUNTED, 0, 0], + ..Default::default() + } + } + + /// A self store with `slots` occupied aura slots and `mount_display` as the mount. + /// + /// Packed as the wire does it: the spell id a dword per slot at `UNIT_FIELD_AURA` 47, and the + /// `UNIT_FIELD_AURAFLAGS` effect-index nibble eight to a dword from 95 — without that nibble + /// `unit_aura` reads the slot as empty whatever the id says, so an id-only fixture would + /// count nothing and the cap test would pass vacuously. + fn store_with(slots: impl Iterator, mount_display: u32) -> ObjectStore { + const F_AURA: u16 = 47; + const F_AURAFLAGS: u16 = 95; + let mut pairs = vec![(133u16, mount_display)]; + let mut flags = std::collections::BTreeMap::::new(); + for slot in slots { + pairs.push((F_AURA + u16::from(slot), 5916 + u32::from(slot))); + // `0xb`: the three effect-index bits' low one plus cancelable, as every other aura + // fixture in this crate writes it. + *flags.entry(F_AURAFLAGS + u16::from(slot / 8)).or_default() |= 0xb << ((slot % 8) * 4); + } + pairs.extend(flags); + ObjectStore(ObjectFields::from_pairs(&pairs)) + } + + /// The common case: `buffs` occupied slots from the bottom of the positive half. + fn store(buffs: u8, mount_display: u32) -> ObjectStore { + store_with(0..buffs, mount_display) + } + + /// The rule's four inputs, each on its own. + #[test] + fn a_mount_press_is_refused_only_at_the_cap_and_only_on_the_ground() { + let on = PressRules { + prevent_mounting_buff_capped: true, + }; + let off = PressRules::default(); + + let capped = store(UNIT_AURA_POSITIVE_SLOTS, 0); + assert!( + on.mount_refusal(Some(&mount()), Some(&capped)), + "32 buffs, on the ground, the row on: the cast would be eaten" + ); + assert!( + !off.mount_refusal(Some(&mount()), Some(&capped)), + "the row off refuses nothing, whatever the state" + ); + + // One free slot is enough for the aura to land. + let room = store(UNIT_AURA_POSITIVE_SLOTS - 1, 0); + assert!(!on.mount_refusal(Some(&mount()), Some(&room))); + + // The dismount exemption: upstream's own label calls a blanket block "(breaks dismount)". + let capped_and_mounted = store(UNIT_AURA_POSITIVE_SLOTS, 4321); + assert!( + !on.mount_refusal(Some(&mount()), Some(&capped_and_mounted)), + "a press while mounted is a dismount and is never refused" + ); + + // Anything that is not a mount spell is none of this rule's business. + let frostbolt = SpellDisplay::default(); + assert!(!on.mount_refusal(Some(&frostbolt), Some(&capped))); + + // Fail open on either missing input. + assert!(!on.mount_refusal(None, Some(&capped))); + assert!(!on.mount_refusal(Some(&mount()), None)); + } + + /// Only the **positive** half counts: 32 debuffs do not fill the buff half, and the harmful + /// slots a mount aura can never use must not be mistaken for a full one. + #[test] + fn the_cap_counts_the_positive_half_alone() { + let on = PressRules { + prevent_mounting_buff_capped: true, + }; + let s = store_with(UNIT_AURA_POSITIVE_SLOTS..UNIT_AURA_SLOTS, 0); + assert_eq!(free_buff_slots(&s), usize::from(UNIT_AURA_POSITIVE_SLOTS)); + assert!( + !on.mount_refusal(Some(&mount()), Some(&s)), + "a full debuff half leaves every buff slot free" + ); + } + + /// Against the real `Spell.dbc`: [`SPELL_AURA_MOUNTED`] is the number this realm's own mount + /// spells carry, not one taken on trust. Skips without client data. + /// + /// A wrong constant here is invisible in play — the refusal would simply never fire — which is + /// exactly the class of mistake a data-gated assertion is for. + #[test] + fn the_mount_aura_is_what_this_realms_mount_spells_actually_carry() { + let data = benilla_formats::wow_data_or_skip!(); + let mut chain = benilla_formats::open_chain(&data).expect("open chain"); + let catalog = benilla_formats::load_spell_catalog(&mut chain).expect("Spell.dbc"); + + // Three mounts from three sources: a vendor horse (Brown Horse 458), a paladin's summon + // (Summon Warhorse 13819) and a warlock's (Summon Felsteed 5784). Any one of them alone + // could be an oddity; all three agreeing is the aura id. + for (spell_id, name) in [ + (458u32, "Brown Horse"), + (13819, "Summon Warhorse"), + (5784, "Summon Felsteed"), + ] { + let Some(d) = catalog.get(spell_id) else { + panic!("{name} ({spell_id}) is missing from this realm's Spell.dbc"); + }; + assert!( + d.effect_apply_aura.contains(&SPELL_AURA_MOUNTED), + "{name} ({spell_id}) carries auras {:?}, not {SPELL_AURA_MOUNTED} — \ + SPELL_AURA_MOUNTED is wrong and the mount refusal would never fire", + d.effect_apply_aura + ); + } + + // And the converse, so the constant is not merely *a* number these rows share: an + // ordinary spell must not match. Frostbolt 116 applies a slow (aura 33), not a mount. + if let Some(d) = catalog.get(116) { + assert!(!d.effect_apply_aura.contains(&SPELL_AURA_MOUNTED)); + } + } +} diff --git a/crates/benilla-app/src/spell/targeting/mod.rs b/crates/benilla-app/src/spell/targeting/mod.rs index fd8abd21..ecdfdfc6 100644 --- a/crates/benilla-app/src/spell/targeting/mod.rs +++ b/crates/benilla-app/src/spell/targeting/mod.rs @@ -506,6 +506,7 @@ mod tests { world.init_resource::(); world.insert_resource(crate::net::Reputations(Vec::new())); world.init_resource::(); + world.init_resource::(); world.init_resource::(); world.init_resource::(); world.init_resource::(); diff --git a/crates/benilla-app/src/spell/targeting/world.rs b/crates/benilla-app/src/spell/targeting/world.rs index e826da65..609cb536 100644 --- a/crates/benilla-app/src/spell/targeting/world.rs +++ b/crates/benilla-app/src/spell/targeting/world.rs @@ -141,6 +141,7 @@ mod tests { world.init_resource::(); world.insert_resource(crate::net::Reputations(Vec::new())); world.init_resource::(); + world.init_resource::(); world.init_resource::(); world.init_resource::(); world.init_resource::(); diff --git a/crates/benilla-app/src/ui_items/drain.rs b/crates/benilla-app/src/ui_items/drain.rs index 8f50703f..197d71e5 100644 --- a/crates/benilla-app/src/ui_items/drain.rs +++ b/crates/benilla-app/src/ui_items/drain.rs @@ -761,6 +761,7 @@ mod tests { .init_resource::() .init_resource::() .init_resource::() + .init_resource::() .init_resource::() .init_resource::() .insert_resource(NetCommands(tx)); @@ -1719,6 +1720,7 @@ mod bind_confirm_tests { .init_resource::() .init_resource::() .init_resource::() + .init_resource::() .init_resource::() .init_resource::() .insert_resource(NetCommands(tx)); diff --git a/docs/ADDON_COMPAT.md b/docs/ADDON_COMPAT.md index 814bf52f..ceb49695 100644 --- a/docs/ADDON_COMPAT.md +++ b/docs/ADDON_COMPAT.md @@ -1051,7 +1051,7 @@ reason is the same mistake the function list started from: the first pass took t | `NP_EnableSpellEnergizeEvents` | **LIVE** | The seventh `NP_Enable*`; nothing installed writes it, which is why it was missed | | `NP_SpamProtectionEnabled`, `NP_QuickcastOnDoubleCast` | stored | The press-interception half of the queue, which is a separate decision | | `NP_PreventRightClickPvPAttack` | stored | Needs a hook in the right-click interact path, which decides target, interact and attack in one pass; its sibling `NP_PreventRightClickTargetChange` is stored for the same reason | -| `NP_PreventMountingWhenBuffCapped` | stored — **a real gap** | Upstream defaults it **on**, so a real install refuses a mount cast at 32 buffs rather than letting the server eat it. Everything it needs is here (`SPELL_AURA_MOUNTED`, all 32 positive slots occupied, `unit_mount_display_id() == 0`); the test belongs inside `send_spell_cast`, which already takes 22 parameters and none of them is the player's own descriptor. It also refuses a press, so it wants a live test rather than being bolted on | +| `NP_PreventMountingWhenBuffCapped` | **live** | A mount press with all 32 positive aura slots taken is refused above the commit, so neither the packet nor the global cooldown is spent — `spell/np_press.rs`'s `mount_refusal`. The aura number (`SPELL_AURA_MOUNTED` = 78) is asserted against this realm's own `Spell.dbc` for three mount spells rather than taken from mangos' header, because a wrong one would make the refusal silently never fire. **One deliberate deviation, and upstream names the bug itself**: its settings label reads "Preventing mounting due to buff cap (breaks dismount)" — in 1.12 you dismount by re-casting the mount — so a press while `UNIT_FIELD_MOUNTDISPLAYID` is live is exempt here. Refused **silently**, as upstream's own report is a line in its debug log and 1.12's `SpellCastResult` has no code for "too many auras" | | `NP_EnableEnhancedTooltips` | stored | Authored content rather than a binding: the numbers are in `Spell.dbc` columns this client reads, but what the lines say and where they sit is upstream's design, and guessing it produces a tooltip that is neither ours nor theirs | | `NP_PreserveGreaterDemonAutocast` + the three `*AutocastData` | stored | Needs pet-autocast state keyed by demon rather than by the live pet guid. The three data rows are not settings at all — upstream's README calls them "managed automatically" — so they are registered to round-trip a write and nothing here will read one |