Decomp - #43
Conversation
ser-pounce
left a comment
ser-pounce
left a comment
There was a problem hiding this comment.
Hello and thanks for the PR!
Broadly speaking, there's a lot in here that's absolutely fine and I'd have no trouble merging, but there's also a lot of oddities for which I've provided feedback.
The build breaks if I check it out locally, but this seems only due to duplicate declarations such as vs_battle_getCameraPosition which I've pointed out. It's actually a weakness in one of the python dev scripts but the dupes aren't necessary anyway.
For any subsequent PRs, I'd appreciate if they were a little more focused. It's fine to include multiple unrelated small functions, or a few large ones if they are clearly working on the same data, but it's quite hard to stay focused when reviewing this much.
A note on AI: it's clear it can be a great help, as a lot in here is perfectly serviceable as mentioned. That said I try to avoid introducing any broad claims it will often make in comments however, unless there is very good evidence for them.
Finally, and I really need to add this to the readme so you're not at fault here, I have to stress that my goals for this project are beyond just matching the code, for the most part it needs to make sense to a human being as well. The comments I made about bitfields are a good example, it's highly likely these can be improved and still yield matches if the correct bitfields are referenced. If matching code were the only criteria then I would just as well do a static recompilation and be done with it.
If you find anything here disagreeable I understand completely, I don't expect everyone to share my interests, and if you'd rather go in a different direction then by all means maintain a separate fork (I'd probably end up referring to and stealing from it anyway). Either way good luck on your own project, it looks very promising!
|
Absolutely hear you, this was the first initial PR, and I was hesitant to split it up into multiple with us not having spoken before, future ones will be more focused. On AI, I agree, since I posted this, I was thinking about it and decided that I want to actually know this vs "just get it working", So I'm going to take the comments as guidance. I'm thoroughly impressed at the work you've done since last year. This started as an effort for me to create a native port of VS, but I figured that instead of jumping off from your work, I may as well try to contribute. I'll take the comments into consideration and adjust! |
|
And VSCode didn't push any of my comments from their extension... |
ser-pounce
left a comment
ser-pounce
left a comment
There was a problem hiding this comment.
Please rebuild and check the build warnings and errors.
|
Rookie mistake, resolved build errors. |
ser-pounce
left a comment
ser-pounce
left a comment
There was a problem hiding this comment.
What is this in aid of?
Comment not addressed.
See the comment where this function is used, __asm__ hacks are invariably the wrong solution.
| INCLUDE_ASM("build/src/BATTLE/BATTLE.PRG/nonmatchings/3A1A0", func_800A48CC); | ||
|
|
||
| INCLUDE_ASM("build/src/BATTLE/BATTLE.PRG/nonmatchings/3A1A0", func_800A4A24); | ||
| u_int func_800A4A24(int arg0) |
There was a problem hiding this comment.
Function looks like it returns a func_8008D2C0_t*? Could also benefit from a //BUG comment in that case because it's returning a stack address. The end would change to return NULL; in this case.
|
|
||
| void func_800ACF54(D_800F45E0_t*); | ||
| INCLUDE_ASM("build/src/BATTLE/BATTLE.PRG/nonmatchings/40564", func_800ACF54); | ||
| void func_800ACF54(D_800F45E0_t* arg0) |
There was a problem hiding this comment.
Looks like the wrong parameter type, following the call chain upwards it seems to be called with a D_800F4538_t*, which woudl mean what is arg0->unk60 and arg0->unk60 below are actually structs starting at D_800F4538_unk0.currentTileX and D_800F4538_unk0.lastTouchedTileX, so those would need to be factored out into a separate struct to line up with the struct copy here.
| { | ||
| int first = func_800D5170_int(arg0); | ||
| int second = func_800D5170_int(arg0); | ||
| int third = func_800D5170_int(arg0); |
There was a problem hiding this comment.
These three work fine as
| int third = func_800D5170_int(arg0); | |
| char first = func_800D5170(arg0); |
| INCLUDE_ASM("build/src/BATTLE/BATTLE.PRG/nonmatchings/5BF94", func_800C5798); | ||
|
|
||
| INCLUDE_ASM("build/src/BATTLE/BATTLE.PRG/nonmatchings/5BF94", func_800C58A4); | ||
| /* Name of the lowest status effect set in arg0, or the empty entry if none. */ |
There was a problem hiding this comment.
Please rebase, this has been merged from another PR.
| INCLUDE_ASM("build/src/BATTLE/BATTLE.PRG/nonmatchings/5BF94", func_800C58F8); | ||
|
|
||
| void func_800C64D0(u_long* arg0, int* arg1) | ||
| void func_800C64D0(u_long* arg0, u_long* arg1) |
There was a problem hiding this comment.
Please rebase, this is already merged.
|
|
||
| INCLUDE_ASM("build/src/BATTLE/BATTLE.PRG/nonmatchings/5BF94", func_800CFC8C); | ||
| /* vs_battle_lerp with the cosine easing of func_800D18B4, inlined. */ | ||
| int func_800CFC8C(int arg0, int arg1, int arg2, int arg3) |
There was a problem hiding this comment.
Please rebase, this has been merged.
|
Sorry messed up my last comment, I've left some more feedback. I'm a little concerned that some of my original comments don't seem to have been addressed, here's another example |
|
I might close this PR, I'm working through WSL and git in VSCode isn't really working with me, and it was moving the comments around. |
|
Understood, if you'd like to spit things up into smaller PRs it might be easier to manage. With something this large by the time I've been able to review everything the repo starts to drift. |
2 participants
No description provided.