Improve decompiled symbol names for ocarina interactions - #6967
Conversation
Rename the shared ocarina message helpers and the per-actor ocarina wait/listen handlers from raw func_ADDRESS / unk_ names to descriptive ones. No behavior change. - Message_StartOcarinaWithSongEffect / Message_StartOcarinaNoSongEffect (func_8010BD58 / func_8010BD88) - MessageContext.disableSongEffect (unk_E40E) - Player.ocarinaTargetActor (unk_6A8) - En_Okarina_Tag: WaitForPlayer / WaitForOcarina / WaitForPlaybackResult / HandleSongEvent / OfferTalk / WaitForDialogue; EnOkarinaTag.success - En_Box: EnBox_WaitForOcarina - En_Du: EnDu_WaitForOcarina / EnDu_ListenToOcarina - Obj_Warp2block: PlayerIsInRange / WaitForOcarina / WaitForSong / UpdateSongObserver, songObserverFunc + songEndTimer fields (mirrors its Obj_Timeblock sibling) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| /* 0xE408 */ Actor* talkActor; | ||
| /* 0xE40C */ s16 disableWarpSongs; // warp song flag set by scene commands | ||
| /* 0xE40E */ s16 unk_E40E; // ocarina related | ||
| /* 0xE40E */ s16 disableSongEffect; // ocarina related |
There was a problem hiding this comment.
we should be striving to match decomp, for example they named this field disableSunsSong
where they haven't made a rename there should be an upstream PR at least proposing the renames there
|
I think it is probably a better use of AI to incorporate named things in decomp into SoH than coming up with new names. I wouldn't trust AI names to fully reflect the usage of a certain function/variable, nor to conform to informal naming conventions (such as |
I'm not sure I understand your comment, I did not use the AI to guess the names, I changed the name using VSCode rename symbol tool, then asked the AI to review and create the PR to save some time. Maybe I misunderstood the code or the naming conventions tho, this is possible as I am still learning the codebase... The AI did not need this symbol renaming at all, Claude is very good without it... This was so it's easier to read the code myself, not for him him... Anyway, I'll close this, I added comments instead in #6951 . |
Drop the dependency on the ocarina symbol-rename PR (HarbourMasters#6967): the En_Du and En_Okarina_Tag listening handlers this feature calls are already non-static on develop, just not yet given descriptive names, so reference them by their raw func_ symbols (documented inline). Every other handler (En_Md, En_Ma2, Obj_Timeblock, Shot_Sun) is already named on develop. The feature now touches no decompiled files at all. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HjDjMt2YeFw6wzqYL1zM6Y
|
Sorry, it's hard to tell as, as you said, your PR text is AI-generated. But my point stands that the AI review didn't point out that |
Drop the dependency on the ocarina symbol-rename PR (HarbourMasters#6967): the En_Du and En_Okarina_Tag listening handlers this feature calls are already non-static on develop, just not yet given descriptive names, so reference them by their raw func_ symbols (documented inline). Every other handler (En_Md, En_Ma2, Obj_Timeblock, Shot_Sun) is already named on develop. The feature now touches no decompiled files at all. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HjDjMt2YeFw6wzqYL1zM6Y
Drop the dependency on the ocarina symbol-rename PR (HarbourMasters#6967): the En_Du and En_Okarina_Tag listening handlers this feature calls are already non-static on develop, just not yet given descriptive names, so reference them by their raw func_ symbols (documented inline). Every other handler (En_Md, En_Ma2, Obj_Timeblock, Shot_Sun) is already named on develop. The feature now touches no decompiled files at all. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HjDjMt2YeFw6wzqYL1zM6Y
3 participants
What
Renames raw
func_ADDRESS/unk_symbols involved in ocarina interactions to descriptive names. No behavior change - pure documentation.Shared message helpers
func_8010BD58->Message_StartOcarinaWithSongEffectfunc_8010BD88->Message_StartOcarinaNoSongEffectMessageContext.unk_E40E->disableSongEffectPlayer.unk_6A8->ocarinaTargetActorPer-actor ocarina handlers
WaitForPlayer/WaitForOcarina/WaitForPlaybackResult/HandleSongEvent/OfferTalk/WaitForDialogue;EnOkarinaTag.unk_158->successEnBox_WaitForOcarinaEnDu_WaitForOcarina/EnDu_ListenToOcarinaPlayerIsInRange/WaitForOcarina/WaitForSong/UpdateSongObserver,songObserverFunc+songEndTimerfields (mirrors itsObj_Timeblocksibling)Why
These names were reverse-engineered while working on ocarina-related features; splitting the naming out as a standalone, behavior-neutral change to keep functional PRs readable.
Testing
Builds clean; no functional changes.
Build Artifacts