Fix Mido without Emerald softlock #4824

Varuuna · 2025-01-07T11:14:28Z

First PR. Still learning the ropes of this project and Cpp in general.

This aims to fix the softlock caused by talking to Mido without having the Kokiri Emerald after having defeated Gohma.

Checks the Deku Tree blue warp event flag to determine if we've defeated Gohma and also makes sure we are in KF when it happens so we don't raise the flag when playing Saria's Song to Mido as adult.

Build Artifacts

…lling Gohma

Pepper0ni · 2025-01-07T12:13:43Z

soh/src/overlays/actors/ovl_En_Md/z_en_md.c

@@ -794,8 +794,10 @@ void func_80AABD0C(EnMd* this, PlayState* play) {
        return;
    }

-    if (CHECK_QUEST_ITEM(QUEST_KOKIRI_EMERALD) && !Flags_GetEventChkInf(EVENTCHKINF_SPOKE_TO_MIDO_AFTER_DEKU_TREES_DEATH) &&
-        (play->sceneNum == SCENE_KOKIRI_FOREST)) {
+    if ((GameInteractor_Should(VB_FIX_MIDO_EMERALD_SOFTLOCK, false, this) && play->sceneNum == SCENE_KOKIRI_FOREST) || 


Not the hook expert so someone more senior can correct me if I'm wrong, but I believe it's more correct to use the vanilla condition in place of false in the default condition argument. This presets *should to that value, which can and should be checked in the hook to make sure we don't set the flag badly. It is definitly better to have the && play->sceneNum == SCENE_KOKIRI_FOREST condition in the hook rarther than in src though even if it should have no effect, as we want the hook to be the only change to src.

Sounds good. I've moved the scene check to the hook and put the vanilla condition inside of GameInteractor_Should. Re-tested with both rando and vanilla playing and it seems to be behave as expected on both. Hope I understood that correctly.

* moved vanilla conditioon into GameInteractor_Should

Pepper0ni · 2025-01-07T14:04:40Z

soh/soh/Enhancements/randomizer/hook_handlers.cpp

@@ -1616,6 +1616,10 @@ void RandomizerOnVanillaBehaviorHandler(GIVanillaBehavior id, bool* should, va_l
            }
            break;
        }
+        case VB_FIX_MIDO_EMERALD_SOFTLOCK: {
+            *should = Flags_GetEventChkInf(EVENTCHKINF_USED_DEKU_TREE_BLUE_WARP) && gPlayState->sceneNum == SCENE_KOKIRI_FOREST;


As *should is not part of the comparison, it is being over-written regardless of the vanilla condition (which is the default state of *should). Because of this while in rando (as this hook check only runs in rando) it will always return true if the player is in kokiri and has beaten gohma, and will always return false if not. I doubt this is what you intended. To replicate the condition you had before the most recent change you would want *should || (Flags_GetEventChkInf(EVENTCHKINF_USED_DEKU_TREE_BLUE_WARP) && gPlayState->sceneNum == SCENE_KOKIRI_FOREST) so that it remains true if the vanilla condition is true.

Thanks! Yes that is what I wanted. Didn't consider the fact I was overriding it always.

I'd just put an if statement around your *should declaration. If your conditions are met,, change it to whatever it needs to be changed to, otherwise it just won't touch the *should period. Makes it easier to see what the intention is too.

Pepper0ni · 2025-01-07T14:06:21Z

soh/src/overlays/actors/ovl_En_Md/z_en_md.c

+            VB_FIX_MIDO_EMERALD_SOFTLOCK,
+            CHECK_QUEST_ITEM(QUEST_KOKIRI_EMERALD) && !Flags_GetEventChkInf(EVENTCHKINF_SPOKE_TO_MIDO_AFTER_DEKU_TREES_DEATH) &&
+                (play->sceneNum == SCENE_KOKIRI_FOREST),
+            this)


Tiny nitpick that I didn't tell you about before, as we aren't using using any arguments in the hook, we don't need to declare any. Hooks are set up to use varadic arguments, which is to say any number, including 0, so specifying this isn't needed unless we are actually using the argument.

Excuse me inexperience here, but is there something "unecessary" or wrong then with how I set this up? I looked at another example in the same file that seemed to do something similar: GameInteractor_Should(VB_MOVE_MIDO_IN_KOKIRI_FOREST, this->interactInfo.talkState == NPC_TALK_STATE_ACTION, this) (behavior, condition, "this") so I was using that as an example.

That's also unnecessary from what I can tell. The existing devs aren't 100% perfect and like I said, it's a nitpick, it won't affect the codes execution at all.

* removed 'this'

aMannus · 2025-01-07T18:23:42Z

soh/soh/Enhancements/game-interactor/GameInteractor.h

+    VB_FIX_MIDO_EMERALD_SOFTLOCK,



This name should be changed to something more descriptive of what the vanilla code within it does. Basically, it reads as "GameInteractor_Should(DoWhateverTheCodeWithinItDoes)". So you want to describe wether or not it should execute the code within instead of what the intention of skipping said code block is. I hope I makes sense here. :P

I haven't looked into the code too deeply, but it looks like it's setting the flag for talking to mido after deku tree's death? So an example would be VB_HANDLE_MIDO_TALK_AFTER_DEKU_TREE_DEATH but preferably something shorter based on what it does (I'm terrible at names).

