mirror of
https://github.com/jkdansereau/goldeneye-pc-port.git
synced 2026-10-23 15:52:42 +02:00
D122: fix propDef obj half-swap for ObjectRecord-derived types
tools_pc/d88_propdefs.py had a per-type handler table plus a generic fallback. Six `inherits ObjectRecord` propDef types (47 TINTED_GLASS, 39 VEHICHLE, 40 AIRCRAFT, 45 TANK, then 13 AUTOGUN, 20 AMMO/MultiAmmoCrate) had no handler and fell through to the generic arm, which bswap32's N64 word 1. For an ObjectRecord that word is [s16 obj][s16 pad] and needs a half-swap, not a word-swap - so the model id `obj` landed in the wrong 16 bits, giving a garbage/OOB index into PitemZ_entries[] and a crash in modelLoad (loadobjectmodel.c:393) / modelInitRwData (model.c:6249). BUNKER1/Silo were unaffected because neither emits any of the six types. Fix (converter + matching walk stride, no game logic): - d88_propdefs.py: OBJ_TAIL_DESC handler - real 144B PC ObjectRecord prefix + typed tail (pointers widened 4->8B/8-aligned, [u16|u16] words half-swapped). PROPDEF_PC_BYTES 39/40 176/180->208, 45 224->248. - loadobjectmodel.c sizepropdef() #ifdef PORT arm: VEHICHLE/AIRCRAFT 44/45->52, TANK 56->62 words (N64 values in the #else sizeof switch). Verified: -level_33/34/35 clear the model-load chain (they now crash further in, in unrelated chr/fast3d code - separate blockers); -level_09 BUNKER1 and -level_20 Silo unregressed. Sidecars regenerated (d88_emit.py --regen, 21/21). Full write-up: PCPortResearch.md D122 (uncommitted, rides with the in-progress docs restructure).
This commit is contained in:
1 parent
c53e1ac698
commit
a8b174a829
4 files changed
+221
-6
No files matched your search
@@ -0,0 +1,165 @@
|
||||
# BRIEF D122 — per-level prop/item model-load crash (blocks the level sweep)
|
||||
|
||||
Repo: `REPO_ROOT` (git, branch `master`, Windows;
|
||||
PowerShell + Bash tool available).
|
||||
|
||||
## READ FIRST
|
||||
|
||||
- `docs/HANDOFF.md` (top) — current state, environment, repro line.
|
||||
- `docs/PORT-LEARNINGS.md` — recurring bug classes. **A / C are the
|
||||
prime suspects here** (pointer-width struct growth; BE rodata on LE).
|
||||
- `docs/PCPortResearch.md` §F index (top) + entries **D43, D53.2, D56,
|
||||
D57, D67, D68, D39, D108–D112, D120** — model/asset conversion +
|
||||
pointer-width history. Jump to each via the index; do NOT linear-read.
|
||||
- `AGENTS.md` non-negotiables (esp. #2 game logic unmodified — narrow
|
||||
`#ifdef PORT` ABI/layout/format edits only, each logged in §F).
|
||||
|
||||
## CONTEXT (established this session, M-12)
|
||||
|
||||
- **WS1 landed** (commit `02c12068`, D121): a bare
|
||||
`./build-pc/ge007.x86_64.exe -level_XX` now auto-injects that stage's
|
||||
`memallocstringtable[]` pool row. Every solo level boots with correct
|
||||
pools now — no manual `-m*` needed.
|
||||
- **D88.4 (propDefs converter) is effectively done for the 21 solo
|
||||
levels.** `tools_pc/d88_emit.py` + `d88_propdefs.py` are committed
|
||||
(`ff563812`, `4ccc0d3c`); `python tools_pc/d88_emit.py ntsc-final
|
||||
--regen` reports `21/21 converted … ALL CHECKS PASSED`. The placeholder
|
||||
`PROPDEF_PC_BYTES` for VEHICLE/AIRCRAFT/TANK/AMMO are **not exercised by
|
||||
any solo level** (`tools_pc/d88_propdef_scan.py` histogram — no
|
||||
VEHICLE/AIRCRAFT/TANK in any of the 21). So D88.4 is NOT the blocker.
|
||||
- **The actual blocker: prop/item model loading crashes on most levels.**
|
||||
Smoke test (each `-level_XX`, `GE_PCDUMP="60-240:60"`, ~25 s):
|
||||
|
||||
| level | # | result |
|
||||
|---|---|---|
|
||||
| Bunker1 | 09 | renders, playable (known good) |
|
||||
| Silo | 20 | renders ~91% frame content, no crash |
|
||||
| Dam | 33 | **CRASH** `modelInitRwData` (`src/game/model.c:6249`) — `node->Opcode` deref, bad `node` in the ModelNode walk |
|
||||
| Facility | 34 | **CRASH** `modelLoad` (`src/game/loadobjectmodel.c:393`) — `PitemZ_entries[modelid].header->RootNode`, bad `header` (or OOB `modelid`) |
|
||||
| Runway | 35 | **CRASH** same as Facility (`loadobjectmodel.c:393`) |
|
||||
|
||||
Crashes are **early and deterministic** (before frame 1 renders), same
|
||||
PC every run — distinct from BUNKER1's rare intermittent
|
||||
`loadobjectmodel.c:393` (D117 nondeterminism). `EXCEPTION 0xc0000005`,
|
||||
`FAULT ADDR: ffffffffffffffff`. `Rax` at the Facility crash holds ASCII
|
||||
bytes ("…o ng nol…") — looks like a model/texture name string landed in
|
||||
a register, consistent with a mis-strided table read.
|
||||
|
||||
## TASK
|
||||
|
||||
**One question: why do prop/item models load fine for BUNKER1/Silo but
|
||||
crash for Dam/Facility/Runway, and what is the narrow fix?**
|
||||
|
||||
Almost certainly one of:
|
||||
1. `PitemZ_entries[341]` (`struct ItemModelFileRecord { ModelFileHeader
|
||||
*header; char *filename; float scale; }`, `src/bondtypes.h:1466`) —
|
||||
table stride or the `header`/`filename` pointer values wrong on PC.
|
||||
Find where this array is **defined** (it is `extern` everywhere I
|
||||
looked — `src/game/prop.c:40`, `chrobjdata.h:20`; the definition is
|
||||
likely a generated `.s`/rodata blob or an asset — `grep` harder,
|
||||
check `port/src/romassets_*.s`, `scripts/gen_romassets.py`,
|
||||
`scripts/gen_*`). If the `header` pointers come from BE ROM rodata
|
||||
they need rebasing/byteswap (D39 Globalimagetable / D68 class).
|
||||
2. `modelid` passed to `modelLoad` is out of range / unswapped — it comes
|
||||
from a propDef `ObjectRecord`-family `modelid` field. If a record type
|
||||
used by Dam/Facility props (but not BUNKER1) has its `modelid` at a
|
||||
field offset the `d88_propdefs.py` converter byteswaps wrongly (or
|
||||
not at all), `modelid` is garbage. Cross-check the per-type field
|
||||
maps in `tools_pc/d88_propdefs.py` against the record structs.
|
||||
3. The `pcmodels` sidecar (`tools_pc/d43_emit.py`, loaded at
|
||||
`0x10C00000`, "512 sidecars") is missing/mis-converting a model
|
||||
feature (ModelNode opcode, `PointUsage[]` chain — cf. D120,
|
||||
`d43_emit.py` opcode-0x18 gap) that Dam/Facility prop models use.
|
||||
`model.c:6249` already has a `GE_D86` env probe for the node walk —
|
||||
use it.
|
||||
|
||||
Determine which, root-cause, and apply the **narrowest** fix (converter
|
||||
extension preferred over runtime fixup for a whole format — D43/D88
|
||||
pattern; or a narrow `#ifdef PORT` ABI edit with the N64 line kept under
|
||||
`#else`).
|
||||
|
||||
## FILES YOU MAY TOUCH
|
||||
|
||||
- `src/game/model.c`, `src/game/loadobjectmodel.c`, `src/game/prop.c`
|
||||
(narrow `#ifdef PORT` only).
|
||||
- `tools_pc/d43_emit.py` (+ `tools_pc/d43_*.py` helpers, `d43_layoutprobe.c`).
|
||||
- `tools_pc/d88_propdefs.py` / `tools_pc/d88_emit.py` (only if the cause
|
||||
is a propDef `modelid` field).
|
||||
- `port/src/` model/asset shims if the table is patched at runtime like
|
||||
`pcmodelsPatchTable` (`grep pcmodelsPatchTable`, `port/src/romdata*.c`).
|
||||
- New probe files: `tools_pc/d122_*.py` / env-gated `GE_D122` blocks.
|
||||
- `docs/PORT-LEARNINGS.md` (append any new generalisable quirk),
|
||||
`docs/PCPortResearch.md` §F (write up as **D122**, add to the index).
|
||||
|
||||
Nothing else is running — no file collisions to worry about.
|
||||
|
||||
## KNOWN-GOOD / RULED OUT — do not re-investigate
|
||||
|
||||
- WS1 per-level pool injection (D121) — works, committed.
|
||||
- D88.4 propDefs stream conversion for solo levels — works (21/21).
|
||||
- BUNKER1 + Silo prop/model loading — works.
|
||||
- The intermittent BUNKER1 `loadobjectmodel.c:393` crash — that is
|
||||
D117-class nondeterminism, separate; do not chase it, use the
|
||||
deterministic Dam/Facility/Runway repro.
|
||||
- `d88_propdef_scan.py` "STOP … unmapped type NOTHING" — that is the
|
||||
*scan tool's* limitation, not a converter bug (`d88_emit.py` completes).
|
||||
|
||||
## PRE-FLIGHT (done — results above)
|
||||
|
||||
- `./build-pc.sh ntsc-final` green; binary at `build-pc/ge007.x86_64.exe`.
|
||||
- Sidecars regenerated this session (`d43_emit.py`, `d69_emit.py`,
|
||||
`d88_emit.py --regen`) — all pass.
|
||||
- Smoke-test table above.
|
||||
|
||||
## ENVIRONMENT
|
||||
|
||||
```sh
|
||||
export PATH="/c/msys64/mingw64/bin:$PATH"
|
||||
./build-pc.sh ntsc-final # ~5 s, from repo root
|
||||
./build-pc/ge007.x86_64.exe -level_34 # Facility repro (bare — D121)
|
||||
```
|
||||
|
||||
- `timeout` does NOT reliably kill the game on Windows. Launch with
|
||||
`… &`, `sleep N`, then `kill $PID; pkill -f ge007`.
|
||||
- Crash log: `ge007.crash.log` (repo root). Symbolicate:
|
||||
`addr2line -e build-pc/ge007.x86_64.exe -f -C 0x<PC>` (image base
|
||||
`0x140000000`; PC in the log is already absolute).
|
||||
- Frame capture: `GE_PCDUMP="a-b:step"` → `./ppm/`; content check
|
||||
`python tools_pc/pixcount.py ppm/frame_XXXX.ppm`.
|
||||
- gdb: **attach** mode is fast (`gdb -batch -x cmds.txt -p <winpid>`,
|
||||
winpid = 4th col of `ps -p <bashpid>`; game must already be running).
|
||||
Launch mode is too slow. A hardware watchpoint on `PitemZ_entries` or
|
||||
the bad `node` catches the corrupting write fast.
|
||||
- Standalone probe compiles: `-std=c11`.
|
||||
|
||||
## BUDGET
|
||||
|
||||
Structural bug → generous: **~12 build→run→inspect cycles or ~90 min**.
|
||||
On expiry: revert all temp probes (leave only committed env-gated ones),
|
||||
write up the spec progress + best hypothesis in `docs/PCPortResearch.md`
|
||||
§F as **D122** with an explicit confidence rating, and list exactly which
|
||||
files you changed. A good half-solved write-up is a deliverable.
|
||||
|
||||
## CONSTRAINTS
|
||||
|
||||
- No game-logic / control-flow changes. ABI/layout/format only, each
|
||||
`#ifdef PORT` with the N64 line kept verbatim under `#else`, each
|
||||
documented in §F/D122.
|
||||
- Prefer an offline sidecar-converter extension over a runtime fixup for
|
||||
a whole asset format.
|
||||
- Revert your own temp probes before reporting.
|
||||
- Regenerate sidecars after any converter change:
|
||||
`python tools_pc/d43_emit.py ntsc-final && python tools_pc/d69_emit.py
|
||||
ntsc-final && python tools_pc/d88_emit.py ntsc-final --regen`.
|
||||
|
||||
## REPORT
|
||||
|
||||
1. Root cause — `file:line` evidence, which of the 3 hypotheses (or
|
||||
other), why Dam/Facility/Runway differ from BUNKER1/Silo.
|
||||
2. Fix — the diff, or why-not if unresolved.
|
||||
3. Verification — `-level_33`, `-level_34`, `-level_35` each boot to ≥1
|
||||
non-degenerate frame (`pixcount.py` > a few %), AND `-level_09`
|
||||
(BUNKER1) still renders (no regression — `tools_pc/framediff.py`).
|
||||
4. Probes left in the tree (env-gated, capped).
|
||||
5. Confidence rating.
|
||||
6. Append any generalisable quirk to `docs/PORT-LEARNINGS.md`.
|
||||
@@ -53,6 +53,16 @@ through a converter or a runtime bswap fixup reads scrambled.
|
||||
(StandTile id/room, header mid/tail).
|
||||
- **Rule:** prefer an offline sidecar converter (D43, D69, D88 pattern,
|
||||
`tools_pc/d*_emit.py`) over a runtime fixup for a whole format.
|
||||
- A polymorphic-record converter with a per-type handler table + a
|
||||
"generic" fallback silently corrupts any type the table forgot: the
|
||||
fallback's word-granular bswap is wrong for sub-word fields. D122 —
|
||||
`d88_propdefs.py` had no arm for 6 `inherits ObjectRecord` propDef types
|
||||
(TINTED_GLASS/VEHICHLE/AIRCRAFT/TANK/AUTOGUN/AMMO); the generic arm
|
||||
bswap32'd the `[s16 obj][s16 pad]` word as one u32, putting the model id
|
||||
in the wrong half → OOB table deref. When you add a converter, enumerate
|
||||
*every* `type` byte a level can emit and assert the handler set is
|
||||
total; `[u16|u16]` / `[s16|s16]` packed words need a half-swap
|
||||
(`_hh_word`), never a 32-bit swap.
|
||||
|
||||
## C2. Port-layer / SDL shims
|
||||
|
||||
|
||||
@@ -91,10 +91,10 @@ s32 sizepropdef(PropDefHeaderRecord *pdef)
|
||||
case PROPDEF_ARMOUR: return 38;
|
||||
case PROPDEF_COLLECTABLE: return 40;
|
||||
case PROPDEF_TINTED_GLASS: return 42;
|
||||
case PROPDEF_VEHICHLE: return 44;
|
||||
case PROPDEF_AIRCRAFT: return 45;
|
||||
case PROPDEF_VEHICHLE: return 52; /* D122: was 44 (N64) */
|
||||
case PROPDEF_AIRCRAFT: return 52; /* D122: was 45 (N64) */
|
||||
case PROPDEF_AMMO: return 50;
|
||||
case PROPDEF_TANK: return 56;
|
||||
case PROPDEF_TANK: return 62; /* D122: was 56 (N64) */
|
||||
case PROPDEF_AUTOGUN: return 62;
|
||||
case PROPDEF_CCTV: return 68;
|
||||
case PROPDEF_MONITOR: return 72;
|
||||
|
||||
@@ -83,6 +83,23 @@ OBJ_PTR_WORDS = (4, 5, 26, 27)
|
||||
OBJ_N64_WORDS = 32
|
||||
OBJ_PC_BYTES = 144
|
||||
|
||||
# D122: ObjectRecord-derived types whose tail (N64 words >= 32) the converter
|
||||
# must lay out with pointer widening. {type: (ptr_word_set, hh_word_set)}.
|
||||
# 47 TintedGlassRecord tail: TintDist,CullDist,calculatedopacity,portalnum,unk90
|
||||
# 39 VehichleRecord ailist*@w32, [u16 aioffset|s16 aireturnlist]@w33, path*@w41, Sound*@w43
|
||||
# 40 AircraftRecord ailist*@w32, [u16|s16]@w33, path*@w43, Sound*@w44
|
||||
# 45 TankRecord collision*@w32 (rest scalar; struct locs unconfirmed)
|
||||
OBJ_TAIL_DESC = {
|
||||
47: (frozenset(), frozenset()),
|
||||
39: (frozenset((32, 41, 43)), frozenset((33,))),
|
||||
40: (frozenset((32, 43, 44)), frozenset((33,))),
|
||||
45: (frozenset((32,)), frozenset()),
|
||||
# 13 AutogunRecord: unkC4*@w49 unkC8*@w50 beam*@w51 (is_active@w52 unkD4@w53)
|
||||
13: (frozenset((49, 50, 51)), frozenset()),
|
||||
# 20 MultiAmmoCrateRecord: slots[13] of [u16 modelnum][u16 quantity]
|
||||
20: (frozenset(), frozenset(range(32, 45))),
|
||||
}
|
||||
|
||||
# Per-type PC struct size (bytes) the converter emits and sizepropdef must
|
||||
# return / 4. For ObjectRecord-derived types this is 144 + tail growth.
|
||||
PROPDEF_PC_BYTES = {
|
||||
@@ -114,10 +131,10 @@ PROPDEF_PC_BYTES = {
|
||||
35: 16, # WATCH_MENU_OBJ_TEXT (4w)
|
||||
37: 48, # RenameObjectRecord
|
||||
38: 32, # LockDoorRecord
|
||||
39: 176, # VehichleRecord (guess: 44w + tail; refine if a level needs it)
|
||||
40: 180, # AircraftRecord
|
||||
39: 208, # VehichleRecord (D122: 144 prefix + widened tail)
|
||||
40: 208, # AircraftRecord (D122)
|
||||
44: 24, # SafeObjectRecord fragment (5w + next*), refine on use
|
||||
45: 224, # TankRecord
|
||||
45: 248, # TankRecord (D122: 144 prefix + 23-word scalar tail + collision*)
|
||||
46: 28, # CutsceneRecord
|
||||
47: 168, # TintedGlassRecord
|
||||
48: 4, # END
|
||||
@@ -275,6 +292,29 @@ def convert_record(src, so, type_byte):
|
||||
# word 6 = Data* -> zero @24 (already; 20-byte u16 block then 8-align pad@20)
|
||||
return bytes(out)
|
||||
|
||||
if type_byte in OBJ_TAIL_DESC: # D122: ObjectRecord prefix + typed tail
|
||||
# 47 TINTED_GLASS / 39 VEHICHLE / 40 AIRCRAFT / 45 TANK. These all
|
||||
# `inherits ObjectRecord` but had no handler -> fell through to the
|
||||
# generic arm, which bswap32'd N64 word 1 ([s16 obj][s16 pad]) as one
|
||||
# 32-bit value. That put `obj` in the wrong half -> garbage modelid ->
|
||||
# OOB PitemZ_entries[] deref crash (loadobjectmodel.c:393 /
|
||||
# model.c:6249). Emit the real 144B PC ObjectRecord prefix, then the
|
||||
# tail with pointer members widened 4->8B and 8-aligned.
|
||||
_emit_object_prefix(out, src, so)
|
||||
ptr_words, hh_words = OBJ_TAIL_DESC[type_byte]
|
||||
pc = OBJ_PC_BYTES
|
||||
for i in range(OBJ_N64_WORDS, n64w):
|
||||
w = src[so + 4 * i: so + 4 * i + 4]
|
||||
if i in ptr_words:
|
||||
pc = (pc + 7) & ~7
|
||||
pc += 8 # 8 zero bytes already present
|
||||
elif i in hh_words:
|
||||
out[pc:pc + 4] = _hh_word(w); pc += 4
|
||||
else:
|
||||
out[pc:pc + 4] = _bswap32(w); pc += 4
|
||||
assert pc <= pcb, (type_byte, pc, pcb)
|
||||
return bytes(out)
|
||||
|
||||
# ---- generic: header-only records (objectives, END, DOOR_SCALE, ...) ----
|
||||
# No serialized pointers. header word special, rest plain bswap32.
|
||||
out[0:4] = _hdr_word(src[so:so + 4])
|
||||
|
||||
Reference in new issue
Block a user