The trainer's ability icon shows its spell tooltip on the first hover (#158)

- Fix trainer tooltip on first hover
- The spell-tooltip feed keeps its in-game gate, and a test holds it after the trainer feed

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Sam <samuv.eth@gmail.com>

crates/benilla-app/src/test_support.rs

@@ -1,4 +1,5 @@
-//! Source readers shared by the crate's structural tests, which check this crate's own code.
+//! What the crate's structural tests share: source readers over its own code, and a check of
+//! the order its `Update` schedule builds.
 
 /// Every `.rs` file under `root`, recursively.
 pub(crate) fn rust_files(root: &std::path::Path) -> Vec<std::path::PathBuf> {
@@ -98,6 +99,62 @@ fn matching(text: &str, open: usize, lhs: u8, rhs: u8) -> Option<usize> {
     None
 }
 
+/// `a` runs before `b` in `app`'s built `Update`, by system identity: a `.after()` on a system
+/// missing from the schedule is silently nothing, and debug names may be off. The dependency graph
+/// is walked with the set hierarchy unfolded, so a set `a` sits in precedes what that set
+/// precedes, and a successor set brings its members.
+pub(crate) fn runs_before<MA, MB>(
+    app: &mut bevy::app::App,
+    a: impl bevy::ecs::system::IntoSystem<(), (), MA>,
+    b: impl bevy::ecs::system::IntoSystem<(), (), MB>,
+) -> bool {
+    use bevy::ecs::schedule::graph::Direction::{Incoming, Outgoing};
+    use bevy::ecs::schedule::NodeId;
+    use bevy::ecs::system::{IntoSystem, System};
+    use std::any::TypeId;
+    use std::collections::HashSet;
+
+    let a = System::type_id(&IntoSystem::into_system(a));
+    let b = System::type_id(&IntoSystem::into_system(b));
+    // `schedule_scope`, not `resource_scope::<Schedules>`: initializing the schedule inserts
+    // `Schedules`, which a resource scope refuses.
+    app.world_mut()
+        .schedule_scope(bevy::app::Update, |world, schedule| {
+            schedule
+                .initialize(world)
+                .expect("the Update schedule builds");
+            let key = |id: TypeId| -> NodeId {
+                schedule
+                    .systems()
+                    .expect("initialized")
+                    .find(|&(_, s)| System::type_id(&**s) == id)
+                    .map(|(k, _)| NodeId::System(k))
+                    .expect("the system is in Update")
+            };
+            let (a, b) = (key(a), key(b));
+            let dep = schedule.graph().dependency().graph();
+            let hier = schedule.graph().hierarchy().graph();
+            let (mut after, mut containers) = (HashSet::new(), HashSet::new());
+            let mut work = vec![(a, false)];
+            while let Some((n, is_after)) = work.pop() {
+                let fresh = if is_after {
+                    after.insert(n)
+                } else {
+                    containers.insert(n)
+                };
+                if !fresh {
+                    continue;
+                }
+                work.extend(dep.neighbors_directed(n, Outgoing).map(|m| (m, true)));
+                work.extend(hier.neighbors_directed(n, Incoming).map(|p| (p, false)));
+                if is_after {
+                    work.extend(hier.neighbors_directed(n, Outgoing).map(|c| (c, true)));
+                }
+            }
+            after.contains(&b)
+        })
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;

crates/benilla-app/src/ui_session.rs

@@ -437,70 +437,22 @@ mod tests {
         );
     }
 
-    /// Checked in the built schedule graph, by system identity: a `.after()` on a system missing
-    /// from the schedule is silently nothing, and debug names may be off.
     #[test]
     fn the_interact_npc_writer_is_seated_between_the_net_apply_and_the_facing_chain() {
-        use bevy::ecs::schedule::graph::Direction::{Incoming, Outgoing};
-        use bevy::ecs::schedule::{NodeId, Schedule};
-        use bevy::ecs::system::{IntoSystem, System};
-        use std::any::TypeId;
-        use std::collections::HashSet;
-
-        /// `a` runs before `b`: the dependency graph walked with the set hierarchy unfolded, so a
-        /// set `a` sits in precedes what that set precedes, and a successor set brings its members.
-        fn runs_before(schedule: &Schedule, a: NodeId, b: NodeId) -> bool {
-            let dep = schedule.graph().dependency().graph();
-            let hier = schedule.graph().hierarchy().graph();
-            let (mut after, mut containers) = (HashSet::new(), HashSet::new());
-            let mut work = vec![(a, false)];
-            while let Some((n, is_after)) = work.pop() {
-                let fresh = if is_after {
-                    after.insert(n)
-                } else {
-                    containers.insert(n)
-                };
-                if !fresh {
-                    continue;
-                }
-                work.extend(dep.neighbors_directed(n, Outgoing).map(|m| (m, true)));
-                work.extend(hier.neighbors_directed(n, Incoming).map(|p| (p, false)));
-                if is_after {
-                    work.extend(hier.neighbors_directed(n, Outgoing).map(|c| (c, true)));
-                }
-            }
-            after.contains(&b)
-        }
-
-        let apply = System::type_id(&IntoSystem::into_system(crate::net::apply_net_updates));
-        let writer = System::type_id(&IntoSystem::into_system(feed_interact_npc));
-        let facing = System::type_id(&IntoSystem::into_system(crate::net::drive_display_facing));
-
+        use crate::test_support::runs_before;
         let mut app = App::new();
         app.add_plugins((crate::net::NetPlugin { connect: false }, UiSessionPlugin));
-        // `schedule_scope`, not `resource_scope::<Schedules>`: initializing the schedule inserts
-        // `Schedules`, which a resource scope refuses.
-        app.world_mut().schedule_scope(Update, |world, schedule| {
-            schedule
-                .initialize(world)
-                .expect("the Update schedule builds");
-            let key = |id: TypeId| -> NodeId {
-                schedule
-                    .systems()
-                    .expect("initialized")
-                    .find(|&(_, s)| System::type_id(&**s) == id)
-                    .map(|(k, _)| NodeId::System(k))
-                    .expect("the system is in Update")
-            };
-            let (apply, writer, facing) = (key(apply), key(writer), key(facing));
-            assert!(
-                runs_before(schedule, apply, writer),
-                "feed_interact_npc must run after apply_net_updates"
-            );
-            assert!(
-                runs_before(schedule, writer, facing),
-                "drive_display_facing must run after feed_interact_npc"
-            );
-        });
+        assert!(
+            runs_before(&mut app, crate::net::apply_net_updates, feed_interact_npc),
+            "feed_interact_npc must run after apply_net_updates"
+        );
+        assert!(
+            runs_before(
+                &mut app,
+                feed_interact_npc,
+                crate::net::drive_display_facing
+            ),
+            "drive_display_facing must run after feed_interact_npc"
+        );
     }
 }

