Merge pull request 'feat(assets): a creature's picture is whichever action has one (Phase 6)' (#39) from feat/asset-bridge-p6 into edge

Reviewed-on: #39

server/db/schema.sql

@@ -675,7 +675,7 @@ CREATE TABLE IF NOT EXISTS shard_atlas_pending (
 -- because the manifest reports them before the pixels are fetched and a screen
 -- that lists what WOULD be imported needs them then.
 CREATE TABLE IF NOT EXISTS shard_assets (
-  asset_key   VARCHAR(191) NOT NULL PRIMARY KEY,  -- §5's key: `body/34/a0`
+  asset_key   VARCHAR(191) NOT NULL PRIMARY KEY,  -- §5's key: `body/34/a0`, `body/820/a23`
   family      VARCHAR(24)  NOT NULL DEFAULT 'body',
   sha256      CHAR(64)     NOT NULL,
   bytes       INT          NOT NULL DEFAULT 0,
@@ -708,6 +708,21 @@ CREATE TABLE IF NOT EXISTS shard_assets (
 -- same test and costs one re-fetch.
 ALTER TABLE shard_assets ADD COLUMN IF NOT EXISTS catalog VARCHAR(32) NULL;
 
+-- Which action a body's thumbnail came from (§11.2, phase 6).
+--
+-- The catalogue is still one row per body and still a first frame; what changed
+-- is that a body with no art at action 0 is catalogued at the first action that
+-- has any, and the key says so — `body/820/a23` is a horse whose action 0 is
+-- empty. 73 of a stock client's bodies are in that state, and they rendered as
+-- text on the bestiary until this phase looked one action further.
+--
+-- It is stored rather than parsed back out of the key because the atlas join
+-- needs it in SQL, and re-deriving it there with SUBSTRING_INDEX would put a
+-- second, weaker parser of §5's key scheme in the schema. NULL means a row
+-- written before this column existed, which is action 0 by definition — every
+-- key the catalogue had then ended in `a0`.
+ALTER TABLE shard_assets ADD COLUMN IF NOT EXISTS action TINYINT NULL;
+
 -- Slug → body id, as the shard itself answered it (§8).
 --
 -- **Deliberately NOT a column on `shard_spawn_creatures`.** That table is

server/model/shardAssets/shardAssets.db.js

@@ -33,7 +33,8 @@ async function batched(conn, sql, rows) {
 /** Every asset row we hold, as a Map of key → row. */
 async function allAssets() {
   const rows = await query(
-    'SELECT asset_key, family, sha256, bytes, width, height, body, direction, file, catalog FROM shard_assets',
+    'SELECT asset_key, family, sha256, bytes, width, height, body, action, direction, file, catalog ' +
+      'FROM shard_assets',
   )
 
   const map = new Map()
@@ -47,6 +48,7 @@ async function allAssets() {
       width: Number(row.width) || 0,
       height: Number(row.height) || 0,
       body: row.body === null ? null : Number(row.body),
+      action: row.action === null ? null : Number(row.action),
       direction: row.direction === null ? null : Number(row.direction),
       file: row.file || null,
       catalog: row.catalog || null,
@@ -81,6 +83,7 @@ async function saveAssets(rows, meta) {
       r.width ?? 0,
       r.height ?? 0,
       r.body ?? null,
+      r.action ?? null,
       r.direction ?? null,
       r.file ?? null,
       r.catalog ?? meta?.catalog ?? null,
@@ -88,12 +91,13 @@ async function saveAssets(rows, meta) {
 
     await batched(
       conn,
-      'INSERT INTO shard_assets (asset_key, family, sha256, bytes, width, height, body, direction, file, catalog) ' +
-        'VALUES (?,?,?,?,?,?,?,?,?,?) ' +
+      'INSERT INTO shard_assets ' +
+        '(asset_key, family, sha256, bytes, width, height, body, action, direction, file, catalog) ' +
+        'VALUES (?,?,?,?,?,?,?,?,?,?,?) ' +
         'ON DUPLICATE KEY UPDATE family = VALUES(family), sha256 = VALUES(sha256), ' +
         'bytes = VALUES(bytes), width = VALUES(width), height = VALUES(height), ' +
-        'body = VALUES(body), direction = VALUES(direction), file = VALUES(file), ' +
-        'catalog = VALUES(catalog), imported_at = CURRENT_TIMESTAMP',
+        'body = VALUES(body), action = VALUES(action), direction = VALUES(direction), ' +
+        'file = VALUES(file), catalog = VALUES(catalog), imported_at = CURRENT_TIMESTAMP',
       values,
     )
 
@@ -260,17 +264,24 @@ async function countBodies() {
  * first-class state everywhere it is consumed and the expected one for two thirds
  * of the player bodies (§5.2).
  *
- * **The join is pinned to the catalogue key, not merely to the body id.** Today
- * one body has exactly one asset, so `a.body = b.body` alone would be correct —
- * and it would stop being correct the moment phase 6 adds `body/400/a2/f0`, at
- * which point one slug would match dozens of rows and whichever the engine
- * returned last would become the portrait. Naming the key here means that phase
- * adds rows without changing what a creature page shows.
+ * **The join is pinned to the catalogue key, not merely to the body id** — and as
+ * of phase 6 that key is no longer always `a0`. 73 of this client's bodies have
+ * no art at action 0 and are catalogued at the first action that does (§11.2), so
+ * a join hardcoding `a0` would silently drop exactly the creatures this phase
+ * added — a horse among them. It reads the row's own `action` instead, which
+ * still excludes any deeper key a later phase adds (`body/400/a2/f0` does not
+ * equal `body/400/a2`), so one slug still matches at most one row.
+ *
+ * `COALESCE(a.action, 0)` because a row written before this column existed has
+ * NULL there and a NULL inside `CONCAT` makes the whole comparison NULL — which
+ * would have dropped every portrait on the site until the next import, with the
+ * database perfectly correct.
  */
 async function artBySlug() {
   const rows = await query(
     'SELECT b.slug, a.file FROM shard_creature_bodies b ' +
-      "JOIN shard_assets a ON a.asset_key = CONCAT('body/', b.body, '/a0') " +
+      "JOIN shard_assets a ON a.body = b.body AND a.family = 'body' " +
+      "AND a.asset_key = CONCAT('body/', b.body, '/a', COALESCE(a.action, 0)) " +
       "WHERE b.status = 'ok' AND b.body IS NOT NULL AND a.file IS NOT NULL",
   )
 

server/model/shardAssets/shardAssets.model.js

@@ -25,7 +25,13 @@ const log = require('../../core').logger('shardAssets')
 // **The catalogue** (§4.8, §11) is one thumbnail per creature body: the shard
 // walks bodies 0–2047, validates each index entry, decodes the ones that are real
 // and hands back `{ key, sha256 }` first and the PNG second. On a stock client
-// that is **787 sprites**, not the 1,144 the decoder claims — see below.
+// that is **1,095 sprites** — 787 out of the legacy anim files, 235 more out of
+// the UOP packages (phase 4), and 73 more since phase 6, which have no art at
+// action 0 and real art at a later one. Never the 1,144 the decoder claims.
+//
+// A key therefore names its action — `body/820/a23` is a horse whose action 0 is
+// empty — and the key is still one per body. Nothing here treats `a0` as the
+// shape of a body key; the atlas join reads the row's own action (§11.2).
 //
 // **Body resolution** (§8) is the join. The atlas knows a creature by the class
 // name in `Spawns/*.xml`; the client knows it by a body id; nothing in the ServUO
@@ -296,6 +302,7 @@ async function importAssets({ force = false, approve = false } = {}) {
       width: got.width || row.width,
       height: got.height || row.height,
       body: got.body ?? row.body,
+      action: got.action ?? row.action ?? 0,
       direction: got.direction ?? row.direction,
       file: name ?? existing?.file ?? null,
     })

server/model/shardAssets/shardItemArt.model.js

@@ -13,7 +13,7 @@ const log = require('../../core').logger('shardItemArt')
 // ── Why this is not the body catalogue with a different prefix ─────────
 //
 // The bestiary wants every creature, so phase 3 imports a SET: walk a manifest,
-// diff the hashes, fetch what moved. That works because the set is 1,022 rows
+// diff the hashes, fetch what moved. That works because the set is 1,095 rows
 // and one megabyte.
 //
 // This side has no set. The shard's client addresses 49,152 item graphics and

server/test/assetBridge.test.js

@@ -250,6 +250,45 @@ test('a fetch passes the catalogue id and decodes the PNG', async (t) => {
   assert.equal(assets.get('body/12/a0').width, 24)
 })
 
+test('a body catalogued at a later action keeps that action in its row', async (t) => {
+  // §11.2, phase 6. 73 of a stock client's bodies have no art at action 0 and are
+  // catalogued at the first action that does — body 820's is 23, and it is a
+  // horse. The action travels with the row because the atlas join needs it in
+  // SQL; re-deriving it from the key would put a second parser of §5's scheme in
+  // the schema.
+  stub({
+    manifest: [
+      manifestPage([
+        { ...row(12), action: 0 },
+        { key: 'body/820/a23', sha256: 'bb', bytes: 900, width: 68, height: 69, body: 820, action: 23, direction: 1 },
+      ]),
+    ],
+  })
+  t.after(restore)
+
+  const { rows } = await bridge.readManifest({})
+
+  assert.deepEqual(
+    rows.map((r) => [r.key, r.action]),
+    [
+      ['body/12/a0', 0],
+      ['body/820/a23', 23],
+    ],
+  )
+})
+
+test('an overlay older than phase 6 reads as action 0 rather than as unknown', async (t) => {
+  // A phase-3 through phase-5 overlay omits `action` entirely, and every key it
+  // ever produced ended in `a0`. Reading that as null would make the atlas join
+  // COALESCE it back to 0 anyway; reading it as 0 here says so once.
+  stub({ manifest: [manifestPage([row(12)])] })
+  t.after(restore)
+
+  const { rows } = await bridge.readManifest({})
+
+  assert.equal(rows[0].action, 0)
+})
+
 test('an absent asset is a counted row, not a failed fetch', async (t) => {
   // The whole reason this is not an error: two thirds of the playable ghost and
   // gargoyle bodies have no art on a stock client (§5.2), and an import that

server/test/shardAssets.model.test.js

@@ -221,6 +221,37 @@ test('only the keys whose hash moved are fetched', async (t) => {
   assert.equal(result.written, 1)
 })
 
+test('a body catalogued at a later action is imported under that key', async (t) => {
+  // §11.2, phase 6. Body 820 has no art at action 0 and a horse at action 23, so
+  // its key is `body/820/a23` — and the filename, the stored row and the atlas
+  // join all have to agree on that. A name built as `uo-body-820-a0-…` would be
+  // a file nothing ever asks for, with the creature page still showing text.
+  const dir = useTempUploads(t)
+
+  const seen = stubEverything({
+    manifest: manifestOf([
+      { ...row(820, 'new'), key: 'body/820/a23', action: 23 },
+    ]),
+    fetched: {
+      assets: new Map([['body/820/a23', { ...sprite('new'), action: 23 }]]),
+      missing: { absent: 0, unsupported: 0 },
+    },
+  })
+  t.after(restore)
+
+  const result = await model.importAssets({ force: true })
+
+  assert.equal(result.status, 'imported')
+  assert.deepEqual(seen.fetchedKeys, ['body/820/a23'])
+
+  const saved = seen.saved[0]
+  assert.equal(saved.key, 'body/820/a23')
+  assert.equal(saved.action, 23)
+  // Content-addressed, and the stem is the key: the action is IN the filename.
+  assert.equal(saved.file, 'uo-body-820-a23-new.png')
+  assert.ok(fs.existsSync(path.join(dir, model.ART_SUBDIR, saved.file)))
+})
+
 test('an unchanged key whose file is missing from disk is fetched again', async (t) => {
   // The row and the file can disagree — a wiped uploads volume, a restore from a
   // database dump. Trusting the row alone leaves a broken image on a creature

server/utils/assetBridge.js

@@ -332,6 +332,11 @@ async function readManifest({ family = FAMILY } = {}) {
         width: Number(row?.width) || 0,
         height: Number(row?.height) || 0,
         body: Number.isFinite(Number(row?.body)) ? Number(row.body) : null,
+        // Which action the thumbnail came from (§11.2, phase 6). All but 73 of
+        // this client's bodies answer 0; the rest have no art there and are
+        // catalogued deeper, with the key naming the action. An overlay older
+        // than phase 6 omits it, and 0 is the right reading of that.
+        action: Number.isFinite(Number(row?.action)) ? Number(row.action) : 0,
         direction: Number.isFinite(Number(row?.direction)) ? Number(row.direction) : null,
       })
     }
@@ -374,7 +379,7 @@ async function readManifest({ family = FAMILY } = {}) {
 /**
  * The bytes for an explicit list of keys.
  *
- * Returns a Map of key → `{ sha256, bytes, width, height, body, direction, png }`
+ * Returns a Map of key → `{ sha256, bytes, width, height, body, action, direction, png }`
  * where `png` is a Buffer. A key the shard could not serve is **absent from the
  * map** rather than present with a null — the caller then decides what that means
  * for its own row, and the two ways it happens (`absent`, `unsupported`) are
@@ -449,6 +454,7 @@ async function fetchAssets({ keys, catalog } = {}) {
           width: Number(row.width) || 0,
           height: Number(row.height) || 0,
           body: Number.isFinite(Number(row.body)) ? Number(row.body) : null,
+          action: Number.isFinite(Number(row.action)) ? Number(row.action) : null,
           direction: Number.isFinite(Number(row.direction)) ? Number(row.direction) : null,
           // Phase 5's art families carry these; the body catalogue does not, and a
           // consumer that wants neither is unaffected by either.