From b8e8fd4382cb1901f121594b88f208b7839efd96 Mon Sep 17 00:00:00 2001 From: grom Date: Mon, 26 Oct 2026 07:28:31 -0600 Subject: [PATCH] A creature-skin record fills its slot only where it holds a path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Monster1/2/3` exist so `CreatureDisplayInfo`'s skin variation can be dropped in at spawn, and the record's own filename is the fallback where the display has no variation for that index. The fallback was unconditional, and the slot does not always hold a name: the stock models leave whatever the exporter left there. That matters because a filled `texture` wins over the slot downstream (`benilla_app::entities::display::skin_for`), so junk in it would mask the display's real variation for good. Measured over this chain, separating the two populations by whether the name reads as a path at all: * **26 records in 17 models bake a real `.blp` path** and are the winning copy — the Crab, Scorpion, Crocodile, Thunder Lizard and RavenLord mounts, the WC2 Death Knight (body, mount and a cape out of `ITEM\ObjectComponents`), the Wizard Frog's hat, the Duck wearing a turkey skin, the Goblin rocket car, the Worgen Demon Mage reaching for the Felhound's skin, Incubus, Crystal Satyr, the alternate and Northrend bear mounts. These have nothing else to draw with. * **55 more carry unprintable leftovers** — Banshee, every DragonSpawn, DruidCat, FrostSabre, Lobstrok, ObsidianDestroyer and the rest — all of them in `model.MPQ` and `patch.MPQ` copies that this chain's letter patches shadow, so none of them is a copy a read reaches today. So `monster_skin_name` keeps only what reads as a texture path: printable ASCII, no control bytes, longer than the extension, ending `.blp`. Every one of the 26 passes it and not one of the leftovers does. benilla-bourbon (tripleslash/benilla-bourbon, "junk creature-skin records") drops the fallback outright, on Turtle WoW's `Stag.m2` carrying one junk byte there. Applied here that removes the skin from all 17 models above — the new test fails exactly that way, "17 of 17 models with a baked skin path did not get it" — so this takes the diagnosis and not the patch. The leftovers it names are real; dropping the whole fallback is what this chain cannot afford. Tests: a synthetic pair beside the guard for both kinds of name, and a corpus test gated on `$WOW_DATA` that measures both populations over the winning copies and asserts a path reaches `texture` while a leftover never does. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SgstFWtn1RxEJ5kVrpgYwn --- .../benilla-formats/src/models/m2_batches.rs | 69 +++++++++- .../tests/m2_monster_skin_record.rs | 120 ++++++++++++++++++ 2 files changed, 184 insertions(+), 5 deletions(-) create mode 100644 crates/benilla-formats/tests/m2_monster_skin_record.rs diff --git a/crates/benilla-formats/src/models/m2_batches.rs b/crates/benilla-formats/src/models/m2_batches.rs index 15bbe477..02d82cf8 100644 --- a/crates/benilla-formats/src/models/m2_batches.rs +++ b/crates/benilla-formats/src/models/m2_batches.rs @@ -27,8 +27,21 @@ fn parent_dir(path: &str) -> &str { } } +/// A creature-skin record's own filename, where it is one. The slot is meant to be filled from +/// `CreatureDisplayInfo`, and the stock models leave whatever the exporter left in it, so the test +/// is whether it reads as a texture path at all: printable, and a `.blp`. The 26 custom models in +/// this realm's chain that use the slot as a path all end `.blp`; not one of the stock models' +/// leftovers is even printable. +fn monster_skin_name(embedded: &str) -> Option { + let ok = embedded.is_ascii() + && !embedded.chars().any(|c| c.is_ascii_control()) + && embedded.len() > 4 + && embedded.to_ascii_lowercase().ends_with(".blp"); + ok.then(|| embedded.to_owned()) +} + /// A batch's `.blp`: `Monster1/2/3` come from the creature's skin variations -/// (`\.blp`), else the embedded filename. +/// (`\.blp`), else the record's own filename where it is a path. fn resolve_texture( tex: &benilla_m2::M2Texture, dir: &str, @@ -40,6 +53,7 @@ fn resolve_texture( let f = tex.filename.string.to_string_lossy(); (!f.is_empty()).then(|| f.into_owned()) }; + let monster = embedded.as_deref().and_then(monster_skin_name); let variation = |i: usize| { skins .get(i) @@ -55,11 +69,21 @@ fn resolve_texture( M2TextureType::Other(8) => Some(CharSkinSlot::SkinExtra), _ => None, }; - // The skin slot is reported so a skin-less load can fill it at spawn. + // The skin slot is reported so a skin-less load can fill it at spawn, and the record's own + // name is the fallback where the display has no variation for that index — 26 of this realm's + // custom creatures and mounts bake a real path there (the Crab, Scorpion, Crocodile, Thunder + // Lizard and RavenLord mounts, the WC2 Death Knight, the Wizard Frog, the Worgen Demon Mage + // reaching for the Felhound's skin), and they have nothing else to draw with. + // + // But the slot is not always a name. The stock models leave junk bytes there — Banshee, every + // DragonSpawn, DruidCat, FrostSabre, Lobstrok and 55 more in the shadowed copies of + // `model.MPQ` and `patch.MPQ` — and a junk fallback is worse than none, because a filled + // `texture` wins over the slot (`benilla_app::entities::display::skin_for`), so the display's + // real variation would never go on. [`monster_skin_name`] keeps only what reads as a path. match tex.texture_type { - M2TextureType::Monster1 => (variation(0).or(embedded), Some(0), None, false), - M2TextureType::Monster2 => (variation(1).or(embedded), Some(1), None, false), - M2TextureType::Monster3 => (variation(2).or(embedded), Some(2), None, false), + M2TextureType::Monster1 => (variation(0).or(monster), Some(0), None, false), + M2TextureType::Monster2 => (variation(1).or(monster), Some(1), None, false), + M2TextureType::Monster3 => (variation(2).or(monster), Some(2), None, false), _ => (embedded, None, char_slot, icon_slot), } } @@ -787,6 +811,41 @@ pub fn m2_sequence_visible_textures(bytes: &[u8], anim_id: u16) -> Option = chain + .list() + .expect("list the patch chain") + .into_iter() + .map(|e| e.name) + .filter(|n| n.to_ascii_lowercase().ends_with(".m2")) + .collect(); + paths.sort(); + paths.dedup(); + assert!( + paths.len() > 5000, + "expected the full M2 corpus, found {} files", + paths.len() + ); + + // Phase one: who carries a name there at all. Parsing the header is far cheaper than building + // every mesh in the corpus, and this is the only set the question is about. + let mut paths_named: Vec = Vec::new(); + let mut junk: Vec<(String, Vec)> = Vec::new(); + for p in &paths { + let Ok(bytes) = chain.read_file(p) else { + continue; + }; + let Ok(fmt) = benilla_m2::parse_m2(&mut std::io::Cursor::new(bytes.as_slice())) else { + continue; + }; + for tex in &fmt.model().textures { + if !matches!( + tex.texture_type, + M2TextureType::Monster1 | M2TextureType::Monster2 | M2TextureType::Monster3 + ) { + continue; + } + let raw = tex.filename.string.to_bytes(); + if raw.is_empty() { + continue; + } + let looks_like_a_path = raw.iter().all(|&c| (32..127).contains(&c)) + && raw.len() > 4 + && raw[raw.len() - 4..].eq_ignore_ascii_case(b".blp"); + if looks_like_a_path { + paths_named.push(p.clone()); + } else { + junk.push((p.clone(), raw.to_vec())); + } + } + } + + // 26 on this chain's winning copies: the Crab, Scorpion, Crocodile, Thunder Lizard and + // RavenLord mounts, the WC2 Death Knight, the Wizard Frog, the Worgen Demon Mage reaching for + // the Felhound's skin, and the rest. A regression that dropped the fallback would leave every + // one of them untextured wherever the display has no variation. + assert!( + paths_named.len() > 20, + "expected this chain's custom models to bake a skin path in the record, found {}", + paths_named.len() + ); + + // Phase two: a name that reads as a path reaches `texture` on a skin-less load. + paths_named.sort(); + paths_named.dedup(); + let mut filled = 0u32; + for p in &paths_named { + let Ok(subs) = load_m2_mesh(&mut chain, p) else { + continue; + }; + if subs + .iter() + .any(|s| s.skin_slot.is_some() && s.texture.is_some()) + { + filled += 1; + } + } + assert_eq!( + filled, + paths_named.len() as u32, + "{} of {} models with a baked skin path did not get it", + paths_named.len() as u32 - filled, + paths_named.len() + ); + + // And a name that does not read as a path never reaches `texture`. The stock leftovers all sit + // in shadowed copies on this install, so this half may have no witness here — the synthetic + // pair in `m2_batches::tests` covers it either way. Where there is a witness, it must hold. + for (p, raw) in &junk { + let Ok(subs) = load_m2_mesh(&mut chain, p) else { + continue; + }; + for sub in &subs { + if sub.skin_slot.is_some() { + assert!( + sub.texture.is_none(), + "{p}: a leftover record ({raw:?}) filled the skin slot with {:?}", + sub.texture + ); + } + } + } +}