diff --git a/crates/benilla-app/src/addon_harness/mod.rs b/crates/benilla-app/src/addon_harness/mod.rs index ab1997ed..90ada1c9 100644 --- a/crates/benilla-app/src/addon_harness/mod.rs +++ b/crates/benilla-app/src/addon_harness/mod.rs @@ -1680,6 +1680,10 @@ fn seat_a_session(script: &mut UiScript) { // `realm .. " - " .. faction` at file scope, so a nil faction is 24 addons stopping on // `attempt to concatenate local 'faction'`. Every playable race has a side. faction_group: Some("Alliance".into()), + // `0x5efe00`'s team digit for Human (race 1) — the rank-title key's second `%d`. + // Seated beside the faction group because they are different questions + // (`ui_unit::race_pvp_team`), not because they agree here. + pvp_team: crate::ui_unit::race_pvp_team(1), ..Default::default() }), ); diff --git a/crates/benilla-app/src/lib.rs b/crates/benilla-app/src/lib.rs index 2fa1624f..141f64ba 100644 --- a/crates/benilla-app/src/lib.rs +++ b/crates/benilla-app/src/lib.rs @@ -109,6 +109,7 @@ mod shaders; mod game_tip; mod name_persist; +mod opaque2d; /// Where "the client is going down" may be observed, and why that is `Last` and not `Update` /// (decision 1528). Every system that persists state on the way out registers through it. mod shutdown; diff --git a/crates/benilla-app/src/opaque2d.rs b/crates/benilla-app/src/opaque2d.rs new file mode 100644 index 00000000..433c4c84 --- /dev/null +++ b/crates/benilla-app/src/opaque2d.rs @@ -0,0 +1,128 @@ +//! **The 2D opaque pass, skipped when it has nothing to draw** (decision 2225). +//! +//! bevy's `MainOpaquePass2dNode` opens a command encoder and a render pass whether or not its two +//! phases hold an item, and on every camera of ours they never do: each player-UI quad is in the +//! transparent phase (`AlphaMode2d::Blend`, decision 0254), Bevy UI paints through its own node, +//! egui through its own. 2197 parked the pass as "price it on an immediate-mode GPU"; 2205 priced +//! its GPU at zero; the crowd profile of 2225 priced its CPU at ~0.2 ms a frame, parked, traced — +//! an encoder and a pass that wgpu sizes and clears its usage trackers for, empty or not (the +//! residency term `perf::gpu`'s census names). +//! +//! The pass exists in bevy for the clear, and the clear does not need it: whichever pass touches +//! the view target first clears it (`ColorAttachment`'s first-call semantics, the same rule +//! `static_gx` and the FFX combine already lean on), and bevy's transparent 2D node opens its pass +//! unconditionally for exactly that reason. So an empty opaque pass has nothing to do that the +//! pass after it does not. +//! +//! This is bevy's node with one early return, registered under bevy's own label: +//! `RenderGraph::add_node` is a map insert, so the later registration replaces the node and the +//! graph's edges — keyed by label — carry over untouched. + +use bevy::core_pipeline::core_2d::graph::{Core2d, Node2d}; +use bevy::core_pipeline::core_2d::{AlphaMask2d, Opaque2d}; +use bevy::ecs::query::QueryItem; +use bevy::prelude::*; +use bevy::render::camera::ExtractedCamera; +use bevy::render::diagnostic::RecordDiagnostics; +use bevy::render::render_graph::{ + NodeRunError, RenderGraphContext, RenderGraphExt, ViewNode, ViewNodeRunner, +}; +use bevy::render::render_phase::{TrackedRenderPass, ViewBinnedRenderPhases}; +use bevy::render::render_resource::{CommandEncoderDescriptor, RenderPassDescriptor, StoreOp}; +use bevy::render::renderer::RenderContext; +use bevy::render::view::{ExtractedView, ViewDepthTexture, ViewTarget}; +use bevy::render::RenderApp; + +#[derive(Default)] +struct SkipEmptyOpaque2dNode; + +impl ViewNode for SkipEmptyOpaque2dNode { + type ViewQuery = ( + &'static ExtractedCamera, + &'static ExtractedView, + &'static ViewTarget, + &'static ViewDepthTexture, + ); + + fn run<'w>( + &self, + graph: &mut RenderGraphContext, + render_context: &mut RenderContext<'w>, + (camera, view, target, depth): QueryItem<'w, '_, Self::ViewQuery>, + world: &'w World, + ) -> Result<(), NodeRunError> { + let (Some(opaque_phases), Some(alpha_mask_phases)) = ( + world.get_resource::>(), + world.get_resource::>(), + ) else { + return Ok(()); + }; + let view_entity = graph.view_entity(); + let (Some(opaque_phase), Some(alpha_mask_phase)) = ( + opaque_phases.get(&view.retained_view_entity), + alpha_mask_phases.get(&view.retained_view_entity), + ) else { + return Ok(()); + }; + // The one line bevy's node does not have. Nothing to draw means nothing to encode: the + // transparent pass that follows opens its own pass unconditionally and takes the target's + // first-call clear. + if opaque_phase.is_empty() && alpha_mask_phase.is_empty() { + return Ok(()); + } + + let diagnostics = render_context.diagnostic_recorder(); + let color_attachments = [Some(target.get_color_attachment())]; + let depth_stencil_attachment = Some(depth.get_attachment(StoreOp::Store)); + + render_context.add_command_buffer_generation_task(move |render_device| { + let mut command_encoder = + render_device.create_command_encoder(&CommandEncoderDescriptor { + label: Some("main_opaque_pass_2d_command_encoder"), + }); + let render_pass = command_encoder.begin_render_pass(&RenderPassDescriptor { + label: Some("main_opaque_pass_2d"), + color_attachments: &color_attachments, + depth_stencil_attachment, + timestamp_writes: None, + occlusion_query_set: None, + }); + let mut render_pass = TrackedRenderPass::new(&render_device, render_pass); + let pass_span = diagnostics.pass_span(&mut render_pass, "main_opaque_pass_2d"); + if let Some(viewport) = camera.viewport.as_ref() { + render_pass.set_camera_viewport(viewport); + } + if !opaque_phase.is_empty() { + if let Err(err) = opaque_phase.render(&mut render_pass, world, view_entity) { + error!("Error encountered while rendering the 2d opaque phase {err:?}"); + } + } + if !alpha_mask_phase.is_empty() { + if let Err(err) = alpha_mask_phase.render(&mut render_pass, world, view_entity) { + error!("Error encountered while rendering the 2d alpha mask phase {err:?}"); + } + } + pass_span.end(&mut render_pass); + drop(render_pass); + command_encoder.finish() + }); + Ok(()) + } +} + +/// Replaces bevy's 2D opaque node under its own label. Added by [`crate::ui_pass::PlayerUiPlugin`], +/// which owns every camera that runs the `Core2d` graph, after `DefaultPlugins` has registered the +/// node it replaces. +pub(crate) struct SkipEmptyOpaque2dPlugin; + +impl Plugin for SkipEmptyOpaque2dPlugin { + fn build(&self, app: &mut App) { + let Some(render_app) = app.get_sub_app_mut(RenderApp) else { + return; + }; + render_app.add_render_graph_node::>( + Core2d, + Node2d::MainOpaquePass, + ); + } +} diff --git a/crates/benilla-app/src/target/click.rs b/crates/benilla-app/src/target/click.rs index 018081c9..1198ed2c 100644 --- a/crates/benilla-app/src/target/click.rs +++ b/crates/benilla-app/src/target/click.rs @@ -30,8 +30,10 @@ use super::*; /// app's cursor-transition watcher (`crate::sound`), matching the ref's `ClearCursor` play. pub(super) fn world_right_click_payload( mut right_clicks: MessageReader, - hovered: Res, - hovered_object: Res, + // The **press** pick, not the live hover (decision 2230) — this leg and [`act_on_right_click`] + // are two halves of one click's routing, so they must classify the same pick or a right-click + // can dismiss the cursor payload *and* interact with the thing it was held over. + press: Res, script: Option>, ) { if right_clicks.read().last().is_none() { @@ -40,7 +42,7 @@ pub(super) fn world_right_click_payload( let Some(mut script) = script else { return; }; - if hovered.target.is_some() || hovered_object.target.is_some() { + if press.hovered.target.is_some() || press.object.target.is_some() { return; } script.clear_cursor_payload(); @@ -56,16 +58,21 @@ pub(super) fn world_right_click_payload( /// replayed a [`WorldClick`] and was guarded on the press pick naming the same unit; the live probe /// caught it — the scripted click selected nothing. /// -/// The RIGHT button is the other way round, deliberately. [`act_on_right_click`] acts on the LIVE -/// hover across many legs (interact, attack, loot, GameObject), so feeding it a unit the pointer is -/// not on could attack the wrong thing; it is replayed only when the hover already names the -/// plate's unit — the physical case. A scripted right-click on a plate does nothing yet, which is -/// a stated gap rather than a guess about which leg it should take. +/// The RIGHT button is the other way round, deliberately. [`act_on_right_click`] acts on the +/// frame's [`PressPick`] across many legs (interact, attack, loot, GameObject), so feeding it a +/// unit that pick does not name could attack the wrong thing; it is replayed only when the press +/// pick already names the plate's unit. A scripted right-click on a plate does nothing yet, which +/// is a stated gap rather than a guess about which leg it should take. +/// +/// The **physical** right-click on a plate does not come through here at all and never did: the +/// press engages freelook (the camera looks through a plate — 2159), freelook hands the plates' +/// mouse back (`0x60f830`), and the release therefore hit-tests off the plate, so its `OnClick` +/// never fires. That gesture is the camera arbiter's own [`WorldRightClick`], acting on the pick +/// the press latched — which is exactly what decision 2230 made it read. #[allow(clippy::too_many_arguments)] pub(super) fn select_on_plate_click( mut plate: ResMut, press: Res, - hovered: Res, ground: Res, mut selection: ResMut, mut seam: crate::creature_anim::AttackSeam, @@ -95,7 +102,7 @@ pub(super) fn select_on_plate_click( greeting.write(crate::sound::NpcGreetingRequest { npc: entity }); // The attack classification is the press pick's, and only when the press was actually on // this unit: a scripted click has no cursor behind it, so it is not an attack-cursor click. - let attack = press.attack && press.hovered.target == Some(entity); + let attack = press.attack() && press.hovered.target == Some(entity); scan::commit( &mut selection, &mut seam, @@ -107,7 +114,7 @@ pub(super) fn select_on_plate_click( attack, ); } - if right.into_iter().any(|e| hovered.target == Some(e)) { + if right.into_iter().any(|e| press.hovered.target == Some(e)) { right_clicks.write(WorldRightClick); } } @@ -179,7 +186,7 @@ pub(super) fn select_on_click( stores.get(entity).ok(), engaged, self_guid, - press.attack, + press.attack(), ); } // Clicked nothing targetable → deselect (only sends the clear if we actually had a @@ -288,9 +295,16 @@ fn interaction_already_open_on(target: u64, interact: &crate::ui_session::Intera #[allow(clippy::too_many_arguments, clippy::type_complexity)] pub(super) fn act_on_right_click( mut clicks: MessageReader, - hovered: Res, - hovered_object: Res, - cursor: Res, + // **The press pick, not the live hover** (decision 2230) — the same latch the left button has + // read since 1122, and for the same reason: the reference picks exactly once, on the down edge + // (`0x481f00`, one caller image-wide), into the WorldFrame's own slots, and nothing re-picks on + // move or on release. Reading the live hover here only looked right because the release frame + // re-picks at the restored cursor and, for a body click that moved nothing, lands back on the + // same unit. A V-plate has no body under the cursor to land back on: the plate published the + // mouseover at the press (`0x7cb850` → `0x492890`), freelook then took the plates' mouse away + // (`0x60f830`), and the live hover at the release was empty — so a right-click on a plate + // reached every branch below with nothing hovered and did nothing at all. + press: Res, mut selection: ResMut, mut seam: crate::creature_anim::AttackSeam, self_player: Query<(Entity, &Guid, Has), With>, @@ -333,6 +347,8 @@ pub(super) fn act_on_right_click( if clicks.read().last().is_none() { return; } + // The whole ladder below reads ONE pick — the gesture's own, frozen at the press. + let (hovered, hovered_object, cursor) = (&press.hovered, &press.object, &press.cursor); // ── The interact family's ACTOR gate: am I in the saddle? ──────────────────────────────── // One predicate, the reference's own — the PLAYER's `UNIT_FIELD_MOUNTDISPLAYID` (decision // 0481's "one mounted predicate"; wow-re `mounted-action-gate.md`: no aura, no taxi @@ -359,7 +375,7 @@ pub(super) fn act_on_right_click( // suppresses the send with no toast (the reference auto-walks there instead — `0x610300`, also // no packet). The lock arm runs first in `0x5f3130`, and it does here too: a `Refuse` toasts // even when we are also out of range. - if go_is_nearest(&hovered, &hovered_object) { + if go_is_nearest(hovered, hovered_object) { // **The interact chain's first link** (tag `use`). "I clicked it and nothing happened" spans // three systems — this decision, the `CMSG_GAMEOBJ_USE` it sends, and the `SMSG_SPELL_GO` // the server answers with — and until this line existed only the *taken* branches said @@ -2246,4 +2262,90 @@ mod tests { set(&mut world, None); assert_eq!(run(&mut world, "TargetLastEnemy()"), None); } + use crate::net::{ClientCommand, NetCommands, ObjectStore}; + use bevy::ecs::system::RunSystemOnce; + + const F_HEALTH: u16 = 22; + const F_MAXHEALTH: u16 = 28; + const BOAR: u64 = 0xB0A2; + const ME: u64 = 0x5E1F; + + fn store(pairs: &[(u16, u32)]) -> ObjectStore { + ObjectStore(benilla_protocol::ObjectFields::from_pairs(pairs)) + } + + /// Everything [`act_on_right_click`] and the commit under it reach for, and nothing else. + fn right_click_world() -> (World, Entity) { + let (tx, _rx) = crossbeam_channel::unbounded::(); + let mut world = World::new(); + world.insert_resource(NetCommands(tx)); + world.init_resource::>(); + world.init_resource::(); + world.init_resource::(); + world.init_resource::(); + world.init_resource::(); + world.init_resource::>(); + world.init_resource::>(); + world.init_resource::(); + world.init_resource::(); + world.init_resource::(); + world.init_resource::(); + world.init_resource::(); + world.init_resource::(); + world.init_resource::(); + world.init_resource::(); + world.init_resource::(); + world.init_resource::(); + world.init_resource::(); + world.init_resource::(); + world.init_resource::(); + world.init_resource::(); + world.init_resource::(); + world.spawn((SelfPlayer, Guid(ME))); + let boar = world + .spawn((Guid(BOAR), store(&[(F_HEALTH, 100), (F_MAXHEALTH, 100)]))) + .id(); + (world, boar) + } + + /// **A right-click that began on a V-plate acts on that plate's unit** — the bug the director + /// reported as "right click on nameplates no longer works", and the reason + /// [`act_on_right_click`] reads [`PressPick`] (decision 2230). + /// + /// The gesture it reconstructs is the whole of it: the plate published the mouseover at the + /// press, the press engaged freelook (the camera looks *through* a plate — 2159), freelook + /// handed the plates' mouse back (`0x60f830`) so the plate's own `OnClick` never fires and its + /// hover goes dark, and `update_hover` blanks the live [`Hovered`] for the whole session. What + /// survives all of that is the press latch — and a plate hover leaves **no body under the + /// cursor** for the release frame's re-pick to land back on, which is exactly why the live + /// hover carried the body case and dropped this one. + #[test] + fn a_right_click_begun_on_a_plate_acts_on_the_plates_unit() { + let (mut world, boar) = right_click_world(); + // The press: the plate's unit is the mouseover (distance 0.0 — topmost UI), classified + // Attack. Nothing here is live state; it is the latch, exactly as `latch_press_pick` wrote + // it one frame before the look session began. + *world.resource_mut::() = PressPick { + hovered: Hovered { + target: Some(boar), + guid: Some(BOAR), + distance: 0.0, + ..Hovered::default() + }, + cursor: WorldCursor { + kind: cursor_mode::CursorKind::Attack, + unable: false, + }, + ..PressPick::default() + }; + world + .resource_mut::>() + .write(WorldRightClick); + world.run_system_once(act_on_right_click).unwrap(); + assert_eq!( + world.resource::().guid, + Some(BOAR), + "the release must act on the unit whose plate the press was over" + ); + } } diff --git a/crates/benilla-app/src/target/mod.rs b/crates/benilla-app/src/target/mod.rs index 3307633f..a33bcfe3 100644 --- a/crates/benilla-app/src/target/mod.rs +++ b/crates/benilla-app/src/target/mod.rs @@ -242,14 +242,30 @@ impl Default for PickOcclusion { /// /// Written by [`latch_press_pick`] at the head of the target chain, which runs **before** /// [`hover::update_hover`] can clear anything, so the values it copies are the pick as of the press. +/// +/// **Both buttons read it** (decision 2230). 1122 built this for the left button and left +/// [`click::act_on_right_click`] on the live hover, where it appeared to work only because the +/// release frame re-picks at the restored cursor and — for a body click that moved nothing — +/// lands back on the same unit. It is not the same pick: the plate case has no body under the +/// cursor to re-find, so a right-click on a V-plate acted on nothing at all. #[derive(Resource, Default, Clone, Copy)] pub(crate) struct PressPick { pub(crate) hovered: Hovered, pub(crate) object: HoveredObject, pub(crate) occlusion: PickOcclusion, - /// The cursor's Attack classification at the press — the reference's new-target validation - /// (`0x5ecb70`), which likewise reads the pick the gesture started on, not a live one. - pub(crate) attack: bool, + /// The **context cursor as of the press** — the classification the whole right-click ladder + /// forks on (Attack/Speak/Loot/the GameObject arms) plus its `unable` range gray, and the + /// reference's new-target validation (`0x5ecb70`) likewise reads the pick the gesture started + /// on, not a live one. Latched whole rather than as the single Attack bit it used to be, + /// because every one of those forks is the same frozen pick's answer. + pub(crate) cursor: cursor_mode::WorldCursor, +} + +impl PressPick { + /// Was the press an **Attack**-cursor press? `0x5ecb70`'s new-target validation. + pub(crate) fn attack(&self) -> bool { + self.cursor.kind == cursor_mode::CursorKind::Attack + } } /// Latch the frame's pick as a press begins — the [`PressPick`] writer. @@ -281,7 +297,7 @@ pub(crate) fn latch_press_pick( hovered: *hovered, object: *object, occlusion: *occlusion, - attack: cursor.kind == cursor_mode::CursorKind::Attack, + cursor: *cursor, }; } @@ -567,7 +583,7 @@ mod tests { .press(MouseButton::Left); world.run_system(id).unwrap(); assert_eq!(world.resource::().hovered.guid, Some(BOAR)); - assert!(world.resource::().attack, "Attack rode along"); + assert!(world.resource::().attack(), "Attack rode along"); // The drag begins: the look session blanks the live hover, every frame, for as long as it // lasts. The latch must not follow it down. diff --git a/crates/benilla-app/src/ui_battlefield_score.rs b/crates/benilla-app/src/ui_battlefield_score.rs index 2bbf16fa..9532ddc7 100644 --- a/crates/benilla-app/src/ui_battlefield_score.rs +++ b/crates/benilla-app/src/ui_battlefield_score.rs @@ -78,13 +78,12 @@ fn resolve_board( let (race, class) = names .player_traits(r.guid) .map_or((0, 0), |(race, class, _)| (race, class)); - // `0` Horde, `1` Alliance, `-1` neither — the reference's mask walk lands on the same - // three values (§6.2), and every playable race has a fixed side in 1.12. - let faction = match crate::ui_unit::race_faction_group(race) { - Some("Alliance") => 1, - Some(_) => 0, - None => -1, - }; + // `0` Horde, `1` Alliance, `-1` neither. `0x4aa200` walks the name-cache record's race + // through `ChrRaces` → `FactionTemplate` → factionGroupMask, which is `0x5efe00`'s walk + // with the null-row guards omitted — so it is [`crate::ui_unit::race_pvp_team`] and not a + // second copy of it. (It used to be one, re-derived here out of `race_faction_group`; a + // second copy of this walk is exactly what report B378 was, one surface over.) + let faction = i32::from(crate::ui_unit::race_pvp_team(race)); let mut stats = [0u32; 8]; for (slot, v) in stats.iter_mut().zip(&r.stats) { *slot = *v; diff --git a/crates/benilla-app/src/ui_honor.rs b/crates/benilla-app/src/ui_honor.rs index 764a757c..b98697b7 100644 --- a/crates/benilla-app/src/ui_honor.rs +++ b/crates/benilla-app/src/ui_honor.rs @@ -96,11 +96,19 @@ impl Plugin for UiHonorPlugin { /// Read the honor block off our own descriptor, or `None` while none of it has streamed. /// -/// **`None` and all-zeroes are different states and the difference is visible**: a fresh character -/// who has never fought has every counter at 0 *and* the fields present, while a player who has -/// only just entered the world may have none of them yet. The reference paints the second as -/// blank, not as zero, so the feed pushes nothing until at least one field arrives rather than -/// inventing a zeroed snapshot. +/// **`None` and all-zeroes are the same thing on screen, and that is the point of the gate, not a +/// flaw in it.** This doc used to say the reference "paints a player whose fields have not arrived +/// as blank, not as zero". It does not: `0x51a4b0`–`0x51a7c0`'s "absent → 0.0" tails are about the +/// absent player OBJECT, and once the object exists every one of those bindings reads a +/// descriptor array that is allocated and zeroed, so a field the server never sent reads `0` +/// (wow-re `honor-panel-law.md` §3.1–3.5). Our bindings answer the same zeros — +/// `honor(lua).unwrap_or_default()` — so what the gate actually decides is only *when the first +/// snapshot is pushed*, and with it when `PLAYER_PVP_KILLS_CHANGED`/`PLAYER_PVP_RANK_CHANGED` +/// first fire. It cannot make a row differ from the reference. +/// +/// That is worth stating because this gate was the other candidate cause of report B378, and it +/// is ruled out by exactly this: a missing snapshot and a zeroed one paint the same pane. The +/// cause was the rank title's team digit (decision 2227). fn honor_snapshot(store: &ObjectStore) -> Option { let f = &store.0; let session = f.player_session_kills(); diff --git a/crates/benilla-app/src/ui_party/feed.rs b/crates/benilla-app/src/ui_party/feed.rs index 1cca8e0e..2756b6ba 100644 --- a/crates/benilla-app/src/ui_party/feed.rs +++ b/crates/benilla-app/src/ui_party/feed.rs @@ -114,7 +114,7 @@ pub(super) fn feed_party( stores: Query<&ObjectStore>, changed_stores: Query<(), Changed>, mut removed_stores: RemovedComponents, - self_q: Query<(&Guid, &ObjectStore), With>, + self_q: Query<(Entity, &Guid, &ObjectStore), With>, factions: Option>, names: Res, areas: Option>, @@ -152,7 +152,20 @@ pub(super) fn feed_party( let area_moved = fed.area.moved(here.area().map_or(u64::MAX, u64::from)); let group_changed = group.is_changed(); let index_changed = index.is_changed(); - let stores_changed = !changed_stores.is_empty(); + // The members' descriptors and our own — the only stores the merged view below reads. This + // used to be "any store in the world moved", which in a crowd is true on every frame: the + // crowd profile of 2225 read this feed re-pushing a 39-member raid to the VM every frame of + // the pin, 0.47 ms traced, for rows that had not changed. + let stores_changed = self_q + .iter() + .next() + .is_some_and(|(e, _, _)| changed_stores.get(e).is_ok()) + || group.members.iter().any(|m| { + index + .0 + .get(&m.guid) + .is_some_and(|&e| changed_stores.get(e).is_ok()) + }); let stores_removed = !removed_stores.is_empty(); let factions_changed = factions.as_ref().is_some_and(|r| r.is_changed()); let areas_changed = areas.as_ref().is_some_and(|r| r.is_changed()); @@ -189,12 +202,12 @@ pub(super) fn feed_party( return; } let self_pair = self_q.iter().next(); - let self_guid = self_pair.map(|(g, _)| g.0); + let self_guid = self_pair.map(|(_, g, _)| g.0); // The party's PvP faction group (decision 0646 §1): our own. A 1.12 party is always one // faction, and a member out of streaming range has no descriptor to resolve one from — so // reading it off ourselves is exact for every member, present or not. - let own_group = - self_pair.and_then(|(_, store)| crate::ui_unit::faction_group(store, factions.as_deref())); + let own_group = self_pair + .and_then(|(_, _, store)| crate::ui_unit::faction_group(store, factions.as_deref())); // The party1..4 slot view: own-subgroup members, packet order (`GroupState::party_slots`, // the 0440 byte law) — in a plain party this is simply the roster. @@ -257,7 +270,7 @@ pub(super) fn feed_party( }; // The raid roster — `GetNumRaidMembers`/`GetRaidRosterInfo`/`UnitInRaid` all read this one // list, so the count can never disagree with the array the way it would if we kept both. - let me = self_pair.map(|(g, store)| RaidSelf { + let me = self_pair.map(|(_, g, store)| RaidSelf { guid: g.0, flags: group.own_flags, level: store.0.unit_level().unwrap_or(0), @@ -372,7 +385,7 @@ pub(super) fn feed_party( for (i, token) in RAID_TOKENS.iter().enumerate() { let snap = raid_guids.get(i).and_then(|guid| { if Some(*guid) == self_guid { - let (_, store) = self_pair?; + let (_, _, store) = self_pair?; let name = names.peek(*guid).map(str::to_string); let mut s = crate::ui_unit::snapshot(store, name, 0, chr); s.is_player = true; diff --git a/crates/benilla-app/src/ui_pass.rs b/crates/benilla-app/src/ui_pass.rs index 0def3b7e..74efcf53 100644 --- a/crates/benilla-app/src/ui_pass.rs +++ b/crates/benilla-app/src/ui_pass.rs @@ -578,6 +578,8 @@ impl Plugin for PlayerUiPlugin { Material2dPlugin::::default(), // Owned here, not in main.rs: the lane's decode is not optional (see its doc). crate::ui_gamma::UiGammaPlugin, + // The 2D opaque pass skipped when empty — which on this camera is always (2225). + crate::opaque2d::SkipEmptyOpaque2dPlugin, )) .add_systems(Startup, (spawn_ui_camera, init_white_texture)) .configure_sets( diff --git a/crates/benilla-app/src/ui_script/honor_frame_tests.rs b/crates/benilla-app/src/ui_script/honor_frame_tests.rs index 3a303d0c..845c3885 100644 --- a/crates/benilla-app/src/ui_script/honor_frame_tests.rs +++ b/crates/benilla-app/src/ui_script/honor_frame_tests.rs @@ -96,6 +96,11 @@ fn alliance_player() -> UnitState { level: 60, sex: 2, faction_group: Some("Alliance".into()), + // The rank title's team digit is `pvp_team` (`0x5efe00`: race → ChrRaces → + // FactionTemplate), NOT `faction_group` (`UnitFactionGroup`: the live template). Human is + // race 1 → Alliance → 1. Both are seated because a real Alliance player has both; the GM + // test below is what drives them apart. + pvp_team: crate::ui_unit::race_pvp_team(1), race: Some("Human".into()), race_file: Some("Human".into()), class: Some("Warrior".into()), @@ -206,6 +211,68 @@ fn an_unranked_character_reads_none_and_shows_no_badge() { assert!(s.errors().is_empty(), "script errors: {:?}", s.errors()); } +/// **Report B378 — a GM-flagged Grand Marshal's Highest Rank read `None`.** The regression test +/// for decision 2227, and the one that holds the two sides apart. +/// +/// vmangos forces a GM to faction template 35, whose group mask is 0, so `UnitFactionGroup` +/// genuinely answers nil — that half is faithful and the PvP icon is *supposed* to vanish with it. +/// The rank title is not on that wire at all: `0x5efe00` reads the unit's RACE through +/// `ChrRaces`/`FactionTemplate` (`[obj+0x110]+0x78`), and a GM's race does not change, which is why +/// the reporter's own 1.12 client on the same server read "Grand Marshal" from the same byte. We +/// had the title keyed off the faction group, so both rows went to NONE together. +/// +/// The control is the pair: the two rank rows must name the rank **and** the bar must still take +/// the sideless arm's Horde red — a fix that "restored the side" would pass the first half and +/// silently break the second. Skips without client data. +#[test] +fn a_gm_flagged_player_keeps_his_rank_title_and_loses_only_the_faction_group() { + let _data = benilla_formats::wow_data_or_skip!(); + let mut s = shown_honor_page(); + s.set_honor(Some(HonorState { + // Grand Marshal — internal 18, the top of the Alliance list. + rank: 18, + highest_rank: 18, + ..state() + })); + s.set_unit( + "player", + Some(UnitState { + // `.gm on`: template 35, group mask 0 → no side. + faction_group: None, + faction_group_localized: None, + pvp_rank: 18, + ..alliance_player() + }), + ); + s.run("HonorFrame_Update(1)").unwrap(); + s.resolve(); + assert_eq!( + text(&mut s, "HonorFrameLifeTimeRankValue"), + "Grand Marshal", + "the Highest Rank row — the reported symptom" + ); + assert_eq!( + text(&mut s, "HonorFrameCurrentPVPTitle"), + "Grand Marshal", + "and the title under the badge, which reads the same key" + ); + assert!( + s.eval::(r#"return UnitFactionGroup("player") == nil"#) + .unwrap(), + "the side itself is still gone — that half of GM mode is the reference's" + ); + // The control: the bar's colour is `UnitFactionGroup`'s, so a sideless player takes the + // `else` arm (Horde red, `HonorFrame.lua:68`) and must KEEP taking it. + let (r, g, b) = s + .eval::<(f64, f64, f64)>("return HonorFrameProgressBar:GetStatusBarColor()") + .unwrap(); + assert!( + r > g && r > b, + "a sideless player keeps the else-arm's red bar, got ({r}, {g}, {b})" + ); + assert!(s.errors().is_empty(), "script errors: {:?}", s.errors()); +} + /// The bar takes the 0..1 fraction straight, and wears the player's own faction colour. Alliance /// navy here; the `else` arm is Horde red for everyone else, including a unit whose side has not /// resolved — a bar with no colour at all would read as a broken pane. @@ -298,11 +365,11 @@ fn the_real_global_strings_name_the_rank() { load_page(&s); s.set_honor(Some(state())); - for (group, want) in [("Alliance", "Knight-Captain"), ("Horde", "Legionnaire")] { + for (team, want) in [(1i8, "Knight-Captain"), (0, "Legionnaire")] { s.set_unit( "player", Some(UnitState { - faction_group: Some(group.into()), + pvp_team: team, ..alliance_player() }), ); @@ -312,7 +379,7 @@ fn the_real_global_strings_name_the_rank() { assert_eq!( text(&mut s, "HonorFrameCurrentPVPTitle"), want, - "rank 12 to the {group}" + "rank 12 to team {team}" ); } assert!(s.errors().is_empty(), "script errors: {:?}", s.errors()); diff --git a/crates/benilla-app/src/ui_unit.rs b/crates/benilla-app/src/ui_unit.rs index 76fac8cb..5fe2f4be 100644 --- a/crates/benilla-app/src/ui_unit.rs +++ b/crates/benilla-app/src/ui_unit.rs @@ -605,6 +605,42 @@ pub(crate) fn race_faction_group(race: u8) -> Option<&'static str> { }) } +/// A unit's **PvP team digit** — `0x5efe00`'s tri-state: `0` Horde, `1` Alliance, `-1` no side. +/// +/// **This is NOT [`faction_group`], and the difference is the whole of report B378.** The two read +/// different sources and only agree while nothing has moved a unit off its racial faction: +/// +/// * `UnitFactionGroup` (`0x516630`) reads the unit's LIVE `UNIT_FIELD_FACTIONTEMPLATE` +/// (`0x5166b8 mov eax,[eax+0x110]` / `0x5166be mov eax,[eax+0x74]`, byte-read here) — so a +/// vmangos GM, forced to template 35, genuinely has no side and the PvP flag icon genuinely +/// hides. That is faithful. +/// * The rank title's team digit (`0x5efe00`) reads the unit's **RACE** and walks +/// `[obj+0x110]+0x78` → `ChrRaces.dbc` field 2 (FactionTemplate id) → `FactionTemplate.dbc` +/// field 3 (factionGroupMask) → `& 4` ⇒ 0, else `& 2` ⇒ 1, else −1 — never the live template. +/// A GM's race does not change, so the reference names his rank exactly as it always did. +/// +/// We had the second wired to the first, which is why a Grand Marshal's Honor tab read `NONE` on +/// a GM-flagged account while the 1.12 client on the same server read "Grand Marshal" +/// (decision 2227). Every `0x5efe00` caller is race-derived: `GetPVPRankInfo`'s team +/// (`0x51a9af`/`0x51a9c8`), `UnitPVPName`'s rank decoration (`0x5efe60`), the battlefield +/// scoreboard's per-row side (`0x4aa200`, which inlines the same walk off the name-cache record). +/// +/// The table is the shipped one, frozen: `ChrRaces.dbc` has **nine** rows in 5875 and race 9 +/// (Goblin, unplayable) shares Human's faction template 1, so it answers Alliance — not `None`, +/// which is what a "playable races only" table would say. +/// [`tests::race_pvp_team_matches_the_shipped_tables`] walks the real DBCs and pins every row. +pub(crate) fn race_pvp_team(race: u8) -> i8 { + match race { + // factionGroupMask 3 = Player|Alliance → `& 4` clear, `& 2` set. + 1 | 3 | 4 | 7 | 9 => 1, + // factionGroupMask 5 = Player|Horde → `& 4` set, tested first. + 2 | 5 | 6 | 8 => 0, + // No `ChrRaces` row (a creature's race byte, or an unstreamed descriptor): the engine's + // bounds/NULL failure tail, `-1`. It formats into the key and matches no GlobalString. + _ => -1, + } +} + /// Resolve a UnitPopup unit token to the **player guid** it names — `"target"` through the /// selection iff it really is a player (the target frame's PLAYER menu), a `"partyN"` token through /// the roster (the party frame's PARTY menu). `"player"` (yourself) and any unresolved token answer @@ -1004,6 +1040,13 @@ pub(crate) fn snapshot( // pane's `UnitPVPRank("target")` can answer at all. A creature has no PLAYER block and // reads 0, the reference's own answer for one. pvp_rank: store.0.player_pvp_rank().unwrap_or(0), + // `0x5efe00`'s team digit — the second `%d` of `PVP_RANK__`. It sits here + // beside the rank byte for the same reason that one does (nothing but the descriptor is + // needed) and it reads the RACE, not `faction_group`: the engine walks the race through + // `ChrRaces`/`FactionTemplate` and never looks at the live `UNIT_FIELD_FACTIONTEMPLATE` + // this unit is carrying, so a GM-flagged player keeps his rank title while losing the PvP + // icon. See [`race_pvp_team`] and decision 2227 (report B378). + pvp_team: store.0.unit_race().map_or(-1, race_pvp_team), // `PLAYER_BYTES_3` byte 2 — the city-protector title, the same PUBLIC dword as the rank // byte above. `UnitPVPName` appends a `PVP_MEDAL` line for a non-zero one; 0 is "no // medal", which is every character on this server (vmangos never writes the byte). @@ -1556,20 +1599,22 @@ fn feed_units( // being broken, which has now cost two separate sessions an investigation. So it is a BENCH // diagnostic, not UI: nothing appears on screen, exactly as in the reference. // - // The honor arc (1512) put a SECOND surface behind this same side: a rank's title is the - // GlobalString `PVP_RANK__`, and with no side there is no team digit, so - // `GetPVPRankInfo` answers nil and the Honor tab renders `NONE` at every rank — for a Grand - // Marshal. That reads exactly like an unbuilt pane, which is why it is named in the warning - // rather than left for the next investigation to rediscover. + // The honor arc (1512) was once listed here as a SECOND surface behind this same side, and + // it is not one: the rank title's team digit is `0x5efe00`, which reads the RACE through + // `ChrRaces`/`FactionTemplate` and never the live template, so a GM's Honor tab names his + // rank exactly as it always did (`race_pvp_team`, decision 2227 — the cause of report B378 + // was that we had wired the two together, not the GM mode itself). What this warning still + // covers is every genuinely template-derived surface: `UnitFactionGroup` and the icons and + // comparisons built on it. if let Some(p) = &player { let sideless = p.faction_group.is_none(); if sideless && !feed.warned_sideless { warn!( "faction: our own template names no side (usually GM mode — vmangos forces \ - template 35, group mask 0). Faction-derived UI cannot resolve a side while this \ - holds: the PvP flag icon stays hidden however flagged you are, and the Honor tab's rank \ - title reads NONE at every rank (PVP_RANK__ has no team digit). \ - `.gm off` restores both." + template 35, group mask 0). Every UnitFactionGroup-derived surface loses its \ + side while this holds — the PvP flag icon stays hidden however flagged you are. \ + `.gm off` restores it. (The Honor tab's rank title is NOT one of these: its team \ + digit comes from your race, not your template.)" ); } feed.warned_sideless = sideless; @@ -2158,6 +2203,89 @@ fn combo_edge(last: Option<(u8, u64)>, now: (u8, u64)) -> Option { mod tests { use super::*; + /// The DESCRIPTOR leg of the same answer: `snapshot` takes the team digit off + /// `UNIT_FIELD_BYTES_0` byte 0 and **never** off `UNIT_FIELD_FACTIONTEMPLATE`. + /// + /// The pane-level regression (`ui_script::honor_frame_tests::a_gm_flagged_player_…`) seats + /// `pvp_team` by hand, so it proves the key is built from the right field and not that the + /// right field is read. This is that half: a template-35 GM — the exact descriptor vmangos + /// gives one — still answers his race's side. + #[test] + fn the_team_digit_comes_off_the_race_byte_not_the_faction_template() { + use benilla_protocol::ObjectFields; + /// `UNIT_FIELD_FACTIONTEMPLATE` / `UNIT_FIELD_BYTES_0`, one dword apart — the reference's + /// own `[obj+0x110]+0x74` and `+0x78`. + const FACTIONTEMPLATE: u16 = 35; + const BYTES_0: u16 = 36; + /// vmangos's GM template: `FactionTemplate.dbc` group mask 0, friendly to everyone. + const GM_TEMPLATE: u32 = 35; + + let team = |fields: &[(u16, u32)]| { + snapshot( + &ObjectStore(ObjectFields::from_pairs(fields)), + None, + 0, + None, + ) + .pvp_team + }; + // Byte 0 of BYTES_0 is the race; the class in byte 1 must not disturb it. + let human_warrior = 1 | (1 << 8); + let scourge_mage = 5 | (8 << 8); + assert_eq!(team(&[(BYTES_0, human_warrior)]), 1, "Human → Alliance"); + assert_eq!(team(&[(BYTES_0, scourge_mage)]), 0, "Scourge → Horde"); + // **The report.** The sideless GM template sits right beside the race byte and is not + // consulted: the answer is the race's, unchanged. + assert_eq!( + team(&[(BYTES_0, human_warrior), (FACTIONTEMPLATE, GM_TEMPLATE)]), + 1, + "a GM keeps his race's side (report B378)" + ); + assert_eq!( + team(&[(BYTES_0, scourge_mage), (FACTIONTEMPLATE, GM_TEMPLATE)]), + 0, + "…on both sides" + ); + // A unit whose race byte has not streamed is the engine's bounds-failure −1, and a + // faction template alone cannot stand in for it. + assert_eq!(team(&[]), -1, "no race byte, no team digit"); + assert_eq!( + team(&[(FACTIONTEMPLATE, 1)]), + -1, + "and a template is not one" + ); + } + + /// [`race_pvp_team`]'s frozen table against the **shipped tables it is a copy of** — the walk + /// the engine runs at `0x5efe00`, on the real `ChrRaces.dbc` and `FactionTemplate.dbc`. + /// + /// The table is hardcoded because it is nine constant rows of a 2006 file and threading a DBC + /// resource through every unit snapshot to read them would be pure ceremony. This is what + /// makes that safe: the file decides, and a row that ever disagrees — or a race the file has + /// and the table does not (race 9, Goblin, which shares Human's template and is **not** a + /// `None`) — fails here. Skips without client data. + #[test] + fn race_pvp_team_matches_the_shipped_tables() { + let data = benilla_formats::wow_data_or_skip!(); + let mut chain = benilla_formats::open_chain(&data).expect("open chain"); + let want = benilla_formats::load_race_pvp_teams(&mut chain).expect("ChrRaces walk"); + // The misparse guard: 5875 ships nine rows, not eight. An empty or truncated map would + // otherwise let this test pass by asserting nothing. + assert_eq!(want.len(), 9, "ChrRaces.dbc row count"); + for (&race, &team) in &want { + assert_eq!(race_pvp_team(race), team, "race {race}"); + } + // Both sides are actually represented — a walk that answered one digit for everything + // would satisfy the loop above and name every rank off one list. + assert!(want.values().any(|&t| t == 0), "some race is Horde"); + assert!(want.values().any(|&t| t == 1), "some race is Alliance"); + // Off the end of the file is the engine's bounds-failure tail, not a guess. + for race in [0u8, 10, 255] { + assert!(!want.contains_key(&race)); + assert_eq!(race_pvp_team(race), -1, "race {race} has no ChrRaces row"); + } + } + /// **The tapped bit fires `UNIT_FACTION`, and without this the verbs are decorative.** /// /// `UnitIsTapped` answering correctly is only half of it. pfUI's grey-bar branch lives inside diff --git a/crates/benilla-formats/src/lib.rs b/crates/benilla-formats/src/lib.rs index 2d334618..03f299ff 100644 --- a/crates/benilla-formats/src/lib.rs +++ b/crates/benilla-formats/src/lib.rs @@ -307,6 +307,9 @@ mod text_filter_lists; pub use text_filter_lists::{load_chat_profanity, load_spam_messages, FilterPattern}; mod race_sound; pub use race_sound::{load_exploration_sound_catalog, ExplorationSoundCatalog}; + +mod race_pvp_team; +pub use race_pvp_team::load_race_pvp_teams; mod zone_map; pub use zone_map::{load_zone_map, ZONE_MAP_EDGE}; diff --git a/crates/benilla-formats/src/race_pvp_team.rs b/crates/benilla-formats/src/race_pvp_team.rs new file mode 100644 index 00000000..23dfbe7c --- /dev/null +++ b/crates/benilla-formats/src/race_pvp_team.rs @@ -0,0 +1,76 @@ +//! `ChrRaces.dbc` → the **PvP team digit** — `0x5efe00`, the second `%d` of +//! `PVP_RANK__`. +//! +//! The engine never asks a unit what faction template it is *currently* carrying for this; it asks +//! what RACE it is, and walks the race's own table: +//! +//! ```text +//! 5efe06 movzx eax, BYTE PTR [[obj+0x110]+0x78] ; UNIT_FIELD_BYTES_0 byte 0 = race +//! -> ChrRaces [0xc0dee0][race] +0x08 ; field 2 = FactionTemplate id +//! -> FactionTemplate [0xc0dd3c][id] +0x0c ; field 3 = factionGroupMask +//! 5efe42 test al,4 ; je ... ; mask & 4 -> 0 (Horde) +//! 5efe49 and al,2 ; … ; mask & 2 -> 1 (Alliance), else -1 +//! 5efe54 (any bound/NULL failure) -> -1 +//! ``` +//! +//! (wow-re `system/ui/scratch/honor-panel-law.md` §3.11, VERIFIED; the `0`/`1` assignment is +//! settled there against the shipped `FactionGroup.dbc` and `GlobalStrings.lua`, not inferred.) +//! +//! **This is not `UnitFactionGroup`**, which reads the live `UNIT_FIELD_FACTIONTEMPLATE` +//! (`0x5166b8`/`0x5166be`) and so genuinely loses its side under GM mode. Confusing the two is +//! report B378 / decision 2227. +//! +//! Loaded only by [`crate::race_pvp_team::load_race_pvp_teams`]'s one consumer — the test that +//! pins `ui_unit::race_pvp_team`'s frozen table to the shipped tables. The runtime answer is that +//! table: nine constant rows of a 2006 file are not worth a DBC resource threaded through every +//! unit snapshot, but they are worth a test that fails if the file ever disagrees. + +use std::collections::HashMap; + +use anyhow::{Context, Result}; +use benilla_dbc::{FieldType, Schema, SchemaField}; + +use crate::dbc::{parse, u32_at}; +use crate::Chain; + +const CHR_RACES: &str = "DBFilesClient\\ChrRaces.dbc"; + +/// Every `ChrRaces.dbc` row's team digit, by race id — `0` Horde, `1` Alliance, `-1` no side. +/// +/// The map holds one entry per row the file actually has (nine in 5875, ids 1–9); a race id with +/// no row is the engine's `-1` tail and is simply absent here. +pub fn load_race_pvp_teams(chain: &mut Chain) -> Result> { + let bytes = chain + .read_file(CHR_RACES) + .with_context(|| format!("reading {CHR_RACES}"))?; + // 29 columns in 5875; the string columns are declared here only so the field count matches — + // this loader reads none of them. Same schema as `crate::characters`' creation catalog. + let mut schema = Schema::new("ChrRaces"); + for i in 0..29 { + let ty = match i { + 15 | 26 | 27 | 28 => FieldType::String, + _ => FieldType::UInt32, + }; + schema.add_field(SchemaField::new(format!("f{i}"), ty)); + } + let rs = parse(&bytes, schema, "ChrRaces")?; + let factions = crate::load_faction_catalog(chain)?; + let mut out = HashMap::new(); + for r in rs.records() { + let (Some(race), Some(template)) = (u32_at(r, 0), u32_at(r, 2)) else { + continue; + }; + let Ok(race) = u8::try_from(race) else { + continue; + }; + // `0x5efe42`'s order: the Horde bit is tested FIRST, so a hypothetical row carrying both + // side bits would read Horde. Reproduced rather than tidied. + let team = match factions.template(template).map(|t| t.group_mask) { + Some(m) if m & 4 != 0 => 0, + Some(m) if m & 2 != 0 => 1, + _ => -1, + }; + out.insert(race, team); + } + Ok(out) +} diff --git a/crates/benilla-ui/src/script/pvp/mod.rs b/crates/benilla-ui/src/script/pvp/mod.rs index 9b1ba805..e4284926 100644 --- a/crates/benilla-ui/src/script/pvp/mod.rs +++ b/crates/benilla-ui/src/script/pvp/mod.rs @@ -348,22 +348,22 @@ fn global_string(lua: &Lua, key: &str) -> Option { } /// A unit's PvP **team digit** — `0x5efe00`'s tri-state: `0` Horde, `1` Alliance, `-1` for a unit -/// with no side (a neutral/monster faction template, or a race the tables can't resolve). +/// whose race resolves to no side. /// /// The engine walks race → `ChrRaces` → `FactionTemplate` and tests `[rec+0xc]`'s factionGroupMask -/// `& 4` → 0, `& 2` → 1, else −1. This crate holds no DBC (decision 0068 §3), so the same answer -/// arrives pre-resolved on the snapshot as [`UnitState::faction_group`](super::UnitState) — the -/// app resolves it from `UNIT_FIELD_FACTIONTEMPLATE` through the same mask. Different hop, same -/// output space (the credit line's own inline copy at `0x62530a` reads the template one hop -/// earlier than `0x5efe00` does and lands in the same three values). +/// `& 4` → 0, `& 2` → 1, else −1. This crate holds no DBC (decision 0068 §3), so the walk is the +/// app's and the answer arrives pre-resolved as [`UnitState::pvp_team`](super::UnitState). +/// +/// **It is not [`UnitState::faction_group`](super::UnitState), which this used to read.** That +/// field is `UnitFactionGroup`'s — the unit's LIVE `UNIT_FIELD_FACTIONTEMPLATE` — and the two +/// part company the moment anything moves a unit off its racial faction. A vmangos GM is forced +/// to template 35 (group mask 0), so every rank title in the Honor tab read `NONE` for a Grand +/// Marshal while the 1.12 client on the same server read "Grand Marshal" off his unchanged race: +/// report B378, decision 2227. /// /// −1 needs no special case: it formats into the key, and `PVP_RANK_9_-1` matches no GlobalString. fn team_of(u: &super::UnitState) -> i64 { - match u.faction_group.as_deref() { - Some("Horde") => 0, - Some("Alliance") => 1, - _ => -1, - } + i64::from(u.pvp_team) } /// `GetPVPRankInfo`'s **second argument** → the team digit, the engine's three-way dispatch @@ -399,11 +399,11 @@ fn team_arg(lua: &Lua, v: Value) -> mlua::Result { .and_then(|t| model.unit(t)) // `0x51a9af`'s `shr edx,4; test dl,1` player gate. Applied here, unlike in `UnitPVPRank` // (see there), because on this arm nothing else can tell a creature from a player: a - // PvP-flagged city guard's faction template carries Player| too, so its - // `faction_group` would otherwise name a side the engine answers 0 for. The cost is the - // mirror-image miss — an un-enriched player snapshot reads as a non-player — which lands - // on a different title rather than on none, and this arm is addon surface: neither pane - // passes a second argument at all. + // creature carries a race byte and a faction template like anything else, and the two + // misses are different numbers anyway (this gate's 0 against `0x5efe00`'s −1) — it is + // this gate the engine reaches first. The cost is the mirror-image miss — an un-enriched player snapshot reads as a non-player — which + // lands on a different title rather than on none, and this arm is addon surface: neither + // pane passes a second argument at all. .filter(|u| u.is_player) .map_or(0, team_of)) } diff --git a/crates/benilla-ui/src/script/pvp/tests.rs b/crates/benilla-ui/src/script/pvp/tests.rs index 697114ec..11bc8bf7 100644 --- a/crates/benilla-ui/src/script/pvp/tests.rs +++ b/crates/benilla-ui/src/script/pvp/tests.rs @@ -64,7 +64,12 @@ fn player() -> UnitState { name: Some("Benilla".into()), sex: 3, // female is_player: true, + // Both, because a real Alliance player has both — and they are not the same field. + // `faction_group` is `UnitFactionGroup`'s live faction template; `pvp_team` is + // `0x5efe00`'s race walk, which is what every rank title is keyed by. The GM-mode test + // below is the one that drives them apart. faction_group: Some("Alliance".into()), + pvp_team: 1, pvp_rank: 9, ..Default::default() } @@ -337,6 +342,7 @@ fn the_pane_title_is_keyed_by_team_and_is_never_gendered() { Some(UnitState { sex: 2, faction_group: Some("Horde".into()), + pvp_team: 0, ..player() }), ); @@ -417,7 +423,7 @@ fn a_missing_rank_global_reads_nil_not_empty() { s.set_unit( "player", Some(UnitState { - faction_group: None, + pvp_team: -1, ..player() }), ); @@ -425,6 +431,59 @@ fn a_missing_rank_global_reads_nil_not_empty() { assert!(s.eval::("return GetPVPRankInfo(9) == nil").unwrap()); } +/// **The team digit is NOT the faction group** — report B378 at the binding level (decision 2227). +/// +/// `UnitFactionGroup` reads the unit's live `UNIT_FIELD_FACTIONTEMPLATE` (`0x516630`) and the rank +/// title's team digit reads the unit's RACE (`0x5efe00`), so a vmangos GM — template 35, group +/// mask 0 — has no side and keeps his rank. Both `0x5efe00` surfaces are asserted here: the +/// binding's key and `UnitPVPName`'s decoration (`0x5efe60`, the same walk). +#[test] +fn a_sideless_player_still_has_a_team_digit_because_his_race_has_one() { + let mut s = seated(); + seat_rank_globals(&s); + s.lua() + .globals() + .set("UNIT_PVP_NAME", "%s %s") + .expect("global"); + s.set_unit( + "player", + Some(UnitState { + // `.gm on` — the template names nothing … + faction_group: None, + faction_group_localized: None, + // … and the race still names Alliance. + pvp_team: 1, + sex: 2, + pvp_rank: 18, + ..player() + }), + ); + assert_eq!( + s.eval::("return (GetPVPRankInfo(18))").unwrap(), + "Grand Marshal", + "the sideless template must not reach the key" + ); + assert_eq!( + s.eval::(r#"return UnitPVPName("player")"#).unwrap(), + "Grand Marshal Benilla", + "and the name decoration reads the same digit" + ); + // The control: a unit whose RACE resolves to nothing is still −1, and −1 still misses. + s.set_unit( + "player", + Some(UnitState { + faction_group: Some("Alliance".into()), + pvp_team: -1, + pvp_rank: 18, + ..player() + }), + ); + assert!( + s.eval::("return GetPVPRankInfo(18) == nil").unwrap(), + "a side on the template cannot stand in for a missing team digit either" + ); +} + /// The internal→visual conversion across its whole range — **and the negative half runs the /// opposite way from the server's `visualRank`**, which is the assertion that matters: rank 1 is /// −4 and rank 4 is −1, because `0x51aa38` subtracts 5 rather than negating. vmangos's @@ -520,6 +579,7 @@ fn get_pvp_rank_info_takes_a_second_argument_three_different_ways() { name: Some("Thrall".into()), is_player: true, faction_group: Some("Horde".into()), + pvp_team: 0, pvp_rank: 14, ..Default::default() }), @@ -575,7 +635,7 @@ fn get_pvp_rank_info_takes_a_second_argument_three_different_ways() { Some(UnitState { exists: true, name: Some("Timber Wolf".into()), - faction_group: Some("Alliance".into()), + pvp_team: 1, ..Default::default() }), ); @@ -593,7 +653,7 @@ fn get_pvp_rank_info_takes_a_second_argument_three_different_ways() { Some(UnitState { exists: true, is_player: true, - faction_group: None, + pvp_team: -1, ..Default::default() }), ); @@ -643,6 +703,7 @@ fn unit_pvp_rank_answers_for_a_foreign_unit() { name: Some("Thrall".into()), is_player: true, faction_group: Some("Horde".into()), + pvp_team: 0, pvp_rank: 14, ..Default::default() }), diff --git a/crates/benilla-ui/src/script/unit/mod.rs b/crates/benilla-ui/src/script/unit/mod.rs index c5d21272..22796bfe 100644 --- a/crates/benilla-ui/src/script/unit/mod.rs +++ b/crates/benilla-ui/src/script/unit/mod.rs @@ -341,6 +341,24 @@ pub struct UnitState { /// return, which stock uses as display text (`PlayerFrame.lua`'s PvP hit-area tooltip title). /// Never interchangeable with the English half above. pub faction_group_localized: Option, + /// The unit's **PvP team digit** — `0x5efe00`'s tri-state: `0` Horde, `1` Alliance, `-1` a + /// unit with no side. The second `%d` of `PVP_RANK__`, and **not** + /// [`Self::faction_group`] restated. + /// + /// The two answer different questions and the difference is a shipped bug's whole cause + /// (report B378, decision 2227): `UnitFactionGroup` reads the unit's LIVE + /// `UNIT_FIELD_FACTIONTEMPLATE` (`0x5166b8`/`0x5166be`), while every rank-title surface reads + /// the unit's **RACE** and walks `ChrRaces` → `FactionTemplate` → factionGroupMask + /// (`0x5efe00`, `[obj+0x110]+0x78`). A vmangos GM is forced to template 35 and so genuinely + /// loses the PvP flag icon — and keeps his rank title, because his race did not move. Wiring + /// this to the faction group instead read `NONE` at every rank for a Grand Marshal. + /// + /// **`Default` is `0`, and that is the reference's answer too, not a placeholder.** A literal + /// snapshot is one the object manager could not resolve (the out-of-range roster leg), and + /// `GetPVPRankInfo`'s team register is left at its initial `0` on exactly that edge + /// (`0x51a9af`'s lookup failing) — so a roster-only unit is named off the Horde list on both + /// clients. Every snapshot built from a live descriptor fills it (`ui_unit::snapshot`). + pub pvp_team: i8, /// The unit's GUID (`OBJECT_FIELD_GUID`) — the identity the cross-token predicates compare /// (`UnitIsUnit`, `UnitInParty`; decision 0434 §5's popup gating). `0` = the app's feed didn't /// resolve one; two zero guids never compare equal.