From cf891dc3756a36dc0af4376f861ffb0c847ba5e9 Mon Sep 17 00:00:00 2001 From: Mathih13 Date: Sat, 3 Oct 2026 12:36:21 +0200 Subject: [PATCH] fix: close a loot window that opens while you move, as 1.12.1 does (#786) - A loot window that opens while you move closes, as in 1.12.1 (#782) - Set auto_loot in the LootConfig initializer (clippy field_reassign_with_default) - Close a window opened on the move in the feed's pass, so it never paints Co-Authored-By: Claude Opus 5.5 (1M context) Co-Authored-By: Sam --- .../benilla-app/src/creature_anim/select.rs | 5 +- crates/benilla-app/src/game_plugins.rs | 2 + crates/benilla-app/src/target/click.rs | 2 +- crates/benilla-app/src/ui_loot/mod.rs | 40 ++- crates/benilla-app/src/ui_loot/net.rs | 323 ++++++++++++++++-- crates/benilla-app/src/ui_script/test_ui.rs | 2 +- 6 files changed, 334 insertions(+), 40 deletions(-) diff --git a/crates/benilla-app/src/creature_anim/select.rs b/crates/benilla-app/src/creature_anim/select.rs index 2c35e9d9..926bb17c 100644 --- a/crates/benilla-app/src/creature_anim/select.rs +++ b/crates/benilla-app/src/creature_anim/select.rs @@ -111,10 +111,13 @@ pub(crate) mod move_flags { /// Any horizontal direction bit: the client's `[9e8] & 0xf` gate. pub const ANY_MOVE: u32 = FORWARD | BACKWARD | STRAFE_LEFT | STRAFE_RIGHT; + /// The low byte: forward, back, strafe, turn and pitch, the loot response's test (`0x5ebc40`). + pub const STEERING: u32 = 0xff; + /// The bits that get a mover integrated at all, the client's `0x20ff` (`0x616e20`, `0x6166f5`): /// without one a mover keeps its last packet's pose, and a flag-less unit is not even in the /// mover list (`0x618940`). Swim and the mode bits are not among them. - pub const INTEGRATED: u32 = 0xff | FALLING; + pub const INTEGRATED: u32 = STEERING | FALLING; /// The committed-lower-body test routing a one-shot to the masked overlay (`0x5fe6dc`); the /// client's separate mouse-turn test (`d58 & 0x1800`) reads a field benilla does not model. diff --git a/crates/benilla-app/src/game_plugins.rs b/crates/benilla-app/src/game_plugins.rs index 4a9e49ac..53e216f0 100644 --- a/crates/benilla-app/src/game_plugins.rs +++ b/crates/benilla-app/src/game_plugins.rs @@ -1149,6 +1149,8 @@ pub(crate) mod schedule_tests { "both packets answer the `CMSG_LOGOUT_REQUEST`/cancel the game menu sent"), ("ui_loot/mod.rs", "drain_loot", Because::Deliberate, "the pre-VM take of `LootMoveStart` is documented at the line and publishes nothing to the VM; the event-firing takes are Lua's queues"), + ("ui_loot/mod.rs", "feed_loot", Because::PlayerRoundTrip, + "it takes `LootMoveStart` only on a window it has just opened, which answers a loot click"), ("ui_loot_roll.rs", "drain_loot_rolls", Because::FilledByVm, "only a Need/Greed/Pass click queues a confirm or vote, held in the VM"), ("ui_loot_roll.rs", "feed_loot_rolls", Because::PlayerRoundTrip, diff --git a/crates/benilla-app/src/target/click.rs b/crates/benilla-app/src/target/click.rs index 3f27b934..9c8ecef6 100644 --- a/crates/benilla-app/src/target/click.rs +++ b/crates/benilla-app/src/target/click.rs @@ -1430,7 +1430,7 @@ pub(crate) fn service_action( /// Drain the guid-scoped deselects ([`DeselectGuid`]). `ClearTarget()`, the last leg of the Esc /// chain (`UIParent.lua:1492`), is a script call and lands in call order /// ([`crate::script_calls`]). -pub(super) fn clear_target_requests( +pub(crate) fn clear_target_requests( mut selection: ResMut, mut seam: crate::creature_anim::AttackSeam, engaged: Query<(), (With, With)>, diff --git a/crates/benilla-app/src/ui_loot/mod.rs b/crates/benilla-app/src/ui_loot/mod.rs index f7e8e682..b9d41cae 100644 --- a/crates/benilla-app/src/ui_loot/mod.rs +++ b/crates/benilla-app/src/ui_loot/mod.rs @@ -363,7 +363,8 @@ impl LootLatch { /// A movement start that closes the loot: the reference's guard `0x60e990`, called by every /// movement-start emitter, turning included, but not by mouse-look facing. No loot target reaches /// the range gate `0x493230`, so there is no distance leash. vmangos also releases on movement -/// (`MovementHandler.cpp:1104`), but the close is the client's. +/// (`MovementHandler.cpp:1104`), but the close is the client's. A loot response that lands while +/// we move sets it too (`0x5ebc3a`). #[derive(Resource, Default)] pub(crate) struct LootMoveStart(pub(crate) bool); @@ -674,6 +675,7 @@ fn feed_loot( group: Res, names: Res, objects: LootSourceObjects, + mut moving: OpenWhileMoving, ) { let Some(mut script) = script else { return; @@ -755,6 +757,19 @@ fn feed_loot( } } } + // The response's tail (`0x5ebc3a`-`0x5ebc51`) closes a window that opened while we + // move, after the copier's open and sweep and in the same handler: `LOOT_CLOSED` fires + // in this pass, so the window never paints. + if std::mem::take(&mut moving.move_start.0) { + let closed = close_on_move_start(&mut loot, &mut moving.latch, &commands); + if closed.is_some() { + moving.dead.after_close(closed); + script.set_loot(None); + script.fire_event("LOOT_CLOSED", vec![]); + *last = None; + return; + } + } } // One `LOOT_SLOT_CLEARED` per row that went, with its 1-based row: the stock handler hides // that button and pages down when the page empties (`LootFrame.lua:22-50`). @@ -862,6 +877,14 @@ fn close_on_move_start( close_interaction(loot, latch, Some(commands)) } +/// What [`feed_loot`] needs to take the move-start close on a window it has just opened. +#[derive(bevy::ecs::system::SystemParam)] +struct OpenWhileMoving<'w, 's> { + move_start: ResMut<'w, LootMoveStart>, + latch: ResMut<'w, LootLatch>, + dead: DeadUnitDeselect<'w, 's>, +} + /// Sends what the Lua asked for, as the take dispatcher `0x4c2790(slot, flag)`: a row click /// (a `LootButton`'s own, flag 0) and the `LOOT_BIND` confirm (`LootSlot`, flag 1) stay apart, /// so a second click on a bind-on-pickup row asks again. @@ -1170,6 +1193,11 @@ mod tests { let (tx, _rx) = crossbeam_channel::unbounded(); let mut app = App::new(); app.add_message::() + // What the feed's move-start close reads (`OpenWhileMoving`). + .add_message::() + .init_resource::() + .init_resource::() + .init_resource::() .init_resource::() .init_resource::() .init_resource::() @@ -1227,6 +1255,11 @@ mod tests { let (tx, rx) = crossbeam_channel::unbounded(); let mut app = App::new(); app.add_message::() + // What the feed's move-start close reads (`OpenWhileMoving`). + .add_message::() + .init_resource::() + .init_resource::() + .init_resource::() .init_resource::() .init_resource::() .init_resource::() @@ -1460,6 +1493,11 @@ mod tests { let (tx, rx) = crossbeam_channel::unbounded(); let mut app = App::new(); app.add_message::() + // What the feed's move-start close reads (`OpenWhileMoving`). + .add_message::() + .init_resource::() + .init_resource::() + .init_resource::() .init_resource::() .init_resource::() .init_resource::() diff --git a/crates/benilla-app/src/ui_loot/net.rs b/crates/benilla-app/src/ui_loot/net.rs index ffd92655..fcb1ed0c 100644 --- a/crates/benilla-app/src/ui_loot/net.rs +++ b/crates/benilla-app/src/ui_loot/net.rs @@ -5,7 +5,7 @@ use benilla_protocol::messages::ItemPushResult; use benilla_protocol::{SessionEvent, SessionEventKind}; use bevy::prelude::*; -use super::{LootLatch, LootState}; +use super::{LootLatch, LootMoveStart, LootState}; use crate::net::{ClientCommand, NetCommands, NetHandlerApp, SelfGuid}; use crate::pending_item_ops::{LockTransitions, PendingItemOps}; use crate::ui_action::{UiError, UiErrorKeys}; @@ -26,8 +26,15 @@ pub(super) fn register(app: &mut App) { } /// An admitted response selects a looted unit (`0x5ebc35` → `0x48f3a0`) before the window opens -/// (`0x4c1cb0`), so the outgoing target's teardown closes a window still open on it. -fn on_response(In(ev): In, mut select: crate::target::SelectCommit) { +/// (`0x4c1cb0`), so the outgoing target's teardown closes a window still open on it. One that lands +/// while our own body moves takes the move-start close (`0x5ebc3a`-`0x5ebc51`), which the feed runs +/// after it fires `LOOT_OPENED`, or the drain when the window waits on an item template. +fn on_response( + In(ev): In, + mut select: crate::target::SelectCommit, + player: Option>, + mut move_start: ResMut, +) { let SessionEvent::LootResponse { guid, loot_type, @@ -47,6 +54,12 @@ fn on_response(In(ev): In, mut select: crate::target::SelectCommit ); select.select_unit(guid); select.seam.loot.open(guid, loot_type, gold, items); + if player.is_some_and(|p| { + p.foreign_mover.is_none() + && p.move_flags() & crate::creature_anim::move_flags::STEERING != 0 + }) { + move_start.0 = true; + } } fn on_error( @@ -763,6 +776,8 @@ mod tests { world.init_resource::(); world.init_resource::>(); world.init_resource::>(); + world.init_resource::(); + world.init_resource::(); world.spawn(( SelfPlayer, Guid(1), @@ -988,34 +1003,33 @@ mod tests { ); } - /// The issue's symptom on the stock frames: the Tab target is on `TargetFrame` when the body's - /// window opens, and the unit feed then names the body there (`TargetFrame.lua:63`, - /// `UnitFrame.lua:25`). - #[test] - fn the_stock_target_frame_names_the_looted_body() { + /// The stock unit frames over `TargetFrame`, for [`stock_app`]. + const TARGET_UI: &[&str] = &[ + "Interface\\FrameXML\\Fonts.xml", + "Interface\\FrameXML\\GlobalStrings.lua", + "Interface\\FrameXML\\UIParent.xml", + "Interface\\FrameXML\\BasicControls.xml", + "Interface\\FrameXML\\MoneyFrame.lua", + "Interface\\FrameXML\\MoneyFrame.xml", + "Interface\\FrameXML\\GameTooltip.xml", + "Interface\\FrameXML\\UIDropDownMenu.xml", + "Interface\\FrameXML\\TextStatusBar.lua", + "Interface\\FrameXML\\TextStatusBar.xml", + "Interface\\FrameXML\\BuffFrame.xml", + "Interface\\FrameXML\\CombatFeedback.xml", + "Interface\\FrameXML\\UnitPopup.xml", + "Interface\\FrameXML\\UnitFrame.xml", + "Interface\\FrameXML\\PlayerFrame.xml", + "Interface\\FrameXML\\PartyFrame.xml", + "Interface\\FrameXML\\TargetFrame.xml", + ]; + + /// [`tabbed_world`] under the stock `ui` files, with the unit feed and both creatures named. + fn stock_app(ui: &[&[&str]]) -> App { use benilla_ui::script::UiScript; - benilla_formats::wow_data_or_skip!(); let mut s = UiScript::new().expect("VM"); s.set_screen_size(1024.0, 768.0); - for f in crate::ui_script::test_ui::production_order(&[&[ - "Interface\\FrameXML\\Fonts.xml", - "Interface\\FrameXML\\GlobalStrings.lua", - "Interface\\FrameXML\\UIParent.xml", - "Interface\\FrameXML\\BasicControls.xml", - "Interface\\FrameXML\\MoneyFrame.lua", - "Interface\\FrameXML\\MoneyFrame.xml", - "Interface\\FrameXML\\GameTooltip.xml", - "Interface\\FrameXML\\UIDropDownMenu.xml", - "Interface\\FrameXML\\TextStatusBar.lua", - "Interface\\FrameXML\\TextStatusBar.xml", - "Interface\\FrameXML\\BuffFrame.xml", - "Interface\\FrameXML\\CombatFeedback.xml", - "Interface\\FrameXML\\UnitPopup.xml", - "Interface\\FrameXML\\UnitFrame.xml", - "Interface\\FrameXML\\PlayerFrame.xml", - "Interface\\FrameXML\\PartyFrame.xml", - "Interface\\FrameXML\\TargetFrame.xml", - ]]) { + for f in crate::ui_script::test_ui::production_order(ui) { crate::ui_script::test_ui::load_ui(&s, f); } @@ -1048,20 +1062,257 @@ mod tests { .init_resource::() .insert_non_send_resource(s); crate::ui_unit::add_unit_feed(&mut app); - let shown = |app: &mut App| -> String { - let s = app.world().non_send_resource::(); - assert!(s.errors().is_empty(), "script errors: {:?}", s.errors()); - s.eval("return TargetFrame:IsShown() and TargetFrame.name:GetText() or ''") - .expect("the target frame's name") - }; + app + } + + /// The name on the stock `TargetFrame`, empty while it is hidden. + fn target_frame_name(app: &App) -> String { + let s = app + .world() + .non_send_resource::(); + assert!(s.errors().is_empty(), "script errors: {:?}", s.errors()); + s.eval("return TargetFrame:IsShown() and TargetFrame.name:GetText() or ''") + .expect("the target frame's name") + } + + /// The issue's symptom on the stock frames: the Tab target is on `TargetFrame` when the body's + /// window opens, and the unit feed then names the body there (`TargetFrame.lua:63`, + /// `UnitFrame.lua:25`). + #[test] + fn the_stock_target_frame_names_the_looted_body() { + benilla_formats::wow_data_or_skip!(); + let mut app = stock_app(&[TARGET_UI]); app.update(); - assert_eq!(shown(&mut app), "Kobold Vermin", "the Tab target"); + assert_eq!(target_frame_name(&app), "Kobold Vermin", "the Tab target"); app.world_mut().resource_mut::().0 = Some(CORPSE); respond(app.world_mut(), CORPSE, 1); app.update(); - assert_eq!(shown(&mut app), "Kobold Worker", "the looted body"); + assert_eq!(target_frame_name(&app), "Kobold Worker", "the looted body"); + } + + /// What [`super::super::drain_loot`] reads beside [`seat_tabbed`]'s, with our body's movement + /// word at `player`'s. + fn seat_drain(world: &mut World, player: crate::player::Player) { + world.insert_resource(player); + world.init_resource::(); + world.init_resource::(); + world.init_resource::>(); + } + + /// The deselects asked for since the last call. + fn deselects(world: &mut World) -> Vec { + world + .resource_mut::>() + .drain() + .map(|d| d.0) + .collect() + } + + /// A response on the corpse with `player` seated, then one drain. + fn open_while( + player: crate::player::Player, + ) -> (World, crossbeam_channel::Receiver) { + let (mut world, rx) = tabbed_world(); + seat_drain(&mut world, player); + world.resource_mut::().0 = Some(CORPSE); + respond(&mut world, CORPSE, 1); + world + .run_system_once(super::super::drain_loot) + .expect("the drain runs as a one-shot system"); + (world, rx) + } + + /// The response's tail (`0x5ebc3a`-`0x5ebc51`): a bit of our movement word's low byte, forward, + /// back, strafe, turn or pitch, takes the move-start close on the window it just opened, the + /// release after the selection and the dead body's deselect last (`0x48f369`). + #[test] + fn a_window_that_opens_while_moving_closes_as_a_movement_start_does() { + for bit in (0..8).map(|b| 1u32 << b) { + let (mut world, rx) = open_while(crate::player::Player::with_move_flags(bit)); + assert_eq!(world.resource::().source(), None, "{bit:#x}"); + assert_eq!(world.resource::().0, None, "{bit:#x}"); + let sent: Vec<_> = rx.try_iter().collect(); + assert!( + matches!( + sent[..], + [ClientCommand::SetSelection { guid: a }, ClientCommand::LootRelease { guid: b }] + if a == CORPSE && b == CORPSE + ), + "{bit:#x}: {sent:?}" + ); + assert_eq!(deselects(&mut world), [CORPSE], "{bit:#x}"); + } + } + + /// Only that byte, and only our body's: walking, falling or swimming alone leaves the window + /// open, as does a step of the creature we possess. + #[test] + fn a_window_opened_falling_swimming_or_possessing_stays_open() { + use crate::creature_anim::move_flags as f; + use crate::player::Player; + let mut possessing = Player::with_move_flags(f::FORWARD); + possessing.foreign_mover = Some(KOBOLD); + for (what, player) in [ + ("still", Player::with_move_flags(0)), + ("walk mode", Player::with_move_flags(f::WALK_MODE)), + ("falling", Player::with_move_flags(f::FALLING)), + ("swimming", Player::with_move_flags(f::SWIMMING)), + ("possessing", possessing), + ] { + let (mut world, rx) = open_while(player); + assert_eq!( + world.resource::().source(), + Some(CORPSE), + "{what}" + ); + let sent: Vec<_> = rx.try_iter().collect(); + assert!( + matches!(sent[..], [ClientCommand::SetSelection { guid }] if guid == CORPSE), + "{what}: {sent:?}" + ); + assert!(deselects(&mut world).is_empty(), "{what}"); + } + } + + /// The copier `0x4c1cb0` sweeps an auto-loot window (`0x4c1fa0`) before the response's + /// movement close (`0x5ebc51`), so a body auto-looted on the run is taken, then released. + #[test] + fn auto_loot_on_the_run_takes_the_rows_before_the_release() { + const TOUGH_JERKY: u32 = 117; + let mut app = App::new(); + let rx = seat_tabbed(app.world_mut()); + seat_drain( + app.world_mut(), + crate::player::Player::with_move_flags(crate::creature_anim::move_flags::FORWARD), + ); + app.world_mut() + .resource_mut::() + .insert_template( + TOUGH_JERKY, + Some(crate::items::test_template("Tough Jerky")), + ); + app.insert_resource(super::super::LootConfig { + auto_loot: true, + ..Default::default() + }) + .init_resource::() + .init_resource::() + .init_resource::>() + .insert_non_send_resource(benilla_ui::script::UiScript::new().expect("VM")) + .add_systems( + Update, + (super::super::feed_loot, super::super::drain_loot).chain(), + ); + + app.world_mut().resource_mut::().0 = Some(CORPSE); + app.world_mut() + .run_system_once_with( + on_response, + SessionEvent::LootResponse { + guid: CORPSE, + loot_type: 1, + gold: 0, + items: vec![benilla_protocol::messages::LootItem { + slot: 0, + item_id: TOUGH_JERKY, + count: 1, + display_info_id: 1117, + random_property_id: 0, + slot_type: 0, + }], + }, + ) + .expect("the handler runs as a one-shot system"); + app.update(); + let sent: Vec<_> = rx.try_iter().collect(); + assert!( + matches!( + sent[..], + [ + ClientCommand::SetSelection { .. }, + ClientCommand::AutostoreLootItem { slot: 0 }, + ClientCommand::LootRelease { guid }, + ] if guid == CORPSE + ), + "{sent:?}" + ); + } + + /// The issue's spot on the stock frames: a body's window that opens on the run is shown by + /// `LOOT_OPENED` and hidden by `LOOT_CLOSED` in the same frame (`LootFrame.lua:13-16`, + /// `:53-55`), its one release + /// sent, and `TargetFrame` lets the body go; standing, the window and the target stay. + #[test] + fn the_stock_loot_frame_closes_on_a_body_looted_on_the_run() { + use crate::creature_anim::move_flags as f; + use benilla_ui::script::UiScript; + benilla_formats::wow_data_or_skip!(); + for (flags, stays) in [(f::FORWARD, false), (0, true)] { + let mut app = stock_app(&[ + crate::ui_script::test_ui::LOOT_UI, + TARGET_UI, + &["Interface\\FrameXML\\LootFrame.xml"], + ]); + let (tx, rx) = crossbeam_channel::unbounded(); + app.insert_resource(crate::net::NetCommands(tx)); + seat_drain( + app.world_mut(), + crate::player::Player::with_move_flags(flags), + ); + app.init_resource::() + .init_resource::>() + .add_systems( + Update, + ( + super::super::feed_loot, + super::super::drain_loot, + crate::target::click::clear_target_requests, + ) + .chain(), + ); + app.world() + .non_send_resource::() + .run( + "LOOT_EVENTS = {} local f = CreateFrame('Frame') \ + f:RegisterEvent('LOOT_OPENED') f:RegisterEvent('LOOT_CLOSED') \ + f:SetScript('OnEvent', function() tinsert(LOOT_EVENTS, event) end)", + ) + .unwrap(); + app.update(); + + app.world_mut().resource_mut::().0 = Some(CORPSE); + respond(app.world_mut(), CORPSE, 1); + // One frame: the reference opens and closes in the response's handler. + app.update(); + let (events, shown): (String, bool) = app + .world() + .non_send_resource::() + .eval("return table.concat(LOOT_EVENTS, ','), LootFrame:IsShown() == 1") + .unwrap(); + for _ in 0..2 { + app.update(); + } + let releases = rx + .try_iter() + .filter(|c| matches!(c, ClientCommand::LootRelease { guid } if *guid == CORPSE)) + .count(); + if stays { + assert_eq!(events, "LOOT_OPENED"); + assert!(shown, "standing, the window stays"); + assert_eq!(releases, 0); + assert_eq!(target_frame_name(&app), "Kobold Worker"); + } else { + assert_eq!(events, "LOOT_OPENED,LOOT_CLOSED"); + assert!( + !shown, + "on the run, the window closes in the frame it opens" + ); + assert_eq!(releases, 1, "released once"); + assert_eq!(target_frame_name(&app), "", "and the body is let go"); + } + } } /// Only a releasing arm reaches `UnlockItem 0x495420`, through the tail `0x5ebac2`. diff --git a/crates/benilla-app/src/ui_script/test_ui.rs b/crates/benilla-app/src/ui_script/test_ui.rs index f2436e9b..00ab9176 100644 --- a/crates/benilla-app/src/ui_script/test_ui.rs +++ b/crates/benilla-app/src/ui_script/test_ui.rs @@ -501,7 +501,7 @@ pub(crate) const GOSSIP_UI: &[&str] = &[ /// lacks what the app loads at setup (the master-loot menu reads `GROUP` and `GIVE_LOOT`), /// `ItemButtonTemplate.xml` because a missing template only warns, and `PartyFrame.xml` because /// `LootFrame.lua:217` reads `MAX_PARTY_MEMBERS` at load. Needs client data. -pub(super) const LOOT_UI: &[&str] = &[ +pub(crate) const LOOT_UI: &[&str] = &[ "Interface\\FrameXML\\GlobalStrings.lua", "Interface\\FrameXML\\Fonts.xml", "Interface\\FrameXML\\BasicControls.xml", // `TEXT`, which UnitPopup.lua reads at file scope