Timespinner: move options checks out of rules #4708
Conversation
| return (self.flag_pyramid_start or self.flag_inverted) and self.flag_back_to_the_future and state.has_all({'Timespinner Wheel', 'Timespinner Spindle'}, self.player) | ||
|
|
||
| def lock_key_amadeus_enabled(self) -> bool: | ||
| return self.flag_lock_key_amadeus |
There was a problem hiding this comment.
Not really a fan of having this method just delegate to checking the flag, but basically everything involving this flag has logic unique to one or two locations or regions meaning this approach is likely to be the most comprehensible.
ScipioWright
added
is: refactor/cleanup
worlds/timespinner/Locations.py
Outdated
| @@ -92,15 +92,15 @@ def get_location_datas(player: Optional[int], options: Optional[TimespinnerOptio | |||
| LocationData('Military Fortress (hangar)', 'Military Fortress: Pedestal', 1337065, lambda state: state.has('Water Mask', player) if flooded.flood_lab else (logic.has_doublejump_of_npc(state) or logic.has_forwarddash_doublejump(state))), | |||
| LocationData('The lab', 'Lab: Coffee break', 1337066), | |||
| LocationData('The lab', 'Lab: Lower trash right', 1337067, logic.has_doublejump), | |||
| LocationData('The lab', 'Lab: Lower trash left', 1337068, lambda state: logic.has_doublejump_of_npc(state) if options.lock_key_amadeus else logic.has_upwarddash ), | |||
| LocationData('The lab', 'Lab: Lower trash left', 1337068, lambda state: logic.has_doublejump_of_npc(state) if logic.lock_key_amadeus_enabled() else logic.has_upwarddash ), | |||
There was a problem hiding this comment.
Does this really has a significant impact? logic.lock_key_amadeus_enabled() will still be evaluated every time the rule is called during the fill.
Moving the whole if would really be significant as it would only be evaluated once instead of everytime the rule is evaluated.
LocationData('The lab', 'Lab: Lower trash left', 1337068, logic.has_doublejump_of_npc) \
if logic.lock_key_amadeus_enabled() \
else LocationData('The lab', 'Lab: Lower trash left', 1337068, logic.has_upwarddash)There was a problem hiding this comment.
I think you're correct that as written this doesn't actually have any significant impact, so I decided to do a set of test generations (10x with 64 players) without and with this change and didn't see a significant difference in generation time (28.1s average without, 27.9s average with).
Since I'm set up to check, I'm going to see if factoring the flags out of the rules entirely (as illustrated above) makes any significant difference.
There was a problem hiding this comment.
The latest commit here now shows a first pass at pulling all of the options checks out of LogicExtensions as well, instead favoring determining which function to use in advance at the time of setting the rules.
The result of the 10x test run as I described above is 27.6 seconds average generation time, which is a negligible improvement.
I no longer particularly think this is worth pursuing - the performance benefits don't seem to be there - but I'm going to leave this PR up for now for posterity.
What is this fixing or adding?
As suggested in #4559 just before it was merged (see #4559 (review)), pulling the options checks directly out of the rules (here by using the established pattern of the world of caching the relevant options values in the world in LogicExtensions.py) should result in a performance improvement.
How was this tested?
Reran numerous generations and inspected the results.
If this makes graphical changes, please attach screenshots.
N/A