From 0807144f38fced01eb23881160cf7bd7f14e2a0c Mon Sep 17 00:00:00 2001 From: grom Date: Sat, 17 Oct 2026 13:41:35 -0600 Subject: [PATCH] modelframe: a unit pane holds the unit it was given MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `SetUnit` resolved nothing: it stored the token and the extract re-read it every paint. unitscan's alert is the live consumer — it `SetUnit'target'`s the rare it just found by name, and the addon restores the player's own target on the very next line — so the card showed the rare for one frame and then went empty. It only looked right while our by-name targeting was leaving the target on the rare, which is the accident the previous commit removed. The reference resolves the token in `SetUnit`'s own rebuild worker (`0x505b50`, the one `RefreshUnit` also runs) and nothing re-resolves it per paint. Three things say so independently: FrameXML re-issues `SetUnit` from `PaperDollFrame_OnShow` and on `UNIT_MODEL_CHANGED`, which would be pointless against a live token; `RefreshUnit` exists at all; and pfUI detects a portrait change with *no event* by pinning a scratch pane and comparing `GetModel` against the shown one (`api/unitframes.lua:2069`), a test that says nothing unless a pane holds what it was given. `ModelState::unit_guid` is that pin, read through one helper (`Model::unit_pane_subject`) so the paint gate and the extract cannot disagree about whether a pane has a subject. A pane seated *before* its unit streamed in keeps re-reading its token — ours arrives asynchronously where the reference's is already in the object manager — and `RefreshUnit` re-pins, as its worker does. `modelframe.rs`'s own extract test passed only because its `SetUnit` ran with no target; its comment claimed the general rule, so it now states the real one and pins both halves. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SgstFWtn1RxEJ5kVrpgYwn --- crates/benilla-ui/src/script/extract.rs | 14 ++-- crates/benilla-ui/src/script/model.rs | 15 ++++ crates/benilla-ui/src/script/modelframe.rs | 32 ++++++-- .../src/script/tests/model_clock.rs | 78 +++++++++++++++++++ .../benilla-ui/src/script/tests/modelframe.rs | 28 +++++-- crates/benilla-ui/src/script/tick.rs | 15 ++-- crates/benilla-ui/src/widget/kinds/mod.rs | 18 +++++ 7 files changed, 174 insertions(+), 26 deletions(-) diff --git a/crates/benilla-ui/src/script/extract.rs b/crates/benilla-ui/src/script/extract.rs index b0ace18e..9148601a 100644 --- a/crates/benilla-ui/src/script/extract.rs +++ b/crates/benilla-ui/src/script/extract.rs @@ -94,14 +94,14 @@ impl UiScript { name: frame.and_then(|f| f.name.clone()), model: m.path.clone(), skins: m.skins.clone(), - // Resolved here, where the token grammar lives - // ([`super::types::UnitPaneSubject`]); a token naming nothing right - // now leaves the pane with no subject, which empties its booth. - unit: m.unit.as_deref().and_then(|token| { - model.unit(token).map(|u| super::types::UnitPaneSubject { - guid: u.guid, + // The guid `SetUnit` pinned, or — for a pane seated before its unit + // existed — its token re-read here, where the token grammar lives + // ([`super::types::UnitPaneSubject`]). No subject empties the booth. + unit: model.unit_pane_subject(m).map(|guid| { + super::types::UnitPaneSubject { + guid, camera: m.unit_camera, - }) + } }), facing: m.facing, model_scale: m.scale, diff --git a/crates/benilla-ui/src/script/model.rs b/crates/benilla-ui/src/script/model.rs index 9a42c5c7..9d6376f2 100644 --- a/crates/benilla-ui/src/script/model.rs +++ b/crates/benilla-ui/src/script/model.rs @@ -1403,6 +1403,21 @@ impl Model { self.warnings.push(msg); } + /// A model pane's subject: the guid its `SetUnit` pinned ([`crate::widget::ModelState::unit_guid`]), + /// else its token re-read, for a pane seated before its unit streamed in. + /// + /// One reader for the paint gate and the extract, so the two cannot disagree about whether the + /// pane has anything to draw. `Some(0)` is a token naming a unit with no guid, which only a + /// harness feed produces; the app resolves it to no entity, as it always has. + pub(crate) fn unit_pane_subject(&self, m: &crate::widget::ModelState) -> Option { + m.unit_guid.or_else(|| { + m.unit + .as_deref() + .and_then(|token| self.unit(token)) + .map(|u| u.guid) + }) + } + /// A unit token's state, folded as 1.12's resolver `0x515970` folds (`_strnicmp`: `A`..`Z` /// only, never a byte of 0x80 or more), so `to_ascii_lowercase`, never `to_lowercase`. pub(crate) fn unit(&self, token: &str) -> Option<&UnitState> { diff --git a/crates/benilla-ui/src/script/modelframe.rs b/crates/benilla-ui/src/script/modelframe.rs index c96951ef..f812b517 100644 --- a/crates/benilla-ui/src/script/modelframe.rs +++ b/crates/benilla-ui/src/script/modelframe.rs @@ -516,12 +516,26 @@ pub(super) fn install(lua: &Lua) -> mlua::Result<()> { playermodel_install(lua) } +/// The guid a `SetUnit`/`RefreshUnit` token names right now, which the pane then holds +/// ([`crate::widget::ModelState::unit_guid`]). +/// +/// A unit with no guid — only a harness feed has one — is left unpinned, so such a pane keeps +/// re-reading its token and the paint gate behaves as it did before pinning. +fn pinned_guid(lua: &Lua, token: Option<&str>) -> Option { + let token = token?; + let model = lua.app_data_ref::().expect("model app_data"); + model.unit(token).map(|u| u.guid).filter(|&guid| guid != 0) +} + /// `PlayerModel`'s own three methods (table `0x84f1fc`); the rest are `Model`'s, reached on a miss. fn playermodel_install(lua: &Lua) -> mlua::Result<()> { let m = lua.create_table()?; - // `SetUnit(unit)` (`0x84f1fc[0]` → `0x505d70`) clears the `SetModel` path; the unit token is - // resolved at render. + // `SetUnit(unit)` (`0x84f1fc[0]` → `0x505d70`) clears the `SetModel` path and resolves the + // token **here**, pinning the guid it named: the reference's worker (`0x505b50`, the one + // `RefreshUnit` also runs) rebuilds the pane's model instance at the call, and nothing + // re-resolves the token per paint. See [`crate::widget::ModelState::unit_guid`] for the + // evidence and for the one case that keeps re-reading the token. // // **A number is a creature display id**, not a token. That is the realm's extension, not // 1.12's, and OctoWoW's shop is built on it: every mount, companion and illusion card calls @@ -567,8 +581,10 @@ fn playermodel_install(lua: &Lua) -> mlua::Result<()> { Value::String(s) => Some(s.to_str()?.to_string()), _ => None, }; + let guid = pinned_guid(lua, unit.as_deref()); with_model(lua, &this, |m| { m.unit = unit; + m.unit_guid = guid; m.path = None; // The duplicate's load installs the characterinfo camera (`0x505890`); an addon // that wants the portrait bust asks for index 0 after this call, as pfUI does. @@ -579,12 +595,18 @@ fn playermodel_install(lua: &Lua) -> mlua::Result<()> { })?, )?; - // `RefreshUnit()` (`0x84f1fc[1]` → `0x505e40`) re-reads the pane's unit; the token already - // resolves at render, so only a dress-up pane has work to do. + // `RefreshUnit()` (`0x84f1fc[1]` → `0x505e40`) re-reads the pane's unit: the same worker as + // `SetUnit`'s (`0x505b50`), so the token is resolved again and the pane re-pinned to whatever + // it names now — a unit gone from under it leaves the pane with nothing, as the reference's + // rebuild does. m.set( "RefreshUnit", lua.create_function(|lua, this: Table| { - with_model(lua, &this, |_| ())?; + let token = with_model(lua, &this, |m| m.unit.clone())?; + if token.is_some() { + let guid = pinned_guid(lua, token.as_deref()); + with_model(lua, &this, |m| m.unit_guid = guid)?; + } // The same worker as `SetUnit`'s (`0x505b50`). super::dressup::redress_if_dressup(lua, &this) })?, diff --git a/crates/benilla-ui/src/script/tests/model_clock.rs b/crates/benilla-ui/src/script/tests/model_clock.rs index 71bf64ce..b212827a 100644 --- a/crates/benilla-ui/src/script/tests/model_clock.rs +++ b/crates/benilla-ui/src/script/tests/model_clock.rs @@ -361,6 +361,84 @@ fn a_unit_panes_handler_fires_once_its_token_names_a_unit() { assert!(s.errors().is_empty(), "{:?}", s.errors()); } +/// A pane holds the unit it was **given**, not whatever its token means later. +/// +/// unitscan is the whole reason: its alert reads `SetUnit'target'` on the rare it just targeted by +/// name, and the line after that restores the player's own target. A pane that re-read `"target"` +/// per paint therefore showed the card for one frame and then went empty — which is exactly what +/// grom saw once the by-name targeting stopped leaving his target on the rare. The reference +/// resolves the token in `SetUnit`'s own rebuild (`0x505b50`), so the card keeps the rare. +#[test] +fn a_unit_pane_keeps_the_unit_it_was_given_when_the_token_moves_on() { + let mut s = script(); + s.set_screen_size(800.0, 600.0); + const RARE: u64 = 0xf130_0001_0000_0001; + const SELF: u64 = 0x2a; + s.set_unit( + "target", + Some(crate::script::UnitState { + name: Some("Ghostcrawler".into()), + guid: RARE, + ..Default::default() + }), + ); + s.run( + r#" + seen = 0 + m = CreateFrame("PlayerModel", "U", UIParent) + m:SetScript("OnUpdateModel", function() seen = seen + 1 end) + m:SetUnit("target") + "#, + ) + .unwrap(); + assert_eq!(s.model_pane("U").unwrap().unit_guid, Some(RARE)); + + // The restore, one Lua line later in the addon: the token now names someone else. + s.set_unit( + "target", + Some(crate::script::UnitState { + name: Some("Timmy".into()), + guid: SELF, + ..Default::default() + }), + ); + s.tick(0.1); + s.tick(0.1); + assert_eq!( + s.model_pane("U").unwrap().unit_guid, + Some(RARE), + "the card still shows the rare" + ); + assert_eq!( + s.eval::("return seen").unwrap(), + 2, + "and it still paints, which is where unitscan makes it visible" + ); + + // `RefreshUnit` is the verb that re-reads the token, as the reference's rebuild does. + s.run("U:RefreshUnit()").unwrap(); + assert_eq!(s.model_pane("U").unwrap().unit_guid, Some(SELF)); + + // A token the pane was seated on before its unit streamed in keeps being re-read, so nothing + // is lost when `SetUnit` runs too early. + s.run(r#"e = CreateFrame("PlayerModel", "E", UIParent) e:SetUnit("party1")"#) + .unwrap(); + assert_eq!(s.model_pane("E").unwrap().unit_guid, None); + s.set_unit( + "party1", + Some(crate::script::UnitState { + name: Some("Later".into()), + guid: 0x99, + ..Default::default() + }), + ); + assert_eq!( + s.model_ref().unit_pane_subject(&s.model_pane("E").unwrap()), + Some(0x99) + ); + assert!(s.errors().is_empty(), "{:?}", s.errors()); +} + /// `0x719370` enqueues the completion before it tests the loop flag, so a loop's first pass fires /// `OnAnimFinished`; a pane with no file fails `0x76d24c`'s gate and never paints. #[test] diff --git a/crates/benilla-ui/src/script/tests/modelframe.rs b/crates/benilla-ui/src/script/tests/modelframe.rs index dd1ebb43..c2fb976b 100644 --- a/crates/benilla-ui/src/script/tests/modelframe.rs +++ b/crates/benilla-ui/src/script/tests/modelframe.rs @@ -525,11 +525,14 @@ fn a_unit_panes_camera_is_set_cameras_index_over_set_units_default() { ); } -/// The extract resolves the pane's unit token to a GUID, because the host's booth looks a unit up -/// by GUID and the token grammar belongs to this crate. A token naming nothing carries no -/// subject, which is what empties the booth rather than leaving it on a stale body. +/// The extract carries the pane's subject as a GUID, because the host's booth looks a unit up by +/// GUID and the token grammar belongs to this crate. A pane with no subject carries none, which +/// is what empties the booth rather than leaving it on a stale body. +/// +/// Which guid: the one the pane **pinned** at its `SetUnit` ([`crate::widget::ModelState::unit_guid`]), +/// or — only for a pane whose token named nothing at the call, as here — the token re-read. #[test] -fn a_unit_pane_extracts_the_guid_its_token_names_now() { +fn a_unit_pane_extracts_the_guid_it_pinned_or_rereads_a_token_that_named_nothing() { use crate::script::{QuadContent, UnitState}; let subject = |s: &UiScript| { @@ -571,7 +574,8 @@ fn a_unit_pane_extracts_the_guid_its_token_names_now() { "the installed camera travels with the subject" ); - // A new target is a new subject with no second `SetUnit`: the token is re-read every paint. + // This pane never pinned — its `SetUnit` ran before the client had a target — so it is still + // re-reading the token, and a new target is a new subject with no second `SetUnit`. s.set_unit( "target", Some(UnitState { @@ -582,6 +586,20 @@ fn a_unit_pane_extracts_the_guid_its_token_names_now() { ); assert_eq!(subject(&s).map(|u| u.guid), Some(0xfeed_face)); + // A `SetUnit` made while the token does name someone pins that someone, and the next target + // does not move it: see `model_clock.rs`'s + // `a_unit_pane_keeps_the_unit_it_was_given_when_the_token_moves_on` for why unitscan needs it. + s.run("MSubj:SetUnit(\"target\")").unwrap(); + s.set_unit( + "target", + Some(UnitState { + guid: 0x0bad_c0de, + exists: true, + ..Default::default() + }), + ); + assert_eq!(subject(&s).map(|u| u.guid), Some(0xfeed_face)); + // A file pane has no subject at all, whichever it was asked for last. s.run(r#"MSubj:SetModel("Creature\\Wolf\\Wolf.mdx")"#) .unwrap(); diff --git a/crates/benilla-ui/src/script/tick.rs b/crates/benilla-ui/src/script/tick.rs index dbbb1cd0..16c3fc3f 100644 --- a/crates/benilla-ui/src/script/tick.rs +++ b/crates/benilla-ui/src/script/tick.rs @@ -403,15 +403,15 @@ impl UiScript { // the handler (`0x76d1bc`) is gated on `[widget+0x318] ≠ 0` (`0x76d24c`), which is the // pane's **loaded model**, not the file string. // - // A `SetUnit` pane has one of those too, and keeps no path at all — its subject - // resolves per render, app-side (`modelframe.rs`'s `SetUnit` writes `unit` and nils + // A `SetUnit` pane has one of those too, and keeps no path at all — its subject is + // the unit it pinned (`modelframe.rs`'s `SetUnit` writes `unit`/`unit_guid` and nils // `path`) — so gating on `path` alone left every unit pane's handler permanently // inert. Not cosmetic: unitscan's alert model is made visible *only* from inside its // own `OnUpdateModel` (`unitscan.lua:273-284` counts ten paints as a settle delay // after `reset()` has set alpha 0), so the alert drew a correctly placed, correctly - // sized, fully transparent model and the horn played over an empty card. A unit token - // that names nothing still fires nothing, which is the `≠ 0` half of the gate. On any - // other kind the script is inert. + // sized, fully transparent model and the horn played over an empty card. A pane with + // no subject still fires nothing, which is the `≠ 0` half of the gate. On any other + // kind the script is inert. let frames: Vec = model .on_update_model_frames .iter() @@ -421,10 +421,7 @@ impl UiScript { f.effective_visible && match &f.kind_state { crate::widget::KindState::Model(m) => { - m.path.is_some() - || m.unit - .as_deref() - .is_some_and(|t| model.unit(t).is_some()) + m.path.is_some() || model.unit_pane_subject(m).is_some() } _ => false, } diff --git a/crates/benilla-ui/src/widget/kinds/mod.rs b/crates/benilla-ui/src/widget/kinds/mod.rs index 2231539d..acbb3eeb 100644 --- a/crates/benilla-ui/src/widget/kinds/mod.rs +++ b/crates/benilla-ui/src/widget/kinds/mod.rs @@ -264,6 +264,21 @@ pub struct ModelState { pub path: Option, /// `PlayerModel:SetUnit`'s unit (`0x505d70`); it and [`Self::path`] each clear the other. pub unit: Option, + /// The guid [`Self::unit`] named **at the call**, the pane's subject from then on — not the + /// token's current meaning. + /// + /// `SetUnit` and `RefreshUnit` run one worker (`0x505b50`) that resolves the token and + /// rebuilds the pane's model instance there and then; nothing re-resolves it per paint. That + /// is why FrameXML re-issues `SetUnit` on `UNIT_MODEL_CHANGED` and from + /// `PaperDollFrame_OnShow`, and why pfUI can spot a portrait change with no event at all: + /// it pins a scratch pane to the current token and compares `GetModel` against the shown one + /// (`api/unitframes.lua:2069`), which only tells it anything if a pane holds what it was + /// given. unitscan is the live consumer — it `SetUnit`s the rare it found, and the target + /// restored one line later must not take the card. + /// + /// `None` while the token named nothing yet, and then the token is re-read per paint: our + /// subject streams in asynchronously, so a pane can be seated before it arrives. + pub unit_guid: Option, /// The creature skin variations for a pane showing a display id, by M2 skin slot; empty for /// an ordinary `SetModel` file, whose batches name their own textures. Cleared by /// [`Self::set_file`] and written after it, so a plain file never inherits the last @@ -375,6 +390,7 @@ impl Default for ModelState { Self { path: None, unit: None, + unit_guid: None, skins: Default::default(), sequence: 0, clock_ms: 0, @@ -494,6 +510,7 @@ impl ModelState { pub fn set_file(&mut self, path: String, facts: Option<&ModelFileFacts>) { self.path = Some(path); self.unit = None; + self.unit_guid = None; self.skins = Default::default(); self.icon = None; self.armed = None; @@ -507,6 +524,7 @@ impl ModelState { pub fn clear_file(&mut self) { self.path = None; self.unit = None; + self.unit_guid = None; self.skins = Default::default(); self.icon = None; self.armed = None;