HK: skip for loop #2390
HK: skip for loop #2390
Conversation
ScootyPuffJr1
added
the
is: refactor/cleanup
worlds/hk/Rules.py
Outdated
| @@ -38,15 +37,13 @@ def hk_set_rule(hk_world: World, location: str, rule): | |||
|
|
|||
|
|
|||
| def set_rules(hk_world: World): | |||
| player = hk_world.player | |||
| world = hk_world.multiworld | |||
| player = hk_world | |||
There was a problem hiding this comment.
how the frick did the unittests pass
There was a problem hiding this comment.
if player is not None:
return [location for location in self._cached_locations if location.player == player]accidental bug find moment
There was a problem hiding this comment.
tested on the new region cache and this would've crashed so that's good.
There was a problem hiding this comment.
lgtm, but approval of either @ThePhar or @BadMagic100 would be good
|
Is this change really necessary? I know there is only the one region now but it seems silly to remove it for truly miniscule performance gains when it's likely to be added back anyway (I am assuming phar is planning to do something about regions at least) |
|
Or, I suppose to frame that question another way: how much would this actually be saving? I assume it to be miniscule (an extra function call per generation?) but in reality don't know any better and will ultimately defer to phar for an approval or denial |
|
The proper refactor for what this is doing wouldn't look like the original method, as looping over every region will be even slower once HK has proper regions. Preferably, it'd add the shop locations to a cache on creation and just loop through that. |
|
Fair enough I suppose, but given that solution isn't the one implemented by this pr I'm not sure it really answers my question |
|
The change to region caching in #2366 makes looping over "all locations for player" preferable over "all regions for player, and then all locations for those regions". It not only makes the code nicer, it also makes it faster, because the collection you want to iterate over already exists anyway. Edit: Re-reading your comments, I feel like you misunderstand the change. We don't iterate "just one region" in the changed code, but instead we iterate over "all locations, ignoring the region it's from". |
|
Oh, you are right, this is what happens when I read code at midnight I suppose |
What is this fixing or adding?
just a tiny optimization I happened to notice when looking at region and location access
How was this tested?
wasn't
If this makes graphical changes, please attach screenshots.