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: Better scaling explicit indirect conditions #4582

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Mysteryem
Copy link
Contributor

What is this fixing or adding?

When the number of connections to retry was large and queue was large new_entrance not in queue would get slow.

For the average supported world, the difference this change makes is negligible.

For a game like Blasphemous, with a lot of explicit indirect conditions, generation of 10 template Blasphemous yamls with
--skip_output --seed 1 and progression balancing disabled went from 19.0s to 17.9s (5.9% reduction in generation duration).

How was this tested?

Ran generations before and after, producing identical results

When the number of connections to retry was large and `queue` was large
`new_entrance not in queue` would get slow.

For the average supported world, the difference this change makes is
negligible.

For a game like Blasphemous, with a lot of explicit indirect conditions,
generation of 10 template Blasphemous yamls with
`--skip_output --seed 1` and progression balancing disabled went from
19.0s to 17.9s (5.9% reduction in generation duration).
@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 Jan 30, 2025
Copy link
Member

@NewSoupVi NewSoupVi left a comment

Choose a reason for hiding this comment

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

I also wonder if it'd be faster to rewrite this whole thing to wait until the queue is empty while building a list of new regions, then add all new indirect conditions to the queue at once somehow

Something like

while queue:  # Outer loop which makes sure to retry indirect conditions
    new_regions_in_iteration = []
    while queue:  # Inner loop which has the normal code
        ... normal code, but ...
        if entrance.can_reach(state):
            new_regions_in_iteration.append(entrance.target)

    for region in new_regions_in_iteration:
        for region, entrances in multiworld.indirect_conditions[player].get(region, []):
            if entrance not in blocked_connections ór entrance.target.can_reach():
                continue
            queue.push(entrance)

Idk that's probably very wrong

@NewSoupVi
Copy link
Member

Actually if what I suggested works we could probably even merge the two different functions again? 🤔 Anyway

@Exempt-Medic Exempt-Medic added the is: enhancement Issues requesting new features or pull requests implementing new features. label Jan 30, 2025
BaseClasses.py Outdated
queue.append(new_entrance)
entrances = self.multiworld.indirect_connections.get(new_region)
if entrances is not None:
entrances = entrances.intersection(blocked_connections)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
entrances = entrances.intersection(blocked_connections)
entrances.intersection_update(blocked_connections)

nitpicky, admittedly, but if we're using _update one line below, I feel this looks better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A new set needs to be created here because entrances starts off as a set instance that is stored within self.multiworld.indirect_connections, which must not be modified by this function.

I could make a new variable for better clarity.
relevant_entrances = entrances.intersection(blocked_connections)

Copy link
Member

Choose a reason for hiding this comment

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

Would it be better to copy it once at the start then, so that you can modify the instance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. I don't see how copying the set would help.

first_set.intersection(second_set) only needs to create a new empty set, iterate the smaller of the two sets, and add the iterated elements that are in the larger set into the new empty set.

Copying the set should take more time and memory.

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: peer-review Issue/PR has not been reviewed by enough people yet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants