diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 193c3394..aa857731 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -13,9 +13,14 @@ rules there are non-negotiable. ## Scope check -- [ ] No changes under `src/` or `include/` — **or** the only changes are the - narrow `#ifdef PORT` ABI exception (CONTRIBUTING.md rule 2), and each is - documented in `docs/porting-notes.md` / `docs/dev/findings.md`. +- [ ] No changes under `src/` or `include/` — **or** the only changes are + the narrow `#ifdef PORT` ABI exception (CONTRIBUTING.md rule 2), + checked against `docs/porting-notes.md` §A1, and documented in + `docs/dev/findings.md` (§F/D3x). +- [ ] If this PR contains a genuine `src/game` behavior change (not an + ABI/layout fix): an explicit rule-2 sign-off was granted + (`docs/dev-process.md` §7) and is recorded in a + `RULE-2-SIGNOFF`-tagged `docs/dev/findings.md` entry linked here. - [ ] `Makefile`, `tools/`, `rsp/`, `ld/` untouched (N64 build). - [ ] If `CMakeLists.txt` `REGION_DEFS` changed, it still matches the N64 `Makefile` per-region macro set exactly. diff --git a/AGENTS.md b/AGENTS.md index 55622cfa..eaa2fedc 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -11,7 +11,7 @@ ## Non-negotiables 1. **N64 build untouched.** `Makefile`, `tools/`, `rsp/`, `ld/` belong to the N64 build. Never modify them for the PC port. -2. **Game logic is unmodified.** The decomp's control flow and behavior are ground truth for 1:1 fidelity — never change them. All N64 *hardware* dependencies are satisfied by the `port/` layer; if a game file seems to need a behavioral change, stop — the fix belongs in `port/`. **Narrow exception (ABI/layout only):** the 32→64-bit transition forces a small class of mechanical, semantics-preserving edits that cannot be isolated in `port/` — chiefly pointer-width reconciliation in ROM-serialized structs (a struct with a 32-bit-pointer field misaligns when read as 64-bit). These follow the PD ground-truth pattern (store the embedded address as `u32`, cast to a real pointer at the use site), change no logic or behavior, and are each documented in `docs/dev/findings.md` §F/D3x. No other game-code edits are permitted. +2. **Game logic is unmodified.** The decomp's control flow and behavior are ground truth for 1:1 fidelity — never change them. All N64 *hardware* dependencies are satisfied by the `port/` layer; if a game file seems to need a behavioral change, stop and check the exception below before assuming the fix belongs in `port/`. **Diagnosis is never restricted — only the fix.** Trace a bug as deep into `src/game` state as the evidence leads, and name the exact struct/field/logic responsible, before deciding which of three buckets it falls in (full framing: `docs/dev-process.md`). **Narrow exception (ABI/layout only):** the 32→64-bit pointer-width transition forces a small class of mechanical, semantics-preserving edits that cannot be isolated in `port/` — **any struct-layout or pointer-width-driven misread caused by 32→64-bit widening**, whether in a ROM-serialized record (a struct with a 32-bit-pointer field misaligns when read as 64-bit) or a **live runtime struct/union** (a raw-byte offset alias into a union arm whose true field shifted because an earlier member in the same union widened — the D209/D210/D255 pattern; see `docs/porting-notes.md` §A1 for the full catalogue and its diagnostic tells). These follow the PD ground-truth pattern (store the embedded address as `u32` and cast to a real pointer at the use site; or read the correctly-named/typed field instead of a raw-offset/mistyped alias), change no logic or behavior, and are each documented in `docs/dev/findings.md` §F/D3x, cross-tagged to §A1 where that pattern applies. **A genuine behavioral difference** — the decomp's byte-identical code, given verified-correct inputs, still diverging from real N64 behavior — is not covered by this exception and requires the rule-2 sign-off procedure (`docs/dev-process.md`) before any `src/game` edit. No other game-code edits are permitted. 3. **Region macros mirror the Makefile.** `CMakeLists.txt` `REGION_DEFS` must match the N64 Makefile's per-region macro set exactly (finding A1). Divergence = silent branch divergence + link failures. 4. **`src/libultrare/Makefile.libultrare` is ground truth** for original-vs-Rare libultra files (finding B3). The PC build compiles: `libultra/audio`, `libultrare/audio` (drvrNew/env/reverb), `libultra/gu`, and `libultrare/io/vitbl.c` only. All other `io/` + `os/` files are excluded and shimmed in `port/src/libultra.c`. 5. **`rsp/graphics/gmain.s` is the RSP ground truth** — the authoritative reference for which GBI commands GE emits (modified fast3d, 1545 lines). We do not run it on PC; `port/fast3d/` replaces it. Use it to validate the software RSP's command decoding and the custom CC/RM modes. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index a15b7689..5237893d 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -11,12 +11,19 @@ keep the port faithful to the original game. belong to the N64 build and are never touched for PC work. If PC code seems to need a behavioural change in the game, the fix belongs in `port/`. -2. **Narrow ABI exception.** The 32→64-bit transition forces a small class of - mechanical, semantics-preserving edits to ROM-serialized structs (storing an - embedded 32-bit pointer as `u32` and casting at the use site). These are - allowed, must be guarded with `#ifdef PORT`, must change no behaviour, and - must be documented. See `docs/porting-notes.md` for the catalogue of - patterns. +2. **Narrow ABI exception; diagnosis is always allowed.** Trace a bug as + deep into the decomp's state as the evidence leads — that's never + restricted. Only the *fix* is constrained: the 32→64-bit transition + forces a small class of mechanical, semantics-preserving edits — any + struct-layout or pointer-width-driven misread caused by 32→64-bit + widening, in a ROM-serialized record or a live runtime struct/union alike + (see `docs/porting-notes.md` §A1 for the pattern and its tells). These + are allowed, must be guarded with `#ifdef PORT`, must change no + behaviour, and must be documented. A genuine behavioral difference from + real N64 behavior is a separate, much rarer case that needs explicit + maintainer sign-off first — see `docs/dev-process.md`'s rule-2 sign-off + procedure; it is not something a contributor can self-approve or infer + from this exception. 3. **Region macros mirror the Makefile.** `CMakeLists.txt` `REGION_DEFS` must match the N64 `Makefile`'s per-region macro set exactly. diff --git a/docs/dev-process.md b/docs/dev-process.md index f1147b16..d1096190 100644 --- a/docs/dev-process.md +++ b/docs/dev-process.md @@ -22,6 +22,45 @@ faster than its predecessor. That project bit-matches compiler output; this one matches runtime behavior, so the mechanics differ; but the project-structure lessons carry over directly. +### Diagnosis is unrestricted; only the fix is constrained + +AGENTS.md non-negotiable #2 restricts *changing* `src/game` behavior. It +has never restricted *investigating* it. An agent that stops short of +naming the exact `src/game` struct, field, or logic responsible for a bug +— because touching that file "feels" off-limits — is doing worse +diagnosis than the rules require, not better compliance with them. +Several of this port's hardest, highest-impact bugs (D209, D210, D255) +were exactly this shape: a `src/game` state variable reading wrong, +root-caused only by tracing fully into the game file that owned it. + +Every investigation should end able to say which of three buckets +applies: + +- **(a) ABI/layout-class.** The true cause is a struct-layout or + pointer-width misread introduced by the 32→64-bit transition — in a + ROM-serialized record or a live runtime struct/union alike. Fixable + under the narrow exception, entirely inside the game file under + `#ifdef PORT`, no sign-off needed. Check `docs/porting-notes.md` §A1 + first (the pattern's tells + worked examples) — this is the most + common shape a "`src/game` behavior looks wrong" report turns out to + have. +- **(b) Port-layer bug that merely manifests through game state.** The + game code is reading state that's correct for what N64 would have + produced; something in `port/` (a shim, fast3d, frame timing, an + uninitialized or mis-ordered port-owned value) is what actually + differs. Fixable entirely in `port/`, no `src/game` touch, no sign-off + needed. +- **(c) Genuine game-logic behavior difference.** The decomp's + byte-identical control flow, given verified-correct inputs, provably + diverges from real N64 behavior. Rare, and requires the rule-2 + sign-off procedure (§7 below) before any `src/game` edit. + +Bucket (c) is rare by design — most bugs that look like (c) turn out to +be (a) or (b) in disguise — but ruling it in or out requires actually +tracing the state to its source, not stopping at the file boundary. +"This state lives in `src/game`" is a reason to keep tracing, not a +reason to stop. + ## 1. Every investigation task gets a visible budget An open-ended tool; for them the permuter, for us "rebuild and run the game @@ -147,6 +186,68 @@ justifies. Rules: is still true as a fresh, small PR against current `main`. Merging stale content back over newer information is a regression, not a save. +## 7. The rule-2 sign-off procedure (genuine `src/game` behavior changes) + +Use this only for bucket (c) from the diagnosis framing above — never for +the ABI/layout exception (§A1-class fixes need no sign-off, only +documentation) and never for anything expressible in `port/`. + +**To request sign-off, the write-up must state:** + +1. The exact `src/game` file:line and the specific behavior being + proposed for change. +2. **Proof of divergence.** Given verified-correct inputs — i.e. every + upstream ABI/layout suspect (§A1) and every port-layer suspect has + been checked and ruled out, with the ruling-out cited — the + byte-identical decomp code still produces behavior that differs from + real N64 reference behavior. Name the reference (a capture, documented + prior N64 behavior, a cited external source). "It looks wrong" is not + proof; "it disagrees with ``, verified by ``" is. +3. Why the fix cannot be expressed in `port/`, and why it does not + qualify under the ABI/layout exception — the two questions that must + be answered "no" before this procedure applies at all. +4. The proposed fix, scoped as tightly as possible to the one named + behavior, plus a same-engine precedent if one exists — the Perfect + Dark port is the standing reference; cite the analogous PD change the + way `docs/dev/WIDESCREEN-FOV-PLAN.md`'s "Rule #2 exception framing" + section did. + +**Granting.** The user reviews the above and replies with an explicit +go-ahead in-thread. There is no implicit or inferred sign-off: silence, +"seems reasonable," a prior similar grant, or approval of a *different* +behavior in the same file do not carry over to a new request. + +**Recording.** Every sign-off gets its own `docs/dev/findings.md` entry +(next `Dxx` label), tagged **`RULE-2-SIGNOFF`** in its status line +alongside the normal finding classification, and recording: the request +as stated above, the grant (quoted or summarized, with date), and the +resulting diff. Add the label to `findings-index.csv` like any other row, +so `tools_pc/gen_findings_index.py` keeps it discoverable — chat history +is not the record of approval; the finding entry is. + +**Constraints that still apply after approval:** + +- Still `#ifdef PORT` — the N64 build is untouched, and the `#else` arm + (or the file's un-PORT-guarded default) still matches the original + decomp exactly. +- Still scoped to the one named behavior — a grant is not a standing + license; a different behavior difference in the same file needs its own + request, even if related. +- Still fully documented in the `RULE-2-SIGNOFF` finding entry — that + entry is the permanent record of *why* this specific exception to + non-negotiable #2 exists. +- If the fix would visibly change behavior at *default* + settings/difficulty/region beyond the one bug being fixed, treat that + as a signal the scope drifted mid-implementation — stop and go back for + a fresh sign-off on the wider scope rather than shipping it under the + narrower original grant. + +This generalizes the ad hoc precedent in +`docs/dev/WIDESCREEN-FOV-PLAN.md`'s "Rule #2 exception framing" section (a +real, already-granted case, kept as historical record) into a reusable +process; new requests cite this section rather than re-deriving the case +for a process each time. + ## Investigation-brief template ``` @@ -156,7 +257,16 @@ FILES YOU MAY TOUCH: . KNOWN-GOOD / RULED OUT: . PRE-FLIGHT ATTACHED: . BUDGET: run cycles / ~M min>. On expiry: revert probes, write up with confidence. -CONSTRAINTS: no game-logic changes; ABI/layout/format only; #ifdef PORT; documented in the finding log. +CONSTRAINTS: diagnosis may trace as deep into src/game state as the evidence + needs — name the exact struct/field/logic responsible before concluding + which bucket applies (see "Diagnosis is unrestricted" above). The FIX is + still constrained: no src/game behavior changes unless (a) it's an + ABI/layout-class struct-layout or pointer-width misread — check + docs/porting-notes.md §A1 first; #ifdef PORT; documented in the finding + log under §F/D3x — or (b) it's fixable entirely in port/ (no src/game + touch). A genuine game-logic behavior difference (bucket c) is NOT + authorized by this brief — stop and write up a rule-2 sign-off request + per docs/dev-process.md §7 instead of applying it. VERIFY: . REPORT: (a) root cause + file:line evidence (b) fix diff, or why not (c) probes left in tree (d) confidence. diff --git a/docs/dev/WIDESCREEN-FOV-PLAN.md b/docs/dev/WIDESCREEN-FOV-PLAN.md index 8577d50d..41469ad0 100644 --- a/docs/dev/WIDESCREEN-FOV-PLAN.md +++ b/docs/dev/WIDESCREEN-FOV-PLAN.md @@ -203,6 +203,10 @@ byte-identical to today. ### Rule #2 exception framing (needs explicit sign-off to start Phase 2) +(This project's now-generalized rule-2 sign-off procedure lives in +`docs/dev-process.md` §7; this section is the original case that +motivated it, kept for its PD-precedent argument.) + This is a behavior-affecting game-code edit class, outside the ABI-only D3x exception. The case for granting it: diff --git a/docs/dev/findings.md b/docs/dev/findings.md index a07eeaa7..13668fd8 100644 --- a/docs/dev/findings.md +++ b/docs/dev/findings.md @@ -10664,6 +10664,60 @@ crash class C2 (Facility `-level_34`, Runway `-level_35`): `import_texture_i8` A **Status (from §F table):** OPEN — ninth pass closed with no fix; see D276/D280 +**Next pass framing (post rule-2-clarification):** + +``` +TASK: D236 pass 10 — determine why the 0x0c184b50 discrete tree-card class +instantiates inconsistently (~4 tris on deterministic -level_36 boot vs +~720 in a free run) despite being geometrically in front of the confirmed +N64-faithful noise-quad "wall" (D280). +READ FIRST: docs/porting-notes.md §A1 (pattern + tells, now including D255); +docs/dev-process.md's "Diagnosis is unrestricted" section and §7 sign-off +procedure; docs/dev/findings.md D236 full entry (D265, D271, D272, +D273/D274, D276, D280) plus D209/D210/D255 as worked A1 examples. +FILES YOU MAY TOUCH: none pre-committed — this pass is diagnosis-first. +Touching src/game requires first clearing buckets (a) and (b) below, with +evidence, per CONSTRAINTS. +KNOWN-GOOD / RULED OUT (do not re-investigate): IA4/IA8 decode, blend +pipeline, fog combine (verified N64-faithful, ±2 truncation over 2505 +verts), portal-culling over-draw (separately fixed, not the cause), +converter/UV-conversion, CI palette bit-layout, mip presence, "identical UV +window" for the CI8 classes. +PRE-FLIGHT ATTACHED: D280's z/w depth-race data (0x0c184b50 confirmed in +front of the noise quad); the instantiation tri-count delta (4 vs 720) +between deterministic and free boot. +BUDGET: <=6 build->run cycles / ~90 min. On expiry: revert probes, write up +with confidence, no forced fix. +CONSTRAINTS: Trace the instantiation-count/gating decision into src/game +(prop-spawn, room-streaming, or distance-cull logic — likely bg.c) as far +as the evidence leads; name the exact struct/field/counter responsible +before concluding anything. Specifically, before writing this up as bucket +(c) (a genuine timing/streaming behavior difference worth a rule-2 sign-off +request): + - Check bucket (a) first: is the count/visibility-gating field read at a + struct offset that shifted under 32->64-bit pointer widening? (§A1 + tells: deterministic-but-wrong output; a raw/literal offset or narrow + field read into a struct/union with any pointer member anywhere in its + layout.) + - Check bucket (b) next: is a port-layer timing/order-of-operations + difference (frame pacing, load order, an uninitialized or + differently-timed port-owned counter) feeding src/game a different + input than N64 would have produced upstream of the gating logic? + - Only once both are actively ruled out with cited evidence (not + assumption) does this qualify for a rule-2 sign-off write-up under + docs/dev-process.md §7. Do not request sign-off preemptively or as a + fallback for "didn't find an ABI/port explanation in the time budget." +VERIFY: the deterministic -level_36 boot repro + tri-count probe already +used in D280; attach before/after instantiation counts for any change. +REPORT: (a) the exact struct/field/counter gating instantiation, file:line, + and which bucket (a/b/c) it falls in, with the evidence for that + classification (b) fix diff if (a) or (b), or a full rule-2 + sign-off request per §7 if (c), never a partial/ambiguous one + (c) probes left in tree (d) confidence. + Append any generalisable quirk to docs/porting-notes.md — extend + §A1 if this turns out to be another instance of the pattern. +``` + ## D237 — QoL ask (M-106, user QA): F10 options overlay — make it scrollable with the mouse wheel, expose more settings into it (e.g. screen-flash / no-hit-flash… **QoL ask (M-106, user QA): F10 options overlay — make it scrollable with the mouse wheel, expose more settings into it (e.g. screen-flash / no-hit-flash `Game.NoHitFlash`, and other easily-exposed config keys), and organise rows by category (mouse settings together, video together, etc.).** The overlay (`port/src/optionsoverlay.c`) already renders over the registered `config.c` option list; what's missing is wheel-scroll nav, a broader row set, and grouping headers. Cross-ref `OPTIONS-MENU-PLAN.md` §3 (v1 minimum set) + QOL-INVENTORY "PD has, GE lacks". diff --git a/docs/porting-notes.md b/docs/porting-notes.md index fed645f4..7d4d7577 100644 --- a/docs/porting-notes.md +++ b/docs/porting-notes.md @@ -55,8 +55,9 @@ state. `.act_*.attack_item`; aliased `WeaponObjRecord.weaponnum` at 0x80 on N64 via act-union@0x2C+84; act union moves to ~0x38 on PC). -**A1. Raw-byte aliases into a union (`act_ubytes.padding[N]`); the silent -variant.** The nastiest members of this class do not crash: a literal byte +**A1. Raw-byte or mistyped aliases into a union/struct whose true field +shifted because a sibling member widened; the silent variant.** The nastiest +members of this class do not crash: a literal byte index into a union that overlays pointer-bearing structs still *reads*, it just reads the wrong byte, so the game runs and merely behaves wrong. Grep for hardcoded indices into any `padding[]`/`u8[]` union arm. @@ -86,13 +87,60 @@ for hardcoded indices into any `padding[]`/`u8[]` union arm. named writes of the *same* arm. The rest of the `padding[N]` sweep (`chrlvTickAnim`/`chrlvTickDead` → `act_anim`/`act_dead`, both pointer-free) is layout-stable. +- **D255** (a mistyped-field variant, not a raw-offset variant — same + mechanism, different syntax): `vtxstore_fix_refs` read a record's + `Model*` through `((Model*)((ChrRecord*)prop->chr)->chrflags)` — + `ChrRecord.chrflags` is typed `CHRFLAG`, a **4-byte enum**. On 32-bit + N64 the cast is bit-exact; on 64-bit PC it reads only the pointer's + low 32 bits and zero-extends, discarding the real high bits. The + truncated result is `(nil)` whenever the real pointer's low 32 bits + happen to be zero — which read exactly like a legitimate NULL-model + case, and cost two prior investigation sessions chasing a + "destroy-path race" theory before the field itself was the suspect. + **Tell that generalizes past D209/D210's `padding[N]` shape:** it + doesn't have to be a raw numeric offset — a field that is *correctly + named* but *undersized/mistyped relative to a pointer it's secretly + meant to hold* (an enum, a `u32`, a `s32` doing double duty as a + pointer slot) is the same bug through a different syntax. Fix: read + through the correctly-typed sibling field (`ObjectRecord.model`) + instead, `#ifdef PORT`, N64 line/cast kept verbatim under `#else`. + §F **D255** (M-140). -**Lesson.** When a value reads as a clean constant (0, 1) rather than -garbage, a pointer byte is a prime suspect; the high bytes of a low-4GB -heap pointer are all zero, so a misaligned read looks like a legitimate -"feature off" value. Prefer the named field under `#ifdef PORT`; the alias -is only correct at 32-bit pointer width. Also: verify that every caller -reaches the site with the union arm you are naming actually active. +**Lesson.** When a value reads as a clean constant (0, 1, NULL) rather +than garbage, a pointer byte is a prime suspect; the high bytes of a +low-4GB heap pointer are all zero, so a misaligned or truncated read +looks like a legitimate "feature off" / "not set" value. **Tell-tale +signs of this pattern, any one of which should trigger an A1-first +check before assuming a bug is genuine game-logic behavior:** +- The observed value is **byte-exact-looking but wrong from the very + first call/frame** — not intermittent corruption, not "usually right + but occasionally wrong." +- A **raw/literal offset or byte-width read** into a union or struct + that has **any pointer member**, anywhere in that union/struct's + layout (not just the field being read) — the read doesn't need to + touch the pointer itself; any member *after* one that widened is at + risk. +- A field that is **correctly named but too narrow to hold what it's + actually storing** (an enum/`u32`/`s32` reused as a truncated pointer + slot — the D255 shape, distinct from D209/D210's literal-offset shape + but the same underlying cause). +- The resulting behavior is **deterministic and internally + self-consistent, but demonstrably not what the N64 original would + produce** — the code isn't crashing or producing garbage, it's + confidently doing the wrong (but stable) thing. +- Prefer the named field (or a correctly-typed sibling field) under + `#ifdef PORT`; the alias/pun is only correct at 32-bit pointer width. + Verify every caller reaches the site with the union/struct arm you're + naming actually active. + +**Distinguish from D284** (`port/src/random.c`'s MIPS64 +shift-emulation bug): same *epistemic* shape — deterministic, +self-consistent, silently not-N64 — but a different *mechanism* +(instruction semantics, not struct layout/pointer width) and, +critically, **entirely inside `port/`, no `src/game` touch at all**. +D284 was never a rule-2 question in the first place; don't fold it +into this catalogue's "looks like game logic, is really ABI" +framing — it never looked like game logic to begin with. - **Open landmine:** raw hardcoded-offset accessors into `struct player` / `struct hand`; see `docs/dev/AUDIT-M6-player-offsets.md`. - **D126 corollary; a trailing runtime list pointer in a ROM-serialized