Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Battle Overlay Progress + General Documentation #152

Merged
merged 14 commits into from
Jun 10, 2023

Conversation

adrienntindall
Copy link
Collaborator

Battle overlays won't be done for awhile, but since I'm changing (aka documenting) a lot of files as I go along, I figure I should keep a PR open to be merged from time to time so I don't get too out of sync with the master branch. Since some of the battle overlay files are 20k+ lines long and can't be split any further, I'm ok merging these while incomplete as long as the other reviewers are also ok with that.

Copy link
Member

@red031000 red031000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like int -> s32 but I feel like there's some fuckery going on there and it may not match, need to figure that out

include/battle.h Outdated Show resolved Hide resolved
include/battle.h Outdated
//This is information used for selecting a target on the bottom screen in a double battle
typedef struct TargetPokemon {
u8 selectedMon;
u8 gender : 2,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idk if it's just me but imo every member of the bitfield should have an explicit type attached, feels weird to see it without

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it doesn't make a difference compile-wise, the only reason I have it like that is to make it easier to see what's in the same 8 bit area in the bitfield. Totally willing to change it though

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please do change this

include/dex_mon_measures.h Outdated Show resolved Hide resolved
out Outdated Show resolved Hide resolved
src/battle_command.c Outdated Show resolved Hide resolved
src/overlay_12_0224E4FC.c Show resolved Hide resolved
src/overlay_12_0224E4FC.c Show resolved Hide resolved
src/overlay_12_0224E4FC.c Show resolved Hide resolved
src/overlay_12_0224E4FC.c Show resolved Hide resolved
src/overlay_12_0224E4FC.c Outdated Show resolved Hide resolved
Copy link
Member

@red031000 red031000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

turns out that a moving train while you're trying to concentrate on which stop to get off may not be the best place to review code, but eh

@@ -126,34 +127,34 @@ typedef struct UnkBtlCtxSub_76 {
typedef struct UnkBattlemonSub {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this struct be named? it seems to be about moves that affect the battle status

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm very open to suggestions if you have any

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BattleMonMoveStatus maybe?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ehhh it also includes the flags for custap and quick claw triggers, but those might arguably be related to moves? it's less move statuses though and more just flags, counts, etc

include/battle.h Outdated
//This is information used for selecting a target on the bottom screen in a double battle
typedef struct TargetPokemon {
u8 selectedMon;
u8 gender : 2,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please do change this

include/battle_input.h Show resolved Hide resolved
include/dex_mon_measures.h Outdated Show resolved Hide resolved
@@ -3,7 +3,7 @@

#include "heap.h"

struct ZknHeightWeight {
struct PokedexData {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typedef struct

src/battle_command.c Outdated Show resolved Hide resolved
mon->unk88.bindingMove = *data16;
break;
case BMON_DATA_HELD_ITEM_RESTORE_HP:
mon->unk88.unk30 = *data32;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this be named?

if (ctx->unk_2174 & (1 << 29)) {
*out = ov12_02258348(ctx, 2, ctx->unk_2174);
ctx->unk_2174 = 0;
if (!(ctx->moveStatusFlag & 0x801FDA49)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems like a completely arbitrary number. any chance for constant or explanation comment?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tbh I have no idea what the number means, the original has it as "WAZA_STATUS_FLAG_NO_OUT," so my best guess is that it's probably a concatenation of all the move statuses that can return something? regardless, I'd rather wait until I can define it as a concatenation once I get to the move status constants

If I missed a bitfield declaration let me know
@red031000 red031000 merged commit 790bcdb into pret:master Jun 10, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants