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

Implementation of 100 Skulltula Check #2325

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

Conversation

Jaybone25
Copy link

PR Contains the implementation of a 100 Gold Skulltula check.

In this PR I have also taken the liberty of moving the Skulltula Misc hints out of hacks.asm and into /Hacks/ovl_en_ssh.asm to clean up the hacks file, and added the code for the 100 Skulltula Misc hint in this file as well.

Current implementation has it as a default check, if desired, I can make it a setting, however it didn't feel correct to make something a setting that can just be excluded on another tab.

Since this would potentially affect current presets, I have went through and disabled the check in all presets except Hell Mode, as it felt appropriate to keep it in there.

@fenhl fenhl added Type: Enhancement New feature or request Component: Logic Non-trivial changes to the JSON logic files Component: ASM/C Changes some internals of the ASM/C libraries Status: Needs Review Someone should be looking at it Status: Needs Testing Probably should be tested Status: Under Consideration Developers are considering whether to accept or decline the feature described labels Nov 4, 2024
@fenhl
Copy link
Collaborator

fenhl commented Nov 4, 2024

I feel like this should be a setting. It's normally a repeatable item which isn't typically something included in the basic set of shuffled locations. It also follows precedent set by frog shuffle being a setting even though it works much more like a regular check.

@Jaybone25
Copy link
Author

Jaybone25 commented Nov 4, 2024

That's fine. I can do this. I just expected leaving it as default would be the preferred way since you can just disable checks form the excluded checks area so this as a setting kind of has the same function as that.

I'll expand the setting then to make it a list of which skull checks to turn on then, so that you can turn off any of the skull checks from there rather than just the 100.

@fenhl
Copy link
Collaborator

fenhl commented Nov 4, 2024

I don't think expanding the setting makes sense, since that would be redundant with excluding locations, and the other skull checks already exist so the frog shuffle precedent doesn't apply.

@Jaybone25
Copy link
Author

Sounds good

@cjohnson57
Copy link
Collaborator

Agree with Fenhl, since it's a non-standard location type it should be a setting

World.py Outdated Show resolved Hide resolved
@fenhl fenhl added the Status: Waiting for Author Changes or response requested label Nov 5, 2024
@Jaybone25
Copy link
Author

Jaybone25 commented Nov 5, 2024 via email

SettingsList.py Outdated Show resolved Hide resolved
@fenhl fenhl removed Status: Waiting for Author Changes or response requested Status: Needs Review Someone should be looking at it labels Nov 6, 2024
data/Glitched World/Overworld.json Outdated Show resolved Hide resolved
data/World/Overworld.json Outdated Show resolved Hide resolved
@cjohnson57 cjohnson57 removed the Status: Under Consideration Developers are considering whether to accept or decline the feature described label Dec 5, 2024
@fenhl
Copy link
Collaborator

fenhl commented Dec 5, 2024

With 100 skulls being included in Hell Mode, Gbk should probably be changed to 20 hearts to have those required as well.

@r0bd0g
Copy link

r0bd0g commented Dec 5, 2024

Nothing about having this check shuffled actually forces 100 gs to be collected every time though?

@Jaybone25
Copy link
Author

Thx should be fixed as soon as the tests go through.

@fenhl
Copy link
Collaborator

fenhl commented Dec 6, 2024

Nothing about having this check shuffled actually forces 100 gs to be collected every time though?

Right, but now there's a check which can potentially require the tokens, which isn't the case for the hearts.

@fenhl fenhl requested a review from cjohnson57 December 6, 2024 00:24
@fenhl fenhl added the Status: Needs Review Someone should be looking at it label Dec 6, 2024
@r0bd0g
Copy link

r0bd0g commented Dec 6, 2024

It's not super likely that something required goes on 100. I think 100 wincon is still worse just b/c there's so many more of them.

@cjohnson57
Copy link
Collaborator

It's not super likely that something required goes on 100. I think 100 wincon is still worse just b/c there's so many more of them.

I agree with this after giving it some thought. 100GS bridge should still be a higher number of required checks, on average

@fenhl
Copy link
Collaborator

fenhl commented Dec 6, 2024

Yeah, that's fair.

@Jaybone25
Copy link
Author

Jaybone25 commented Dec 6, 2024 via email

@fenhl fenhl removed the Status: Needs Review Someone should be looking at it label Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: ASM/C Changes some internals of the ASM/C libraries Component: Logic Non-trivial changes to the JSON logic files Status: Needs Testing Probably should be tested Type: Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants