jqueuniet/dm2-tools

Fork 0

dm2-saves: checkcode missile_termination_flag never cleared; standalone-missile sub-chain uses chain mode instead of single-record mode #169

New issue

Closed

opened 2026-07-04 15:14:47 +02:00 by jqueuniet · 2 comments

jqueuniet commented

2026-07-04 15:14:47 +02:00

(Migrated from codeberg.org)

Problem

Two divergences from the engine's standalone-missile handling in DM2_READ_RECORD_CHECKCODE, verified against skproject/SKWINSPX/src/v5/sksvgame.cpp:945-955:

missile_termination_flag is never cleared. The engine sets ddat.v1d6525 = 1, runs the sub-read, then unconditionally clears it immediately after the call returns. The crate sets it (checkcode.rs:637 decoder, :990 encoder) and never resets it — the comment claims it "persists (consumed by a subsequent type-15 termination record)", but no consumption/clear code exists. The context lives on the shared decoder that spans phases 4→5→6→3→7.
The sub-chain is read with chain mode 1. The engine calls DM2_READ_RECORD_CHECKCODE(-1, &uw_02, vl_18, 0, vl_18) with vl_18 == 0 in this branch: single-record mode, no trailing has-record bit. The crate uses read_chain(0, 0, 1).

Impact

Once any standalone (non-creature-possessed) missile is decoded, every subsequent type-15 record in the entire save is misparsed as a 7-bit termination with early return instead of a cloud body + 1-bit has-index + optional 10-bit — a hard bitstream desync on engine-written saves that contain a standalone missile followed anywhere later by a cloud. The encoder mirrors the bug, so crate-internal round-trips pass; the current fixtures evidently lack the missile→cloud combination.

Bug 2 alone is bit-identical for the two shapes the engine writer emits (empty slot; single termination record), but diverges if the slot ever holds a regular record, and fixing 1+2 together makes the model match the engine exactly (termination lands in sub_records, flag cleared afterwards).

Note: the unit test read_type15_termination_after_missile encodes the termination as a sibling of the missile — a stream shape the engine reader would parse differently (missile with empty sub-chain, then a full cloud). It will need updating along with the fix.

Fix

In both read_chain and write_chain: use single-record mode for the standalone-missile sub-chain and clear missile_termination_flag immediately after the sub-call returns.

## Problem Two divergences from the engine's standalone-missile handling in `DM2_READ_RECORD_CHECKCODE`, verified against `skproject/SKWINSPX/src/v5/sksvgame.cpp:945-955`: 1. **`missile_termination_flag` is never cleared.** The engine sets `ddat.v1d6525 = 1`, runs the sub-read, then **unconditionally clears it** immediately after the call returns. The crate sets it (`checkcode.rs:637` decoder, `:990` encoder) and never resets it — the comment claims it "persists (consumed by a subsequent type-15 termination record)", but no consumption/clear code exists. The context lives on the shared decoder that spans phases 4→5→6→3→7. 2. **The sub-chain is read with chain mode 1.** The engine calls `DM2_READ_RECORD_CHECKCODE(-1, &uw_02, vl_18, 0, vl_18)` with `vl_18 == 0` in this branch: single-record mode, no trailing has-record bit. The crate uses `read_chain(0, 0, 1)`. ## Impact Once any standalone (non-creature-possessed) missile is decoded, **every subsequent type-15 record in the entire save** is misparsed as a 7-bit termination with early return instead of a cloud body + 1-bit has-index + optional 10-bit — a hard bitstream desync on engine-written saves that contain a standalone missile followed anywhere later by a cloud. The encoder mirrors the bug, so crate-internal round-trips pass; the current fixtures evidently lack the missile→cloud combination. Bug 2 alone is bit-identical for the two shapes the engine writer emits (empty slot; single termination record), but diverges if the slot ever holds a regular record, and fixing 1+2 together makes the model match the engine exactly (termination lands in `sub_records`, flag cleared afterwards). Note: the unit test `read_type15_termination_after_missile` encodes the termination as a *sibling* of the missile — a stream shape the engine reader would parse differently (missile with empty sub-chain, then a full cloud). It will need updating along with the fix. ## Fix In both `read_chain` and `write_chain`: use single-record mode for the standalone-missile sub-chain and clear `missile_termination_flag` immediately after the sub-call returns.

