From a9bb9624e95d1bc832386ed8ca789a96a67b9488 Mon Sep 17 00:00:00 2001 From: grom Date: Sat, 24 Oct 2026 23:46:42 -0600 Subject: [PATCH] =?UTF-8?q?app+ui:=20nampower's=20press-interception=20que?= =?UTF-8?q?ue=20=E2=80=94=20the=20half=20that=20changes=20how=20keys=20fee?= =?UTF-8?q?l?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The half of upstream's queue that `queue.rs` has said for three rounds it deliberately left out. A press the client would refuse for a **timing** reason is now held in the same slot an explicit `QueueSpellByName` uses, and the drain sends it the moment the guard clears: no packet, no global cooldown, no error line while it waits. One queue, because upstream has one — a second set of slots beside the first would be a different feature wearing the same name. **The guard reopens early, and that is the headline.** `PendingCast` kept one timestamp and now keeps two. 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 resolution arriving a round trip later is still recognised as the answer to that cast. That split 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 half a trip after the send, the server finishes its cast half a trip after our own 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. The buffer is jitter margin on top. `NP_NonGcdBufferTimeMs` is wider because a non-GCD spell has nothing *but* the guard between two presses. **`NP_RetryServerRejectedSpells` is the mitigation for that**, and that is why upstream defaults it on. If the margin is ever wrong the server refuses the next cast and the press is simply gone — the dropped cast the early guard buys its speed with. It goes back once, held a buffer's worth, on exactly two codes: 0x3c not-ready and 0x61 another-action. Every other refusal is the server telling the player something true, and re-sending one would be a packet a second that never succeeds. **What a window measures, and how that was decided.** Upstream publishes four and no sentence saying. Two readings fit: how long an entry survives, or how close to the guard's end a press must land. This takes the first, because `queue.rs` already reads `NP_SpellQueueWindowMs` that way and one number meaning two things in one client is a trap nobody finds twice; because `Replacing queued nonGcd spell X with Y` means the last press wins, so holding every press costs nothing in staleness; and because the other reading would make the 250 ms cooldown window refuse almost every press made during a global cooldown, which is the complaint nampower exists to answer. Two rows do read a window as remaining time, and both say so themselves. `NP_InterruptChannelsOutsideQueueWindow`'s name states one. And `NP_CooldownQueueWindowMs` has to be one: a cast ends in a second, a Hearthstone cooldown is half an hour, and holding that press means either firing it half an hour later or swallowing it with no error line. A quarter of a second is an allowance for pressing a touch early, not a queue. **Where the rung sits.** 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. Everything below is a real refusal a player should read: no reagent, out of range, no power, stunned, moving, wrong form. Upstream hooks the cast entry and holds those too, so its error arrives a second later about something the player stopped doing. Refusing them now is the deviation and the better half of the trade. Fourteen CVar rows went live. What stays stored is named with its cost: the dynamic buffer optimizer and its ceiling (off in upstream's own defaults), `NP_OnSwingBufferCooldownMs` (nothing to rate-limit — a re-press of the queued strike already takes the reference's silent bail), the channel latency trim, and the three quickcast rows, all off by default and all needing the world point under the mouse in the cast path. `CastSpellNoQueue`/`CastSpellByNameNoQueue` became meaningful: they take `CastLadder::send_now`, which skips the rung. The queue's own pop takes it too — a pop that could be re-held is a loop that never casts anything. The test worlds now seed the **registered** defaults rather than `Default`'s all-zeroes, which is a configuration nobody runs: three ladder tests were passing only because the queue was off in them. Each now asserts the queue behaviour and keeps the reference's 0x61 in a world where a player unticked the queue. And one test drives the whole cycle through the real drain — press held, nothing on the wire, guard cleared, one packet out — because the failure this guards against is a keypress that produced no packet, no error and no cast, which is the one a player cannot describe. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SgstFWtn1RxEJ5kVrpgYwn --- crates/benilla-app/src/cvars.rs | 166 ++-- crates/benilla-app/src/spell/cast_send.rs | 346 +++++++- crates/benilla-app/src/spell/cast_target.rs | 17 + crates/benilla-app/src/spell/inflight.rs | 192 ++++- crates/benilla-app/src/spell/net.rs | 36 +- crates/benilla-app/src/spell/np_press.rs | 786 +++++++++++++++++- crates/benilla-app/src/spell/queue.rs | 141 +++- crates/benilla-app/src/spell/targeting/mod.rs | 2 + .../benilla-app/src/spell/targeting/world.rs | 2 + crates/benilla-app/src/ui_items/drain.rs | 4 + crates/benilla-app/src/ui_self_cast.rs | 4 +- crates/benilla-app/src/ui_spellbook.rs | 209 ++++- .../benilla-ui/src/script/compat/nampower.rs | 38 +- crates/benilla-ui/src/script/model.rs | 7 + crates/benilla-ui/src/script/spellbook.rs | 6 + docs/ADDON_COMPAT.md | 109 ++- 16 files changed, 1848 insertions(+), 217 deletions(-) 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 |