Makes sense! Yeah names is not my strong suit either :P I'll update it.

My problem is that there is technically a hook with a name close enough to what I would like to use: VB_MIDO_CONSIDER_DEKU_TREE_DEAD. It checks whether or not we have received the Kokiri Emerald (which we might not in rando) and that is what this PR corrects.

I'm gonna take another look at what flags are raised during gameplay, perhaps I can use that hook instead instead of making a new one for my specific case?

Archez

Hey, thanks for opening this PR 🙂 and glad to see you trying the project out.

This works for the provided codeflow, but it works because it is side-stepping some other behavior. The goal with the Vanilla Behavior system is to try and preserve those code flows, and in the case of randomizer create the alternate condition requirements.

In this case, the behavior that we want to focus on is checks for the Kokiri Emerald. You've identified that in a rando context Kokiri Emerald checks should be redirected to the deku tree bluewarp flag.

What I would like to see is that instead of this hook passing in the whole vanilla condition, we should instead just have GameInteractor_Should() wrapped around all the checks for kokiri emerald in Mido's code.

We already have one of these instances dealt with by an existing VB_MIDO_CONSIDER_DEKU_TREE_DEAD. It uses a different flag than the bluewarp flag, but both are set when using the blue warp so it is an equivalent check.

e.g.

if (GameInteractor_Should(VB_MIDO_CONSIDER_DEKU_TREE_DEAD, CHECK_QUEST_ITEM(QUEST_KOKIRI_EMERALD)) &&
    !Flags_GetEventChkInf(EVENTCHKINF_SPOKE_TO_MIDO_AFTER_DEKU_TREES_DEATH) &&
    (play->sceneNum == SCENE_KOKIRI_FOREST)) {
    // ...
}

Varuuna · 2025-01-07T19:15:18Z

Hey, thanks for opening this PR 🙂 and glad to see you trying the project out.

This works for the provided codeflow, but it works because it is side-stepping some other behavior. The goal with the Vanilla Behavior system is to try and preserve those code flows, and in the case of randomizer create the alternate condition requirements.

In this case, the behavior that we want to focus on is checks for the Kokiri Emerald. You've identified that in a rando context Kokiri Emerald checks should be redirected to the deku tree bluewarp flag.

What I would like to see is that instead of this hook passing in the whole vanilla condition, we should instead just have GameInteractor_Should() wrapped around all the checks for kokiri emerald in Mido's code.

We already have one of these instances dealt with by an existing VB_MIDO_CONSIDER_DEKU_TREE_DEAD. It uses a different flag than the bluewarp flag, but both are set when using the blue warp so it is an equivalent check.

e.g.
if (GameInteractor_Should(VB_MIDO_CONSIDER_DEKU_TREE_DEAD, CHECK_QUEST_ITEM(QUEST_KOKIRI_EMERALD)) &&
    !Flags_GetEventChkInf(EVENTCHKINF_SPOKE_TO_MIDO_AFTER_DEKU_TREES_DEATH) &&
    (play->sceneNum == SCENE_KOKIRI_FOREST)) {
    // ...
}

Thanks for the input and information. Appreciate it!
I'll take another at this in a few hours. I'm sure I can make it a better solution.

…into develop

* changed actor to use existing hook

Varuuna · 2025-01-07T22:41:20Z

Gave it some thought. Since the flag for obtaining the Kokiri Emerald + Deku Tree Dead is raised in the game after defeating Gohma and blue warping out of there, I figured I could instead use an existing hook, VB_MIDO_CONSIDER_DEKU_TREE_DEAD, because that is the behavior we are looking for.

So I've reverted GameInteractor.h as well as hook_handlers.cpp, removing what I previously had added, and changed z_en_md.c to use the VB_MIDO_CONSIDER_DEKU_TREE_DEAD hook instead.

Would there be anything wrong with using this approach?

Archez · 2025-01-07T23:31:00Z

Almost!
Re-using that VB is fine in this case yea, but double check the snippet I posted above. What we would want is for the Should call to only have the kokir emerald check passed in likeGameInteractor_Should(VB_MIDO_CONSIDER_DEKU_TREE_DEAD, CHECK_QUEST_ITEM(QUEST_KOKIRI_EMERALD)). The other parts of the vanilla condition like the flag and scene check should be outside it.

We also need to wrap a couple other kokiri emerald checks in this file, such as

Shipwright/soh/src/overlays/actors/ovl_En_Md/z_en_md.c

Lines 730 to 733 in 47ba512

    
           if (CHECK_QUEST_ITEM(QUEST_KOKIRI_EMERALD) && !Flags_GetEventChkInf(EVENTCHKINF_SPOKE_TO_MIDO_AFTER_DEKU_TREES_DEATH) && 
        
               (play->sceneNum == SCENE_KOKIRI_FOREST)) { 
        
               play->msgCtx.msgMode = MSGMODE_PAUSED; 
        
           }

Varuuna · 2025-01-07T23:40:34Z

Almost! Re-using that VB is fine in this case yea, but double check the snippet I posted above. What we would want is for the Should call to only have the kokir emerald check passed in likeGameInteractor_Should(VB_MIDO_CONSIDER_DEKU_TREE_DEAD, CHECK_QUEST_ITEM(QUEST_KOKIRI_EMERALD)). The other parts of the vanilla condition like the flag and scene check should be outside it.

We also need to wrap a couple other kokiri emerald checks in this file, such as

Shipwright/soh/src/overlays/actors/ovl_En_Md/z_en_md.c

Lines 730 to 733 in 47ba512

if (CHECK_QUEST_ITEM(QUEST_KOKIRI_EMERALD) && !Flags_GetEventChkInf(EVENTCHKINF_SPOKE_TO_MIDO_AFTER_DEKU_TREES_DEATH) &&

(play->sceneNum == SCENE_KOKIRI_FOREST)) {

play->msgCtx.msgMode = MSGMODE_PAUSED;

}

Understood! Thanks again! Will give it another shot.

…into develop

Varuuna · 2025-01-08T07:08:21Z

I hope I understood this correctly now. (Apologize if I'm still missing something)

The Should now only contains the kokirir emerald check, and has been added to other conditions that checked for the kokiri emerald in the same manner.

Archez

Just two last parenthesis scoping issues, then it is good to go!

soh/src/overlays/actors/ovl_En_Md/z_en_md.c

Varuuna · 2025-01-08T17:25:07Z

Just two last parenthesis scoping issues, then it is good to go!

Whoops, missed those. Corrected.

Archez

🎉

* fixes softlock when talking to Mido without Kokiri Emerald after ki…

d2015f5

…lling Gohma

Pepper0ni reviewed Jan 7, 2025

View reviewed changes

* moved scene check to hook

ef1a75a

* moved vanilla conditioon into GameInteractor_Should

Pepper0ni suggested changes Jan 7, 2025

View reviewed changes

* corrected hook condition

281b2ee

* removed 'this'

aMannus reviewed Jan 7, 2025

View reviewed changes

Archez reviewed Jan 7, 2025

View reviewed changes

Varuuna added 2 commits January 7, 2025 20:17

Merge branch 'develop' of https://github.com/HarbourMasters/Shipwright …

3b1f1cc

…into develop

* reverted GameInteractor and hook_handlers

9339478

* changed actor to use existing hook

Varuuna added 2 commits January 8, 2025 08:06

Merge branch 'develop' of https://github.com/HarbourMasters/Shipwright …

186d565

…into develop

* updated kokiri emerald conditions

b6a2272

Archez reviewed Jan 8, 2025

View reviewed changes

soh/src/overlays/actors/ovl_En_Md/z_en_md.c Outdated Show resolved Hide resolved

soh/src/overlays/actors/ovl_En_Md/z_en_md.c Outdated Show resolved Hide resolved

* missed parentheses

f73fb75

Archez approved these changes Jan 8, 2025

View reviewed changes

Malkierian merged commit 3b26bd0 into HarbourMasters:develop Jan 11, 2025
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Mido without Emerald softlock #4824

Fix Mido without Emerald softlock #4824

Varuuna commented Jan 7, 2025 •

edited by github-actions bot

Loading

Pepper0ni Jan 7, 2025

Varuuna Jan 7, 2025 •

edited

Loading

Pepper0ni Jan 7, 2025

Varuuna Jan 7, 2025

aMannus Jan 7, 2025

Pepper0ni Jan 7, 2025

Varuuna Jan 7, 2025

Pepper0ni Jan 7, 2025

aMannus Jan 7, 2025

Varuuna Jan 7, 2025

Varuuna Jan 7, 2025

Archez left a comment •

edited

Loading

Varuuna commented Jan 7, 2025

Varuuna commented Jan 7, 2025

Archez commented Jan 7, 2025

Varuuna commented Jan 7, 2025

Varuuna commented Jan 8, 2025

Archez left a comment

Varuuna commented Jan 8, 2025

Archez left a comment

Fix Mido without Emerald softlock #4824

Fix Mido without Emerald softlock #4824

Conversation

Varuuna commented Jan 7, 2025 • edited by github-actions bot Loading

Build Artifacts

Choose a reason for hiding this comment

Varuuna Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Archez left a comment • edited Loading

Choose a reason for hiding this comment

Varuuna commented Jan 7, 2025

Varuuna commented Jan 7, 2025

Archez commented Jan 7, 2025

Varuuna commented Jan 7, 2025

Varuuna commented Jan 8, 2025

Archez left a comment

Choose a reason for hiding this comment

Varuuna commented Jan 8, 2025

Archez left a comment

Choose a reason for hiding this comment

Varuuna commented Jan 7, 2025 •

edited by github-actions bot

Loading

Varuuna Jan 7, 2025 •

edited

Loading

Archez left a comment •

edited

Loading