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: allow for a single player all_state #2418

Closed

Conversation

alwaysintreble
Copy link
Collaborator

@alwaysintreble alwaysintreble commented Nov 2, 2023

What is this fixing or adding?

allows a world to provide a player to multiworld.get_all_state(), building an all_state that only has the items and sweeps the locations of that player. This also fixes an existing bug with get_all_state where it returns the actual state the first time it gets built instead of a copy of it, allowing extra items to be added to the pool.

How was this tested?

hours of debugging 😔
Tested in combination with #2415 and this gave an additional ~25% speed increase. for 300 worlds using pre_fill methods

If this makes graphical changes, please attach screenshots.

@NewSoupVi
Copy link
Member

NewSoupVi commented Nov 3, 2023

Question: When you say "this gave an additional ~25% increase", that means you went and changed some get_all_states to get_all_state(player) in some worlds - But that isn't included in this PR because you moreso wanted to get a feel for it and not actually touch other people's worlds?

If so, this is probably worth a world maintainer ping in the Discord or something, like "please switch to get_all_state(player) if your implementation allows it"

Anyway, lgtm but I don't think I have the core knowledge to be sure that it's right


if use_cache:
self._all_state = ret
if player:
setattr(self, f"_all_state{player}", None)
Copy link
Member

@NewSoupVi NewSoupVi Nov 3, 2023

Choose a reason for hiding this comment

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

Wait what? None?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

lmao oops. i was testing it with a cache then temp removed it to find a bug and put it back wrong

@alwaysintreble
Copy link
Collaborator Author

Question: When you say "this gave an additional ~25% increase", that means you went and changed some get_all_states to get_all_state(player) in some worlds - But that isn't included in this PR because you moreso wanted to get a feel for it and not actually touch other people's worlds?

i tested it under the same conditions as the linked PR

@alwaysintreble
Copy link
Collaborator Author

While I like this idea, think I'm going to close this and reimplement it as a full API change

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants