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: Add Trap Distribution setting #4601

Open
wants to merge 26 commits into
base: main
Choose a base branch
from

Conversation

agilbert1412
Copy link
Collaborator

What is this fixing or adding?

I originally wanted to put this in 7.x.x, but I figured out a decent way to make this work without being a breaking change in the mod. So in the spirit of making smaller, more incremental PRs, I decided to submit it as is.

This adds an option that allows users to specify a distribution for traps (assuming they enabled traps at all). They can use it to disable certain traps, or change how many they'll get of each.

As OptionDict() doesn't work on webhost, this is a yaml-only setting. If it ever gets added to webhost, I added it to groups properly and it'll work day1.

This only impacts the chances of rolling each trap. In practice, there will still be variation.

Lastly, to make this work, I had to do a refactor. Our class items.py has been split into two item_classes.py and item_creation.py. This is to break up a circular dependency, where item_creation needs options, but now the options need the item_classes to build the Schema for this option.

How was this tested?

Added some tests specifically for this, but since the feature is inherently random, the test cannot have 100% success rate. It is always possible that a freak seed ends up generating many of a trap that was requested with low odds, for example.
So I locked these tests behind one of our flags that makes it not run in the github pipeline, only with our specific local run configs. This way, the rare failure won't bother anyone except us.

I also generated a couple seeds with various odds to see the number of resulting traps matching the expected ballparks.

@github-actions github-actions bot added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Feb 3, 2025
@agilbert1412 agilbert1412 added the is: enhancement Issues requesting new features or pull requests implementing new features. label Feb 3, 2025
Copy link
Contributor

@Jouramie Jouramie left a comment

Choose a reason for hiding this comment

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

I think the comments on the weights parameter really need to be addressed, as this probably hit the performances a lot because of the loop. The potential caching issue in the tests needs to be validated as well. The rest is pretty minor.

worlds/stardew_valley/__init__.py Outdated Show resolved Hide resolved
worlds/stardew_valley/options/options.py Outdated Show resolved Hide resolved
worlds/stardew_valley/options/options.py Outdated Show resolved Hide resolved
worlds/stardew_valley/options/options.py Outdated Show resolved Hide resolved
worlds/stardew_valley/options/options.py Show resolved Hide resolved
worlds/stardew_valley/test/__init__.py Outdated Show resolved Hide resolved
worlds/stardew_valley/item_creation.py Outdated Show resolved Hide resolved
worlds/stardew_valley/item_creation.py Outdated Show resolved Hide resolved
worlds/stardew_valley/item_classes.py Outdated Show resolved Hide resolved
@agilbert1412 agilbert1412 requested a review from Jouramie February 6, 2025 01:59
Copy link
Contributor

@Jouramie Jouramie left a comment

Choose a reason for hiding this comment

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

Just nitpicking, LGTM

worlds/stardew_valley/items/__init__.py Show resolved Hide resolved
worlds/stardew_valley/options/options.py Outdated Show resolved Hide resolved
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: peer-review Issue/PR has not been reviewed by enough people yet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants