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

Stardew Valley: Fix a bug where walnutsanity would get deactivated even tho ginger island got forced activated (and move some files) #4311

Conversation

Jouramie
Copy link
Contributor

@Jouramie Jouramie commented Dec 1, 2024

What is this fixing or adding?

🙄
image

This fixes a bug with the options force activation where ginger island would get activated because of a ginger island goal, but walnutsanity would get deactivated right after. This happens because the exclude_ginger_island variable was not refreshed after ginger island got activated.

This branch also contains a bunch of moved files to regroup option related stuff in their own module. If you want to review specifically the fix, it is in this commit: af14bcb

How was this tested?

  • Wrote some unit tests to validate specifically the function forcing options activation
  • Generate games with the walnut hunter goal, walnutsanity settings and ginger island excluded/included. With those changes, walnutsanity stay active.

If this makes graphical changes, please attach screenshots.

N/A

@github-actions github-actions bot added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Dec 1, 2024
@Jouramie Jouramie changed the title Stardew Valley: Fix a bug where walnutsanity would get deactivated even tho ginger island got forced activated Stardew Valley: Fix a bug where walnutsanity would get deactivated even tho ginger island got forced activated (and move some files) Dec 1, 2024
@agilbert1412 agilbert1412 added is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. is: refactor/cleanup Improvements to code/output readability or organizization. labels Dec 1, 2024
Comment on lines +7 to +10
try:
from Options import OptionGroup
except ImportError:
logging.warning("Old AP Version, OptionGroup not available.")
Copy link
Member

Choose a reason for hiding this comment

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

Do you need this still?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably not, but I would rather not change too much the content of this file, since it was only supported to be moved.

@@ -112,36 +113,9 @@ def interpret_slot_data(self, slot_data: Dict[str, Any]) -> Optional[int]:
return seed

def generate_early(self):
self.force_change_options_if_incompatible()
force_change_options_if_incompatible(self.options, self.player, self.player_name)
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason this doesn't just pass the world and you grab the options, player, and name off that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It makes it easier to test specifically that options were changed without creating a complete world. We've often been rightfully criticized for generating too many worlds just to test small things that could be unit tested, so I'm slowly trying to address that.

See in TestForcedOptions.py

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.

Overall it seems fine. Tested some generations to see the new warnings. Did not look at any of the test changes (don't really see why I would). Checked the file refactoring through text comparisons and before/after were the same aside from things like options. being removed. Had some comments on the changes (see above), but it's mostly fine. Honestly, I'm surprised world_options.walnutsanity != options.Walnutsanity.preset_none works the way it does, but it seems to so that's good.

@Jouramie Jouramie requested a review from agilbert1412 December 2, 2024 01:56
@agilbert1412 agilbert1412 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 Dec 2, 2024
Copy link
Member

@Berserker66 Berserker66 left a comment

Choose a reason for hiding this comment

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

25 files changed for a bugfix is probably a new record.

@Berserker66 Berserker66 merged commit 51c4fe8 into ArchipelagoMW:main Dec 9, 2024
16 checks passed
@Jouramie Jouramie deleted the StardewValley/fix-walnut-force-options-change branch December 9, 2024 02:05
@agilbert1412
Copy link
Collaborator

25 files changed for a bugfix is probably a new record.

image

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. is: refactor/cleanup Improvements to code/output readability or organizization. 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