Rename the stage id enums to the game's own grkind/stkind - #2969
Conversation
ground.c reports a failed stage-param lookup as "not found stage param in DAT(grkind=%d stkind=%d,num=%d)", filling the varargs from stage_info.internal_stage_id and StageIdPair::external_id (GALE01 0x801C2A48). So grkind is what this tree calls InternalStageId and stkind is ExternalStageId. The stkind reading is independently checkable off the disc: the list Ground_801C28CC searches is the archive's grGroundParam public symbol, so its keys ship in the Gr??.dat files. Across the 279 stage_id_map entries whose two ids differ, 271 archives are keyed by the external id and none by the internal one; GrSt.dat is grkind 10 keyed 0x08, GrCs.dat is grkind 2 keyed 0x04, and GrNBa.dat holds 18 rows, one per mode variant built on that ground. Forcing the lookup to miss in Dolphin reports grkind=22 stkind=26,num=9 for the second Icicle Mountain entry, matching the live internal id and the selected external id. Only the enum type names and the InternalStageID_/ExternalStageID_ member prefixes change; the unprefixed stage names and all field names are untouched. All 21 affected translation units compile byte-identical.
ribbanya
left a comment
ribbanya
left a comment
There was a problem hiding this comment.
Let's do another renaming pass. We need to rename internal_stage_id wherever it shows up to grkind and external_stage_id to stkind. If something is a shortened form like stage_id now, we can just rename that to kind. Examples in comments below.
ribbanya
left a comment
ribbanya
left a comment
There was a problem hiding this comment.
Apply these too.
- grGroundParam for the type currently called UnkStage6B0 (stage_info.param). Not inferred at all — it's the literal archive symbol the game looks up: HSD_ArchiveGetPublicAddress(sp14, "grGroundParam") in grdatfiles.c.
- Its xB0 / xB4 are the stage-param row array and its count — the same message names them: "not found stage param… num=%d", and xB4 is exactly the loop bound over xB0.
- UnkBgmStruct is a stage-param row, not a BGM struct, and its x0 is the stkind key — the sibling message prints each row's x0 as " stageid=%d", and both search loops (Ground_801C28CC, Ground_801C24F8) match it against a stkind. BGM is just one field on the row (x4). Provenance for the row data is in the message too: StageParam.csv / StageItem.csv, stdata.c.
grdatfiles.c fetches StageInfo::param by name: HSD_ArchiveGetPublicAddress(archive, "grGroundParam"), so UnkStage6B0 is the archive's grGroundParam and is renamed GroundParam. ground.c names the rows it points at. Ground_801C28CC reports a miss as "not found stage param in DAT(grkind=%d stkind=%d,num=%d)" followed by "check StageParam.csv or StageItem.csv, stdata.c", and then lists every row as " stageid=%d" -- printing the field this tree called UnkBgmStruct::x0. num is the loop bound, so xB0/xB4 are the stage param array and its count, and x0 is the key both searches compare against. That key is a stkind: Ground_801C0754 passes StageIdPair::external_id, and every Gr??.dat on the disc is keyed the same way (271 of the 279 stage_id_map entries whose two ids differ are keyed by the external id, none by the internal one). Hence StageParam::stkind, GroundParam::stage_params and GroundParam::stage_param_count. BGM is one field on such a row rather than the point of the struct, so the local named bgm in Ground_801C28CC becomes param. grdatfiles.c's no-archive fallback fits: a one-row list (stage_param_count 1) whose row is stkind 0 with BGM -1. ground.c and grdatfiles.c compile byte-identical.
ribbanya
left a comment
ribbanya
left a comment
There was a problem hiding this comment.
I could be wrong but it doesn't look like any of the GrKind enum members appear in asserts, in which case they should be renamed with the GrKind prefix. Both enums should use upper camel case instead of all-caps.
For what it's worth, the stage names mostly appear in if/soundtest starting at un_803FAB10:
TEST
Izumi
PStadium
Castle
Kongo
Zebes
Corneria
Story
Onett
MuteCity
RCruise
Garden
GreatBay
Shrine
Kraid
Yoster
Greens
Fourside
Inishie1
Inishie2
Akaneia
Venom
Pura
BigBlue
Icemt
Icetop
Flatzone
old ppp
old yosh
old kong
battle
last
TMario
TCaptain
TClink
TDonkey
TDrmario
TFalco
TFox
TIceclim
TKirby
TKoopa
TLink
TLuigi
TMars
TMewtwo
TNess
TPeach
TPichu
TPikachu
TPurin
TSamus
TSeak
TYoshi
TZelda
TGamewat
TEmblem
TGanon
Follows the review on doldecomp#2969: internal_stage_id becomes grkind, external stage ids become stkind, and shortened forms like stage_id become kind where the enum type already says which space it is. StageInfo::grkind, StageData::grkind, StageIdPair::{grkind,stkind}, the stage_id_map entry and selection fields, the Stage_ setup parameters, and the preload cache's stkind all follow their types. The four ground.c lookups that search StageParam::stkind take a named StKind parameter now, and gm_80187F48_GetStageId becomes gm_GetStKind over a stkind field, as suggested in review. Fighter-side internal_id in ft/ and pl/ is an unrelated character id and is untouched; StartMeleeRules::xE keeps its offset name. All 37 affected translation units compile byte-identical.
|
@ribbanya For code style do you prefer inline comments justifying names or should that just be left in git history? Don't want to spam unnecessary comments but I can see how this could be helpful info for determining that this is a correct and justified name. |
|
Comments like Are borderline LLM slop just due to length. It is useful info but it's also encoding a lot of info that's not directly relevant to the type. |
|
It's... slop. But it's all immediately verifiable. If anything, it just needs an editing pass and/or for the information to be spread out across different members in the docs. For now just change it to a multiline doc comment: /** @remarks
* The numbering used by #StartMeleeRules::xE and Stage_802251E8.
* #stage_id_map converts it to #GrKind, and each archive's @c grGroundParam
* list is keyed by it; entries 0x21+ are single-player, event, and other mode
* variants.
*
* Both names come from ground.c's report
* `not found stage param in DAT(grkind=%d stkind=%d,num=%d)`, which passes
* #StageInfo::grkind and then this value.
*/ |
Sounds good, yeah they're unjustified. 2 stages appear in all caps anywhere in the DOL it seems but it's not a consistent thing. Is |
|
Oh right, I forgot about |
I missed this earlier. I think it's preferable to have it live in the repo in some form so that it can be cleaned up later. I consider issues, PRs, Discord, and even the wiki to be transient. If it's kind of sus then you can always put it in .dox or a whole separate page in |
The ALL-CAPS spellings were invented: of 41 probed, only TEST (verbatim in the develop-mode stage list) and BATTLE (inside grbattle.c's i<BATTLE_BG_MAX assert) appear in main.dol at all, and none as enum members. Two sources do name these stages, so the members follow them, prefixed like It_Kind_ per CONTRIBUTING. GrKind takes the per-stage gr*.c module names the asserts embed, which 70 of its 71 members already matched; the exception was ICEMTN, whose module is gricemt.c, so it becomes Gr_Kind_Icemt. StKind takes the develop-mode stage list (strings at 0x803FAB04, pointers at 0x803FAF0C, 86 entries indexed 0x00..0x55, aligning with stkind: 0x02 Izumi, 0x49 8-1bbroute, 0x55 heal). That list names three holes: 0x00 Dummy, 0x15 Akaneia, 0x1A Icetop, the last being one of the two stkinds with no grGroundParam row. The two sources disagree on Yoshi's Island, so St_Kind_Yoster follows the list and Gr_Kind_Yorster follows gryorster.c, with a note at the member. Entries the list names beyond 0x20 are not added here. Also gives both enums hex value prefixes and moves the multi-line doc comments to block style. All 83 affected translation units compile byte-identical.
…rectives The two /// clang-format off|on lines in mplib.c never did anything: clang-format only honours the // and /* */ forms, so the table they wrapped was being reflowed regardless.
|
I probably don't have time to merge it tonight but looks good to me at first glance. |
ribbanya
left a comment
ribbanya
left a comment
There was a problem hiding this comment.
Could you do me a favor and have Claude output everything it renamed in this PR in the form of FROM:TO (melee-replace-symbols format) including field names, locals, etc.? I have a parallel PR that is going to conflict with this so it would help a lot.
Sounds good, take your time! And I really appreciate all the feedback and help! I'll self review it as a whole as well and take it out of draft mode after I do if it looks good to me. |
Here's the list (with commentary) from Claude: But feel free to land your changes first. Happy to replay these changes myself if master changes a lot. |
MarkMcCaskey
left a comment
MarkMcCaskey
left a comment
There was a problem hiding this comment.
Other than the comments I mentioned and some cases where constants could be replaced by enums / better typed this is fine. I'll do a bit more clean up but otherwise looks good
Replace the remaining 32-bit grkind/stkind declarations and obvious raw stage values in the rename diff with GrKind/StKind members.
2 participants
@ribbanya noticed that via a debug print that these have existing names we can use. This PR follows up on #2939 to use these names which is more consistent with the style of this repo.
From Claude:
More in depth evidence here https://github.com/MarkMcCaskey/melee-runtime-audits/blob/main/grkind-stkind/README.md
From Claude:
Also renamed the variable / member names that still say internal/external. And applied findings downstream of this; there's evidence for some more names for followups: