From 7ebae975d9517ef4af4f297321b3ff93da0c9697 Mon Sep 17 00:00:00 2001 From: grom Date: Thu, 29 Oct 2026 09:24:00 -0600 Subject: [PATCH] A recipe never names the spell it teaches, so "Already known" never fired MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit He is still told he can learn recipes he already knows. The check was there; it was written against a shape no recipe in the game has. Read off his own item cache, the realm's Plans: Copper Chain Vest (3609) carries exactly one spell: 3349, at trigger 0. The rule looked for trigger 6 carrying the taught spell. Neither half matches — and 3349 is not the craft spell either, it is a learn *wrapper* whose single SPELL_EFFECT_LEARN_SPELL teaches 3321. Nothing in the item record names 3321 at all, so no check that reads only the item can ever answer this question. The app now makes the hop while it has the catalog in hand, through SpellCatalog::learned_spell, and hands the result over as ItemTemplateView::teaches. The tooltip reds ITEM_SPELL_KNOWN when any of those is in the known-spell set. The trigger-6 arm stays for an item that really does carry its taught spell. The old test passed throughout, which is the part worth keeping in mind: its fixture was invented — trigger 6, spell 3330 — and 3330 is Silvered Bronze Shoulders, nothing to do with this item. It now uses the realm's own record, and a second test pins the 3349 -> 3321 hop against the shipped Spell.dbc so the fixture can never drift from the data again. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SgstFWtn1RxEJ5kVrpgYwn --- crates/benilla-app/src/ui_items/feed.rs | 35 +++++++++++++++++++ crates/benilla-ui/src/script/item_stats.rs | 11 ++++++ .../src/script/tests/tooltip_item/law.rs | 17 ++++++--- .../src/script/tooltip_item/render.rs | 12 ++++--- 4 files changed, 66 insertions(+), 9 deletions(-) diff --git a/crates/benilla-app/src/ui_items/feed.rs b/crates/benilla-app/src/ui_items/feed.rs index 5afc758b..359e8eb0 100644 --- a/crates/benilla-app/src/ui_items/feed.rs +++ b/crates/benilla-app/src/ui_items/feed.rs @@ -213,6 +213,14 @@ fn template_view( .filter(|s| s.spell_id != 0) .filter_map(|s| spell_text(s.spell_id).map(|n| (s.trigger, s.spell_id, n))) .collect(), + // Each of the item's spells hopped through `SPELL_EFFECT_LEARN_SPELL`, which is the only + // place the taught spell is written down — see `ItemTemplateView::teaches`. + teaches: t + .spells + .iter() + .filter(|s| s.spell_id != 0) + .filter_map(|s| spells.and_then(|c| c.catalog.learned_spell(s.spell_id))) + .collect(), charges: charges_count(&t.spells), description: t.description.clone(), page_text: t.page_text, @@ -1876,6 +1884,33 @@ mod tests { ); } + /// A recipe item's taught spell, resolved off the **real** `Spell.dbc`. + /// + /// The realm's Plans: Copper Chain Vest (3609) carries one spell, 3349, at trigger 0 — read + /// out of grom's own item cache, not invented. 3349 is a learn wrapper: its single + /// `SPELL_EFFECT_LEARN_SPELL` teaches 3321, the craft spell. Nothing in the item record names + /// 3321, so a known-recipe check written against the item's own spell id can never fire, and + /// that is why he kept being told he could learn recipes he already knew. + /// + /// Pinned here on the shipped DBC rather than a fixture, because the whole defect was a + /// fixture that asserted a shape the data does not have. + #[test] + fn a_recipe_items_taught_spell_is_the_learn_wrappers_on_real_data() { + let data = benilla_formats::wow_data_or_skip!(); + let mut chain = benilla_formats::open_chain(&data).expect("open chain"); + let catalog = benilla_formats::load_spell_catalog(&mut chain).expect("Spell.dbc"); + assert_eq!( + catalog.learned_spell(3349), + Some(3321), + "spell 3349, the item's only spell, is the wrapper that teaches the craft spell" + ); + assert_eq!( + catalog.learned_spell(3321), + None, + "and the craft spell teaches nothing — the hop is one step, not a chain" + ); + } + /// On the real Spell.dbc, an undescribed spell such as a key's `Opening` builds no trigger /// line (`0x52da29`-`0x52da31`), and a described one does. #[test] diff --git a/crates/benilla-ui/src/script/item_stats.rs b/crates/benilla-ui/src/script/item_stats.rs index 36c4894f..6222523e 100644 --- a/crates/benilla-ui/src/script/item_stats.rs +++ b/crates/benilla-ui/src/script/item_stats.rs @@ -85,6 +85,17 @@ pub struct ItemTemplateView { /// Green trigger-spell lines `(trigger, spell id, text)` in wire order: 0 and 5 "Use:", /// 1 "Equip:", 2 "Chance on hit:", 6 a taught spell (no line, only the "Already known" red). pub spell_triggers: Vec<(u32, u32, String)>, + /// The spells this item **teaches**, resolved app-side through `Spell.dbc`'s + /// `SPELL_EFFECT_LEARN_SPELL` (`SpellCatalog::learned_spell`). Knowing any of them is what + /// puts the red `ITEM_SPELL_KNOWN` on the plate. + /// + /// It has to be resolved rather than read off the trigger, because a recipe item does not + /// carry the craft spell at all: the realm's Plans: Copper Chain Vest (3609) carries exactly + /// one spell, **3349 at trigger 0**, and 3349 is a learn *wrapper* whose one effect teaches + /// 3321. A rule written against trigger 6 and the item's own spell id — which is what this + /// was — matches no recipe in the game, and grom kept being told he could learn recipes he + /// already knew. + pub teaches: Vec, pub lock_id: u32, /// "N Charge(s)" for the first spell slot past the builder's gate (`0x52db51`: 0 and -1 /// print nothing, else the absolute value); 0 prints no line. diff --git a/crates/benilla-ui/src/script/tests/tooltip_item/law.rs b/crates/benilla-ui/src/script/tests/tooltip_item/law.rs index b827c781..95d39ed5 100644 --- a/crates/benilla-ui/src/script/tests/tooltip_item/law.rs +++ b/crates/benilla-ui/src/script/tests/tooltip_item/law.rs @@ -120,6 +120,13 @@ fn red_lines_track_player_state() { /// window and not the spellbook — so a check that read `spellbook.slots` answered "unknown" for /// every recipe in the game. The spellbook is left deliberately **empty** here: that is the real /// shape, and it is what makes this test fail if the check goes back to reading it. +/// +/// **The fixture is the realm's own record, and that matters.** This test first shipped with a +/// made-up one — trigger 6 carrying the craft spell — and passed while he went on being told he +/// could learn recipes he already knew, because no recipe in the game has that shape. Item 3609 +/// as the realm sends it carries exactly one spell: **3349 at trigger 0**, and 3349 is a learn +/// *wrapper* whose single `SPELL_EFFECT_LEARN_SPELL` teaches 3321 (both read out of the shipped +/// `Spell.dbc`). The app resolves that hop into `ItemTemplateView::teaches`. #[test] fn a_known_recipe_reds_already_known_with_an_empty_spellbook() { let mut s = script(); @@ -128,9 +135,11 @@ fn a_known_recipe_reds_already_known_with_an_empty_spellbook() { 3609, ItemTemplateView { name: "Plans: Copper Chain Vest".into(), - quality: 1, - // Trigger 6 is "use: teaches you"; 3330 is the recipe's trade-skill spell. - spell_triggers: vec![(6, 3330, "Copper Chain Vest".into())], + quality: 2, + // The realm's record: one spell, trigger 0 ("Use:"), the learn wrapper 3349 … + spell_triggers: vec![(0, 3349, "Teaches you Copper Chain Vest.".into())], + // … which Spell.dbc says teaches 3321, the craft spell he actually knows. + teaches: vec![3321], ..Default::default() }, ); @@ -161,7 +170,7 @@ fn a_known_recipe_reds_already_known_with_an_empty_spellbook() { // Learned. The spellbook stays empty, as it does in a real session. s.set_player_req_state(PlayerReqState { - known_spells: std::collections::HashSet::from([3330]), + known_spells: std::collections::HashSet::from([3321]), ..Default::default() }); show(&mut s); diff --git a/crates/benilla-ui/src/script/tooltip_item/render.rs b/crates/benilla-ui/src/script/tooltip_item/render.rs index e5c6c644..f64d57f2 100644 --- a/crates/benilla-ui/src/script/tooltip_item/render.rs +++ b/crates/benilla-ui/src/script/tooltip_item/render.rs @@ -108,11 +108,13 @@ pub(super) fn render_view( // `SPELL_ATTR_IS_TRADESKILL` and so is never in the book (`PlayerReqState::known_spells`). let knows = |id: u32| model.player_req.known_spells.contains(&id); let known = v.required_spell != 0 && knows(v.required_spell); - // A taught spell (trigger 6, learn) the player knows: the always-red ITEM_SPELL_KNOWN. - let taught = v - .spell_triggers - .iter() - .any(|&(t, id, _)| t == 6 && id != 0 && knows(id)); + // A taught spell the player knows: the always-red ITEM_SPELL_KNOWN. Two sources, because + // the trigger alone does not name it — see `ItemTemplateView::teaches`. The trigger-6 arm + // stays for an item that really does carry the taught spell directly. + let taught = v.teaches.iter().any(|&id| id != 0 && knows(id)) + || v.spell_triggers + .iter() + .any(|&(t, id, _)| t == 6 && id != 0 && knows(id)); // The set view (a miss records the ask) and the equipped ids the set block counts. let set_view = (v.item_set != 0) .then(|| {