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

Rando: Zelda sequence fixes #966

Closed

Conversation

aMannus
Copy link
Contributor

@aMannus aMannus commented Jul 28, 2022

Resolves #749

Changes made in this PR:

  • Skip Child Stealth is set to false when Skip Child Zelda is on, making Skip Child Stealth appear as off in the spoiler log when Skip Child Zelda is on.
  • Changed Child Stealth Skip to only trigger when Zelda has not been met yet.
  • Added flag to Skip Child Zelda option that reflects receiving Zelda's letter.
  • Changed conditionals used for deciding wether the guards should block Link's path after the crawlspace or not.

As a summary, this is the behaviour after these changes:

  • Vanilla game: Guards do not block the way even after meeting zelda, and will only block the way after the "Ocarina thrown into moat" cutscene has played. This didn't change and is vanilla behaviour.
  • Rando, only Skip Child Stealth enabled: Player will be placed at Zelda's Courtyard after going through the crawlspace. Trying this again when Zelda has already been met doesn't skip to Zelda's courtyard and Link is stopped by the guards instead. Guards will not stop Link when only the Ocarina in moat cutscene has been triggered, but Zelda hasn't been met.
  • Rando, only Skip Child Zelda is selected: Guards will immediately stop the player after going through the crawlspace.
  • Rando, both Skip Child Zelda and Skip Child Stealth are enabled: Player is immediately stopped by the guards, Skip Child Stealth shows up in spoiler log as not enabled.

All these scenarios have been tested ingame and confirmed working as stated above.

I hope it's not too confusing for someone else to review this. If anyone has any questions, please ask them.

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.

i think the logic here looks solid, but there are a couple little things that'd be good to clean up before merging

// eventChkInf[4] & 1 = Got Zelda's Letter
// eventChkInf[5] & 0x200 = Got Impa's reward
if (gSaveContext.n64ddFlag && Randomizer_GetSettingValue(RSK_SKIP_CHILD_STEALTH) &&
!(gSaveContext.eventChkInf[4] & 1) && !(gSaveContext.eventChkInf[5] & 0x200)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

might be nice to add a little more context to the rest of this if block via comments explaining what's happening

all of this makes me think it'd be awesome to replace all the entrance index magic numbers with nice names in an enum or something but for now just adding some comments here would go a long way

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'm going to be honest, I don't really know what the entrance values here are either, I just changed the conditional to trigger them. Do you happen to remember how this part worked, or do you want me to dive in and find out?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't remember off the top of my head, I can look into it later if you don't have time to dive in

@@ -790,6 +790,9 @@ void Sram_InitSave(FileChooseContext* fileChooseCtx) {
gSaveContext.eventChkInf[1] |= (1 << 3);
gSaveContext.eventChkInf[1] |= (1 << 4);

// Got Zelda's Letter
gSaveContext.eventChkInf[4] |= 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

we're already doing this later with // make sure saria is at SFM (probably worth just updating that comment to explain that it's also the got zelda's letter flag. i'd probably change it to

// Set "Got Zelda's Letter" flag (also ensures saria is at SFM and TODO: what else does this flag do?)

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 totally missed that. I'll change this.

Comment on lines 119 to 123
// Rando'd but not met Zelda yet
bool randoNotMetZelda =
(gSaveContext.n64ddFlag) && !(gSaveContext.eventChkInf[5] & 0x200) && !(gSaveContext.eventChkInf[4] & 1);
// Rando'd and already met Zelda
bool randoMetZelda = (gSaveContext.n64ddFlag) && (gSaveContext.eventChkInf[5] & 0x200) && (gSaveContext.eventChkInf[4] & 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure how i feel about these

i get the desire to clean up the if statements, but i think combining the rando check and the met zelda check may be going a little too far

i also don't like there being 2 of them, i think ideally we'd have

if ((!rando && non-rando-logic) || (rando && !metZelda)) {

and

if((!rando && non-rando-logic) || (rando && metZelda)) {

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 went back and forth quite a few times on how to handle this myself. I'll change it to what you suggested though, looks better than what I was doing.

@Kenix3
Copy link
Collaborator

Kenix3 commented Aug 3, 2022

Any rando features for Zhora should go to rando-next rather than Zhora.

@aMannus
Copy link
Contributor Author

aMannus commented Aug 3, 2022

Any rando features for Zhora should go to rando-next rather than Zhora.

This is a bugfix, not a new feature.

// eventChkInf[4] & 1 = Got Zelda's Letter
// eventChkInf[5] & 0x200 = Got item from impa
// eventChkInf[8] & 1 = Ocarina thrown in moat
bool rando = gSaveContext.n64ddFlag;
Copy link
Contributor

Choose a reason for hiding this comment

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

having this as a variable here isn't a good idea imo, if we want to move away from having n64ddflag used we should clean it up everywhere instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's fair, I'll change it back. That said, it might be an idea to make a helper function called Randomizer_IsActive() in the future that replaces all the n64dd flags to improve readability and make the n64dd flag less obscure. Not for this PR obviously but yeah.

@briaguya-ai
Copy link
Contributor

This is a bugfix

so it should be pointed to rachael

@aMannus
Copy link
Contributor Author

aMannus commented Aug 4, 2022

This is a bugfix

so it should be pointed to rachael

That's fair. I might have to make a new PR again because rebasing to rachael-develop has not worked well for me so far. I'll do that once I adressed the remaining open review comments.

@aMannus
Copy link
Contributor Author

aMannus commented Aug 8, 2022

Closing because of rebasing being nearly impossible, new PR here: #1095

New PR also adressed everything from the review comments in this PR.

@aMannus aMannus closed this Aug 8, 2022
@aMannus aMannus deleted the rando-zelda-sequence-fixes branch August 13, 2022 05:46
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.

Rando: Child Zelda sequence oddities
3 participants