diff --git a/crates/benilla-app/src/spell/queue.rs b/crates/benilla-app/src/spell/queue.rs index 5003ed79..5db0337e 100644 --- a/crates/benilla-app/src/spell/queue.rs +++ b/crates/benilla-app/src/spell/queue.rs @@ -81,14 +81,32 @@ impl Kind { #[derive(Clone, Copy, Debug, PartialEq, Eq)] 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. + /// Where the window is measured from: the latest guard end this entry has been told about. + /// + /// **It ratchets forward**, in [`SpellQueue::expire`], while something is still guarding. + /// A fixed `expires` stamped at queue time looks right and has a hole in it: a cast pushed + /// back twice (`SMSG_SPELL_DELAYED`) ends after the window a press made at its start was + /// given, so the entry would time out while the thing it is waiting for is still running and + /// the press would vanish with no packet and no error. The same hole swallows a press held + /// through a cast whose global cooldown then outlasts the window. + base: Instant, + /// How long past [`Self::base`] this entry survives: one of nampower's four windows, picked + /// by what guarded the press (`np_press.rs`'s `window_for`). + window: Duration, + /// The earliest [`SpellQueue::pop`] will take it. The moment it was queued 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, } +impl Slot { + /// Whether the window has run out, `base` having already been brought up to date. + fn expired(&self, now: Instant) -> bool { + now >= self.base + self.window + } +} + /// One queued script, with the priority that orders it. #[derive(Clone, Debug, PartialEq, Eq)] struct Script { @@ -172,10 +190,10 @@ impl SpellQueue { 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(window_ms)), + base: in_flight_until.unwrap_or(now).max(now), + window: Duration::from_millis(u64::from(window_ms)), not_before: now, }; *self.slot_mut(kind) = Some(slot); @@ -208,7 +226,8 @@ impl SpellQueue { 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)), + base: not_before, + window: Duration::from_millis(u64::from(window_ms)), not_before, }); self.events.push((kind.queued_code(), spell_id)); @@ -273,9 +292,18 @@ impl SpellQueue { /// **No popped event for an expiry**, and that is the point of the distinction: a consumer /// reads `*_POPPED` as "this spell was sent". An entry that timed out was not. Upstream has /// no code for it either. - pub(crate) fn expire(&mut self, now: Instant) { + pub(crate) fn expire(&mut self, now: Instant, guard_until: Option) { for kind in [Kind::OnSwing, Kind::NonGcd, Kind::Normal] { - if self.slot_mut(kind).is_some_and(|s| now >= s.expires) { + let Some(slot) = self.slot_mut(kind) else { + continue; + }; + // Ratchet the base forward while something still guards the press: the window is + // "this long past the guard's end", and the guard's end can move — pushback stretches + // a cast, and a global cooldown outlives the cast that armed it. See [`Slot::base`]. + if let Some(guard) = guard_until { + slot.base = slot.base.max(guard); + } + if slot.expired(now) { let dropped = self.slot_mut(kind).take(); if let Some(s) = dropped { debug!( @@ -285,6 +313,8 @@ impl SpellQueue { } } } + // The scripts keep a fixed deadline: their contract is "after queued spells", and a + // script whose spells never fired has nothing left to run after. self.scripts.retain(|s| now < s.expires); } @@ -440,9 +470,9 @@ mod tests { ); // An expiry raises nothing: `*_POPPED` means "this was sent", and this was not. - q.expire(t0 + Duration::from_millis(499)); + q.expire(t0 + Duration::from_millis(499), None); assert_eq!(q.peek(Kind::Normal), Some(133), "inside the window"); - q.expire(t0 + Duration::from_millis(500)); + q.expire(t0 + Duration::from_millis(500), None); assert!(q.is_empty(), "every slot gave up at the window"); assert!( q.take_events().is_empty(), @@ -462,24 +492,68 @@ mod tests { let cast_ends = t0 + Duration::from_secs(3); q.queue(133, Kind::Normal, t0, Some(cast_ends)); - q.expire(t0 + Duration::from_millis(600)); + q.expire(t0 + Duration::from_millis(600), None); assert_eq!( q.peek(Kind::Normal), Some(133), "600 ms in, the cast is still running — the window has not started" ); - q.expire(cast_ends + Duration::from_millis(499)); + q.expire(cast_ends + Duration::from_millis(499), None); assert_eq!(q.peek(Kind::Normal), Some(133)); - q.expire(cast_ends + Duration::from_millis(500)); + q.expire(cast_ends + Duration::from_millis(500), None); assert_eq!(q.peek(Kind::Normal), None); // A deadline already in the past is not a shorter window than a fresh queue's: the `max` // is what stops a lapsed cast making the entry expire instantly. q.queue(133, Kind::Normal, t0, Some(t0 - Duration::from_secs(10))); - q.expire(t0 + Duration::from_millis(499)); + q.expire(t0 + Duration::from_millis(499), None); assert_eq!(q.peek(Kind::Normal), Some(133)); } + /// The window is measured from the **live** guard end, not the one stamped at queue time. + /// + /// A fixed deadline has a hole in it: a cast pushed back twice ends after the window a press + /// made at its start was given, so the entry would time out while the thing it is waiting for + /// is still running, and the press would vanish with no packet and no error line. The same + /// hole swallows a press held through a cast whose global cooldown then outlasts the window. + #[test] + fn a_held_entry_cannot_time_out_while_something_still_guards_it() { + let t0 = Instant::now(); + let mut q = SpellQueue { + window_ms: 500, + ..Default::default() + }; + let cast_ends = t0 + Duration::from_secs(3); + q.queue(133, Kind::Normal, t0, Some(cast_ends)); + + // Two pushbacks of 500 ms each: the cast now ends at t0 + 4 s, well past the entry's + // original t0 + 3.5 s deadline. + let pushed = cast_ends + Duration::from_secs(1); + q.expire(t0 + Duration::from_millis(3_600), Some(pushed)); + assert_eq!( + q.peek(Kind::Normal), + Some(133), + "past where the window would have ended, but the cast is still running" + ); + + // And once it really ends, the window runs from there. + q.expire(pushed + Duration::from_millis(499), None); + assert_eq!(q.peek(Kind::Normal), Some(133)); + q.expire(pushed + Duration::from_millis(500), None); + assert_eq!(q.peek(Kind::Normal), None, "500 ms past the real end"); + + // The base only ever moves forward: a guard that reads *earlier* than the one already + // recorded cannot shorten an entry's life. + q.queue(133, Kind::Normal, t0, Some(cast_ends)); + q.expire(t0, Some(t0)); + q.expire(cast_ends + Duration::from_millis(499), None); + assert_eq!( + q.peek(Kind::Normal), + Some(133), + "a shorter guard end does not pull the deadline in" + ); + } + /// `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] @@ -545,7 +619,7 @@ mod tests { // And they expire on the same window as a spell does. q.queue_script("gone".into(), 1, t0); - q.expire(t0 + Duration::from_millis(500)); + q.expire(t0 + Duration::from_millis(500), None); assert!(q.take_ready_scripts().is_empty()); } } diff --git a/crates/benilla-app/src/ui_spellbook.rs b/crates/benilla-app/src/ui_spellbook.rs index 8521be07..95bb9a0f 100644 --- a/crates/benilla-app/src/ui_spellbook.rs +++ b/crates/benilla-app/src/ui_spellbook.rs @@ -593,8 +593,6 @@ fn drain_spell_queue( } } - 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. // @@ -603,20 +601,32 @@ fn drain_spell_queue( // 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. + // // The channel's trimmed end, not `current(now)`: that one carries two seconds of slack for a // lost `MSG_CHANNEL_UPDATE(0)`, so popping on it would fire a held press a round trip — and // then some — after the channel really finished. `NP_ChannelLatencyReductionPercentage` is // read here for the same reason the admission reads it, and from the same place. - let channel_guards = ladder - .np - .channel_guard_until( - &ladder.channel, - crate::net::measured_latency_ms(&ladder.ping), - now, - ) - .is_some(); - let guarded = ladder.pending.in_flight(now) || channel_guards; - let gcd_running = ladder.cooldowns.gcd(now).is_some(); + let channel_until = ladder.np.channel_guard_until( + &ladder.channel, + crate::net::measured_latency_ms(&ladder.ping), + now, + ); + let gcd_until = ladder.cooldowns.gcd(now).map(|(start, dur)| start + dur); + let guarded = ladder.pending.in_flight(now) || channel_until.is_some(); + let gcd_running = gcd_until.is_some(); + + // The expiry, with the guard end the entries' windows are measured from. Every guard, not + // just the cast: a press held through a cast whose global cooldown then outlasts the window + // must not be dropped a moment before the cooldown lets it fire. + let guard_until = [ + ladder.pending.guard_ends().filter(|_| guarded), + channel_until, + gcd_until, + ] + .into_iter() + .flatten() + .max(); + ladder.queue.expire(now, guard_until); // 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(); diff --git a/docs/ADDON_COMPAT.md b/docs/ADDON_COMPAT.md index 1f2325a4..0cbd757a 100644 --- a/docs/ADDON_COMPAT.md +++ b/docs/ADDON_COMPAT.md @@ -1040,6 +1040,14 @@ a narrower set is the safe direction to be wrong in. | `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` | **LIVE** | How much of the measured round trip comes off a channel's named end before the queue stops treating it as a guard, so a press held through a channel is not sent a round trip late. The arithmetic is the cast buffer's again, and worth writing down because "send early" sounds reckless: the server starts the channel at `T` and ends it at `T + duration`, we learn of it half a trip later, so our own end is already half a trip *after* the server is free — trimming 75 % of `L` puts a packet at the server a quarter of a trip after its own end. The named end, not the slack-inflated one, for the same reason. With no pong measured yet nothing is trimmed | +**A held entry's window is measured from the guard's *live* end, not the one stamped when the +press was made.** A fixed deadline reads right and has a hole in it: a cast pushed back twice +(`SMSG_SPELL_DELAYED`) ends after the window a press made at its start was given, so the entry +would time out while the thing it is waiting for is still running — and the press would vanish with +no packet and no error line, which is the one failure mode a player cannot describe. The same hole +swallows a press held through a cast whose global cooldown then outlasts the window. So the base +ratchets forward while anything still guards, and only ever forward. + **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