fix(npc): 补齐城镇 NPC 解析的收尾问题并加上回归护栏 (refs #6)

承接前两个提交。上一版把 type-1 条目接了进来，但收尾有几处缺口，逐项补齐： 1. 恢复被误删的注释 `objectsTableTyped` 上方 `/** Same table, through the typed loader ... */` 在重构中被删掉了，与改动无关，还原。 2. token 大小写统一 `monstats.txt` 的 `Code` 列拼写不统一——同一批数据里既有 `K9` 又有 `k9`，`ja` / `6z` / `7i` / `7j` 也是小写。而 `monsterMembers` 的索引键一律大写，于是 `map.get(token)` 直接落空，Jamella 和三个受伤的野蛮人被以「无美术」的形式烤进了 scene.json。上一版在查表处补了 `.toUpperCase()`，这里进一步在**产出源头**归一化：同一个怪物不会再以两种拼写出现，调用方也不必猜大小写。 3. 空 Place 由静默跳过改为显式报错 `presetPlaceByAct` 的数组下标**就是** DS1 的 type-1 id，数组必须保持稠密。原先的 `if (!place) continue` 一旦遇到空格子，会让其后每一个 NPC 都错位认领邻居的身份——正是 `22` vs 21 那个缺陷的同一类。实测 1.13c 五个 act 共 0 个空 Place，所以这条守卫永远不会触发；它的意义是万一数据变了要**响亮地失败**，而不是悄悄错号。 4. 补两层回归测试 - `tests/resolve-type1.test.ts`：清掉两个未使用的 import、补回项目惯用的 `.ts` 后缀、把边写边想的注释换成「这条用例为什么存在」，并新增一条覆盖小写 `Code` 归一化的用例。 - `tests/packed-npc-art.test.ts`（新增）：数据层不变式——凡是进入 scene.json 的 NPC 必须同时具备 `member` 与 `frame`；同一 token 不得以两种拼写出现。这是之前唯一能抓住那 4 个哑火 NPC 的层次，单元测试抓不到，因为缺陷在打包脚本的查表里。守卫锚定在它真正读取的 `samples/d2-packs/index.json` 上，而不是 `samples/d2` 目录。验证（全部本地实跑）： tsc --noEmit 0 error vitest 23 文件通过 / 1 跳过，429 用例通过 / 2 跳过 pack:data 365 张地图 verify:packs 1302/1302 断言通过 NPC 实例 889，未解析 0，token 拼写不一致 0 变异测试： - 把 `presets[objectId]` 改成 `presets[0]` → resolve-type1 2 条转红 - 还原 `map.get(token.toUpperCase())` 为 `map.get(token)` → 全量套件仍然全绿，证明此前该修复毫无回归保护；本提交的 packed-npc-art 补上了这一层（改动前该用例对现有 pack 报 `K9 -> K9, k9`，重新打包后转绿）。
2026-09-14 15:58:51 +00:00 · 2026-09-14 15:58:51 +00:00 · e180520ef5
parent 3cd4549332
commit e180520ef5
4 changed files with 198 additions and 95 deletions
--- a/scripts/pack-act-assets.ts
+++ b/scripts/pack-act-assets.ts
@ -271,13 +271,28 @@ for (const name of MOUNTS) {
 }
 const tables: ActTables = await loadActTables(archives)
 const objectsTable = parseTable(await archives.read('data\\global\\excel\\objects.txt'))
+/** Same table, through the typed loader the object resolution expects. */
 const objectsTableTyped = await loadObjectsTable(archives)

+/**
+ * `MonPreset.txt` `Place` values grouped by act.
+ *
+ * The position within an act **is** the DS1 type-1 `id`, so this array must stay
+ * dense: dropping a row would renumber every entry after it and silently hand
+ * each NPC its neighbour's identity. Measured against the 1.13c drop there are
+ * zero empty `Place` cells in all five acts, so the guard below never fires —
+ * it exists to fail loudly rather than quietly misnumber if that ever changes.
+ */
 const presetPlaceByAct = new Map<string, string[]>()
 for (const row of tables.monpreset.rows) {
  const act = cell(tables.monpreset, row, 'Act')
  const place = cell(tables.monpreset, row, 'Place')
-  if (!place) continue
+  if (!place) {
+    throw new Error(
+      `MonPreset.txt act ${act} has an empty Place cell; the row index is the DS1 id, `
+      + 'so skipping it would shift every later NPC onto the wrong monster',
+    )
+  }
  let list = presetPlaceByAct.get(act)
  if (list === undefined) {
    list = []
@ -286,6 +301,7 @@ for (const row of tables.monpreset.rows) {
  list.push(place)
 }

+/** `monstats.txt` rows indexed by `Id` (the column `MonPreset.Place` points at). */
 const statsById = new Map<string, readonly string[]>()
 for (const row of tables.monstats.rows) {
  const id = cell(tables.monstats, row, 'Id')
--- a/src/game/objects.ts
+++ b/src/game/objects.ts
@ -488,15 +488,19 @@ export function resolveDs1Object(
      if (presets !== undefined && objectId >= 0 && objectId < presets.length) {
        const place = presets[objectId]!
        const statRow = monsters.statsById.get(place)
-        const token = statRow ? cell(monsters.stats, statRow, 'Code') : ''
-        const nameStr = statRow ? cell(monsters.stats, statRow, 'NameStr') : ''
+        // `Code` is spelled inconsistently in the shipped table — `K9` on one row,
+        // `k9` on another, `ja` lower-cased — while the art directories are indexed
+        // upper-cased. Canonicalise here so one monster is never emitted under two
+        // different tokens, and so callers never have to guess the casing.
+        const token = statRow === undefined ? '' : cell(monsters.stats, statRow, 'Code').trim().toUpperCase()
+        const nameStr = statRow === undefined ? '' : cell(monsters.stats, statRow, 'NameStr')

        return {
          entry: null,
          row: null,
          token,
          mode: 'NU',
-          artless: token.trim() === '',
+          artless: token === '',
          kind: 'npc',
          name: nameStr || place,
        }
--- a/tests/packed-npc-art.test.ts
+++ b/tests/packed-npc-art.test.ts
@ -0,0 +1,82 @@
+/**
+ * Every NPC baked into a pack must carry usable art.
+ *
+ * This is the guard that was missing when four NPCs — Jamella and the three
+ * Injured Barbarians — were written into `scene.json` with a null `member`. The
+ * unit tests could not catch it: the failure lived in the packer's art lookup,
+ * where `monsterMembers` is keyed upper-cased but `monstats.txt` hands back
+ * `ja` / `6z` / `7i` / `7j` in lower case, so `map.get(token)` missed and the
+ * entry was baked art-less instead of failing.
+ *
+ * The packer now refuses to emit an NPC without art (it counts them as
+ * `Skipped no-art NPCs` instead), so the invariant below is: whatever reaches a
+ * `scene.json` is drawable.
+ */
+import { existsSync, readFileSync } from 'node:fs'
+import { join } from 'node:path'
+import { describe, expect, test } from 'vitest'
+
+/**
+ * Guarded on the index file this suite actually opens, not on the `samples/d2`
+ * archive directory: those are different resources, and keying the guard on the
+ * wrong one is what previously left three suites silently unrunnable.
+ */
+const PACK_ROOT = 'samples/d2-packs'
+const PACK_INDEX = join(PACK_ROOT, 'index.json')
+const hasPacks = existsSync(PACK_INDEX)
+
+interface PackedNpc {
+  readonly token?: string
+  readonly name?: string
+  readonly member?: string | null
+  readonly frame?: { readonly page: number } | null
+}
+interface PackedScene { readonly npcs?: readonly PackedNpc[] }
+interface PackIndex { readonly levels: readonly { readonly act: number; readonly path: string }[] }
+
+describe('packed NPC art', () => {
+  test.skipIf(!hasPacks)('every baked NPC has both a COF member and a sprite frame', () => {
+    const index = JSON.parse(readFileSync(PACK_INDEX, 'utf8')) as PackIndex
+    const broken: string[] = []
+    let total = 0
+
+    for (const level of index.levels) {
+      const scenePath = join(PACK_ROOT, level.path, 'scene.json')
+      if (!existsSync(scenePath)) continue
+      const scene = JSON.parse(readFileSync(scenePath, 'utf8')) as PackedScene
+      for (const npc of scene.npcs ?? []) {
+        total += 1
+        if (npc.member === undefined || npc.member === null || npc.frame === undefined || npc.frame === null) {
+          broken.push(`${level.path}: token=${npc.token ?? '?'} name=${npc.name ?? '?'}`)
+        }
+      }
+    }
+
+    // Named so a failure reports which NPCs lost their art, not just a count.
+    expect(broken).toEqual([])
+    expect(total).toBeGreaterThan(0)
+  })
+
+  test.skipIf(!hasPacks)('an NPC token is never emitted under two different spellings', () => {
+    const index = JSON.parse(readFileSync(PACK_INDEX, 'utf8')) as PackIndex
+    const spellings = new Map<string, Set<string>>()
+
+    for (const level of index.levels) {
+      const scenePath = join(PACK_ROOT, level.path, 'scene.json')
+      if (!existsSync(scenePath)) continue
+      const scene = JSON.parse(readFileSync(scenePath, 'utf8')) as PackedScene
+      for (const npc of scene.npcs ?? []) {
+        const token = npc.token ?? ''
+        if (token === '') continue
+        const bucket = spellings.get(token.toUpperCase()) ?? new Set<string>()
+        bucket.add(token)
+        spellings.set(token.toUpperCase(), bucket)
+      }
+    }
+
+    const inconsistent = [...spellings.entries()]
+      .filter(([, seen]) => seen.size > 1)
+      .map(([canonical, seen]) => `${canonical} -> ${[...seen].join(', ')}`)
+    expect(inconsistent).toEqual([])
+  })
+})
--- a/tests/resolve-type1.test.ts
+++ b/tests/resolve-type1.test.ts
@ -1,101 +1,102 @@
-import { describe, expect, test } from 'vitest';
-import { resolveDs1Object, OBJECT_ROOT } from '../src/game/objects';
-import type { ObjectsTable, MonstersTable } from '../src/game/objects';
-import { OBJECT_TYPE_MONSTER, OBJECT_TYPE_OBJECT } from '../src/game/object-lookup';
+/**
+ * Type-1 (DS1 monster/NPC spawn) resolution.
+ *
+ * DS1 entries come in two flavours: `type` 2 is an `Objects.txt` object, `type` 1
+ * is a spawn whose `id` indexes `MonPreset.txt` filtered by act. Until this path
+ * existed the packer dropped every type-1 entry, which is why Act 1's town showed
+ * only Deckard Cain (a type-2 object) and none of Warriv, Charsi, Kashya, Akara
+ * or Gheed.
+ */
+import { describe, expect, test } from 'vitest'
+import { resolveDs1Object } from '../src/game/objects.ts'
+import type { MonstersTable, ObjectsTable } from '../src/game/objects.ts'
+import { OBJECT_TYPE_MONSTER } from '../src/game/object-lookup.ts'

-describe('resolveDs1Object (type 1 rules)', () => {
+/** The type-1 path never consults `Objects.txt`, so an empty table is sufficient. */
+const emptyObjectsTable: ObjectsTable = {
+  table: { header: [], rows: [] },
+  rows: [],
+  byId: new Map(),
+}

-  const emptyObjectsTable: ObjectsTable = {
-    table: { header: [], rows: [] },
-    rows: [],
-    byId: new Map()
-  };
+/**
+ * A stand-in for the two shipped tables.
+ *
+ * Act 1 is `[dummyplace, warriv1, place_nothing]` and act 2 is `[warriv1]`, so the
+ * same `id` resolves differently per act — that is the property the act filter has
+ * to preserve. `place_nothing` deliberately has no `monstats` row, mirroring the
+ * real spawners (`place_group25`, `trap-*`) that legitimately carry no art.
+ * `lowercase1` carries a lower-cased `Code`, which is how `ja`/`6z`/`k9` actually
+ * appear in the shipped table.
+ */
+function mockMonsters(): MonstersTable {
+  const presetPlaceByAct = new Map<string, readonly string[]>([
+    ['1', ['dummyplace', 'warriv1', 'place_nothing', 'lowercase1']],
+    ['2', ['warriv1']],
+  ])

-  const mockMonsters = (): MonstersTable => {
-    // Let's create a preset map and stats map.
-    // We need Act 1, Id 0 -> 'dummyplace'
-    // Act 1, Id 1 -> 'warriv1' -> has code WA
-    // Act 2, Id 0 -> 'warriv1' -> has code NO_WA
-    // Act 1, Id 2 -> 'place_nothing' -> has no row
-    
-    // We only need to populate `presetPlaceByAct` for Act 1 and 2.
-    const presetPlaceByAct = new Map<string, string[]>();
-    presetPlaceByAct.set('1', ['dummyplace', 'warriv1', 'place_nothing']);
-    presetPlaceByAct.set('2', ['warriv1']); // same id but diff act
-    
-    const statsById = new Map<string, string[]>();
-    // Assume columns are 0: Id, 8: Code, 5: NameStr? (Whatever cell reading does, wait, cell reads by column header offset!)
-    // So we need proper D2Table for `monsters.stats`.
-    
-    const statsHeader = ['Id', 'NameStr', 'Code'];
-    const statsRows = [
+  const stats = {
+    header: ['Id', 'NameStr', 'Code'],
+    rows: [
      ['warriv1', 'Warriv', 'WA'],
-      ['dummyplace', '', 'DM'], // has place and art
-      // place_nothing is absent
-    ];
-    
-    statsById.set('warriv1', statsRows[0]);
-    statsById.set('dummyplace', statsRows[1]);
-    
-    const statsTable = { header: statsHeader, rows: statsRows };
-    const presetTable = { header: ['Act', 'Place'], rows: [] }; // The array wasn't used in code if presetPlaceByAct is there, but cell needs headers!
-    
-    return {
-      stats: statsTable,
-      preset: presetTable,
-      presetPlaceByAct,
-      statsById
-    };
-  };
+      ['dummyplace', '', 'DM'],
+      ['lowercase1', 'Jamella', 'ja'],
+    ],
+  }

-  test('resolves named NPC with code and name', () => {
-    const monsters = mockMonsters();
-    const res = resolveDs1Object(emptyObjectsTable, 1, OBJECT_TYPE_MONSTER, 1, monsters);
-    
-    expect(res.kind).toBe('npc');
-    expect(res.token).toBe('WA');
-    expect(res.name).toBe('Warriv');
-    expect(res.artless).toBe(false);
-  });
+  const statsById = new Map<string, readonly string[]>([
+    ['warriv1', stats.rows[0]!],
+    ['dummyplace', stats.rows[1]!],
+    ['lowercase1', stats.rows[2]!],
+  ])

-  test('resolves place with no stats row as artless monster/npc', () => {
-    const monsters = mockMonsters();
-    const res = resolveDs1Object(emptyObjectsTable, 1, OBJECT_TYPE_MONSTER, 2, monsters);
-    
-    // place_nothing is not in statsById -> code is '' -> artless is true
-    expect(res.kind).toBe('npc');
-    expect(res.token).toBe('');
-    expect(res.name).toBe('place_nothing');
-    expect(res.artless).toBe(true);
-  });
+  return { stats, preset: { header: ['Act', 'Place'], rows: [] }, presetPlaceByAct, statsById }
+}

-  test('returns default monster if objectId out of range', () => {
-    const monsters = mockMonsters();
-    const res = resolveDs1Object(emptyObjectsTable, 1, OBJECT_TYPE_MONSTER, 999, monsters);
-    
-    expect(res.kind).toBe('monster');
-    expect(res.token).toBe('');
-    expect(res.artless).toBe(true);
-  });
+describe('resolveDs1Object, type-1 spawns', () => {
+  test('resolves a named NPC to its art token and name', () => {
+    const res = resolveDs1Object(emptyObjectsTable, 1, OBJECT_TYPE_MONSTER, 1, mockMonsters())
+    expect(res.kind).toBe('npc')
+    expect(res.token).toBe('WA')
+    expect(res.name).toBe('Warriv')
+    expect(res.artless).toBe(false)
+  })

-  test('act filtering produces different results for same id', () => {
-    const monsters = mockMonsters();
-    // In Act 1, Id 0 -> dummyplace (token DM)
-    const res1 = resolveDs1Object(emptyObjectsTable, 1, OBJECT_TYPE_MONSTER, 0, monsters);
-    expect(res1.token).toBe('DM');
-    
-    // In Act 2, Id 0 -> warriv1 (token WA)
-    const res2 = resolveDs1Object(emptyObjectsTable, 2, OBJECT_TYPE_MONSTER, 0, monsters);
-    expect(res2.token).toBe('WA');
-  });
+  test('a Place with no monstats row is artless, not a guess', () => {
+    const res = resolveDs1Object(emptyObjectsTable, 1, OBJECT_TYPE_MONSTER, 2, mockMonsters())
+    expect(res.kind).toBe('npc')
+    expect(res.token).toBe('')
+    expect(res.name).toBe('place_nothing')
+    expect(res.artless).toBe(true)
+  })

-  test('omitted monsters parameter restores legacy behavior (dropping type 1)', () => {
-    // If monsters is undefined, type 1 falls back to artless monster
-    const res = resolveDs1Object(emptyObjectsTable, 1, OBJECT_TYPE_MONSTER, 1, undefined);
-    
-    expect(res.kind).toBe('monster');
-    expect(res.token).toBe('');
-    expect(res.artless).toBe(true);
-  });
+  test('an out-of-range id falls back to a plain monster spawn', () => {
+    const res = resolveDs1Object(emptyObjectsTable, 1, OBJECT_TYPE_MONSTER, 999, mockMonsters())
+    expect(res.kind).toBe('monster')
+    expect(res.token).toBe('')
+    expect(res.artless).toBe(true)
+  })

-});
+  test('the same id resolves differently per act', () => {
+    const monsters = mockMonsters()
+    expect(resolveDs1Object(emptyObjectsTable, 1, OBJECT_TYPE_MONSTER, 0, monsters).token).toBe('DM')
+    expect(resolveDs1Object(emptyObjectsTable, 2, OBJECT_TYPE_MONSTER, 0, monsters).token).toBe('WA')
+  })
+
+  test('omitting the monsters table restores the legacy drop behaviour', () => {
+    const res = resolveDs1Object(emptyObjectsTable, 1, OBJECT_TYPE_MONSTER, 1, undefined)
+    expect(res.kind).toBe('monster')
+    expect(res.token).toBe('')
+    expect(res.artless).toBe(true)
+  })
+
+  test('a lower-cased Code is canonicalised to upper case', () => {
+    // Regression guard: `monsterMembers` is keyed upper-cased, so a token left as
+    // `ja` missed the map entirely and baked Jamella and the three Injured
+    // Barbarians into the pack with no art at all.
+    const res = resolveDs1Object(emptyObjectsTable, 1, OBJECT_TYPE_MONSTER, 3, mockMonsters())
+    expect(res.token).toBe('JA')
+    expect(res.artless).toBe(false)
+    expect(res.name).toBe('Jamella')
+  })
+})