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

Clean up settings and add boss soul files #4891

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

Conversation

Pepper0ni
Copy link
Contributor

@Pepper0ni Pepper0ni commented Jan 16, 2025

This removes the glitched logic option as it is unused and not planned to be used, removes the Skip Glitch Cutscenes randomiser setting as it is redundant, and dummied out the skip child stealth option as it is redundant until freestanding decides to add the checks there.

I also added some boss soul files I forgot to add.

Build Artifacts

@Malkierian
Copy link
Contributor

What do you mean by "dummied out Skip Child Stealth"? What does it do now, under what circumstances? People can still want to do the stealth if they didn't enable Skip Child Zelda.

@Pepper0ni
Copy link
Contributor Author

I removed the randomiser setting from the randomiser UI as it's now handled in cutscene skips as a timesaver, putting it in a similar limbo to Treasure Chest Game shuffle until someone decides they want to add the freestanding checks there into logic.

This is just to avoid redundancy and confusion, as the setting has 0 effect on logic right now.

@Malkierian
Copy link
Contributor

Oh, I didn't realize it was part of time savers now.

@Malkierian
Copy link
Contributor

As far as the rando setting for enabling glitch cutscenes, does that only set flags on file creation, and thus isn't needed because all skips and exclusions are handled in time savers as well?

@Pepper0ni
Copy link
Contributor Author

It doesn't even do that anymore, it got completely disconnected from the system by V3.

@aMannus aMannus added this to the 9.0.0 milestone Jan 17, 2025
Comment on lines 876 to 877
&mOptions[RSK_BIG_POE_COUNT],
&mOptions[RSK_SKIP_CHILD_STEALTH],
&mOptions[RSK_SKIP_CHILD_ZELDA],
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we get both of the skip child stealth RSK entries commented out instead with an explanation that it's not currently needed but that it's kept around until freestanding needs it?

@aMannus
Copy link
Contributor

aMannus commented Jan 18, 2025

Actually, thinking about the freestanding thing. Shouldn't we just keep the option in the timesavers enhancements anyway, and just force it to be off when a player has freestanding shuffle on? And if so, we should just get rid of all the references to RSK_SKIP_CHILD_STEALTH.

Also, please separate PRs more. Make one for the boss soul files, and one for the options settings in this case. Don't have to do it for this PR now, but for the future. It's blocking other parts of the PR that are totally fine and just makes it harder to get stuff in.

@Pepper0ni
Copy link
Contributor Author

Child stealth + freestanding is the opposite problem: it works fine even with freestanding shuffle, but eventually they would be added as checks, at which point rando needs to know if it's skipped so it knows to not add those locations to the pool (there would need to be other changes to make it repeatable too, which is presumably why it's not done already).

@aMannus
Copy link
Contributor

aMannus commented Jan 18, 2025

Oh, my approach is just the opposite of what you're thinking of. I meant disable skipping child stealth whenever the rando settings dictate there's checks there. If you'd want to go a step further, only do it when all the freestanding items in there haven't been obtained yet.

But honestly I can see either solutions work, so I guess it just depends on what we prefer doing.

@Pepper0ni
Copy link
Contributor Author

I'll leave that decision to whoever implements it, and if they don't need the option, they can remove it themselves.

@aMannus
Copy link
Contributor

aMannus commented Jan 18, 2025

Alright sounds good. Could you add the comments then so we know why they're missing and how they can be restored later?

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