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

AHIT: Fix moderate logic rules using add_rule instead of set_rule #3850

Merged

Conversation

Mysteryem
Copy link
Contributor

What is this fixing or adding?

The moderate logic for the Mafia Town Clock Tower Chest and Top of Ruined Tower with nothing, and for clearing Rock the Boat without Ice Hat were mistakenly using add_rule instead of set_rule, which was adding the condition of and True which had no effect.

This patch corrects these moderate logic rules to use set_rule instead.

How was this tested?

A YAML was used with moderate logic difficulty and that act-plandos Rock the Boat onto the starting act and Barrel Battle onto the second act. The playthrough spheres in the spoiler were compared before and after. From additional generations with this YAML after the patch, both Mafia Town - Top of Ruined Tower and Mafia Town - Clock Tower Chest have been observed in sphere 1 of a playthrough as expected.

The moderate logic for the Mafia Town Clock Tower Chest and Top of
Ruined Tower with nothing, and for clearing Rock the Boat without Ice
Hat were mistakenly using `add_rule` instead of `set_rule`, which was
adding the condition of `and True` which had no effect.

This patch corrects these moderate logic rules to use `set_rule`
instead.
@github-actions github-actions bot added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Aug 26, 2024
@Mysteryem
Copy link
Contributor Author

@CookieCat45

@Mysteryem
Copy link
Contributor Author

For the first time, I had a seed that started with Rock the Boat (allowed on moderate logic difficulty) and looking at the sphere tracker, I saw that Act Completion (Rock the Boat) was at sphere 8 when it should have been sphere 1 if the logic was as I expected. After finding the issue, I had a quick look for any others and saw the two Mafia Town locations with the same issue.

@NewSoupVi NewSoupVi added is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. waiting-on: world-maintainer Issue/PR is waiting for feedback or approval by the maintainer of a world. labels Aug 26, 2024
@Exempt-Medic
Copy link
Member

Would the expert rule for The Twilight Bell Act Completion need to be changed to a set rule or an add_rule with "or" as well?

@Mysteryem
Copy link
Contributor Author

Would the expert rule for The Twilight Bell Act Completion need to be changed to a set rule or an add_rule with "or" as well?

Technically that rule is correct because Act Completion (The Twilight Bell) has no requirements on the Location to begin with because normally if you can reach the region it's in, you can reach the location. It could be changed to use set_rule instead.

Copy link
Member

@Exempt-Medic Exempt-Medic left a comment

Choose a reason for hiding this comment

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

Changes LGTM and match my understanding of the game. Looked through the code for other similar cases and the only one I found was addressed

Copy link
Collaborator

@ScipioWright ScipioWright left a comment

Choose a reason for hiding this comment

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

It is changing some rules from adding True to becoming True. Adding True like this can never be correct, so setting it to True is the only thing that makes sense here.

Note: Adding True with add_rule set to "or" is technically functional, but it's also sloppy compared to just setting it to True

@ScipioWright ScipioWright removed the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Aug 26, 2024
@ScipioWright ScipioWright 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: world-maintainer Issue/PR is waiting for feedback or approval by the maintainer of a world. labels Aug 26, 2024
@NewSoupVi NewSoupVi merged commit 9a4e84e into ArchipelagoMW:main Aug 29, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. 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