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: Remove some events for a slight performance increase #4085

Conversation

Jouramie
Copy link
Contributor

What is this fixing or adding?

This PR removes some events that were added to handle indirect connection between the Carpenter shop, Pierre and some buildings (only relevant when building progression is vanilla with entrance randomizer). Since the 5.x.x update, indirect connection are automatically calculated and registered every time a rule is added to an entrance, so those events are unnecessary.

This improves performances for two reasons:

  • The performance overhead for the fill to sweep again every time an event is collected is no longer required since the indirect connection are automatically configured
  • Replacing the rule checking for the event by the actual rule underneath sometime allows simplification.

How was this tested?

  • Added a log to make sure the indirect connection are created

image

  • Rolled 100+ worlds for performance tests. The overall impact on performance is pretty small. It's around 3% faster.

image

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 Oct 24, 2024
worlds/stardew_valley/rules.py Dismissed Show resolved Hide resolved
@agilbert1412 agilbert1412 added the is: refactor/cleanup Improvements to code/output readability or organizization. label Oct 24, 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.

Changes LGTM, the rules apply properly and all instances of the events are converted over. Did some test generations and the indirect connections are properly formed as well.

@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 Oct 29, 2024
@Jouramie
Copy link
Contributor Author

Jouramie commented Nov 24, 2024

Found a bug while running tests in another branch, while working on something else. It appears that entrances in the arcade machines, the mines, the skull caverns, and most importantly, the regions for tool upgrade were not using the set_entrance_rule that automatically check for indirect connection.

Tool upgrade require money, which need access to Pierre's. When Pierre's was entrance rando-ed somewhere with logic, the indirect connection was not properly registered.

Now:
image

@Berserker66 Berserker66 merged commit ed4e44b into ArchipelagoMW:main Nov 29, 2024
18 checks passed
@Jouramie Jouramie deleted the StardewValley/remove-unneeded-events branch November 29, 2024 19:51
Jouramie added a commit to agilbert1412/Archipelago that referenced this pull request Dec 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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