crates/benilla-app/src/ui_tooltip/mod.rs

@@ -16,6 +16,8 @@ use crate::target::{
     go_is_nearest, ring_reaction, Hovered, HoveredObject, GO_FLAG_LOCKED, GO_TYPE_GENERIC,
 };
 use crate::ui_action::{PlayerActions, Spells};
+use crate::ui_script::UiFeed;
+use crate::ui_trainer::{TrainerFeed, TrainerTooltipSubjects};
 use crate::ui_unit::{enrich_unit, snapshot, UnitFeed};
 
 pub struct UiTooltipPlugin;
@@ -26,7 +28,12 @@ impl Plugin for UiTooltipPlugin {
             Update,
             (
                 drive_mouseover_tooltip.in_set(UnitFeed),
-                feed_spell_tooltips.in_set(UnitFeed),
+                // After the trainer feed, so a list that lands this frame is hoverable in its tick;
+                // outside `UnitFeed`, which the trainer feed follows, so it takes that set's gate.
+                feed_spell_tooltips
+                    .in_set(UiFeed)
+                    .after(TrainerFeed)
+                    .run_if(crate::ui_script::ingame_ui_up),
             ),
         );
     }
@@ -317,13 +324,21 @@ struct SpellFeedMemory {
     reagents: std::collections::BTreeMap<u32, (u32, bool)>,
 }
 
