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

Pause Warp Enhancement #3223

Merged
merged 22 commits into from
Feb 16, 2024
Merged

Conversation

mckinlee
Copy link
Contributor

@mckinlee mckinlee commented Sep 23, 2023

Original PR: #3212

This enhancement allows players to initiate warp songs directly from the pause menu, specifically from the quest status subscreen.

Changes:
Added PauseWarpState struct to manage the state of the PauseWarp feature.
Introduced IsPauseWarpEnabled() function to check if the PauseWarp feature is enabled.
Added InitiateWarp() function to initiate the warp process.
Added HandleWarpConfirmation() function to manage the warp song confirmation.
Implemented PauseWarp_Main() function to handle the initiation of the warp process from the pause menu.
Implemented PauseWarp_Idle() function to manage the state and handle edge cases.
Added cooldowns and state transitions to ensure proper functionality and user experience.

Edit:

I have rewritten this enhancement from the ground up. Here are the changes:

  1. Cleaned up the code to make it easier to read and mess with.
  2. Added lists for song messages, fanfares, and entrance locations, so it's clear what's connected to what.
  3. Made starting a warp simpler with ActivateWarp, setting up the song and effects in one go.
  4. PauseWarp_Execute now checks if we're ready to warp and handles it differently if you're playing with randomizer settings.
  5. Picking a song to warp with from the pause menu is now straightforward with PauseWarp_HandleSelection.

Build Artifacts

This commit introduces the PauseWarp mod, a feature that allows players to warp to different locations in the game directly from the pause menu.

- Add PauseWarpState structure to manage flags and cooldowns for the pause warp feature.
- Implement IsStateValid function for state validation.
- Implement ResetStateFlags function to reset all state flags to default values.
- Add InitiateWarp function to handle the initiation of warp sequences.
- Implement HandleWarpConfirmation function to confirm and execute warp actions.
- Implement HandleCooldowns function to manage various cooldown timers.
- Add PauseWarp_Main function as the main logic, called every frame to handle pause warp functionality.
- Map warp song messages to in-game text messages.
-Now if you do not have a warp song you won't be able to select the empty slot and still teleport.
-When selecting a warp song the audio for the applicable warp song will now play for a extra vanilla feel.
-Changed the stateFlag1 because previously it just disabled input allowing enemies to harm you. Now that won't happen because the game is put into a cutscene state.
-A new hook was created 'OnPauseMenu' so now PauseWarp_Main is only called when the pause menu is open
-Moved pauswarp.c to the Enhancements folder
-Removed from graph.c

PR Change:
Changing to the main branch instead of sulu
-Introduced new function 'PauseWarp_Idle' now that 'PauseWarp_Main' is no longer called every frame
-Added C wrapper to access 'GameInteractor::IsSaveLoaded' and scrapped the 'IsStateValid' function
-Added 'PauseWarp_Idle' to the the 'RegisterPauseWarp' function
-Refactored the code some
-Added a missing header that was causing a compile issue for linux
-Hopefully, it won't crash
@mckinlee mckinlee mentioned this pull request Sep 23, 2023
-Now link won't get soft locked when warping to the same location twice
@CasperMcFadden95
Copy link

CasperMcFadden95 commented Sep 25, 2023

Awesome work!

Can you also add this functionality to the Map? There's a decide button which is disabled but that could be used to warp somwhere.

ootmap

Thanks

@PurpleHato
Copy link
Contributor

PurpleHato commented Sep 25, 2023

Awesome work!

Can you also add this functionality to the Map? There's a decide button which is disabled but that could be used to warp somwhere.

ootmap

Thanks

