Progress in battle, ending, field, main, menu, and world#22
Progress in battle, ending, field, main, menu, and world#22armstrca wants to merge 3 commits intoXeeynamo:mainfrom
Conversation
Xeeynamo
left a comment
There was a problem hiding this comment.
I made a non-exhaustive review of the most obvious adjustments for common compiler optimization patterns. I couldn't try every single adjustment I suggested, but most of them work or require minor intervention.
I understand most of this pull request was done with the aid of a coding agent. They're great at bootstrapping small functions, but minor adjustments still need to be done manually function by function, or with the aid of a bunch of skills that are aware of common patterns. Loads of temporary variables or casts can be removed, and some struct fields can be updated to reflect the true nature of the field's underlying type.
In the future, I suggest to have leaner pull requests. They have an smaller scope, they're easier to review for me and easier to iterate to for merging.
| INCLUDE_ASM("asm/us/battle/nonmatchings/battle", func_800A555C); | ||
|
|
||
| extern s32 D_800F3A40; | ||
| extern s32 D_800F3A42; |
There was a problem hiding this comment.
Please have these moved in battle_private.h
| temp_v1 = arg0 * 0x44; | ||
| *(&D_800F5BB8->unkE + temp_v1) = *(&D_800F5BB8->unkE + temp_v1) | 9; |
There was a problem hiding this comment.
| temp_v1 = arg0 * 0x44; | |
| *(&D_800F5BB8->unkE + temp_v1) = *(&D_800F5BB8->unkE + temp_v1) | 9; | |
| D_800F5BB8[arg0].unkE |= 9; |
I believe this array access could be simplified, and s32 temp_v1; removed
| void func_800A7090(s32 arg0) { | ||
| s32 temp_v1; | ||
|
|
||
| temp_v1 = arg0 * 0x44; | ||
| *(&D_800F5BB8->unk29 + temp_v1) = *(&D_800F5BB8->unk29 + temp_v1) | 0x40; | ||
| } |
There was a problem hiding this comment.
| void func_800A7090(s32 arg0) { | |
| s32 temp_v1; | |
| temp_v1 = arg0 * 0x44; | |
| *(&D_800F5BB8->unk29 + temp_v1) = *(&D_800F5BB8->unk29 + temp_v1) | 0x40; | |
| } | |
| void func_800A7090(s32 arg0) { | |
| D_800F5BB8[arg0].unk29 |= 0x40; | |
| } |
This should match. Most of the decompilation stuff is not only about getting a 1:1 match, but also trying to understand the original dev intention and how they could've written the code.
| D_80063014->unk8C = 0xFF; | ||
| D_80063014->unk40 = 0xB0; | ||
| D_80063014->unk80 |= 1; | ||
| D_80063014->unk3C = (s32)D_80063014->unk3C >> 1; |
There was a problem hiding this comment.
| D_80063014->unk3C = (s32)D_80063014->unk3C >> 1; | |
| D_80063014->unk3C = D_80063014->unk3C >> 1; |
(s32) can be deleted. Most casts can actually be removed to make the code more readable.
| D_80063014->unk3C = (s32)D_80063014->unk3C >> 1; | |
| D_80063014->unk3C >>= 1; |
the expression could even be further simplified
| void func_800ADE84(void) { | ||
| s32 var_v1; | ||
|
|
||
| var_v1 = D_80063014->unk48 * (0x200 - D_80063014->unk210); | ||
| if (var_v1 < 0) { | ||
| var_v1 += 0x1F; | ||
| } | ||
| D_80063014->unk214 = func_800AD8DC(var_v1 >> 5); |
There was a problem hiding this comment.
| void func_800ADE84(void) { | |
| s32 var_v1; | |
| var_v1 = D_80063014->unk48 * (0x200 - D_80063014->unk210); | |
| if (var_v1 < 0) { | |
| var_v1 += 0x1F; | |
| } | |
| D_80063014->unk214 = func_800AD8DC(var_v1 >> 5); | |
| void func_800ADE84(void) { | |
| s32 var_v1; | |
| var_v1 = D_80063014->unk48 * (0x200 - D_80063014->unk210); | |
| D_80063014->unk214 = func_800AD8DC(var_v1 / 32); |
When you see expressions such as this, it usually suggests it's a compiler optimization for signed division. I am not sure if you even need var_v1
| if (var_s0 != NULL) { | ||
| do { | ||
| D_8010AD3C = var_s0; | ||
| D_8010ADE4 = var_s0; | ||
| func_800AD63C(var_s0); | ||
| var_s0 = (Unk8010AD3C*)var_s0->unk0; | ||
| } while (var_s0 != NULL); | ||
| } |
There was a problem hiding this comment.
| if (var_s0 != NULL) { | |
| do { | |
| D_8010AD3C = var_s0; | |
| D_8010ADE4 = var_s0; | |
| func_800AD63C(var_s0); | |
| var_s0 = (Unk8010AD3C*)var_s0->unk0; | |
| } while (var_s0 != NULL); | |
| } | |
| while (var_s0 != NULL) { | |
| D_8010AD3C = var_s0; | |
| D_8010ADE4 = var_s0; | |
| func_800AD63C(var_s0); | |
| var_s0 = (Unk8010AD3C*)var_s0->unk0; | |
| } |
When you see a if, do, while it can almost always be simplified as a simple while loop
| s32 func_800B2FA4(void) { | ||
| if (D_8010CA8C != 2) { | ||
| return -(D_8010CA8C == 3); | ||
| } | ||
| return 1; | ||
| } |
There was a problem hiding this comment.
| s32 func_800B2FA4(void) { | |
| if (D_8010CA8C != 2) { | |
| return -(D_8010CA8C == 3); | |
| } | |
| return 1; | |
| } | |
| s32 func_800B2FA4(void) { | |
| if (D_8010CA8C != 2) { | |
| if (D_8010CA8C != 3) { | |
| return 0; | |
| } | |
| return -1; | |
| } | |
| return 1; | |
| } |
I think this is easier to read
| block_4: | ||
| func_8002DA7C(); | ||
| } | ||
| } |
There was a problem hiding this comment.
| } | |
| void func_800B65E0(s32 arg0) { | |
| if (D_8010CB20 < arg0) { | |
| *D_8009A000 = 0x20; | |
| D_8010CB20 = arg0; | |
| *D_8009A004 = 0x40; | |
| *D_8009A008 = arg0; | |
| func_8002DA7C(); | |
| } else if (arg0 == -D_8010CB20) { | |
| D_8010CB20 = 0; | |
| *D_8009A000 = 0xF1; | |
| func_8002DA7C(); | |
| *D_8009A000 = 0xBC; | |
| *D_8009A004 = 0; | |
| func_8002DA7C(); | |
| } | |
| } |
it's better to avoid the use of goto as much as possible
| void func_800B6B28(s16); // extern | ||
|
|
||
| void func_800B7104(s16 arg0) { | ||
| D_80115A58 = (s32)arg0; |
There was a problem hiding this comment.
| D_80115A58 = (s32)arg0; | |
| D_80115A58 = arg0; |
| void func_800BB9A0(u8 arg0) { | ||
| s8* temp_v1; | ||
|
|
||
| temp_v1 = D_801163E8; | ||
| if ((u32)temp_v1 < (u32)&D_801163E8) { | ||
| D_801163E8 = temp_v1 + 1; | ||
| *temp_v1 = (s8)arg0; | ||
| } | ||
| } |
There was a problem hiding this comment.
| void func_800BB9A0(u8 arg0) { | |
| s8* temp_v1; | |
| temp_v1 = D_801163E8; | |
| if ((u32)temp_v1 < (u32)&D_801163E8) { | |
| D_801163E8 = temp_v1 + 1; | |
| *temp_v1 = (s8)arg0; | |
| } | |
| } | |
| void func_800BB9A0(u8 arg0) { | |
| if (D_801163E8 < &D_801163E8) { | |
| *D_801163E8++ = arg0; | |
| } | |
| } |
can be simplified
This is successfully building (with some warnings) on my local machine, at least.
Let me know if there are any conventions for the codebase or PRs you'd like to me to adjust for.