jqueuniet commented

2026-07-06 08:47:23 +02:00

(Migrated from codeberg.org)

Investigated in #209 but not applied — the proposed fix regresses ground truth.

Both changes (single-record-mode sub-chain read + clearing missile_termination_flag after the sub-call, per retail sksvgame.cpp:945-955) were implemented and reverted: they break the DOS Beta v0.9 SKSAVE0 fixture. The mode change desyncs decode_full outright; the flag-clear breaks the byte-exact re-encode. That fixture contains a standalone missile whose stream round-trips faithfully only under the current persistent-flag / sibling-termination model, so the engine (Fires.exe at least) does not match the single-record hypothesis — contradicting this issue's assumption that the fixtures lack the missile-combination.

An explanatory NB comment was added to checkcode.rs (commit e271ee59). Keeping this open: resolving the retail-vs-Beta discrepancy needs binary-level tracing (skull.exe / Fires.exe), not a fixture-blind edit.

Investigated in #209 but **not** applied — the proposed fix regresses ground truth. Both changes (single-record-mode sub-chain read + clearing `missile_termination_flag` after the sub-call, per retail `sksvgame.cpp:945-955`) were implemented and reverted: they break the DOS Beta v0.9 `SKSAVE0` fixture. The mode change desyncs `decode_full` outright; the flag-clear breaks the byte-exact re-encode. That fixture contains a standalone missile whose stream round-trips faithfully **only** under the current persistent-flag / sibling-termination model, so the engine (`Fires.exe` at least) does not match the single-record hypothesis — contradicting this issue's assumption that the fixtures lack the missile-combination. An explanatory NB comment was added to `checkcode.rs` (commit e271ee59). Keeping this open: resolving the retail-vs-Beta discrepancy needs binary-level tracing (`skull.exe` / `Fires.exe`), not a fixture-blind edit.

jqueuniet commented

2026-07-16 07:53:23 +02:00

(Migrated from codeberg.org)

Resolved in PR #233 after the binary-level tracing this issue was gated on.

Binary trace of the Beta engine (Fires.exe, LZEXE-unpacked via scripts/unlzexe.py)

Located the CHECKCODE reader/writer through the per-type mask table (initialized image at file 0x46acb, segment 0x3e09, table offset 0x3c3b — the mask block previously documented at 0x46a5f):

Reader (recursive entry 0x1e76a): standalone-missile branch sets the termination flag ds:0x3c85 at 0x1eaa4, recurses with chain-mode argument 0 (single-record: 'push 0' first, checked at the loop bottom 0x1eb1a), and unconditionally clears the flag at 0x1eac6. Creature/chest sub-calls pass chain mode 1; inside-creature flag is ds:0x3c87 (0x1e9a7/0x1e9e9).
Writer (entry 0x1daef): mirrors with its own flag ds:0x3c7d — termination check at 0x1db7d, set 0x1de41, sub-write mode 0, cleared 0x1de60. The single-record epilogue writes no trailing has-record bit (and nothing at all for a 0xfffe slot, same asymmetry as retail).

The Beta engine matches retail (sksvgame.cpp:945-955) exactly. This issue's proposed fix is correct per both engines.

