forked from Ports/goldeneye-pc-port
fix(prng): correct MIPS64 dsll32/dsrl32 semantics in port/src/random.c (D284)
random.c's dsll32/dsrl32 helpers modelled the real MIPS64 dsll32/dsrl32 instructions as ordinary 32-bit shift-by-n operations, masked to 32 bits. Real MIPS64 dsll32/dsrl32 shift the FULL 64-bit register by n+32 with no masking - they exist specifically to encode a 32-63 shift amount in a 5-bit immediate field. The bug was bit-exact-looking but wrong: it produced a deterministic, self-consistent stream that was never the N64's actual stream, from the very first draw. Surfaced by M-138's security/decomp-fidelity review (a forked review agent's numeric trace + a hand trace of random.s both found the same divergence); fix decided over disclose-as-known-issue per M-139b. Fix: dsll32(x,n) = x << (n+32), dsrl32(x,n) = x >> (n+32), no masking. No other line changes - every call site (randomGetNext, randomGetNextFrom, chrObjRandomGetNext) already mirrors random.s/ chrObjRandom.s line-by-line through these two helpers plus plain <</>> (which already matched dsll/dsrl correctly). Verified against a standalone C reproduction outside the game (first five draws from random.s's real initial seed diverge completely between the buggy and corrected helpers, confirming the fix actually changes the output stream). 242/242 clean build; full 21-level verify.sh sweep PASS, no crashes. This silently changes every RNG-derived sequence (loot, AI variance, anything seeded from randomGetNext/randomGetNextFrom) from the first draw in every build going forward. Savegames are unaffected (seed isn't save state); existing recorded replays will diverge against a post-fix build. Called out in v0.3.0 release notes per M-139c. Claude-Session: https://claude.ai/code/session_01KwZViAFaXua3PKgHHjNgvi
This commit is contained in:
1 parent
6807555c17
commit
0de99b4db4
3 files changed
+48
-9
No files matched your search
@@ -161,3 +161,4 @@ D264,FIXED,"D262's root cause (ABI/layout class, cf.","FIXED — build green, pr
|
||||
D263,RESOLVED,v0.2.1 (user report):,"RESOLVED (not a bug), but refined (v0.2.1 further user testing, 2026-09-15) —"
|
||||
D265,?,"D236 investigation (port-layer, port/fast3d/gfx_pc.cpp):",PARKED — reverted to baseline; evidence + suspect list above.
|
||||
D258,OPEN,PAL/JP asset conversion is broken at the source-data level —,"OPEN — repair path verified, not applied (out of v0.2.0 scope)."
|
||||
D284,FIXED,"port/src/random.c's MIPS64 dsll32/dsrl32 shift helpers were backwards (modelled as 32-bit shifts, not ""shift by n+32 on the full 64-bit register""), desyncing t…","FIXED, 21-level sweep clean, no crashes —"
|
||||
|
@@ -557,6 +557,7 @@ covers D24–D69; the log continues in §H (D32 procedure, D70–D121).
|
||||
| D263 | **v0.2.1 (user report): Facility — "ouromov not shooting Trevelyan"; the scripted opening beat never triggers. RESOLVED: not a port bug — the beat is proximity/LOS-triggered exactly as on N64, and the full chain (incl. the execution shot) was verified firing end-to-end on PC.** The triage suspects (tick pacing, camera-mode gate, silent anim lookup) were all ruled out at runtime. **The beat, fully mapped** (static decode of `UsetuparkZ.c` — remember every DBYTE/TBYTE in the dump is bswap16/32'd relative to game interpretation): Trevelyan = guard chr 67 (body 9 = Trevelyan_006, pad 131) on ai_18: walks to pad 124, wanders, then loops a sees-Bond check; **only when Bond is in his vision** (range <10k, facing ±110°, fog, random gate ≈1/49/tick, LOS + *same stand tile* via `chrCanSeeBond`) does he jump to ai_19: runs at Bond, alarm + dialogue timers, sets an objective bit; BG list ai_41 (ID 0x1000, auto-started) polls that bit and spawns **Ourumov (body 7) at pad 88 with ai_23** plus a 6-soldier squad; ai_23 renumbers him to chrnum 78 and arms him → ai_22: runs to pad 123, waits for Trevelyan within 0x1400 of it, ~95 s dialogue cascade, then the **execution shot** (`HitChrWithItem` from 78 on 67). It is a multi-minute sequence that *starts* only when the player walks Bond west and down a floor into Trevelyan's room — Bond spawns 8900 units away on the upper floor with LOS blocked (probed directly: `stanTestLineUnobstructed` = obstructed, different stan tiles), identical geometry to N64. **Runtime verification** (`-level_34`, `#ifdef PORT` probes in the AI executor/vision path, all since removed): all 15 BG lists + guards start; Trevelyan alive on ai_18; every vision gate passes except the geometric ones at spawn distance; player movement confirmed working (note for future headless tests: under the port's default SOLITARE/natural-pitch scheme, walking is C-button/D-pad fed — W→C-up — and the analog stick carries pitch, so `GE_INPUTSCRIPT` SUP/SDOWN tokens do *not* walk Bond there; use UP/DOWN pulses). **Decisive test:** a temporary PORT-only teleport probe dropped Bond 65 units from Trevelyan at t≈12 s → seconds later: vision fires (chr 67 → ai_19), objective bit set, `SpawnChr body=7 pad=88` + squad, chrnum→78 on ai_23; ~2½ min later **`HitChrWithItem from_chr=78 target=67 part=8 item=6`** — Ourumov executes Trevelyan. Most likely explanation of the report: the tester never walked Bond into Trevelyan's sightline (or pre-D253 crash masked it). No game-code or port-code change required. **Kept from this session (test-harness only):** `port/src/input.c` — `GE_INPUTSCRIPT` analog-stick tokens were applied *after* the stick was already written out, so SUP/SDOWN/SLEFT/SRIGHT were silently discarded; the script override now runs before the clamp/write. | RESOLVED (not a bug), but refined (v0.2.1 further user testing, 2026-09-15) — beat verified firing end-to-end on PC incl. execution shot; **user playtest confirmed the sequence plays normally.** **Refinement: the exact trigger behavior is significantly sensitive to Bond's precise location and does not fire 1:1 identically to the N64 original across different approach paths/positions** — consistent with the beat's own documented sensitivity (range/facing/LOS/same-stand-tile + a ≈1/49-per-tick random gate, see the mapped mechanism above), so this is likely the same finicky-trigger behavior rather than a new bug, but not confirmed byte-identical to N64 for every approach. **User confirms this does NOT block level progression** either way — logged for awareness, not tracked as an open defect. Probes removed; input-script stick fix kept. |
|
||||
| D265 | **D236 investigation (port-layer, `port/fast3d/gfx_pc.cpp`): the tree cards are IA8/IA4, not IA16 — and `import_texture_ia4` decoded the RDP's I2:A2 texel as I3:A1, inverting the alpha ramp. Fixed; awaiting user visual re-test.** The earlier sessions misidentified the format: a live tri-time tile census (probe in `gfx_sp_tri1`, since removed) shows level_36's tree-card classes are (a) **IA8, line=64B → 64×16** (texdump r107), drawn OPA/bilerp/1-cycle/AC_NONE — *genuinely fully opaque* (A nibble F on 1023/1024 texels; opaque on N64 too) — and (b) **IA4, line=64B → 128×16** (texdump r117), drawn ZMODE_XLU/point/2-cycle with m2a=CLR_MEM/m2b=1MA, i.e. texel-alpha blended. There are no 32-wide IA tiles at tri time; the prior "32×32 tile" reading was wrong, and the whole earlier IA16 byte-pair/I11:A5 debate (r107) was analyzing an IA8 image as byte-pairs. **The wall's root cause:** r117 decoded as I2:A2 is a strip of ~13 discrete conifer silhouettes (trunk+canopy shapes visible in the ASCII render, `scratch/r117_ia4_alpha.png`) with a 3-level alpha ramp {0: 95% of texels, 85, 170} — faint dithered distant trees on N64. But `import_texture_ia4` read each nibble as intensity=`part>>1` (3-bit, SCALE_3_8) + alpha=`part&1`: A2=1 canopy pixels became **fully opaque**, A2=2 edge dithering became **invisible** → opaque repeating tree silhouettes = the "wall". The bug is inherited verbatim from the PD port (`pd_port/port/fast3d/gfx_pc.cpp` has the identical I3:A1 read — latent there, fatal for GE's tree strip). **Fix (applied):** `intensity = part>>2`, `alpha = part&3`, both via new `SCALE_2_8`; this is the hardware-correct RDP IA4 layout. Also cleaned up: the ia16 importer comment now states plain I8:A8 (the byte-pair read *is* correct IA16; the old "r107 proof" was misidentified data), and the earlier part-1 dimension fix stays (harmless/correct for real IA16 tiles). **Caveats/owed:** (1) user visual re-test of Surface 1 trees — headless A/B was in flight when work halted; a cross-run region diff was inconclusive (camera drift between captures); (2) the decode change affects *every* IA4 tile game-wide — regression-sweep other levels for dithered/alpha IA4 art that now renders with correct 3-level alpha; (3) if a hard repeating band remains after the fix, it is the opaque IA8 strip (a), which matches hardware and D236 needs re-scoping. **OUTCOME (user re-test): NEGATIVE — trees still render as the wall with the I2:A2 decode applied; the change was fully reverted (`git checkout` of `port/fast3d/gfx_pc.cpp`, back to the original PD-derived importers) and work is parked.** The census/RM/texture data above stands as evidence for the next attempt: the decode was *necessary* (I2:A2 is the hardware layout and r117 is unambiguously an authored tree strip under it) but *not sufficient* — remaining suspects, in order: (i) fast3d's blending of these XLU/2-cycle cards (texel alpha reaching the GL blend at all — the `texture_edge`/`SHADER_OPT_TEXTURE_EDGE` path forces `a>0.19 ? 1 : discard`, which for a 3-level ramp quantizes to opaque/discard, i.e. still hard-edged), (ii) per-card UV windowing/placement (all captured cards sampled the same u:0..64/v:0..16 window — confirm against N64), (iii) fog/distance. Note for the next session: verify what the N64 actually shows in that region before more port-side changes (the opaque IA8 strip is faithful to hardware either way). | PARKED — reverted to baseline; evidence + suspect list above. |
|
||||
| D258 | **PAL/JP asset conversion is broken at the source-data level — `scripts/filelist.e.csv` and `filelist.j.csv` carry naming defects, so `gen_romassets.py e/j` (and the d43/d69/d88 emitters' `find_row`) cannot resolve PAL/JP builds. v0.2.0's drop-in ROM feature therefore ships NTSC-U only.** Discovered while scoping issue #6 (drop-in ROM). **PAL:** 31 of the 512 model rows in `filelist.e.csv` have truncated/renamed basenames (an extraction artifact — e.g. `Pbridge_console` ×6 where the resource table says `Pbridge_console1aZ…3bZ`, `Pchrgrenadelauncher` for `PchrgrenadelaunchZ`). **JP:** one font row is mislabeled — `fontBankGothic_fontchartable.bin` @3044276 is named `fontZurichBold_fontchartable.bin` (a duplicate label). Both were proven without a PAL/JP ROM: (a) greedy two-pointer sequence alignment of the 512-row `file_resource_table.inc.c` model order against each CSV's model rows gives an exact 1:1 match (481 by-name + 31 positional for PAL), offsets strictly non-decreasing, and zero size anomalies vs US (all within [0.5×, 2×]); (b) JP font row proven by the contiguity invariant — Gothic kerning @3043600+676 = 3044276 exactly hits the mislabeled row, whose size (8716) equals the US Gothic chartable, while Zurich's own chain is contiguous one row later. **Consequence:** no PAL or JP PC exe has ever been buildable from this tree (latent — only NTSC was ever exercised); the CSV `file` column is a label only (emitters look up by basename, never read the path), so the repair is a pure data fix: rename the 31 PAL model rows + 1 JP font row to the exact table symbols. **Scope decision (v0.2.0):** ship drop-in ROM support for NTSC-U only — `prepare-assets.py`/the frozen `ge007-convert` gate non-US ROMs with a clear message after SHA-1 region detection; the CSV repair + multi-region matrix + per-region exes are deferred to the Part B follow-up. | OPEN — repair path verified, not applied (out of v0.2.0 scope). |
|
||||
| D284 | **`port/src/random.c`'s MIPS64 `dsll32`/`dsrl32` shift helpers were backwards (modelled as 32-bit shifts, not "shift by n+32 on the full 64-bit register"), desyncing the PC PRNG stream from the N64's from the very first draw, in every release to date (M-138 security/fidelity review; fix decided M-139b).** Fix: `dsll32(x,n) = x << (n+32)`, `dsrl32(x,n) = x >> (n+32)`, no masking — matches real MIPS64 semantics and `src/random.s`/`src/game/chrObjRandom.s` line-by-line. Verified by hand trace + an independent standalone C reproduction (both diverge from the buggy stream from draw 1). Full body below. | FIXED, 21-level sweep clean, no crashes — RNG stream changes from every prior build; existing replays will diverge, savegames unaffected |
|
||||
|
||||
Phase 2 replaced the Phase-1 demo loop with the real `mainproc()` on real OS
|
||||
threads, compiled GE's real `src/sched.c`, and brought in PD's fast3d software
|
||||
@@ -10106,3 +10107,24 @@ Texture content (`GE_TEXDUMP`/`GE_TEXRAW` + offline decode of `texdump/r*.bin`):
|
||||
1. Single instrumented run: `-level_36` + `GE_D270PIX` (both weave points) + `GE_TEXRAW` + `GE_PCDUMP=1400-1600:1`; correlate per tick — does a fog=25 `0x0c184b50` tri cover the point AND is the pixel dark?
|
||||
2. Log `curRoom`/`roomsDrawn` (D271 infra) alongside `GE_D270PIX` to correlate `0x0c184b50` presence with room-streaming state.
|
||||
3. Decode `0x0c184b50`'s CI8 texture offline — needs a run in which it actually uploads (TEXRAW has not captured it yet).
|
||||
|
||||
## D284 — M-140: `port/src/random.c`'s MIPS64 `dsll32`/`dsrl32` shift helpers were backwards — modelled as 32-bit shifts instead of "shift by n+32 on the full 64-bit register" — desyncing the PRNG stream from N64 from the very first draw, in every release to date
|
||||
|
||||
**Origin: M-138's security/decomp-fidelity review** flagged `random.c` as documented bit-exact to the N64 but not actually verified as such; a forked review agent's numeric trace and a hand trace of `src/random.s` in that same session both found the same divergence. Held open pending the user's fix-vs-disclose call; **M-139b: fix it.**
|
||||
|
||||
**Root cause.** `dsll32 rd,rs,n` / `dsrl32 rd,rs,n` are real MIPS64 instructions that shift the **full 64-bit register** by `n+32` — they exist specifically to encode a 32–63 shift amount in a 5-bit immediate field (`dsll`/`dsrl` alone only cover 0–31). The original port modelled them instead as ordinary 32-bit shift-by-`n` operations, masked to 32 bits after: `(x << n) & 0xFFFFFFFF` / `(x >> n) & 0xFFFFFFFF`. Both the shift amount (missing the implicit `+32`) and the masking (real hardware does not truncate — the destination register keeps whatever the 64-bit shift produces) were wrong. This is a "bit-exact-looking but wrong" bug: it compiles, runs, and produces a deterministic, self-consistent stream — it is just not *the N64's* stream, from the very first call.
|
||||
|
||||
**Verified two ways.**
|
||||
1. **Hand trace of `src/random.s`** (`randomGetNext`, `randomGetNextFrom`, and the identical transform in `src/game/chrObjRandom.s` — three call sites, same bug, same fix) against the corrected semantics.
|
||||
2. **A standalone C reproduction** (compiled and run outside the game, this session) comparing the buggy helpers against the corrected ones from the same seed (`0xAB8D9F7781280783`, `random.s`'s initial value): first five draws diverge completely (buggy: `0x783006, 0x83006830, 0x6830068, ...`; corrected: `0x40ec37cf, 0x630aedd7, 0x1f58071e, ...`) — confirms the bug is real and the fix changes the actual output stream, not just the code path.
|
||||
|
||||
**Fix (`port/src/random.c`, mechanical, `#ifdef PORT` not needed — this file only exists on PC, the N64 build uses `src/random.s`/`src/game/chrObjRandom.s` directly and is untouched):**
|
||||
```c
|
||||
static inline u64 dsll32(u64 x, int n) { return x << (n + 32); }
|
||||
static inline u64 dsrl32(u64 x, int n) { return x >> (n + 32); }
|
||||
```
|
||||
(was `(x << n) & 0xFFFFFFFFULL` / `(x >> n) & 0xFFFFFFFFULL`). No other line in the file changes — every call site (`randomGetNext`, `randomSetSeed`, `randomGetNextFrom`, `chrObjRandomGetNext`, `chrObjRandomSetSeed`) already mirrors `random.s`/`chrObjRandom.s` line-by-line using these two helpers plus plain `<<`/`>>` (which already matched `dsll`/`dsrl` correctly — only the `32`-suffixed pair was wrong). One side effect worth flagging: `g_randomSeed`/`g_chrObjRandomSeed`/the `randomGetNextFrom` caller-owned seed now genuinely carry values beyond 32 bits between calls (matching the real `sd $a0, g_randomSeed` full-64-bit store) — callers only ever consumed the `u32`-truncated return value, so this is inert for them, but don't "simplify" the stored seed back to 32 bits.
|
||||
|
||||
**Verified:** 242/242 clean build; full 21-level `verify.sh sweep` PASS, no crashes (loot/AI RNG touches every level, so a sweep is the right breadth check here, not just one level). Not (and cannot be) verified: the corrected stream against a live N64/emulator capture of `random.s`'s real output — the hand trace + independent numeric reproduction is the practical ceiling of verification available in this repo without ROM-attached hardware/emulator tooling.
|
||||
|
||||
**User-facing note:** this silently changes every RNG-derived sequence (loot placement/rolls, AI behavioural variance, anything seeded from `randomGetNext`/`randomGetNextFrom`) from the first draw, in every build going forward — savegames remain compatible (the seed is not part of save state), but **existing replays recorded against the old stream will diverge** (replay state persists `g_randomSeed` itself, so a replay recorded pre-fix and played back post-fix, or vice versa, will not reproduce the same run). Called out in the v0.3.0 release notes per M-139c.
|
||||
+25
-9
@@ -10,10 +10,25 @@
|
||||
* Also ports src/game/chrObjRandom.s: the character-AI PRNG (chroid.c),
|
||||
* which runs the identical transform on its own seed variable.
|
||||
*
|
||||
* The assembly operates on the low 32 bits of the seed (the high 32 bits are
|
||||
* never read), but the seed is stored as a full u64. We mirror each
|
||||
* instruction with explicit masks to guarantee bit-exactness. The initial
|
||||
* seed is the two .words in random.s: 0xAB8D9F77 (hi) / 0x81280783 (lo).
|
||||
* The seed is stored as a full u64 and the transform genuinely operates on
|
||||
* all 64 bits of the register (see the dsll32/dsrl32 note below) — the
|
||||
* stored seed is NOT truncated to 32 bits between calls, matching what the
|
||||
* real `sd $a0, g_randomSeed` in random.s stores. The initial seed is the
|
||||
* two .words in random.s: 0xAB8D9F77 (hi) / 0x81280783 (lo). Only the low
|
||||
* 32 bits are ever returned to callers (`dsll32 $v0,$a0,0; dsra32
|
||||
* $v0,$v0,0` sign-extends the low half into $v0, of which callers only use
|
||||
* the u32 truncation).
|
||||
*
|
||||
* D284 (M-140): the original port of this file had dsll32/dsrl32 backwards
|
||||
* — it shifted by the literal immediate `n` and masked to 32 bits, i.e. it
|
||||
* modelled them as 32-bit shift instructions. Real MIPS64 dsll32/dsrl32 are
|
||||
* "shift by n+32", operating on the FULL 64-bit register with no masking:
|
||||
* they exist specifically to encode a 32..63 shift amount in a 5-bit field.
|
||||
* The bug was bit-exact-but-wrong: every generated value differed from the
|
||||
* real N64 stream from the very first draw, silently desyncing loot RNG, AI
|
||||
* variance, and replay determinism in every release to date. Verified two
|
||||
* independent ways (a forked review agent's numeric trace, and a hand trace
|
||||
* of random.s against this file, both in the M-138 review session).
|
||||
*/
|
||||
|
||||
#include <ultra64.h>
|
||||
@@ -23,12 +38,13 @@ u64 g_randomSeed = 0xAB8D9F7781280783ULL;
|
||||
|
||||
/*
|
||||
* MIPS64 shift helpers, matching the .s instructions exactly:
|
||||
* dsll32 rd,rs,n -> (rs << n) & 0xFFFFFFFF (low 32 bits kept, hi zeroed)
|
||||
* dsrl32 rd,rs,n -> (rs >> n) & 0xFFFFFFFF (low 32 bits kept, hi zeroed)
|
||||
* Plain `<<`/`>>` on u64 match dsll/dsrl (full 64-bit shifts).
|
||||
* dsll32 rd,rs,n -> (rs << (n+32)) (full 64-bit register, no masking)
|
||||
* dsrl32 rd,rs,n -> (rs >> (n+32)) (full 64-bit register, unsigned)
|
||||
* Plain `<<`/`>>` on u64 match dsll/dsrl (full 64-bit shifts, shift amount
|
||||
* used as-is, 0-31).
|
||||
*/
|
||||
static inline u64 dsll32(u64 x, int n) { return (x << n) & 0xFFFFFFFFULL; }
|
||||
static inline u64 dsrl32(u64 x, int n) { return (x >> n) & 0xFFFFFFFFULL; }
|
||||
static inline u64 dsll32(u64 x, int n) { return x << (n + 32); }
|
||||
static inline u64 dsrl32(u64 x, int n) { return x >> (n + 32); }
|
||||
|
||||
/*
|
||||
* Advance the global seed and return the new low 32 bits.
|
||||
|
||||
Reference in new issue
Block a user