-
Notifications
You must be signed in to change notification settings - Fork 92
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
Summary screen part 3 (misc. clean-up) #380
base: main
Are you sure you want to change the base?
Conversation
@@ -30,6 +30,7 @@ enum { | |||
#define MAX_POKEMON_SHEEN 255 | |||
#define MAX_POKEMON_MARKINGS 6 | |||
#define MAX_POKEMON_LEVEL 100 | |||
#define MAX_POKEMON_NAME_LEN 12 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: How does this differ from MON_NAME_LEN
in constants/string.h
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because I missed that it exists, whoops. I was also clearly tripping about the name length.
It's odd that they allocate space for 12 characters instead of 10+1 🤔 I guess we can write it as MON_NAME_LEN + 2
even if it's a little unusual.
@@ -45,8 +45,8 @@ void *sub_0203D578(int param0, FieldSystem *fieldSystem, int param2, int param3, | |||
void *sub_0203D5C8(int param0, FieldSystem *fieldSystem, int param2); | |||
void *sub_0203D644(FieldSystem *fieldSystem, int param1); | |||
PokemonSummary *sub_0203D670(FieldSystem *fieldSystem, int param1, int param2); | |||
void *sub_0203D6E4(int param0, FieldSystem *fieldSystem, u8 param2); | |||
int sub_0203D750(void *param0); | |||
void *FieldSystem_OpenSummaryScreenSelectMove(int heapID, FieldSystem *fieldSystem, u8 partyIndex); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
polish: enum HeapId
case 1: | ||
PokemonSummary *summary = Heap_AllocFromHeap(HEAP_ID_FIELDMAP, sizeof(PokemonSummary)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: Just something to be aware of: following a label with a declaration is a C23 extension.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm do you think that warrants changing? Would it be troublesome to rely on that while using mwcc/skrew? I'm not too familiar with how enforcement works, since the compiler doesn't seem to be griping about it.
case 4: | ||
PokemonSummary *v4 = Heap_AllocFromHeap(HEAP_ID_FIELDMAP, sizeof(PokemonSummary)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: This is one reason why the above is a bit awkward. 🙂
} | ||
|
||
return TRUE; | ||
return !FieldSystem_IsRunningApplication(ctx->fieldSystem); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
praise: 👏
if (*destVar == LEARNED_MOVES_MAX) { | ||
*destVar = 0xFF; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: I expect we'll use a similar construct a lot; maybe this is worth a constant that we can share with field scripts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that the generic scrcmd const file bit the dust in the switch to metang - do you think it would merit making a new file for this? This one's kinda awkward since it's just the one value, not an enum or anything.
[SUMMARY_SPRITE_POKERUS_ICON] = { 0x2A, 0x4C, 0x30, 0x0, 0x0, 0x0, 0x1, NNS_G2D_VRAM_TYPE_2DMAIN, 0x0, 0x0, 0x0, 0x0 } | ||
#define TAP_CIRCLE_BASE_Y 192 | ||
|
||
static const SpriteTemplateFromResourceHeader sSummaryScreenSpriteTemplates[] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
praise: This looks so clean now! 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And it'll be even better when we get the resdats unpacked 👀
Apologies in advance for how scattered this PR is - it got a little kitchen sink-y and I wanted to go ahead and get it created before it grew even bigger.
Overview:
There's still a fair bit left to do with regard to fully documenting the transition from field to summary screen in all scenarios.