grom/benilla-octo
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

pet cooldown list: Cooldowns::add drops the records that have run out before it appends, so neither list outgrows its live records, and the drop fires no flush (#685)

Browse Source 
`Cooldowns::add` appended a record for every tracked insert and never dropped one. Only `prune` removed fully elapsed records, and it ran on the player's list alone. So the pet's list kept a record for every pet GCD and every pet `SMSG_SPELL_GO` recovery for the whole session, and each pet-bar and pet-book read walked all of them.

`add` now prunes fully elapsed, not-on-hold records before it pushes. The list is bounded by its live records plus the one being added. This holds for the pet's list and the player's alike, without a new system or any schedule change.

The prune keeps its contract:
- it moves `expiry_epoch`, never `generation`, so the pet list's flush fires once for the insert and nothing for the prune;
- reads skip elapsed records already, so no reader sees a difference;
- the player's per-frame prune in `ui_action/state.rs` stays, for an expiry that arrives with no insert.

Tests:
- `an_insert_bounds_the_list_by_its_live_records_plus_itself` checks the bound after each of 30 inserts on a synthetic clock.
- `an_insert_keeps_a_running_and_a_parked_record_and_changes_no_read`.
- In the pet rig, after 40 elapsed pet presses, one more press leaves only the live records. That insert fires its one flush, and the bar's reads of the records that stay are unchanged.

Fixes #656

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Sam Sung
2026-09-30 06:59:03 +07:00
committed by GitHub
co-authored by Claude Opus 5.5
parent 3a613971bb
commit 47794e489c
2 changed files with 312 additions and 2 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/benilla-app/src/spell/cooldowns.rs
+144 -1
View File
@@ -113,6 +113,12 @@ impl Cooldowns {
self.generation = self.generation.wrapping_add(1);
}
/// How many records the list holds, elapsed ones included.
#[cfg(test)]
pub(crate) fn record_count(&self) -> usize {
self.records.len()
}
/// The counter a gated feed watches: moves on any mutation or pruned expiry.
pub(crate) fn feed_epoch(&self) -> u64 {
self.generation.wrapping_add(self.expiry_epoch)
@@ -132,8 +138,13 @@ impl Cooldowns {
/// `AddCooldown 0x6e12c0`: always appends, never matching by id, so one spell holds separate
/// nodes for its cast-send GCD and its SPELL_GO recovery. Replacing by id would let the GO
/// insert wipe the running GCD.
///
/// The list only grows here, so this is where it is bounded: the records that have run out by
/// `now` are dropped first ([`Self::prune`]), and a list holds its live records plus those
/// that ran out since its last insert. The pet's list has no other pruner.
fn add(
&mut self,
now: Instant,
spell_id: u32,
item_id: u32,
recovery: Timer,
@@ -152,6 +163,7 @@ impl Cooldowns {
{
return;
}
self.prune(now);
self.records.push(Record {
spell_id,
item_id,
@@ -167,7 +179,9 @@ impl Cooldowns {
}
/// Drop fully elapsed records that are not on hold; invisible to reads, it stands in for the
/// client's event-driven sweeps. Bumps [`Self::expiry_epoch`], not [`Self::generation`].
/// client's event-driven sweeps. Bumps [`Self::expiry_epoch`], not [`Self::generation`], so it
/// flushes no event. [`Self::add`] runs it at every insert, and the player's feed each frame,
/// which the gated feeds need to see an expiry with no insert.
pub(crate) fn prune(&mut self, now: Instant) {
let before = self.records.len();
self.records.retain(|r| {
@@ -207,6 +221,7 @@ impl Cooldowns {
|mods| mods.spell_cooldowns(spell, ranged_attack_time_ms),
);
self.add(
now,
spell_id,
0,
Timer {
@@ -245,6 +260,7 @@ impl Cooldowns {
spell.map_or(0, |s| s.category_recovery_ms)
};
self.add(
now,
use_spell.spell_id,
item_entry,
Timer {
@@ -295,6 +311,7 @@ impl Cooldowns {
);
}
self.add(
now,
spell_id,
0,
Timer::none(now),
@@ -409,6 +426,7 @@ impl Cooldowns {
}
};
self.add(
now,
spell_id,
0,
Timer {
@@ -438,6 +456,7 @@ impl Cooldowns {
now: Instant,
) {
self.add(
now,
spell_id,
item_entry,
Timer {
@@ -461,6 +480,7 @@ impl Cooldowns {
now: Instant,
) {
self.add(
now,
u32::from(cd.spell_id),
u32::from(cd.item_id),
Timer {
@@ -495,6 +515,7 @@ impl Cooldowns {
let category_ms = cd.category_cd_ms & !PET_COOLDOWN_PERMANENT;
let generation = self.generation;
self.add(
now,
cd.spell_id,
0,
Timer {
@@ -927,6 +948,128 @@ mod tests {
assert!(cds.records.is_empty());
}
/// The list is bounded where it grows: whatever the inserts' spacing, it holds the records
/// live at the insert plus the insert itself, never every record ever added.
#[test]
fn an_insert_bounds_the_list_by_its_live_records_plus_itself() {
let t0 = Instant::now();
let mut cds = Cooldowns::default();
let mods = super::SpellModifiers::default();
// Charge's 15 s category timer, then a 1.5 s GCD every 2 s for a minute.
cds.start_spell(100, &charge(), 0, t0, None);
for i in 0..30u64 {
let now = t0 + Duration::from_secs(2 * i);
cds.start_gcd(133, &fireball(), now, &mods);
let charge_live = usize::from(2 * i < 15);
assert_eq!(
cds.record_count(),
1 + charge_live,
"insert {i}: Charge if it still runs, and the new GCD"
);
}
}
/// An insert drops the records that have run out and only those: a running timer and a parked
/// record survive, no read of what stays changes, and the insert is the one mutation.
#[test]
fn an_insert_keeps_a_running_and_a_parked_record_and_changes_no_read() {
let t0 = Instant::now();
let mut cds = Cooldowns::default();
let mods = super::SpellModifiers::default();
let brief = || spell(7, 2_000, 0, (0, 0), 0);
// Parked: `Attributes` bit 25, its timers wait for `SMSG_COOLDOWN_EVENT`.
let parked = || spell(9, 1_000, 0, (0, 0), 1 << 25);
cds.start_gcd(133, &fireball(), t0, &mods);
cds.start_spell(100, &brief(), 0, t0, None);
cds.start_spell(200, &charge(), 0, t0, None);
cds.start_spell(300, &parked(), 0, t0, None);
assert_eq!(cds.record_count(), 4);
let now = t0 + Duration::from_secs(5);
let asks = [
(133, fireball()),
(100, brief()),
(200, charge()),
(201, spell(44, 0, 0, (0, 0), 0)),
(300, parked()),
(301, spell(9, 0, 0, (0, 0), 0)),
(999, spell(0, 0, 0, (0, 0), 0)),
];
let reads = |cds: &Cooldowns| {
asks.iter()
.map(|(id, d)| cds.info(*id, 0, Some(d), now))
.collect::<Vec<_>>()
};
let before = reads(&cds);
assert_eq!(before[2].remaining_ms, 10_000, "Charge runs");
assert!(!before[4].enabled, "and the parked spell reads disabled");
let generation = cds.generation;
// A spell of no category the others share.
cds.start_spell(500, &spell(99, 5_000, 0, (0, 0), 0), 0, now, None);
assert_eq!(
cds.record_count(),
3,
"Charge, the parked record and the new one"
);
assert!(!cds.sweep_pending(now), "nothing that ran out is left");
assert_eq!(
reads(&cds),
before,
"the records that stay read as they did"
);
assert_eq!(
cds.generation,
generation + 1,
"one mutation, the insert's own"
);
}
/// An elapsed record answers no read, so a prune cannot change one: what it keeps, a running
/// timer and a parked record whose own timers have run down, answers as before.
#[test]
fn a_prune_changes_no_read() {
let t0 = Instant::now();
let mut cds = Cooldowns::default();
let mods = super::SpellModifiers::default();
let brief = || spell(7, 2_000, 0, (0, 0), 0);
// Parked: `Attributes` bit 25, its timers wait for `SMSG_COOLDOWN_EVENT`.
let parked = || spell(9, 1_000, 0, (0, 0), 1 << 25);
cds.start_gcd(133, &fireball(), t0, &mods);
cds.start_spell(100, &brief(), 0, t0, None);
cds.start_spell(200, &charge(), 0, t0, None);
cds.start_spell(300, &parked(), 0, t0, None);
assert_eq!(cds.records.len(), 4);
let now = t0 + Duration::from_secs(5);
let asks = [
(133, fireball()),
(100, brief()),
(200, charge()),
// Another spell of Charge's category, and one of the parked spell's.
(201, spell(44, 0, 0, (0, 0), 0)),
(300, parked()),
(301, spell(9, 0, 0, (0, 0), 0)),
(999, spell(0, 0, 0, (0, 0), 0)),
];
let reads = |cds: &Cooldowns| {
asks.iter()
.map(|(id, d)| cds.info(*id, 0, Some(d), now))
.collect::<Vec<_>>()
};
let before = reads(&cds);
assert!(cds.sweep_pending(now), "the GCD and Brief have run out");
assert_eq!(before[2].remaining_ms, 10_000, "Charge runs");
assert!(!before[4].enabled, "and the parked spell reads disabled");
cds.prune(now);
assert_eq!(cds.records.len(), 2, "the GCD and Brief are gone");
assert!(!cds.sweep_pending(now));
assert_eq!(reads(&cds), before);
}
/// A prune that removes nothing must not move the epoch, or the per-frame prune would hold
/// every gated feed open.
#[test]
crates/benilla-app/src/ui_pet/flush_tests.rs
+168 -1
View File
@@ -3,7 +3,7 @@
//! `0x4b31b0` fires `ACTIONBAR_UPDATE_COOLDOWN` then `SPELL_UPDATE_COOLDOWN` and `0x4bce90` fires
//! `PET_BAR_UPDATE_COOLDOWN`; none of the callers fires `PET_BAR_UPDATE`.
use std::time::Duration;
use std::time::{Duration, Instant};
use benilla_formats::SpellDisplay;
use benilla_protocol::messages::{PetSpellCooldown, PetSpells};
@@ -76,6 +76,24 @@ impl Rig {
fn generation(&self) -> u64 {
self.app.world().resource::<PetBar>().cooldowns.generation
}
/// How many records the pet's list holds, elapsed ones included.
fn record_count(&self) -> usize {
self.app
.world()
.resource::<PetBar>()
.cooldowns
.record_count()
}
/// Whether the pet's list still holds a record that has run out.
fn elapsed_pending(&self) -> bool {
self.app
.world()
.resource::<PetBar>()
.cooldowns
.sweep_pending(Instant::now())
}
}
/// `0x6e85f7` inserts into the pet's list, `0x6e85fc` and `0x6e8601` flush. The bar takes no
@@ -406,3 +424,152 @@ fn a_pet_gcd_sweeps_the_spellbooks_pet_tab_button() {
let errors = s.errors();
assert!(errors.is_empty(), "script errors: {errors:?}");
}
/// A spell of no catalog record but the test's own, parked until its cooldown event.
const PARKED: u32 = 9_001;
/// Claw with a short GCD, so a press's record runs out inside a test.
fn brisk_claw() -> SpellDisplay {
SpellDisplay {
start_recovery_ms: 40,
..claw()
}
}
/// A parked spell: `Attributes` bit 25, its timers wait for `SMSG_COOLDOWN_EVENT`.
fn parked() -> SpellDisplay {
SpellDisplay {
name: "Parked".into(),
attributes: 1 << 25,
..Default::default()
}
}
/// Long enough for every `brisk_claw` GCD to have run out.
fn let_the_gcds_run_out() {
std::thread::sleep(Duration::from_millis(120));
}
/// Every pet GCD and every recovery the GO leg inserts is a record, and the insert is where the
/// list is bounded: after forty presses whose GCDs have all run out, the next press leaves the
/// list with its live records and its own, a running recovery and a parked record among them.
#[test]
fn an_insert_leaves_the_pets_list_its_live_records_alone() {
let mut rig = rig(
vec![(CLAW, brisk_claw()), (GROWL, growl()), (PARKED, parked())],
&[CLAW, GROWL, PARKED],
Some(summoned_pet()),
);
rig.receive(vec![
go(PET, GROWL),
// Ten milliseconds, parked until `SMSG_COOLDOWN_EVENT`.
SessionEvent::SpellCooldowns {
caster: PET,
cooldowns: vec![(PARKED, 10)],
},
]);
assert_eq!(
rig.record_count(),
2,
"Growl's recovery and the parked record"
);
for _ in 0..40 {
rig.press(1);
}
let_the_gcds_run_out();
assert!(rig.elapsed_pending(), "the forty GCDs have run out");
rig.press(1);
assert!(!rig.elapsed_pending(), "the insert dropped them");
assert_eq!(
rig.record_count(),
3,
"Growl's recovery, the parked record and the new GCD"
);
rig.frame();
assert_eq!(rig.cooldown(2).1, 5.0, "Growl still sweeps its 5 s");
let parked = rig.cooldown(3);
assert!(
parked.1 > 0.0 && parked.2 == 0,
"the parked record reads disabled: {parked:?}"
);
}
/// The reference flushes no event for a cooldown's end, and the insert's prune is that end: the
/// insert is the list's one mutation, so the frame fires the flush once, for it, and nothing
/// else. An insert that arms nothing changes nothing.
#[test]
fn an_inserts_prune_fires_only_the_inserts_own_flush() {
let mut rig = rig(
vec![(CLAW, brisk_claw()), (GROWL, growl())],
&[CLAW, GROWL],
Some(summoned_pet()),
);
for _ in 0..5 {
rig.press(1);
}
assert_eq!(rig.frame(), FLUSH, "the presses' own flush");
let held = rig.generation();
let_the_gcds_run_out();
assert!(rig.elapsed_pending());
// Growl has no GCD pair: nothing is armed, so nothing is dropped or flushed.
rig.press(2);
assert!(
rig.frame().is_empty(),
"an insert that arms nothing fires nothing"
);
assert_eq!(rig.generation(), held);
rig.press(1);
assert!(!rig.elapsed_pending(), "the insert dropped the five");
assert_eq!(rig.frame(), FLUSH, "the insert's flush, once");
assert_eq!(
rig.generation(),
held + 1,
"the prune is no mutation of its own"
);
assert!(rig.frame().is_empty(), "and nothing follows");
}
/// An insert's prune is invisible to `GetPetActionCooldown`: a running recovery and a parked
/// record whose timers have run down push what they pushed before, the same start included.
#[test]
fn an_insert_changes_no_bar_read_of_the_records_that_stay() {
let mut rig = rig(
vec![(CLAW, brisk_claw()), (GROWL, growl()), (PARKED, parked())],
&[CLAW, GROWL, PARKED],
Some(summoned_pet()),
);
rig.receive(vec![
go(PET, GROWL),
SessionEvent::SpellCooldowns {
caster: PET,
cooldowns: vec![(PARKED, 10)],
},
]);
for _ in 0..5 {
rig.press(1);
}
assert_eq!(rig.frame(), FLUSH);
let (running, parked_read) = (rig.cooldown(2), rig.cooldown(3));
assert_eq!(running.1, 5.0, "Growl sweeps its 5 s");
assert!(
parked_read.1 > 0.0 && parked_read.2 == 0,
"the parked record shows disabled: {parked_read:?}"
);
let_the_gcds_run_out();
assert!(rig.elapsed_pending());
rig.press(1);
assert!(!rig.elapsed_pending(), "the insert pruned");
rig.frame();
assert_eq!(
rig.cooldown(2),
running,
"the running recovery, its start too"
);
assert_eq!(rig.cooldown(3), parked_read, "the parked record");
}