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

Remove gDecompressionBuffer #6029

Open
wants to merge 12 commits into
base: upcoming
Choose a base branch
from

Conversation

DizzyEggg
Copy link
Collaborator

@DizzyEggg DizzyEggg commented Jan 15, 2025

If someone's already started working on it, or would like to do it - let me know and I'll cancel the PR.

If not, I'll continue working on it.

@DizzyEggg
Copy link
Collaborator Author

It is done. gMemeBuffer has been utterly destroyed. The last remnants of it are hidden, buried deeply behind comment blocks.

EWRAM gains:

EWRAM:      243440 B       256 KB     92.86%
EWRAM:      227056 B       256 KB     86.61%

Because GF abused the buffer, this PR affects pretty much every area of the game(though some are more affected than others), so a full playthrough would be advised before merging.
I'm mostly worried about memory leaks, despite being extra cautious with allocating and freeing memory.
I'm also not sure about link features, including link contests, union room and mystery gift/event things. So I'd be grateful if someone checked these areas as well.

@DizzyEggg DizzyEggg marked this pull request as ready for review January 17, 2025 11:07
@DizzyEggg DizzyEggg changed the title Remove gDecompressionBuffer (WIP) Remove gDecompressionBuffer Jan 17, 2025
@hedara90
Copy link
Collaborator

hedara90 commented Jan 17, 2025

Trading: Works.
Link Battles: Works.
Link Contests: Works.
Mugshots: Works.
Berry Blending: Works, a lot in a row, doesn't seem to leak.

@AsparagusEduardo AsparagusEduardo added the type: big feature A feature with big diffs and / or high impact / subjectivity / pervasiveness label Jan 17, 2025
Copy link
Collaborator

@hedara90 hedara90 left a comment

Choose a reason for hiding this comment

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

Haven't had time to look through it properly.
Some initial things.

void LoadCompressedSpriteSheetOverrideBuffer(const struct CompressedSpriteSheet *src, void *buffer);
u32 LoadCompressedSpriteSheet(const struct CompressedSpriteSheet *src);
u32 LoadCompressedSpriteSheetByTemplate(const struct SpriteTemplate *template, s32 offset);
u32 LoadCompressedSpriteSheetOverrideBuffer(const struct CompressedSpriteSheet *src, void *buffer);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why does this return an u32?
The single call to this function doesn't store the result, and the final function called in the chain return an u16

include/decompress.h Show resolved Hide resolved
@@ -255,10 +255,14 @@ static void CB2_MysteryEventMenu(void)
case 11:
if (gReceivedRemoteLinkPlayers == 0)
{
// No clue what is going on here, and from where gDecompressionBuffer gets actually populated with mystery event script.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there even a way to test mystery gifts?

data->status = 0;
data->unusedBuffer = AllocZeroed(CLIENT_MAX_MSG_SIZE);
data->buffer = AllocZeroed(0x2000);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should it be a #defined value here that's more explanatory than 0x2000?

Copy link
Collaborator

@hedara90 hedara90 left a comment

Choose a reason for hiding this comment

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

Should allocations with 0xXXXX sizes have #defined sizes instead?

@@ -1612,6 +1628,9 @@ static void PrintPlayerNames(void)
}
}

// This is a small little addition, that basically speeds up the animation where all players' berries are thrown into the blender. Self-explanatory I hope!
#define BERRY_BLENDER_THROW_ALL_BERRIES_AT_ONCE TRUE
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd say that this should be moved to a config file, probably include/config/overworld.h.


u32 LoadCompressedSpriteSheetOverrideBuffer(const struct CompressedSpriteSheet *src, void *buffer)
{
LZ77UnCompWram(src->data, buffer);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Purely a personal thing, but I would prefer that no new uses of the bios LZ77 decompression functions are introduced, all calls should go through the wrapper functions (LZDecompressWram in this case).

{
buffer[(sBlockRecv[i].pos / 2) + j] = gRecvCmds[i][j + 1];
}
// Too large block was sent.
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this case is empty shouldn't the if statement be switched and the empty case be removed?

@@ -686,8 +685,8 @@ static void LoadLeftHeaderGfxForMenu(u32 menuGfxId)
tag = sMenuLeftHeaderSpriteSheets[menuGfxId].tag;
size = GetDecompressedDataSize(sMenuLeftHeaderSpriteSheets[menuGfxId].data);
LoadPalette(&gPokenavLeftHeader_Pal[tag * 16], OBJ_PLTT_ID(IndexOfSpritePaletteTag(1)), PLTT_SIZE_4BPP);
LZ77UnCompWram(sMenuLeftHeaderSpriteSheets[menuGfxId].data, gDecompressionBuffer);
RequestDma3Copy(gDecompressionBuffer, (void *)OBJ_VRAM0 + (GetSpriteTileStartByTag(2) * 32), size, 1);
LZ77UnCompWram(sMenuLeftHeaderSpriteSheets[menuGfxId].data, menu->leftHeaderMenuBuffer);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Prefer wrapper function over bios call

tag = sPokenavSubMenuLeftHeaderSpriteSheets[menuGfxId].tag;
size = GetDecompressedDataSize(sPokenavSubMenuLeftHeaderSpriteSheets[menuGfxId].data);
LoadPalette(&gPokenavLeftHeader_Pal[tag * 16], OBJ_PLTT_ID(IndexOfSpritePaletteTag(2)), PLTT_SIZE_4BPP);
LZ77UnCompWram(sPokenavSubMenuLeftHeaderSpriteSheets[menuGfxId].data, &gDecompressionBuffer[0x1000]);
RequestDma3Copy(&gDecompressionBuffer[0x1000], (void *)OBJ_VRAM0 + 0x800 + (GetSpriteTileStartByTag(2) * 32), size, 1);
LZ77UnCompWram(sPokenavSubMenuLeftHeaderSpriteSheets[menuGfxId].data, menu->leftHeaderSubMenuBuffer);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Prefer wrapper function over bios call

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: big feature A feature with big diffs and / or high impact / subjectivity / pervasiveness
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants