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

Hollow Knight: Adding Godhome Goal Logic #2952

Merged
merged 9 commits into from
Apr 9, 2024

Conversation

qwint
Copy link
Contributor

@qwint qwint commented Mar 14, 2024

What is this fixing or adding?

Adds all the required Event items and logic ported from Rando4 and Journal Rando when appropriate and transformed to fit AP's formatting.
The only rule that was written mostly by hand is "Godhome_Flower_Quest", which is a mix of Flower quest logic from Rando4 with additions of access to the 3 relevant rooms.

I didn't want to change the Generated Data at all, so I created my own source logic file similar to GeneratedRules.py and added extra functions to item/location creation, to create the Godhome events if relevant for the chosen Goal.

Also there was some simplification done to the rules to not need an exhaustive list of helper Events, all conversion should be represented in the logic but commented out. I mostly stopped where I did to reduce chance of human error, but it could be simplified more if desired.

Note: did not (yet?) remove the need for LogicMixin, this only adds the Godhome goals and their logic.

How was this tested?

rolling a few seeds in Siblings (to confirm i didn't break anything), Godhome, and Godhome+Flower Goal options and visually inspected the spoiler log for the correct Events represented in the Playthrough
I will not be playing these seeds.

If this makes graphical changes, please attach screenshots.

@qwint qwint requested a review from ThePhar as a code owner March 14, 2024 06:16
@github-actions github-actions bot added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Mar 14, 2024
@ScipioWright ScipioWright added waiting-on: world-maintainer Issue/PR is waiting for feedback or approval by the maintainer of a world. is: enhancement Issues requesting new features or pull requests implementing new features. labels Mar 14, 2024
@BadMagic100
Copy link
Collaborator

Please re-title to "Hollow Knight: Adding Godhome Goal Logic"

@BadMagic100 BadMagic100 self-requested a review March 15, 2024 17:13
@qwint qwint changed the title Adding Godhome Goal Logic Hollow Knight: Adding Godhome Goal Logic Mar 15, 2024
Copy link
Collaborator

@BadMagic100 BadMagic100 left a comment

Choose a reason for hiding this comment

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

For the most part the approach seems sane and logic looks correct. Lots of stuff that doesn't need to be there though, especially Trues and Falses throughout

worlds/hk/GodhomeData.py Outdated Show resolved Hide resolved
worlds/hk/GodhomeData.py Outdated Show resolved Hide resolved
worlds/hk/GodhomeData.py Outdated Show resolved Hide resolved
worlds/hk/GodhomeData.py Outdated Show resolved Hide resolved
'*Boss_Geo-Massive_Moss_Charger/': lambda state: state.count('Fungus1_29[left1]', player) or state.count('Fungus1_29[right1]', player),
'Bench-Godhome_Atrium/': lambda state: state.count('Bench-Godhome_Atrium', player),
'Bench-Hall_of_Gods/': lambda state: state.count('Bench-Hall_of_Gods', player),
'Room_Colosseum_01[left1]/': lambda state: state.count('Room_Colosseum_01[left1]', player),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove these and reference the underlying event directly, in rando4 logic the postfix / means "discard any state from this term" so it is not meaningful here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is the * prefix also irrelevant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

went ahead and assumed it was, and just expanded the logic per a different comment

Copy link
Collaborator

Choose a reason for hiding this comment

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

Prefix * essentially means "treat the referenced logic definition as a macro"

worlds/hk/GodhomeData.py Outdated Show resolved Hide resolved
worlds/hk/GodhomeData.py Outdated Show resolved Hide resolved
@BadMagic100 BadMagic100 removed the waiting-on: world-maintainer Issue/PR is waiting for feedback or approval by the maintainer of a world. label Mar 15, 2024
worlds/hk/Items.py Outdated Show resolved Hide resolved
worlds/hk/GodhomeData.py Outdated Show resolved Hide resolved
worlds/hk/GodhomeData.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@agilbert1412 agilbert1412 left a comment

Choose a reason for hiding this comment

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

Sorry I was apparently not up to date the first time

@BadMagic100 BadMagic100 added waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer. and removed waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Mar 22, 2024
@Berserker66 Berserker66 merged commit 32c92e0 into ArchipelagoMW:main Apr 9, 2024
15 checks passed
EmilyV99 pushed a commit to EmilyV99/Archipelago that referenced this pull request Apr 15, 2024
EmilyV99 pushed a commit to EmilyV99/Archipelago that referenced this pull request Apr 15, 2024
@qwint qwint deleted the HK_Godhome branch April 30, 2024 18:13
qwint added a commit to qwint/Archipelago that referenced this pull request Jun 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is: enhancement Issues requesting new features or pull requests implementing new features. waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants