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

[Resources] Clean up old hacks and rename #2546

Merged
merged 1 commit into from
Feb 27, 2023

Conversation

dcvz
Copy link
Contributor

@dcvz dcvz commented Feb 27, 2023

@@ -1210,10 +1210,8 @@ void BossGanon_SetupTowerCutscene(BossGanon* this, PlayState* play) {

void BossGanon_ShatterWindows(u8 windowShatterState) {
s16 i;
// Temporary solution: using LoadTexOrDList to ensure we actually have the texture available
// based on mq/nonmq. This will be handled properly with LUS 1.0
u8* tex1 = ResourceMgr_LoadTexOrDListByName(SEGMENTED_TO_VIRTUAL(ganon_boss_sceneTex_006C18));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We already know we're loading textures, this method was used because there was no other method handling the paths (or they didn't know about it)

// Entering the King Dodongo boss battle was crashing when using only an mq otr
// because it was trying to load a texture from a non-mq path
// HACK: GetResourceDataByName doesn't account for mq vs non-mq paths, LoadTexOrDListByName does.
arg0 = ResourceMgr_LoadTexOrDListByName(arg0);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We already know we're loading textures, this method was used because there was no other method handling the paths (or they didn't know about it)

Copy link
Contributor

@briaguya-ai briaguya-ai left a comment

Choose a reason for hiding this comment

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

This all looks good! The comments I left in here were just me writing down my thought process as I ensured none of the changes were modifying functionality.

Great cleanup! Let's :shipit:

Comment on lines +1213 to +1214
u8* tex1 = GetResourceDataByNameHandlingMQ(SEGMENTED_TO_VIRTUAL(ganon_boss_sceneTex_006C18));
u8* tex2 = GetResourceDataByNameHandlingMQ(SEGMENTED_TO_VIRTUAL(ganon_boss_sceneTex_007418));
Copy link
Contributor

Choose a reason for hiding this comment

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

just kind of thinking out loud in comments here, we know this works because since these are textures, they were always going into the final else in ResourceMgr_LoadTexOrDListByName

directly calling GetResourceDataByNameHandlingMQ makes sense here

// because it was trying to load a texture from a non-mq path
// HACK: GetResourceDataByName doesn't account for mq vs non-mq paths, LoadTexOrDListByName does.
arg0 = ResourceMgr_LoadTexOrDListByName(arg0);
arg0 = GetResourceDataByNameHandlingMQ(arg0);
Copy link
Contributor

Choose a reason for hiding this comment

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

assuming every time this is called arg0 is referring to a texture, and never a dlist or array, then this is not changing behavior

it appears to only ever be called by BossDodongo_Update, and arg0 is gDodongosCavernBossLavaFloorTex, so this makes sense

Comment on lines +887 to +888
extern "C" char* GetResourceDataByNameHandlingMQ(const char* path) {
auto res = GetResourceByNameHandlingMQ(path);
Copy link
Contributor

Choose a reason for hiding this comment

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

got a little worried for a second before i noticed these didn't have the same name
GetResourceByNameHandlingMQ
vs
GetResourceDataByNameHandlingMQ

this makes sense

Comment on lines -1189 to +1192
// Handle mq vs nonmq for cutscenes due to scene/ paths
auto res = ResourceMgr_LoadResource(path);

if (res == nullptr) {
return nullptr;
}

return (s32*)res->GetPointer();
return (s32*)GetResourceDataByNameHandlingMQ(path);
Copy link
Contributor

Choose a reason for hiding this comment

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

everything here is now happening in GetResourceDataByNameHandlingMQ, this makes sense

@dcvz dcvz merged commit 2cb4a6e into HarbourMasters:develop Feb 27, 2023
@dcvz dcvz deleted the chore/resource-mq-cleanup branch February 27, 2023 01:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants