Start documenting some undocumented player data#1045
Start documenting some undocumented player data#1045
Conversation
ItsLeaze
force-pushed
the
document-some-trainer-data
branch
from
c0b9cf8 to
2749862
Compare
ravepossum
left a comment
ravepossum
left a comment
There was a problem hiding this comment.
Welcome, and thank you for the contribution! Don't hesitate to ask for clarification or help with any of the feedback here.
| s32 ov66_0222E760(const UnkStruct_ov66_0222E71C *param0, u32 param1); | ||
| s32 ov66_0222E798(const UnkStruct_ov66_0222E71C *param0); | ||
| u32 ov66_0222E79C(const UnkStruct_ov66_0222E71C *param0); | ||
| u32 GetTrainerID(const UnkStruct_ov66_0222E71C *param0); |
There was a problem hiding this comment.
issue: We follow the general convention of Module_Function() when naming non-static functions to minimize namespace issues and mimic the pseudo-OOP structure of the game. Since that hasn't been identified yet here, you'll need to identify that. This would go for all the non-static functions here.
We have a list of descriptions of each overlay here which indicates that overlay66 is for the Wi-Fi Plaza orchestrator. There's a handful of files in this overlay though, so it's one specific part of the plaza orchestrator. If you're having trouble identifying or naming the module, feel free to ask in the discord.
| UnkStruct_ov66_02231428 ov66_0222E7C4(const UnkStruct_ov66_0222E71C *param0); | ||
| u32 ov66_0222E7C8(const UnkStruct_ov66_0222E71C *param0); | ||
| u32 ov66_0222E80C(const UnkStruct_ov66_0222E71C *param0); | ||
| u32 GetGender(const UnkStruct_ov66_0222E71C *param0); |
There was a problem hiding this comment.
suggestion: It may also be a good idea to identify what exactly UnkStruct_ov66_0222E71C is before naming these functions. I don't want to expand the scope of the PR (I think starting small is good), but we tend to hold off on naming functions until we're certain we know what they do. When reviewing, I apply a lot more scrutiny to a named function than one left unnamed. Leaving a function (or anything really) unnamed that you're not sure about is perfectly valid.
| u8 unk_32[6]; | ||
| u8 unk_38; | ||
| u32 trainerID; | ||
| u16 trainerName[8]; |
There was a problem hiding this comment.
polish: 8 here is TRAINER_NAME_LEN + 1.
| u8 unk_4C[12]; | ||
| s32 unk_58[12]; | ||
| u16 unk_88[2]; | ||
| u16 types[2]; |
There was a problem hiding this comment.
suggestion: I'm a little hesitant to name this without having more context on its usage. I think types is pretty vague.
| { 0x25, 0x1 }, | ||
| { 0x2A, 0x1 }, | ||
| { 0x3F, 0x1 } | ||
| static const TrainerAppearanceAndGender AppearanceGenderCombinations[16] = { |
There was a problem hiding this comment.
suggestion: These constants look like graphics ID values to me - take a look the generated constants in build/generated/object_events_gfx.h. We should use those constants for them and also take that into account in how the related functionality here is named. For instance, I think a more appropriate name for the corresponding struct member may be graphicsID.
I think it would also be better to be more specific with the structure name - maybe something like WifiPlazaTrainerAppearance. If we did that, then I think a more appropriate name for the array would be something like sWifiPlazaTrainerAppearances. In general, when I document things, I tend to err on the side of specificity to eliminate ambiguity and namespace conflicts. Especially since there's TrainerAppearances elsewhere in the codebase like appearance.c.
issue: static variables should be prefixed with s and be camelCase.
polish: 16 here could be APPEARANCES_COUNT * 2. Now that we know it's used elsewhere, we could lift that constant out of appearance.c into appearance.h and include it here.
| } | ||
|
|
||
| int ov66_0222E8E8(const UnkStruct_ov66_0222E71C *param0, u32 param1) | ||
| int ov66_0222E8E8(const UnkStruct_ov66_0222E71C *param0, u32 index) |
There was a problem hiding this comment.
note: Similar thoughts on using types and index here - it could potentially be more descriptive in context once we know the context. I don't know enough about the Wi-Fi Plaza to recognize it off-hand.
| void ov66_0222ECD4(UnkStruct_ov66_0222DFF8 *param0, u16 param1) | ||
| { | ||
| BOOL v0 = 0; | ||
| BOOL isTrainerNameValid = 0; |
There was a problem hiding this comment.
polish: 0 -> FALSE. 1 -> TRUE for the similar assignments below.
| static BOOL ov66_0222F1B4(const u16 *trainerName, u32 param1) | ||
| { | ||
| int v0; | ||
| int idx; |
There was a problem hiding this comment.
polish: I bet this could be declared as it's initialized in the loop.
| Pokedex *pokedex; | ||
| GameTime *v4; | ||
|
|
||
| { |
There was a problem hiding this comment.
polish: There's a handful of unnecessary scope blocks we could remove here.
| { | ||
| TrainerInfo *v0; | ||
| Party *v1; | ||
| TrainerInfo *trainerInfo; |
There was a problem hiding this comment.
polish: I bet a lot of these could be declared as they're initialized as well, but don't stress trying to get every single one if it seems like they don't all match.
2 participants
No description provided.