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: Fixed potential softlock with walnut purchases if Entrance Randomizer locks access to the field office #2261

Merged

Conversation

agilbert1412
Copy link
Collaborator

What is this fixing or adding?

There was a missing logic rule when checking for different walnut areas, to verify if the player can obtain enough walnuts for purchases. The system is designed so that it is impossible for a player to make the "wrong" purchases and get stuck. But since the Field office contains 20 potential walnuts, and wasn't being checked, it was possible for a bad entrance randomizer roll to create a scenario where a player could misspend their walnuts and get softlocked

How was this tested?

Ran existing tests, but did not add any new one for this rule, as I am not sure how to simulate a specific entrance randomizer roll under the current system. After ProfByte's rework of the entrance randomizer, I'll be adding tests for cases like this

…n order to be allowed to spend significant amounts of walnuts
@agilbert1412 agilbert1412 changed the title - Fixed potential softlock with walnut purchases if Entrance Randomizer locks access to the field office Stardew Valley: Fixed potential softlock with walnut purchases if Entrance Randomizer locks access to the field office Oct 5, 2023
@ThePhar ThePhar added the is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. label Oct 17, 2023
Copy link
Collaborator

@alwaysintreble alwaysintreble left a comment

Choose a reason for hiding this comment

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

This method is abysmally slow, but the PR does what it says.

@black-sliver
Copy link
Member

This method is abysmally slow, but the PR does what it says.

Yes, I believe stuff like can_complete_field_office should be cached as soon as it gets used twice, and in this case it gets called (up to) 5 times now.

@agilbert1412
Copy link
Collaborator Author

Berserker expressed a belief that this PR was "waiting for action" on my end, so I figured I'd clarify.

The above comments about what is slow or what should be cached are not relevant to this PR. I have made a second PR since, which does cache these rules and many more, to drastically improve performance both on the field office code and on everything else.

So as far as I'm concerned, there is nothing more to do for this PR specifically, unless someone reviews and requests more changes.

@Ishigh1
Copy link
Contributor

Ishigh1 commented Nov 27, 2023

Well Black Silver also stated that the function shouldn't be moved in this PR

@agilbert1412
Copy link
Collaborator Author

Well Black Silver also stated that the function shouldn't be moved in this PR

I disagree with that suggestion, but most importantly, I disagree with the notion that the world dev should not have control over where his functions are placed. So while he can suggest that, I believe I'm allowed to disagree and not do that kind of change.

@black-sliver
Copy link
Member

It really shouldn't be in here as restructuring code should not be part of a bugfix unless the restructuring makes the bugfix easier/nicer. This is after you already said there will be a major update and we are in maintenance phase of the current Stardew Valley world.

Anyway, simply "marking as resolved" isn't good enough for me to see that you plan to ignore my suggestion.

@black-sliver black-sliver added the affects: release/blocker Issues/PRs that must be addressed before next official release. label Nov 28, 2023
@black-sliver black-sliver merged commit 80fed1c into ArchipelagoMW:main Nov 30, 2023
12 checks passed
Jouramie pushed a commit to Jouramie/Archipelago that referenced this pull request Feb 28, 2024
…rance Randomizer locks access to the field office (ArchipelagoMW#2261)

* - Added logic rules for reaching, then completing, the field office in order to be allowed to spend significant amounts of walnuts

* - Revert moving a method for some reason
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects: release/blocker Issues/PRs that must be addressed before next official release. is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants