diff --git a/crates/benilla-app/src/cvars.rs b/crates/benilla-app/src/cvars.rs index 15c2371a..452905d9 100644 --- a/crates/benilla-app/src/cvars.rs +++ b/crates/benilla-app/src/cvars.rs @@ -1074,93 +1074,103 @@ pub(crate) const REGISTERED: &[Registered] = &[ // quoted from the upstream README (`brues-code/nampower`, pepo's live continuation — // `pepopo978/nampower` itself 404s now), never invented here. // - // **Exactly one of these is live**: `NP_NameplateDistance`, wired to the same two readers - // SuperWoW's `NameplateRange` already drives. Every other row is STORED ONLY — benilla has no - // spell-queueing system for them to tune (we carry namreeb's simpler always-on cast rule, - // `spell/inflight.rs`'s `PendingCast::arm`), so they round-trip through the settings panel and - // change nothing. That is a deliberate, documented gap, not an oversight: a stored row keeps - // the addon's UI honest about what the player chose, while claiming these alter casting would - // be a lie the player would feel. They are listed in this file's `LUA_ONLY` test table with - // that reason, since their only reader is the addon's own Lua. + // **Most of these are live now.** The press-interception half of upstream's queue is built + // (`spell/np_press.rs`), so the admission toggles, three of the four windows, both buffers and + // the behaviour rows all do what their names say. What is left STORED ONLY is named row by + // row below with what building it would take — chiefly the dynamic latency buffer, which is + // off in upstream's own defaults, and the quickcast family, which is too. // // The queue toggles (README "Default is 1" / "Default is 0" per row). ours( "NP_QueueCastTimeSpells", "1", - "nampower v2 addon-compat, STORED ONLY: benilla has no spell queue to enable; it also \ - doubles as the addon's own presence probe (`pcall(GetCVar, ...)` in settings.lua)", + "nampower v2 addon-compat, LIVE: a press of a spell with a cast bar is held while \ + something guards it instead of being refused — see crate::spell::np_press::PressRules. \ + It also doubles as the addon's own presence probe (`pcall(GetCVar, ...)` in settings.lua)", ), ours( "NP_QueueInstantSpells", "1", - "nampower v2 addon-compat, STORED ONLY: no spell queue here", + "nampower v2 addon-compat, LIVE: the same for a press of an instant", ), ours( "NP_QueueChannelingSpells", "1", - "nampower v2 addon-compat, STORED ONLY: no spell queue here", + "nampower v2 addon-compat, LIVE: whether a press made while a channel runs is held", ), ours( "NP_QueueTargetingSpells", "1", - "nampower v2 addon-compat, STORED ONLY: no spell queue here", + "nampower v2 addon-compat, LIVE: the same for a spell whose cast raises the terrain \ + cursor — crate::spell::cast_target::wants_terrain_click names that class", ), // README: "Default is 0 (changed with 1.17.2 due to changes to on swing spells)". ours( "NP_QueueOnSwingSpells", "0", - "nampower v2 addon-compat, STORED ONLY: no spell queue here", + "nampower v2 addon-compat, LIVE: whether an on-next-swing strike may be held. Upstream's \ + own default is 0 and so is this one", ), ours( "NP_QueueSpellsOnCooldown", "1", - "nampower v2 addon-compat, LIVE: whether an entry in `QueueSpellByName`'s queue may wait \ - out the spell's own cooldown, or is dropped — see crate::spell::queue::SpellQueue", + "nampower v2 addon-compat, LIVE: whether an entry may wait out the spell's own cooldown \ + or is dropped, for both halves of the queue — the explicit `QueueSpellByName` and a held \ + press. One row, one answer: see crate::spell::queue::SpellQueue", ), // The queue windows, in ms (README defaults 500 / 1500 / 250 / 500). ours( "NP_SpellQueueWindowMs", "500", - "nampower v2 addon-compat, LIVE: how long past the in-flight cast's end an entry in \ - `QueueSpellByName`'s queue is held before it gives up — see \ - crate::spell::queue::SpellQueue", + "nampower v2 addon-compat, LIVE: how long past the guard's end a queued entry is held \ + before it gives up — the general window, for a cast in flight or a global cooldown. \ + crate::spell::np_press's header sets out what a window measures and why", ), ours( "NP_ChannelQueueWindowMs", "1500", - "nampower v2 addon-compat, STORED ONLY: no spell queue to size", + "nampower v2 addon-compat, LIVE: the same for a running channel, and the threshold \ + `NP_InterruptChannelsOutsideQueueWindow` compares a channel's remaining time against", ), ours( "NP_CooldownQueueWindowMs", "250", - "nampower v2 addon-compat, STORED ONLY: no spell queue to size", + "nampower v2 addon-compat, LIVE: the same for a press whose own spell is still cold", ), ours( "NP_TargetingQueueWindowMs", "500", - "nampower v2 addon-compat, STORED ONLY: no spell queue to size", + "nampower v2 addon-compat, LIVE: the same for a terrain-targeting spell, whatever guards \ + it", ), - // The buffer knobs, in ms (README defaults 55 / 30 / 100 / 500). These tune pepo's dynamic - // latency buffer; benilla's cast guard has no such buffer to adjust. + // The buffer knobs, in ms (README defaults 55 / 30 / 100 / 500). ours( "NP_MinBufferTimeMs", "55", - "nampower v2 addon-compat, STORED ONLY: no dynamic cast buffer here", - ), - ours( - "NP_MaxBufferIncreaseMs", - "30", - "nampower v2 addon-compat, STORED ONLY: no dynamic cast buffer here", + "nampower v2 addon-compat, LIVE: the margin added to a cast's own length before the \ + send guard reopens, which is what lets two casts chain without a round trip between \ + them — see crate::spell::PendingCast::arm for why this is a jitter margin and not a ping", ), ours( "NP_NonGcdBufferTimeMs", "100", - "nampower v2 addon-compat, STORED ONLY: no dynamic cast buffer here", + "nampower v2 addon-compat, LIVE: the same margin for a spell that costs no global \ + cooldown, where the guard is the only gap between two presses", + ), + ours( + "NP_MaxBufferIncreaseMs", + "30", + "nampower v2 addon-compat, STORED ONLY: the ceiling on upstream's *dynamic* widening of \ + the buffer, which is governed by `NP_OptimizeBufferUsingPacketTimings` and off in its \ + own defaults. The static buffer beside it is live", ), ours( "NP_OnSwingBufferCooldownMs", "500", - "nampower v2 addon-compat, STORED ONLY: no on-swing queue here", + "nampower v2 addon-compat, STORED ONLY: a rate limit on re-queueing an on-swing strike. \ + The on-swing slot itself is live; what is missing is a reason to refuse a second press \ + into it, since this client already answers a re-press of the queued strike with the \ + reference's own silent bail (`6e4d43`) and so cannot queue one twice", ), // README: "Default is 75" (percent of latency trimmed off a channel's end). ours( @@ -1177,12 +1187,17 @@ pub(crate) const REGISTERED: &[Registered] = &[ ours( "NP_InterruptChannelsOutsideQueueWindow", "0", - "nampower v2 addon-compat, STORED ONLY: no queue window to be outside of", + "nampower v2 addon-compat, LIVE: a press made while a channel has more than \ + `NP_ChannelQueueWindowMs` left ends the channel instead of being held. Off in upstream's \ + defaults and in these", ), ours( "NP_OptimizeBufferUsingPacketTimings", "0", - "nampower v2 addon-compat, STORED ONLY: no dynamic cast buffer here", + "nampower v2 addon-compat, STORED ONLY: upstream's dynamic tuning of the cast buffer from \ + observed packet timings. Off in its own defaults, so nothing changes by not having it; \ + building it means a model of this realm's jitter, which is measurement rather than a \ + binding. The static buffer it tunes is live (`NP_MinBufferTimeMs`)", ), ours( "NP_PreventRightClickTargetChange", @@ -1197,12 +1212,17 @@ pub(crate) const REGISTERED: &[Registered] = &[ ours( "NP_ReplaceMatchingNonGcdCategory", "0", - "nampower v2 addon-compat, STORED ONLY: no queue slot to replace", + "nampower v2 addon-compat, LIVE: on, a non-GCD press takes the held slot only when the \ + two share a `Spell.dbc` category; off (upstream's default, and this one) every press \ + replaces, which is its own `Replacing queued nonGcd spell X with Y`", ), ours( "NP_RetryServerRejectedSpells", "1", - "nampower v2 addon-compat, STORED ONLY: no cast-retry path here", + "nampower v2 addon-compat, LIVE: a cast the server refuses for a *timing* reason is \ + queued again once, held a buffer's worth — the mitigation for the early send guard. \ + Only 0x3c not-ready and 0x61 another-action, never a refusal the server means; \ + crate::spell::np_press::PressRules::retry_reason says why the set is narrow", ), // The one LIVE row. The README publishes NO fixed default for it — "Defaults to whatever was // set by the game or vanilla tweaks" — so this keeps benilla's own existing nameplate reach @@ -1398,16 +1418,19 @@ pub(crate) const REGISTERED: &[Registered] = &[ ours( "NP_SpamProtectionEnabled", "1", - "nampower addon-compat, STORED ONLY: blocks repeat presses while the server has not yet \ - answered the first cast. That is the press-interception half of the queue, which is not \ - built — see `NP_QueueCastTimeSpells` and the queue section of docs/ADDON_COMPAT.md", + "nampower addon-compat, LIVE: a repeat press of the cast already in flight is swallowed \ + rather than held, which is also the reference's own silent bail (`6e4d43`). Off, such a \ + press may replace itself in its slot", ), ours( "NP_QuickcastOnDoubleCast", "0", "nampower addon-compat, STORED ONLY: a second press casts a terrain-targeting spell at \ - the cursor. Same press-interception half; `NP_QuickcastTargetingSpells` beside it is \ - the one-press form of the same idea and is stored for the same reason", + the cursor instead of waiting for a click. Off in upstream's defaults. It needs the \ + world point under the mouse at press time, which this client resolves in the click path \ + (`spell::targeting::world`) and not in the cast path, so wiring it means giving the \ + ladder a camera ray — a real change, and one whose feel wants its own test. \ + `NP_QuickcastTargetingSpells` beside it is the one-press form of the same idea", ), ours( "NP_PreventRightClickPvPAttack", @@ -4994,36 +5017,20 @@ mod tests { // so each is stored so the panel round-trips and nothing here consumes it. The one // nampower row with a real engine reader, `NP_NameplateDistance`, is deliberately NOT // in this list: it is read by `nameplates.rs`/`vplates.rs` and must keep proving so. + // **Every queue row has left this list.** The admission toggles, all four windows, + // both buffers and the behaviour rows are read by `spell/np_press.rs` and + // `spell/queue.rs` as the press queue's own knobs and must keep proving so — which + // is the whole point of the orphan test: the day one of them stops being read it + // belongs back here with a reason, not silently. ( - "NP_QueueCastTimeSpells", - "NampowerSettings reads it; also its presence probe", + "NP_MaxBufferIncreaseMs", + "NampowerSettings' buffer knobs; it caps a *dynamic* buffer widening that is off \ + in upstream's own defaults and not built", ), - ("NP_QueueInstantSpells", "NampowerSettings' queue toggles"), - ( - "NP_QueueChannelingSpells", - "NampowerSettings' queue toggles", - ), - ("NP_QueueTargetingSpells", "NampowerSettings' queue toggles"), - ("NP_QueueOnSwingSpells", "NampowerSettings' queue toggles"), - // `NP_QueueSpellsOnCooldown` and `NP_SpellQueueWindowMs` are deliberately NOT in - // this list: `spell/queue.rs` reads both as the explicit queue's own two knobs and - // must keep proving so. Every other row here tunes the press-interception half of - // upstream's queue, which is not built (that module's header says why). - ("NP_ChannelQueueWindowMs", "NampowerSettings' queue windows"), - ( - "NP_CooldownQueueWindowMs", - "NampowerSettings' queue windows", - ), - ( - "NP_TargetingQueueWindowMs", - "NampowerSettings' queue windows", - ), - ("NP_MinBufferTimeMs", "NampowerSettings' buffer knobs"), - ("NP_MaxBufferIncreaseMs", "NampowerSettings' buffer knobs"), - ("NP_NonGcdBufferTimeMs", "NampowerSettings' buffer knobs"), ( "NP_OnSwingBufferCooldownMs", - "NampowerSettings' buffer knobs", + "NampowerSettings' buffer knobs; a re-press of the queued strike already takes \ + the reference's silent bail, so there is nothing for it to rate-limit", ), ( "NP_ChannelLatencyReductionPercentage", @@ -5031,15 +5038,11 @@ mod tests { ), ( "NP_DoubleCastToEndChannelEarly", - "NampowerSettings' behaviour toggles", - ), - ( - "NP_InterruptChannelsOutsideQueueWindow", - "NampowerSettings' behaviour toggles", + "NampowerSettings' behaviour toggles; the double-press family, off by default", ), ( "NP_OptimizeBufferUsingPacketTimings", - "NampowerSettings' behaviour toggles", + "NampowerSettings' behaviour toggles; upstream's dynamic buffer, off by default", ), ( "NP_PreventRightClickTargetChange", @@ -5047,15 +5050,7 @@ mod tests { ), ( "NP_QuickcastTargetingSpells", - "NampowerSettings' behaviour toggles", - ), - ( - "NP_ReplaceMatchingNonGcdCategory", - "NampowerSettings' behaviour toggles", - ), - ( - "NP_RetryServerRejectedSpells", - "NampowerSettings' behaviour toggles", + "NampowerSettings' behaviour toggles; the double-press family, off by default", ), // **Every `NP_Enable*` row has left this list.** All seven are read by // `ui_cast_events.rs` or `ui_np_events.rs` as a real event gate and must keep @@ -5065,13 +5060,10 @@ mod tests { // here. Each row in `REGISTERED` says what building it would take; the reader named // below is whoever actually writes it today, which for most of these is the settings // panel and nothing else. - ( - "NP_SpamProtectionEnabled", - "NampowerSettings panel writes it; the press-interception queue is not built", - ), ( "NP_QuickcastOnDoubleCast", - "NampowerSettings panel writes it; same press-interception half", + "NampowerSettings panel writes it; the double-press family needs the world point \ + under the mouse in the cast path, which resolves it only in the click path", ), ( "NP_PreventRightClickPvPAttack", diff --git a/crates/benilla-app/src/spell/cast_send.rs b/crates/benilla-app/src/spell/cast_send.rs index cc48360b..fd68efa6 100644 --- a/crates/benilla-app/src/spell/cast_send.rs +++ b/crates/benilla-app/src/spell/cast_send.rs @@ -15,7 +15,7 @@ //! and 0x28, else `0x6e2ea0` and 0x3c), the power gate `0x60962c` (items skip it), and the opcode //! (`0x6e57d8`: `0xab` vs `0x12e`). -use std::time::Instant; +use std::time::{Duration, Instant}; use benilla_protocol::messages::UseItemTarget; use bevy::ecs::system::SystemParam; @@ -64,6 +64,34 @@ impl CastCommit { } } +/// Whether nampower's press-interception half may hold this send back. +/// +/// A first-class choice rather than a flag, because upstream makes it one: `CastSpellNoQueue` and +/// `CastSpellByNameNoQueue` exist for exactly this, and the queue's own pop needs it too — a pop +/// that could be re-queued is a loop that never sends anything. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) enum Queueing { + /// An ordinary press, from a bar, a key, a macro or `CastSpell`. + Allowed, + /// `CastSpellNoQueue`, `CastSpellByNameNoQueue`, and the queue popping an entry: send it now + /// or refuse it now, never hold it. + Bypassed, +} + +/// nampower's press-side inputs, which the reference's ladder has no equivalent of: the player's +/// rules, the slots a held press lands in, the running channel, and whether this send may be held +/// at all. +/// +/// Bundled rather than four more positional arguments on a function that already has twenty — +/// they arrive together, they are read at one rung, and naming them at the call site is the only +/// way the rung stays legible. +pub(crate) struct NpPress<'a> { + pub(crate) rules: &'a super::np_press::PressRules, + pub(crate) queue: &'a mut super::queue::SpellQueue, + pub(crate) channel: &'a super::ActiveChannel, + pub(crate) queueing: Queueing, +} + /// Everything the cast-send path reads, as one [`SystemParam`]: [`CastLadder::send`] is the only /// way into [`send_spell_cast`], for every caster surface. #[derive(SystemParam)] @@ -89,6 +117,12 @@ pub(crate) struct CastLadder<'w, 's> { /// 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 queue slots. Carried here, rather than taken beside `CastLadder` by whichever + /// system wants them, because **every** press can now land in one: a system holding both + /// would reach the same resource twice and Bevy panics at run time, not in a unit test. + pub(crate) queue: ResMut<'w, super::queue::SpellQueue>, + /// The running channel, which the press queue reads as a guard like any other. + pub(crate) channel: Res<'w, super::ActiveChannel>, /// 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. @@ -184,7 +218,8 @@ impl CastLadder<'_, '_> { .map(|s| s.cast_time_ms(d, d.base_level)) }) .unwrap_or(0); - self.pending.arm(spell_id, now, cast_time_ms, guards); + self.pending + .arm(spell_id, now, cast_time_ms, guards, self.np.buffer_for(def)); } let def = self.spells.as_ref().and_then(|s| s.catalog.get(spell_id)); if let Some(d) = def { @@ -227,13 +262,27 @@ impl CastLadder<'_, '_> { } /// Run the ladder for `spell_id` and commit as `commit` says. + /// + /// An ordinary press: nampower's press-interception half may hold it rather than let the + /// ladder refuse it. [`Self::send_now`] is the same ladder with that half switched off. pub(crate) fn send( &mut self, spell_id: u32, ctx: &cast_target::CastContext, commit: CastCommit, ) { - self.send_bound(spell_id, ctx, commit, None); + self.send_bound(spell_id, ctx, commit, None, Queueing::Allowed); + } + + /// The ladder with nampower's press queue bypassed: `CastSpellNoQueue`, + /// `CastSpellByNameNoQueue`, and the queue's own pop. + pub(crate) fn send_now( + &mut self, + spell_id: u32, + ctx: &cast_target::CastContext, + commit: CastCommit, + ) { + self.send_bound(spell_id, ctx, commit, None, Queueing::Bypassed); } /// The ladder for a cast the caller already bound to a GameObject (a lock opener): TryCast @@ -245,7 +294,13 @@ impl CastLadder<'_, '_> { ctx: &cast_target::CastContext, go_guid: u64, ) { - self.send_bound(spell_id, ctx, CastCommit::Spell, Some(go_guid)); + self.send_bound( + spell_id, + ctx, + CastCommit::Spell, + Some(go_guid), + Queueing::Allowed, + ); } fn send_bound( @@ -254,6 +309,7 @@ impl CastLadder<'_, '_> { ctx: &cast_target::CastContext, commit: CastCommit, on_object: Option, + queueing: Queueing, ) { send_spell_cast( spell_id, @@ -276,11 +332,40 @@ impl CastLadder<'_, '_> { &mut self.trade_skill_opens, &mut self.ground, &mut self.self_casts, - &self.np, + &mut NpPress { + rules: &self.np, + queue: &mut self.queue, + channel: &self.channel, + queueing, + }, ); } } +/// The item entry a cooldown query is keyed on: the item's for an item use, 0 for a spell +/// (`0x60952b`'s fork). +fn queried_item_for_cooldown(commit: CastCommit) -> u32 { + match commit { + CastCommit::Item { entry, .. } => entry, + CastCommit::Spell => 0, + } +} + +/// When the last of a press's guards lets go: what a held entry's window is measured from, so a +/// press made during a long cast is held until that cast's own end and not merely for a window +/// from now. +fn guard_end(guard: &super::np_press::PressGuard) -> Option { + [ + guard.cast.map(|(_, until)| until), + guard.channel, + guard.gcd, + guard.cooldown, + ] + .into_iter() + .flatten() + .max() +} + /// TryCast's dead rung (`0x6e4c49`), after the profession intercept and ahead of the Attack /// short-circuit (`0x6e4c7a`): a dead or ghost caster (`0x605f30`) is refused with 0x13, "You are /// dead", unless the spell is castable while dead (`Attributes & 0x800000`, `0x6e4c55`). @@ -330,7 +415,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, + np: &mut NpPress, ) { let now = Instant::now(); let def = spells.and_then(|s| s.catalog.get(spell_id)); @@ -369,6 +454,90 @@ fn send_spell_cast( debug!("ui_action: cast {spell_id} suppressed — already queued on next swing"); return; } + // ── nampower's press-interception half (`np_press.rs`) ── + // + // The one rung the reference has no equivalent of, and the one the whole queue is for: before + // the client's own timing refusals get a chance, ask whether this press should be **held** + // instead. If it is, no packet goes out, no global cooldown starts, no error line appears, and + // the drain in `ui_spellbook.rs` sends it the moment the guard clears. + // + // **Here, and not at the top of the ladder.** Above this sit five rungs that are not about + // timing at all — the profession opener (which opens a window rather than casting), the + // auto-repeat re-press (which toggles it off), the dead caster, the targeting cursor's clear, + // and the on-swing re-press. Each is what the press *means*, whatever is in flight, and + // queueing them would change the meaning. Everything below is a real refusal a player should + // read: no reagent, no weapon, out of range, no power, stunned, moving, wrong form. Upstream + // hooks the client's cast entry and so holds presses that would fail for those reasons too — + // its error then arrives when the entry pops, a second later, about something the player has + // stopped doing. Refusing them now is the deviation, and it is the better half of the trade. + if np.queueing == Queueing::Allowed { + let legs = cooldowns.legs(spell_id, queried_item_for_cooldown(commit), def, now); + let guard = super::np_press::PressGuard { + cast: pending.guarding(now), + channel: np.channel.ends_at(now), + gcd: legs + .gcd + .as_ref() + .map(|c| now + Duration::from_millis(u64::from(c.remaining_ms))), + // The spell's **own** wait: its individual recovery and its shared category, never + // the global cooldown beside them, which is its own guard with its own window. + cooldown: [legs.individual.as_ref(), legs.category.as_ref()] + .into_iter() + .flatten() + .map(|c| now + Duration::from_millis(u64::from(c.remaining_ms))) + .max(), + }; + let caster_level = ctx + .rel + .self_store + .and_then(|s| s.0.unit_level()) + .unwrap_or(0); + let press_cast_time = + def.map_or(0, |d| spells.map_or(0, |s| s.cast_time_ms(d, caster_level))); + match np.rules.admit( + spell_id, + def, + press_cast_time, + cast_target::wants_terrain_click(def), + &guard, + np.queue.allows_on_cooldown(), + now, + ) { + Some(super::np_press::Press::Queue { kind, window_ms }) => { + // `NP_ReplaceMatchingNonGcdCategory`: off, a press takes the slot whatever is in + // it ("Replacing queued nonGcd spell X with Y"); on, a non-GCD press only + // displaces one sharing its `Spell.dbc` category. + let held = np.queue.peek(kind); + if kind == super::queue::Kind::NonGcd + && held.is_some_and(|h| { + !np.rules + .non_gcd_may_replace(def, spells.and_then(|s| s.catalog.get(h))) + }) + { + debug!( + "np-press: press of {spell_id} left the non-GCD slot alone — {held:?} holds it and the categories differ" + ); + return; + } + debug!("np-press: holding {spell_id} as {kind:?}, giving up in {window_ms} ms"); + np.queue + .queue_with_window(spell_id, kind, now, guard_end(&guard), window_ms); + return; + } + Some(super::np_press::Press::InterruptChannel) => { + if let Some(channelled) = np.channel.current(now) { + debug!( + "np-press: press of {spell_id} ends the channel of {channelled} — NP_InterruptChannelsOutsideQueueWindow" + ); + let _ = commands.0.send(ClientCommand::CancelChannelling { + spell_id: channelled, + }); + } + return; + } + None => {} + } + } if pending.in_flight(now) { // The already-casting refusal: the same spell bails silently (`6e4d43`), another errors // 0x61 "Another action is in progress" (`6e4d97`). The gate reads the in-flight record's @@ -581,7 +750,7 @@ fn send_spell_cast( // 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) { + if np.rules.mount_refusal(def, ctx.rel.self_store) { debug!( "ui_action: mount cast {spell_id} refused locally — every buff slot is taken (NP_PreventMountingWhenBuffCapped)" ); @@ -750,7 +919,13 @@ fn send_spell_cast( .and_then(|s| s.0.unit_level()) .unwrap_or(0); let cast_time_ms = def.map_or(0, |d| spells.map_or(0, |s| s.cast_time_ms(d, caster_level))); - pending.arm(spell_id, now, cast_time_ms, normal_cast); + pending.arm( + spell_id, + now, + cast_time_ms, + normal_cast, + np.rules.buffer_for(def), + ); } if on_next_swing { queued_melee.arm(spell_id); @@ -832,7 +1007,22 @@ mod tests { /// A World with exactly the resources [`CastLadder`] gathers and no `Spells`, so every rung /// that needs the spell's row is inert. + /// + /// nampower's press queue is **on**, as `cvars.rs` registers it: a press that the reference + /// would refuse for a timing reason is held here. [`reference_timing_world`] is the same + /// ladder with it off, for the tests whose subject is that refusal. fn world() -> (World, Receiver) { + world_with(crate::spell::np_press::PressRules::defaults_for_tests()) + } + + /// [`world`] with nampower's press queue switched off, which is what a player who unticks the + /// queue in `NampowerSettings` runs — and the only configuration in which the reference's own + /// timing refusals (`6e4d97`'s 0x61, the validator's not-ready rung) are still reachable. + fn reference_timing_world() -> (World, Receiver) { + world_with(crate::spell::np_press::PressRules::queue_off_for_tests()) + } + + fn world_with(np: crate::spell::np_press::PressRules) -> (World, Receiver) { let (tx, rx) = crossbeam_channel::unbounded(); let mut world = World::new(); world.insert_resource(NetCommands(tx)); @@ -847,7 +1037,9 @@ mod tests { world.init_resource::(); world.init_resource::(); world.init_resource::(); - world.init_resource::(); + world.insert_resource(np); + world.insert_resource(crate::spell::queue::SpellQueue::defaults_for_tests()); + world.init_resource::(); // A bare World has no message storage until it is asked for. world.init_resource::>(); world.init_resource::>(); @@ -870,6 +1062,64 @@ mod tests { .expect("the ladder runs as a one-shot system"); } + /// `CastSpellNoQueue` / `CastSpellByNameNoQueue` and the queue's own pop take + /// [`CastLadder::send_now`], which skips the press-interception rung entirely: the press is + /// sent or refused on the spot, never held. + /// + /// The second half of this is why `send_now` exists at all — a pop that could be held again + /// is a loop that never casts anything. + #[test] + fn send_now_is_refused_where_send_would_be_held() { + let (mut now_world, now_rx) = world(); + now_world.resource_mut::().arm( + HEARTHSTONE, + Instant::now(), + 3_000, + true, + 55, + ); + + now_world + .run_system_once(move |mut ladder: CastLadder| { + ladder.send_now(MOUNT, &ctx(), CastCommit::Spell); + }) + .expect("the ladder runs as a one-shot system"); + + assert!(now_rx.try_recv().is_err(), "nothing on the wire"); + assert_eq!( + now_world.resource::().0, + vec![CastFail::local(MOUNT, 0x61)], + "the reference's own refusal: a caller that said \"do not queue this\" is told it \ + cannot cast, not quietly held" + ); + assert_eq!( + now_world + .resource::() + .peek(crate::spell::queue::Kind::Normal), + None, + "and no slot was touched" + ); + + // The same press through `send` is held, which is what makes the pair meaningful. + let (mut held_world, held_rx) = world(); + held_world.resource_mut::().arm( + HEARTHSTONE, + Instant::now(), + 3_000, + true, + 55, + ); + send(&mut held_world, MOUNT, CastCommit::Spell); + assert!(held_rx.try_recv().is_err()); + assert!(held_world.resource::().0.is_empty()); + assert_eq!( + held_world + .resource::() + .peek(crate::spell::queue::Kind::Normal), + Some(MOUNT) + ); + } + /// 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. @@ -1099,7 +1349,15 @@ mod tests { action is in progress\"" ); - // A different cast mid-opener is the loud refusal (6e4d97). + // A different cast mid-opener is the loud refusal (6e4d97) only with nampower's queue + // off; on, it is held like any other press. + send(&mut world, MOUNT, CastCommit::Spell); + assert!(rx.try_recv().is_err()); + assert!(world.resource::().0.is_empty()); + + let (mut world, rx) = reference_timing_world(); + send_at_go(&mut world, OPENING, CHEST); + let _ = rx.try_recv(); send(&mut world, MOUNT, CastCommit::Spell); assert!(rx.try_recv().is_err()); assert_eq!( @@ -1214,7 +1472,7 @@ mod tests { /// A shot pressed mid-cast meets `6e4d97`'s `0x61` like any press (the gate reads the in-flight /// record's attributes), and the cast keeps its guard. #[test] - fn a_shot_pressed_mid_cast_is_refused_and_the_cast_keeps_its_guard() { + fn a_shot_pressed_mid_cast_is_held_by_the_queue_and_the_cast_keeps_its_guard() { const FROSTBOLT: u32 = 116; let (mut world, rx) = combat_world(false); world.resource_mut::().arm( @@ -1222,15 +1480,23 @@ mod tests { Instant::now(), 1_500, true, + 55, ); send_at(&mut world, AUTO_SHOT, MOB); assert!(rx.try_recv().is_err(), "nothing goes out on the wire"); + assert!( + world.resource::().0.is_empty(), + "and no red line either: nampower's queue holds the press, where the reference \ + answered 0x61 \"Another action is in progress\"" + ); assert_eq!( - world.resource::().0, - vec![CastFail::local(AUTO_SHOT, 0x61)], - "a different spell mid-cast is \"Another action is in progress\", whatever its class" + world + .resource::() + .peek(crate::spell::queue::Kind::NonGcd), + Some(AUTO_SHOT), + "Auto Shot costs no global cooldown, so it waits in the non-GCD slot" ); assert_eq!( world @@ -1242,7 +1508,26 @@ mod tests { assert_eq!( world.resource::().0, None, - "and the refused shot armed no repeat" + "and a held shot armed no repeat — nothing about it has happened yet" + ); + + // The same press with the queue off is the reference's own refusal, unchanged. + let (mut world, rx) = world_with(crate::spell::np_press::PressRules::queue_off_for_tests()); + world.insert_resource(combat_catalog()); + world.spawn(SelfPlayer); + world.resource_mut::().arm( + FROSTBOLT, + Instant::now(), + 1_500, + true, + 55, + ); + send_at(&mut world, AUTO_SHOT, MOB); + assert!(rx.try_recv().is_err()); + assert_eq!( + world.resource::().0, + vec![CastFail::local(AUTO_SHOT, 0x61)], + "a different spell mid-cast is \"Another action is in progress\", whatever its class" ); } @@ -1398,12 +1683,30 @@ mod tests { "the same spell's re-press is the reference's SILENT bail (6e4d43), not a red line" ); + // A *different* spell mid-use is where the two halves part: the reference answers + // 6e4d97's 0x61, and nampower's queue holds the press instead. + send(&mut world, MOUNT, HEARTH_COMMIT); + assert!(rx.try_recv().is_err()); + assert!(world.resource::().0.is_empty()); + assert_eq!( + world + .resource::() + .peek(crate::spell::queue::Kind::Normal), + Some(MOUNT), + "held, not refused — the Normal slot, since this world has no catalogue and an \ + uncatalogued spell waits for the most" + ); + + let (mut world, rx) = reference_timing_world(); + send(&mut world, HEARTHSTONE, HEARTH_COMMIT); + let _ = rx.try_recv(); send(&mut world, MOUNT, HEARTH_COMMIT); assert!(rx.try_recv().is_err()); assert_eq!( world.resource::().0, vec![CastFail::local(MOUNT, 0x61)], - "a different spell mid-cast is 6e4d97's \"Another action is in progress\"" + "with the queue off, a different spell mid-cast is 6e4d97's \"Another action is in \ + progress\"" ); } @@ -1411,7 +1714,11 @@ mod tests { /// (`609549`), a spell 0x3c (`609616`). #[test] fn the_not_ready_reason_forks_on_item_present() { - let (mut world, rx) = world(); + // The queue off: a press made inside `NP_CooldownQueueWindowMs` of a cooldown's end is + // held rather than refused, and these cooldowns are half an hour long, so with the queue + // on the press reaches this rung anyway — but the rung's own fork is the reference's and + // reads clearest without a second mechanism in the frame. + let (mut world, rx) = reference_timing_world(); let use_spell = benilla_protocol::messages::ItemUseSpell { spell_id: HEARTHSTONE, cooldown_ms: 1_800_000, @@ -1565,7 +1872,10 @@ mod tests { /// TryCast's IsCasting refusal (`0x61`) precedes the not-ready rung. #[test] fn in_flight_outranks_not_ready() { - let (mut world, rx) = world(); + // The queue off: with it on neither rung is reached, because the press is held above + // both. That ordering is still the reference's and still worth pinning — it is what a + // player who unticks the queue gets. + let (mut world, rx) = reference_timing_world(); // A long cooldown on MOUNT... let use_spell = benilla_protocol::messages::ItemUseSpell { spell_id: MOUNT, diff --git a/crates/benilla-app/src/spell/cast_target.rs b/crates/benilla-app/src/spell/cast_target.rs index c972ea67..c4218d29 100644 --- a/crates/benilla-app/src/spell/cast_target.rs +++ b/crates/benilla-app/src/spell/cast_target.rs @@ -348,6 +348,23 @@ pub(super) fn unit_word_binds(word: u16, is_self: bool, rel: &TargetRelations) - /// The wire target for casting `def`. An unknown spell sends the selection as is, or no target /// without one, and the server validates. +/// Whether this spell's press raises the **terrain** cursor: nampower's "targeting spell", the +/// class `NP_QueueTargetingSpells` and `NP_TargetingQueueWindowMs` are about. +/// +/// The two arms of [`resolve_cast_target`] that answer `Targeting` without reading a candidate — +/// implicit target 16, and a location word with no unit bit beside it. Deliberately *not* the +/// item, locked or GameObject arms, which also raise a cursor: those want a click on a thing, not +/// a patch of ground, and upstream's own examples for the row are Blizzard and Rain of Fire. +/// +/// A cheap read of the spell's own row, because the press queue has to know the class **before** +/// the ladder runs and the full resolve needs candidates the decision has no business gathering. +pub(crate) fn wants_terrain_click(def: Option<&SpellDisplay>) -> bool { + let Some(def) = def else { return false }; + let word = cast_target_mask(def); + word != 0 + && (def.implicit_target_a1 == 16 || (word & LOCATION_BITS != 0 && word & UNIT_BITS == 0)) +} + pub(crate) fn resolve_cast_target( def: Option<&SpellDisplay>, cand: &CastCandidates, diff --git a/crates/benilla-app/src/spell/inflight.rs b/crates/benilla-app/src/spell/inflight.rs index adb3d03c..3c06ce71 100644 --- a/crates/benilla-app/src/spell/inflight.rs +++ b/crates/benilla-app/src/spell/inflight.rs @@ -48,7 +48,18 @@ pub(crate) struct PendingCast { struct PendingCastState { spell_id: u32, + /// How long the **record** lives: how late a `SMSG_SPELL_GO` or `SMSG_CAST_RESULT` naming + /// this spell is still recognised as the answer to it ([`PendingCast::committed`]), and when + /// a lost answer finally drops it. Generous on purpose — the `modalNextSpell` chain that + /// starts a hunter's Auto Shot matches on it, and that reply arrives a round trip after the + /// cast ends. deadline: Instant, + /// When the next press stops being refused ([`PendingCast::in_flight`]): the cast's own + /// already-known length plus nampower's buffer, and **not** a round trip later. Kept apart + /// from `deadline` because the two answer different questions — the guard asks "may I cast + /// now", the record asks "is this packet about the cast I started" — and nampower's whole + /// point is that the first reopens early while the second must not. + guard_until: Instant, /// This record refuses the next press, lights the in-flight ring and gates the by-name walk: /// ordinary casts and item uses. The reference writes `0xceca88` for every committed cast /// (`0x6e5026`) and discriminates at the gate instead; a ranged shot is recorded here without @@ -57,11 +68,11 @@ struct PendingCastState { } impl PendingCast { - /// A guarding cast is outstanding, inside its deadline. + /// A guarding cast is outstanding and its own length has not run out yet. pub(crate) fn in_flight(&self, now: Instant) -> bool { self.state .as_ref() - .is_some_and(|p| p.guards && now < p.deadline) + .is_some_and(|p| p.guards && now < p.guard_until) } /// The reference's `0xceca88`: the last committed, unresolved cast of any class, which @@ -77,25 +88,42 @@ impl PendingCast { pub(crate) fn current(&self, now: Instant) -> Option { self.state .as_ref() - .filter(|p| p.guards && now < p.deadline) + .filter(|p| p.guards && now < p.guard_until) .map(|p| p.spell_id) } /// Arm at send, the client's `0xceca88` write; a ranged shot passes `guards = false`. /// /// `nampower` (): the reference's own gate reopens only - /// once `SMSG_SPELL_START` round-trips the server's cast time back (see the removed - /// `refine`), so the effective time between casts is the cast time plus a full network - /// round trip. nampower patches around this by trusting the caster's own already-known - /// `cast_time_ms` immediately at send, never waiting on the server to confirm it; we do the - /// same here, always, rather than reproducing the round-trip wait as a "faithful" bug. - /// `delay` (pushback) still extends past this, exactly as nampower's own delayed-cast hook - /// does. - pub(crate) fn arm(&mut self, spell_id: u32, now: Instant, cast_time_ms: u32, guards: bool) { + /// once the server's resolution round-trips back, so the effective time between casts is the + /// cast time plus a full network round trip. nampower patches around this by trusting the + /// caster's own already-known `cast_time_ms` immediately at send, never waiting on the server + /// to confirm it; we do the same here, always, rather than reproducing the round-trip wait as + /// a "faithful" bug. `delay` (pushback) still extends past this, exactly as nampower's own + /// delayed-cast hook does. + /// + /// **Why the buffer, and why it is not a latency figure.** Our packet reaches the server + /// about half a trip after the send; the server starts its own cast then and finishes it half + /// a trip after *our* cast time elapses. So a next cast sent at exactly `cast_time` arrives + /// just as the server frees up, and the latency cancels out of the arithmetic entirely — + /// which is why `NP_MinBufferTimeMs` is 55 ms and not a ping. The buffer is jitter margin: it + /// buys the packet a little slack so it does not land a millisecond early and be refused. + /// Upstream's dynamic tuning of it (`NP_OptimizeBufferUsingPacketTimings`) is off by default + /// and not built — `spell/np_press.rs` says so at the row. + pub(crate) fn arm( + &mut self, + spell_id: u32, + now: Instant, + cast_time_ms: u32, + guards: bool, + buffer_ms: u32, + ) { self.cast_seq = self.cast_seq.wrapping_add(1); + let cast = Duration::from_millis(u64::from(cast_time_ms)); self.state = Some(PendingCastState { spell_id, - deadline: now + Duration::from_millis(u64::from(cast_time_ms)) + CAST_SLACK, + deadline: now + cast + CAST_SLACK, + guard_until: now + cast + Duration::from_millis(u64::from(buffer_ms)), guards, }); } @@ -108,6 +136,10 @@ impl PendingCast { self.state = Some(PendingCastState { spell_id, deadline: now + ITEM_SEND_PROVISIONAL, + // An item use has no cast time to reopen the guard from — the length of whatever it + // casts is not on the item — so the guard and the record end together here, as they + // did before the two were told apart. + guard_until: now + ITEM_SEND_PROVISIONAL, guards: true, }); } @@ -116,15 +148,36 @@ impl PendingCast { /// re-arms. pub(crate) fn delay(&mut self, delay_ms: u32, now: Instant) { if let Some(p) = &mut self.state { - p.deadline = p.deadline.max(now) + Duration::from_millis(u64::from(delay_ms)); + let by = Duration::from_millis(u64::from(delay_ms)); + p.deadline = p.deadline.max(now) + by; + // The guard moves with it: a pushed-back cast really does end later, so a press that + // the pushback made early must still be refused — or queued, which is the same + // question asked of the same timestamp. + p.guard_until = p.guard_until.max(now) + by; } } - /// The outstanding cast's deadline, for nampower's queue: an entry queued during a cast is - /// held until that cast's own end plus the window, so a player who queues early is not - /// punished for it. `None` with nothing in flight. - pub(crate) fn deadline(&self) -> Option { - self.state.as_ref().map(|p| p.deadline) + /// When the guard reopens, for nampower's queue: an entry queued during a cast is held until + /// that cast's own end plus the window, so a player who queues early is not punished for it. + /// `None` with nothing in flight. + /// + /// The **guard's** end and not the record's, which is the whole reason the two are apart: the + /// record outlives the cast by a round trip so a late resolution still matches, and holding a + /// queued press that long would fire it well after the player stopped expecting it. + pub(crate) fn guard_ends(&self) -> Option { + self.state.as_ref().map(|p| p.guard_until) + } + + /// The guarding cast as nampower's press queue reads it: `(spell_id, when the guard reopens)`, + /// or `None` when nothing is holding a press back. + /// + /// One accessor rather than three calls at the site, so "is something in flight", "what is it" + /// and "until when" can never be answered about different instants. + pub(crate) fn guarding(&self, now: Instant) -> Option<(u32, Instant)> { + self.state + .as_ref() + .filter(|p| p.guards && now < p.guard_until) + .map(|p| (p.spell_id, p.guard_until)) } /// [`Self::cast_seq`]'s reader, for the self-cast feed. @@ -219,29 +272,48 @@ const CHANNEL_SLACK: Duration = Duration::from_secs(2); /// Our running channel, the client's `0xceac58`: `IsCurrentAction 0x4e53a0` keeps a channeled /// spell's button checked while it runs. `MSG_CHANNEL_UPDATE(0)` ends it, finished or interrupted. #[derive(Resource, Default)] -pub(crate) struct ActiveChannel(Option<(u32, Instant)>); +pub(crate) struct ActiveChannel(Option); + +#[derive(Clone, Copy)] +struct ChannelState { + spell_id: u32, + /// The named end plus [`CHANNEL_SLACK`]: how long the channel is believed to be running when + /// its own `MSG_CHANNEL_UPDATE(0)` never arrives. + until: Instant, + /// The end the server actually named, with no slack on it. nampower's + /// `NP_InterruptChannelsOutsideQueueWindow` compares a channel's **remaining** time against a + /// window, and two seconds of belt-and-braces would make every channel look longer than it + /// is — a press near a channel's real end would read as "outside the window" and break it. + ends_at: Instant, +} impl ActiveChannel { pub(crate) fn current(&self, now: Instant) -> Option { - self.0 - .filter(|&(_, until)| now < until) - .map(|(spell, _)| spell) + self.0.filter(|c| now < c.until).map(|c| c.spell_id) + } + + /// When the running channel is due to end, as the server named it. `None` with none running. + pub(crate) fn ends_at(&self, now: Instant) -> Option { + self.0.filter(|c| now < c.until).map(|c| c.ends_at) } /// `MSG_CHANNEL_START`. pub(crate) fn start(&mut self, spell_id: u32, duration_ms: u32, now: Instant) { - self.0 = Some(( + let ends_at = now + Duration::from_millis(u64::from(duration_ms)); + self.0 = Some(ChannelState { spell_id, - now + Duration::from_millis(u64::from(duration_ms)) + CHANNEL_SLACK, - )); + until: ends_at + CHANNEL_SLACK, + ends_at, + }); } /// `MSG_CHANNEL_UPDATE`: 0 ends the channel, anything else re-times it (pushback shortens). pub(crate) fn update(&mut self, remaining_ms: u32, now: Instant) { if remaining_ms == 0 { self.0 = None; - } else if let Some((_, until)) = &mut self.0 { - *until = now + Duration::from_millis(u64::from(remaining_ms)) + CHANNEL_SLACK; + } else if let Some(c) = &mut self.0 { + c.ends_at = now + Duration::from_millis(u64::from(remaining_ms)); + c.until = c.ends_at + CHANNEL_SLACK; } } } @@ -433,6 +505,9 @@ mod tests { ); } + /// `NP_MinBufferTimeMs`' registered default, so these read what a player gets. + const BUFFER: u32 = 55; + #[test] fn a_fresh_guard_is_open() { let g = PendingCast::default(); @@ -446,7 +521,7 @@ mod tests { fn arming_closes_the_guard_and_a_matching_resolution_opens_it() { let t0 = Instant::now(); let mut g = PendingCast::default(); - g.arm(FIREBALL, t0, 1_500, true); + g.arm(FIREBALL, t0, 1_500, true, BUFFER); assert!( g.in_flight(t0), "a cast is in flight — a second one is refused" @@ -462,7 +537,7 @@ mod tests { fn a_different_spells_resolution_leaves_the_guard_closed() { let t0 = Instant::now(); let mut g = PendingCast::default(); - g.arm(FIREBALL, t0, 1_500, true); + g.arm(FIREBALL, t0, 1_500, true, BUFFER); g.clear_if(FIREBALL + 1); // a triggered proc's GO for some other spell mid-cast assert!( g.in_flight(t0), @@ -470,26 +545,58 @@ mod tests { ); } - /// nampower: the guard reopens from the cast's own known length at send, not a provisional - /// window held open until the server's round trip — see `PendingCast::arm`. + /// nampower's headline: the guard reopens from the cast's own known length plus the buffer, + /// not when the server's answer arrives — see `PendingCast::arm`. + /// + /// And the half that makes it safe: the **record** outlives the guard by [`CAST_SLACK`], so a + /// `SMSG_CAST_RESULT` arriving a round trip after the cast ended is still recognised as the + /// answer to it. That is what the `modalNextSpell` chain reads, and it is why these two + /// timestamps are not one. #[test] - fn the_guard_opens_from_the_cast_s_own_length_at_send_not_a_round_trip() { + fn the_guard_opens_from_the_casts_own_length_while_the_record_outlives_it() { let t0 = Instant::now(); let mut g = PendingCast::default(); - g.arm(FIREBALL, t0, 1_500, true); // 1.5 s cast + 2 s slack, known at send - assert!(g.in_flight(t0 + Duration::from_millis(3_200))); - assert!(!g.in_flight(t0 + Duration::from_millis(3_600))); + g.arm(FIREBALL, t0, 1_500, true, BUFFER); + assert!( + g.in_flight(t0 + Duration::from_millis(1_500)), + "still the cast's own 1.5 s" + ); + assert!( + !g.in_flight(t0 + Duration::from_millis(1_600)), + "1.5 s plus the 55 ms buffer and no round trip: the next press goes through" + ); + assert_eq!( + g.guarding(t0 + Duration::from_millis(1_600)), + None, + "and nothing is reported as guarding, so nampower's queue pops rather than holds" + ); + assert_eq!( + g.committed(t0 + Duration::from_millis(1_600)), + Some(FIREBALL), + "the record is still ours: a resolution arriving after the guard reopened is not a reply to some other cast" + ); + assert_eq!( + g.committed(t0 + Duration::from_millis(3_600)), + None, + "past the cast plus the slack, a reply is too late to be about it" + ); } + /// A pushback stretches **both** timestamps: the cast really does end later, so a press that + /// the pushback made early must still be held or refused. #[test] fn a_pushback_extends_the_guard_so_it_holds_past_the_stretched_cast() { let t0 = Instant::now(); let mut g = PendingCast::default(); - g.arm(FIREBALL, t0, 1_000, true); // deadline t0 + 1 s + 2 s slack - g.delay(500, t0); // pushed to t0 + 3.5 s + g.arm(FIREBALL, t0, 1_000, true, BUFFER); // the guard would open at t0 + 1.055 s + g.delay(500, t0); // SMSG_SPELL_DELAYED: + 500 ms assert!( - g.in_flight(t0 + Duration::from_millis(3_200)), - "the pushback kept the guard shut past the original deadline" + g.in_flight(t0 + Duration::from_millis(1_400)), + "the pushback kept the guard shut past where it would have opened" + ); + assert!( + !g.in_flight(t0 + Duration::from_millis(1_600)), + "and it opens 500 ms later, not two seconds later" ); } @@ -594,6 +701,7 @@ mod tests { Instant::now(), 1_500, true, + BUFFER, ); mark_casting(&mut app, FIREBALL); @@ -631,6 +739,7 @@ mod tests { Instant::now(), 1_500, true, + BUFFER, ); mark_casting(&mut app, FIREBALL); @@ -687,6 +796,7 @@ mod tests { Instant::now(), 1_500, true, + BUFFER, ); run(&mut app, true); @@ -709,6 +819,7 @@ mod tests { Instant::now(), 1_500, true, + BUFFER, ); run(&mut app, true); @@ -761,6 +872,7 @@ mod tests { Instant::now(), 1_500, true, + BUFFER, ); run(&mut app, false); @@ -815,6 +927,7 @@ mod tests { Instant::now(), 1_500, true, + BUFFER, ); mark_casting(&mut app, FIREBALL); @@ -956,6 +1069,7 @@ mod tests { Instant::now(), 1_500, true, + BUFFER, ); mark_casting(&mut app, FIREBALL); diff --git a/crates/benilla-app/src/spell/net.rs b/crates/benilla-app/src/spell/net.rs index 51800627..d968768e 100644 --- a/crates/benilla-app/src/spell/net.rs +++ b/crates/benilla-app/src/spell/net.rs @@ -228,6 +228,40 @@ fn on_cast_result( let ctx = targeting.context(); ladder.send(next, &ctx, CastCommit::Spell); } + // nampower's `NP_RetryServerRejectedSpells`, and the reason it defaults on: this client + // reopens the cast guard from the spell's own length plus a small buffer rather than + // waiting for the server's answer (`PendingCast::arm`), so a next cast can occasionally + // reach a server whose own timer has not quite run out. The server refuses it, and + // without this the press is simply gone — the "dropped cast" the early guard buys its + // speed with. Putting it back in its slot, once, held a buffer's worth, costs one packet + // and makes that trade safe to take. + // + // The narrow reason set (`np_press.rs`'s `retry_reason`) is what keeps this from becoming + // a retry of refusals the server means: only "not ready" and "another action is in + // progress" are what an early guard can cause. + if success { + // The memo is per rejection chain, not per session: a cast the server accepted means + // the next refusal is a fresh one and may be retried in its turn. + ladder.queue.clear_retry(); + } else if let Some(reason) = reason.filter(|&r| ladder.np.retry_reason(r)) { + let now = Instant::now(); + let def = ladder.spells.as_ref().and_then(|s| s.catalog.get(spell_id)); + let kind = crate::spell::queue::Kind::of(def); + let delay_ms = ladder.np.buffer_for(def); + let window_ms = ladder.np.retry_window_ms(); + if ladder + .queue + .queue_retry(spell_id, kind, now, delay_ms, window_ms) + { + debug!( + "np-press: the server refused {spell_id} ({reason:#x}) — queuing retry as {kind:?} in {delay_ms} ms" + ); + } else { + debug!( + "np-press: the server refused {spell_id} ({reason:#x}) again — not retried twice" + ); + } + } } } @@ -2986,7 +3020,7 @@ mod tests { app.world_mut() .resource_mut::() // `guards: false`: a ranged hunter shot is recorded but does not guard. - .arm(id, Instant::now(), 0, false); + .arm(id, Instant::now(), 0, false, 55); } let (tx, _rx) = crossbeam_channel::unbounded(); let cat = spells(); diff --git a/crates/benilla-app/src/spell/np_press.rs b/crates/benilla-app/src/spell/np_press.rs index a84f1e0e..7344d08a 100644 --- a/crates/benilla-app/src/spell/np_press.rs +++ b/crates/benilla-app/src/spell/np_press.rs @@ -1,20 +1,49 @@ //! 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. +//! This is the half of upstream's queue that `queue.rs` deliberately left out for a long while — +//! the half that intercepts **every ordinary press** rather than waiting for Lua to ask. The +//! slots are still `queue.rs`'s: upstream has one queue, one set of `SPELL_QUEUE_EVENT` codes and +//! one "Replacing queued nonGcd spell X with Y", and a second set of slots beside them would be a +//! different feature wearing the same name. What lives here is the *decision* — whether a press +//! is held, and for how long — 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. +//! +//! ## What a window means, and how that was decided +//! +//! Upstream publishes four of them — `NP_SpellQueueWindowMs` 500, `NP_ChannelQueueWindowMs` 1500, +//! `NP_CooldownQueueWindowMs` 250, `NP_TargetingQueueWindowMs` 500 — and no sentence saying what +//! they measure. Two readings fit the names: *how long an entry survives* before it gives up, and +//! *how close to the guard's end a press must land* to be admitted at all. +//! +//! This takes the first, for three reasons. `queue.rs` already reads `NP_SpellQueueWindowMs` that +//! way for the explicit queue, and one number meaning two things in one client is a trap nobody +//! would find twice. Upstream's own `Replacing queued nonGcd spell X with Y` says a later press +//! replaces an earlier one, so holding every press costs nothing in staleness — the last one +//! wins, which is what a player pressing through a cast means. And the admission reading makes +//! the 250 ms cooldown window refuse almost every press made during a global cooldown, which is +//! the exact complaint nampower exists to answer. +//! +//! Two rows do read a window as a threshold on remaining time, and both say so themselves. +//! `NP_InterruptChannelsOutsideQueueWindow`'s *name* states one and names the channel window to +//! compare against. And `NP_CooldownQueueWindowMs` has to be one: a cast or a global cooldown ends +//! in a second or two, where a spell's own cooldown can be half an hour, and a quarter of a second +//! — which is what that row defaults to — is an allowance for pressing a touch early rather than +//! a queue. [`PressRules::admit`] says this again at the arm itself. + +use std::time::{Duration, Instant}; use benilla_formats::SpellDisplay; use bevy::prelude::*; use crate::net::ObjectStore; +use super::queue::Kind; + /// `EffectApplyAuraName` 78, `SPELL_AURA_MOUNTED`: the aura every mount spell applies, and the /// only thing that distinguishes one from any other summon. /// @@ -32,16 +61,171 @@ pub(crate) const SPELL_AURA_MOUNTED: u32 = 78; pub(crate) struct PressRules { /// `NP_PreventMountingWhenBuffCapped`. prevent_mounting_buff_capped: bool, + /// `NP_QueueCastTimeSpells`: hold a press of a spell that has a cast bar. + queue_cast_time: bool, + /// `NP_QueueInstantSpells`: hold a press of an instant. + queue_instant: bool, + /// `NP_QueueChannelingSpells`: hold a press made while a channel runs. + queue_channeling: bool, + /// `NP_QueueTargetingSpells`: hold a press of a spell whose cast raises the terrain cursor. + queue_targeting: bool, + /// `NP_QueueOnSwingSpells`: hold a press of an on-next-swing strike. Upstream's default is 0 + /// "changed with 1.17.2 due to changes to on swing spells", so this is off unless asked for. + queue_on_swing: bool, + /// `NP_SpellQueueWindowMs`: how long a held press survives past the guard's end, for a cast + /// in flight or a global cooldown. + window_spell_ms: u32, + /// `NP_ChannelQueueWindowMs`: the same, for a running channel — and the threshold + /// `NP_InterruptChannelsOutsideQueueWindow` compares a channel's remaining time against + /// ([`Self::admit`]'s channel arm). + window_channel_ms: u32, + /// `NP_CooldownQueueWindowMs`: the same, for a press whose own spell is still cold. + window_cooldown_ms: u32, + /// `NP_TargetingQueueWindowMs`: the same, for a terrain-targeting spell, whatever guards. + window_targeting_ms: u32, + /// `NP_MinBufferTimeMs`: the jitter margin added to a cast's own length before the guard + /// reopens. See [`super::PendingCast::arm`] for why this is not a latency figure. + buffer_ms: u32, + /// `NP_NonGcdBufferTimeMs`: the same for a spell that costs no global cooldown, where the + /// guard is the only thing between two presses and so has to be wider. + buffer_non_gcd_ms: u32, + /// `NP_SpamProtectionEnabled`: a repeat press of the spell already in flight is swallowed + /// rather than held. + spam_protection: bool, + /// `NP_InterruptChannelsOutsideQueueWindow`: a press made while a channel has more than + /// [`Self::window_channel_ms`] left ends the channel instead of being held. + interrupt_channels_outside_window: bool, + /// `NP_ReplaceMatchingNonGcdCategory`: a non-GCD press replaces a held one only when they + /// share a `Spell.dbc` category, instead of always replacing. + replace_matching_non_gcd_category: bool, + /// `NP_RetryServerRejectedSpells`: a cast the server refuses for a timing reason is sent + /// again instead of lost. The mitigation for the early guard — see [`Self::retry_reason`]. + retry_server_rejected: bool, } /// `NP_PreventMountingWhenBuffCapped`. pub(crate) const CVAR_PREVENT_MOUNTING: &str = "NP_PreventMountingWhenBuffCapped"; +/// The press-queue admission toggles. +pub(crate) const CVAR_QUEUE_CAST_TIME: &str = "NP_QueueCastTimeSpells"; +pub(crate) const CVAR_QUEUE_INSTANT: &str = "NP_QueueInstantSpells"; +pub(crate) const CVAR_QUEUE_CHANNELING: &str = "NP_QueueChannelingSpells"; +pub(crate) const CVAR_QUEUE_TARGETING: &str = "NP_QueueTargetingSpells"; +pub(crate) const CVAR_QUEUE_ON_SWING: &str = "NP_QueueOnSwingSpells"; +/// The four windows. +pub(crate) const CVAR_WINDOW_SPELL: &str = "NP_SpellQueueWindowMs"; +pub(crate) const CVAR_WINDOW_CHANNEL: &str = "NP_ChannelQueueWindowMs"; +pub(crate) const CVAR_WINDOW_COOLDOWN: &str = "NP_CooldownQueueWindowMs"; +pub(crate) const CVAR_WINDOW_TARGETING: &str = "NP_TargetingQueueWindowMs"; +/// The two buffers. +pub(crate) const CVAR_BUFFER: &str = "NP_MinBufferTimeMs"; +pub(crate) const CVAR_BUFFER_NON_GCD: &str = "NP_NonGcdBufferTimeMs"; +/// The behaviour toggles. +pub(crate) const CVAR_SPAM_PROTECTION: &str = "NP_SpamProtectionEnabled"; +pub(crate) const CVAR_INTERRUPT_OUTSIDE_WINDOW: &str = "NP_InterruptChannelsOutsideQueueWindow"; +pub(crate) const CVAR_REPLACE_MATCHING_NON_GCD: &str = "NP_ReplaceMatchingNonGcdCategory"; +pub(crate) const CVAR_RETRY_REJECTED: &str = "NP_RetryServerRejectedSpells"; + +/// A window or buffer's ceiling. The windows hold a press, so a huge one would fire a cast +/// minutes after it was asked for; the buffers delay every cast, so a huge one would feel like +/// the client had stopped answering. Both are typed by a player into a settings panel. +const MS_CEILING: f32 = 10_000.0; /// Apply a change of any press-side row. +/// +/// Written as one `if` per row rather than a table of `&mut` fields, which borrowck refuses: +/// several mutable borrows of one struct cannot sit in an array together. pub(crate) fn on_cvar(ev: On, mut rules: ResMut) { if ev.is(CVAR_PREVENT_MOUNTING) { rules.prevent_mounting_buff_capped = ev.flag(); } + if ev.is(CVAR_QUEUE_CAST_TIME) { + rules.queue_cast_time = ev.flag(); + } + if ev.is(CVAR_QUEUE_INSTANT) { + rules.queue_instant = ev.flag(); + } + if ev.is(CVAR_QUEUE_CHANNELING) { + rules.queue_channeling = ev.flag(); + } + if ev.is(CVAR_QUEUE_TARGETING) { + rules.queue_targeting = ev.flag(); + } + if ev.is(CVAR_QUEUE_ON_SWING) { + rules.queue_on_swing = ev.flag(); + } + if ev.is(CVAR_WINDOW_SPELL) { + rules.window_spell_ms = clamp_ms(&ev); + } + if ev.is(CVAR_WINDOW_CHANNEL) { + rules.window_channel_ms = clamp_ms(&ev); + } + if ev.is(CVAR_WINDOW_COOLDOWN) { + rules.window_cooldown_ms = clamp_ms(&ev); + } + if ev.is(CVAR_WINDOW_TARGETING) { + rules.window_targeting_ms = clamp_ms(&ev); + } + if ev.is(CVAR_BUFFER) { + rules.buffer_ms = clamp_ms(&ev); + } + if ev.is(CVAR_BUFFER_NON_GCD) { + rules.buffer_non_gcd_ms = clamp_ms(&ev); + } + if ev.is(CVAR_SPAM_PROTECTION) { + rules.spam_protection = ev.flag(); + } + if ev.is(CVAR_INTERRUPT_OUTSIDE_WINDOW) { + rules.interrupt_channels_outside_window = ev.flag(); + } + if ev.is(CVAR_REPLACE_MATCHING_NON_GCD) { + rules.replace_matching_non_gcd_category = ev.flag(); + } + if ev.is(CVAR_RETRY_REJECTED) { + rules.retry_server_rejected = ev.flag(); + } +} + +#[allow(clippy::cast_sign_loss, clippy::cast_possible_truncation)] // clamped non-negative +fn clamp_ms(ev: &crate::cvars::CvarChanged) -> u32 { + ev.num().clamp(0.0, MS_CEILING) as u32 +} + +/// What is keeping the client from sending this press right now, as the queue reads it. +/// +/// Gathered by the caller because the caller already holds every one of these — the press rules +/// own no state but the player's CVar rows, which is what keeps the whole decision testable. +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] +pub(crate) struct PressGuard { + /// The in-flight cast's guard end ([`super::PendingCast::guard_ends`]), and the spell it is + /// for; `None` with nothing in flight. + pub(crate) cast: Option<(u32, Instant)>, + /// The running channel's end. + pub(crate) channel: Option, + /// The global cooldown's end. + pub(crate) gcd: Option, + /// When this spell's **own** cooldown — individual or shared category, not the global one — + /// runs out. + pub(crate) cooldown: Option, +} + +impl PressGuard { + /// Whether anything at all is holding the press back. + fn any(&self) -> bool { + self.cast.is_some() + || self.channel.is_some() + || self.gcd.is_some() + || self.cooldown.is_some() + } +} + +/// What a press should do when [`PressRules::admit`] has looked at it. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) enum Press { + /// Hold it in this slot, giving up `window_ms` past the guard's end. + Queue { kind: Kind, window_ms: u32 }, + /// End the running channel and let the press through: `NP_InterruptChannelsOutsideQueueWindow` + /// with a channel that has more than its window left. + InterruptChannel, } impl PressRules { @@ -51,6 +235,222 @@ impl PressRules { pub(crate) fn for_tests(prevent_mounting_buff_capped: bool) -> Self { Self { prevent_mounting_buff_capped, + ..Self::defaults_for_tests() + } + } + + /// [`Self::defaults_for_tests`] with the five admission toggles off: what a player who + /// unticks nampower's queue in its settings panel runs, and the only configuration in which + /// the reference's own timing refusals are still reachable. The buffers stay at their + /// defaults, because those rows are not the queue and a player turning the queue off does not + /// turn them off. + #[cfg(test)] + pub(crate) fn queue_off_for_tests() -> Self { + Self { + queue_cast_time: false, + queue_instant: false, + queue_channeling: false, + queue_targeting: false, + queue_on_swing: false, + ..Self::defaults_for_tests() + } + } + + /// Every row at the default `cvars.rs` registers, so a test reads the behaviour a player who + /// touched nothing gets. + #[cfg(test)] + pub(crate) fn defaults_for_tests() -> Self { + Self { + prevent_mounting_buff_capped: true, + queue_cast_time: true, + queue_instant: true, + queue_channeling: true, + queue_targeting: true, + queue_on_swing: false, + window_spell_ms: 500, + window_channel_ms: 1_500, + window_cooldown_ms: 250, + window_targeting_ms: 500, + buffer_ms: 55, + buffer_non_gcd_ms: 100, + spam_protection: true, + interrupt_channels_outside_window: false, + replace_matching_non_gcd_category: false, + retry_server_rejected: true, + } + } + + /// The guard buffer for a spell: `NP_NonGcdBufferTimeMs` when it costs no global cooldown, + /// `NP_MinBufferTimeMs` otherwise. + /// + /// Why the two differ, which upstream's defaults of 100 and 55 only hint at: a spell with a + /// global cooldown has that cooldown between two presses whatever the guard does, so its + /// buffer need only cover packet jitter. A non-GCD spell has nothing else — the guard is the + /// only gap — so too small a number there is a second press the server reads as a double + /// cast. + pub(crate) fn buffer_for(&self, spell: Option<&SpellDisplay>) -> u32 { + if spell.is_some_and(|d| d.start_recovery_ms == 0) { + self.buffer_non_gcd_ms + } else { + self.buffer_ms + } + } + + /// Whether the press of `spell_id` should be held, interrupted into, or left to the ladder's + /// own refusals (`None`). + /// + /// The order of the tests is the order they can each disqualify a press: + /// + /// 1. **Nothing guards** — the common case, and the one that must cost nothing. + /// 2. **Spam protection**: a repeat press of the cast already in flight is the reference's own + /// silent bail (`6e4d43`). Holding it would mean casting it twice. + /// 3. **The class toggles**, read off the spell's own row. + /// 4. **The channel**, which has a toggle *and* an interrupt arm of its own. + /// 5. **The spell's own cooldown**, the one guard that is not about to end, and so the one + /// read as a threshold rather than a hold. + /// + /// `on_cooldown_allowed` is `NP_QueueSpellsOnCooldown`, passed in rather than read twice: it + /// lives on `SpellQueue`, where the explicit queue already reads it, so "may a cold spell + /// wait" has one answer and not two. + pub(crate) fn admit( + &self, + spell_id: u32, + spell: Option<&SpellDisplay>, + cast_time_ms: u32, + terrain_targeted: bool, + guard: &PressGuard, + on_cooldown_allowed: bool, + now: Instant, + ) -> Option { + if !guard.any() { + return None; + } + // The same spell, already in flight: the reference bails silently and so does this. With + // the row off the press falls through to the class tests and may replace itself, which is + // upstream's own behaviour for a player who turned spam protection off. + if self.spam_protection && guard.cast.is_some_and(|(id, _)| id == spell_id) { + return None; + } + let kind = Kind::of(spell); + // The class gates, most specific first: an on-swing strike is on-swing whether or not it + // also has a cast time, and a terrain spell is a terrain spell whatever its bar. + let class_ok = if kind == Kind::OnSwing { + self.queue_on_swing + } else if terrain_targeted { + self.queue_targeting + } else if cast_time_ms > 0 { + self.queue_cast_time + } else { + self.queue_instant + }; + if !class_ok { + return None; + } + if let Some(until) = guard.channel { + // `NP_InterruptChannelsOutsideQueueWindow`: the one row whose name says the window is + // a threshold on remaining time rather than a lifetime (see the module header). + if self.interrupt_channels_outside_window + && until.saturating_duration_since(now) + > Duration::from_millis(u64::from(self.window_channel_ms)) + { + return Some(Press::InterruptChannel); + } + if !self.queue_channeling { + return None; + } + } + // The spell's **own** cooldown, which is the one guard that is not about to end. + // + // A cast, a channel and a global cooldown all finish in a second or two, so holding a + // press through one is holding it for about as long as the player expected to wait. A + // thirty-minute item cooldown is not like that at all: holding that press means either + // firing it half an hour later or swallowing it with no error line, and a Hearthstone + // pressed on cooldown has to still say "Item is not ready yet". + // + // So this arm is the one place a window reads as a threshold on **remaining** time, and + // `NP_CooldownQueueWindowMs`' default of 250 ms is the evidence for it: a quarter of a + // second is an allowance for pressing a touch early, not a queue. Inside it the press is + // held; outside it the press goes to the ladder and the player reads the truth. + // `NP_QueueSpellsOnCooldown` is the on/off for the whole arm — the same row that decides, + // in `SpellQueue`, whether an already-held entry whose spell went cold is kept. + if let Some(until) = guard.cooldown { + let within = until.saturating_duration_since(now) + <= Duration::from_millis(u64::from(self.window_cooldown_ms)); + if !on_cooldown_allowed || !within { + return None; + } + } + Some(Press::Queue { + kind, + window_ms: self.window_for(terrain_targeted, guard), + }) + } + + /// Which of the four windows a held press gives up on. + /// + /// A terrain spell takes its own whatever guards it, because `NP_TargetingQueueWindowMs` has + /// no other guard to be about. Otherwise the longest-running guard picks: the channel's, the + /// spell's own cooldown's, or the general one, which covers both a cast in flight and a + /// global cooldown — the two things `NP_SpellQueueWindowMs`' unqualified name can mean. + fn window_for(&self, terrain_targeted: bool, guard: &PressGuard) -> u32 { + if terrain_targeted { + return self.window_targeting_ms; + } + let candidates = [ + (guard.channel, self.window_channel_ms), + (guard.cooldown, self.window_cooldown_ms), + (guard.cast.map(|(_, until)| until), self.window_spell_ms), + (guard.gcd, self.window_spell_ms), + ]; + candidates + .into_iter() + .filter_map(|(until, window)| until.map(|u| (u, window))) + .max_by_key(|(until, _)| *until) + .map_or(self.window_spell_ms, |(_, window)| window) + } + + /// `NP_RetryServerRejectedSpells`: whether a server refusal with this `SpellCastResult` is one + /// to send again. + /// + /// **Two codes, and deliberately only two.** `0x3c` not-ready and `0x61` another-action are + /// what an early guard can cause: the client decided the previous cast was over a few + /// milliseconds before the server agreed. Every other refusal — no power, out of range, no + /// line of sight, immune — is the server telling the player something true, and re-sending it + /// would be a packet a second that never succeeds. Upstream's own set is not published and + /// its log line (" spell queued + ignored code ") suggests a wider one; a narrower set is the + /// safe direction to be wrong in, and it is written down here rather than discovered later. + pub(crate) fn retry_reason(&self, reason: u8) -> bool { + self.retry_server_rejected && matches!(reason, 0x3c | 0x61) + } + + /// How long a retried cast is held before it gives up: `NP_SpellQueueWindowMs`, the general + /// window. + /// + /// Not one of the four by guard kind, because a retry has no guard — the cast it answers is + /// already over, refused. The general window is the one whose name is unqualified, and it is + /// the right size for the question "how late may this still fire". + pub(crate) fn retry_window_ms(&self) -> u32 { + self.window_spell_ms + } + + /// `NP_ReplaceMatchingNonGcdCategory`: whether a non-GCD press may take the slot a held one + /// occupies. + /// + /// Off (upstream's default) every press replaces, which is the plain "Replacing queued nonGcd + /// spell X with Y". On, only a press sharing the held spell's `Spell.dbc` category does, so + /// two unrelated off-cooldown abilities pressed together do not cancel each other. + pub(crate) fn non_gcd_may_replace( + &self, + pressed: Option<&SpellDisplay>, + held: Option<&SpellDisplay>, + ) -> bool { + if !self.replace_matching_non_gcd_category { + return true; + } + match (pressed.map(|d| d.category), held.map(|d| d.category)) { + // A category of 0 is "no shared cooldown", which matches nothing, not everything. + (Some(a), Some(b)) => a != 0 && a == b, + _ => false, } } @@ -149,9 +549,7 @@ mod tests { /// 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 on = PressRules::defaults_for_tests(); let off = PressRules::default(); let capped = store(UNIT_AURA_POSITIVE_SLOTS, 0); @@ -188,9 +586,7 @@ mod tests { /// 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 on = PressRules::defaults_for_tests(); 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!( @@ -199,6 +595,376 @@ mod tests { ); } + const FROSTBOLT: u32 = 116; + const PYROBLAST: u32 = 11366; + + /// A guard with a cast of `spell_id` running until `ms` from `t0`. + fn casting(t0: Instant, spell_id: u32, ms: u64) -> PressGuard { + PressGuard { + cast: Some((spell_id, t0 + Duration::from_millis(ms))), + ..Default::default() + } + } + + /// An instant with a global cooldown: the ordinary rotation spell. + fn instant() -> SpellDisplay { + SpellDisplay { + start_recovery_ms: 1_500, + start_recovery_category: 133, + ..Default::default() + } + } + + /// With nothing in the way the rules must answer "nothing" — the common case on every press + /// of every key, and the one that has to cost nothing and change nothing. + #[test] + fn a_press_with_nothing_guarding_it_is_never_held() { + let r = PressRules::defaults_for_tests(); + assert_eq!( + r.admit( + FROSTBOLT, + Some(&instant()), + 0, + false, + &PressGuard::default(), + true, + Instant::now() + ), + None + ); + } + + /// The five class toggles, each refusing only its own class. + #[test] + fn each_class_toggle_governs_only_its_own_kind_of_press() { + let t0 = Instant::now(); + let guard = casting(t0, PYROBLAST, 3_000); + let held = |r: &PressRules, cast_time: u32, terrain: bool, spell: &SpellDisplay| { + r.admit(FROSTBOLT, Some(spell), cast_time, terrain, &guard, true, t0) + .is_some() + }; + let all_on = PressRules::defaults_for_tests(); + + // The defaults hold a cast-time press, an instant and a terrain spell, and not a strike. + assert!(held(&all_on, 1_500, false, &instant()), "a cast-time press"); + assert!(held(&all_on, 0, false, &instant()), "an instant"); + assert!(held(&all_on, 0, true, &instant()), "a terrain spell"); + let strike = SpellDisplay { + attributes: 0x404, + ..Default::default() + }; + assert!( + !held(&all_on, 0, false, &strike), + "upstream's own default for on-swing is 0, so a strike is not held" + ); + + // And each row, alone, takes its own class out. + let without = |f: fn(&mut PressRules)| { + let mut r = PressRules::defaults_for_tests(); + f(&mut r); + r + }; + assert!(!held( + &without(|r| r.queue_cast_time = false), + 1_500, + false, + &instant() + )); + assert!( + held( + &without(|r| r.queue_cast_time = false), + 0, + false, + &instant() + ), + "turning the cast-time row off leaves instants alone" + ); + assert!(!held( + &without(|r| r.queue_instant = false), + 0, + false, + &instant() + )); + assert!(!held( + &without(|r| r.queue_targeting = false), + 0, + true, + &instant() + )); + assert!( + held(&without(|r| r.queue_on_swing = true), 0, false, &strike), + "and switching the on-swing row on holds a strike" + ); + assert!( + held( + &without(|r| r.queue_cast_time = false), + 1_500, + true, + &instant() + ), + "a terrain spell with a cast bar is governed by the terrain row, not the cast-time \ + one — the class tests are most-specific-first" + ); + } + + /// `NP_SpamProtectionEnabled`: a repeat press of the cast already in flight is swallowed, not + /// held. Holding it would mean casting the same spell twice off one extra keypress. + #[test] + fn spam_protection_swallows_a_repeat_of_the_cast_in_flight() { + let t0 = Instant::now(); + let guard = casting(t0, FROSTBOLT, 1_500); + let r = PressRules::defaults_for_tests(); + assert_eq!( + r.admit(FROSTBOLT, Some(&instant()), 1_500, false, &guard, true, t0), + None, + "the same spell again: the reference's own silent bail, and no slot" + ); + assert!( + r.admit(PYROBLAST, Some(&instant()), 1_500, false, &guard, true, t0) + .is_some(), + "a different spell is the press the queue exists for" + ); + let mut off = PressRules::defaults_for_tests(); + off.spam_protection = false; + assert!( + off.admit(FROSTBOLT, Some(&instant()), 1_500, false, &guard, true, t0) + .is_some(), + "off, the repeat falls through to the class tests, as upstream's own row says" + ); + } + + /// Which of the four windows a held press gives up on: the longest-running guard picks, + /// except a terrain spell, which takes its own whatever guards it. + #[test] + fn the_window_is_the_longest_guards_own() { + let t0 = Instant::now(); + let r = PressRules::defaults_for_tests(); + let at = |ms: u64| Some(t0 + Duration::from_millis(ms)); + + assert_eq!( + r.window_for(false, &casting(t0, PYROBLAST, 3_000)), + 500, + "a cast in flight takes the general window" + ); + assert_eq!( + r.window_for( + false, + &PressGuard { + gcd: at(1_500), + ..Default::default() + } + ), + 500, + "so does a global cooldown — `NP_SpellQueueWindowMs`' name is the unqualified one" + ); + assert_eq!( + r.window_for( + false, + &PressGuard { + channel: at(4_000), + gcd: at(1_500), + ..Default::default() + } + ), + 1_500, + "the channel outlasts the global cooldown, so the channel's window applies" + ); + assert_eq!( + r.window_for( + false, + &PressGuard { + cooldown: at(2_000), + channel: at(1_000), + ..Default::default() + } + ), + 250, + "the spell's own cooldown outlasts the channel, so its window applies" + ); + assert_eq!( + r.window_for( + true, + &PressGuard { + cooldown: at(200), + ..Default::default() + } + ), + 500, + "a terrain spell takes `NP_TargetingQueueWindowMs` whatever guards it" + ); + assert_eq!( + r.window_for(false, &PressGuard::default()), + 500, + "and with no guard at all the general window is the answer — `admit` never asks, but \ + a window of 0 would be a silent zero-length hold if it ever did" + ); + } + + /// The channel's two rows. `NP_QueueChannelingSpells` decides whether a press is held; + /// `NP_InterruptChannelsOutsideQueueWindow` reads the window as a threshold on **remaining** + /// time, which is the one place that reading is right (see the module header). + #[test] + fn a_channel_is_held_or_broken_by_its_own_two_rows() { + let t0 = Instant::now(); + let long = PressGuard { + channel: Some(t0 + Duration::from_secs(4)), + ..Default::default() + }; + let nearly_done = PressGuard { + channel: Some(t0 + Duration::from_millis(900)), + ..Default::default() + }; + let press = |r: &PressRules, g: &PressGuard| { + r.admit(FROSTBOLT, Some(&instant()), 0, false, g, true, t0) + }; + + let r = PressRules::defaults_for_tests(); + assert!( + matches!(press(&r, &long), Some(Press::Queue { .. })), + "the defaults hold the press and leave the channel running" + ); + + let mut interrupt = PressRules::defaults_for_tests(); + interrupt.interrupt_channels_outside_window = true; + assert_eq!( + press(&interrupt, &long), + Some(Press::InterruptChannel), + "4 s left is more than the 1.5 s channel window: outside it, so the channel breaks" + ); + assert!( + matches!(press(&interrupt, &nearly_done), Some(Press::Queue { .. })), + "900 ms left is inside the window — nearly finished channels are worth finishing" + ); + + let mut no_channel_queue = PressRules::defaults_for_tests(); + no_channel_queue.queue_channeling = false; + assert_eq!( + press(&no_channel_queue, &long), + None, + "the row off, a press mid-channel is left to the ladder" + ); + } + + /// `NP_QueueSpellsOnCooldown`, which lives on `SpellQueue` so both halves of the queue read + /// one answer: off, a press of a cold spell is left to the ladder's own "not ready" rather + /// than held until the cooldown ends. + /// The cooldown arm: a press just before the cooldown ends is held, one made while half an + /// hour is left is not — it goes to the ladder so the player reads "not ready" instead of + /// losing the press into a queue with no answer. + #[test] + fn a_press_of_a_cold_spell_is_held_only_inside_the_cooldown_window() { + let t0 = Instant::now(); + let cold_for = |ms: u64| PressGuard { + cooldown: Some(t0 + Duration::from_millis(ms)), + ..Default::default() + }; + let r = PressRules::defaults_for_tests(); + let held = |g: &PressGuard, allowed: bool| { + r.admit(FROSTBOLT, Some(&instant()), 0, false, g, allowed, t0) + .is_some() + }; + + assert!( + held(&cold_for(200), true), + "200 ms left is inside the 250 ms window: pressed a touch early, so held" + ); + assert!( + !held(&cold_for(1_800_000), true), + "a thirty-minute item cooldown is not a queue — the press goes to the ladder, which \ + tells the player it is not ready" + ); + assert!( + !held(&cold_for(200), false), + "`NP_QueueSpellsOnCooldown` off switches the whole arm off" + ); + } + + /// The retry set: two timing codes and nothing else, whatever the row says. + #[test] + fn the_retry_set_is_the_two_codes_an_early_guard_can_cause() { + let r = PressRules::defaults_for_tests(); + assert!(r.retry_reason(0x3c), "not ready"); + assert!(r.retry_reason(0x61), "another action is in progress"); + for refusal in [0x4d, 0x0a, 0x13, 0x2e, 0x39, 0x78, 0x17, 0x00] { + assert!( + !r.retry_reason(refusal), + "{refusal:#x} is the server telling the player something true — re-sending it \ + is a packet a second that never succeeds" + ); + } + let mut off = PressRules::defaults_for_tests(); + off.retry_server_rejected = false; + assert!(!off.retry_reason(0x3c)); + } + + /// `NP_ReplaceMatchingNonGcdCategory`: off, every press replaces; on, only a shared + /// `Spell.dbc` category does — and a category of 0 matches nothing, not everything. + #[test] + fn a_non_gcd_press_replaces_by_category_only_when_asked() { + let cat = |category: u32| SpellDisplay { + category, + ..Default::default() + }; + let off = PressRules::defaults_for_tests(); + assert!(off.non_gcd_may_replace(Some(&cat(1)), Some(&cat(2)))); + assert!(off.non_gcd_may_replace(None, None)); + + let mut on = PressRules::defaults_for_tests(); + on.replace_matching_non_gcd_category = true; + assert!(on.non_gcd_may_replace(Some(&cat(7)), Some(&cat(7)))); + assert!(!on.non_gcd_may_replace(Some(&cat(7)), Some(&cat(8)))); + assert!( + !on.non_gcd_may_replace(Some(&cat(0)), Some(&cat(0))), + "category 0 is \"no shared cooldown\" — two of those are not a match" + ); + assert!( + !on.non_gcd_may_replace(None, Some(&cat(7))), + "an uncatalogued press cannot be shown to match, so it does not displace" + ); + } + + /// The buffer forks on the global cooldown, because a non-GCD spell has nothing else between + /// two presses. + #[test] + fn the_buffer_forks_on_whether_the_spell_costs_a_global_cooldown() { + let r = PressRules::defaults_for_tests(); + assert_eq!(r.buffer_for(Some(&instant())), 55); + assert_eq!( + r.buffer_for(Some(&SpellDisplay::default())), + 100, + "`start_recovery_ms == 0`: `NP_NonGcdBufferTimeMs`" + ); + assert_eq!( + r.buffer_for(None), + 55, + "an uncatalogued spell takes the general buffer, not the wider one" + ); + } + + /// A window or buffer a player typed into the settings panel cannot hold a press for an hour + /// or stall every cast. + #[test] + fn a_window_typed_by_hand_is_clamped() { + let mut app = App::new(); + app.init_resource::().add_observer(on_cvar); + let set = |app: &mut App, name: &'static str, value: &'static str| { + app.world_mut().trigger(crate::cvars::CvarChanged { + name: name.to_string(), + old: String::new(), + new: value.to_string(), + }); + app.world_mut().flush(); + }; + set(&mut app, CVAR_WINDOW_SPELL, "999999"); + set(&mut app, CVAR_BUFFER, "-40"); + let r = app.world().resource::(); + assert_eq!(r.window_spell_ms, MS_CEILING as u32); + assert_eq!( + r.buffer_ms, 0, + "a negative buffer is no buffer, not a panic" + ); + } + /// 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. /// diff --git a/crates/benilla-app/src/spell/queue.rs b/crates/benilla-app/src/spell/queue.rs index f0855eef..5003ed79 100644 --- a/crates/benilla-app/src/spell/queue.rs +++ b/crates/benilla-app/src/spell/queue.rs @@ -83,6 +83,10 @@ struct Slot { spell_id: u32, /// When this entry gives up. See [`SpellQueue::queue`] for where it comes from. expires: Instant, + /// The earliest [`SpellQueue::pop`] will take it. `now` for an ordinary entry, which is to + /// say no constraint at all; later only for a retry (`NP_RetryServerRejectedSpells`), where + /// the point is to not re-send inside the same millisecond the server just refused. + not_before: Instant, } /// One queued script, with the priority that orders it. @@ -118,9 +122,24 @@ pub(crate) struct SpellQueue { allow_on_cooldown: bool, /// `ChannelStopCastingNextTick`: the channel is cancelled on the next frame, not this one. stop_channel: bool, + /// The spell a server refusal has already been retried for. See [`Self::queue_retry`]. + retried: Option, } impl SpellQueue { + /// The resource as `cvars.rs` seeds it: `NP_SpellQueueWindowMs` 500 and + /// `NP_QueueSpellsOnCooldown` 1. Test-only, so a test world reads the behaviour a player who + /// touched nothing gets rather than `Default`'s all-zeroes, which is a configuration nobody + /// runs. + #[cfg(test)] + pub(crate) fn defaults_for_tests() -> Self { + Self { + window_ms: 500, + allow_on_cooldown: true, + ..Default::default() + } + } + /// Put `spell_id` in the slot its `kind` implies, replacing whatever was there. /// /// **Replacing, not refusing**, which is upstream's own behaviour — its log line is @@ -137,16 +156,70 @@ impl SpellQueue { kind: Kind, now: Instant, in_flight_until: Option, + ) { + self.queue_with_window(spell_id, kind, now, in_flight_until, self.window_ms); + } + + /// [`Self::queue`] with the window named by the caller, for the press-interception half: a + /// held **press** gives up on one of four windows picked by what guarded it + /// (`np_press.rs`'s `window_for`), where an explicit `QueueSpellByName` has only ever had the + /// one. Both land in the same slot and raise the same event, because upstream has one queue. + pub(crate) fn queue_with_window( + &mut self, + spell_id: u32, + kind: Kind, + now: Instant, + in_flight_until: Option, + window_ms: u32, ) { let until = in_flight_until.unwrap_or(now).max(now); let slot = Slot { spell_id, - expires: until + Duration::from_millis(u64::from(self.window_ms)), + expires: until + Duration::from_millis(u64::from(window_ms)), + not_before: now, }; *self.slot_mut(kind) = Some(slot); self.events.push((kind.queued_code(), spell_id)); } + /// Put `spell_id` back after the **server** refused it for a timing reason + /// (`NP_RetryServerRejectedSpells`), held `delay_ms` before it may be sent again. + /// + /// **Once per spell, and that bound is the whole design.** An unbounded retry of a refusal + /// that keeps arriving is a packet a second forever, which is worse than the dropped cast it + /// was meant to save — upstream's own `Retry cast after cancel still failed` says it gives up + /// too. [`Self::clear_retry`] forgets the memo on any cast the server accepts, so a second + /// rejection later in the fight is retried again; what cannot happen is two retries of one + /// press. + /// + /// Returns whether it was queued, for the caller's log line. + pub(crate) fn queue_retry( + &mut self, + spell_id: u32, + kind: Kind, + now: Instant, + delay_ms: u32, + window_ms: u32, + ) -> bool { + if self.retried == Some(spell_id) { + return false; + } + self.retried = Some(spell_id); + let not_before = now + Duration::from_millis(u64::from(delay_ms)); + *self.slot_mut(kind) = Some(Slot { + spell_id, + expires: not_before + Duration::from_millis(u64::from(window_ms)), + not_before, + }); + self.events.push((kind.queued_code(), spell_id)); + true + } + + /// Forget the retry memo: a cast the server accepted means the next refusal is a new one. + pub(crate) fn clear_retry(&mut self) { + self.retried = None; + } + /// Queue a chunk for [`Self::take_ready_scripts`]. pub(crate) fn queue_script(&mut self, chunk: String, priority: i64, now: Instant) { self.scripts.push(Script { @@ -165,7 +238,13 @@ impl SpellQueue { } /// Take the spell in `kind`'s slot if it is still live, raising its popped event. - pub(crate) fn pop(&mut self, kind: Kind) -> Option { + /// + /// `now` is read only to honour a retry's hold ([`Self::queue_retry`]); an ordinary entry is + /// never held, so this is the same unconditional take it always was for one. + pub(crate) fn pop(&mut self, kind: Kind, now: Instant) -> Option { + if !self.ready(kind, now) { + return None; + } let spell_id = self.slot_mut(kind).take()?.spell_id; self.events.push((kind.popped_code(), spell_id)); Some(spell_id) @@ -173,12 +252,20 @@ impl SpellQueue { /// What is in `kind`'s slot, without taking it. pub(crate) fn peek(&self, kind: Kind) -> Option { + self.slot(kind).map(|s| s.spell_id) + } + + /// Whether `kind`'s slot holds something [`Self::pop`] would take now. + pub(crate) fn ready(&self, kind: Kind, now: Instant) -> bool { + self.slot(kind).is_some_and(|s| now >= s.not_before) + } + + fn slot(&self, kind: Kind) -> Option { match kind { Kind::OnSwing => self.on_swing, Kind::NonGcd => self.non_gcd, Kind::Normal => self.normal, } - .map(|s| s.spell_id) } /// Drop every entry whose window has passed. @@ -324,10 +411,10 @@ mod tests { counting pops against queues would see one too many" ); - assert_eq!(q.pop(Kind::Normal), Some(116)); + assert_eq!(q.pop(Kind::Normal, t0), Some(116)); assert_eq!(q.peek(Kind::Normal), None); assert_eq!(q.take_events(), vec![(queue_event::NORMAL_POPPED, 116)]); - assert_eq!(q.pop(Kind::Normal), None, "an empty slot pops nothing"); + assert_eq!(q.pop(Kind::Normal, t0), None, "an empty slot pops nothing"); assert!(q.take_events().is_empty()); // The three slots are independent: a normal spell does not displace a strike. @@ -393,6 +480,50 @@ mod tests { assert_eq!(q.peek(Kind::Normal), Some(133)); } + /// `NP_RetryServerRejectedSpells`: a refused cast goes back in its slot, held for the delay, + /// and **once** — a retry that could itself be retried is a packet a second forever. + #[test] + fn a_retry_is_held_for_its_delay_and_happens_once() { + let t0 = Instant::now(); + let mut q = SpellQueue { + window_ms: 500, + ..Default::default() + }; + + assert!(q.queue_retry(133, Kind::Normal, t0, 55, 500)); + assert_eq!(q.peek(Kind::Normal), Some(133)); + assert!( + !q.ready(Kind::Normal, t0 + Duration::from_millis(40)), + "inside the delay the entry is there but not sendable — re-sending inside the \ + millisecond the server refused it is the one thing a retry must not do" + ); + assert_eq!(q.pop(Kind::Normal, t0 + Duration::from_millis(40)), None); + assert!(q.ready(Kind::Normal, t0 + Duration::from_millis(55))); + assert_eq!( + q.pop(Kind::Normal, t0 + Duration::from_millis(55)), + Some(133) + ); + + // The same spell refused again is not retried a second time. + assert!( + !q.queue_retry(133, Kind::Normal, t0, 55, 500), + "once per rejection chain" + ); + assert_eq!(q.peek(Kind::Normal), None, "and nothing was put back"); + + // A cast the server accepted forgets the memo, so a later rejection is its own. + q.clear_retry(); + assert!(q.queue_retry(133, Kind::Normal, t0, 55, 500)); + + // An ordinary queue is never held: `ready` is about retries alone. + let mut plain = SpellQueue { + window_ms: 500, + ..Default::default() + }; + plain.queue(133, Kind::Normal, t0, None); + assert!(plain.ready(Kind::Normal, t0)); + } + /// Scripts come back in ascending priority, and two at one priority keep their order — /// `SuperCleveRoidMacros` passes `3` for "after queued spells", so a bigger number is later. #[test] diff --git a/crates/benilla-app/src/spell/targeting/mod.rs b/crates/benilla-app/src/spell/targeting/mod.rs index ecdfdfc6..6eeb5049 100644 --- a/crates/benilla-app/src/spell/targeting/mod.rs +++ b/crates/benilla-app/src/spell/targeting/mod.rs @@ -507,6 +507,8 @@ mod tests { world.insert_resource(crate::net::Reputations(Vec::new())); world.init_resource::(); world.init_resource::(); + 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 609cb536..9f0771ca 100644 --- a/crates/benilla-app/src/spell/targeting/world.rs +++ b/crates/benilla-app/src/spell/targeting/world.rs @@ -142,6 +142,8 @@ mod tests { world.insert_resource(crate::net::Reputations(Vec::new())); world.init_resource::(); world.init_resource::(); + 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 197d71e5..f98280d6 100644 --- a/crates/benilla-app/src/ui_items/drain.rs +++ b/crates/benilla-app/src/ui_items/drain.rs @@ -762,6 +762,8 @@ mod tests { .init_resource::() .init_resource::() .init_resource::() + .init_resource::() + .init_resource::() .init_resource::() .init_resource::() .insert_resource(NetCommands(tx)); @@ -1721,6 +1723,8 @@ mod bind_confirm_tests { .init_resource::() .init_resource::() .init_resource::() + .init_resource::() + .init_resource::() .init_resource::() .init_resource::() .insert_resource(NetCommands(tx)); diff --git a/crates/benilla-app/src/ui_self_cast.rs b/crates/benilla-app/src/ui_self_cast.rs index 2122b9e3..89c7e827 100644 --- a/crates/benilla-app/src/ui_self_cast.rs +++ b/crates/benilla-app/src/ui_self_cast.rs @@ -265,7 +265,7 @@ mod tests { let now = Instant::now(); app.world_mut() .resource_mut::() - .arm(133, now, 3_000, true); + .arm(133, now, 3_000, true, 55); app.world_mut().resource_mut::().arm(78); app.world_mut().resource_mut::().0 = Some(75); app.world_mut().run_system_once(feed_self_cast).unwrap(); @@ -379,7 +379,7 @@ mod tests { .unwrap(); app.world_mut() .resource_mut::() - .arm(133, Instant::now(), 3_000, true); + .arm(133, Instant::now(), 3_000, true, 55); app.world_mut() .resource_mut::() .push(SelfCastSend { diff --git a/crates/benilla-app/src/ui_spellbook.rs b/crates/benilla-app/src/ui_spellbook.rs index a1dbed40..fc932feb 100644 --- a/crates/benilla-app/src/ui_spellbook.rs +++ b/crates/benilla-app/src/ui_spellbook.rs @@ -446,12 +446,10 @@ fn drain_spell_casts( script: Option>, targeting: cast_target::CastTargeting, hovered: Option>, + // nampower's queue slots and the running channel ride `CastLadder` now that **every** press + // can land in a slot (`np_press.rs`): a system holding either beside the ladder would reach + // the same resource twice, which Bevy answers with a run-time panic. mut ladder: CastLadder, - // nampower's explicit queue (`spell/queue.rs`). Folded into this system for the reason its - // own header gives: only one system may hold `CastLadder`, and the queue's pop is a cast - // send like any other. - mut queue: ResMut, - channel: Res, // nampower's `DisenchantAll` run, driven from here for the same reason: it sends casts. mut disenchant: ResMut, items: Res, @@ -507,7 +505,15 @@ fn drain_spell_casts( ); ladder.send(spell_id, &targeting.context(), CastCommit::Spell); } - drain_spell_queue(&mut script, &targeting, &mut ladder, &mut queue, &channel); + // nampower's `CastSpellNoQueue` / `CastSpellByNameNoQueue`: the same tail as the ordinary + // cast above, through the ladder entry that skips the press queue. The toggle forks are not + // repeated — those read the press as a *toggle*, and a macro saying "cast this, do not queue + // it" has asked for a cast. + for spell_id in script.take_no_queue_spell_casts() { + debug!("ui_spellbook: no-queue cast {spell_id}"); + ladder.send_now(spell_id, &targeting.context(), CastCommit::Spell); + } + drain_spell_queue(&mut script, &targeting, &mut ladder); drive_disenchant( &mut script, &targeting, @@ -542,8 +548,6 @@ fn drain_spell_queue( script: &mut UiScript, targeting: &cast_target::CastTargeting, ladder: &mut CastLadder, - queue: &mut crate::spell::queue::SpellQueue, - channel: &crate::spell::ActiveChannel, ) { use crate::spell::queue::Kind; @@ -552,9 +556,9 @@ fn drain_spell_queue( // `ChannelStopCastingNextTick`: armed on the frame it was called, sent on the next. The delay // is the point — see the binding. if script.take_channel_stops() > 0 { - queue.request_channel_stop(); - } else if queue.take_channel_stop() { - if let Some(spell_id) = channel.current(now) { + ladder.queue.request_channel_stop(); + } else if ladder.queue.take_channel_stop() { + if let Some(spell_id) = ladder.channel.current(now) { debug!("spell-queue: stopping the channel of {spell_id} (next tick)"); let _ = ladder .commands @@ -566,7 +570,7 @@ fn drain_spell_queue( // The requests, resolved once here: a name becomes a spell id through the player's own book, // which is the same walk `CastSpellByName` makes, so one rank cannot be queued and another // cast. - let in_flight_until = ladder.pending.deadline(); + let in_flight_until = ladder.pending.guard_ends(); for request in script.take_queue_requests() { match request { benilla_ui::script::QueueRequest::Spell { name, priority } => { @@ -581,20 +585,29 @@ fn drain_spell_queue( debug!( "spell-queue: queueing {spell_id} ({name:?}) as {kind:?}, priority {priority}" ); - queue.queue(spell_id, kind, now, in_flight_until); + ladder.queue.queue(spell_id, kind, now, in_flight_until); } benilla_ui::script::QueueRequest::Script { chunk, priority } => { - queue.queue_script(chunk, priority, now); + ladder.queue.queue_script(chunk, priority, now); } } } - queue.expire(now); + ladder.queue.expire(now); // The pop. `in_flight` is the ladder's own guard test, so a queued spell waits exactly as // long as a press would have been refused for. - let guarded = ladder.pending.in_flight(now); + // + // A running **channel** holds every slot back too. That gate is new with the + // press-interception half: an explicit `QueueSpellByName` could only ever be popped into a + // channel by a macro that asked for it, where a held *press* made mid-channel is the ordinary + // case, and sending it before the channel ends would cancel the channel server-side — the + // player's own press interrupting their own cast, which is not what holding it meant. + let guarded = ladder.pending.in_flight(now) || ladder.channel.current(now).is_some(); let gcd_running = ladder.cooldowns.gcd(now).is_some(); + // Decided first and sent after, because a send borrows the whole ladder where a slot read + // borrows one field of it. + let mut popped: Vec = Vec::new(); for (kind, ready) in [ (Kind::NonGcd, !guarded), (Kind::Normal, !guarded && !gcd_running), @@ -603,7 +616,13 @@ fn drain_spell_queue( if !ready { continue; } - let Some(spell_id) = queue.peek(kind) else { + // `ready` and not `peek`: a retry's hold (`NP_RetryServerRejectedSpells`) must keep the + // cooldown test below from reading it as "cold now, drop it" a millisecond after the + // server refused it. + if !ladder.queue.ready(kind, now) { + continue; + } + let Some(spell_id) = ladder.queue.peek(kind) else { continue; }; // The spell's own cooldown, which `NP_QueueSpellsOnCooldown` governs: off, an entry whose @@ -611,24 +630,30 @@ fn drain_spell_queue( // the cooldown ends — which is a cast the player asked for a long time ago. let def = ladder.spells.as_ref().and_then(|s| s.catalog.get(spell_id)); if ladder.cooldowns.not_ready(spell_id, 0, def, now) { - if queue.allows_on_cooldown() { + if ladder.queue.allows_on_cooldown() { continue; } debug!("spell-queue: dropping {spell_id} — on cooldown, and NP_QueueSpellsOnCooldown is off"); - queue.pop(kind); + ladder.queue.pop(kind, now); continue; } - let Some(spell_id) = queue.pop(kind) else { + let Some(spell_id) = ladder.queue.pop(kind, now) else { continue; }; debug!("spell-queue: popping {kind:?} spell {spell_id}"); - ladder.send(spell_id, &targeting.context(), CastCommit::Spell); + popped.push(spell_id); + } + for spell_id in popped { + // `send_now`, not `send`: a pop that could be held again is a loop that never casts + // anything. Nothing guards at this point, so the press rules would answer "send" anyway — + // this says so at the call rather than relying on that staying true. + ladder.send_now(spell_id, &targeting.context(), CastCommit::Spell); } // The scripts, once every **spell** slot is empty — "after queued spells", which is // `SuperCleveRoidMacros`' own words for what it uses `QueueScript` for. - if queue.no_spells_waiting() { - for chunk in queue.take_ready_scripts() { + if ladder.queue.no_spells_waiting() { + for chunk in ladder.queue.take_ready_scripts() { debug!( "spell-queue: running a queued script ({} bytes)", chunk.len() @@ -637,7 +662,7 @@ fn drain_spell_queue( } } - for (code, spell_id) in queue.take_events() { + for (code, spell_id) in ladder.queue.take_events() { script.fire_event( "SPELL_QUEUE_EVENT", vec![ @@ -749,6 +774,142 @@ mod tests { use benilla_formats::SpellDisplay; use std::collections::HashMap; + /// The whole press-queue cycle through the real drain: a press held by nampower's queue is + /// **sent** once the guard clears, and sent exactly once. + /// + /// Worth the setup, because the failure this guards against is the one a player would hit and + /// not be able to describe — a keypress that produced no packet, no error line and no cast. + /// Every other test of the queue proves a decision; this proves the press comes back out. + #[test] + fn a_press_the_queue_held_is_sent_when_the_guard_clears_and_sent_once() { + use crate::spell::queue::{Kind, SpellQueue}; + use benilla_protocol::messages::ObjectType; + use benilla_protocol::ObjectFields; + use bevy::ecs::system::RunSystemOnce; + use std::time::Instant; + + const FROSTBOLT: u32 = 116; + const FIREBALL: u32 = 133; + + let (tx, rx) = crossbeam_channel::unbounded(); + let mut app = App::new(); + app.insert_resource(crate::net::NetCommands(tx)) + .init_resource::() + .init_resource::() + .init_resource::() + .init_resource::() + .init_resource::() + .init_resource::() + .init_resource::() + .init_resource::() + .init_resource::() + .init_resource::() + .init_resource::() + .init_resource::() + .init_resource::() + .init_resource::() + .init_resource::() + .init_resource::() + .init_resource::() + .insert_resource(crate::spell::np_press::PressRules::defaults_for_tests()) + .insert_resource(SpellQueue::defaults_for_tests()) + .add_message::(); + // Two ordinary cast-time spells, so the press lands in the Normal slot. + let mut spells = crate::ui_action::Spells::empty_for_tests(); + spells.catalog = benilla_formats::SpellCatalog::from_displays(HashMap::from([ + ( + FROSTBOLT, + SpellDisplay { + start_recovery_ms: 1_500, + start_recovery_category: 133, + ..Default::default() + }, + ), + ( + FIREBALL, + SpellDisplay { + start_recovery_ms: 1_500, + start_recovery_category: 133, + ..Default::default() + }, + ), + ])); + app.insert_resource(spells); + app.world_mut().spawn(( + crate::net::SelfPlayer, + crate::net::ObjectStore( + ObjectFields::from_pairs(&[(22, 100), (28, 100)]).into_created(ObjectType::Player), + ), + Transform::default(), + )); + + // A Fireball in flight, and a Frostbolt pressed into it: `send` is the ladder a bar press + // takes, so this is the player's own keypress and not a back door into the slot. + let t0 = Instant::now(); + app.world_mut() + .resource_mut::() + .arm(FIREBALL, t0, 3_000, true, 55); + app.world_mut() + .run_system_once( + move |mut ladder: crate::spell::CastLadder, t: cast_target::CastTargeting| { + ladder.send(FROSTBOLT, &t.context(), CastCommit::Spell); + }, + ) + .expect("the ladder runs as a one-shot system"); + assert!(rx.try_recv().is_err(), "nothing on the wire: it was held"); + assert_eq!( + app.world().resource::().peek(Kind::Normal), + Some(FROSTBOLT) + ); + assert!(app + .world() + .resource::() + .0 + .is_empty()); + + // The drain with the guard still up sends nothing — a held press must not leak out early. + let script = benilla_ui::script::UiScript::new().expect("a VM"); + app.insert_non_send_resource(script); + app.world_mut() + .run_system_once(drain_spell_casts) + .expect("the drain runs as a one-shot system"); + assert!( + rx.try_recv().is_err(), + "the Fireball still guards, so the slot stays full" + ); + assert_eq!( + app.world().resource::().peek(Kind::Normal), + Some(FROSTBOLT) + ); + + // The server answers the Fireball; now the drain pops. + app.world_mut() + .resource_mut::() + .clear_if(FIREBALL); + app.world_mut() + .run_system_once(drain_spell_casts) + .expect("the drain runs as a one-shot system"); + assert!( + matches!( + rx.try_recv(), + Ok(crate::net::ClientCommand::CastSpell { spell_id, .. }) if spell_id == FROSTBOLT + ), + "the held press reached the wire by itself — no second keypress needed" + ); + assert_eq!( + app.world().resource::().peek(Kind::Normal), + None, + "and the slot is empty" + ); + + // Once. The pop re-armed the guard for the Frostbolt, and a second drain must not find + // anything to send — a pop that could be re-queued is a loop that casts forever. + app.world_mut() + .run_system_once(drain_spell_casts) + .expect("the drain runs as a one-shot system"); + assert!(rx.try_recv().is_err(), "exactly one packet for one press"); + } + #[test] fn leading_number_scans_the_first_digit_run() { assert_eq!(leading_number("Rank 1"), 1); diff --git a/crates/benilla-ui/src/script/compat/nampower.rs b/crates/benilla-ui/src/script/compat/nampower.rs index 7f85875c..f70ab731 100644 --- a/crates/benilla-ui/src/script/compat/nampower.rs +++ b/crates/benilla-ui/src/script/compat/nampower.rs @@ -1790,11 +1790,10 @@ pub(in crate::script) fn install(lua: &Lua) -> mlua::Result<()> { // `CastSpellNoQueue(spellId, spellBook [, unit])` (v3.1+) and `CastSpellByNameNoQueue(name // [, unit])` (v2.0+): cast without entering nampower's spell queue. // - // **benilla has no spell queue to bypass**, so these are the plain cast — and that is the - // honest mapping rather than a stub, because what the caller asked for ("cast this now, do not - // queue it") is exactly what happens here. `spell/inflight.rs` carries namreeb's always-on - // cast-ready rule, not pepo's queue windows; `docs/ADDON_COMPAT.md` records why the queue is - // unbuilt. + // There is a queue to bypass now (`benilla-app`'s `spell/np_press.rs`), and these take the + // ladder entry that skips it: a press from here is sent or refused on the spot, never held. + // That is the whole reason upstream publishes the pair — a macro that has already decided + // *when* to cast does not want the client deciding to cast later instead. // // Unlike stock `CastSpell`, the first argument is a **spell id**, not a book slot — upstream's // whole reason for the function. The id goes on the same queue `CastSpell` fills, so a cast @@ -1821,9 +1820,11 @@ pub(in crate::script) fn install(lua: &Lua) -> mlua::Result<()> { let spell_id = spell_ident(&model, Some(&first)); if let Some(spell_id) = spell_id.filter(|&id| id != 0) { if mouseover { + // The mouseover path commits straight at the hovered unit without + // running the ladder at all, so there is no queue there to bypass. model.mouseover_spell_casts.push(spell_id); } else { - model.spell_casts.push(spell_id); + model.no_queue_spell_casts.push(spell_id); } } Ok(()) @@ -2408,10 +2409,12 @@ mod tests { } /// The `*NoQueue` pair takes a **spell id** where stock `CastSpell` takes a book slot — the - /// whole reason upstream has them — and reaches the same queue a bar press does, since there - /// is no spell queue here to bypass. `"mouseover"` routes to the mouseover queue. + /// whole reason upstream has them — and reaches the **no-queue** drain, so the app sends it + /// through the ladder entry that skips nampower's press queue (`benilla-app`'s + /// `spell/np_press.rs`). `"mouseover"` routes to the mouseover queue, which commits straight + /// at the hovered unit and so has no queue to bypass in the first place. #[test] - fn the_no_queue_casts_push_a_spell_id_onto_the_ordinary_queue() { + fn the_no_queue_casts_push_a_spell_id_onto_the_no_queue_drain() { let mut s = UiScript::new().unwrap(); s.set_spellbook(crate::script::SpellBookState { tabs: Vec::new(), @@ -2424,30 +2427,35 @@ mod tests { }); s.run("CastSpellNoQueue(133, 1)").unwrap(); - assert_eq!(s.take_spell_casts(), vec![133]); + assert_eq!(s.take_no_queue_spell_casts(), vec![133]); + assert!( + s.take_spell_casts().is_empty(), + "and NOT the ordinary queue: a press the app would be free to hold is exactly what \ + this verb exists to avoid" + ); // An id the book does not carry: `spell_ident` answers for ids directly, as upstream's // "cast by spell ID" says, so this is a send and not a refusal. s.run("CastSpellNoQueue(4242, 1)").unwrap(); - assert_eq!(s.take_spell_casts(), vec![4242]); + assert_eq!(s.take_no_queue_spell_casts(), vec![4242]); s.run(r#"CastSpellByNameNoQueue("Fireball")"#).unwrap(); - assert_eq!(s.take_spell_casts(), vec![133]); + assert_eq!(s.take_no_queue_spell_casts(), vec![133]); // The unit is third for the id form and second for the name form. s.run(r#"CastSpellNoQueue(133, 1, "mouseover")"#).unwrap(); - assert!(s.take_spell_casts().is_empty()); + assert!(s.take_no_queue_spell_casts().is_empty()); assert_eq!(s.take_mouseover_spell_casts(), vec![133]); s.run(r#"CastSpellByNameNoQueue("Fireball", "mouseover")"#) .unwrap(); - assert!(s.take_spell_casts().is_empty()); + assert!(s.take_no_queue_spell_casts().is_empty()); assert_eq!(s.take_mouseover_spell_casts(), vec![133]); // A name nothing answers for, and no argument at all, are silent. s.run(r#"CastSpellByNameNoQueue("Nothing At All")"#) .unwrap(); s.run("CastSpellNoQueue()").unwrap(); - assert!(s.take_spell_casts().is_empty()); + assert!(s.take_no_queue_spell_casts().is_empty()); } /// The bag group — `FindPlayerItemSlot`, `GetBagItem`, `GetItemIdCooldown` — shares one diff --git a/crates/benilla-ui/src/script/model.rs b/crates/benilla-ui/src/script/model.rs index 5c230ae4..43b06c2b 100644 --- a/crates/benilla-ui/src/script/model.rs +++ b/crates/benilla-ui/src/script/model.rs @@ -945,6 +945,12 @@ pub(crate) struct Model { /// the current selection (`CastSpellByName`'s doc, `script/spellbook.rs`); a separate queue /// so the stock `spell_casts` path (current-selection target) is untouched. pub(crate) mouseover_spell_casts: Vec, + /// nampower's `CastSpellNoQueue` / `CastSpellByNameNoQueue` ids: the same send as + /// [`Self::spell_casts`] with the press queue bypassed, which is upstream's whole reason for + /// the pair. A separate queue rather than a flag per id, because the app's drain reaches the + /// two through different ladder entries (`send` and `send_now`) and a list it has to re-sort + /// is a list it can get wrong. + pub(crate) no_queue_spell_casts: Vec, /// `CastSpell(id, "pet")` ids, sent as `CMSG_PET_ACTION` with a type-1 word (`0x4b34ce`). pub(crate) pet_spell_casts: Vec, /// `ToggleSpellAutocast` ids for `CMSG_PET_SPELL_AUTOCAST` (`0x2F3`), which names a spell. @@ -1988,6 +1994,7 @@ impl Model { macro_icon_buckets: (Vec::new(), Vec::new()), macro_bound_spells: std::collections::HashMap::new(), spell_casts: Vec::new(), + no_queue_spell_casts: Vec::new(), mouseover_spell_casts: Vec::new(), pet_spell_casts: Vec::new(), pet_spell_autocasts: Vec::new(), diff --git a/crates/benilla-ui/src/script/spellbook.rs b/crates/benilla-ui/src/script/spellbook.rs index 1e318275..cf48577a 100644 --- a/crates/benilla-ui/src/script/spellbook.rs +++ b/crates/benilla-ui/src/script/spellbook.rs @@ -232,6 +232,12 @@ impl super::UiScript { std::mem::take(&mut self.model_mut().spell_casts) } + /// Drain nampower's `CastSpellNoQueue` / `CastSpellByNameNoQueue` ids: the ordinary send with + /// the press queue bypassed. + pub fn take_no_queue_spell_casts(&mut self) -> Vec { + std::mem::take(&mut self.model_mut().no_queue_spell_casts) + } + /// Drain SuperWoW's `CastSpellByName(name, "CLICK")` ids: cast at `"mouseover"`, not the /// current selection. pub fn take_mouseover_spell_casts(&mut self) -> Vec { diff --git a/docs/ADDON_COMPAT.md b/docs/ADDON_COMPAT.md index ceb49695..e3d6ba44 100644 --- a/docs/ADDON_COMPAT.md +++ b/docs/ADDON_COMPAT.md @@ -896,19 +896,16 @@ Three decisions at that mapping: **Built**, on grom's "62/62": `QueueSpellByName`, `QueueScript`, `ChannelStopCastingNextTick` and `DisenchantAll`. -**Upstream's queue has two halves, and one of them is deliberately not built.** The Lua surface — -"remember this spell and send it when the client will accept it" — is `spell/queue.rs`. The other -half intercepts **every ordinary press**, so a keypress during a cast is held rather than refused; -it is governed by `NP_QueueCastTimeSpells`, `NP_QueueInstantSpells`, `NP_QueueTargetingSpells`, -the buffer rows and the quickcast pair. That half is not a binding: it changes how every key in the -game feels, it is the one piece whose bugs are double-casts and dropped casts, and it is not -something to switch on as a side effect of porting three function names. **Those twenty CVars stay -stored-only**, each row saying so. If grom wants press-interception, it is its own piece of work -with its own live test — and the machinery it would sit on is now all here. +> **Superseded by "The press-interception half, built" below.** The paragraph that stood here said +> the press half was deliberately unbuilt and its twenty CVars stored-only. It is built now, on +> grom's say-so and with a live test to follow; the slots are still the ones described in this +> section, because upstream has one queue. -Two of the twenty-two went **live**, because they are the explicit queue's own knobs: -`NP_SpellQueueWindowMs` (how long past the in-flight cast's end an entry is held before it gives -up) and `NP_QueueSpellsOnCooldown` (whether an entry may wait out the spell's own cooldown). +**Upstream's queue has two halves, and both are built now.** The Lua surface — "remember this spell +and send it when the client will accept it" — is `spell/queue.rs`, below. The half that intercepts +**every ordinary press** is `spell/np_press.rs`, and the section after this one covers it. They +share the three slots and the one `SPELL_QUEUE_EVENT`: upstream has one queue, and a second set of +slots beside it would be a different feature wearing the same name. **Three slots, because upstream has three and its own event says so.** `SPELL_QUEUE_EVENT`'s first argument is `0`/`1` on-swing queued/popped, `2`/`3` normal, `4`/`5` non-GCD — so the queue is keyed @@ -966,9 +963,88 @@ comm -23 /tmp/np /tmp/ours | **Built** | **62** | `CancelPlayerAuraSlot`, `CancelPlayerAuraSpellId`, `CastSpellByNameNoQueue`, `CastSpellNoQueue`, `ChannelStopCastingNextTick`, `CombatLogFlush`, `CustomFileExists`, `DisenchantAll`, `EncryptPassword`, `EncryptedServerLogin`, `ExecuteCustomLuaFile`, `ExportFile`, `FindPlayerItemSlot`, `GetBagItem`, `GetBagItems`, `GetCastInfo`, `GetCurrentCastingInfo`, `GetEquippedItem`, `GetEquippedItems`, `GetItemIconTexture`, `GetItemIdCooldown`, `GetItemLevel`, `GetItemStats`, `GetItemStatsField`, `GetNampowerVersion`, `GetPlayerAuraDuration`, `GetQuestDialogQuestId`, `GetQuestLogQuestIds`, `GetRaidTargets`, `GetSpellDuration`, `GetSpellIconTexture`, `GetSpellIdCooldown`, `GetSpellIdForName`, `GetSpellModifiers`, `GetSpellNameAndRankForId`, `GetSpellPower`, `GetSpellRangeData`, `GetSpellRec`, `GetSpellRecField`, `GetSpellSlotTypeIdForName`, `GetTrinketCooldown`, `GetTrinkets`, `GetUnitData`, `GetUnitField`, `GetUnitGUID`, `ImportFile`, `IsAuraHidden`, `IsSpellInRange`, `IsSpellUsable`, `LearnTalentRank`, `PlayerIsMoving`, `PlayerIsRooted`, `PlayerIsSwimming`, `QueueScript`, `QueueSpellByName`, `ReadCustomFile`, `SetLocalRaidTargetIndex`, `SetMouseoverUnit`, `StartItemExport`, `UseItemIdOrName`, `UseTrinket`, `WriteCustomFile` | -**62 of 62.** The `comm -23` above is the check, and it now prints nothing. What remains unbuilt is -not a *name* — it is upstream's press-interception half of the queue, which has no Lua surface of -its own and whose twenty CVars say at each row that they are stored-only and why. +**62 of 62.** The `comm -23` above is the check, and it prints nothing. + +`CastSpellNoQueue` and `CastSpellByNameNoQueue` became meaningful with the press half: they take +the ladder entry that skips the queue (`CastLadder::send_now`), so a macro that has already decided +*when* to cast is not held by the client deciding to cast later instead. Before the press half they +were the plain cast, which was the honest mapping at the time and is no longer the whole story. + +### The press-interception half, built + +`spell/np_press.rs`. A press that the client would refuse for a **timing** reason is held in the +same slot an explicit `QueueSpellByName` would use, and the drain sends it the moment the guard +clears. No packet, no global cooldown, no error line while it waits. + +**Where the rung sits, and the one deviation that comes of it.** Five rungs stay above it — the +profession opener, the auto-repeat re-press, the dead caster, the targeting cursor's clear and the +on-swing re-press — because each is what the press *means* whatever is in flight, and holding them +would change the meaning. Everything below it is a real refusal the player should read: no reagent, +no weapon, out of range, no power, stunned, moving, wrong form. Upstream hooks the client's cast +entry and so holds presses that would fail for those reasons too, which means its error arrives +when the entry pops, a second later, about something the player has stopped doing. Refusing them +immediately is the deviation, and it is the better half of the trade. + +**What a window measures.** Upstream publishes four and no sentence saying. Two readings fit the +names: how long an entry survives, or how close to the guard's end a press must land. This takes +the first, for three reasons — `spell/queue.rs` already reads `NP_SpellQueueWindowMs` that way, and +one number meaning two things in one client is a trap nobody finds twice; upstream's own `Replacing +queued nonGcd spell X with Y` means the last press wins, so holding every press costs nothing in +staleness; and the admission reading would make the 250 ms cooldown window refuse almost every +press made during a global cooldown, which is the exact complaint nampower exists to answer. + +Two rows do read a window as a threshold on remaining time, and both say so themselves: +`NP_InterruptChannelsOutsideQueueWindow`, whose name states one, and `NP_CooldownQueueWindowMs`, +which has to be one. A cast or a global cooldown ends in a second or two; a spell's own cooldown can +be half an hour. Holding that press means either firing it half an hour later or swallowing it with +no error line, and a Hearthstone pressed on cooldown has to still say "Item is not ready yet" — so +a quarter of a second, which is what the row defaults to, is an allowance for pressing a touch early +rather than a queue. + +**The guard reopens early, and that is the headline.** `PendingCast` now keeps two timestamps where +it kept one. The **guard** reopens at the cast's own already-known length plus +`NP_MinBufferTimeMs`; the **record** still lives for the cast plus two seconds, so a `SMSG_SPELL_GO` +or `SMSG_CAST_RESULT` arriving a round trip later is still recognised as the answer to that cast. +Splitting them is not tidiness: the `modalNextSpell` chain that starts a hunter's Auto Shot reads the +record, and a record that died with the guard would break shooting. + +The buffer is **not** a latency figure, which is why upstream's default is 55 ms and not a ping. Our +packet reaches the server about half a trip after the send; the server starts its cast then and +finishes half a trip after our own cast time elapses. A next cast sent at exactly `cast_time` +therefore arrives just as the server frees up, and the latency cancels out of the arithmetic +entirely. The buffer is jitter margin on top. `NP_NonGcdBufferTimeMs` is wider (100 ms) because a +non-GCD spell has nothing *but* the guard between two presses. + +**`NP_RetryServerRejectedSpells` is the mitigation for that early guard**, and that is why upstream +defaults it on. If the margin is ever wrong — jitter, or a server cast time that differs from ours — +the server refuses the next cast and without a retry the press is simply gone. It goes back in its +slot **once**, held a buffer's worth, on exactly two reason codes: `0x3c` not-ready and `0x61` +another-action, the two an early guard can cause. Every other refusal is the server telling the +player something true, and re-sending one would be a packet a second that never succeeds. Upstream's +own set is not published and its log line (`" spell queued + ignored code "`) hints at a wider one; +a narrower set is the safe direction to be wrong in. + +| Row | State | Note | +|---|---|---| +| `NP_QueueCastTimeSpells` | **LIVE** | Holds a press of a spell with a cast bar. Also the addon's presence probe | +| `NP_QueueInstantSpells` | **LIVE** | The same for an instant | +| `NP_QueueChannelingSpells` | **LIVE** | Whether a press made while a channel runs is held | +| `NP_QueueTargetingSpells` | **LIVE** | The terrain-cursor class, named by `cast_target::wants_terrain_click` — implicit target 16, or a location word with no unit bit. Deliberately *not* the item/locked/GameObject cursors, which want a click on a thing rather than a patch of ground | +| `NP_QueueOnSwingSpells` | **LIVE** | Default 0, as upstream's is "changed with 1.17.2 due to changes to on swing spells" | +| `NP_SpellQueueWindowMs` / `ChannelQueueWindowMs` / `CooldownQueueWindowMs` / `TargetingQueueWindowMs` | **LIVE** | See "What a window measures" above. The general one covers both a cast in flight and a global cooldown, which is what its unqualified name can mean | +| `NP_MinBufferTimeMs`, `NP_NonGcdBufferTimeMs` | **LIVE** | The guard's reopening margin | +| `NP_InterruptChannelsOutsideQueueWindow` | **LIVE** | A press while a channel has more than `NP_ChannelQueueWindowMs` left ends the channel instead of being held. Off by default | +| `NP_ReplaceMatchingNonGcdCategory` | **LIVE** | On, a non-GCD press takes a held slot only when the two share a `Spell.dbc` category — and a category of 0 means "no shared cooldown", so two of those do not match. Off by default, which is the plain always-replace | +| `NP_RetryServerRejectedSpells` | **LIVE** | Above | +| `NP_MaxBufferIncreaseMs`, `NP_OptimizeBufferUsingPacketTimings` | stored | Upstream's *dynamic* tuning of the buffer from observed packet timings. **Off in its own defaults**, so nothing changes by not having it; building it means a model of this realm's jitter, which is measurement rather than a binding | +| `NP_OnSwingBufferCooldownMs` | stored | A rate limit on re-queueing an on-swing strike. The slot is live; what is missing is anything to rate-limit, since a re-press of the queued strike already takes the reference's own silent bail (`6e4d43`) and so cannot queue twice | +| `NP_ChannelLatencyReductionPercentage` | stored | Trims a percentage of latency off a channel's end. The channel's true end is now kept apart from its slack (`ActiveChannel::ends_at`), so the input exists; what it needs is the measured latency in the cast path, which lives behind `PingShared` and is read only by the net-stats feed today | + +**One behaviour worth knowing before testing.** Pressing a spell during a long cast holds it for the +whole cast and fires it at the end — the last press wins, so pressing three different spells during +a Pyroblast fires the third. That is upstream's `Replacing queued nonGcd spell X with Y` applied to +presses, and it is what "queue the next spell" means in practice. What it is *not* is a queue of +several casts in a row. ### The event table, accounted for @@ -1049,7 +1125,8 @@ reason is the same mistake the function list started from: the first pass took t | `NP_ChatBubblesWhisper` / `Raid` / `Battleground` | **LIVE** | Three kinds the reference bubbles never. `Raid` is the raid-**leader** and **warning** kinds, which is upstream's own wording — reading it as "raid chat" would bubble every `/raid` line | | `NP_EnableLocalSetRaidTarget` | **LIVE** | Solo, the wire leg marks nothing at all: the server answers by echoing to a group that is not there. This switch routes it to the local board instead — the path SuperWoW's `"local"` argument already built | | `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_SpamProtectionEnabled` | **LIVE** | A repeat press of the cast already in flight is swallowed rather than held — which is also the reference's own silent bail (`6e4d43`), so the two agree. Off, such a press may replace itself in its slot | +| `NP_QuickcastOnDoubleCast`, `NP_QuickcastTargetingSpells`, `NP_DoubleCastToEndChannelEarly` | stored | The double-press family, **all three off in upstream's own defaults**. Each needs the world point under the mouse *at press time*, which this client resolves in the click path (`spell/targeting/world.rs`) and not in the cast path, so wiring them means handing the ladder a camera ray — a real change whose feel wants its own test | | `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` | **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 |