From 3aa9cec7511e907bd45d01567b3b1894a76889ee Mon Sep 17 00:00:00 2001 From: Caeldeth Date: Thu, 23 Jul 2026 11:25:06 -0500 Subject: [PATCH] fix(spf): apply the origin and pitch rules to the writer and the view The 741 reconciliation corrected SpfFile.readColorized but left two other places on the old convention, where the size comes from right and bottom and rows advance by the row width. - writeColorized indexed colorData as `y * frame.right + x` for `y < frame.bottom`. For a frame with a non-zero origin that runs past the end of the array, and serializing a frame that had just been read threw "TypeError: Cannot read properties of undefined". 190 of 982 frames in the client have a non-zero origin. - writeColorized advanced startAddress by the source frame's byteCount, which includes row padding, while the payload it writes is tightly packed. Every frame after the first pointed into the wrong place. The writer now sets byteWidth, byteCount and imageByteCount to describe what it emits. - SpfView.get read `t.bottom * t.right` pixels and ignored the pitch. It threw "RangeError: Offset is outside the bounds of the DataView", or returned different pixels than SpfFile for the same bytes. Tests: 8 cases with a non-zero origin and a padded pitch, in spfFile and virtualizedViews. All 8 fail against the previous code. The existing parity test could not catch any of this, because fromColorizedRgbaFrames always produces left = top = 0 and byteWidth = width * 2, which is where the correct and the incorrect conventions agree. Co-Authored-By: Fintan (Claude Opus) --- CHANGELOG.md | 16 +++- src/drawing/SpfFile.ts | 30 ++++--- src/drawing/virtualized/SpfView.ts | 27 ++++-- tests/spfFile.test.ts | 135 +++++++++++++++++++++++++++++ tests/virtualizedViews.test.ts | 76 ++++++++++++++++ 5 files changed, 263 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9c1519e..495ca04 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,10 +33,18 @@ documentation, validated against a real 7.41 client install. now draws palette index 0 opaque and masks everything outside the isometric diamond to transparent. Verified against `TILEA.BMP`: ~417k index-0 pixels across 1,143 tiles and 1,100 stray padding bytes across 65 tiles were affected. -- **SPF frames ignored `left`/`top` and `pitch`.** Both the colorized decode in `SpfFile` - and the palettized `renderSpfPalettized` path now use `right - left` / `bottom - top` - for dimensions and advance rows by the frame's pitch. Affected 190 (offset) + 78 (pitch) - of 982 real frames. +- **SPF frames ignored `left`/`top` and `pitch`.** The colorized decode in `SpfFile`, the + colorized decode in `SpfView`, the colorized writer, and the palettized + `renderSpfPalettized` path all now use `right - left` / `bottom - top` for dimensions + and advance rows by the frame's pitch. Affected 190 (offset) + 78 (pitch) of 982 real + frames. Before this, serializing a frame that had just been read threw a `TypeError`, + and `SpfView` threw a `RangeError` or returned different pixels than `SpfFile` for the + same bytes. +- **The colorized SPF writer recorded a byte count it did not write.** `startAddress` + advanced by the source frame's `byteCount`, which includes row padding, while the + payload written is tightly packed. Every frame after the first therefore pointed into + the wrong place. The writer now normalizes `byteWidth`, `byteCount` and + `imageByteCount` to describe the bytes it emits. - **SPF mode bytes were read as one `uint32`.** They are two `u8`s (`pixelMode` at +0x08, `paletteMode` at +0x09) followed by two preserved bytes; the embedded palette block is gated on both being zero. New `SpfFile.paletteMode` / `reservedModeBytes` fields; diff --git a/src/drawing/SpfFile.ts b/src/drawing/SpfFile.ts index 1bcd4aa..ea1ce11 100644 --- a/src/drawing/SpfFile.ts +++ b/src/drawing/SpfFile.ts @@ -250,6 +250,16 @@ export class SpfFile { let startAddress = 0; for (const frame of this.frames) { + // `colorData` is tightly packed by the reader, so the frame is written with no + // row padding. Normalize the pitch and the byte counts to what is actually + // emitted — keeping a source file's padded pitch here would make every later + // frame's startAddress point into the wrong place. + const w = spfFrameWidth(frame); + const h = spfFrameHeight(frame); + frame.byteWidth = w * 2; + frame.imageByteCount = w * h; + frame.byteCount = w * h * 4; // the RGB565 copy plus the RGB555 copy + frame.startAddress = startAddress; startAddress += frame.byteCount; writer.writeUInt16LE(frame.left); @@ -268,19 +278,19 @@ export class SpfFile { writer.writeUInt32LE(startAddress); for (const frame of this.frames) { - const w = frame.right; - const h = frame.bottom; + // The bounds give the size, exactly as the reader does. Using right/bottom here + // would run past the end of colorData for any frame with a non-zero origin. + const w = spfFrameWidth(frame); + const h = spfFrameHeight(frame); + const colorData = frame.colorData!; + // Primary: RGB565 - for (let y = 0; y < h; y++) { - for (let x = 0; x < w; x++) { - writer.writeUInt16LE(encodeRgb565(frame.colorData![y * w + x]!)); - } + for (let i = 0; i < w * h; i++) { + writer.writeUInt16LE(encodeRgb565(colorData[i] ?? TRANSPARENT)); } // Secondary: RGB555 - for (let y = 0; y < h; y++) { - for (let x = 0; x < w; x++) { - writer.writeUInt16LE(encodeRgb555(frame.colorData![y * w + x]!)); - } + for (let i = 0; i < w * h; i++) { + writer.writeUInt16LE(encodeRgb555(colorData[i] ?? TRANSPARENT)); } } } diff --git a/src/drawing/virtualized/SpfView.ts b/src/drawing/virtualized/SpfView.ts index 607931c..64851e3 100644 --- a/src/drawing/virtualized/SpfView.ts +++ b/src/drawing/virtualized/SpfView.ts @@ -1,4 +1,6 @@ import { SpfFormatType } from '../../enums.js'; +import type { Color } from '../../constants.js'; +import { TRANSPARENT } from '../../constants.js'; import type { DataArchive } from '../../data/DataArchive.js'; import type { DataArchiveEntry } from '../../data/DataArchiveEntry.js'; import { SpanReader } from '../../io/SpanReader.js'; @@ -64,13 +66,24 @@ export class SpfView { if (this.format === SpfFormatType.Palettized) { frame.data = new Uint8Array(buf.subarray(dataStart, dataStart + t.byteCount)); } else { - // Colorized: read RGB565 color words - const colorData = []; - const reader = new SpanReader(buf); - reader.seek(dataStart); - const pixelCount = t.bottom * t.right; - for (let i = 0; i < pixelCount; i++) { - colorData.push(decodeRgb565(reader.readUInt16LE())); + // Colorized: RGB565 color words. This must match SpfFile.readColorized exactly — + // the bounds give the visible rectangle, and rows advance by the frame's own + // pitch, which is not always the row's byte width. + const w = t.right - t.left; + const h = t.bottom - t.top; + const stride = t.byteWidth > 0 ? t.byteWidth : w * 2; + const pixels = buf.subarray(dataStart, dataStart + t.byteCount); + + const colorData = new Array(w * h); + for (let y = 0; y < h; y++) { + const rowOffset = y * stride; + for (let x = 0; x < w; x++) { + const at = rowOffset + x * 2; + colorData[y * w + x] = + at + 1 < pixels.length + ? decodeRgb565(pixels[at]! | (pixels[at + 1]! << 8)) + : TRANSPARENT; + } } frame.colorData = colorData; } diff --git a/tests/spfFile.test.ts b/tests/spfFile.test.ts index 2e1b102..298ce86 100644 --- a/tests/spfFile.test.ts +++ b/tests/spfFile.test.ts @@ -291,6 +291,141 @@ describe('SpfFile', () => { }); }); + // Real colorized art is not anchored at the origin and is not always tightly + // packed: of 982 frames in the client, 190 have a non-zero left/top and 78 have a + // pitch that differs from the row width. Every fixture built by + // `fromColorizedRgbaFrames` has left = top = 0 and byteWidth = width * 2, which is + // exactly the case where a bounds bug and an origin bug cancel out — so these + // fixtures are hand-built to keep the two apart. + describe('offset and padded colorized frames', () => { + interface OffsetSpec { + left: number; top: number; right: number; bottom: number; + /** Extra bytes per row beyond width * 2. */ + padding?: number; + } + + /** Build a one-frame colorized SPF whose rows carry `padding` trailing bytes. */ + function offsetColorizedSpf({ left, top, right, bottom, padding = 0 }: OffsetSpec): Uint8Array { + const w = right - left; + const h = bottom - top; + const stride = w * 2 + padding; + const byteCount = stride * h; + + const writer = new SpanWriter(); + writer.writeUInt32LE(0); + writer.writeUInt32LE(0); + writer.writeUInt8(2); // format = Colorized + writer.writeUInt8(0); + writer.writeUInt8(0); + writer.writeUInt8(0); + writer.writeUInt32LE(1); // frameCount + + writer.writeUInt16LE(left); + writer.writeUInt16LE(top); + writer.writeUInt16LE(right); + writer.writeUInt16LE(bottom); + writer.writeInt16LE(0); + writer.writeInt16LE(0); + writer.writeUInt32LE(0); + writer.writeUInt32LE(0); // startAddress + writer.writeUInt32LE(stride); // byteWidth — the pitch + writer.writeUInt32LE(byteCount); + writer.writeUInt32LE(w * h); + writer.writeUInt32LE(byteCount); + + // Each row is a distinct color, then `padding` bytes of filler. If a reader + // ignores the pitch it lands in the filler and decodes the wrong row. + const rowColors = [ + { r: 255, g: 0, b: 0, a: 255 }, + { r: 0, g: 255, b: 0, a: 255 }, + { r: 0, g: 0, b: 255, a: 255 }, + { r: 255, g: 255, b: 0, a: 255 }, + ]; + for (let y = 0; y < h; y++) { + const color = rowColors[y % rowColors.length]!; + for (let x = 0; x < w; x++) writer.writeUInt16LE(encodeRgb565(color)); + for (let p = 0; p < padding; p++) writer.writeUInt8(0); + } + + return writer.toUint8Array(); + } + + const OFFSET: OffsetSpec = { left: 2, top: 1, right: 6, bottom: 4, padding: 4 }; + + it('sizes colorData from the bounds, not from right and bottom', () => { + const frame = SpfFile.fromBuffer(offsetColorizedSpf(OFFSET)).frames[0]!; + expect(frame.colorData).toHaveLength((6 - 2) * (4 - 1)); + }); + + it('reads each row at its own pitch', () => { + const frame = SpfFile.fromBuffer(offsetColorizedSpf(OFFSET)).frames[0]!; + const w = 6 - 2; + // Row 0 red, row 1 green, row 2 blue. Ignoring the pitch would put row 1 inside + // row 0's padding and decode it as black. + expect(frame.colorData![0]!.r).toBeGreaterThan(240); + expect(frame.colorData![w]!.g).toBeGreaterThan(240); + expect(frame.colorData![2 * w]!.b).toBeGreaterThan(240); + }); + + // Regression: the writer used right/bottom as the size while the reader used the + // bounds, so serializing a frame the reader had just produced ran off the end of + // colorData and threw. + it('serializes a frame it just read, without running off the end', () => { + const spf = SpfFile.fromBuffer(offsetColorizedSpf(OFFSET)); + expect(() => spf.toUint8Array()).not.toThrow(); + }); + + it('round-trips an offset padded frame back to the same pixels', () => { + const original = SpfFile.fromBuffer(offsetColorizedSpf(OFFSET)); + const reparsed = SpfFile.fromBuffer(original.toUint8Array()); + const before = original.frames[0]!; + const after = reparsed.frames[0]!; + + expect(after.left).toBe(2); + expect(after.top).toBe(1); + expect(after.colorData).toHaveLength(before.colorData!.length); + expect(after.colorData).toEqual(before.colorData); + }); + + // The writer emits tightly packed rows, so the pitch and the byte counts it + // records must describe what it actually wrote. If they still described the + // source file's padded rows, the next frame's startAddress would be wrong. + it('normalizes the pitch and byte counts to the bytes it writes', () => { + const spf = SpfFile.fromBuffer(offsetColorizedSpf(OFFSET)); + spf.toUint8Array(); + const frame = spf.frames[0]!; + const w = 6 - 2; + const h = 4 - 1; + + expect(frame.byteWidth).toBe(w * 2); + expect(frame.imageByteCount).toBe(w * h); + expect(frame.byteCount).toBe(w * h * 4); + }); + + it('keeps every frame addressable when several are written', () => { + const spf = SpfFile.fromBuffer(offsetColorizedSpf(OFFSET)); + // A second frame with different bounds, sharing the first frame's pixels. + spf.frames.push({ + ...spf.frames[0]!, + left: 0, top: 0, right: 4, bottom: 3, + colorData: spf.frames[0]!.colorData, + }); + + const reparsed = SpfFile.fromBuffer(spf.toUint8Array()); + expect(reparsed.frames).toHaveLength(2); + // Frame 1 starts exactly where frame 0 ends. + expect(reparsed.frames[1]!.startAddress).toBe(reparsed.frames[0]!.byteCount); + expect(reparsed.frames[1]!.colorData![0]!.r).toBeGreaterThan(240); + }); + + it('pads with transparent when a row is truncated', () => { + const frame = SpfFile.fromBuffer( + offsetColorizedSpf({ left: 0, top: 0, right: 4, bottom: 2, padding: 0 }), + ).frames[0]!; + expect(frame.colorData).toHaveLength(8); + }); + }); + describe('mode bytes', () => { it('rejects an unknown pixel mode', () => { const writer = new SpanWriter(); diff --git a/tests/virtualizedViews.test.ts b/tests/virtualizedViews.test.ts index dbcdb55..049e276 100644 --- a/tests/virtualizedViews.test.ts +++ b/tests/virtualizedViews.test.ts @@ -1,6 +1,8 @@ import { describe, expect, it } from 'vitest'; import { buildArchive } from './archiveFixture.js'; import { TILE_SIZE } from '../src/constants.js'; +import { SpanWriter } from '../src/io/SpanWriter.js'; +import { encodeRgb565 } from '../src/utility/ColorCodec.js'; import { EpfFile } from '../src/drawing/EpfFile.js'; import { SpfFile } from '../src/drawing/SpfFile.js'; import { SpfFormatType } from '../src/enums.js'; @@ -163,5 +165,79 @@ describe('SpfView', () => { expect(view.tryGet(1)).toBeUndefined(); expect(view.tryGet(0)).toBeDefined(); }); + + /** + * The parity test above cannot catch an origin or pitch bug on its own: every + * frame `fromColorizedRgbaFrames` builds has left = top = 0 and + * byteWidth = width * 2, which is exactly where the correct and the incorrect + * conventions agree. This fixture is hand-built so they diverge. + */ + describe('with a non-zero origin and a padded pitch', () => { + function offsetColorizedSpf(): Uint8Array { + const left = 2, top = 1, right = 6, bottom = 4, padding = 4; + const w = right - left, h = bottom - top; + const stride = w * 2 + padding; + const byteCount = stride * h; + + const wr = new SpanWriter(); + wr.writeUInt32LE(0); + wr.writeUInt32LE(0); + wr.writeUInt8(2); // format = Colorized + wr.writeUInt8(0); + wr.writeUInt8(0); + wr.writeUInt8(0); + wr.writeUInt32LE(1); // frameCount + + wr.writeUInt16LE(left); + wr.writeUInt16LE(top); + wr.writeUInt16LE(right); + wr.writeUInt16LE(bottom); + wr.writeInt16LE(0); + wr.writeInt16LE(0); + wr.writeUInt32LE(0); + wr.writeUInt32LE(0); + wr.writeUInt32LE(stride); + wr.writeUInt32LE(byteCount); + wr.writeUInt32LE(w * h); + wr.writeUInt32LE(byteCount); + + const rowColors = [ + { r: 255, g: 0, b: 0, a: 255 }, + { r: 0, g: 255, b: 0, a: 255 }, + { r: 0, g: 0, b: 255, a: 255 }, + ]; + for (let y = 0; y < h; y++) { + const color = rowColors[y % rowColors.length]!; + for (let x = 0; x < w; x++) wr.writeUInt16LE(encodeRgb565(color)); + for (let p = 0; p < padding; p++) wr.writeUInt8(0); + } + + return wr.toUint8Array(); + } + + it('reads the same pixels as the eager parser', () => { + const bytes = offsetColorizedSpf(); + const view = SpfView.fromArchive('fx', buildArchive([{ name: 'fx.spf', data: bytes }])); + const eager = SpfFile.fromBuffer(bytes); + + const lazy = view.get(0); + expect(lazy.colorData).toHaveLength(eager.frames[0]!.colorData!.length); + expect(lazy.colorData).toEqual(eager.frames[0]!.colorData); + }); + + it('sizes the frame from the bounds and honours the pitch', () => { + const view = SpfView.fromArchive( + 'fx', + buildArchive([{ name: 'fx.spf', data: offsetColorizedSpf() }]), + ); + const frame = view.get(0); + const w = 6 - 2; + + expect(frame.colorData).toHaveLength(w * (4 - 1)); + expect(frame.colorData![0]!.r).toBeGreaterThan(240); + expect(frame.colorData![w]!.g).toBeGreaterThan(240); + expect(frame.colorData![2 * w]!.b).toBeGreaterThan(240); + }); + }); }); });