The point of this enhancement is to have warp songs (where warp pedestals are) being easy usable;
The map contains points that doesn't have pedestal, hence why it's not a good idea to do that an d for multiple aspects

  • Zone names become incoherant (since only 2 have the name of the zones used by warp songs, the rests are literally unrelated to warps and break logic
  • Do you warp then on the zone name like Lon Lon Ranch directly? If yes, you need to add that option to use that logic in randomizer to not break it ?
  • Do you create custom points, with custom texture when it's enabled and basically redo a complete new map ?

That's why it's not viable to do as a QoL but rather a code mod made by the community in my opinion

@Malkierian
Copy link
Contributor

I don't like that idea for several reasons. First, there are more locations there than warp songs, and differentiating between locations you can and can't warp to would be awkward. Second, warp shuffle would make that even more difficult because then you'd have to know where the warp song was taking you and then apply that to the location on the map. At the very least, it feels like a different PR to me, if not mod territory, depending on how you wanted to apply it.

@CasperMcFadden95
Copy link

Thanks I understand. Can you also make this mod play the non-warp songs? I find it annoying that I have to press the notes. Thanks

@mckinlee
Copy link
Contributor Author

Thanks I understand. Can you also make this mod play the non-warp songs? I find it annoying that I have to press the notes. Thanks

Thanks you for your comments! I appreciate it.

That might be something I would consider working on in the future.. maybe, but it would have to be a totally different PR and probably something more akin to the Skip Scarecrow Song enhancement. The groundwork is set with this PR, so it shouldn't be to bad for someone to come along and do that if they wished. I think I've done all I want to do with this current feature and have moved on to another project. I'm trying to implement a banking system similar to the one in MM.

@mckinlee
Copy link
Contributor Author

After merging to latest dev build, game now soft locks unless the player has a Ocarina in their inventory. Will work on a fix when I can.

-Added more checks to ensure vanilla behavior when a Ocarina is not in the players inventory.
@mckinlee
Copy link
Contributor Author

Bug fixed. This is merge ready again.

@mckinlee
Copy link
Contributor Author

Updated to MacReady Bravo (8.0.1)

Comment on lines 48 to 51
extern "C" bool IsSaveLoadedWrapper() {
GameInteractor* gameInteractor = GameInteractor::Instance;
return gameInteractor->IsSaveLoaded();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

i think following the naming patterns used for other extern c GI things might make sense here

#ifdef __cplusplus
extern "C" {
#endif
// MARK: - Gameplay
void GameInteractor_ExecuteOnLoadGame(int32_t fileNum);
void GameInteractor_ExecuteOnExitGame(int32_t fileNum);

void GameInteractor_ExecuteOnExitGame(int32_t fileNum) {
GameInteractor::Instance->ExecuteHooks<GameInteractor::OnExitGame>(fileNum);
}

so something like

Suggested change
extern "C" bool IsSaveLoadedWrapper() {
GameInteractor* gameInteractor = GameInteractor::Instance;
return gameInteractor->IsSaveLoaded();
}
extern "C" bool GameInteractor_IsSaveLoaded() {
return GameInteractor::Instance->IsSaveLoaded();
}

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.

commented about a name change but i don't see that as blocking

@mckinlee
Copy link
Contributor Author

commented about a name change but i don't see that as blocking

Again, thanks for reviewing! Like I mentioned in discord, this was a utter mess due to my inexperience with coding at the beginning.. Given that, I have committed a rewrite that is a lot cleaner, works with rando now, and removes that IsSaveLoadedWrapper. It was much better to just use GameInteractor::IsSaveLoaded directly in mods.cpp. This should be re-reviewed whenever yall get a chance.

Copy link
Contributor

@garrettjoecox garrettjoecox left a comment

Choose a reason for hiding this comment

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

This is borderline mod territory, but the fact it's well isolated and the only touch point in source is a generic hook in my mind makes it fine.

One comment/question on the new hook

@@ -221,6 +221,7 @@ class GameInteractor {

DEFINE_HOOK(OnFileDropped, void(std::string filePath));
DEFINE_HOOK(OnAssetAltChange, void());
DEFINE_HOOK(OnPauseMenu, void());
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this maybe makes more sense as OnKaleidoUpdate

I also assume that this means OnGameFrameUpdate does not run with kaleido open? So you couldn't just also use OnGameFrameUpdate and instead just check some sort of pauseCtx value?

Copy link
Contributor Author

@mckinlee mckinlee Feb 12, 2024

Choose a reason for hiding this comment

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

Nah, OnGameFrameUpdate does run with Kaleido open. You're right, I could of just checked if pauseCtx->pageIndex == PAUSE_QUEST. However, on my original PR for this @jbodner09 liked the idea of having a hook specific to the pause menu for custom item mods. That comment is here. That's why I kept it in, but it's up to yall. It's not necessary for my use case so I could remove it.

Copy link
Contributor

Choose a reason for hiding this comment

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

On second thought, OnGameFrameUpdate is not something we want to use often, so this may be better after all

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. I tried to figure out a way around using it for PauseWarp_Execute, but couldn't figure out a easy way to do it. I've made the checks pretty specific so it shouldn't cause any issue anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea should be fine, I still stand by my original rename suggestion though 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fine by me and I just changed it in the latest commit.

@garrettjoecox garrettjoecox merged commit 3187564 into HarbourMasters:develop Feb 16, 2024
8 checks 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

6 participants