Renaming variables and functions related to overworld poison damage and FieldOverworldState #307

Viperio19 · 2024-11-18T21:24:50Z

First commit: Renaming sub_02054B04 to Pokemon_DoPoisonDamage and renamng all the variables in that function and related functions.
Second and third commit: Renaming various functions and variables in field_overworld_state.c and related files.
Fourth commit: Renaming variables in unk_02054884.c.

There's one function name in field_overworld_state.c and some in unk_02054884.c which I wasn't sure what to name, so I left those as they are. Also, let me know if this PR was too all over the place, I just started from somewhere and then one renamed variable lead me to other variables to be renamed.

ravepossum

A few thoughts on names, and a variety of small style stuff we're working on cleaning up. For the style matters, I only commented on the first occurrence of each that I noticed. Thanks for another contribution :)

ravepossum · 2024-11-19T01:32:42Z

include/field_map_change.h

-void FieldTask_ChangeMapByLocation(FieldTask *param0, const Location *param1);
-void FieldTask_ChangeMapToLocation(FieldTask *param0, int param1, int param2, int param3, int param4, int param5);
-void FieldTask_StartMapChangeFull(FieldTask *param0, int param1, int param2, int param3, int param4, int param5);
+void FieldSystem_StartChangeMapTask(FieldTask *taskMan, const Location *param1);


thought: we're kind of all over the place with this in the repo at the moment - I'm seeing task, taskMan, and taskManager. Maybe something we should look into, but doesn't need to be solved with this PR or anything.

It's being normalized to task for FieldTask, because there's no "management" aspect to it. There is a light call-stack, but that doesn't totally qualify it as a "manager" to me.

should I change all of them to task then? I can just immediately do it in other files as well, if this is something that you want to normalize across the whole repo

The ones in this PR would be nice, but I wouldn't sweat the whole repo unless you're just enthusiastic to do so. I'd probably do that as a follow-up PR if you did though.

ravepossum · 2024-11-19T01:34:30Z

include/field_overworld_state.h

@@ -18,10 +18,10 @@ void FieldOverworldState_Init(FieldOverworldState *param0);
 Location *FieldOverworldState_GetPlayerLocation(FieldOverworldState *param0);
 Location *FieldOverworldState_GetEntranceLocation(FieldOverworldState *param0);
 Location *FieldOverworldState_GetPrevLocation(FieldOverworldState *param0);
-Location *sub_0203A72C(FieldOverworldState *param0);
+Location *FieldOverworldState_GetExitLocation(FieldOverworldState *param0);


polish: The declarations' parameter names should match the definitions' ones. (same with the one in unk_02054884.h)

ravepossum · 2024-11-19T02:04:22Z

src/overlay005/field_control.c

@@ -905,14 +905,14 @@ static BOOL Field_UpdatePoison(FieldSystem *fieldSystem)
        return FALSE;
    }

