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 "has_all_counts" and "has_any_count" functions to CollectionState #2933

Merged
merged 6 commits into from
May 7, 2024

Conversation

NewSoupVi
Copy link
Member

@NewSoupVi NewSoupVi commented Mar 11, 2024

What was done

If you have a requirement like: 3 Progressive Tools, Bucket and 1 Progressive Resource Crafting, right now you'd have to express that as state.has_all(["Progressive Resource Crafting", "Bucket"], player) and state.has("Progressive Tools", player, 3). It's not horrible, but it's slightly unwieldy and a little slower than it could be because more function calls do in fact == longer runtime. I'm not sure how that optimises for an .exe build though.

So, this PR aims to add two new functions:
CollectionState.has_all_counts and CollectionState.has_any_count.
These take a dict of item names to counts. So instead, the condition from the example above could be written as:
state.has_all_counts({"Progressive Tools": 3, "Bucket": 1, "Progressive Resource Crafting": 1}, player), which looks really nice in my opinion.

These functions are really simple and just use the any function / the all function on a generator comprehension over the dict items. This yields fast, lazy evaluation that should be on par with other functions.

Personal Motivation

I don't actually know what my location's requirements are when I set them in The Witness.
I have a standardised "Logic Condition" format. At the start, this gets filled for each interactable entity in the game for all items that could possibly ever needed to activate it, and what other entities it depends on.
I then run a function that automatically 1. recursively flattens the recursive requirements to a single requirement per entity, 2. removes any items that don't exist due to settings, and then 3. optimises the condition (Getting rid of redundancies).

At this point, what I have is basically a Set of and chains of items, with the chains being ored together, per entity. This has yielded incredibly fast results for Witness.

However, I do not have a general way of converting these "or chains of and chains" to a simple CollectionRule, specifically because has_all doesn't support items with multiple copies!. If this existed, I could just do:

location.access_rule = any(state.has_all_counts(possibility, player) for possibility in condition)

Don't take it from just me though! One new world dev has said "I was just wanting this" and another world dev pointed out that KH2 already implements this behavior because it was useful to them. However, notably (no shade intended), they did a much worse job of implementing this behavior.

I think thesenew functions are very pretty, they should be really fast, and they are useful to me and multiple other world devs.

On naming

I would've just called the "all" function has_counts, but I couldn't think of a good way to name the "any" function, which was suggested for addition as well. And I thought they should be named similarly, so I decided to go with has_all_counts and has_any_count. Absolutely willing to hear alternate suggestions.

@github-actions github-actions bot added 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 Mar 11, 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.

Just a small wording suggestion.

BaseClasses.py Show resolved Hide resolved
BaseClasses.py Show resolved Hide resolved
NewSoupVi and others added 2 commits March 13, 2024 21:16
Co-authored-by: Exempt-Medic <60412657+Exempt-Medic@users.noreply.github.com>
Co-authored-by: Exempt-Medic <60412657+Exempt-Medic@users.noreply.github.com>
Copy link
Contributor

@silasary silasary left a comment

Choose a reason for hiding this comment

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

These seem useful

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, seems useful. I don't personally have a use case to test these with.

@Exempt-Medic Exempt-Medic 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 Mar 28, 2024
@Exempt-Medic Exempt-Medic added the is: enhancement Issues requesting new features or pull requests implementing new features. label Apr 8, 2024
BaseClasses.py Outdated Show resolved Hide resolved
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.

lgtm. the more helpers we have here, the easier a potential state optimization would be to pull off.

@NewSoupVi NewSoupVi merged commit 0ac8844 into ArchipelagoMW:main May 7, 2024
16 checks passed
jnschurig pushed a commit to Tranquilite0/Archipelago-SoulBlazer that referenced this pull request Jun 13, 2024
…nState (ArchipelagoMW#2933)

* Add has_all_counts and has_any_counts

* Messenger gave me a red x and I'm mad about it

* Update BaseClasses.py

Co-authored-by: Exempt-Medic <60412657+Exempt-Medic@users.noreply.github.com>

* Update BaseClasses.py

Co-authored-by: Exempt-Medic <60412657+Exempt-Medic@users.noreply.github.com>

* Mapping instead of Dict

---------

Co-authored-by: Exempt-Medic <60412657+Exempt-Medic@users.noreply.github.com>
qwint pushed a commit to qwint/Archipelago that referenced this pull request Jun 24, 2024
…nState (ArchipelagoMW#2933)

* Add has_all_counts and has_any_counts

* Messenger gave me a red x and I'm mad about it

* Update BaseClasses.py

Co-authored-by: Exempt-Medic <60412657+Exempt-Medic@users.noreply.github.com>

* Update BaseClasses.py

Co-authored-by: Exempt-Medic <60412657+Exempt-Medic@users.noreply.github.com>

* Mapping instead of Dict

---------

Co-authored-by: Exempt-Medic <60412657+Exempt-Medic@users.noreply.github.com>
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. 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.

6 participants