Skip to content

Match 600+ P2 functions & VU0/splice foundations#263

Open
lindskogen wants to merge 136 commits into
TheOnlyZac:mainfrom
lindskogen:fable
Open

Match 600+ P2 functions & VU0/splice foundations#263
lindskogen wants to merge 136 commits into
TheOnlyZac:mainfrom
lindskogen:fable

Conversation

@lindskogen

Copy link
Copy Markdown
Contributor

Brings the P2 decomp from 2567 → 1890 remaining INCLUDE_ASM stubs (~677 functions newly matched) across 118 source files / 23 headers, plus the supporting struct, SIMD, and tooling work to make those matches possible. Every change is verified against the ground-truth checksum (out/SCUS_971.98: OK).

Highlights

  • Engine base structs — SO padded to its real 0x550. ALO/LO remain intentionally truncated; subclasses lay out own fields from 0x550 and reach base fields via STRUCT_OFFSET.
  • VU0 / SIMD foundation — VU_VECTOR/VU_FLOAT fixed to 128-bit quadwords; VU0 inline-asm idiom established (CalculateWaterCurrent, UpdateWaterBounds, ClipVismapSphereOneHop), unblocking the water/clip math.
  • Splice subsystem — large bif/eval/frame/sidebag/ref progress (RefOp BIFs, matrix/vec ops, CFrame, CSidebag node lists); Splice crosses 20%.
  • Fully-decompiled units — sort.c (HeapSort), pipe.c, frzg.c.
  • Broad leaf/accessor coverage — getters/setters and small helpers across alo, so, sw, sound, mpeg, screen, rog, jt, crv, stepguard, and ~100 other units.

Symbol & comment cleanup (final commit)

  • SetAMRegister and OnDifficultyPlayerDeath keep extern "C" — both pass match.sh per-symbol but fail the full checksum when converted (caller-side uchar narrowing / a GCC 2.95 register-allocation flip); documented as load-bearing exceptions.
  • Trimmed decomp/tooling-explanation, AI-process, and TODO/blocker comments; condensed the "kept wrapped (unwrapping breaks the checksum)" notes to one line. Struct-field annotations and Doxygen retained.

Notes for reviewers

  • Some functions remain wrapped in #ifdef SKIP_ASM on purpose (they match per-symbol but mis-size a still-asm neighbor); the one-line notes say why.

Verification

./scripts/build.sh → out/SCUS_971.98: OK.

lindskogen and others added 30 commits June 5, 2026 22:35
scripts/decomp_lhf_draft.workflow.js fans out parallel read-only agents that
draft matching C for batches of tiny leaf functions. scripts/decomp_apply_drafts.py
verifies each draft in isolation (instructions + relocations vs the asm), rejects
ones that grow non-.text sections (rodata-layout ripple) or whose raw FUN_ symbol
has asm callers, then applies the byte-exact matches and renames FUN_ symbols.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
AddWaterExternalAccelerations dispatches through the SO vtable (offset 0x128)
via STRUCT_OFFSET; FUN_001ef830 clears zpd.cploThrow. Annotate the remaining
INCLUDE_ASM stubs with why they're deferred (VU0 SIMD, or permuter-class
codegen for PostWaterLoad/HandleWaterMessage).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Small getters/setters and helpers on ALO/SO/SW: interact/look-at/throb/ack
accessors, FInflictSoZap, UpdateSoPosWorldPrev, and SW list/world helpers.
Base-struct fields past the truncated structs are reached via STRUCT_OFFSET.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Small wrappers/getters/setters across act, chkpnt, cplcy, difficulty, dmas,
path, po, puffer, pzo, rog, sb, sensor, stepguard, and xform, drafted by the
batch workflow and verified byte-exact (instructions + relocations).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
VU_VECTOR was a 2-byte stub { ushort data; }, so passing it by value compiled
to lhu instead of lq. It is a 128-bit VU quadword passed in a single 128-bit GP
register, so define it as { qword data; } (TImode, 16 bytes, 16-aligned). Kept
as a struct named VU_VECTOR to preserve the 9VU_VECTOR by-value mangling.

This resolves the long-standing 89.47% "single load mismatch" in
ClipVismapSphereOneHop, now matched as plain C. VU_VECTOR is only used by value
in signatures (never embedded), so the size change ripples nothing.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
VU_FLOAT was a 2-byte stub { uint16_t data; }; like VU_VECTOR it is a 128-bit
VU scalar (the VU_FLOAT(float) ctor stores it with sq, and it is passed by value
in a single 128-bit GP register). Define it as { qword data; }. Used only by
value in signatures, so the size change ripples nothing; full checksum OK.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
VU_VECTOR was a 2-byte stub { ushort data; }, so passing it by value compiled
to lhu instead of lq. It is a 128-bit VU quadword passed in a single 128-bit GP
register, so define it as { qword data; } (TImode, 16 bytes, 16-aligned). Kept
as a struct named VU_VECTOR to preserve the 9VU_VECTOR by-value mangling.

This resolves the long-standing 89.47% "single load mismatch" in
ClipVismapSphereOneHop, now matched as plain C. VU_VECTOR is only used by value
in signatures (never embedded), so the size change ripples nothing.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
VU_FLOAT was a 2-byte stub { uint16_t data; }; like VU_VECTOR it is a 128-bit
VU scalar (the VU_FLOAT(float) ctor stores it with sq, and it is passed by value
in a single 128-bit GP register). Define it as { qword data; }. Used only by
value in signatures, so the size change ripples nothing; full checksum OK.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Establish the inline-asm VU0 idiom on the small vector/scalar primitives,
building on the VU_VECTOR/VU_FLOAT 128-bit type fixes:

- VU_VECTOR(const VECTOR&) and VECTOR::operator=(VU_VECTOR): lq/sq quadword
  copies (plain C through the qword member).
- operator*(VU_FLOAT, VU_VECTOR): qmtc2.ni / vmulx.xyzw / qmfc2.ni via inline
  asm, with VU types passed by value in single 128-bit GP registers ($4/$5/$2,
  no extra moves).

Adds the ctor/operator declarations to vec.h. VU_FLOAT(float) stays INCLUDE_ASM
for now (one scheduling instruction off). Full checksum OK.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
First water VU0 function matched: the vadd.xyzw combining the warp result is
emitted via inline asm (lqc2/lqc2/vadd.xyzw/sqc2) over VU_VECTOR stack locals;
the surrounding ConvertAloVec/CalculateAloTransformAdjust/WarpWrTransform calls
and the *pv/*pw quadword copies are plain C. Adds WATER::vecCurrent at 0x570 and
references the shared D_00248D30 rodata constant. Full checksum OK.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The sphere-bounds length (dot product + vsqrt) and the AABB vsub/vadd are
emitted as one VU0 inline-asm block over the GetWrBounds result and the SO
bounds fields (sRadiusBounds/unk_0x3d4/vecBoundsMin/vecBoundsMax). vsqrt has no
assembler mnemonic so it's a .word, like the original .s. Full checksum OK.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
LSG's apos/anormal are 16-byte VU vectors; widen them to VECTOR4 and 16-align
the struct so sizeof(LSG) is the real 0x70 (au at 0x40/0x44, apos[1] at 0x10).
This is the key fix for water's clip-using functions: a typed LSG[16] clip
workspace now lands at frame 0x20 (after the edge vectors) instead of the base,
matching the original frame layout. No ripple — full checksum OK.

With that, UGetWaterSubmerged is written (VU0 vmulax/vmaddx transform, g_pjt
height logic, ClsgClipEdgeToBsp into LSG[16], float return) and ~95% matched;
kept under SKIP_ASM (asm still used by the real build) pending permuter work on
a few g_pjt-branch scheduling instructions.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Byte-exact, checksum-green (out/SCUS_971.98: OK):
- PtnfnFromTn (tn): null-check ternary returning &D_00275980 or ptn+0x2F0
- FUN_0014c820 (crusher): indexed pointer into 0xB0-stride array
- extern "C" leaf accessors called by other asm via unmangled names:
  InitCplcy, FActiveCplcy, SetCpmanCpmt, FUN_001e4880, FUN_00145DD8,
  FUN_001c9a48

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Byte-exact, checksum-green:
- RetractLasen/ExtendLasen (sensor): +-1.0f/dt stored at 0xB08
- GetActvalScale (act): 3x qword copy ACTVAL+0x90 -> MATRIX3 rows
- SetLookerSgvr (shdanim): store 3 LOOKER field addrs into SGVR
- InvalidateSwXpForObject (bbmark): flags &= ~grfpva on pso->sw
- FUN_0014c838 (crusher): indexed append into 0x14-stride array

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
These showed t==c "DIFFER" in the dual-build only because their jal
targets a still-INCLUDE_ASM sibling (reloc-name artifact); the full
link is byte-identical (out/SCUS_971.98: OK):
- ImpactClue -> ImpactSo
- func_0015F658 -> func_0015F618
- AddStepCustomXps -> AddStepCustomXpsBase

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two tail-call wrappers whose raw C-linkage siblings needed forward
declarations (extern "C"). checksum-green (out/SCUS_971.98: OK):
- call_search_level_by_id -> search_level_by_id
- FUN_001d34e0 -> FUN_001bc4d8 (renamed FUN_001d34e0 -> mangled in
  symbol_addrs; it has no asm callers)

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
UMaxCrv/SMaxCrv (crv): mpicvu/mpicvs[ccv-1]. TouchFlake/TouchBublet/
OnTrailRemove (rip): thin guarded/forwarding wrappers. All showed
"DIFFER" only via dual-build artifacts (trailing unsymboled JUNK
macros, and jal-to-external-sibling reloc names); full link is
byte-identical (out/SCUS_971.98: OK).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
checksum-green (out/SCUS_971.98: OK):
- GetActrefScale (act): explicit qword copy of MATRIX3 via loaded ptr
  (MATRIX3 is float[3][3], align-4; a plain *pmat=*p went unaligned)
- AddFrzgObject (frzg): cache coid in a local so it isn't reloaded
- FUN_001a93c8 (rwm, extern "C", has asm callers) + FUN_001a86f8
  (renamed to mangled; no asm callers)
- FUN_001cf138 (stephide, extern "C"): add 1000.0f loaded from
  D_00274E3C rather than materialized as an immediate

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two leaf functions; checksum-green (out/SCUS_971.98: OK):
- GetSuvCpdefi: tail-call forwarding to GetSoCpdefi
- SetAsegaSpeed: chained float multiply stored at 0x18

(match.sh's per-symbol check falsely reported DIFFER on both due to
flaky configure.py asm regeneration / trailing-junk artifacts; the
full-link checksum is the ground truth.)

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…CrvcCache/DuGetCrvSearchIncrement/MeasureCrvl)

checksum-green (out/SCUS_971.98: OK).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
checksum-green (out/SCUS_971.98: OK): UpdateSuvInternalXps, OidFromSmIsms,
PostFlyingEmit (+SgnCmpHp), InitDysh (wrapper to InitAlo), FUN_0014f900.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…harness

checksum-green (out/SCUS_971.98: OK). Accessors, wrappers and vtable-dispatch
leaves across 6 units; size/content-mismatched drafts auto-rejected.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…F harness

checksum-green (out/SCUS_971.98: OK). Accessors, vtable-dispatch and
forwarding wrappers; size/content/data-growth mismatches auto-rejected.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
checksum-green (out/SCUS_971.98: OK): PushCplookLookk, FUN_0014a8d0,
VacateCredit, SetLightHighlightColor, BreakClue, SetShadowFrustrumUp.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Tail wrapper FUN_001aea70(1, 0xFFFF); extern "C" fwd-decl of the raw
sibling; renamed FUN_001aec90 -> mangled (no asm callers). checksum-green.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
grfrob bit tests on g_plsCur; '(x & 2) > 0' steers GCC to andi+sltu
(matching the ROM) instead of the shift-fold. Removed stale 80%% scratch.
checksum-green (out/SCUS_971.98: OK).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
lindskogen and others added 3 commits June 23, 2026 00:03
These were local agent/tooling artifacts that don't belong in the repo. The
ignore patterns (CLAUDE.md, .claude/, docs/DECOMP_PROMPT.md, report_*.json)
move to .git/info/exclude so they stay ignored locally without being tracked;
.gitignore returns to its upstream form. CLAUDE.md stays on disk, untracked.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
origin/main's content is fully contained in fable (every main commit has a
patch-equivalent here), so this records the integration without changing the
tree. Verified: out/SCUS_971.98: OK.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
PchzFriendlyFromWid is an UpperCamelCase C++ function (its real symbol is
mangled), so its extern "C" in game.h was only a still-asm shortcut, not a
genuine requirement. Pre-mangle the symbol to PchzFriendlyFromWid__Fi (update
symbol_addrs.txt + the INCLUDE_ASM label) and declare it plain C++; it has no C
callers, so this is byte-neutral.

The remaining extern "C" in game.h (tally_world_completion, reload_post_death,
clr_8_bytes_1) are genuinely C-linkage exports of the level/save subsystem
-- siblings of get_level_completion_by_id / LsFromWid, which are bound to their
literal C symbols via __asm__(). Their real symbol IS the unmangled name, so
extern "C" is the correct permanent declaration, not a wart.

Verified: out/SCUS_971.98: OK.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@lindskogen lindskogen marked this pull request as draft June 22, 2026 22:15
lindskogen and others added 2 commits June 23, 2026 00:19
Remove inline offset trailers, "// ..." placeholders, commented-out fields, and
the padding/mangling/node-list rationale notes. Doxygen API blocks kept.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
match.sh, structoff.sh, decomp_apply_drafts.py and decomp_lhf_draft.workflow.js
are local agent/decomp tooling, not part of the game build (they were committed
to main by an earlier session). Remove from tracking; they stay on disk via
.git/info/exclude.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@TheOnlyZac TheOnlyZac left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow that's a lot of code! it might a while to review this. The first and main thing I've noticed is that many functions are declared extern "C" in the P2 sources, but I think the P2 sources are all CPP (the only place I've seen C functions are the statically linked libraries).

@TheOnlyZac

TheOnlyZac commented Jun 23, 2026

Copy link
Copy Markdown
Owner

I'm actually getting build errors after configuring the project with the --objects flag (which builds with SKIP_ASM defined), I think because of the discrepancy between the C function declarations in the headers and the CPP function definitions in the source files.

The codebase compiles as C++, so functions whose binary symbol is unmangled
needed an extern "C" wrapper. Many FUN_xxxx/func_xxxx placeholders, however,
are already decompiled (real C body, no INCLUDE_ASM) and only kept the wrapper
to emit the unmangled symbol_addrs label. Convert ~77 of them to plain C++:
pre-mangle their config/symbol_addrs.txt entries to the GCC2-mangled form and
drop extern "C". This is checksum-neutral (symbols are stripped from the final
ELF; the compiled bytes are unchanged) — splat regenerates the asm callers,
.s filenames, and auto linker scripts on the next configure.

Also fix two latent SKIP_ASM build errors surfaced by the cleanup:
- OnDifficultyPlayerDeath: header declared it plain C++ while the definition
  used extern "C"; drop the wrapper so both agree.
- ub.c: declare PostGomerLoad in its owning header (gomer.h).

Left as-is: functions still in assembly (INCLUDE_ASM / asm forward-decls),
where extern "C" is correct scaffolding until they are reversed; plus
FUN_001c9a48 (conflicting signatures across files) and the rwm/font mixed
extern "C" blocks.

Verified: out/SCUS_971.98: OK.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@lindskogen

Copy link
Copy Markdown
Contributor Author

@TheOnlyZac I fixed the build errors and a bunch of extern "C". I will resolve the merge conflict.

lindskogen and others added 2 commits June 24, 2026 00:19
Resolve conflicts in 13 files between our extern "C" cleanup / leaf matches
and upstream's newer decomp work. Per translation unit either branch's matched
version is byte-identical to the target, so each conflicted unit was resolved to
one coherent side:

- ours (we matched; upstream still INCLUDE_ASM): water.c, dmas.c, gifs.c,
  sound.c, alo.c, so.c, and our FUN_ symbol pre-manglings (alo/sw) in
  symbol_addrs.txt.
- theirs (upstream advanced further): murray.c, font.c, the real CFontBrx
  layout in font.h, so.h comments, and FUN_001c9a48 unified to (STEPGUARD*,int)
  in stepguard.c (resolving the prior cross-file signature conflict).
- combined declarations: stepguard.h, font.h.

Integration fixups so the merged tree links:
- rename VTACT globals D_00219560/D_0021A790/D_002195D8 -> g_vtact/g_vtactla/
  g_vtactadj in act.c and alo.c (upstream renamed them in symbol_addrs).
- game.h: reload_post_death is now plain C++ (reload_post_death__Fv), drop
  its extern "C".

Verified: out/SCUS_971.98: OK.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
tally_world_completion has no C++ callers — it's only referenced asm-to-asm,
so the declaration in game.h is inert and the extern "C" served no purpose
(it only matters when C++ code emits a reference to an unmangled label). Our
459b0b1 added it defensively; revert to the plain declaration, matching
upstream/base.

A sweep of every extern "C" declaration (inline + brace-blocks) confirmed this
was the only caller-less one; all others have real C++ callers.

Verified: out/SCUS_971.98: OK.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@lindskogen lindskogen marked this pull request as ready for review June 23, 2026 23:20
@TheOnlyZac

Copy link
Copy Markdown
Owner

Awesome, thanks. I will start making review comments, but it will take me some time to finish.

@TheOnlyZac

TheOnlyZac commented Jun 24, 2026

Copy link
Copy Markdown
Owner

@lindskogen It seems that the model has littered the code with extern data and function declarations directly above function definitions that reference them, even when those data/functions should logically be declared somewhere else (for example see P2/cm.c). Do you think this can be addressed?

@lindskogen

Copy link
Copy Markdown
Contributor Author

@lindskogen It seems that the model has littered the code with extern data and function declarations directly above function definitions that reference them, even when those data/functions should logically be declared somewhere else (for example see P2/cm.c). Do you think this can be addressed?

I will check it out

Comment thread src/P2/dialog.c Outdated
INCLUDE_ASM("asm/nonmatchings/P2/dialog", PostDialogLoad__FP6DIALOG);
void PostDialogLoad(DIALOG *pdialog)
{
extern int *PfLookupDialog(LS *pls, OID oid);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move function prototypes elsewhere.

Comment thread src/P2/splice/bif.cpp Outdated
INCLUDE_ASM("asm/nonmatchings/P2/splice/bif", RefOpSetMusicRegister__FiP4CRefP6CFrame);
CRef RefOpSetMusicRegister(int carg, CRef *aref, CFrame *pframe)
{
SetAMRegister__FiUc(aref[0].m_tag.m_n, aref[1].m_tag.m_n);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unmangle name.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now it's conclusive. The body @ 0x1c0c08 doesn't mask its 2nd param either — move a2,a1 then uses it as a full word throughout (beq a2,v0 / sw a2 / passes a2). So nowhere — not the callers, not the body — does the original narrow the 2nd argument to 8 bits.

Answer: no, no natural combination can unmangle it

The original SetAMRegister__FiUc symbol is essentially a misnomer — it's named as taking unsigned char but is compiled as taking a full int everywhere. That makes a true unmangle impossible:

  1. Parameters aren't free — __FiUc decodes to exactly (int, unsigned char), and unsigned char is the only type that mangles to Uc (typedef/const don't change that). So any naturally-mangled declaration must declare the 2nd param uchar.

  2. A uchar param must narrow somewhere — GCC 2.95 will emit an andi …,0xff at a call site (or the callee). But the original has no narrowing instruction anywhere. Any insertion is a byte the original doesn't have → broken match. (That's the andi that killed the bif caller.)

  3. Arguments can't rescue it either — to avoid the narrow you'd need the source value to already be a byte, but the original loads it as a word (lw a1,12(a2)). A uchar-typed field would compile that load as lbu instead → now you break the load rather than the call.

I don't really understand why, but the function cannot be unmangled

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When 545u says unmangle the name here, I believe they just mean drop the __FiUc from the name. But I think we're getting at the real problem, which is the declaration of SetAMRegister in sound.h.

// The symbol's GCC2 mangling says (int, unsigned char), but the ROM call
// sites pass the second argument as a plain word with no narrowing, so the
// in-game declaration must have taken ints; bind by literal symbol name.
extern "C" void SetAMRegister__FiUc(int n, int bReg);

This cannot be faithful to the original source code, and it's illogical to say it "can't be unmangled"; the fact that we have debug symbols with mangled function signatures is evidence that there is a canonical, original signature that the compiler mangled to produce the symbols.

Keep in mind we use the debug symbols from the May prototype as hints, but the code changed between May and September so the mangled function name might not be accurate indicator of the parameter types in the final build, which we are targeting. So the goal isn't really to "unmangle" it to get the signature, but rather to holistically figure out what the signature is supposed to be based on the code.

Comment thread src/P2/989snd.c
lindskogen and others added 2 commits June 25, 2026 00:23
Per PR review (example: P2/cm.c): forward-declarations of functions defined
elsewhere were scattered inline directly above the functions referencing them.
Move each to its owning unit's header so declarations live in one logical place.

- same-unit decls to that unit's header: cm.h, binoc.h, crusher.h, frm.h,
  mpeg.h, puffer.h, rwm.h (deduped a double-decl), screen.h
- cross-unit to the owning unit's header: PreloadVag1 to sound.h,
  FUN_001d4c98 to stepzap.h, strcpy1 to text.h
- new headers for units that lacked one: jp.h (func_001781E0),
  sce/libs.h (func_00202120/58, memmove)
- drop frm.c's duplicate SignalSema decl (already in eekernel.h)
- game.h: the relocated search_level_by_id replaces its stale commented-out line

Declaration-only change. out/SCUS_971.98: OK.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Three "Match wave-N" commits removed previously-matched functions while adding
new ones (bad rebase/merge artifacts): c436f8a (wave-4), e02bf9b (wave-8),
459b0b1 (wave-9). The functions still exist upstream. Restore them -- all are
#ifdef SKIP_ASM bodies, so the binary is unchanged:

- 989snd.c: snd_GotReturns
- util.c:   CSolveQuadratic
- light.c:  CloneLight
- sensor.c: SetSensorSensors, FUN_001afaf8
- game.c:   UnlockIntroCutsceneFromWid

out/SCUS_971.98: OK.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@TheOnlyZac

TheOnlyZac commented Jul 2, 2026

Copy link
Copy Markdown
Owner

Hi again @lindskogen, thank you for the PR and I appreciate you wanting to push our progress forward with all the new function matches. Unfortunately the code has a many small problems and will require a lot of clean up before merging. I would ask that you please make any requested changes yourself and not have the AI do it, as it appears the model doesn't fully understand the project and is getting stuck in problematic loops.

I am going to continue making review comments until I have gotten through all the changes. Then in the interest of getting this merged as soon as possible, I will also lend a hand when I have time, so please make sure to pull any commits I make before working on it yourself. Also please refrain from adding any new code until we wrap up this PR.

@TheOnlyZac TheOnlyZac left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I've done my first pass and I noticed the vast majority of issues with the code fall into one of four categories:

  1. Unnecessarily separation of a variable declaration from its definition and use
  2. Nested if statements that can be combined with &&
  3. Functions definitions marked as extern "C" that should be C++
  4. Using repeated if statements to set a variable where the ternary operator could be used

There's a few other common quirks, but these cover a majority of my comments.

What will really take the most time to fix, I think, is that whenever the model got an undefined reference error while matching a function, it just defined whatever data/function was missing directly above the function body.

We will have to go through each one and determine where to move the relevant data/function declaration to. I'm open to suggestions as for the logistics of that.

Comment thread src/P2/crv.c Outdated
float GWrapApos(float g, int cpos, float *mpiposg, int fClosed)
{
float f0 = g;
if (fClosed == 0) {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(fClosed == 0) —> (!fClosed)

Comment thread include/splice/splotheap.h Outdated
Comment on lines +49 to +50
extern "C" void *FUN_0011C498();
extern "C" void FUN_0011C418(void *psbbFrom, void *psbbTo, CFrame *pframe);

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are these extern "C"?

Comment thread include/font.h Outdated
Comment on lines +127 to +132
extern "C"
{
CFont *FUN_0015c1c0(int i);
int FUN_0015c188(int i);
}

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are these and why are they extern C?

Comment thread include/jt.h

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please re-add the deleted comments

Comment thread include/so.h
Comment thread src/P2/so.c

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is too complex for me to fully review in this first pass, just leaving this note so we can circle back on it later.

Comment thread include/jp.h
struct JT;
struct LOCKG;

extern "C" void func_001781E0(JT *pjt, LOCKG *plockg);

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No extern C

Comment thread src/P2/sound.c Outdated
Comment on lines +29 to +31
extern u_int D_0027473C;
extern u_int D_00274728;
extern int D_00274720;

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete, should be global data

Comment thread src/P2/sound.c Outdated

INCLUDE_ASM("asm/nonmatchings/P2/sound", FUN_001be708);
extern u_int D_00274744;
void StopVag();

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete

Comment thread src/P2/sound.c Outdated
Comment on lines +171 to +174
if (D_00274744 == 0)
{
if (D_0027472C != 0)
{

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merge if conditions

@lindskogen

lindskogen commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

@TheOnlyZac soo I might be fully gaslit by the AI by now, but your suggestions of merging the if statements and introducing ternaries in a few places made it not match anymore. And when I asked claude why, the answer was that gcc would optimize it differently..

I could try to split up the PR to make it less of a pain to review, just say the word.

Fix compile/link breakage from the review-comment pass, move remaining
extern declarations into headers, and drop extern "C" via pre-mangled
symbol names. Verified: out/SCUS_971.98 checksum OK.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@lindskogen

Copy link
Copy Markdown
Contributor Author

There! I got a bunch of comments resolved!

Some notes:

  • UNALIGNED_INT is load bearing and is needed to reproduce a unaligned store in FUN_001ddb20.
  • nullptr doesn't exist in GCC 2.95.
  • Merging some if-statements with && and ternaries broke the match since it was now optimized differently.

Lets work on this together! Hoping this helps!

@TheOnlyZac

TheOnlyZac commented Jul 3, 2026

Copy link
Copy Markdown
Owner
  • UNALIGNED_INT is load bearing and is needed to reproduce a unaligned store in FUN_001ddb20.
  • nullptr doesn't exist in GCC 2.95.
  • Merging some if-statements with && and ternaries broke the match since it was now optimized differently.
  • Sorry I don't remember asking about UNALIGNED_INT or FUN_001ddb20 specifically, what are you referring to?
  • You're right, I wrote nullptr in a few of my comments where I should have wrote NULL
  • I see, I didn't think anywhere I commented that would change the asm, but I might have been mistaken. Could you give me an example of where you tried to change it and it broke the match?

Also, again I would ask you to please stop using AI to edit the code, as it is creating more problems.

@TheOnlyZac TheOnlyZac left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Second pass of just the files changed since my last review (it's a lot more of the same). I realized I've been using the word "definition" a lot when I really mean "initialization", so be aware of that.

Comment thread include/binoc.h
enum JV m_jv; // Vertical text justification;
};

extern "C" void open_close_binoc(BINOC *pbinoc, int state);

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No extern C

Comment thread include/cm.h
Comment on lines +556 to +561
void ResetCmLookAtSmooth(CM *pcm, void *pv);

void SetupCmRotateToCam(CM *pcm);
void SetCmLookAtSmooth(CM *pcm, int a1, VECTOR *pposEye, VECTOR *pposCenter, int a4, float u0, float u1, float u2, float u3, float u4, float u5);
void ConvertCylindToWorldVelocity(void *a, void *b, void *c, float f0, float f1, float f2);
void ConvertWorldToCylindVelocity(void *a, void *b, void *c, void *d, void *e, void *f);

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's either make sure these are in the correct order in the header, or add a @todo comment to come back and do that later

Comment thread include/crusher.h
Comment on lines +76 to +80
extern "C" void FUN_0014c5e8(void *p);

extern "C" void FUN_0014c858(void *p);

extern "C" void FUN_0014cba8(void *p);

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No extern C

Comment thread include/frm.h

extern uchar g_abRenderLoopStack[0x20000]; // TODO: Move elsewhere?

extern "C" void func_0015F618(int, int);

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No extern C

Comment thread include/game.h
// LevelLoadData * search_level_by_load_data(LevelLoadData *search_level);

// LevelLoadData * search_level_by_id(int search_id);
extern "C" LevelLoadData *search_level_by_id(int search_id);

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No extern C

Comment thread src/P2/suv.c
Comment on lines +23 to +28
if (track * 0.5f < relative) {
relative -= track;
} else if (relative < track * (-0.5f)) {
relative += track;
}
return relative;

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put curly braces on new lines

Comment thread src/P2/emitter.c
#ifdef SKIP_ASM
void LoadEmitblipColorsFromBrx(EMITBLIP *pemitblip, int crgba, LO *ploEmit, CBinaryInputStream *pbis)
{
int i;

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try and declare i when setting up the loop

Comment thread src/P2/screen.c
Comment on lines +308 to +309
CFont *pfont;
void *pv;

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try merging decls

Comment thread src/P2/sound.c
Comment on lines +31 to +32
extern u_int D_0027473C;
extern u_int D_00274728;

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete

Comment thread src/P2/sound.c
Comment on lines +244 to +246
extern int D_00274720;
extern u_int D_00274728;
extern u_int D_0027473C;

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants