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

Zora's River waterfall always open, take two #4459

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

JordanLongstaff
Copy link
Contributor

@JordanLongstaff JordanLongstaff commented Oct 21, 2024

Second attempt at adding the enhancement. This time, it is handled entirely by VB hooks, and it requires that Link has an ocarina (either one will do) and has learnt Zelda's Lullaby.

Also, TIL the waterfall is called Sleeping Waterfall. 😆

2024-10-21 (1)
2024-10-20

Also added a randomizer setting to keep it open. Can be set to open all the time or only as an adult.

2024-11-01

Build Artifacts

Actor* innerActor = static_cast<Actor*>(innerActorRef);
if (innerActor->id == ACTOR_BG_SPOT03_TAKI && Flags_GetEventChkInf(EVENTCHKINF_LEARNED_ZELDAS_LULLABY) &&
(INV_CONTENT(ITEM_OCARINA_TIME) == ITEM_OCARINA_TIME ||
INV_CONTENT(ITEM_OCARINA_FAIRY) == ITEM_OCARINA_FAIRY) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

I might be reading this wrong, but with the check for Ocarina 6 lines up, wouldn't this be redundant? And the ZL flag check, for that matter?

if (innerActor->id == ACTOR_BG_SPOT03_TAKI && Flags_GetEventChkInf(EVENTCHKINF_LEARNED_ZELDAS_LULLABY) &&
(INV_CONTENT(ITEM_OCARINA_TIME) == ITEM_OCARINA_TIME ||
INV_CONTENT(ITEM_OCARINA_FAIRY) == ITEM_OCARINA_FAIRY) &&
(IS_RANDO || CVarGetInteger(CVAR_ENHANCEMENT("TimeSavers.SleepingWaterfall"), IS_RANDO))
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 the separate IS_RANDO check here is unnecessary because you're using IS_RANDO as the default for the CVarGet.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not quite sure how I feel about it being default on for rando, either, but that's not blocking for me. I'd have to hear how others feel.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer it to not be forced on for rando. I'm sure quite a lot of people will keep it on, but I imagine some people, especially in a race scenario, would appreciate the nuance that having this off would bring. For instance, it may be faster to clip past the waterfall with a Cucco or Hoverboots than playing the song, even if you have an Ocarina and the song handy. Maybe even the Mega-sidehop would be faster if you can set it up quickly enough. I think there's enough nuance between the convenience of playing the song vs tricks you can do potentially being faster that it's worth having the ability to turn it off in a rando run. Maybe there's merit to having this on but only for Ocarina of Time but that would take a lot more discussion. For now I'd merge this with it not being forced on for rando.

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 would surmise that the best option here is to have the checkbox in Enhancements that only affects vanilla, and a separate option that only affects rando (which could be configured to open the waterfall at any time OR only if ZL can be played OR never unless it is played).

The separate IS_RANDO check is something I've seen in several other places in this file. It could very well be redundant, but if it is, it should be removed in all instances.

Copy link
Contributor

@Malkierian Malkierian Oct 22, 2024

Choose a reason for hiding this comment

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

No, it was only in your specific case because IS_RANDO was accounted for in the CVarGet itself. Since I think we've decided that it shouldn't default on in rando, then just removing the check and only relying on the CVar is appropriate.

Copy link
Contributor

@Malkierian Malkierian Oct 22, 2024

Choose a reason for hiding this comment

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

Though, tbf, if there were other instances of IS_RANDO being used as the default for a CVarGet paired with a separate IS_RANDO check, those should be changed, too. I was just saying that the redundancy was specific.

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.

Have two points of feedback but otherwise looks good:

  • Use CHECK_QUEST_ITEM(QUEST_SONG_LULLABY) instead of Flags_Get
  • The way you have it now, setting up the OnActorUpdate is entirely unnecessary, and doesn't really do anything. If your intent was to handle the case where the player enters the scene, then after they have entered the scene became eligible, you should move the eligibility condition to within the actor update. If you only want to check if they are eligible on ActorInit you can remove the OnActorUpdate usage

@garrettjoecox
Copy link
Contributor

The way you have it now, setting up the OnActorUpdate is entirely unnecessary, and doesn't really do anything. If your intent was to handle the case where the player enters the scene, then after they have entered the scene became eligible, you should move the eligibility condition to within the actor update. If you only want to check if they are eligible on ActorInit you can remove the OnActorUpdate usage

Oh, I realize now malk pointed out that you had duplicated this condition both in and out of the OnActorUpdate, looks like you just removed the wrong one

@JordanLongstaff
Copy link
Contributor Author

JordanLongstaff commented Nov 4, 2024

Is there a more efficient way to check that Link has an ocarina? Even if it's just a macro? I wanted to write a HAS_OCARINA macro for that check where it's either the Fairy Ocarina or the Ocarina of Time.

@garrettjoecox
Copy link
Contributor

garrettjoecox commented Nov 4, 2024

Is there a more efficient way to check that Link has an ocarina?

Technically you could do INV_CONTENT(ITEM_OCARINA_FAIRY) != ITEM_NONE, but it's not foolproof if the player somehow puts something else into the ocarina slot

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.

4 participants