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: Dynamically simplify access rules based on options (PoC) #4273

Draft
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

Zannick
Copy link
Contributor

@Zannick Zannick commented Nov 28, 2024

What is this fixing or adding?

This feature aims to support a new way of writing rules to improve performance. Many worlds write their access rules like lambda state: state.has("Item 1", player) and (world.options.setting == "Something" or state.has("Item 2", player)). These rules are run many times over a single generation, so optimizing them is worth it for a small improvement. Here are some issues we could address with this feature:

  1. Settings don't change during generation, so comparing e.g. world.options.setting == "Something" can be done once outside the rule instead of every time.
  2. When we know a rule requires multiple items, or one of a subset, we can use a has_all call or a has_any call to reduce the number of function calls, e.g. if the setting here doesn't match, we need both items.
  3. A rule definition like lambda state: [...] world.options.setting [...] captures a reference to world, which results in a circular reference because world will indirectly reference this rule when it is set. We don't need that (especially when it's mostly used for reading static options).
  4. If two rules are exactly the same, we can reference the same lambda object to save on memory.

Design

We can describe most access rule requirements as some combination of items and counts. Namely, I define

class Req(NamedTuple):
    item: str
    count: int = 1

And I define AnyReq and AllReq to each be a series of requirements, with the former needing any one of its elements to be fulfilled to pass, and the latter requiring all of them. Then, given any of these and a player id, we can make a rule that simply checks whether that player fulfills the combination, and if the rule requirement is a hashable type1, we can cache the generated function for use whenever we have exactly the same combo.

This allows definitions of complex requirements like

             AnyReq([
                 brewing_hat_req,
                 dweller_hat_req,
                 sprint_hat_req,
                 AllReq([time_stop_hat_req, Req("Umbrella", 1)]),
             ]))

i.e. requiring the Brewing Hat, Dweller Hat, Sprint Hat, or both the Time Stop Hat and the Umbrella. The intention is that we can generate the list of Reqs in set_rules and call a library method to turn them into a function on a CollectionState.

(Note that None can be used in place of a Req to indicate no requirement, which is considered always true, and an empty AnyReq is always considered false, while an empty AllReq is considered true.)

Proof of Concept

I've added the rule generation functions in BaseRules.py, and adapted two games as a Proof of Concept with minimal other changes to make it possible: A Hat in Time, and Timespinner. My generation runs see around a 1-3% speed improvement for each game (a little more marked when spoiler=3) and generate the same spoilers. Timespinner was done second (after additional pieces were added to BaseRules) and mostly uses complex_reqs_to_rule as the rule generator function to demonstrate how we could structure the library to have one general-case entry function.

Potential Improvements

Some rules are not adaptable at the moment because they have components like state.can_reach("Region Name", player). These components can be replaced with event items at the given region, potentially events generated on the spot rather than pre-defined. Alternatively, we can write a new "RegionReq" type to include somehow (though then we have to separate them out from the item Reqs we pass in one method call).

I figured it would be worth it to get some feedback at this stage before adapting more games, adding more features, or settling on type names.

How was this tested?

Generated and profiled using e.g. python Generate.py --seed=42 and comparing spoilers/using --skip_output.

Footnotes

  1. which is why AnyReq and AllReq are actually tuples instead of mutable lists

@github-actions github-actions bot added the affects: core Issues/PRs that touch core and may need additional validation. label Nov 28, 2024
@NewSoupVi
Copy link
Member

NewSoupVi commented Nov 28, 2024

All of this is basically what The Witness does. As such, let me comment on this part:

Alternatively, we can write a new "RegionReq" type to include somehow (though then we have to separate them out from the item Reqs we pass in one method call).

The region check is so so so much faster than using event items that this is exactly what I chose to go with. For any given "AllReq" as you call it, I separate out regions from items, then optimise the items requirements and regions requirements separately, then put that back together.

In fact, some games might still need more complex functions, so it would probably also be good to have an ArbitraryRule that uses the old format. You can't do as many fancy optimisations to that, but the lack of this would make this less usable to me, for example, as I still need complicated functions for exactly 2 of my 300+ locations.

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants