A creature-skin record fills its slot only where it holds a path

`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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SgstFWtn1RxEJ5kVrpgYwn

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<String> {
+    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
-/// (`<model-dir>\<name>.blp`), else the embedded filename.
+/// (`<model-dir>\<name>.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<Vec<St
 
 #[cfg(test)]
 mod tests {
+    /// The creature-skin slot keeps a path and drops a leftover. A filled `texture` wins over the
+    /// slot downstream (`benilla_app::entities::display::skin_for`), so junk there would mask the
+    /// display's real variation for good.
+    #[test]
+    fn a_monster_slot_keeps_a_path_and_drops_a_leftover() {
+        assert_eq!(
+            monster_skin_name("creature\\crab\\crabskinvermillian.blp").as_deref(),
+            Some("creature\\crab\\crabskinvermillian.blp"),
+            "a real path is the model's only skin where the display has no variation"
+        );
+        assert_eq!(
+            monster_skin_name("CREATURE\\TURKEY\\TURKEYSKIN05.BLP").as_deref(),
+            Some("CREATURE\\TURKEY\\TURKEYSKIN05.BLP"),
+            "the extension test is case-insensitive"
+        );
+        // What the stock models leave there: Banshee's record starts 0xff 0xff.
+        assert_eq!(monster_skin_name("\u{ff}\u{ff}"), None);
+        assert_eq!(
+            monster_skin_name("\u{1}\u{2}\u{3}.blp"),
+            None,
+            "control bytes"
+        );
+        assert_eq!(monster_skin_name(""), None);
+        assert_eq!(
+            monster_skin_name(".blp"),
+            None,
+            "an extension is not a path"
+        );
+        assert_eq!(
+            monster_skin_name("Creature\\Kobold\\Kobold.m2"),
+            None,
+            "a model is not a texture"
+        );
+    }
+
     use super::*;
 
     /// A bucket never sorts under the exact rung, which its sibling quad cloud keeps.

crates/benilla-formats/tests/m2_monster_skin_record.rs

@@ -0,0 +1,120 @@
+//! What a creature-skin record (`Monster1/2/3`) carries in its own filename, and what the loader
+//! may do with it.
+//!
+//! The slot exists so `CreatureDisplayInfo`'s skin variation can be dropped in at spawn. Where the
+//! display has no variation for that index the record's own name is the only thing left — and this
+//! realm's custom creatures and mounts rely on that. But the stock models leave whatever the
+//! exporter left there, and a *junk* fallback is worse than none: a filled `texture` wins over the
+//! slot downstream (`benilla_app::entities::display::skin_for`), so the display's real variation
+//! would never go on.
+//!
+//! This measures both populations over the chain's winning copies, so the guard in
+//! `m2_batches::monster_skin_name` is held to what the corpus actually holds rather than to a
+//! guess.
+
+use benilla_formats::{load_m2_mesh, Chain};
+use benilla_m2::M2TextureType;
+
+#[test]
+fn a_monster_skin_record_is_kept_when_it_is_a_path_and_dropped_when_it_is_not() {
+    let data = benilla_formats::wow_data_or_skip!();
+    let mut chain = Chain::open(&data).expect("open vanilla patch chain");
+    let mut paths: Vec<String> = 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<String> = Vec::new();
+    let mut junk: Vec<(String, Vec<u8>)> = 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
+                );
+            }
+        }
+    }
+}