-    switch (sub_02054B04(party, MapHeader_GetMapLabelTextID(fieldSystem->location->mapId))) {
+    switch (Pokemon_DoPoisonDamage(party, MapHeader_GetMapLabelTextID(fieldSystem->location->mapId))) {
    case 0:


suggestion: It would be good to add descriptive constants for these cases (and the corresponding return values in Pokemon_DoPoisonDamage().

ravepossum · 2024-11-19T02:09:19Z

src/overlay005/fieldmap.c

-    u32 v1;
-    int v2, v3;
-    FieldOverworldState *v4;
+    u32 mapId;


polish: We're working on cleaning up C89-style declarations to be more concise and declared when needed, as long as it matches. For example:

- u32 mapId; ... - mapId = fieldSystem->location->mapId; + u32 mapId = fieldSystem->location->mapId;

I think these should match.

unfortunately that doesn't seem to match

ravepossum · 2024-11-19T02:10:44Z

src/overlay005/fieldmap.c

@@ -426,10 +426,10 @@ static BOOL FieldMap_ChangeZone(FieldSystem *fieldSystem)
    }

    {


polish: We're also working on cleaning up unnecessary scope blocks like these.

Looks like there's still a couple pesky extra scope blocks in this function.

Should be taken care of now

ravepossum · 2024-11-19T02:29:28Z

src/unk_02054884.c

-        if (Pokemon_CanBattle(v4)) {
-            if (Pokemon_GetValue(v4, MON_DATA_STATUS_CONDITION, NULL) & (MON_CONDITION_TOXIC | MON_CONDITION_POISON)) {
-                u32 v5 = Pokemon_GetValue(v4, MON_DATA_CURRENT_HP, NULL);
+        if (Pokemon_CanBattle(mon)) {


suggestion: I think you can collapse these two if statements into one with a conjunction and it'll still match.

ravepossum · 2024-11-19T02:34:49Z

src/unk_020553DC.c

@@ -119,23 +119,23 @@ void sub_020553DC()
    return;
 }

-void sub_020553F0(FieldSystem *fieldSystem, u16 param1)
+void Sound_SetSpecialBGM(FieldSystem *fieldSystem, u16 param1)


suggestion: I think param1 here is a SDAT ID. For all the invocations of this function that pass a constant, you should be able to find a matching constant in the SDAT consts file. I would recommend just ctrl+Fing for the number in the generated file build/consts/sdat.h.

ravepossum · 2024-11-19T02:35:33Z

src/unk_020553DC.c

 {
-    u16 *v0 = sub_0203A748(SaveData_GetFieldOverworldState(fieldSystem->saveData));
+    u16 *v0 = FieldOverworldState_GetSpecialBGM(SaveData_GetFieldOverworldState(fieldSystem->saveData));

    *v0 = param1;
    return;


polish: This return can be removed.

ravepossum · 2024-11-19T02:42:32Z

src/unk_02054884.c

+    BOOL result;
+    TrainerInfo *trainerInfo = SaveData_GetTrainerInfo(param1);
+    Party *party = Party_GetFromSavedata(param1);
+    Pokemon *mon = Pokemon_New(32);


polish: The 32 here is a heap ID - HEAP_ID_FIELD_TASK.

ravepossum · 2024-11-19T02:44:56Z

src/unk_02054884.c

@@ -31,55 +31,55 @@ BOOL Pokemon_CanBattle(Pokemon *mon)

 BOOL sub_020548B0(int param0, SaveData *param1, u16 param2, u8 param3, u16 param4, int param5, int param6)


thought: I wonder if this and the function below are documented enough to name at this point. Looks like this particular one is only invoked in the script command ScrCmd_096, which DSPRE has named GivePokemon. The one below still has a bit more mystique in how it's invoked, so maybe just this one.

100% agree that we should aim to name scripting commands as we finish documenting functions which they invoke.

…sary return; and scoping, more renaming)

Viperio19 · 2024-11-19T10:18:44Z

A few thoughts on names, and a variety of small style stuff we're working on cleaning up. For the style matters, I only commented on the first occurrence of each that I noticed. Thanks for another contribution :)

Thanks for all the feedback, I've processed most it in the new commit (and also got carried away again with some new additions, whoops), let me know if there's anything else (or new) to improve.

…rom this PR with `task`

Viperio19 added 5 commits November 18, 2024 17:43

Merge branch 'pret'

a942454

Rename variable and function names related to overworld poison damage

9da11d6

Rename variable and function names from field_overworld_state.c

00890c6

Rename variables related to FieldOverworldState

0dea833

Rename variables in unk_02054884.c

37ba739

ravepossum reviewed Nov 19, 2024

View reviewed changes

Improving on previous commits (concise declarations, removing unneces…

e7e7f44

…sary return; and scoping, more renaming)

Remove more unnecessary scope blocks and replace taskMan in files f…

da15089

…rom this PR with `task`

ravepossum approved these changes Nov 23, 2024

View reviewed changes

ravepossum merged commit 73de609 into pret:main Nov 23, 2024
2 checks passed

github-actions bot pushed a commit that referenced this pull request Nov 23, 2024

Merge pull request #307 from Viperio19/overworld-renaming

3ada877

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Renaming variables and functions related to overworld poison damage and FieldOverworldState #307

Renaming variables and functions related to overworld poison damage and FieldOverworldState #307

Viperio19 commented Nov 18, 2024

ravepossum left a comment

ravepossum Nov 19, 2024

lhearachel Nov 19, 2024

Viperio19 Nov 20, 2024

ravepossum Nov 21, 2024

ravepossum Nov 19, 2024

ravepossum Nov 19, 2024

ravepossum Nov 19, 2024

Viperio19 Nov 19, 2024

ravepossum Nov 19, 2024

ravepossum Nov 21, 2024

Viperio19 Nov 21, 2024

ravepossum Nov 19, 2024

ravepossum Nov 19, 2024

ravepossum Nov 19, 2024

ravepossum Nov 19, 2024

ravepossum Nov 19, 2024

lhearachel Nov 19, 2024

Viperio19 commented Nov 19, 2024 •

edited

Loading

		@@ -426,10 +426,10 @@ static BOOL FieldMap_ChangeZone(FieldSystem *fieldSystem)
		}

		{

		@@ -31,55 +31,55 @@ BOOL Pokemon_CanBattle(Pokemon *mon)

		BOOL sub_020548B0(int param0, SaveData *param1, u16 param2, u8 param3, u16 param4, int param5, int param6)

Renaming variables and functions related to overworld poison damage and FieldOverworldState #307

Renaming variables and functions related to overworld poison damage and FieldOverworldState #307

Conversation

Viperio19 commented Nov 18, 2024

ravepossum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Viperio19 commented Nov 19, 2024 • edited Loading

Viperio19 commented Nov 19, 2024 •

edited

Loading