Conversation
Okumura's reference implementation zero-fills the 4096-byte window, and Extreme-G's streams expect that. Max Payne's `RA->` records use the same encoding but prime it with spaces, which changes what a match reaching back before the first literal produces.
InstallShield cabinets are not particular to Incoming; a Max Payne install disc ships them too. Move `find_unshield` and `run_unshield` alongside the other tool locators and leave `dade.incoming.tools` with the tools only that extractor uses.
Reads the formats Remedy built on its `rl` library: RAS archives and their seeded stream cipher, the `RA->` LZSS blocks inside them, the tagged `R_MemoryFile` streams every asset is written as, the `.ldb` level database, and the `.kfs`/`.kf2` models NPCs and pickups are drawn with. `dade maxpane ldb2glb` writes one `.glb` per level, in about nine seconds for all twenty-nine. Four things about the level format are easy to get backwards, and each one is a visible defect: - A level is not one space. Every room is modelled about its own origin, and the exits carry the transforms that put them together; skip that and all 703 room pairs of `Part1_Level1` overlap. - A material's second string is the material's name, not a filename. Only the level's category table says which image it draws with, and matching on filename leaves a fifth of a level untextured. - A face's corner count is not its number of sides. The editor drops extra corners along edges shared with other faces, so a fan can start with a straight line, and taking the winding from that turns 822 faces inside out. - A model's texture coordinates run V negative and are used exactly as stored, Direct3D's wrapping doing the rest. Negating them for a tidy `0..1` range flips every skin upside down. Skybox faces are drawn rather than dropped, since they are what closes a level off where it opens to the air, and `dade.maxpane.decals` lifts graffiti, signage and switchable surfaces off the plane they share with the wall behind them, which the engine's tree order used to keep apart.
There was a problem hiding this comment.
🟡 Changes recommended
Several robustness/documentation issues in newly added maxpane modules can cause crashes or mislead API consumers (e.g., unguarded division by zero, missing archive/model input validation, and conflicting docs).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR adds a new dade maxpane CLI surface and supporting readers/converters for Max Payne (2001) assets, including RAS/MPM archives and level-to-binary-glTF export, while also generalizing a couple of shared utilities (lzss ring fill and unshield tooling) for reuse across games.
Changes:
- Introduces the
dade.maxpanepackage (blocks/crypto/memoryfile/ras/model/etc.) plus Click commands (ras-list,ras-extract,inspect-tags,ldb-textures,ldb2glb). - Adds extensive
tests/maxpanecoverage and CLI wiring/docs for the new game module. - Moves InstallShield
unshieldinvocation intodade.common.toolsand extends LZSS decompression to support a configurable ring-buffer fill byte.
File summaries
| File | Description |
|---|---|
| tests/test_cli.py | Registers maxpane in the top-level CLI test matrix. |
| tests/maxpane/test_ras.py | Validates RAS header/table parsing, member extraction, and integrity detection. |
| tests/maxpane/test_models_command.py | Tests discovery/loading of NPC/pickup models and texture search behavior. |
| tests/maxpane/test_model.py | Exercises .kfs/.kf2 model parsing edge cases and coordinate/orientation handling. |
| tests/maxpane/test_memoryfile.py | Tests tagged R_MemoryFile primitive decoding and stream walking. |
| tests/maxpane/test_ldb.py | Validates .ldb level decoding including geometry/material/placements/lightmaps. |
| tests/maxpane/test_gltf.py | Validates GLB structure/materials/animations/lightmaps/decals behavior (export correctness). |
| tests/maxpane/test_decals.py | Tests coplanar-surface layering logic for decal lifting. |
| tests/maxpane/test_crypto.py | Verifies archive cipher stepping and decrypt behavior. |
| tests/maxpane/test_commands.py | CLI-level integration tests for new maxpane commands and archive source resolution. |
| tests/maxpane/test_blocks.py | Tests RA-> / RC-> wrappers: detect/decrypt/decompress/unwrap behavior. |
| tests/maxpane/conftest.py | Provides fixtures for constructing synthetic RAS/LDB/models and crypto helpers. |
| tests/incoming/test_tools.py | Updates incoming tests after moving unshield support out of dade.incoming.tools. |
| tests/common/test_tools.py | Adds common tests for run_unshield in its new shared location. |
| README.md | Documents the new dade maxpane command set and usage examples. |
| docs/api/maxpane.rst | Adds API docs index page for dade.maxpane modules. |
| docs/api/index.rst | Links maxpane into the API docs toctree. |
| dade/maxpane/typing.py | Adds typed structures for decoded Max Payne assets. |
| dade/maxpane/ras.py | Implements RAS/MPM archive parsing and member extraction. |
| dade/maxpane/model.py | Implements .kfs/.kf2 model parsing (chunked R_MemoryFile streams). |
| dade/maxpane/memoryfile.py | Implements the tagged stream decoder and iterator utilities. |
| dade/maxpane/main.py | Adds the Click group and wires subcommands for dade maxpane. |
| dade/maxpane/decals.py | Adds logic to separate coplanar faces to avoid depth fighting in viewers. |
| dade/maxpane/crypto.py | Implements the seeded stream cipher used by RAS archives/blocks. |
| dade/maxpane/commands/utils.py | Adds shared command helpers (debug option wiring). |
| dade/maxpane/commands/sources.py | Resolves archives from files/dirs/cabs/disc images and stages cabinets for unshield. |
| dade/maxpane/commands/ras_list.py | Implements dade maxpane ras-list. |
| dade/maxpane/commands/ras_extract.py | Implements dade maxpane ras-extract. |
| dade/maxpane/commands/models.py | Loads models/textures from a game database for placement rendering. |
| dade/maxpane/commands/ldb2glb.py | Implements parallel .ldb → .glb conversion. |
| dade/maxpane/commands/ldb_textures.py | Extracts embedded textures from .ldb files. |
| dade/maxpane/commands/inspect_tags.py | Implements inspect-tags for tagged-stream introspection. |
| dade/maxpane/commands/init.py | Declares the commands package. |
| dade/maxpane/blocks.py | Implements block wrapper detection/peeling and LZSS decrypt/decompress plumbing. |
| dade/maxpane/init.py | Adds package-level description for dade.maxpane. |
| dade/main.py | Registers maxpane as a new top-level game command group. |
| dade/incoming/tools.py | Removes unshield from incoming tools exports/implementation. |
| dade/incoming/sources.py | Updates imports to use shared dade.common.tools.run_unshield. |
| dade/common/tools.py | Adds shared find_unshield / run_unshield support. |
| dade/common/lz.py | Extends LZSS decompressor to accept a fill byte for ring initialization. |
| .vscode/dictionary.txt | Adds Max Payne- and glTF-related terms to the cspell dictionary. |
Review details
- Files reviewed: 44/44 changed files
- Comments generated: 7
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
There are concrete security/robustness issues (path traversal risk when writing extracted files and a short-buffer unwrap crash) that should be fixed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
dade/maxpane/blocks.py:152
unwrap()only checks for the compressed header size (12 bytes) before attempting to decrypt anRC->block, but encrypted blocks need a 16-byte header. A short buffer starting withRC->will raisestruct.errorinstead of cleanly stopping/raising a ValueError.
while len(data) >= _COMPRESSED_HEADER_SIZE:
if is_encrypted(data):
data = decrypt_block(data)
layers.append('crypt')
dade/maxpane/model.py:88
_chunks()usesstruct.unpack_from()without guarding against a truncated chunk header. For malformed/truncated inputs this raisesstruct.error, bypassing the intendedInvalidModelErrorerror surface documented byread_model().
while offset < end:
if data[offset] != BasicType.CHUNK:
return
identifier, version, size = struct.unpack_from('<3I', data, offset + 1)
if size < CHUNK_HEADER_SIZE or offset + size > end:
dade/maxpane/model.py:12
- The module docstring says texture V is "flipped", but
read_model()deliberately keeps UVs exactly as stored (tests assert negative V is preserved). This is likely to mislead future readers/maintainers.
Models are Z-up, the convention of the tool that exported them, while the game and glTF are both
Y-up; positions and normals are rotated on the way out so a character stands up. Texture V runs
negative and is flipped for the same reason.
- Files reviewed: 44/44 changed files
- Comments generated: 2
- Review effort level: Lite
`[pre-commit.ci] pre-commit autoupdate` moved the hook to v0.16.5 on its own. The project pins `ruff==0.16.4` and `exclude-newer` is a week, so 0.16.5 -- published two days ago -- cannot be installed here at all, and the hook was enforcing `pytest-fixture-autouse`, a rule it added, over twelve fixtures the installed Ruff has no opinion about. Every pull request failed on it. Tatsh/wiswa#90 stops the two drifting apart again by taking the rev from the pinned version.
There was a problem hiding this comment.
🔵 Needs a closer look
A few newly added parsing utilities can raise unintended exception types or silently accept invalid tags, which can lead to confusing CLI failures and incorrect decoding on malformed input.
Review details
Suppressed comments (4)
Previously missed (2) — in code that hasn't changed since the last review.
dade/maxpane/memoryfile.py:210
read_intcurrently accepts any tag present inTAG_SIZES(including FLOAT/VECTOR/MAP/PAIR), so malformed data can be silently mis-parsed as an integer instead of raisingValueError, which contradicts the function contract and can desync decoding.
tag = data[offset]
if tag not in TAG_SIZES or tag in {BasicType.ARRAY, BasicType.CHUNK}:
msg = f'Not an integer tag at offset {offset}: 0x{tag:02x}.'
raise ValueError(msg)
dade/maxpane/ras.py:71
_read_nameusesdata.index(b"\x00", offset)which raisesValueErroron malformed/truncated tables; this bypassesInvalidArchiveErrorhandling in CLI commands and can surface as an unhandled exception.
def _read_name(data: bytes, offset: int) -> tuple[str, int]:
end = data.index(b'\x00', offset)
return data[offset:end].decode('latin-1'), end + 1
dade/maxpane/ras.py:156
read_directorytrusts the file table's directory index; a corrupted archive can trigger anIndexErroratdirectories[directory]instead of producing a clearInvalidArchiveError. Validating the index makes failures deterministic and user-friendly.
path=(directories[directory].name + name).replace('\\', '/').lstrip('/'),
size=size,
dade/maxpane/model.py:12
- The module docstring says texture V “is flipped”, but the implementation/tests keep V exactly as stored (including negative values) and rely on wrapping; the docstring should match the actual behavior to avoid incorrect downstream changes.
Models are Z-up, the convention of the tool that exported them, while the game and glTF are both
Y-up; positions and normals are rotated on the way out so a character stands up. Texture V runs
negative and is flipped for the same reason.
- Files reviewed: 45/45 changed files
- Comments generated: 0 new
- Review effort level: Lite
…ather than crash Review of #37 found nine things, all of them real. Two let an archive or a level decide where a file lands: `ras-extract` joined a member's stored path straight onto the output directory, and `ldb-textures` stripped drive letters and separators from an authored Windows path but left `..` alone. Both now keep to the directory they were given. Five turned malformed input into an exception the caller was not told about: a RAS header shorter than 44 bytes, a file table naming a directory the archive does not hold, a chunk header cut off mid-stream, an `RC->` block long enough to look compressed and too short to decrypt, and a face index count read without the plausibility guard its neighbours use. A negative index was worse than an exception -- in range for Python, so it quietly picked a vertex off the far end of the pool -- and `inspect-tags` divided by the length of an asset that could be empty. The last two were documentation that stopped being true: `model` still said texture V is flipped, which it stopped being when the skins were fixed, and `Vector3` said centimetres when a level unit is about a metre -- `gognitti_vinnie_l0.kfs` is a man 1.88 units tall.
There was a problem hiding this comment.
🟡 Changes recommended
A few verified error-handling and robustness issues can currently surface as misleading variables or uncaught exceptions on malformed inputs, and should be fixed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
Previously missed (3) — in code that hasn't changed since the last review.
dade/maxpane/commands/ras_extract.py:94
_unpack()returns(total_bytes, member_count), but the loop assigns it towritten, members, which makes the subsequenttotal += written/count += memberslogic correct but misleading. Renaming the variables to reflect what they contain will prevent future mistakes (e.g., printing or reusing the wrong value).
for label, data in iter_archives(source):
written, members = _unpack(label, data, patterns, output_dir, raw=raw)
total += written
count += members
dade/maxpane/ras.py:71
_read_name()usesdata.index(b"\x00", offset)which raisesValueErrorif the directory/file tables are truncated or malformed. That exception is currently uncaught and will bypass the CLI’sInvalidArchiveErrorhandling, causing a crash on bad inputs; wrap it and raiseInvalidArchiveErrorinstead.
def _read_name(data: bytes, offset: int) -> tuple[str, int]:
end = data.index(b'\x00', offset)
return data[offset:end].decode('latin-1'), end + 1
dade/maxpane/model.py:391
- In packed meshes,
_read_faces()unpackscount16-bit indices without checking the chunk boundary. If the count is inconsistent with the chunk size,struct.unpack_fromwill raisestruct.error, which bypassesInvalidModelErrorand can crash callers. Validatecursor + 2*count <= endand raiseInvalidModelErroron overflow.
count, cursor = _read_count(data, offset)
if not packed:
return _read_face_indices(data, cursor, end)
indices = struct.unpack_from(f'<{count}H', data, cursor)
return [tuple(indices[at:at + _TRIANGLE]) for at in range(0, count - 2, _TRIANGLE)]
- Files reviewed: 45/45 changed files
- Comments generated: 1
- Review effort level: Lite
Slicing a buffer that stops inside the file table hands back a short table instead of failing, so the walk over it died on `ValueError: subsection not found` from a name that was never there. The header already had a length check; the tables it describes did not.
There was a problem hiding this comment.
🔵 Needs a closer look
There are a few correctness/robustness issues in newly added parsing utilities (notably read_int tag validation and consistent InvalidArchiveError wrapping) that should be fixed before approval.
Review details
Suppressed comments (4)
Previously missed (3) — in code that hasn't changed since the last review.
dade/maxpane/memoryfile.py:210
read_intcurrently accepts many non-integer tags (e.g. FLOAT, MAP, PAIR) because it only excludes ARRAY and CHUNK. This can silently mis-parse corrupt streams (notably string length prefixes) instead of raisingValueErroras the docstring promises.
tag = data[offset]
if tag not in TAG_SIZES or tag in {BasicType.ARRAY, BasicType.CHUNK}:
msg = f'Not an integer tag at offset {offset}: 0x{tag:02x}.'
raise ValueError(msg)
width = TAG_SIZES[tag]
dade/maxpane/commands/ras_extract.py:94
- The
_unpackhelper returns(total_bytes, members_written), but the caller assigns towritten, members. The logic is correct, but the variable names are swapped and make future edits error-prone.
for label, data in iter_archives(source):
written, members = _unpack(label, data, patterns, output_dir, raw=raw)
total += written
count += members
dade/maxpane/ras.py:155
_read_name/_system_timeerrors (missing NUL terminator, truncated SYSTEMTIME) currently surface asValueError/struct.errorfrom inside the loop rather thanInvalidArchiveError, even thoughread_directorydocumentsInvalidArchiveErrorfor malformed tables. Wrapping these keeps the public error type consistent.
This issue also appears on line 159 of the same file.
for _ in range(header.directory_count):
name, offset = _read_name(directory_table, offset)
directories.append(RASDirectory(modified=_system_time(directory_table, offset), name=name))
offset += _SYSTEMTIME_SIZE
dade/maxpane/ras.py:166
- Like the directory table loop, the file table loop can raise
ValueError/struct.errordirectly on malformed content (missing NUL terminator, truncated fixed-width fields). Converting these toInvalidArchiveErrormakes error handling consistent for callers and Click commands.
for _ in range(header.file_count):
name, offset = _read_name(file_table, offset)
size, stored_size, _, directory, _, _ = struct.unpack_from('<6I', file_table, offset)
if directory >= len(directories):
msg = (f'`{name}` names directory {directory} of {len(directories)}.')
raise InvalidArchiveError(msg)
modified = _system_time(file_table, offset + 24)
offset += _FILE_FIELDS_SIZE
- Files reviewed: 45/45 changed files
- Comments generated: 0 new
- Review effort level: Lite
`read_int` took anything in the tag table but an array or a chunk, so a `FLOAT` came back as its bit pattern and a `MAP`, whose width is nought, came back as a silent zero -- neither the `ValueError` the docstring promises. It now names the sixteen integral tags outright. All 29 levels and all 987 models still read, so nothing shipped relies on the looser set. The two table walks in `read_directory` were the other half of the same complaint: a name with no terminator or a field cut short reached the caller as `ValueError` or `struct.error` from inside `_read_name`, where the function documents `InvalidArchiveError`. They move into `_read_tables` and what comes out of it is translated. Also renames a pair of variables in `ras-extract` that had bytes and members the wrong way round.
There was a problem hiding this comment.
🔵 Needs a closer look
The model parser currently lacks chunk-boundary length checks in a couple of packed-read paths, which can read across chunk boundaries or raise raw struct.error/IndexError instead of a deterministic InvalidModelError.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
dade/maxpane/model.py:356
- When reading the positions chunk, the code trusts the element count and then reads vectors without ensuring the data fits inside the current chunk (
tail). For malformed/corrupt files this can read into the next chunk (or raisestruct.error/IndexError) instead of raisingInvalidModelErrortied to the offending chunk.
This issue also appears on line 387 of the same file.
elif identifier == _POSITIONS:
count, cursor = _read_count(data, body)
positions, cursor = _read_vectors(data, cursor, count, packed=packed)
if packed:
normals, _ = _read_vectors(data, cursor, count, packed=True)
dade/maxpane/model.py:391
- For packed face data,
struct.unpack_fromreadscount16-bit indices starting atcursorwithout checking thatcount * 2bytes are available before the end of this chunk (end). This can read across chunk boundaries (or raisestruct.error) on malformed inputs instead of raisingInvalidModelErrorwith a clear message.
count, cursor = _read_count(data, offset)
if not packed:
return _read_face_indices(data, cursor, end)
indices = struct.unpack_from(f'<{count}H', data, cursor)
return [tuple(indices[at:at + _TRIANGLE]) for at in range(0, count - 2, _TRIANGLE)]
- Files reviewed: 45/45 changed files
- Comments generated: 0 new
- Review effort level: Lite
A count read out of a file says how much to read, not how much is there. Both packed paths took it at its word: a positions chunk claiming a thousand vectors when it holds one read the chunks behind it and called the result geometry, and a face chunk did the same with its index buffer, or fell over with `struct.error` when the file simply ended. Both now take the chunk's end and refuse a run that will not fit. All 987 shipped models and all 29 levels still read, so the bound is not tighter than the data.
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces a large new feature area (multiple binary format readers plus a glTF exporter and CLI surface) that warrants final human review despite strong test coverage.
Review details
- Files reviewed: 45/45 changed files
- Comments generated: 0 new
- Review effort level: Lite
Adds
dade maxpane, a reader and converter for the formats Remedy built on itsrllibrary forMax Payne (2001): RAS archives and their seeded stream cipher, the
RA->LZSS blocks insidethem, the tagged
R_MemoryFilestreams every asset is written as, the.ldblevel database, andthe
.kfs/.kf2models NPCs and pickups are drawn with.dade maxpane ldb2glbwrites one.glbper level — all twenty-nine in about nine seconds, withrooms assembled into one world, textures and alpha masks resolved, baked lighting atlases, named
nodes for every NPC and pickup, and every prop animation the level can play.
Two shared changes come with it:
dade.common.lztakes afillbyte, because Max Payne primes the LZSS ring buffer with spacesrather than NULs.
find_unshield/run_unshieldmove fromdade.incoming.toolstodade.common.tools, since aMax Payne install disc ships InstallShield cabinets too.
Checks run locally
yarn qa— clean (mypy, pyright, ty, ruff, cspell, prettier/yapf/markdownlint)uv run pytest tests/maxpane --cov=dade.maxpane— 225 passed, 100% statement and branchyarn gen-docs— builds; the only warning is a missing localgraphvizbinarydade maxpane ldb2glbover the shipped data — 29/29 levelsThe eight failing
tests/bitrocktests on this machine are pre-existing and environmental(
RuntimeError: Failed to find CUDA headers); they are untouched by this branch.