From b249e6fbb65612642c6d1dd81bd950cc9b130556 Mon Sep 17 00:00:00 2001 From: Sam Date: Tue, 15 Sep 2026 17:00:35 +0700 Subject: [PATCH] learning a spell says so in chat, and the map's backdrop is back - learning a spell prints "You have learned a new spell/ability/recipe" in chat, and unlearning one prints its line, on the reference's own gates (2243, 2246) - the world map's dark backdrop is back: the screen size is seeded before the interface loads, so BlackoutWorld is sized from the real window, not 1024x768 (2242) - the trainer filter repaints again: the engine fires TRAINER_UPDATE from inside the filter verbs, as the reference does (2244) - the zone channels, the keybinding table and the default language seed at the entry edge, before any addon asks (2241) - WOW_UI_COST=1 counts the frame's UI material asset events and names the first hundred modified (2236) Co-Authored-By: Claude Fable 5.1 --- crates/benilla-app/src/bindings.rs | 123 +++--- crates/benilla-app/src/net/apply.rs | 22 +- crates/benilla-app/src/net/apply/spells.rs | 353 +++++++++++++++++- crates/benilla-app/src/ui_chat.rs | 4 + crates/benilla-app/src/ui_chat/channels.rs | 33 ++ crates/benilla-app/src/ui_pass.rs | 50 ++- crates/benilla-app/src/ui_script/lifecycle.rs | 63 +++- .../src/ui_script/trainer_tests.rs | 46 +++ .../src/ui_script/world_entry_tests.rs | 161 ++++++++ crates/benilla-app/src/ui_trainer/mod.rs | 13 +- crates/benilla-app/src/ui_unit.rs | 29 ++ crates/benilla-formats/src/lib.rs | 4 +- crates/benilla-formats/src/spells/display.rs | 226 +++++++++++ crates/benilla-formats/src/spells/mod.rs | 7 +- crates/benilla-ui/src/script/trainer/mod.rs | 28 ++ 15 files changed, 1070 insertions(+), 92 deletions(-) diff --git a/crates/benilla-app/src/bindings.rs b/crates/benilla-app/src/bindings.rs index 295a05a4..61aec3d8 100644 --- a/crates/benilla-app/src/bindings.rs +++ b/crates/benilla-app/src/bindings.rs @@ -183,13 +183,14 @@ impl BindingsState { } /// Which files this session's bindings live in — the macros-files pattern -/// ([`crate::ui_macro::MacroFiles`]), resolved once the character is known. +/// ([`crate::ui_macro::MacroFiles`]), written by [`seed_bindings_for_vm`] and read by the save +/// verb. It carried a session-keyed `identity` memo while the character set was loaded from a +/// per-frame system and had to recognise "same character, new VM"; the seed runs exactly once per +/// VM by construction, so there is nothing left to dedupe (decision 2241). #[derive(Resource, Default)] struct BindingFiles { account: Option, character: Option, - /// Whose set 2 is loaded — **in the VM that is live now**. See [`load_character_bindings`]. - identity: crate::ui_script::VmMemo>, } /// Label for this module's systems inside [`crate::ui_script::UiInput`] — the UI key feed is @@ -208,20 +209,21 @@ impl Plugin for BindingsPlugin { .add_systems( Update, ( - // Once per **VM**, not once per process (decision 1290): a login builds a - // fresh one, and an unseeded VM has no command registry at all — - // `sync_dispatch` would build an empty map and every keybind in the session - // would be dead. Hence `Update` with a session-keyed claim rather than - // `PostStartup`, ordered ahead of the pair that reads what it registers. - seed_bindings.before(sync_dispatch), + // **The registry and both sets are seeded at the VM's birth, not here** + // (decision 2241): [`seed_bindings_for_vm`] runs inside + // `load_ingame_ui_on_world_entry`, before FrameXML and every addon. A + // session-keyed `Update` claim answered *which* VM but not *when* inside its + // life — and this table's readers are load-edge readers: stock + // `ActionButton_OnLoad` paints its hotkey corner from `GetBindingKey` at + // OnLoad, and an addon's `SetBinding` on a stock command needs the command to + // exist. (sync_dispatch, latch_and_dispatch) .chain() .in_set(crate::ui_script::UiInput) .in_set(BindingSet) .before(benilla_world::schedule::WorldStage::Input) .run_if(in_state(ClientState::InWorld)), - load_character_bindings.run_if(in_state(ClientState::InWorld)), - drain_binding_requests.after(load_character_bindings), + drain_binding_requests, ), ); } @@ -242,24 +244,32 @@ pub(crate) fn registry_commands() -> Vec { .collect() } -/// Register the command registry with the engine table and seed the account set from disk — once -/// per VM, before any window opens. -fn seed_bindings( - script: Option>, - mut files: ResMut, - mut seeded: Local>, -) { - let Some(mut script) = script else { return }; - if !seeded.claim(&script) { - return; - } +/// **The keybinding table goes into the VM before a single interface file runs** (decision 2241, +/// through the seam 2240 established): the command registry, the account set, and the character's +/// own set if it has one — all of it in one call at the VM's birth. +/// +/// This was two `Update` systems with session-keyed claims. That answered *which* VM the memory was +/// about (1290) and not *when inside its life* it ran, and since 2226 the whole interface load — +/// FrameXML, every addon's file scope, `ADDON_LOADED`, `VARIABLES_LOADED`, `PLAYER_LOGIN` — happens +/// inside one exclusive call that precedes the first `Update`. Two readers that costs: +/// +/// - stock `ActionButton_OnLoad` ends in `ActionButton_UpdateHotkeys`, which paints the corner from +/// `GetBindingText(GetBindingKey(action), …)`. With no registry every hotkey corner painted blank +/// and only recovered on the `UPDATE_BINDINGS` `sync_dispatch` fires a frame later; +/// - an addon's own `Bindings.xml` rows ARE registered during the walk, so the table held *only* +/// those during the burst: `GetBindingKey("TOGGLEWORLDMAP")` answered nothing, `SetBinding` on a +/// stock command was a silent nil, and the addons' rows occupied the low indices the Key Bindings +/// window walks. +/// +/// Seeding before the walk is also the reference's own order, and the engine was already built for +/// it: `seed_binding_set` keeps the diff by NAME as well as positionally precisely so a set can be +/// seeded before a command exists (1201), and `register_bindings` is idempotent per name, so the +/// addon rows the walk registers afterwards still land. +pub(crate) fn seed_bindings_for_vm(world: &mut World, script: &mut UiScript) { script.register_bindings(®istry_commands()); - files.account = crate::local_state::bindings_account_path(); - if let Some(overrides) = read_diff(&files.account) { - script.seed_binding_set(1, Some(store::resolve(&overrides))); - } else { - script.seed_binding_set(1, Some(store::resolve(&[]))); - } + let account = crate::local_state::bindings_account_path(); + let overrides = read_diff(&account).unwrap_or_default(); + script.seed_binding_set(1, Some(store::resolve(&overrides))); script.load_binding_set(1); // The pair, not just the first half: `SPECS` ∪ `ABSENT` is the client's whole 1.12 command // surface, and a log line that says only how many landed cannot say how much is left @@ -270,6 +280,32 @@ fn seed_bindings( SPECS.len() + commands::ABSENT.len(), commands::ABSENT.len() ); + + // The character's own set, if the roster names one — its file existing makes it the active set, + // the reference's own rule. The identity is the same one the edge resolves for the AddOn enable + // state a few lines above this call; absent (a rigged or capture run) leaves set 2 unseeded, + // exactly as the old per-frame system's early return did. + let id = world + .get_resource::() + .and_then(crate::ui_macro::identity); + let character = id + .as_ref() + .and_then(|(realm, name)| crate::local_state::bindings_character_path(realm, name)); + match read_diff(&character) { + Some(overrides) => { + script.seed_binding_set(2, Some(store::resolve(&overrides))); + script.load_binding_set(2); + info!("bindings: character-specific set loaded"); + } + None => { + script.seed_binding_set(2, None); + script.load_binding_set(1); + } + } + if let Some(mut files) = world.get_resource_mut::() { + files.account = account; + files.character = character; + } } /// Read + parse one diff file; `None` when absent/unreadable (defaults). @@ -285,37 +321,6 @@ fn read_diff(path: &Option) -> Option>, - roster: Res, - mut files: ResMut, -) { - let Some(mut script) = script else { return }; - let Some(id) = crate::ui_macro::identity(&roster) else { - return; - }; - // Session-keyed (1290): re-entering the world as the SAME character still meets a fresh VM - // with no set 2 in it, so "same identity" is only a reason to skip within one VM. - if files.identity.get(&script).as_ref() == Some(&id) { - return; - } - files.character = crate::local_state::bindings_character_path(&id.0, &id.1); - *files.identity.get(&script) = Some(id); - match read_diff(&files.character) { - Some(overrides) => { - script.seed_binding_set(2, Some(store::resolve(&overrides))); - script.load_binding_set(2); - info!("bindings: character-specific set loaded"); - } - None => { - script.seed_binding_set(2, None); - script.load_binding_set(1); - } - } -} - /// Drain the VM's queued binding requests. /// /// Two of them. `Save` persists on the window's SaveBindings (Okay): write the set's diff; saving diff --git a/crates/benilla-app/src/net/apply.rs b/crates/benilla-app/src/net/apply.rs index 405ae9fd..eb321b52 100644 --- a/crates/benilla-app/src/net/apply.rs +++ b/crates/benilla-app/src/net/apply.rs @@ -1141,12 +1141,28 @@ pub(crate) fn apply_net_updates( cooldowns, } => spell_book(spell_ids, cooldowns, &mut ui_actions.0, &mut ui_actions.9), SessionEvent::ActionButtons { buttons } => action_buttons(buttons, &mut ui_actions.0), - SessionEvent::SpellLearned { spell_id } => learned_spell(spell_id, &mut ui_actions.0), - SessionEvent::SpellRemoved { spell_id } => removed_spell(spell_id, &mut ui_actions.0), + SessionEvent::SpellLearned { spell_id } => learned_spell( + spell_id, + &mut ui_actions.0, + ui_actions.10.as_deref(), + &mut ui_error_keys, + ), + SessionEvent::SpellRemoved { spell_id } => removed_spell( + spell_id, + &mut ui_actions.0, + ui_actions.10.as_deref(), + &mut ui_error_keys, + ), SessionEvent::SpellSuperceded { old_spell_id, new_spell_id, - } => superceded_spell(old_spell_id, new_spell_id, &mut ui_actions.0), + } => superceded_spell( + old_spell_id, + new_spell_id, + &mut ui_actions.0, + ui_actions.10.as_deref(), + &mut ui_error_keys, + ), SessionEvent::CastResult { spell_id, success, diff --git a/crates/benilla-app/src/net/apply/spells.rs b/crates/benilla-app/src/net/apply/spells.rs index ac9e71f6..8b40482f 100644 --- a/crates/benilla-app/src/net/apply/spells.rs +++ b/crates/benilla-app/src/net/apply/spells.rs @@ -5,12 +5,13 @@ use std::time::{Duration, Instant}; +use benilla_formats::LearnAnnouncement; use benilla_protocol::messages::{ActionButton, SpellCooldown}; use bevy::prelude::*; use crate::cooldowns::Cooldowns; use crate::creature_anim::{CastEvent, CastEventKind, Casting, SpellGoTargets}; -use crate::ui_action::{AutoRepeatActive, CastErrors, PlayerActions, Spells}; +use crate::ui_action::{AutoRepeatActive, CastErrors, PlayerActions, Spells, UiError, UiErrorKeys}; use crate::ui_aura::AuraDurations; use crate::ui_cast::{ActiveChannel, CastBarEdge, CastBarFeed, PendingCast, QueuedMeleeSpell}; @@ -50,11 +51,51 @@ pub(super) fn action_buttons(buttons: Vec, actions: &mut PlayerAct /// a level-up rank gain; decision 0237). The spellbook feed diffs `spells` each frame, so the insert /// is all it needs to surface (the add-gate that decides which known spells are *book* entries is /// the feed's, decision 0227). No action-bar change — learning a spell does not bar it. -pub(super) fn learned_spell(spell_id: u32, actions: &mut PlayerActions) { +/// +/// It also **announces the learn in chat**, which is the reference's own tail on this packet and +/// not a nicety: `0x5e61c0` -> `AddSpell(id, slot, 1, 1)` -> the registrar `0x4b25b0` with its +/// announce flag set (decision 2243, [`announce_learn`]). +pub(super) fn learned_spell( + spell_id: u32, + actions: &mut PlayerActions, + spells: Option<&Spells>, + errors: &mut UiErrorKeys, +) { debug!("net: learned spell {spell_id}"); if actions.spells.insert(spell_id) { actions.dirty = true; } + announce_learn(spell_id, spells, errors); +} + +/// **The learn announcement** — the `ERR_LEARN_*` chat line the registrar `0x4b25b0` prints at +/// `0x4b2909` for a spell learned *mid-session* (decision 2243). +/// +/// Which of the three lines, and whether the argText carries the rank, is +/// [`benilla_formats::SpellDisplay::learn_announcement`]'s — it is a `Spell.dbc` `Attributes` read +/// and its byte trail lives with the record. What is this side's is the mapping to catalog keys: +/// ids `0x37`/`0x38`/`0x39` are `MsgKind::Chat` rows carrying chat type `10`, so +/// [`crate::ui_action::UiErrorKeys`] puts all three on the chat window's system channel without +/// this call site naming a surface (decision 1770). +/// +/// A missing catalog (`Spells` absent, a DBC that failed to load) says nothing at all, like every +/// other display path here — and so does an unknown id, which is the reference's own bounds/null +/// bail on the `Spell.dbc` store at the registrar's head (`0x4b25c6` / `0x4b25d2` / `0x4b25e0`, +/// all three jumping straight to the epilogue). +fn announce_learn(spell_id: u32, spells: Option<&Spells>, errors: &mut UiErrorKeys) { + let Some(display) = spells.and_then(|s| s.catalog.get(spell_id)) else { + return; + }; + let Some(kind) = display.learn_announcement() else { + return; + }; + let (key, arg) = match kind { + LearnAnnouncement::Spell => ("ERR_LEARN_SPELL_S", display.ranked_name()), + LearnAnnouncement::Ability => ("ERR_LEARN_ABILITY_S", display.ranked_name()), + // The recipe arm pushes the bare name — it returns before the rank composer. + LearnAnnouncement::Recipe => ("ERR_LEARN_RECIPE_S", display.name.clone()), + }; + errors.0.push(UiError::s(key, arg)); } /// A spell taken back out of the book (`SMSG_REMOVED_SPELL`, decision 1584) — the inverse of @@ -64,16 +105,48 @@ pub(super) fn learned_spell(spell_id: u32, actions: &mut PlayerActions) { /// only visible effect was the points coming back — the talent window went on drawing the ranks it /// had, because [`crate::ui_talent`] derives rank from exactly this set. /// +/// It **announces the unlearn in chat** — *"You have unlearned %s."* — under +/// [`benilla_formats::SpellDisplay::announces_unlearn`]'s four gates (decision 2246). 2243 claimed +/// the opposite here, on a byte citation, and was wrong: it bounded `RemoveSpell 0x5e9fe0` at the +/// `ret 0x8` at `0x5ea28f` and so never read the block at `0x5ea292`, which is a live branch +/// target past that `ret`. `0x5ea2ab push 0x14a` is that block, and `SMSG_REMOVED_SPELL`'s arm +/// (`0x5e43e3`) is the caller that reaches it with the flag set. +/// /// The insert's mirror image, and deliberately no more than that: the spellbook, talent and pet /// feeds all diff `spells` and fire their own refresh events, and /// [`crate::ui_action::LearnedAbilities`] re-derives off the same change (the reference's own /// unlearn write site, `0x4b2c50`). What happens to a **bar button** still pointing at the removed /// spell is a separate law this arm deliberately does not invent — see 1584's scope note. -pub(super) fn removed_spell(spell_id: u32, actions: &mut PlayerActions) { +pub(super) fn removed_spell( + spell_id: u32, + actions: &mut PlayerActions, + spells: Option<&Spells>, + errors: &mut UiErrorKeys, +) { debug!("net: removed spell {spell_id}"); if actions.spells.remove(&spell_id) { actions.dirty = true; } + announce_unlearn(spell_id, spells, errors); +} + +/// **The unlearn announcement** — `ERR_SPELL_UNLEARNED_S` (`0x5ea2ab`), the counterpart to +/// [`announce_learn`] and deliberately not a mirror of it (decision 2246). +/// +/// The argText is the **bare** name: `0x5ea2a3` pushes `SpellRec+0x1e0` and there is no rank +/// composer on this path at all, so a respec'd talent rank reads "You have unlearned Improved +/// Fireball." with no "(Rank 3)". Which spells say it is +/// [`benilla_formats::SpellDisplay::announces_unlearn`]'s — the bytes live with the record. +fn announce_unlearn(spell_id: u32, spells: Option<&Spells>, errors: &mut UiErrorKeys) { + let Some(display) = spells.and_then(|s| s.catalog.get(spell_id)) else { + return; + }; + if !display.announces_unlearn() { + return; + } + errors + .0 + .push(UiError::s("ERR_SPELL_UNLEARNED_S", display.name.clone())); } /// A rank-up (`SMSG_SUPERCEDED_SPELL`): the new rank replaces the old **in the book** (decision @@ -83,11 +156,21 @@ pub(super) fn removed_spell(spell_id: u32, actions: &mut PlayerActions) { /// *here* as well would be a second, weaker copy of that law — weaker because this packet doesn't /// arrive at all when the rank was gained while the character was loading (vmangos suppresses it /// with `IsInWorld()`), which is exactly the case that shipped a dead rank-1 button. -pub(super) fn superceded_spell(old_spell_id: u32, new_spell_id: u32, actions: &mut PlayerActions) { +pub(super) fn superceded_spell( + old_spell_id: u32, + new_spell_id: u32, + actions: &mut PlayerActions, + spells: Option<&Spells>, + errors: &mut UiErrorKeys, +) { debug!("net: superceded spell {old_spell_id} -> {new_spell_id}"); actions.spells.remove(&old_spell_id); actions.spells.insert(new_spell_id); actions.dirty = true; + // A rank-up announces exactly like a first learn, and only once: the supersede pair `0x4b2f50` + // calls the unlearn `0x4b2c50` and then the registrar `0x4b25b0` with `mov edx,0x1` + // (`0x4b2f61`), and the unlearn half holds no `DisplayError` call at all (decision 2243). + announce_learn(new_spell_id, spells, errors); } /// The server's verdict on our cast (`SMSG_CAST_RESULT`). @@ -1014,15 +1097,167 @@ mod tests { use super::*; use benilla_protocol::messages::ACTION_KIND_SPELL; + /// A catalog holding one row, so the announce tests can name a real `Attributes` value. + fn catalog_with(id: u32, display: benilla_formats::SpellDisplay) -> Spells { + let mut spells = Spells::empty_for_tests(); + spells.catalog = + benilla_formats::SpellCatalog::from_displays([(id, display)].into_iter().collect()); + spells + } + + /// The report this arm was missing entirely: training at a class trainer printed nothing in + /// chat. Heroic Strike carries `SPELL_ATTR_ABILITY`, so the reference's `0x4b29b3 add eax,0x37` + /// lands on `0x38` — the *ability* wording — and the argText is the client's `"%s (%s)"`. + #[test] + fn a_learned_ability_announces_the_ability_line_with_its_rank() { + let spells = catalog_with( + 78, + benilla_formats::SpellDisplay { + name: "Heroic Strike".to_string(), + rank: Some("Rank 1".to_string()), + attributes: 0x10, + ..Default::default() + }, + ); + let mut actions = PlayerActions::default(); + let mut errors = UiErrorKeys::default(); + learned_spell(78, &mut actions, Some(&spells), &mut errors); + + assert_eq!(errors.0.len(), 1, "one line, once"); + assert_eq!(errors.0[0].key, "ERR_LEARN_ABILITY_S"); + assert_eq!(errors.0[0].arg_s(), Some("Heroic Strike (Rank 1)")); + } + + /// The other two arms of the same block, from the same entry point: a plain row is a *spell*, + /// and a tradeskill row is a *recipe* whose argText drops the rank (the reference returns from + /// `0x4b2944` without ever reaching the subtext composer). + #[test] + fn the_spell_and_recipe_arms_pick_their_own_key_and_argument() { + let spells = catalog_with( + 133, + benilla_formats::SpellDisplay { + name: "Fireball".to_string(), + rank: Some("Rank 1".to_string()), + attributes: 0x10000, + ..Default::default() + }, + ); + let mut errors = UiErrorKeys::default(); + learned_spell( + 133, + &mut PlayerActions::default(), + Some(&spells), + &mut errors, + ); + assert_eq!(errors.0[0].key, "ERR_LEARN_SPELL_S"); + assert_eq!(errors.0[0].arg_s(), Some("Fireball (Rank 1)")); + + let spells = catalog_with( + 2550, + benilla_formats::SpellDisplay { + name: "Cooking".to_string(), + rank: Some("Apprentice".to_string()), + attributes: 0x20, + ..Default::default() + }, + ); + let mut errors = UiErrorKeys::default(); + learned_spell( + 2550, + &mut PlayerActions::default(), + Some(&spells), + &mut errors, + ); + assert_eq!(errors.0[0].key, "ERR_LEARN_RECIPE_S"); + assert_eq!( + errors.0[0].arg_s(), + Some("Cooking"), + "the recipe arm pushes the bare name" + ); + } + + /// A rank-up is `SMSG_SUPERCEDED_SPELL`, and the supersede pair `0x4b2f50` reaches the + /// registrar with `edx = 1` — so it announces, naming the NEW rank, and exactly once (the + /// unlearn half holds no `DisplayError` call). + #[test] + fn a_rank_up_announces_the_new_rank_once() { + let mut spells = Spells::empty_for_tests(); + spells.catalog = benilla_formats::SpellCatalog::from_displays( + [ + ( + 78, + benilla_formats::SpellDisplay { + name: "Heroic Strike".to_string(), + rank: Some("Rank 1".to_string()), + attributes: 0x10, + ..Default::default() + }, + ), + ( + 284, + benilla_formats::SpellDisplay { + name: "Heroic Strike".to_string(), + rank: Some("Rank 2".to_string()), + attributes: 0x10, + ..Default::default() + }, + ), + ] + .into_iter() + .collect(), + ); + let mut actions = PlayerActions::default(); + actions.spells.insert(78); + let mut errors = UiErrorKeys::default(); + superceded_spell(78, 284, &mut actions, Some(&spells), &mut errors); + + assert_eq!(errors.0.len(), 1, "one line for a rank-up, not two"); + assert_eq!(errors.0[0].key, "ERR_LEARN_ABILITY_S"); + assert_eq!(errors.0[0].arg_s(), Some("Heroic Strike (Rank 2)")); + } + + /// The silent cases, which are the ones a naive "print on every learn" would get wrong: a + /// `DO_NOT_DISPLAY` row (every language, every weapon proficiency — the packets a fresh + /// character receives in bulk), and a spell the catalog does not know at all. + #[test] + fn a_do_not_display_spell_and_an_unknown_id_announce_nothing() { + let spells = catalog_with( + 668, + benilla_formats::SpellDisplay { + name: "Language: Common".to_string(), + attributes: 0xC0, + ..Default::default() + }, + ); + let mut errors = UiErrorKeys::default(); + learned_spell( + 668, + &mut PlayerActions::default(), + Some(&spells), + &mut errors, + ); + learned_spell( + 99999, + &mut PlayerActions::default(), + Some(&spells), + &mut errors, + ); + assert!( + errors.0.is_empty(), + "PASSIVE|DO_NOT_DISPLAY is silent, and so is an id with no record" + ); + } + #[test] fn learned_spell_adds_to_the_book_once() { let mut actions = PlayerActions::default(); - learned_spell(6603, &mut actions); + let mut errors = UiErrorKeys::default(); + learned_spell(6603, &mut actions, None, &mut errors); assert!(actions.spells.contains(&6603)); assert!(actions.dirty, "a new spell dirties the feed"); actions.dirty = false; - learned_spell(6603, &mut actions); + learned_spell(6603, &mut actions, None, &mut errors); assert!( !actions.dirty, "re-learning a known spell is a no-op (insert returns false)" @@ -1044,7 +1279,7 @@ mod tests { }, ); - superceded_spell(78, 284, &mut actions); + superceded_spell(78, 284, &mut actions, None, &mut UiErrorKeys::default()); assert!( !actions.spells.contains(&78), @@ -1799,14 +2034,114 @@ mod tests { let mut actions = PlayerActions::default(); actions.spells.extend([14522, 14788, 14789]); - removed_spell(14788, &mut actions); + let mut errors = UiErrorKeys::default(); + removed_spell(14788, &mut actions, None, &mut errors); assert!(!actions.spells.contains(&14788)); assert!(actions.dirty); actions.dirty = false; - removed_spell(14788, &mut actions); + removed_spell(14788, &mut actions, None, &mut errors); assert!(!actions.dirty, "a spell we never knew is not a repaint"); } + + /// The unlearn line and its four silent gates (decision 2246) — the behaviour decision 2243 + /// asserted, with a byte citation, did not exist. The argText is the BARE name: there is no + /// rank composer on this path, so a respec'd rank says "Improved Fireball", never + /// "Improved Fireball (Rank 3)". + #[test] + fn an_unlearn_announces_the_bare_name_and_four_things_silence_it() { + let row = |attributes: u32, cast_ui: u32| benilla_formats::SpellDisplay { + name: "Improved Fireball".to_string(), + rank: Some("Rank 3".to_string()), + attributes, + cast_ui, + ..Default::default() + }; + + let mut errors = UiErrorKeys::default(); + removed_spell( + 11069, + &mut PlayerActions::default(), + Some(&catalog_with(11069, row(0, 0))), + &mut errors, + ); + assert_eq!(errors.0.len(), 1); + assert_eq!(errors.0[0].key, "ERR_SPELL_UNLEARNED_S"); + assert_eq!( + errors.0[0].arg_s(), + Some("Improved Fireball"), + "no rank on the unlearn path" + ); + + // …and each gate on its own, every one of them silent. + for (attributes, cast_ui, why) in [ + ( + 0x20, + 0, + "IS_TRADESKILL — the container join zeroes the flag at 0x5ea170", + ), + ( + 0, + 1, + "castUI > 0 takes the container walk and never reaches 0x5ea292", + ), + (0x80, 0, "DO_NOT_DISPLAY — the sign test at 0x5ea29b"), + ] { + let mut errors = UiErrorKeys::default(); + removed_spell( + 11069, + &mut PlayerActions::default(), + Some(&catalog_with(11069, row(attributes, cast_ui))), + &mut errors, + ); + assert!(errors.0.is_empty(), "{why}"); + } + + // An unknown id says nothing, like every other display path here. + let mut errors = UiErrorKeys::default(); + removed_spell( + 99999, + &mut PlayerActions::default(), + Some(&catalog_with(11069, row(0, 0))), + &mut errors, + ); + assert!(errors.0.is_empty()); + } + + /// `castUI` gates the unlearn line and NOT the learn line — the reference tests it only + /// afterwards, at `0x4b29bf`, to decide the book slot. So a `castUI > 0` spell announces when + /// it arrives and is silent when it goes. Asserted because the two methods reading the same + /// field for different answers is exactly the kind of asymmetry a later edit "tidies" away. + #[test] + fn cast_ui_silences_the_unlearn_but_not_the_learn() { + let spells = catalog_with( + 1234, + benilla_formats::SpellDisplay { + name: "Some Castbar Spell".to_string(), + cast_ui: 2, + ..Default::default() + }, + ); + + let mut errors = UiErrorKeys::default(); + learned_spell( + 1234, + &mut PlayerActions::default(), + Some(&spells), + &mut errors, + ); + assert_eq!(errors.0.len(), 1, "the learn block never reads castUI"); + assert_eq!(errors.0[0].key, "ERR_LEARN_SPELL_S"); + + let mut errors = UiErrorKeys::default(); + removed_spell( + 1234, + &mut PlayerActions::default(), + Some(&spells), + &mut errors, + ); + assert!(errors.0.is_empty(), "…but the unlearn block does"); + } /// **The GO-deferred auto-attack start** (`HandleSpellGo` @ `0x6e83c0`, decision 1593) — the /// half of `combat-feel-law.md` §A3 benilla shipped without, because ten hand-picked warrior /// rows were read as a census of `AttributesEx2 & 0x100000`. The real file carries the bit on diff --git a/crates/benilla-app/src/ui_chat.rs b/crates/benilla-app/src/ui_chat.rs index 3f77f471..9f17526b 100644 --- a/crates/benilla-app/src/ui_chat.rs +++ b/crates/benilla-app/src/ui_chat.rs @@ -51,6 +51,10 @@ mod tests; pub(crate) use away::AfkMirror; pub(crate) use broadcast::Broadcast; +/// The zone-channel catalog's seed. Called from the world-entry UI load, like the restore below +/// and for the neighbouring reason: the verbs that read it are read at addon file scope, and an +/// `Update` push lands after the whole burst — decision 2241. +pub(crate) use channels::seed_zone_channel_catalog; /// The joined-channel roster + the `ChatChannels.dbc` catalog. Read outside this module by the /// world-state readout ([`crate::world_state_ui`]), whose `Type == 1` gate is "has the player /// joined a zone-dependent defense channel". diff --git a/crates/benilla-app/src/ui_chat/channels.rs b/crates/benilla-app/src/ui_chat/channels.rs index 50c97c78..90f34e92 100644 --- a/crates/benilla-app/src/ui_chat/channels.rs +++ b/crates/benilla-app/src/ui_chat/channels.rs @@ -635,6 +635,39 @@ pub(super) fn auto_join_zone_channels( walk.at = Some(zone_id); } +/// **The zone-channel catalog goes into the VM before a single interface file runs** (decision +/// 2241, through the seam 2240 established). +/// +/// The walk above already feeds it "before any of those gates" — but its gates are the *zone's*, +/// and the load edge is earlier than all of them: since 2226 the entry mints a fresh VM and runs +/// FrameXML, every addon's file scope, `ADDON_LOADED`, `VARIABLES_LOADED` and `PLAYER_LOGIN` inside +/// one exclusive call, and this system's first `Update` tick is after that whole burst. An empty +/// catalog is not "no zone yet" to the three verbs that read it — it is *"no such built-in +/// channel"*, the leg whose damage is documented at length above: a `General` filed as a **custom** +/// channel in the chat cache, and a real `CMSG_JOIN_CHANNEL("General")` on the wire. +/// +/// Zone-less on purpose: at this edge the zone is unknowable by construction (the walk is not armed +/// — `EnteredWorldMessage` is read in `Update` — and the body is still settling), and zone-less is +/// the right content rather than a degraded one, for the reason [`auto_join_zone_channels`] gives. +/// The walk re-feeds with the resolved names the moment a zone lands, and pays one redundant +/// identical push per login for it: its memo is a `VmMemo` on its own `Local`, which this call +/// cannot reach, and a resource it could reach would not survive a `/reload` correctly. +pub(crate) fn seed_zone_channel_catalog( + world: &mut World, + script: &mut benilla_ui::script::UiScript, +) { + let Some(channels) = world.get_resource::() else { + return; + }; + if channels.channels.is_empty() { + return; // no `ChatChannels.dbc` — the no-install case, a no-op exactly as in the walk + } + let city = world + .get_resource::() + .and_then(|a| city_word(&a.0)); + script.set_zone_channel_catalog(zone_channel_catalog(&channels.channels, "", false, city)); +} + /// Every `ChatChannels.dbc` row as the VM needs it (decision 1908; wow-re chat-cache-grammar.md /// §5-6): the id, the Shortcut the verbs compare a typed name against, the name composed for /// `zone_name` — `None` when the composition has nothing to substitute (a zone-dependent row with diff --git a/crates/benilla-app/src/ui_pass.rs b/crates/benilla-app/src/ui_pass.rs index 203f29c3..2deb3d44 100644 --- a/crates/benilla-app/src/ui_pass.rs +++ b/crates/benilla-app/src/ui_pass.rs @@ -578,7 +578,8 @@ impl Plugin for PlayerUiPlugin { UiQuadAppend.after(benilla_world::schedule::WorldStage::Input), ) .add_systems(Update, clear_ui_overlays.before(UiQuadAppend)) - .add_systems(Update, rebuild_ui_mesh.after(UiQuadAppend)); + .add_systems(Update, rebuild_ui_mesh.after(UiQuadAppend)) + .add_systems(Last, count_material_events); // Dev-only demo feeder (mirrors the repo's env-var dev-instrument gating — e.g. `$WOW_CAPTURE`, // `$WOW_TILE_RADIUS` — since the compile-time `dev` cargo feature decision 0026 sets as the @@ -822,6 +823,53 @@ impl Run { } } +/// `WOW_UI_COST=1` also counts the frame's [`UiQuadMaterial`] asset events. bevy re-extracts and +/// re-prepares a material — one bind group and one uniform buffer, created on the render thread — +/// for exactly each `Added`/`Modified` event, and nothing else re-prepares one (2236: two or three +/// such events a frame at a parked pin cost 9.5 traced ms on Intel's DX12 driver, which prices one +/// re-prepared material in milliseconds there). So this count is the render side's material work +/// for the frame, read where it is caused. +fn count_material_events( + mut events: MessageReader>, + materials: Res>, + server: Res, + mut named: Local, +) { + if !crate::ui_script::extract::ui_cost_enabled() { + events.clear(); + return; + } + let (mut added, mut modified, mut unused) = (0usize, 0usize, 0usize); + for event in events.read() { + match event { + AssetEvent::Added { .. } => added += 1, + AssetEvent::Modified { id } => { + modified += 1; + // The first hundred name themselves: the texture and the tint the write left. + if *named < 100 { + *named += 1; + if let Some(m) = materials.get(*id) { + let path = m + .texture + .as_ref() + .and_then(|t| server.get_path(t.id())) + .map(|p| p.to_string()); + info!( + "[ui-mat] modified {id:?} texture={path:?} tint={:?}", + m.tint + ); + } + } + } + AssetEvent::Removed { .. } | AssetEvent::Unused { .. } => unused += 1, + AssetEvent::LoadedWithDependencies { .. } => {} + } + } + if added + modified + unused > 0 { + info!("[ui-mat] added={added} modified={modified} unused={unused}"); + } +} + /// The rebuild's reused GPU-facing state, kept across frames so a rebuild pays for what CHANGED, /// never for existence (the 0353 demand-price law, UI lane): batch entities and their mesh assets /// are pools reused by run index (geometry rewritten in place — no allocator or archetype churn), diff --git a/crates/benilla-app/src/ui_script/lifecycle.rs b/crates/benilla-app/src/ui_script/lifecycle.rs index 5b3e9d9d..aca510e3 100644 --- a/crates/benilla-app/src/ui_script/lifecycle.rs +++ b/crates/benilla-app/src/ui_script/lifecycle.rs @@ -363,24 +363,49 @@ pub(crate) fn run_pending_entry_load(world: &mut World) { ); } -/// Install the host's font engine into `script` for the CURRENT raster seam, if the glyph atlas -/// exists yet — the load edge's half of [`super::extract::seat_text_measurer`]. +/// **The raster seam, at the load edge**: the VM's screen size and its font engine, both under the +/// CURRENT window and `uiScale` — the load-edge half of [`super::extract::tick_script`]'s own +/// per-frame pair. +/// +/// **The screen size is the half that had been missing, and it is a real screen bug** (decision +/// 2242). A fresh `Model` starts at **1024×768** and `set_screen_size` is called from +/// `tick_script`, an `Update` system — so between 2226 (the entry mints its own VM) and this, every +/// `` in FrameXML and every addon's file scope read `GetScreenWidth()/GetScreenHeight()` as +/// 1024×768 no matter what window the player had. Verified live with a probe addon: `SCREENPROBE +/// file-scope … 1024x768`, on both logins of a round trip. +/// +/// Almost everything survives that, because almost everything is *anchored* and the first `Update` +/// resize re-solves it. What does not survive is a number a file **computed once** from those +/// getters — and the stock world map is exactly that: `WorldMapFrame_OnLoad` sizes `BlackoutWorld`, +/// the full-screen quad that hides the world behind the map, with `GetScreenWidth()`/ +/// `GetScreenHeight()` and never touches it again. Sized 1024×768 UI units on a wider window, it +/// leaves the world showing along the right and bottom edges — the director's report. /// /// No atlas means no measure to be had (it bakes on the first `Update`, from the patch chain and /// the window's real `scale_factor`); the per-frame pass seats one on the first frame it appears, -/// exactly as before. -fn seat_text_measurer_for_load(world: &mut World, script: &mut UiScript) { +/// exactly as before. The screen size goes in either way, and only when the window has a real size +/// — a bare test world has none, and `0` would be worse than the default it already carries. +fn seat_raster_seam_for_load(world: &mut World, script: &mut UiScript) { let ui_scale = world .get_resource::() .map_or(1.0, |c| c.0); - let h = { + let (w, h) = { let mut q = world.query_filtered::<&Window, With>(); - q.single(world).map_or(0.0, Window::height) + q.single(world) + .map_or((0.0, 0.0), |win| (win.width(), win.height())) }; + let s = super::seam_scale(h, ui_scale); + if w > 0.0 && h > 0.0 { + // `tick_script`'s own line, with its own units: the VM lives in 768-tall virtual space + // (decision 0582), so the window's logical size divided by the seam scale IS the screen + // the getters answer. No `UIParent_ManageFramePositions` beside it, unlike there: nothing + // is laid out yet, and the first tick will see no resize to re-run it for. + script.set_screen_size(w / s, h / s); + } let Some(atlas) = world.get_resource::() else { return; }; - super::extract::seat_text_measurer(script, atlas, super::seam_scale(h, ui_scale)); + super::extract::seat_text_measurer(script, atlas, s); } /// Materialize the in-game UI for **this** session. @@ -499,6 +524,20 @@ pub(crate) fn load_ingame_ui_on_world_entry(world: &mut World) { .get_resource::() .is_none_or(crate::cvars::CvarPersist::addon_version_check) }); + // **…and the rest of the same class** (decision 2241). Each of these was a per-VM claim in + // `Update`, which answers *which* VM and not *when inside its life* — and every one of them + // backs a Lua getter the load burst below reads: + // + // - the **zone-channel catalog**, whose empty state is not "no zone yet" to its three verbs + // but *"no such built-in channel"* — the leg that files `General` as a custom channel in the + // chat cache and puts a real `CMSG_JOIN_CHANNEL` on the wire; + // - the **keybinding table**, without which stock `ActionButton_OnLoad` paints a blank hotkey + // corner and an addon's `SetBinding` on a stock command is a silent nil; + // - the **default language**, which `ChatFrame_OnEvent`'s `PLAYER_ENTERING_WORLD` arm turns + // into the `[Language]` prefix gate for every chat line of the session. + crate::ui_chat::seed_zone_channel_catalog(world, &mut script); + crate::bindings::seed_bindings_for_vm(world, &mut script); + crate::ui_unit::seed_default_language(world, &mut script); // **The world map's catalog, before the first addon file runs** (decision 2240). The continent // and zone lists behind `GetMapContinents`/`GetMapZones` are static DBC data, and the corpus // reads them at file scope: Astrolabe — the positioning library under Questie and Cartographer @@ -507,7 +546,13 @@ pub(crate) fn load_ingame_ui_on_world_entry(world: &mut World) { // so that table was built from two empty lists and every icon placement afterwards indexed a // nil zone. Same shape as the four seeds above, and the reference has no timing here at all. crate::ui_world_map::seed_world_map_catalog(world, &mut script); - // **The VM's font engine, before the first `` runs** (decision 2028). Every file the + // **The raster seam — the screen size and the font engine — before the first `` runs** + // (decisions 2242 and 2028). The screen size is 2242's: a fresh VM starts at 1024×768 and the + // feed that corrects it is an `Update` system, so every OnLoad that *computes* from + // `GetScreenWidth()`/`GetScreenHeight()` baked the wrong number for the session — the stock + // world map's full-screen blackout quad among them. + // + // The font engine is 2028's. Every file the // walk below loads may measure the text it just set — the era's own tab law is // `label:GetStringWidth() + 40` at OnLoad, and the addon corpus writes the same pair — and a // `GetStringWidth` with no measurer installed answers 0. Seated only from the per-frame pass @@ -516,7 +561,7 @@ pub(crate) fn load_ingame_ui_on_world_entry(world: &mut World) { // boot VM in `end_ui_session` and calls straight into here, so every `/reload` measured 0 // through its whole load edge and only converged a frame later off whatever poll the caller // had written to survive it. - seat_text_measurer_for_load(world, &mut script); + seat_raster_seam_for_load(world, &mut script); let _ = load_ingame_ui(&mut script, identity.as_ref(), version_check); // The Minimap widget was born a moment ago with `MinimapState::default()`; seed its two live // zoom indices from the persisted CVars now, before anything reads them — the reference's own diff --git a/crates/benilla-app/src/ui_script/trainer_tests.rs b/crates/benilla-app/src/ui_script/trainer_tests.rs index 6453abbe..862cb611 100644 --- a/crates/benilla-app/src/ui_script/trainer_tests.rs +++ b/crates/benilla-app/src/ui_script/trainer_tests.rs @@ -1252,3 +1252,49 @@ fn learning_a_spell_takes_the_detail_pane_with_it_instead_of_stranding_the_last_ assert_eq!(pane(&mut s), (true, "Cleave".into())); assert!(s.errors().is_empty(), "script errors: {:?}", s.errors()); } + +/// **The filter click has to repaint the LIST, not just the engine** (decision 2244) — the +/// director's "filter no longer work", with the dropdown showing Available only and red rows still +/// under it. +/// +/// Every other test in this file asserts `GetNumTrainerServices()`, which is the engine's own +/// answer and moves the instant the mask does. What the player looks at is the row buttons, and +/// those are painted by `ClassTrainerFrame_Update` — which after a filter click is reached from +/// exactly one place: `ClassTrainerFrame_OnEvent`'s `TRAINER_UPDATE` arm. The stock click handler +/// fires no event and calls no update (its `ScrollBar:SetValue(0)` is a no-op at zero), because in +/// the reference the **engine** fires it from the mask-commit thunk `0x4d8c90` +/// (`mov ecx,0x136; jmp 0x703e50`). Ours did not, so the checkbox moved and the list did not. +#[test] +fn a_filter_click_repaints_the_rows_the_player_is_looking_at() { + let mut s = trainer_script(); + s.set_money(50); + s.set_trainer(Some(menu())); + s.fire_event("TRAINER_SHOW", vec![ScriptValue::Str("Sana".into())]); + + // What the player sees: the painted row buttons, counted the way the eye does. + let painted = "\ +local n = 0 +for i = 1, 11 do + local b = getglobal(\"ClassTrainerSkill\"..i) + if b and b:IsVisible() then n = n + 1 end +end +return n"; + let count = |s: &mut UiScript| s.eval::(painted).unwrap(); + assert_eq!(count(&mut s), 6, "six rows on screen before any filtering"); + + s.run("ToggleDropDownMenu(1, nil, ClassTrainerFrameFilterDropDown)") + .unwrap(); + s.run("this = DropDownList1Button3; UIDropDownMenuButton_OnClick()") + .unwrap(); + // The engine's queued `TRAINER_UPDATE` lands on the next tick (a binding cannot re-enter the + // handler dispatch from inside Lua — the queue's own contract). + s.tick(0.016); + assert!(s.errors().is_empty(), "script errors: {:?}", s.errors()); + + assert_eq!( + count(&mut s), + 5, + "the already-known row must LEAVE THE SCREEN, not just the engine's count — this is the \ + director's report: the checkbox moved and the list underneath did not" + ); +} diff --git a/crates/benilla-app/src/ui_script/world_entry_tests.rs b/crates/benilla-app/src/ui_script/world_entry_tests.rs index 21873195..9ce2209d 100644 --- a/crates/benilla-app/src/ui_script/world_entry_tests.rs +++ b/crates/benilla-app/src/ui_script/world_entry_tests.rs @@ -1496,3 +1496,164 @@ MapProbeZones = table.getn({ GetMapZones(1) }) drop(world); let _ = std::fs::remove_dir_all(&tmp); } + +/// **The keybinding table is in the VM before the first addon file runs** (decision 2241). +/// +/// Stock `ActionButton_OnLoad` paints its hotkey corner from `GetBindingText(GetBindingKey(action))` +/// at OnLoad, and an addon that rebinds a stock command needs that command to exist. Seeded from an +/// `Update` system, the table held only the addons' own `Bindings.xml` rows for the whole load edge: +/// `GetBindingKey("TOGGLEWORLDMAP")` answered nothing and `SetBinding` on a stock command was a +/// silent nil. +/// +/// No fixture: the registry is a compile-time table, which is exactly why its absence during the +/// burst was a timing bug and nothing else. +#[test] +fn an_addon_reads_the_keybinding_table_at_file_scope() { + const TOC: &str = "\ +## Interface: 11200 +BindProbe.lua +"; + const LUA: &str = "\ +BindProbeKey = GetBindingKey(\"TOGGLEWORLDMAP\") +BindProbeSet = SetBinding(\"J\", \"TOGGLEWORLDMAP\") +"; + let _l = ENV_LOCK + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let (tmp, _c, _h) = hermetic_addon("bindprobe", "BindProbe", TOC, LUA); + // No `BindingFiles` planted on purpose: the seed takes it optionally, which is what keeps + // every other harness in this file working. + let mut world = booted_world(); + + log_in_as(&mut world, "Onewarrior", 1); + + let script = world + .get_non_send_resource::() + .expect("VM"); + assert_eq!( + script + .eval::>("BindProbeKey") + .ok() + .flatten() + .as_deref(), + Some("M"), + "an addon's file scope must read the stock binding — `M` is TOGGLEWORLDMAP's own default" + ); + assert_eq!( + script.eval::>("BindProbeSet").ok().flatten(), + Some(1), + "…and a rebind of a stock command must take, not answer the empty table's silent nil" + ); + + drop(world); + let _ = std::fs::remove_dir_all(&tmp); +} + +/// **The zone-channel catalog is in the VM before the first addon file runs** (decision 2241) — +/// and this one puts a packet on the wire when it is not. +/// +/// An empty catalog is not "no zone yet" to `JoinChannelByName`: it is *"no such built-in +/// channel"*, so `JoinChannelByName("General")` takes the custom-channel leg and queues a real +/// `CMSG_JOIN_CHANNEL("General")` — a custom channel of that name on the server, and the chat +/// cache damage `ui_chat::channels` documents. Seeded zone-less, the same call matches the row, +/// finds `resolved: None`, and does nothing — the reference's own answer while there is no zone +/// text. +#[test] +fn an_addon_that_joins_general_at_file_scope_puts_nothing_on_the_wire() { + const TOC: &str = "\ +## Interface: 11200 +JoinProbe.lua +"; + const LUA: &str = "JoinChannelByName(\"General\")\n"; + let _l = ENV_LOCK + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let (tmp, _c, _h) = hermetic_addon("joinprobe", "JoinProbe", TOC, LUA); + let mut world = booted_world(); + world.insert_resource(crate::ui_chat::ChannelState { + channels: benilla_formats::ChatChannelsCatalog::from_rows(vec![ + // Row 1 as the shipped table has it: auto-joined, and its `%s` is the zone's name — + // which is what makes it unanswerable before a zone and answerable after. + benilla_formats::ChatChannelRow { + id: 1, + flags: benilla_formats::chat_channel_flags::INITIAL + | benilla_formats::chat_channel_flags::ZONE_DEP, + pattern: "General - %s".into(), + shortcut: "General".into(), + }, + ]), + ..Default::default() + }); + + log_in_as(&mut world, "Onewarrior", 1); + + let queued = world + .get_non_send_resource_mut::() + .expect("VM") + .take_channel_commands(); + assert!( + queued.is_empty(), + "a built-in shortcut with no zone text yet is a no-op, not a custom channel on the \ + server — queued {queued:?}" + ); + + drop(world); + let _ = std::fs::remove_dir_all(&tmp); +} + +/// **The screen size is in the VM before the first `` runs** (decision 2242) — the +/// director's "the map is missing the bg again", from the side that causes it. +/// +/// A fresh `Model` starts at 1024×768 and only `tick_script` (an `Update` system) corrects it, so +/// since 2226 gave the entry its own VM, every OnLoad and every addon file scope read that default. +/// Anchored frames survive it — the first frame's resize re-solves them — but a number a file +/// *computes once* does not, and the stock `WorldMapFrame_OnLoad` computes exactly one: the size of +/// `BlackoutWorld`, the quad that hides the world behind the map. At 1024×768 units on a wider +/// window it stops short of the edges and the world shows through. +#[test] +fn an_addon_reads_the_real_screen_size_at_file_scope() { + const TOC: &str = "\ +## Interface: 11200 +ScreenProbe.lua +"; + const LUA: &str = "\ +ScreenProbeWidth = GetScreenWidth() +ScreenProbeHeight = GetScreenHeight() +"; + let _l = ENV_LOCK + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let (tmp, _c, _h) = hermetic_addon("screenprobe", "ScreenProbe", TOC, LUA); + let mut world = booted_world(); + // 2560×1440 with no `UiScaleCvar` planted (so the dial is 1): the seam scale is 1440/768 = + // 1.875, which puts the VM's screen at 1365.33 × 768 units. The WIDTH is the discriminator — + // 768 is what the height reads under any window, and 1024 is what the width read before this. + world.spawn(( + Window { + resolution: bevy::window::WindowResolution::new(2560, 1440), + ..Default::default() + }, + bevy::window::PrimaryWindow, + )); + + log_in_as(&mut world, "Onewarrior", 1); + + let script = world + .get_non_send_resource::() + .expect("VM"); + let read = |expr: &str| script.eval::>(expr).ok().flatten(); + let width = read("ScreenProbeWidth").expect("the probe ran at file scope"); + assert!( + (width - 2560.0 * 768.0 / 1440.0).abs() < 0.01, + "an addon's file scope must read the REAL screen width in UI units — got {width}, and \ + 1024 is the fresh model's default, i.e. the bug" + ); + assert_eq!( + read("ScreenProbeHeight"), + Some(768.0), + "…and the height is the 768-tall virtual base (decision 0582)" + ); + + drop(world); + let _ = std::fs::remove_dir_all(&tmp); +} diff --git a/crates/benilla-app/src/ui_trainer/mod.rs b/crates/benilla-app/src/ui_trainer/mod.rs index 9e83f151..aaec197f 100644 --- a/crates/benilla-app/src/ui_trainer/mod.rs +++ b/crates/benilla-app/src/ui_trainer/mod.rs @@ -235,20 +235,17 @@ fn resolve_service( // and INDEPENDENT of the service's overall category — so a spell gated only by LEVEL still shows // its already-learned prev-rank prerequisite WHITE, not red. The req id is a real ability id (not // a learn wrapper — verified there too), so there's no hop: look it up directly. The name carries - // its rank exactly as the client does — `"Name (Rank)"` when the spell has a rank subtext, else - // the bare name (the client's `"%s (%s)"`). The client also ORs `KnownHigherRank`; benilla has no - // rank chain, and sequential trainer ranks never reach that clause, so the direct known-check - // covers every real case. + // its rank exactly as the client does — `SpellDisplay::ranked_name`, the shared composer for the + // client's `"%s (%s)"` literal (decision 2243). The client also ORs `KnownHigherRank`; benilla + // has no rank chain, and sequential trainer ranks never reach that clause, so the direct + // known-check covers every real case. let ability_reqs = wire .req_spells .iter() .filter(|&&s| s != 0) .map(|&s| { let name = match spells.get(s) { - Some(d) => match d.rank.as_deref() { - Some(rank) if !rank.is_empty() => format!("{} ({})", d.name, rank), - _ => d.name.clone(), - }, + Some(d) => d.ranked_name(), None => format!("Spell {s}"), }; TrainerAbilityReq { diff --git a/crates/benilla-app/src/ui_unit.rs b/crates/benilla-app/src/ui_unit.rs index 87a2f0ba..95f23c9e 100644 --- a/crates/benilla-app/src/ui_unit.rs +++ b/crates/benilla-app/src/ui_unit.rs @@ -449,6 +449,35 @@ fn load_default_languages( } } +/// **The player's default language goes into the VM at its birth** (decision 2241, through the +/// seam 2240 established) — because `GetDefaultLanguage()` is read *inside* the load burst, and +/// until now the answer during that burst was `nil` and then a race. +/// +/// Two readers, both stock: `ChatEdit_OnLoad` takes it at OnLoad (dead in 1.12, but it is the +/// era's idiom), and `ChatFrame_OnEvent`'s `PLAYER_ENTERING_WORLD` arm stores it as +/// `this.defaultLanguage`, which gates the `[Common]`/`[Orcish]` prefix on every readable chat +/// line for the session. We fire `PLAYER_ENTERING_WORLD` from [`feed_units`] — an `Update` system +/// in the same set as [`feed_default_language`], with no ordering between them — so whether that +/// gate was seeded when the event arrived was Bevy's intra-set order to decide, per run. +/// +/// The race comes from the **roster row** rather than the object store, because the avatar does not +/// exist yet at this edge; it is the same value from the same login, and it is the row +/// [`crate::ui_script::seat_from_roster`] builds the player seat from a few lines earlier in the +/// same call. [`feed_default_language`] still runs and still owns the live answer — this only +/// makes sure the burst does not read a nil. +pub(crate) fn seed_default_language(world: &mut World, script: &mut UiScript) { + let (Some(langs), Some(roster)) = ( + world.get_resource::(), + world.get_resource::(), + ) else { + return; + }; + let Some(row) = roster.pending_row() else { + return; + }; + script.set_default_language(langs.0.name(u32::from(row.race), 0).map(str::to_string)); +} + /// Push `GetDefaultLanguage()`'s one string, on change only. /// /// The reference resolves it per call from the live player object; we resolve it once per race diff --git a/crates/benilla-formats/src/lib.rs b/crates/benilla-formats/src/lib.rs index 03f299ff..1e2b833c 100644 --- a/crates/benilla-formats/src/lib.rs +++ b/crates/benilla-formats/src/lib.rs @@ -196,8 +196,8 @@ mod spells; pub use spells::{ cc_exemption, grants_immunity, load_shapeshift_forms, load_spell_cast_times, load_spell_catalog, load_spell_dispel_types, load_spell_durations, load_spell_radii, - load_spell_ranges, min_max_range, substitute, CcExemption, FormRefusal, OpenLock, - ShapeshiftForm, SpellCastTime, SpellCastTimeCatalog, SpellCatalog, SpellDispelTypes, + load_spell_ranges, min_max_range, substitute, CcExemption, FormRefusal, LearnAnnouncement, + OpenLock, ShapeshiftForm, SpellCastTime, SpellCastTimeCatalog, SpellCatalog, SpellDispelTypes, SpellDisplay, SpellDuration, SpellDurationCatalog, SpellRadius, SpellRadiusCatalog, SpellRange, SpellRangeCatalog, TokenContext, ATTR_CASTABLE_WHILE_DEAD, ATTR_NOT_IN_COMBAT, ATTR_ONLY_STEALTHED, COMBAT_REACH_ADD, MELEE_RANGE_FLOOR, ON_NEXT_SWING_RANGE, diff --git a/crates/benilla-formats/src/spells/display.rs b/crates/benilla-formats/src/spells/display.rs index 2ad694fd..9760ab6e 100644 --- a/crates/benilla-formats/src/spells/display.rs +++ b/crates/benilla-formats/src/spells/display.rs @@ -854,6 +854,129 @@ impl SpellDisplay { None } } + /// The spell's name with its rank subtext appended **the way the client composes it** — + /// `"%s (%s)"` (the literal at `0x8468b0`, read out of the reference image) when [`Self::rank`] + /// is a non-empty string, and the bare name when it is not. + /// + /// Two surfaces build this string and both reach the same literal, so it is one method rather + /// than a copy each: the trainer window's prerequisite-ability list + /// (`GetTrainerServiceAbilityReq`, wow-re `system/ui/scratch/trainer-requirement.md`) and the + /// learn announcement's argText (`0x4b2963`'s empty-subtext test, then either + /// `0x4b2982 call 0x64a7f0` — `SStrPrintf(buf, 0x200, "%s (%s)", name, subtext)` — or + /// `0x4b29a0 call 0x64a5a0`, the plain copy). The format is a property of the spell record, + /// so it belongs with the record. + pub fn ranked_name(&self) -> String { + match self.rank.as_deref() { + Some(rank) if !rank.is_empty() => format!("{} ({})", self.name, rank), + _ => self.name.clone(), + } + } + + /// **Which line the client prints in chat when this spell is learned**, or `None` for one it + /// learns silently (decision 2243). + /// + /// The announcement is the tail of the spell-added registrar `0x4b25b0` — the same function + /// whose head sets the known-spell bit and whose gates [`Self::in_spellbook`] models — and it + /// runs only when the registrar's `edx` flag is set. That flag is **not** an "announce" flag, + /// though this is the leg benilla uses it for: at `0x4b2b4f` it gates the book re-sort + + /// `SPELLS_CHANGED`, `LEARNED_SPELL_IN_TAB` and tutorial trigger 40 atomically with the chat + /// line, which is why the login drain fires one batched `0x4b2fd0(1,0)` rather than one per + /// spell. It is the **live-mutation** flag; decision 2246 names the legs still unbuilt. That flag is the whole + /// reason logging in is silent while a trainer purchase is not: the `SMSG_INITIAL_SPELLS` + /// drain replays the book through `AddSpell` with it **clear** (`0x5deaa4 push 0x1; + /// 0x5deaa6 push 0x0` — arg3 is the flag `AddSpell` forwards as `edx` at `0x5e9c5c`), while + /// `SMSG_LEARNED_SPELL`'s handler passes `1` (`0x5e61c0` -> `AddSpell(id, slot, 1, 1)`) and + /// `SMSG_SUPERCEDED_SPELL`'s reaches the registrar through the supersede pair `0x4b2f50`, + /// which hardcodes `mov edx,0x1` at `0x4b2f61`. **So a rank-up announces too** — and the + /// unlearn half of that same pair (`0x4b2c50`) contains no `DisplayError` call at all, which + /// is why a rank-up prints one line and not two, and why `SMSG_REMOVED_SPELL` prints nothing. + /// + /// The three-way itself is `0x4b2909`, reading `Attributes` (`SpellRec+0x18`) and nothing + /// else: + /// + /// ```text + /// 0x4b290f test al,al / js ; 0x80 DO_NOT_DISPLAY -> no line at all + /// 0x4b2917 test al,0x20 / je ; 0x20 IS_TRADESKILL -> id 0x39 ERR_LEARN_RECIPE_S + /// 0x4b294a and eax,0x10 ; 0x10 ABILITY + /// 0x4b29a9 setne al / add eax,0x37 ; -> 0x37 ERR_LEARN_SPELL_S or 0x38 ERR_LEARN_ABILITY_S + /// ``` + /// + /// The recipe arm pushes the **bare** name (`0x4b292c`, `SpellRec+0x1e0`) and returns from the + /// function outright; the other two push [`Self::ranked_name`]. Every one of the three ids is + /// a `MsgKind::Chat` row carrying chat type `10` (`CHAT_MSG_SYSTEM`) in the message catalog, + /// so all three are chat lines, never `UIErrorsFrame` toasts. + /// + /// Note what the block does **not** read: the spell's power type, its school, its class, and + /// its `SPELL_ATTR_PASSIVE` bit are all irrelevant — a passive is announced like anything + /// else unless it also carries `DO_NOT_DISPLAY`, which in the shipped data it usually does. + /// Whether the client announces **unlearning** this spell — *"You have unlearned %s."* + /// (`ERR_SPELL_UNLEARNED_S`, message id `0x14a`), with the bare localized name and no rank + /// (decision 2246). + /// + /// A **different** gate set from [`Self::learn_announcement`]'s, in a different function, and + /// not guessable from it — which is how decision 2243 came to claim, with a byte citation, + /// that the reference never prints an unlearn line at all. It does. The claim came from + /// bounding `RemoveSpell 0x5e9fe0` at the `ret 0x8` at `0x5ea28f`; that `ret` is a *block* + /// end, and `0x5ea292` is a live branch target past it — the function really runs to + /// `0x5ea2ba`, and the announce is in the part 2243 never read. + /// + /// Four gates, all silent, taken in the binary's order: + /// + /// ```text + /// 0x5ea03a sete cl ; announce := (suppress == 0) — the caller's arg + /// 0x5ea03d test al,0x20 / je 0x5ea172 ; IS_TRADESKILL falls into the container path, + /// 0x5ea170 xor ecx,ecx ; whose join ZEROES the flag -> silent + /// 0x5ea17a jle 0x5ea292 ; castUI > 0 takes the castUI-container walk and + /// ; rejoins at 0x5ea245 -> silent (0x5ea292 has + /// ; exactly ONE entry, this one) + /// 0x5ea29b js 0x5ea248 ; DO_NOT_DISPLAY -> silent + /// 0x5ea2ab push 0x14a / call 0x496720 ; else the line, with SpellRec+0x1e0 (name only) + /// ``` + /// + /// `suppress` is the caller's, not the record's, so it is not modelled here: benilla's only + /// producer is `SMSG_REMOVED_SPELL` (`0x5e43e3`, `suppress = 0`), and the rank-up path that + /// passes `1` (`0x5e6392`) does not reach this function on our side at all. + /// + /// Note `castUI` gates the *unlearn* line and **not** the learn line — the learn block tests + /// it only afterwards, at `0x4b29bf`, to decide the book slot. A spell with `castUI > 0` + /// therefore announces when it is learned and says nothing when it is taken away. That + /// asymmetry is the reference's, not an oversight here. + pub fn announces_unlearn(&self) -> bool { + self.attributes & SPELL_ATTR_IS_TRADESKILL == 0 + && self.cast_ui == 0 + && self.attributes & ATTR_DO_NOT_DISPLAY == 0 + } + + pub fn learn_announcement(&self) -> Option { + if self.attributes & ATTR_DO_NOT_DISPLAY != 0 { + return None; + } + if self.attributes & SPELL_ATTR_IS_TRADESKILL != 0 { + return Some(LearnAnnouncement::Recipe); + } + Some(if self.attributes & ATTR_ABILITY != 0 { + LearnAnnouncement::Ability + } else { + LearnAnnouncement::Spell + }) + } +} + +/// A [`SpellDisplay::learn_announcement`] verdict — which of the three `ERR_LEARN_*` chat lines +/// the client prints when the spell is learned. The key each one names, and whether the argText +/// carries the rank, is the caller's to map (`benilla::net::apply::spells`): the ids live in the +/// message catalog, which is a UI-layer table, not a `Spell.dbc` fact. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum LearnAnnouncement { + /// Message id `0x37` — `ERR_LEARN_SPELL_S`, "You have learned a new spell: %s." + Spell, + /// Message id `0x38` — `ERR_LEARN_ABILITY_S`, "You have learned a new ability: %s." + /// (`Attributes & 0x10`, `SPELL_ATTR_ABILITY`.) + Ability, + /// Message id `0x39` — `ERR_LEARN_RECIPE_S`, "You have learned how to create a new item: %s." + /// (`Attributes & 0x20`, `SPELL_ATTR_IS_TRADESKILL`.) Its argText is the **bare** name: the + /// reference's recipe arm never reaches the rank-subtext composer. + Recipe, } /// A [`SpellDisplay::form_refusal`] verdict — which cast-fail reason `0x612480` writes. @@ -905,4 +1028,107 @@ mod tests { d.targets = 0x100; assert!(!d.is_harmful(), "the ally flag short-circuits the walk"); } + + /// The learn announcement's three-way at `0x4b2909`, read off `Attributes` alone — and its + /// one silent case. The bit order is the binary's: `DO_NOT_DISPLAY` is tested first and wins + /// over a row that also declares `IS_TRADESKILL`, which in turn wins over `ABILITY`. + #[test] + fn learn_announcement_reads_the_three_attribute_bits_in_the_clients_order() { + let mut d = SpellDisplay::default(); + assert_eq!( + d.learn_announcement(), + Some(LearnAnnouncement::Spell), + "a plain row is a spell — Fireball 133 carries Attributes 0x10000" + ); + d.attributes = 0x10; + assert_eq!( + d.learn_announcement(), + Some(LearnAnnouncement::Ability), + "SPELL_ATTR_ABILITY picks the ability wording" + ); + d.attributes = 0x20; + assert_eq!( + d.learn_announcement(), + Some(LearnAnnouncement::Recipe), + "SPELL_ATTR_IS_TRADESKILL diverts to the recipe line" + ); + d.attributes = 0x20 | 0x10; + assert_eq!( + d.learn_announcement(), + Some(LearnAnnouncement::Recipe), + "the tradeskill branch is taken before the ability bit is ever read" + ); + d.attributes = 0x80; + assert_eq!( + d.learn_announcement(), + None, + "SPELL_ATTR_DO_NOT_DISPLAY is announced silently — every language and proficiency" + ); + d.attributes = 0x80 | 0x20; + assert_eq!( + d.learn_announcement(), + None, + "…and the sign test at 0x4b290f runs before the tradeskill test at 0x4b2917" + ); + d.attributes = 0x40; + assert_eq!( + d.learn_announcement(), + Some(LearnAnnouncement::Spell), + "PASSIVE is not read by the block at all — only DO_NOT_DISPLAY silences a spell" + ); + } + + /// The unlearn line's four gates (`0x5e9fe0`'s block at `0x5ea292`) — a different set from + /// the learn block's, in a different function. `castUI` is the one that is easy to miss: it + /// silences the unlearn and does nothing to the learn. + #[test] + fn announces_unlearn_is_not_the_mirror_of_the_learn_gates() { + let mut d = SpellDisplay::default(); + assert!(d.announces_unlearn(), "a plain row says it"); + d.attributes = 0x10; + assert!( + d.announces_unlearn(), + "ABILITY is a learn-wording bit and nothing to this path" + ); + d.attributes = 0x40; + assert!(d.announces_unlearn(), "PASSIVE is not read here either"); + d.attributes = 0x20; + assert!( + !d.announces_unlearn(), + "IS_TRADESKILL: 0x5ea170 zeroes the flag" + ); + d.attributes = 0x80; + assert!(!d.announces_unlearn(), "DO_NOT_DISPLAY: 0x5ea29b"); + d.attributes = 0; + d.cast_ui = 1; + assert!( + !d.announces_unlearn(), + "castUI > 0 never reaches 0x5ea292 — its single entry is 0x5ea17a jle" + ); + assert_eq!( + d.learn_announcement(), + Some(LearnAnnouncement::Spell), + "…while the SAME row still announces its learn: castUI is read after the message" + ); + } + + /// `"%s (%s)"` (`0x8468b0`) versus the bare copy, keyed on the rank subtext being a non-empty + /// string — the reference tests the first BYTE of the subtext (`0x4b2963 cmp BYTE PTR [eax],0x0`), + /// so a present-but-empty column is the bare name, not `"Name ()"`. + #[test] + fn ranked_name_appends_the_subtext_only_when_there_is_one() { + let mut d = SpellDisplay { + name: "Fireball".to_string(), + ..SpellDisplay::default() + }; + assert_eq!(d.ranked_name(), "Fireball", "no subtext, no parentheses"); + d.rank = Some(String::new()); + assert_eq!( + d.ranked_name(), + "Fireball", + "an empty subtext is no subtext" + ); + d.rank = Some("Rank 2".to_string()); + assert_eq!(d.ranked_name(), "Fireball (Rank 2)"); + } } diff --git a/crates/benilla-formats/src/spells/mod.rs b/crates/benilla-formats/src/spells/mod.rs index 7932a264..9e9ea791 100644 --- a/crates/benilla-formats/src/spells/mod.rs +++ b/crates/benilla-formats/src/spells/mod.rs @@ -122,7 +122,7 @@ mod tokens; pub use cast_times::{load_spell_cast_times, SpellCastTime, SpellCastTimeCatalog}; pub use dispel_types::{load_spell_dispel_types, SpellDispelTypes}; -pub use display::{FormRefusal, OpenLock, SpellDisplay}; +pub use display::{FormRefusal, LearnAnnouncement, OpenLock, SpellDisplay}; pub use duration::{load_spell_durations, SpellDuration, SpellDurationCatalog}; pub use forms::{load_shapeshift_forms, ShapeshiftForm}; mod immunity; @@ -426,6 +426,11 @@ const ATTR_EX2_DO_NOT_RESET_COMBAT_TIMERS: u32 = 0x20000; /// (decision 0216 §8, `benilla-ui/src/script/spellbook.rs`) refuses it outright rather than /// sending a doomed cast the server would just reject. const ATTR_PASSIVE: u32 = 0x40; +/// `Attributes` bit `0x10` — `SPELL_ATTR_ABILITY` (cmangos `SpellDefines.h`). The **only** thing +/// the 1.12 client reads it for is the learn announcement's wording: `0x4b29a9 setne al` / +/// `0x4b29b3 add eax,0x37` picks message id `0x37` `ERR_LEARN_SPELL_S` when the bit is clear and +/// `0x38` `ERR_LEARN_ABILITY_S` when it is set. See [`SpellDisplay::learn_announcement`]. +const ATTR_ABILITY: u32 = 0x10; /// `Attributes` bit `0x80` — `SPELL_ATTR_DO_NOT_DISPLAY` (cmangos `SpellDefines.h`: "Hidden in /// Spellbook, Aura Icon, Combat Log"): THE spellbook add-gate (decision 0227) AND the `Attributes` /// half of the aura-bar display filter ([`SpellDisplay::hidden_from_aura_bar`] — the cache diff --git a/crates/benilla-ui/src/script/trainer/mod.rs b/crates/benilla-ui/src/script/trainer/mod.rs index 45c441bd..abc3f3cc 100644 --- a/crates/benilla-ui/src/script/trainer/mod.rs +++ b/crates/benilla-ui/src/script/trainer/mod.rs @@ -607,6 +607,29 @@ fn selected_row(model: &Model) -> Option { /// getters were carved to share (`0x4d89b0`, bounded by the total); these two bindings' own gate was /// **not** carved, and the difference is unobservable through the stock window, which only ever /// passes a visible header's index or `0`. +/// Queue `TRAINER_UPDATE` — **the repaint the reference fires from the mask-commit thunk itself**, +/// not from Lua (decision 2244). +/// +/// `SetTrainerServiceTypeFilter`'s four legs all commit through `0x4d8c90`, whose whole body is +/// `mov ds:0xb73a1c,ecx; call 0x4d8410; mov ecx,0x136; jmp 0x703e50` — write the mask, re-run the +/// finalizer, **fire event `0x136` = `TRAINER_UPDATE`** (wow-re `system/ui/ledger.tsv`'s `0x4d8c90` +/// row; the id is `ui.md`'s own event table). Its siblings `0x4d8cb0` (the skill-line mask) and +/// `0x4d8cd0` (the expand mask, which is what Collapse/ExpandTrainerSkillLine commit) are recorded +/// there as the same shape. +/// +/// That matters because **nothing in the stock window repaints after a filter click**: +/// `ClassTrainerFrameFilterDropDown_OnClick` sets the saved global, calls the filter verb, and then +/// only does `ClassTrainerListScrollFrameScrollBar:SetValue(0)` — a no-op when the bar is already at +/// zero. The list is repainted by `ClassTrainerFrame_OnEvent`'s `TRAINER_UPDATE` arm, and by nothing +/// else. While our own `TrainerFrame.xml` ran (before 1957) its click handler repainted explicitly, +/// which is why this only became visible when the window went stock: the checkboxes moved and the +/// list underneath did not. +fn queue_trainer_update(model: &mut Model) { + model + .pending_events + .push(("TRAINER_UPDATE".to_string(), Vec::new())); +} + fn set_collapsed(model: &mut Model, id: usize, collapse: bool) { if id == 0 && !collapse { model.trainer_collapsed.clear(); @@ -913,6 +936,9 @@ pub(super) fn install(lua: &Lua) -> mlua::Result<()> { // Era passes 1 / 0 (or true/nil); anything truthy-but-not-0 enables. let enable = !matches!(on, Value::Nil | Value::Integer(0) | Value::Boolean(false)); model.trainer_filter[c.filter_slot()] = enable; + // …and the repaint, from here rather than from Lua ([`queue_trainer_update`]). + // Unconditional, like the thunk: it fires whether or not the bit moved. + queue_trainer_update(&mut model); } Ok(()) })?, @@ -925,6 +951,7 @@ pub(super) fn install(lua: &Lua) -> mlua::Result<()> { lua.create_function(|lua, id: usize| { let mut model = lua.app_data_mut::().expect("model app_data"); set_collapsed(&mut model, id, true); + queue_trainer_update(&mut model); Ok(()) })?, )?; @@ -933,6 +960,7 @@ pub(super) fn install(lua: &Lua) -> mlua::Result<()> { lua.create_function(|lua, id: usize| { let mut model = lua.app_data_mut::().expect("model app_data"); set_collapsed(&mut model, id, false); + queue_trainer_update(&mut model); Ok(()) })?, )?;