From 6bcf9379863e386391e1f23681a3a5f7cdfd839f Mon Sep 17 00:00:00 2001 From: grom Date: Thu, 5 Nov 2026 16:28:29 -0700 Subject: [PATCH] A second profession's opener updated the book instead of reopening it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit grom: "nothing happens when I click another tab." Advanced Trade Skill Window's side tabs cast the other profession's opener and rebuild on TRADE_SKILL_SHOW alone — its update arm is `UpdateCount = UpdateCount + 1` and nothing else — so the book swapped underneath it and nothing redrew. The open is `0x4fc910` and it carries three rules we had one of: * one event per *open*, not per change: a changed line while a book is up is a fresh TRADE_SKILL_SHOW (`0x4fc99f`), never an update. The update is a change *within* one open book — a filter, a fold, an item count. * the opener is the window's toggle (`0x4fc918`): casting the profession already open closes it, which is what ATSW2's `this.Name ~= ` guard exists to avoid tripping. * an empty rebuilt list signals nothing (`0x4fc997`, the count at `0xbde04c`): the line is open and no window appears. Written over the list about to be pushed rather than as the literal count read, because ours can be a frame late where the reference builds its list inside the open call — a book that arrives empty and fills next frame still fires exactly one show, on the frame it fills. `0x4f6000` is the Craft window's same function, keyed on the craft type at `0xbdcfb8` and with no empty guard; both of its rules land in `feed_craft` and `open_trade_skill` too, since CRAFT_SHOW is the arm ATSW2 takes for Enchanting. The filter reset on a line change was already right — `set_trade_skill` keys it on `trade_skill_last_line`, the `0xbde064` above. Verified end to end offline with $WOW_CAPTURE_SPELLS seating a real book: Blacksmithing open with 24 recipes, `ATSWFrameTab3:Click()`, and the window comes back title=Cooking line=Cooking rows=9 with the check moved to tab 3. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SgstFWtn1RxEJ5kVrpgYwn --- crates/benilla-app/src/ui_craft.rs | 14 ++ crates/benilla-app/src/ui_tradeskill.rs | 188 ++++++++++++++++++++++-- docs/ADDON_COMPAT.md | 139 ++++++++++++++++++ 3 files changed, 328 insertions(+), 13 deletions(-) diff --git a/crates/benilla-app/src/ui_craft.rs b/crates/benilla-app/src/ui_craft.rs index a668746f..bb0b0b31 100644 --- a/crates/benilla-app/src/ui_craft.rs +++ b/crates/benilla-app/src/ui_craft.rs @@ -238,11 +238,25 @@ fn feed_craft( .flat_map(|r| r.reagents.iter().map(|re| re.item)), ); } + // As on the trade-skill side (`ui_tradeskill::feed_trade_skill`): the open fires one event + // per *open* (`0x4f6040`), so a different craft type while a window is up is a fresh + // `CRAFT_SHOW`, not an update. `CRAFT_UPDATE` is a change within one open window. The craft + // open has no empty-list guard — `0x4f601c` signals unconditionally, where the skill line's + // tests its count first. + let type_changed = last.as_ref().map(|f| f.craft_type) != fresh.as_ref().map(|f| f.craft_type); match (&*last, &fresh) { (None, Some(f)) => { debug!("ui_craft: window opens — {} recipe(s)", f.recipes.len()); script.fire_event("CRAFT_SHOW", vec![]); } + (Some(_), Some(f)) if type_changed => { + debug!( + "ui_craft: window opens on craft type {} — {} recipe(s)", + f.craft_type, + f.recipes.len() + ); + script.fire_event("CRAFT_SHOW", vec![]); + } (Some(_), Some(_)) => script.fire_event("CRAFT_UPDATE", vec![]), (Some(_), None) => { debug!("ui_craft: window closes"); diff --git a/crates/benilla-app/src/ui_tradeskill.rs b/crates/benilla-app/src/ui_tradeskill.rs index 38bda86c..d668c263 100644 --- a/crates/benilla-app/src/ui_tradeskill.rs +++ b/crates/benilla-app/src/ui_tradeskill.rs @@ -145,17 +145,34 @@ fn open_trade_skill( .and_then(|s| s.catalog.get(spell_id)) .and_then(|d| u32::try_from(d.effect_misc_value[0]).ok()) .unwrap_or(0); + // The opener is the window's **toggle**: the open's own first test (`0x4fc918` for a + // skill line, `0x4f6000` for a craft) compares the request against what is open and + // closes on a match, firing `TRADE_SKILL_CLOSE` / `CRAFT_CLOSE` rather than reopening. + // Casting your own open profession a second time shuts the book. Advanced Trade Skill + // Window's side tab guards exactly this (`this.Name ~= ` before it casts), which + // is how the behaviour reads off an addon as well as off the binary. The craft side keys + // on the craft type, not the line, because `0xbdcfb8` is what it stores and compares. if craft_type != 0 { + if craft_open.craft_type == craft_type && craft_open.line.is_some() { + debug!("ui_tradeskill: opener {spell_id} re-cast on the open craft — closes"); + craft_open.line = None; + craft_open.craft_type = 0; + continue; + } debug!( "ui_tradeskill: opener {spell_id} opens the CraftFrame (line {line}, craft type {craft_type})" ); craft_open.line = Some(line); craft_open.craft_type = craft_type; } else { - debug!("ui_tradeskill: opener {spell_id} opens skill line {line}"); - if open.line != Some(line) { + if open.line == Some(line) { + debug!("ui_tradeskill: opener {spell_id} re-cast on open line {line} — closes"); + open.line = None; repeat.clear(); + continue; } + debug!("ui_tradeskill: opener {spell_id} opens skill line {line}"); + repeat.clear(); open.line = Some(line); } } @@ -318,6 +335,39 @@ fn resolve_recipe( }) } +/// Which event one feed step owes the VM, from the book it had against the book it now has. +/// +/// The open (`0x4fc910`) fires one event per **open**, not one per change: a different line while +/// a book is already open is a fresh `TRADE_SKILL_SHOW` (`0x4fc99f`), never an update. +/// `TRADE_SKILL_UPDATE` is for a change *within* one open book — a filter, a fold, an item count. +/// The client also withholds the show when the rebuilt list came back empty (`0x4fc997`, over the +/// count at `0xbde04c`): the line is open and no window appears. +/// +/// Advanced Trade Skill Window is the addon that tells the two apart. Its side tabs cast the other +/// profession's opener (`CastSpellByName(this.Name)`) and it rebuilds on `TRADE_SKILL_SHOW` alone +/// — its update arm increments a counter and does nothing else. With the update this used to send, +/// a tab click swapped the book underneath it and redrew nothing, which is what "I can't switch to +/// other professions" looked like. +/// +/// The emptiness test is written over the list we are about to push rather than as the reference's +/// literal count read, because ours can be a frame late where the reference builds its list inside +/// the open call: a book that arrives empty and fills next frame still fires exactly one show, on +/// the frame it fills. +fn book_event( + last: &Option, + fresh: &Option, +) -> Option<&'static str> { + let shown = |s: &Option| s.as_ref().is_some_and(|f| !f.recipes.is_empty()); + let line_of = |s: &Option| s.as_ref().map(|f| f.line); + if fresh.is_none() { + return last.is_some().then_some("TRADE_SKILL_CLOSE"); + } + if shown(fresh) && (!shown(last) || line_of(last) != line_of(fresh)) { + return Some("TRADE_SKILL_SHOW"); + } + last.is_some().then_some("TRADE_SKILL_UPDATE") +} + /// Build the book: the known recipes of the open line, banded against the current rank. fn feed_trade_skill( script: Option>, @@ -406,23 +456,29 @@ fn feed_trade_skill( std::iter::once(r.product_item).chain(r.reagents.iter().map(|re| re.item)) })); } - match (&*last, &fresh) { - (None, Some(f)) => { - debug!( - "ui_tradeskill: window opens — {} recipe(s), first group {:?}", - f.recipes.len(), - f.recipes.first().and_then(|r| r.group.clone()) - ); + match book_event(last, &fresh) { + Some("TRADE_SKILL_SHOW") => { + if let Some(f) = &fresh { + debug!( + "ui_tradeskill: window opens on line {} — {} recipe(s), first group {:?}", + f.line, + f.recipes.len(), + f.recipes.first().and_then(|r| r.group.clone()) + ); + } script.fire_event("TRADE_SKILL_SHOW", vec![]); } - (Some(_), Some(_)) => { - script.fire_event("TRADE_SKILL_UPDATE", vec![]); + Some("TRADE_SKILL_CLOSE") => { + debug!("ui_tradeskill: window closes"); + script.fire_event("TRADE_SKILL_CLOSE", vec![]); + } + Some(other) => { + script.fire_event(other, vec![]); if repeat_changed { script.fire_event("UPDATE_TRADESKILL_RECAST", vec![]); } } - (Some(_), None) => script.fire_event("TRADE_SKILL_CLOSE", vec![]), - (None, None) => {} + None => {} } *last = fresh; } @@ -579,6 +635,112 @@ mod tests { assert_eq!(recipe_icon(&d, None, &deps.items, &deps.commands), None); } + fn book(line: u32, recipes: usize) -> Option { + Some(TradeSkillState { + line, + line_name: format!("Line {line}"), + rank: 1, + max_rank: 75, + recipes: (0..recipes) + .map(|i| TradeSkillRecipe { + group: None, + spell_id: 3000 + i as u32, + name: format!("Recipe {i}"), + difficulty: TradeSkillDifficulty::Optimal, + num_available: 1, + icon: None, + min_made: 1, + max_made: 1, + cooldown_secs: None, + product_item: 0, + product_inv_type: 0, + product_item_level: 0, + reagents: Vec::new(), + tools: Vec::new(), + }) + .collect(), + repeat_count: 0, + }) + } + + /// The defect grom played: with the book open on one profession, clicking Advanced Trade + /// Skill Window's side tab for another casts that profession's opener, and the window used to + /// get `TRADE_SKILL_UPDATE` — which that addon counts and ignores. The reference signals an + /// open per open (`0x4fc99f`), so a changed line is a show. + #[test] + fn a_second_professions_opener_reopens_the_book_rather_than_updating_it() { + assert_eq!( + book_event(&book(164, 24), &book(185, 8)), + Some("TRADE_SKILL_SHOW"), + "Blacksmithing open, Cooking cast: a new book, so a new show", + ); + assert_eq!( + book_event(&None, &book(164, 24)), + Some("TRADE_SKILL_SHOW"), + "the first open", + ); + assert_eq!( + book_event(&book(164, 24), &book(164, 23)), + Some("TRADE_SKILL_UPDATE"), + "the same line with a changed list is a change within one open book", + ); + assert_eq!(book_event(&book(164, 24), &None), Some("TRADE_SKILL_CLOSE"),); + assert_eq!(book_event(&None, &None), None); + } + + /// `0x4fc997`: the show is signalled only when the rebuilt list came back non-empty — the + /// line is open either way. Ours can arrive a frame before the recipes do, so the show waits + /// for them rather than being skipped outright. + #[test] + fn an_empty_book_shows_nothing_until_it_fills() { + assert_eq!(book_event(&None, &book(164, 0)), None); + assert_eq!( + book_event(&book(164, 0), &book(164, 24)), + Some("TRADE_SKILL_SHOW"), + "one show, on the frame the list fills", + ); + assert_eq!( + book_event(&book(164, 24), &book(164, 24)), + Some("TRADE_SKILL_UPDATE"), + "and not a second one", + ); + } + + /// `0x4fc918`, the open's own first test: the opener is the window's toggle, so casting the + /// profession that is already open shuts it. Advanced Trade Skill Window's side tab guards + /// exactly this before it casts (`this.Name ~= `). + #[test] + fn the_open_professions_opener_closes_the_book() { + use bevy::ecs::system::RunSystemOnce; + + let mut app = App::new(); + app.insert_resource(SkillLines { + catalog: benilla_formats::SkillLineCatalog::from_spell_lines([(2018, 164)]), + }); + app.init_resource::() + .init_resource::() + .init_resource::() + .init_resource::(); + + app.world_mut() + .resource_mut::() + .0 + .push(2018); + app.world_mut().run_system_once(open_trade_skill).unwrap(); + assert_eq!(app.world().resource::().line, Some(164)); + + app.world_mut() + .resource_mut::() + .0 + .push(2018); + app.world_mut().run_system_once(open_trade_skill).unwrap(); + assert_eq!( + app.world().resource::().line, + None, + "the same opener a second time closes the book", + ); + } + /// Driven through the real handler registration. #[test] fn the_session_end_closes_the_book_and_drops_the_repeat() { diff --git a/docs/ADDON_COMPAT.md b/docs/ADDON_COMPAT.md index b0133d81..0fe852fa 100644 --- a/docs/ADDON_COMPAT.md +++ b/docs/ADDON_COMPAT.md @@ -4360,3 +4360,142 @@ His own session agrees: 11 `frame hitch:` lines in an hour, every one of them at largest being the loading screens, and **zero** live pipeline compiles. So the thing to pin down next is which "laggy" it is — a sustained cost, or motion trailing the input, which would be the zoom ease's 0.29 s half-life rather than a frame cost. + +## 2026-11-05, second session: every `` in the interface read text widths as 0 + +grom, with ATSW2 finally loading: *"okay atsw2 is loading now, but barely functioning."* Its sort +row was the visible part — `Sort by: ◉ Catego⟨Difficulty⟩ry ◯ Name` — and chasing that one +overlapping label landed on something that had nothing to do with the addon. + +`seat_text_measurer`'s own comment says why it exists: *"so `GetStringWidth` right after `SetText` +measures … seated before the manifest loads, **for every ``**"*. It was not being seated. +`UiFontAtlas` is inserted by an `Update` system through `Commands`, so it appears at the next +command flush — and `load_ingame_ui_on_world_entry` is an exclusive system on the `InWorld` state +transition, which comes **first**. `seat_raster_seam_for_load` took its `let Some(atlas) = … else +{ return }`, and the VM loaded the whole interface with no measurer behind +`measure::ensure_measured`. + +So for all of FrameXML and every addon, `FontString:GetWidth()`, `GetHeight()` and +`GetStringWidth()` answered **0** for a string that plainly had text. The reference measures inline +and always knows (`0x79e510` → `0x772890`). ATSW2's line 1642 is the case that showed: + +```lua +ATSWNameSortCheckbox:SetPoint('LEFT', ATSWDifficultySortCheckbox, 'RIGHT', + ATSWDifficultySortCheckboxText2:GetWidth()+5, 0) +``` + +which stored **5.0** instead of 53.8, and drew "Difficulty" across "Category". + +**Why it never looks like this.** Nothing errors, nothing logs, and by the time anything asks, the +widths are all correct — the measure is taken on the next read, after the load. The only reads that +are wrong are the ones taken *during* an ``, and what they leave behind is a stored +geometry nobody recomputes. The three probes that named it, in order, each cheap: + +1. Is it the geometry or the drawing? `GetPoint()` on the misplaced frame — a stored offset of 5 + where the label measures 48.8 says the arithmetic ran on a zero. +2. Re-run the addon's own `` from a capture eval (they are globals). Same code, same frame, + right answer ⇒ the difference is *when*, not *what*. +3. One `eprintln!` in `region::measured_wh` when a FontString with text answers 0, printing + `model.measurer.is_some()`. It printed `measurer=false`. + +`ensure_font_atlas(&mut World)` is now `init`'s body for a caller holding the world, and a world +that still cannot build one warns instead of loading the interface in silence. The check that it +stays fixed is a walk of `UIParent` in a live capture counting FontStrings that have text and +measure 0: dozens before, **0** after. + +The class to watch: **a resource another system inserts through `Commands` is not there for an +exclusive system running on a state transition.** Anything the UI load needs must be built by the +load, not awaited. + +### The map's glide is a setting, because two reports bracketed one guess + +The same constant was reported from both sides — "slippery" on the 4th, *"its the maps motion that +is laggy"* on the 5th — which is the signal to stop guessing and hand it over. `mapMotionSmoothing` +(Options → Interface, 0–100 in tens, 0 reads "Off") scales the one half-life both the pan and the +zoom ease on, `7 / (pct/100 × 0.30)`. Measured frames for one wheel notch to settle at 60 fps: + +| 0% | 10% | **25% (ships)** | 50% | 100% | +|---|---|---|---|---| +| 1 | 17 | **42 (0.70 s)** | 84 | 167 | + +What he had played was 167. The top of the slider is still that, deliberately — it is the slowest +anyone could want — and the default is a quarter of it. One rate drives both axes because a zoom +that settles while the pan is still drifting reads as the pan being broken. A drag still writes the +scroll directly and never eases. + +### `$WOW_CAPTURE_SPELLS`: seat a real spellbook in a capture + +A capture never logs in, so its spellbook is whatever a fixture hardcodes — and a whole class of +addon is driven by what is *in* that book. ATSW2 finds the player's professions by walking tab 1 +and matching each spell's icon against its own thirteen-leaf `Professions` list, so with no +profession spell seated it finds nothing, draws no tab and switches nothing, which is +indistinguishable from the defect being chased. `$WOW_CAPTURE_SPELLS=[,…]` adds spells to +whatever book the fixture seats, rather than replacing it. The realm's own ids are in the comment +beside it: Survival 46050, Jewelcrafting 30220, and 1.12's Alchemy 2259, Blacksmithing 2018, +Cooking 2550, Enchanting 7411, Engineering 4036, First Aid 3273, Leatherworking 2108, Tailoring +3908, Smelting 2656. Seat a handful of **recipe** ids with the opener and the window comes up with +a real list, which is the only way to photograph one of these addons offline. + +## A second profession's opener updated the book instead of reopening it + +grom: *"nothing happens when I click another tab."* Advanced Trade Skill Window's side tabs are the +whole reason it exists, and their `` is two lines: + +```lua +if this.Name ~= ATSW_Profession[ATSW_realm][ATSW_player] then + CastSpellByName(this.Name) +``` + +Everything up to the cast was right — the tab scan builds five correct tabs with their names and +icons, the saved profession key resolves, the opener resolves through the book, and +`send_spell_cast` intercepts it as a profession opener (`0x6e4bce`) and sets the open skill line. +What was wrong was the **event**. `feed_trade_skill` fired one event per *change*: a first open was +`TRADE_SKILL_SHOW` and everything after it `TRADE_SKILL_UPDATE`. ATSW2's update arm is +`UpdateCount = UpdateCount + 1` and nothing else; it rebuilds on `TRADE_SKILL_SHOW` alone, and its +show arm is explicitly written for the window **already being up**: + +```lua +elseif event == 'TRADE_SKILL_SHOW' then + if UIParent:IsVisible() then + if ATSWFrame:IsVisible() or ATSWCSFrame:IsVisible() then … CloseCraft() … end +``` + +so the book swapped underneath it and nothing redrew. + +The open is `0x4fc910`, and it reads cleanly: + +``` +4fc913 call 0x4fc890 ; drop the old list +4fc918 cmp esi, [0xbde040] ; the line already open +4fc91e jne 0x4fc935 +4fc92a mov ecx, 0x13b ; → TRADE_SKILL_CLOSE, and return +4fc94c cmp [0xbde064], esi ; the last line shown +4fc95a or eax,-1 … [0x84dd60/64/68] ; → different: reset the three filters +4fc985 call 0x4fca20 ; rebuild +4fc997 jbe 0x4fc9a4 ; the rebuilt count at [0xbde04c] is 0 → no event +4fc999 mov ecx, 0x139 ; → TRADE_SKILL_SHOW +``` + +Three rules, all of them ours now, in `book_event`: + +- **one event per open, not per change** — a changed line while a book is up is a fresh + `TRADE_SKILL_SHOW`; `TRADE_SKILL_UPDATE` is a change *within* one open book (a filter, a fold, an + item count); +- **the opener is the window's toggle** — casting the profession already open closes it, which is + exactly what ATSW2's `this.Name ~=` guard is there to avoid tripping; +- **an empty rebuilt list signals nothing** — the line is open, no window appears. Written over the + list about to be pushed rather than as the literal count read, because ours can be a frame late + where the reference builds its list inside the open call: a book that arrives empty and fills next + frame still fires exactly one show, on the frame it fills. + +`0x4f6000` is the Craft window's same function, keyed on the craft type at `0xbdcfb8` instead of a +skill line and with no empty guard; both of its rules are in `feed_craft` and `open_trade_skill` +too, since `CRAFT_SHOW` is the arm ATSW2 uses for Enchanting. + +The filter reset on a line change was already right — `set_trade_skill` keys it on +`trade_skill_last_line`, the `0xbde064` above. + +Verified end to end offline, which `$WOW_CAPTURE_SPELLS` is what made possible: Blacksmithing open +with 24 recipes, `ATSWFrameTab3:Click()`, and the window comes back `title=Cooking line=Cooking +rows=9` with the check moved to tab 3. Before the fix the same click left the title on +Blacksmithing.