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

Core: add specific can_reach helpers to CollectionState #2867

Merged
merged 1 commit into from
Mar 3, 2024

Conversation

alwaysintreble
Copy link
Collaborator

What is this fixing or adding?

Adds specific can_reach helpers to CollectionState so that passing a string and strict string comparisons are no longer necessary. Should be marginally faster but didn't benchmark it.

How was this tested?

Used the new helper in my world and made sure it was still passing logic tests. Existing can_reach also calls into the new helpers so any tests with logic related to those also hits this.

If this makes graphical changes, please attach screenshots.

@alwaysintreble alwaysintreble added is: enhancement Issues requesting new features or pull requests implementing new features. affects: core Issues/PRs that touch core and may need additional validation. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Feb 26, 2024
Copy link
Contributor

@Ixrec Ixrec left a comment

Choose a reason for hiding this comment

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

Code LGTM.

My Outer Wilds apworld passes all tests when changed to use these helpers.

(unfortunately I can't meaningfully benchmark this because all of my remaining can_reach calls are in test code)

@Jouramie
Copy link
Contributor

LGTM

Pretty sure using those would (very slightly) improve performances of stardew rules. Unfortunately, I don't really have time to benchmark.

@alwaysintreble alwaysintreble 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 Feb 28, 2024
@Jouramie
Copy link
Contributor

Jouramie commented Mar 2, 2024

So I ended up benchmarking the Stardew 5.x.x branch with these new helpers instead of the usual can_reach, generation is between 1-2% faster.

Copy link
Member

@black-sliver black-sliver left a comment

Choose a reason for hiding this comment

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

I'm not happy with can_reach(..., "..."), so this change is welcome. Maybe we can even deprecate the old code one day and only allow a proper spot in can_reach.

@black-sliver black-sliver merged commit d124df7 into ArchipelagoMW:main Mar 3, 2024
15 checks passed
@github-actions github-actions bot removed the waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer. label Mar 3, 2024
EmilyV99 pushed a commit to EmilyV99/Archipelago that referenced this pull request Apr 15, 2024
EmilyV99 pushed a commit to EmilyV99/Archipelago that referenced this pull request Apr 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects: core Issues/PRs that touch core and may need additional validation. is: enhancement Issues requesting new features or pull requests implementing new features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants