diff --git a/scripts/pack-act-assets.ts b/scripts/pack-act-assets.ts index 72b049e..dbf8399 100644 --- 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() 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() for (const row of tables.monstats.rows) { const id = cell(tables.monstats, row, 'Id') diff --git a/src/game/objects.ts b/src/game/objects.ts index 1086f10..1ecb757 100644 --- 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, } diff --git a/tests/packed-npc-art.test.ts b/tests/packed-npc-art.test.ts new file mode 100644 index 0000000..fb71120 --- /dev/null +++ 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>() + + 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() + 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([]) + }) +}) diff --git a/tests/resolve-type1.test.ts b/tests/resolve-type1.test.ts index 47a677b..ebf008e 100644 --- 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([ + ['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(); - presetPlaceByAct.set('1', ['dummyplace', 'warriv1', 'place_nothing']); - presetPlaceByAct.set('2', ['warriv1']); // same id but diff act - - const statsById = new Map(); - // 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([ + ['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') + }) +})