-/// Push a view for every spell the UI can hover (the book, the class's talent ranks, the auras)
-/// before it is hovered, as the reference reads them all locally; an ask for any other id too.
+/// The hoverable spell sources [`feed_spell_tooltips`] reads, one parameter under Bevy's 16.
+#[derive(bevy::ecs::system::SystemParam)]
+struct SpellTooltipSources<'w> {
+    spells: Option<Res<'w, Spells>>,
+    trainer_subjects: Option<Res<'w, TrainerTooltipSubjects>>,
+    talents: Option<Res<'w, crate::ui_talent::Talents>>,
+}
+
+/// Push a view for every spell the UI can hover (the book, the class's talent ranks, the open
+/// trainer's services, the auras) before it is hovered, as the reference reads them all locally;
+/// an ask for any other id too.
 fn feed_spell_tooltips(
     script: Option<NonSendMut<UiScript>>,
     actions: Option<Res<PlayerActions>>,
-    spells: Option<Res<Spells>>,
-    talents: Option<Res<crate::ui_talent::Talents>>,
+    spell_sources: SpellTooltipSources,
     auras: Option<Res<crate::ui_aura::PlayerAuraCache>>,
     selection: Res<crate::target::Selection>,
     stores: Query<&ObjectStore>,
@@ -343,6 +358,11 @@ fn feed_spell_tooltips(
     mut memory: Local<crate::ui_script::VmMemo<SpellFeedMemory>>,
 ) {
     let (sub_classes, spell_mods) = &lookups;
+    let SpellTooltipSources {
+        spells,
+        trainer_subjects,
+        talents,
+    } = spell_sources;
     let Some(mut script) = script else {
         return;
     };
@@ -360,6 +380,16 @@ fn feed_spell_tooltips(
                 .filter(|s| !memory.pushed.contains(s)),
         );
     }
+    // The open trainer's services, which the detail icon's `SetTrainerService` renders.
+    if let Some(trainer) = trainer_subjects.as_deref() {
+        wanted.extend(
+            trainer
+                .0
+                .iter()
+                .copied()
+                .filter(|s| !memory.pushed.contains(s)),
+        );
+    }
     // Every rank of the class's talents: a talent tooltip reads the current and the next rank.
     if let (Some(talents), Ok(store)) = (talents.as_deref(), self_q.single()) {
         let race = store.0.unit_race().unwrap_or(0);

crates/benilla-app/src/ui_tooltip/tests.rs

@@ -1,4 +1,4 @@
-//! The spell-view cell tests, against the real 5875 data.
+//! The spell-view cell tests, against the real 5875 data, and the feed's place in the schedule.
 
 use super::*;
 use crate::ui_action::Spells;
@@ -505,3 +505,15 @@ fn the_locked_line_greens_when_the_lock_can_be_opened() {
     );
     assert_eq!(locked_line_tint(None), TooltipTint::LockOpen);
 }
+
+/// A trainer list that lands this frame is hoverable in this frame's tick: the trainer feed derives
+/// the subjects the spell feed pushes.
+#[test]
+fn the_spell_feed_runs_after_the_trainer_feed() {
+    let mut app = crate::game_plugins::schedule_tests::headless_client();
+    assert!(crate::test_support::runs_before(
+        &mut app,
+        crate::ui_trainer::feed_trainer,
+        feed_spell_tooltips
+    ));
+}

crates/benilla-app/src/ui_trainer/mod.rs

@@ -11,7 +11,7 @@ use bevy::prelude::*;
 
 use benilla_ui::script::{
     ScriptValue, TrainerAbilityReq, TrainerService, TrainerServiceCategory, TrainerSkillReq,
-    TrainerState, UiScript,
+    TrainerState, TrainerTooltip, UiScript,
 };
 
 use crate::entities::ItemDisplays;
@@ -99,6 +99,14 @@ impl TrainerOpen {
 #[derive(Resource, Default)]
 pub(crate) struct TrainerErrors(pub Vec<u32>);
 
+/// The spells the open trainer's services show as tooltips, which the spell-tooltip feed pushes.
+#[derive(Resource, Default)]
+pub(crate) struct TrainerTooltipSubjects(pub(crate) Vec<u32>);
+
+/// The trainer feed, which the spell-tooltip feed runs after.
+#[derive(SystemSet, Debug, Clone, Copy, PartialEq, Eq, Hash)]
+pub(crate) struct TrainerFeed;
+
 mod net;
 mod reeval;
 
@@ -109,6 +117,7 @@ impl Plugin for UiTrainerPlugin {
         net::register(app);
         app.init_resource::<TrainerOpen>()
             .init_resource::<TrainerErrors>()
+            .init_resource::<TrainerTooltipSubjects>()
             .add_systems(
                 Update,
                 (
@@ -116,7 +125,10 @@ impl Plugin for UiTrainerPlugin {
                     close_npc_session_out_of_range::<TrainerOpen>.before(feed_trainer),
                     // The unit feeds also write the pet bar the re-evaluator reads; either order
                     // reads the same pet spellbook, but the schedule check wants one declared.
-                    feed_trainer.in_set(UiFeed).after(crate::ui_unit::UnitFeed),
+                    feed_trainer
+                        .in_set(UiFeed)
+                        .in_set(TrainerFeed)
+                        .after(crate::ui_unit::UnitFeed),
                     drain_trainer.after(UiInput),
                 ),
             );
@@ -264,7 +276,7 @@ fn snapshot(
 /// The re-evaluator's inputs and its descriptor triggers: the player's descriptor and its field
 /// edges (money, level, a skill slot), the pet bar, and the stores that resolve the pet.
 #[derive(SystemParam)]
-struct ReEvalInputs<'w, 's> {
+pub(crate) struct ReEvalInputs<'w, 's> {
     self_player: Query<'w, 's, (Entity, &'static ObjectStore), With<SelfPlayer>>,
     stores: Query<'w, 's, &'static ObjectStore>,
     index: Res<'w, GuidIndex>,
@@ -275,7 +287,7 @@ struct ReEvalInputs<'w, 's> {
 /// Push the current trainer into the VM and fire its events on a change. Another trainer while
 /// open is a close then an open: `ShowUIPanel` returns early on a visible frame.
 #[allow(clippy::too_many_arguments)] // one Bevy system's full input set
-fn feed_trainer(
+pub(crate) fn feed_trainer(
     script: Option<NonSendMut<UiScript>>,
     // Writes the re-derived states, as `0x4d7d40` overwrites its own records.
     mut open: ResMut<TrainerOpen>,
@@ -289,6 +301,7 @@ fn feed_trainer(
     mut errors: ResMut<TrainerErrors>,
     commands: Res<NetCommands>,
     names: Res<NameCache>,
+    mut tooltip_subjects: ResMut<TrainerTooltipSubjects>,
     mut last: Local<crate::ui_script::VmMemo<Option<TrainerState>>>,
     mut last_trainer: Local<crate::ui_script::VmMemo<Option<u64>>>,
     mut last_name: Local<crate::ui_script::VmMemo<Option<String>>>,
@@ -416,6 +429,20 @@ fn feed_trainer(
                 .filter(|t| !t.is_empty())
         },
     );
+    // A spell subject's view must be in the store before the detail icon is hovered.
+    let mut fresh_tooltip_subjects: Vec<u32> = fresh
+        .iter()
+        .flat_map(|state| &state.services)
+        .filter_map(|service| match &service.tooltip {
+            TrainerTooltip::Spell { spell_id, .. } => Some(*spell_id),
+            TrainerTooltip::Item(_) => None,
+        })
+        .collect();
+    fresh_tooltip_subjects.sort_unstable();
+    fresh_tooltip_subjects.dedup();
+    if tooltip_subjects.0 != fresh_tooltip_subjects {
+        tooltip_subjects.0 = fresh_tooltip_subjects;
+    }
     // A name-only change re-fires `TRAINER_UPDATE`, so the title's `UnitName("npc")` repaints.
     // The name rides as arg1, which the 1.12 trainer events do not carry.
     let trainer_name = open