From 08d574b09c0ec7890d45b589a6e8414165c8633f Mon Sep 17 00:00:00 2001 From: grom Date: Fri, 30 Oct 2026 13:31:26 -0600 Subject: [PATCH] =?UTF-8?q?GetCastTime=20applies=20spell-mod=20op=200xa=20?= =?UTF-8?q?itself=20=E2=80=94=20so=20a=20talented=20Slam=20is=202.0=20s,?= =?UTF-8?q?=20not=202.5?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit grom's probe closed the slam hunt in one line: mx=2.7 st=1.0255 u=2.7 c=2.5 w=7.4074 Both of my remaining candidates were wrong. The swing speed arrived correctly (2.7, not the 2000 ms stand-in the last commit instrumented) and the swing timer was running, so neither the feed nor the combat events were at fault. That leaves one term: `c`, the cast time, and `w = (2.70 - 2.50) / 2.70` falls straight out of it. `SuperCleveRoidMacros` does not ask a verb for that number. `GetSlamCastTime()` walks the spellbook for Slam, builds a nil-parent `GameTooltipTemplate` scanning tooltip, calls `SetSpell`, and reads the `"%d+%.?%d* sec cast"` line — so the whole conditional rests on one line of our spellbook tooltip. `Spell_C::GetCastTime` (`0x6e3340`) applies `SPELLMOD_CASTING_TIME` **inside itself**, after the row's minimum clamp: 6e33b0: cmp %esi,%eax ; row minimum (2500 for Slam) 6e33b4: mov %esi,-0x4(%ebp) ; ...clamped to it 6e33b7: push $0xa ; op 10 = SPELLMOD_CASTING_TIME 6e33be: call 0x6e6af0 ; value = (value + flat) * pct / 100 and the tooltip's cast line is a call to it, not a DBC read (`52eb48`/`52eb4b`/ `52eb4d: call 0x6e3340`). We read `SpellCastTimes.dbc` directly, so a talent-shortened cast showed its base length — a deviation `ui_action/mod.rs` and `cast_times.rs` both *documented* without anyone pricing it. The realm's own data prices it. Slam (1464/8820/11604/11605) is `CastingTimeIndex` 20, row `{base 2500, perLevel 0, minimum 2500}`. Improved Slam is two ranks — 12862 and 12330, aura 107 / misc 10 / `basePoints` -251 and -501 with `dieSides` 1, a flat -250 or -500 ms on op 10 against `SpellFamilyFlags` bit 21, family 4 (warrior, `ChrClasses.dbc` field 15; the five-rank version is `zzOLD`). So a 2/2 warrior's Slam is 2.0 s, and on his 2.70 s main hand: 2.5 s (ours) 0.20 s window 7.4% of the swing 2.0 s (his client) 0.70 s window 25.9% of the swing It is a size, not a sign. 0.20 s is narrower than human reaction time, so a player pressing by hand overshoots it every press; 0.70 s he lands in every swing. "477 Heroic Strikes, zero Slams" and "works just fine on the normal client" are the same observation. Note the order: the minimum clamp is 2500 and it runs *before* the modifier. Applying op 0xa first would pin Slam at 2.5 s for ever and look identical to the bug it was meant to fix. `Spells::cast_time_ms` is now `GetCastTime` — row resolve, minimum clamp, op 0xa — with `cast_time_signed_ms` for the reference's `allowNegative` form the tooltip's "Instant cast" ladder needs (`0x52ebce`), and `cast_time_row_ms` kept for the one question that wants the row alone: whether a spell has a cast time *authored* at all, which is `opens_cast_bar`'s gate. The tooltip's cast line and the four live readers pass the modifier tables through — including both `PendingCast` arms, so nampower's local lockout on a talented cast was 500 ms too long as well. The tooltip feed re-pushes on a level change now that the cast line is level-scaled. Still deviating, written where it lives: the final `UNIT_MOD_CAST_SPEED` multiplier (`6e342c`, taken only when `Attributes & 0x30` is clear) is not applied, because no site streams unit field 139. It is 1.0 without a casting-speed aura, so it moves nothing a physical ability does. Tests: `a_talent_shortened_cast_line_shows_its_talented_length` grades Slam on the real DBCs untalented, at 2/2, and against a foreign class family (the gate that stops a mage's table shortening a warrior's Slam). The harness test now drives grom's own addon chain through both widths — `WINDOW_PCT` 7.4074074074074 and `WINDOW_PCT_TALENTED` 25.925925925926 — with the addon's own `RefreshSlamCastTime` between them, because its scan is cached for 2 s and a test that re-pushed without it would measure the stale value and pass. Both earlier answers are written down in `WINDOW_PCT`'s doc and in docs/ADDON_COMPAT.md: I called this "not ours" twice, from two green tests, and each was green because it asserted a number I had supplied. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SgstFWtn1RxEJ5kVrpgYwn --- .../src/addon_harness/tick_bench_tests.rs | 82 ++++++++++++++--- crates/benilla-app/src/spell/cast_send.rs | 13 ++- crates/benilla-app/src/spell/mod.rs | 2 +- crates/benilla-app/src/spell/mods.rs | 12 ++- crates/benilla-app/src/spell/net.rs | 6 +- crates/benilla-app/src/ui_action/mod.rs | 59 +++++++++++- crates/benilla-app/src/ui_tooltip/mod.rs | 22 +++-- crates/benilla-app/src/ui_tooltip/tests.rs | 77 ++++++++++++++++ docs/ADDON_COMPAT.md | 91 +++++++++++++++++++ 9 files changed, 330 insertions(+), 34 deletions(-) diff --git a/crates/benilla-app/src/addon_harness/tick_bench_tests.rs b/crates/benilla-app/src/addon_harness/tick_bench_tests.rs index a4d1286d..57de7c91 100644 --- a/crates/benilla-app/src/addon_harness/tick_bench_tests.rs +++ b/crates/benilla-app/src/addon_harness/tick_bench_tests.rs @@ -24,22 +24,41 @@ use super::{ }; /// Plates a crowded pull shows; the nameplate CVar cap and `pfValidUnits`' own range. -/// `CleveRoids.GetSlamWindowPercent()` for grom's own numbers, as Lua prints it. +/// `CleveRoids.GetSlamWindowPercent()` for grom's own numbers, as Lua prints it: the window an +/// **untalented** warrior gets, which is what this client used to hand every warrior. /// -/// `(mainhand - slamCast) / mainhand * 100` = `(2.70 - 2.50) / 2.70 * 100`. **Both numbers are -/// the realm's, not vanilla's.** Slam on OctoWoW is a 2.5 s cast — `Spell.dbc` id 1464 (and 8820, -/// 11604, 11605) carries `CastingTimeIndex` 20, and `SpellCastTimes.dbc` row 20 is 2500 ms, where -/// vanilla's Slam is 1.5 s. SuperCleveRoidMacros' own `DEFAULT_SLAM_CAST = 2.5 -- Turtle WoW` -/// agrees independently. +/// `(mainhand - slamCast) / mainhand * 100` = `(2.70 - 2.50) / 2.70 * 100`. Both numbers are the +/// realm's, not vanilla's: Slam on OctoWoW is a 2.5 s cast — `Spell.dbc` 1464 (and 8820, 11604, +/// 11605) carries `CastingTimeIndex` 20, and `SpellCastTimes.dbc` row 20 is +/// `{base 2500, perLevel 0, minimum 2500}`, where vanilla's Slam is 1.5 s. +/// SuperCleveRoidMacros' own `DEFAULT_SLAM_CAST = 2.5 -- Turtle WoW` agrees independently. /// -/// So the no-clip window on a 2.70 s weapon is **0.2 s wide, 7.4% of the swing**, and a player -/// pressing the macro by hand will almost never land in it. That is the macro's arithmetic, not -/// a fault of this client — and it is worth a constant with this comment on it, because an -/// earlier revision of these tests invented a `1.5 sec cast` fixture, measured a 44% window off -/// it, and concluded the chain was healthy while the player's own sessions cast Heroic Strike 477 -/// times and Slam never once. +/// **What took two wrong turns to find is that 2.5 is not the number a talented warrior's client +/// reports.** Improved Slam (`Spell.dbc` 12862 Rank 1, 12330 Rank 2) is aura 107 / misc 10 / +/// `basePoints` −251 and −501 with `dieSides` 1 — a flat −250 or −500 ms on spell-mod op `0xa`, +/// and `Spell_C::GetCastTime` applies op `0xa` *itself* (`6e33b7: push $0xa`), after the row's +/// minimum clamp and before returning to every caller, the spellbook tooltip's cast line included +/// (`52eb4d: call 0x6e3340`). This client read `SpellCastTimes.dbc` directly instead, so it +/// reported 2.5 s to a warrior whose real cast was 2.0 — see +/// [`crate::ui_action::Spells::cast_time_signed_ms`]. +/// +/// The consequence is the whole defect, and it is a size, not a sign: a 2.0 s cast on a 2.70 s +/// weapon leaves a **0.70 s** no-clip window ([`WINDOW_PCT_TALENTED`], 25.9% of the swing), a +/// 2.5 s cast leaves **0.20 s**. 0.20 s is narrower than human reaction time, so a player +/// pressing by hand overshoots it every single time, which is exactly the record: Heroic Strike +/// 477 times, Slam never once, on a macro that "works just fine on the normal client". +/// +/// Both wrong turns are worth keeping written down, because each produced a green test: +/// one revision invented a `1.5 sec cast` fixture and measured a 44% window off a number nothing +/// shipped; the next corrected the fixture to the realm's 2.5 and concluded from it that the +/// arithmetic was the macro's own and not this client's fault. The fixture was right that time. +/// The conclusion was not — the feed behind it was wrong. const WINDOW_PCT: &str = "7.4074074074074"; +/// [`WINDOW_PCT`] with 2/2 Improved Slam applied — `(2.70 - 2.00) / 2.70 * 100`, the window the +/// reference client gives the same character on the same weapon. +const WINDOW_PCT_TALENTED: &str = "25.925925925926"; + const PLATES: u32 = 40; /// Frames per timed window. @@ -1844,8 +1863,10 @@ fn seat_a_warrior_mid_fight(s: &mut UiScript, offhand: bool) { /// * The `` delta reaching `SP_ST_OnUpdate(arg1)` as seconds. /// /// **All five answer correctly** once the sheet is pushed — see [`WINDOW_PCT`] for the window -/// they produce and for the fixture mistake that made an earlier revision of this test read as a -/// clean bill of health while the macro was still picking Heroic Strike every time. It is the first bullet's edge: before the stats feed has pushed, +/// they produce, for the two fixture-and-conclusion mistakes that made earlier revisions of this +/// test read as a clean bill of health while the macro was still picking Heroic Strike every +/// time, and for the cast-time feed that was the real cause. It is the first bullet's edge: +/// before the stats feed has pushed, /// `UnitAttackSpeed("player")` answered `0`, and SP_SwingTimer's `ADDON_LOADED` arm latches that /// into `st_timerMax` behind `if not st_timerMax` — where `0` is truthy, so the guard never /// re-fires. The first write to that global in a session is therefore always the pre-push answer. @@ -2002,7 +2023,7 @@ fn the_slam_clip_conditional_answers_on_the_players_own_interface() { ask(&mut s, "CleveRoids.GetSlamWindowPercent()"), WINDOW_PCT, "(2.70 - 2.50) / 2.70 — the share of the swing in which Slam still beats the \ - auto-attack: 0.2 s of 2.7, which is the whole story of this macro on this weapon" + auto-attack: 0.2 s of 2.7, off the untalented cast time seeded above" ); assert_eq!( ask(&mut s, "CleveRoids.ValidateNoSlamClip()"), @@ -2011,6 +2032,37 @@ fn the_slam_clip_conditional_answers_on_the_players_own_interface() { first clause is the one that should fire" ); + // And the same chain on the cast time a 2/2 Improved Slam warrior's client actually reports, + // which is the half this client used to get wrong: the only input that moves is the `sec + // cast` line, and it has to move the window from 0.2 s of the swing to 0.7 s. + // `RefreshSlamCastTime` is the addon's own invalidator — its scan is cached for 2 s, and a + // test that re-pushed the tooltip without it would measure the stale value and pass. + s.set_spell_tooltip( + 1464, + benilla_ui::script::SpellTooltipView { + name: "Slam".into(), + rank: Some("Rank 1".into()), + cost: Some("15 Rage".into()), + range: Some("Melee Range".into()), + cast_time: Some("2 sec cast".into()), + description: "Slams the opponent.".into(), + ..Default::default() + }, + ); + let _ = s.eval::<()>("CleveRoids.RefreshSlamCastTime()"); + assert_eq!( + ask(&mut s, "CleveRoids.GetSlamCastTime()"), + "2", + "`\"2 sec cast\"` parses — the addon's own regex is `(%d+%.?%d*) sec cast`, so the \ + `%.3g` template's bare `2` has to match as readily as `2.5`" + ); + assert_eq!( + ask(&mut s, "CleveRoids.GetSlamWindowPercent()"), + WINDOW_PCT_TALENTED, + "(2.70 - 2.00) / 2.70 — 0.7 s of a 2.7 s swing. This is the number his client should \ + have been showing all along, and the only thing that changed to get it is the cast line" + ); + // The `` delta: the timer has to run down in real seconds, or the window never // closes and `[slamclip]` never takes over. for _ in 0..6 { diff --git a/crates/benilla-app/src/spell/cast_send.rs b/crates/benilla-app/src/spell/cast_send.rs index e1a20f7a..7305d426 100644 --- a/crates/benilla-app/src/spell/cast_send.rs +++ b/crates/benilla-app/src/spell/cast_send.rs @@ -220,7 +220,7 @@ impl CastLadder<'_, '_> { .and_then(|d| { self.spells .as_ref() - .map(|s| s.cast_time_ms(d, d.base_level)) + .map(|s| s.cast_time_ms(d, d.base_level, &self.spell_mods)) }) .unwrap_or(0); self.pending @@ -500,8 +500,9 @@ fn send_spell_cast( .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))); + let press_cast_time = def.map_or(0, |d| { + spells.map_or(0, |s| s.cast_time_ms(d, caster_level, spell_mods)) + }); match np.rules.admit( spell_id, def, @@ -790,7 +791,7 @@ fn send_spell_cast( .self_store .and_then(|s| s.0.unit_level()) .unwrap_or(0); - let cast_time_ms = spells.map_or(0, |s| s.cast_time_ms(d, caster_level)); + let cast_time_ms = spells.map_or(0, |s| s.cast_time_ms(d, caster_level, spell_mods)); if validator::cast_moving_refusal(ctx.self_move_flags, cast_time_ms, def) { debug!("ui_action: cast {spell_id} refused locally — moving (0x2e)"); cast_errors.push_local(spell_id, 0x2e); @@ -936,7 +937,9 @@ fn send_spell_cast( .self_store .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))); + let cast_time_ms = def.map_or(0, |d| { + spells.map_or(0, |s| s.cast_time_ms(d, caster_level, spell_mods)) + }); pending.arm( spell_id, now, diff --git a/crates/benilla-app/src/spell/mod.rs b/crates/benilla-app/src/spell/mod.rs index 828364bf..4eeb37c1 100644 --- a/crates/benilla-app/src/spell/mod.rs +++ b/crates/benilla-app/src/spell/mod.rs @@ -33,7 +33,7 @@ pub(crate) use inflight::{ inflight, ActiveChannel, AutoRepeatActive, LocalMoveStart, PendingCast, QueuedMeleeSpell, SPELL_INTERRUPT_MOVEMENT, }; -pub(crate) use mods::{SpellModifiers, OP_COST}; +pub(crate) use mods::{SpellModifiers, OP_CASTING_TIME, OP_COST}; // `TargetingWants` is exported for the ground reticle, which draws for the location word alone. pub(crate) use targeting::{ ground_cast_radius, CorpsePick, PicksSelf, SpellTargeting, TargetingWants, diff --git a/crates/benilla-app/src/spell/mods.rs b/crates/benilla-app/src/spell/mods.rs index 69b8c1dc..fdbb2629 100644 --- a/crates/benilla-app/src/spell/mods.rs +++ b/crates/benilla-app/src/spell/mods.rs @@ -15,8 +15,8 @@ //! boolean, so a bare `(flat, pct)` pair would zero every unmodified spell; hence //! [`SpellModifiers::modifiers`] returns `Option` and [`SpellMod`] is private. //! -//! All 29 ops are stored; only op 14 (`COST`) is read here. Op 24 is the one the client routes -//! aura 65 (casting speed) to, whatever vmangos names it. +//! All 29 ops are stored; two are read: op 14 ([`OP_COST`]) and op 10 ([`OP_CASTING_TIME`]). Op 24 +//! is the one the client routes aura 65 (casting speed) to, whatever vmangos names it. use bevy::prelude::*; @@ -36,6 +36,14 @@ const CELLS: usize = BITS * OPS; /// `6e32e3`). pub(crate) const OP_COST: u8 = 14; +/// `SPELLMOD_CASTING_TIME`, read by [`crate::ui_action::Spells::cast_time_ms`]. The reference +/// applies it *inside* `Spell_C::GetCastTime` (`6e33b7: push $0xa`, `6e33be: call 0x6e6af0`), +/// after the level scale and the row's minimum clamp — so every caller of `GetCastTime` sees the +/// talent-shortened cast, the spellbook tooltip's own cast line included (`52eb4d: call +/// 0x6e3340`). Treating it as a call-site concern instead is what shipped a warrior's Slam as its +/// untalented 2.5 s for a whole release; see that method's doc. +pub(crate) const OP_CASTING_TIME: u8 = 10; + /// `SPELL_ATTR_EX3_IGNORE_CASTER_MODIFIERS` (vmangos `SpellDefines.h:940`): a spell carrying it /// takes no modifier (`0x6e6b52`). const ATTR_EX3_IGNORE_CASTER_MODIFIERS: u32 = 0x2000_0000; diff --git a/crates/benilla-app/src/spell/net.rs b/crates/benilla-app/src/spell/net.rs index d968768e..523ef1c1 100644 --- a/crates/benilla-app/src/spell/net.rs +++ b/crates/benilla-app/src/spell/net.rs @@ -974,13 +974,17 @@ fn cast_result( /// Shoot are instant rows too. An unknown spell, or a build without the cast-time table, keeps the /// reference's bar rather than losing every bar. /// +/// The row-only read ([`crate::ui_action::Spells::cast_time_row_ms`]) is deliberate: spell-mod op +/// `0xa` can take a real cast to zero, and a spell a talent made instant still is not a spell that +/// was *authored* instant. The bar follows the wire's own cast time either way. +/// /// Pulled out of [`spell_start`] because ClassicAPI's `C_Spell.UnitCastingInfo` has to answer the /// *same* question (`ui_cast_events.rs`'s cast windows) — pfUI's cast bar replaces the stock one, /// so if the two gates could disagree, one of the two bars would be wrong, and the whole point of /// routing both off one wire read is that neither can be. One body, two callers. fn opens_cast_bar(spells: Option<&crate::ui_action::Spells>, spell_id: u32) -> bool { match (spells, spells.and_then(|s| s.catalog.get(spell_id))) { - (Some(s), Some(d)) if !s.cast_times.is_empty() => s.cast_time_ms(d, d.base_level) > 0, + (Some(s), Some(d)) if !s.cast_times.is_empty() => s.cast_time_row_ms(d, d.base_level) > 0, _ => true, } } diff --git a/crates/benilla-app/src/ui_action/mod.rs b/crates/benilla-app/src/ui_action/mod.rs index a744e00d..78174373 100644 --- a/crates/benilla-app/src/ui_action/mod.rs +++ b/crates/benilla-app/src/ui_action/mod.rs @@ -97,10 +97,14 @@ pub(crate) struct Spells { } impl Spells { - /// The cast time in ms (`GetCastTime 0x6e3340`), level-scaled from the `CastingTimeIndex` - /// row; a missing row reads 0. Spell-mod op `0xa` is not applied, so a talent-shortened cast - /// shows its base length. - pub(crate) fn cast_time_ms( + /// The `SpellCastTimes.dbc` row alone (`0x6e3340`'s first half): level-scaled and floored to + /// the row minimum, with no spell modifier applied. A missing row reads 0. + /// + /// Only the "does this spell have a cast at all" question wants this + /// ([`crate::spell::net`]'s `opens_cast_bar`), because a modifier can take a real cast to zero + /// and that is not the same thing as a spell that was always instant. Everything asking how + /// long a cast will actually take wants [`Self::cast_time_ms`]. + pub(crate) fn cast_time_row_ms( &self, def: &benilla_formats::SpellDisplay, caster_level: u32, @@ -109,6 +113,53 @@ impl Spells { .get(def.casting_time_index) .map_or(0, |row| row.resolved_ms(caster_level, def.base_level)) } + + /// `Spell_C::GetCastTime(spell, unit, 1)` (`0x6e3340`): the row resolve, then spell-mod op + /// `0xa` (`SPELLMOD_CASTING_TIME`) applied in place (`6e33b7`). Signed, because the modifier + /// runs *after* the row's own clamp and can carry the result below zero — the reference's + /// `allowNegative` argument, which the tooltip passes as 1 (`52eb4b: push $0x1`) so that its + /// own ladder can read a negative as "Instant cast" (`0x52ebce`). + /// + /// This is the step whose absence pinned `[noslamclip]` false for a whole release. The realm's + /// Slam is a 2.5 s cast (`CastingTimeIndex` 20 → 2500 ms) and Improved Slam is a flat −500 ms + /// on op 10 at 2/2 (`Spell.dbc` 12862/12330: aura 107, misc 10, basePoints −251/−501 with + /// `dieSides` 1), so a talented warrior's Slam is 2.0 s. On a 2.70 s weapon that is the + /// difference between a 0.70 s no-clip window and a 0.20 s one — 25.9% of the swing against + /// 7.4% — and a 0.20 s window is narrower than human reaction time, so the macro's Slam leg + /// never fired once in 477 presses while the reference client's fired every swing. + /// + /// Deviation: the final `UNIT_MOD_CAST_SPEED` multiplier (`6e342c`, taken only when + /// `Attributes & 0x30` is clear and the unit argument is non-null) is not applied — no site + /// streams unit field 139 yet. It is 1.0 for every unit without a casting-speed aura, so it + /// moves nothing a physical ability does; a Curse of Tongues target's cast lines are the case + /// that still reads short. + pub(crate) fn cast_time_signed_ms( + &self, + def: &benilla_formats::SpellDisplay, + caster_level: u32, + mods: &crate::spell::SpellModifiers, + ) -> i32 { + let row = self.cast_time_row_ms(def, caster_level); + mods.apply( + def, + crate::spell::OP_CASTING_TIME, + i32::try_from(row).unwrap_or(i32::MAX), + ) + } + + /// `Spell_C::GetCastTime(spell, unit, 0)`: [`Self::cast_time_signed_ms`] with a negative + /// clamped to zero (`6e3468`..`6e3470`, the `setle`/`dec`/`and` the reference reaches when its + /// third argument is 0). What every caller wanting a duration wants. + pub(crate) fn cast_time_ms( + &self, + def: &benilla_formats::SpellDisplay, + caster_level: u32, + mods: &crate::spell::SpellModifiers, + ) -> u32 { + self.cast_time_signed_ms(def, caster_level, mods) + .max(0) + .unsigned_abs() + } } #[cfg(test)] diff --git a/crates/benilla-app/src/ui_tooltip/mod.rs b/crates/benilla-app/src/ui_tooltip/mod.rs index 3e603585..9e4a2841 100644 --- a/crates/benilla-app/src/ui_tooltip/mod.rs +++ b/crates/benilla-app/src/ui_tooltip/mod.rs @@ -148,11 +148,12 @@ fn spell_tooltip_view( let cast_time = if d.tooltip_omits_cast_line() { None } else { - let base = spells - .cast_times - .get(d.casting_time_index) - .map(|c| c.base_ms as i32) - .unwrap_or(0); + // `GetCastTime(spell, unit, 1)` (`52eb48`-`52eb4d`), not the raw `SpellCastTimes.dbc` + // cell: level-scaled, and with spell-mod op `0xa` applied. Reading the cell directly is + // what made a talented warrior's Slam tooltip read its untalented 2.5 s, which is the one + // number `SuperCleveRoidMacros` scans this very line for. + let caster_level = vctx.store.and_then(|s| s.0.unit_level()).unwrap_or(0); + let base = spells.cast_time_signed_ms(d, caster_level, vctx.spell_mods); // `%.3g` templates: the seconds go over as a real. if base > 0 { let (key, v) = if base >= 60_000 { @@ -319,6 +320,8 @@ struct SpellFeedMemory { avoidance: Option<[u32; 4]>, /// The caster's and its auto-attack target's reach as bit patterns. combat_reach: Option<(Option, Option)>, + /// The caster's level, which the cast line scales by (`0x6e3340`); a level-up re-pushes. + caster_level: Option, /// Per reagent on show, `(owned count, name resolved)` (the inline red, `0x854120`). reagents: std::collections::BTreeMap, } @@ -479,10 +482,17 @@ fn feed_spell_tooltips( memory.combat_reach = Some(reaches); wanted.extend(memory.pushed.drain()); } - // So does a spell-modifier change: the cost cell resolves through the talent tables. + // So does a spell-modifier change: the cost and cast cells both resolve through the talent + // tables, which is how a freshly-spent Improved Slam point reaches the cast line. if spell_mods.is_changed() { wanted.extend(memory.pushed.drain()); } + // And so does a level-up: the cast line is level-scaled. + let caster_level = self_store.and_then(|s| s.0.unit_level()); + if memory.caster_level != caster_level { + memory.caster_level = caster_level; + wanted.extend(memory.pushed.drain()); + } let watched: Vec = memory.reagents.keys().copied().collect(); let reagent_state: std::collections::BTreeMap = watched .into_iter() diff --git a/crates/benilla-app/src/ui_tooltip/tests.rs b/crates/benilla-app/src/ui_tooltip/tests.rs index f82bee68..686dbfc0 100644 --- a/crates/benilla-app/src/ui_tooltip/tests.rs +++ b/crates/benilla-app/src/ui_tooltip/tests.rs @@ -628,3 +628,80 @@ fn a_late_creature_answer_still_rebuilds_the_plate() { "a name and a subtitle are lines: they must rebuild the plate" ); } + +/// Slam's cast line with Improved Slam at 2/2, which is the one number +/// `SuperCleveRoidMacros` reads to size its `[noslamclip]` window. +/// +/// The realm's own data: `Spell.dbc` 1464 is `CastingTimeIndex` 20, and `SpellCastTimes.dbc` +/// row 20 is `{base 2500, perLevel 0, minimum 2500}`; Improved Slam (12862 Rank 1, 12330 Rank 2) +/// is aura 107 / misc 10 / `basePoints` −251 and −501 with `dieSides` 1, i.e. a flat −250 or −500 +/// on spell-mod op `0xa` against `SpellFamilyFlags` bit 21 — Slam's own bit, in family 4. +/// 2500 − 500 = 2000. +/// +/// Two things this pins beyond the number. The **order**: the reference clamps to the row's +/// minimum at `6e33b0` and applies the modifier at `6e33b7`, so with a minimum of 2500 a +/// modifier applied before the clamp would leave Slam at 2.5 s for ever. And the **route**: the +/// tooltip's cast line is `GetCastTime(spell, unit, 1)` (`52eb4d`), not a `SpellCastTimes.dbc` +/// read, which is what reading the row directly got wrong. +#[test] +fn a_talent_shortened_cast_line_shows_its_talented_length() { + let data = benilla_formats::wow_data_or_skip!(); + let mut chain = benilla_formats::open_chain(&data).expect("open chain"); + let spells = Spells { + catalog: benilla_formats::load_spell_catalog(&mut chain).expect("Spell.dbc"), + forms: benilla_formats::load_shapeshift_forms(&mut chain).expect("SpellShapeshiftForm.dbc"), + ranges: benilla_formats::load_spell_ranges(&mut chain).expect("SpellRange.dbc"), + cast_times: benilla_formats::load_spell_cast_times(&mut chain).expect("SpellCastTimes.dbc"), + durations: benilla_formats::load_spell_durations(&mut chain).expect("SpellDuration.dbc"), + radii: benilla_formats::load_spell_radii(&mut chain).expect("SpellRadius.dbc"), + }; + const SLAM: u32 = 1464; + /// Slam's `SpellFamilyFlags` bit, and the warrior family it is gated by. + const SLAM_BIT: u8 = 21; + const WARRIOR_FAMILY: u32 = 4; + + let mut objs = no_objects(); + let objects = objs.get(); + + // Untalented, which is what every other cell in this file is graded as. + let mut t = TestCtx::new(); + let untalented = spell_tooltip_view(SLAM, &spells, &mut t.ctx(&objects, 0, None)) + .expect("Slam view") + .cast_time; + assert_eq!( + untalented.as_deref(), + Some("2.5 sec cast"), + "the realm's Slam is a 2.5 s cast before any talent — vanilla's is 1.5" + ); + + // 2/2 Improved Slam, as the wire delivers it: one absolute cell, flat, (bit 21, op 10). + let mut t = TestCtx::new(); + t.spell_mods.set_class_family(WARRIOR_FAMILY); + t.spell_mods + .set(true, SLAM_BIT, crate::spell::OP_CASTING_TIME, -500); + let talented = spell_tooltip_view(SLAM, &spells, &mut t.ctx(&objects, 0, None)) + .expect("Slam view") + .cast_time; + assert_eq!( + talented.as_deref(), + Some("2 sec cast"), + "op 0xa is applied inside `GetCastTime`, so the cast line is the talented length. \ + Reading `SpellCastTimes.dbc` directly left this at 2.5, which cut the macro's no-clip \ + window on a 2.70 s weapon from 0.70 s to 0.20 s — narrower than a human's reaction \ + time, so its Slam leg never once fired in 477 presses" + ); + + // The one cell this spell's own family gate must not leak into: a mage's modifier table + // cannot shorten a warrior's Slam, whatever bit it sets. + let mut t = TestCtx::new(); + t.spell_mods.set_class_family(3); + t.spell_mods + .set(true, SLAM_BIT, crate::spell::OP_CASTING_TIME, -500); + assert_eq!( + spell_tooltip_view(SLAM, &spells, &mut t.ctx(&objects, 0, None)) + .expect("Slam view") + .cast_time, + untalented, + "`GetSpellModifiers` gates on the spell's family equalling the player's own (`6e6b30`)" + ); +} diff --git a/docs/ADDON_COMPAT.md b/docs/ADDON_COMPAT.md index 0158d471..5b37764f 100644 --- a/docs/ADDON_COMPAT.md +++ b/docs/ADDON_COMPAT.md @@ -1450,3 +1450,94 @@ does nothing, and reports a clean load over an empty screen. `Step::Plates(n)` s plates through `sync_nameplates` and counts how many an addon has hung a frame on — which doubles as the contract check on the plate's own shape, since a plate that stopped being a `Button`, or stopped putting its border texture first, would read as an addon adopting nothing. + +## The slam macro: a talent the engine never applied + +`SuperCleveRoidMacros`' `[noslamclip]` picked Heroic Strike every time — 477 casts in grom's own +`WoWCombatLog.txt`, Slam not once — on a macro he reports working on the stock client. Three +answers were wrong before the right one, and all three are worth the space, because each of the +first two came with a green test. + +### How the conditional is actually built + +`Conditionals.lua` sizes the window itself: + +```lua +local maxSlamStart = attackSpeed - CleveRoids.GetSlamCastTime() +if maxSlamStart <= 0 then return 0 end +return (maxSlamStart / attackSpeed) * 100 -- GetSlamWindowPercent +... +return CleveRoids.GetSwingPercentElapsed() <= maxPercent -- ValidateNoSlamClip +``` + +`GetSlamCastTime()` is not a verb call. It walks the spellbook with +`GetSpellName(i, BOOKTYPE_SPELL)` for Slam, builds a **nil-parent `GameTooltipTemplate`** scanning +tooltip, calls `tooltip:SetSpell(slot, bookType)`, and reads `TextLeft` with `getglobal` +looking for `"%d+%.?%d* sec cast"` — with `DEFAULT_SLAM_CAST = 2.5` as its fallback. So the whole +conditional rests on one line of our spellbook tooltip, and on this realm a failed scan and a +correct scan return the same 2.5, which is why the number alone never discriminated. + +### The three answers + +| | Reading | Why it was wrong | +|---|---|---| +| 1 | Harness test, fixture `"1.5 sec cast"`, 44% window → "the chain is healthy" | 1.5 s is vanilla's Slam. Nothing on this realm ships it; the number was invented. | +| 2 | Fixture corrected off the realm's DBC to 2.5 s, 7.4% window → "the macro's own arithmetic, not ours" | The fixture was right. The conclusion was not: 2.5 is the *untalented* cast. | +| 3 | Our tooltip never applied spell-mod op `0xa` | His client reports **2.0 s**, not 2.5. | + +What settled it was one live probe printing every term at once — +`mx=2.7 st=1.0255 u=2.7 c=2.5 w=7.4074` — which showed the swing speed and the swing timer both +healthy and left exactly one term standing. + +### The defect + +`Spell_C::GetCastTime` (`0x6e3340`) applies `SPELLMOD_CASTING_TIME` **inside itself**: + +``` +6e33aa: mov %eax,-0x4(%ebp) ; base + perLevel·(level − baseLevel) +6e33b0: cmp %esi,%eax ; row minimum +6e33b4: mov %esi,-0x4(%ebp) ; …clamped to it +6e33b7: push $0xa ; op 10 = SPELLMOD_CASTING_TIME +6e33be: call 0x6e6af0 ; value = (value + flat) · pct / 100 +``` + +and the tooltip's cast line is a call to it, not a DBC read (`52eb48: mov 0x8(%ebx),%ecx` / +`52eb4b: push $0x1` / `52eb4d: call 0x6e3340`). We read `SpellCastTimes.dbc` directly, so a +talent-shortened cast showed its base length — a deviation `ui_action/mod.rs` and `cast_times.rs` +both *documented* without anyone pricing it. + +The realm's numbers make the price concrete. Slam (1464/8820/11604/11605) is `CastingTimeIndex` +20, and row 20 is `{base 2500, perLevel 0, minimum 2500}`. Improved Slam is two ranks — 12862 and +12330, aura 107 / misc 10 / `basePoints` −251 and −501 with `dieSides` 1, a flat −250 or −500 ms +on op 10 against `SpellFamilyFlags` bit 21, family 4 (the five-rank version is `zzOLD`). So a 2/2 +warrior casts Slam in 2.0 s. + +On his 2.70 s main hand: + +| Slam cast | no-clip window | share of the swing | +|---|---|---| +| 2.5 s (ours) | 0.20 s | 7.4% | +| 2.0 s (his client) | 0.70 s | 25.9% | + +It is a size, not a sign. 0.20 s is narrower than human reaction time, so a player pressing by +hand overshoots it every press; 0.70 s he lands in every swing. 477-to-0 and "works just fine on +the normal client" are the same observation. + +Note the **order**: the minimum clamp is 2500 and it runs *before* the modifier. Applying op +`0xa` before the clamp would pin Slam at 2.5 s for ever and look exactly like the bug it was +meant to fix. + +### What changed + +`Spells::cast_time_ms` is now `GetCastTime` — row resolve, minimum clamp, then op `0xa` — with +`cast_time_signed_ms` for the reference's `allowNegative` form that the tooltip's "Instant cast" +ladder needs (`0x52ebce`), and `cast_time_row_ms` kept for the one question that wants the row +alone: whether a spell has a cast time *authored* at all, which is `opens_cast_bar`'s gate. +The four live readers — the two `PendingCast` arms, the nampower press window and the +moving-refusal — and the tooltip's cast line all pass the modifier tables through. The tooltip +feed re-pushes on a level change too, since the cast line is level-scaled now. + +Still deviating, and now written where it lives: the final `UNIT_MOD_CAST_SPEED` multiplier +(`6e342c`, taken only when `Attributes & 0x30` is clear) is not applied, because no site streams +unit field 139. It is 1.0 without a casting-speed aura, so it moves nothing a physical ability +does — a Curse of Tongues target's cast lines are the case that still reads short.