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

TLOZ: Fix rings classification, so they are actually considered for logic #3253

Merged
merged 1 commit into from
May 2, 2024

Conversation

t3hf1gm3nt
Copy link
Collaborator

What is this fixing or adding?

We have a whole set of rules set up (Lines 31-38 of Rules.py) to make it so players are not expected to go into dungeons without a certain level of defense, which is n-1 heart containers for each level. We also had it set up that blue ring would reduce the number of containers expected by half, and red ring by a quarter...but currently both rings are classified as useful, meaning they aren't actually considered for logic. I was informed about this a while ago and thought I had already submitted a fix for it, but apparently I forgor. I'm rectifying that now, changing the classification for both rings to progression and also adding a comment to the loop that sets up this defense rule in Rules.py as I kept losing track of where it was.

How was this tested?

Ran dozens of test gens and the unit tests, as well as checked spoilers and noted that rings showed up in playthroughs now.

If this makes graphical changes, please attach screenshots.

… logic

We have a whole set of rules set up (Lines 31-38 of Rules.py) to make it so players are not expected to go into dungeons without a certain level of defense, which is n-1 heart containers for each level. We also had it set up that blue ring would reduce the number of containers expected by half, and red ring by a quarter...but currently both rings are classified as useful, meaning they aren't actually considered for logic. I was informed about this a while ago and thought I had already submitted a fix for it, but apparently I forgor. I'm rectifying that now, changing the classification for both rings to progression and also adding a comment to the loop that sets up this defense rule in Rules.py as I kept losing track of where it was.
@github-actions github-actions bot added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label May 2, 2024
@ScipioWright ScipioWright added the is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. label May 2, 2024
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.

Simple change, does fix the issue

@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: peer-review Issue/PR has not been reviewed by enough people yet. labels May 2, 2024
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.

LGTM. The Rings do not appear in playthroughs without the change and they do with the change.

@ThePhar ThePhar changed the title [TLOZ]: Fix rings classification, so they are actually considered for logic TLOZ: Fix rings classification, so they are actually considered for logic May 2, 2024
@ThePhar ThePhar merged commit 255e526 into ArchipelagoMW:main May 2, 2024
16 checks passed
@t3hf1gm3nt t3hf1gm3nt deleted the ring_progression_fix branch May 8, 2024 23:17
jnschurig pushed a commit to Tranquilite0/Archipelago-SoulBlazer that referenced this pull request Jun 13, 2024
qwint pushed 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: 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.

4 participants