From 39932b12da850c03d3bb5325c601a0c37b283cff Mon Sep 17 00:00:00 2001 From: grom Date: Sun, 25 Oct 2026 00:03:10 -0600 Subject: [PATCH] =?UTF-8?q?app:=20nampower's=20channel=20latency=20trim=20?= =?UTF-8?q?=E2=80=94=20the=20last=20default-on=20queue=20row?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `NP_ChannelLatencyReductionPercentage` ships 75, not 0, so a real install really does take three quarters of the round trip off a channel's end. Without it a press held through a channel is sent a round trip late, which is the cost the whole press queue exists to remove — leaving this stored would have left the queue's worst case exactly where it started. **Why sending early here is conservative and not reckless**, which is the part worth writing down. The server starts the channel at T and ends it at T + duration; we only learn of it half a trip later, so our own end sits at T + L/2 + duration — half a trip *after* the server is already free. Trimming 75% of L puts our end at T + duration − L/4, and a packet sent then reaches the server at T + duration + L/4: still after its own end, by a quarter of a trip. Same arithmetic as the cast buffer, and the round trip it removes was never real work. It reads the channel's **named** end, not the slack-inflated one. That slack is two seconds of belt-and-braces for a lost `MSG_CHANNEL_UPDATE(0)`, and reading it here would make every channel look longer than it is — the pop gate was doing exactly that, so the press would have fired a round trip and then some after the channel finished. With no pong measured yet nothing is trimmed, which is the safe direction to be wrong in. The lock is recovered rather than unwrapped, as every reader of it is, and that matters more here than at the latency meter: the cast path reads it now, and a poisoned lock must not take the ability to cast with it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SgstFWtn1RxEJ5kVrpgYwn --- crates/benilla-app/src/cvars.rs | 10 +-- crates/benilla-app/src/net.rs | 12 ++- crates/benilla-app/src/spell/cast_send.rs | 11 ++- crates/benilla-app/src/spell/np_press.rs | 80 +++++++++++++++++++ crates/benilla-app/src/spell/targeting/mod.rs | 1 + .../benilla-app/src/spell/targeting/world.rs | 1 + crates/benilla-app/src/ui_items/drain.rs | 2 + crates/benilla-app/src/ui_spellbook.rs | 15 +++- docs/ADDON_COMPAT.md | 2 +- 9 files changed, 125 insertions(+), 9 deletions(-) diff --git a/crates/benilla-app/src/cvars.rs b/crates/benilla-app/src/cvars.rs index 452905d9..fec83580 100644 --- a/crates/benilla-app/src/cvars.rs +++ b/crates/benilla-app/src/cvars.rs @@ -1176,7 +1176,11 @@ pub(crate) const REGISTERED: &[Registered] = &[ ours( "NP_ChannelLatencyReductionPercentage", "75", - "nampower v2 addon-compat, STORED ONLY: no channel-latency trimming here", + "nampower v2 addon-compat, 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 — see \ + crate::spell::np_press::PressRules::channel_guard_until, which sets out why sending \ + early here is conservative and not reckless", ), // The remaining behaviour toggles (README "Default is 0" / "Default is 1" per row). ours( @@ -5032,10 +5036,6 @@ mod tests { "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", - "NampowerSettings' channel-latency slider", - ), ( "NP_DoubleCastToEndChannelEarly", "NampowerSettings' behaviour toggles; the double-press family, off by default", diff --git a/crates/benilla-app/src/net.rs b/crates/benilla-app/src/net.rs index f23a75ae..42baee75 100644 --- a/crates/benilla-app/src/net.rs +++ b/crates/benilla-app/src/net.rs @@ -458,9 +458,19 @@ pub(crate) struct NetStatus { /// The ping clock and RTT history, shared with both net threads: the write thread stamps each /// `CMSG_PING`, the read thread times `SMSG_PONG` on arrival. The app only reads and clears it, /// never times a pong from the drain. -#[derive(Resource)] +#[derive(Resource, Default)] pub(crate) struct PingShared(pub(crate) std::sync::Arc>); +/// The average `SMSG_PONG` round trip in ms, `None` before the first one. +/// +/// Recovered rather than unwrapped, as every reader of this lock is: a panic on a net thread +/// holding it ends the connection, not the app. Here that matters more than at the latency meter, +/// because the cast path reads it — a poisoned lock must not take the ability to cast with it. +pub(crate) fn measured_latency_ms(ping: &PingShared) -> Option { + use benilla_assets::LockRecover; + ping.0.lock_recover().avg_latency_ms() +} + /// Server packets the codec dropped, by opcode: `unknown` has no parse arm, `unparseable` failed /// its parser. Never cleared on reconnect. #[derive(Resource, Default)] diff --git a/crates/benilla-app/src/spell/cast_send.rs b/crates/benilla-app/src/spell/cast_send.rs index fd68efa6..7e3b9a69 100644 --- a/crates/benilla-app/src/spell/cast_send.rs +++ b/crates/benilla-app/src/spell/cast_send.rs @@ -89,6 +89,8 @@ 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, + /// The measured round trip, `None` before the first pong. + pub(crate) latency_ms: Option, pub(crate) queueing: Queueing, } @@ -123,6 +125,9 @@ pub(crate) struct CastLadder<'w, 's> { 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>, + /// The measured round trip, for `NP_ChannelLatencyReductionPercentage`. Behind a mutex the + /// net read thread writes; locked once per press and once per drain, which is nothing. + pub(crate) ping: Res<'w, crate::net::PingShared>, /// 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. @@ -336,6 +341,7 @@ impl CastLadder<'_, '_> { rules: &self.np, queue: &mut self.queue, channel: &self.channel, + latency_ms: crate::net::measured_latency_ms(&self.ping), queueing, }, ); @@ -474,7 +480,9 @@ fn send_spell_cast( 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), + // The named end less `NP_ChannelLatencyReductionPercentage` of the round trip, not + // the slack-inflated one — `PressRules::channel_guard_until` has the arithmetic. + channel: np.rules.channel_guard_until(np.channel, np.latency_ms, now), gcd: legs .gcd .as_ref() @@ -1039,6 +1047,7 @@ mod tests { world.init_resource::(); world.insert_resource(np); world.insert_resource(crate::spell::queue::SpellQueue::defaults_for_tests()); + world.init_resource::(); world.init_resource::(); // A bare World has no message storage until it is asked for. world.init_resource::>(); diff --git a/crates/benilla-app/src/spell/np_press.rs b/crates/benilla-app/src/spell/np_press.rs index 7344d08a..681c0ddf 100644 --- a/crates/benilla-app/src/spell/np_press.rs +++ b/crates/benilla-app/src/spell/np_press.rs @@ -101,6 +101,9 @@ pub(crate) struct PressRules { /// `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_ChannelLatencyReductionPercentage`: what fraction of the measured round trip comes off + /// a channel's named end before the queue stops treating it as a guard. + channel_latency_reduction_pct: u32, } /// `NP_PreventMountingWhenBuffCapped`. @@ -124,6 +127,7 @@ 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"; +pub(crate) const CVAR_CHANNEL_LATENCY_REDUCTION: &str = "NP_ChannelLatencyReductionPercentage"; /// 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 @@ -183,6 +187,12 @@ pub(crate) fn on_cvar(ev: On, mut rules: ResMut, + now: Instant, + ) -> Option { + let ends_at = channel.ends_at(now)?; + let trim = u64::from(latency_ms.unwrap_or(0)) + * u64::from(self.channel_latency_reduction_pct) + / 100; + let until = ends_at.checked_sub(Duration::from_millis(trim))?; + (now < until).then_some(until) + } + /// How long a retried cast is held before it gives up: `NP_SpellQueueWindowMs`, the general /// window. /// @@ -879,6 +920,45 @@ mod tests { ); } + /// `NP_ChannelLatencyReductionPercentage`: the trim comes off the channel's **named** end, + /// and a channel whose trimmed end has passed stops guarding at all. + #[test] + fn the_channel_guard_is_its_named_end_less_a_share_of_the_round_trip() { + let t0 = Instant::now(); + let r = PressRules::defaults_for_tests(); + let mut channel = super::super::ActiveChannel::default(); + channel.start(FROSTBOLT, 2_000, t0); + + assert_eq!( + r.channel_guard_until(&channel, Some(100), t0), + Some(t0 + Duration::from_millis(1_925)), + "75 % of a 100 ms round trip is 75 ms off a 2 s channel" + ); + assert_eq!( + r.channel_guard_until(&channel, None, t0), + Some(t0 + Duration::from_secs(2)), + "no measurement yet: nothing is trimmed, which is the safe direction" + ); + assert_eq!( + r.channel_guard_until(&channel, Some(100), t0 + Duration::from_millis(1_950)), + None, + "past the trimmed end the channel no longer guards, so a held press pops" + ); + assert_eq!( + r.channel_guard_until(&super::super::ActiveChannel::default(), Some(100), t0), + None, + "and with no channel there is nothing to trim" + ); + + let mut off = PressRules::defaults_for_tests(); + off.channel_latency_reduction_pct = 0; + assert_eq!( + off.channel_guard_until(&channel, Some(100), t0), + Some(t0 + Duration::from_secs(2)), + "0 % is the channel's own end, untouched" + ); + } + /// 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() { diff --git a/crates/benilla-app/src/spell/targeting/mod.rs b/crates/benilla-app/src/spell/targeting/mod.rs index 6eeb5049..abcbd2ee 100644 --- a/crates/benilla-app/src/spell/targeting/mod.rs +++ b/crates/benilla-app/src/spell/targeting/mod.rs @@ -509,6 +509,7 @@ mod tests { 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 9f0771ca..1a073cb0 100644 --- a/crates/benilla-app/src/spell/targeting/world.rs +++ b/crates/benilla-app/src/spell/targeting/world.rs @@ -144,6 +144,7 @@ mod tests { 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 f98280d6..f4874b2d 100644 --- a/crates/benilla-app/src/ui_items/drain.rs +++ b/crates/benilla-app/src/ui_items/drain.rs @@ -764,6 +764,7 @@ mod tests { .init_resource::() .init_resource::() .init_resource::() + .init_resource::() .init_resource::() .init_resource::() .insert_resource(NetCommands(tx)); @@ -1725,6 +1726,7 @@ mod bind_confirm_tests { .init_resource::() .init_resource::() .init_resource::() + .init_resource::() .init_resource::() .init_resource::() .insert_resource(NetCommands(tx)); diff --git a/crates/benilla-app/src/ui_spellbook.rs b/crates/benilla-app/src/ui_spellbook.rs index fc932feb..8521be07 100644 --- a/crates/benilla-app/src/ui_spellbook.rs +++ b/crates/benilla-app/src/ui_spellbook.rs @@ -603,7 +603,19 @@ 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. - let guarded = ladder.pending.in_flight(now) || ladder.channel.current(now).is_some(); + // 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(); // Decided first and sent after, because a send borrows the whole ladder where a slot read // borrows one field of it. @@ -811,6 +823,7 @@ mod tests { .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::(); diff --git a/docs/ADDON_COMPAT.md b/docs/ADDON_COMPAT.md index e3d6ba44..1f2325a4 100644 --- a/docs/ADDON_COMPAT.md +++ b/docs/ADDON_COMPAT.md @@ -1038,7 +1038,7 @@ a narrower set is the safe direction to be wrong in. | `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 | +| `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 | **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