Why the earlier attempt (#209) seemed to refute it

Two things conflated:

The mode change alone is what desyncs the Beta fixture decode; a symmetric flag-clear (decoder+encoder, keeping chain mode 1) actually passes slot 0 — the earlier 'flag-clear breaks byte-exact re-encode' result was likely from an asymmetric attempt.
More fundamentally, byte-exact round-trips cannot arbitrate the model: the Beta decode is truncated at BETA_TILE_CHAIN_MAP_COUNT maps with the residual spliced verbatim, so any self-consistent parse (including a misaligned one) round-trips byte-exact. The 'sane' structures under the persist model are not evidence either — the tile list comes from the dungeon, not the stream.

The Beta fixtures needing the persistent-flag model therefore points at a residual divergence in the Beta tile walk (most likely the trailing-map accounting already tracked in docs/formats/save-record-stream.md), which the persist model was compensating for.

What landed

Retail: engine-exact semantics (single-record sub-chain, flag cleared; termination nests in sub_records; later type-15s parse as clouds). No retail fixture regresses — none combines a standalone missile with a later cloud (Mac inside_skullkeep/skullkeep_troll standalone missiles carry nested terminations, bit-identical under both modes).
Beta: keeps the chain-mode/persistent-flag model gated on set_beta, explicitly documented as a compensating model to revisit with the trailing-maps walk work.
Unit tests for both shapes; new doc section 'Standalone-missile termination: engine vs Beta model (#169)'.

Resolved in PR #233 after the binary-level tracing this issue was gated on. ## Binary trace of the Beta engine (Fires.exe, LZEXE-unpacked via scripts/unlzexe.py) Located the CHECKCODE reader/writer through the per-type mask table (initialized image at file 0x46acb, segment 0x3e09, table offset 0x3c3b — the mask block previously documented at 0x46a5f): - **Reader** (recursive entry 0x1e76a): standalone-missile branch sets the termination flag ds:0x3c85 at 0x1eaa4, recurses with chain-mode argument **0** (single-record: 'push 0' first, checked at the loop bottom 0x1eb1a), and **unconditionally clears the flag at 0x1eac6**. Creature/chest sub-calls pass chain mode 1; inside-creature flag is ds:0x3c87 (0x1e9a7/0x1e9e9). - **Writer** (entry 0x1daef): mirrors with its own flag ds:0x3c7d — termination check at 0x1db7d, set 0x1de41, sub-write mode 0, cleared 0x1de60. The single-record epilogue writes no trailing has-record bit (and nothing at all for a 0xfffe slot, same asymmetry as retail). **The Beta engine matches retail (sksvgame.cpp:945-955) exactly.** This issue's proposed fix is correct per both engines. ## Why the earlier attempt (#209) seemed to refute it Two things conflated: 1. The *mode change alone* is what desyncs the Beta fixture decode; a **symmetric** flag-clear (decoder+encoder, keeping chain mode 1) actually passes slot 0 — the earlier 'flag-clear breaks byte-exact re-encode' result was likely from an asymmetric attempt. 2. More fundamentally, **byte-exact round-trips cannot arbitrate the model**: the Beta decode is truncated at BETA_TILE_CHAIN_MAP_COUNT maps with the residual spliced verbatim, so any self-consistent parse (including a misaligned one) round-trips byte-exact. The 'sane' structures under the persist model are not evidence either — the tile list comes from the dungeon, not the stream. The Beta fixtures needing the persistent-flag model therefore points at a residual divergence in the Beta tile walk (most likely the trailing-map accounting already tracked in docs/formats/save-record-stream.md), which the persist model was compensating for. ## What landed - Retail: engine-exact semantics (single-record sub-chain, flag cleared; termination nests in sub_records; later type-15s parse as clouds). No retail fixture regresses — none combines a standalone missile with a later cloud (Mac inside_skullkeep/skullkeep_troll standalone missiles carry nested terminations, bit-identical under both modes). - Beta: keeps the chain-mode/persistent-flag model gated on set_beta, explicitly documented as a compensating model to revisit with the trailing-maps walk work. - Unit tests for both shapes; new doc section 'Standalone-missile termination: engine vs Beta model (#169)'.

No labels

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

jqueuniet/dm2-tools#169

No description provided.

Rows
Columns