MapNamePopUp Documentation #352
Conversation
I have literally no idea what I'm doing
…map_name_popup
There was a problem hiding this comment.
This looks great overall! Thanks for your contribution. I have some feedback and collaborative thoughts scattered throughout.
You'll also want to do a pull from main before continuing, as some generated headers are being shifted around. I tried to call those out where I saw them.
src/map_header_util.c
Outdated
| u32 MapHeader_GetStringWidth(MessageLoader *msgLoader, u32 entryID, Strbuf *strbuf) | ||
| { | ||
| u32 width; | ||
|
|
||
| MessageLoader_GetStrbuf(msgLoader, entryID, strbuf); | ||
| width = Font_CalcStrbufWidth(FONT_SYSTEM, strbuf, 0); | ||
|
|
||
| return width; | ||
| } | ||
|
|
||
| void sub_02071D10(int headerID, u32 heapID, Strbuf *strbuf) | ||
| { | ||
| u32 mapLabelTextID; | ||
| MessageLoader *msgLoader; | ||
|
|
||
| msgLoader = MessageLoader_Init(MESSAGE_LOADER_NARC_HANDLE, NARC_INDEX_MSGDATA__PL_MSG, message_bank_location_names, heapID); | ||
| mapLabelTextID = MapHeader_GetMapLabelTextID(headerID); | ||
|
|
||
| MapHeader_GetStringWidth(msgLoader, mapLabelTextID, strbuf); // It should be MessageLoader_GetStrbuf() | ||
| MessageLoader_Free(msgLoader); | ||
| } |
There was a problem hiding this comment.
The first of these two functions seems to be more generic than just MapHeader; rather, it seems like it's used to pull any given string from any given text bank, then return its width. I would suggest naming these functions as:
MapHeader_GetStringWidth->MapHeader_LoadStringsub_02071D10->MapHeader_LoadName
src/map_header_util.c
Outdated
| return width; | ||
| } | ||
|
|
||
| void sub_02071D10(int headerID, u32 heapID, Strbuf *strbuf) |
There was a problem hiding this comment.
polish: Try replacing headerID here with enum MapHeader from generated/map_headers.h after merging from main.
src/map_header_util.c
Outdated
| #include <string.h> | ||
|
|
||
| #include "field/field_system_sub2_t.h" | ||
| #include "text/pl_msg.naix" |
There was a problem hiding this comment.
chore: Replace this with generated/text_banks.h after merging from main.
src/map_header_util.c
Outdated
|
|
||
| u32 MapHeader_GetStringWidth(MessageLoader *msgLoader, u32 entryID, Strbuf *strbuf) | ||
| { | ||
| u32 width; |
There was a problem hiding this comment.
polish: Try to declare + define variables inline where possible, e.g., prefer
u32 width = Font_CalcStrbufWidth(FONT_SYSTEM, strbuf, 0);vs.
u32 width;
// other code
width = Font_CalcStrbufWidth(FONT_SYSTEM, strbuf, 0);
src/overlay005/map_name_popup.c
Outdated
|
|
||
| static void MapNamePopUp_LoadPalette(void *src, u16 size, u16 offset) | ||
| { | ||
| DC_FlushRange((void *)src, size * 32); |
There was a problem hiding this comment.
question: Is this void * cast necessary?
There was a problem hiding this comment.
Apparently not, it gives a match, the same with the const void *on GX_LoadBGPltt
There was a problem hiding this comment.
That's fairly typical. 😁 An explicit void * cast is almost never necessary, and the compiler will often do it for you.
| // I'm confused by this | ||
| narcMemberIdx = mapPopUp->windowID * 2; |
There was a problem hiding this comment.
Looking at the structure of the NARC itself (arc/area_win_gra.narc), it looks like the window ID is just which window type is being used for this pop-up. There are 2 files for each pop-up type: a set of tiles, and a palette. So, multiplying the type ID by 2 yields the member index of the tile-data.
src/overlay005/map_name_popup.c
Outdated
| mapPopUp = Heap_AllocFromHeap(4, sizeof(MapNamePopUp)); | ||
| mapPopUp->strbuf = Strbuf_Init(22, 4); |
There was a problem hiding this comment.
polish: Replace these 4s with HEAP_ID_FIELD from constants/heap.h
src/overlay005/map_name_popup.c
Outdated
| mapPopUp = NULL; | ||
| } | ||
|
|
||
| void MapNamePop_Show(MapNamePopUp *mapPopUp, s32 mapLabelTextID, s32 mapLabelWindowID) |
There was a problem hiding this comment.
polish:
| void MapNamePop_Show(MapNamePopUp *mapPopUp, s32 mapLabelTextID, s32 mapLabelWindowID) | |
| void MapNamePopUp_Show(MapNamePopUp *mapPopUp, s32 mapLabelTextID, s32 mapLabelWindowID) |
| u8 v6; | ||
| u8 v7; | ||
|
|
||
| v3 = (strWidth + 8) / 8 * 8; |
There was a problem hiding this comment.
This should be the total number of pixels used by the string. 8 here is the width of a tile in pixels.
src/map_header_util.c
Outdated
| u32 mapLabelTextID; | ||
| MessageLoader *msgLoader; | ||
|
|
||
| msgLoader = MessageLoader_Init(MESSAGE_LOADER_NARC_HANDLE, NARC_INDEX_MSGDATA__PL_MSG, message_bank_location_names, heapID); |
There was a problem hiding this comment.
After you merge from main, message_bank_location_names will become TEXT_BANK_LOCATION_NAMES.
…MapHeader_LoadName
…map_name_popup
estellarc
force-pushed
the
doc/map_name_popup
branch
from
b304633 to
d2bbaa6
Compare
I'm unsure how I should name
sub_02071D10and about the most variables inMapNamePopUp_DrawWindowFrame#325