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.

Sign up for GitHub

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

Timespinner: move options checks out of rules #4708

Closed
wants to merge 3 commits into from
Closed

Timespinner: move options checks out of rules #4708

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1e94762
Timespinner: move options checks out of rules
Mar 8, 2025
1d272fd
Timespinner: add missing flag_inverted to LogicExtensions
Mar 8, 2025
923483e
Timespinner: move options checks out of LogicExtensions rules
Mar 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 11 additions & 11 deletions worlds/timespinner/Locations.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 ),
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

LocationData('The lab', 'Lab: Below lab entrance', 1337069, logic.has_doublejump),
LocationData('The lab (power off)', 'Lab: Trash jump room', 1337070, lambda state: not options.lock_key_amadeus or logic.has_doublejump_of_npc(state) ),
LocationData('The lab (power off)', 'Lab: Dynamo Works', 1337071, lambda state: not options.lock_key_amadeus or (state.has_all(('Lab Access Research', 'Lab Access Dynamo'), player)) ),
LocationData('The lab (power off)', 'Lab: Trash jump room', 1337070, lambda state: not logic.lock_key_amadeus_enabled() or logic.has_doublejump_of_npc(state) ),
LocationData('The lab (power off)', 'Lab: Dynamo Works', 1337071, lambda state: not logic.lock_key_amadeus_enabled() or (state.has_all(('Lab Access Research', 'Lab Access Dynamo'), player)) ),
LocationData('The lab (upper)', 'Lab: Genza (Blob Mom)', 1337072),
LocationData('The lab (power off)', 'Lab: Experiment #13', 1337073, lambda state: not options.lock_key_amadeus or state.has('Lab Access Experiment', player) ),
LocationData('The lab (power off)', 'Lab: Experiment #13', 1337073, lambda state: not logic.lock_key_amadeus_enabled() or state.has('Lab Access Experiment', player) ),
LocationData('The lab (upper)', 'Lab: Download and chest room chest', 1337074),
LocationData('The lab (upper)', 'Lab: Lab secret', 1337075, logic.can_break_walls),
LocationData('The lab (power off)', 'Lab: Spider Hell', 1337076, lambda state: logic.has_keycard_A and not options.lock_key_amadeus or state.has('Lab Access Research', player)),
LocationData('The lab (power off)', 'Lab: Spider Hell', 1337076, lambda state: logic.has_keycard_A and not logic.lock_key_amadeus_enabled() or state.has('Lab Access Research', player)),
LocationData('Emperors tower', 'Emperor\'s Tower: Courtyard bottom chest', 1337077),
LocationData('Emperors tower', 'Emperor\'s Tower: Courtyard floor secret', 1337078, lambda state: logic.has_upwarddash(state) and logic.can_break_walls(state)),
LocationData('Emperors tower', 'Emperor\'s Tower: Courtyard upper chest', 1337079, lambda state: logic.has_upwarddash(state)),
Expand Down Expand Up @@ -203,7 +203,7 @@ def get_location_datas(player: Optional[int], options: Optional[TimespinnerOptio

# 1337156 - 1337170 Downloads
if not options or options.downloadable_items:
location_table += (
location_table += (
LocationData('Library', 'Library: Terminal 2 (Lachiem)', 1337156, lambda state: state.has('Tablet', player)),
LocationData('Library', 'Library: Terminal 1 (Windaria)', 1337157, lambda state: state.has('Tablet', player)),
# 1337158 Is lost in time
Expand All @@ -214,11 +214,11 @@ def get_location_datas(player: Optional[int], options: Optional[TimespinnerOptio
LocationData('Library top', 'Library: Backer room terminal (Vandagray Metropolis Map)', 1337163, lambda state: state.has('Tablet', player)),
LocationData('Varndagroth tower right (elevator)', 'Varndagroth Towers (Right): Medbay terminal (Bleakness Research)', 1337164, lambda state: state.has('Tablet', player) and logic.has_keycard_B(state)),
LocationData('The lab (upper)', 'Lab: Download and chest room terminal (Experiment #13)', 1337165, lambda state: state.has('Tablet', player)),
LocationData('The lab (power off)', 'Lab: Middle terminal (Amadeus Laboratory Map)', 1337166, lambda state: state.has('Tablet', player) and (not options.lock_key_amadeus or state.has('Lab Access Research', player))),
LocationData('The lab (power off)', 'Lab: Sentry platform terminal (Origins)', 1337167, lambda state: state.has('Tablet', player) and (not options.lock_key_amadeus or state.has('Lab Access Genza', player) or logic.can_teleport_to(state, "Time", "GateDadsTower"))),
LocationData('The lab (power off)', 'Lab: Middle terminal (Amadeus Laboratory Map)', 1337166, lambda state: state.has('Tablet', player) and (not logic.lock_key_amadeus_enabled() or state.has('Lab Access Research', player))),
LocationData('The lab (power off)', 'Lab: Sentry platform terminal (Origins)', 1337167, lambda state: state.has('Tablet', player) and (not logic.lock_key_amadeus_enabled() or state.has('Lab Access Genza', player) or logic.can_teleport_to(state, "Time", "GateDadsTower"))),
LocationData('The lab', 'Lab: Experiment 13 terminal (W.R.E.C Farewell)', 1337168, lambda state: state.has('Tablet', player)),
LocationData('The lab', 'Lab: Left terminal (Biotechnology)', 1337169, lambda state: state.has('Tablet', player)),
LocationData('The lab (power off)', 'Lab: Right terminal (Experiment #11)', 1337170, lambda state: state.has('Tablet', player) and (not options.lock_key_amadeus or state.has('Lab Access Research', player)))
LocationData('The lab (power off)', 'Lab: Right terminal (Experiment #11)', 1337170, lambda state: state.has('Tablet', player) and (not logic.lock_key_amadeus_enabled() or state.has('Lab Access Research', player)))
)

# 1337176 - 1337176 Cantoran
Expand Down Expand Up @@ -257,7 +257,7 @@ def get_location_datas(player: Optional[int], options: Optional[TimespinnerOptio
# 1337199 - 1337232 Reserved for future use

# 1337233 - 1337235 Pyramid Start checks
if not options or options.pyramid_start:
if not options or options.pyramid_start:
location_table += (
LocationData('Ancient Pyramid (entrance)', 'Dark Forest: Training Dummy', 1337233),
LocationData('Ancient Pyramid (entrance)', 'Temporal Gyre: Forest Entrance', 1337234, lambda state: logic.has_upwarddash(state) or logic.can_teleport_to(state, "Time", "GateGyre")),
Expand All @@ -279,5 +279,5 @@ def get_location_datas(player: Optional[int], options: Optional[TimespinnerOptio
LocationData('Ifrit\'s Lair', 'Ifrit: Pre fight', 1337244),
LocationData('Ifrit\'s Lair', 'Ifrit: Post fight (chest)', 1337245),
)

return location_table
26 changes: 26 additions & 0 deletions worlds/timespinner/LogicExtensions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,12 @@ class TimespinnerLogic:
flag_unchained_keys: bool
flag_eye_spy: bool
flag_specific_keycards: bool
flag_enter_sandman: bool
flag_back_to_the_future: bool
flag_prism_break: bool
flag_lock_key_amadeus: bool
flag_gate_keep: bool
flag_royal_roadblock: bool
pyramid_keys_unlock: Optional[str]
present_keys_unlock: Optional[str]
past_keys_unlock: Optional[str]
Expand All @@ -23,7 +28,13 @@ def __init__(self, player: int, options: Optional[TimespinnerOptions],
self.flag_specific_keycards = bool(options and options.specific_keycards)
self.flag_eye_spy = bool(options and options.eye_spy)
self.flag_unchained_keys = bool(options and options.unchained_keys)
self.flag_enter_sandman = bool(options and options.enter_sandman)
self.flag_back_to_the_future = bool(options and options.back_to_the_future)
self.flag_prism_break = bool(options and options.prism_break)
self.flag_lock_key_amadeus = bool(options and options.lock_key_amadeus)
self.flag_pyramid_start = bool(options and options.pyramid_start)
self.flag_gate_keep = bool(options and options.gate_keep)
self.flag_royal_roadblock = bool(options and options.gate_keep)

if precalculated_weights:
if self.flag_unchained_keys:
Expand Down Expand Up @@ -113,3 +124,18 @@ def can_teleport_to(self, state: CollectionState, era: str, gate: str) -> bool:
return self.time_keys_unlock == gate and state.has("Mysterious Warp Beacon", self.player)
else:
raise Exception("Invallid Era: {}".format(era))

def has_pyramid_warp(self, state: CollectionState) -> bool:
return self.can_teleport_to(state, "Time", "GateGyre") or self.can_teleport_to(state, "Time", "GateLeftPyramid") or (not self.flag_unchained_keys and self.flag_enter_sandman)

def has_present_access_from_refugee_camp(self, state: CollectionState) -> bool:
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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


def can_traverse_drawbridge(self, state: CollectionState) -> bool:
return not self.flag_gate_keep or state.has('Drawbridge Key', self.player) or self.has_upwarddash(state)

def can_open_royal_towers_door(self, state: CollectionState) -> bool:
return not self.flag_royal_roadblock or self.has_pink(state)
20 changes: 10 additions & 10 deletions worlds/timespinner/Regions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,14 +77,14 @@ def create_regions_and_locations(world: MultiWorld, player: int, options: Timesp
connect(world, player, 'Lake desolation', 'Space time continuum', logic.has_teleport)
connect(world, player, 'Upper lake desolation', 'Lake desolation')
connect(world, player, 'Upper lake desolation', 'Eastern lake desolation')
connect(world, player, 'Lower lake desolation', 'Lake desolation')
connect(world, player, 'Lower lake desolation', 'Lake desolation')
connect(world, player, 'Lower lake desolation', 'Eastern lake desolation')
connect(world, player, 'Eastern lake desolation', 'Space time continuum', logic.has_teleport)
connect(world, player, 'Eastern lake desolation', 'Library')
connect(world, player, 'Eastern lake desolation', 'Lower lake desolation')
connect(world, player, 'Eastern lake desolation', 'Upper lake desolation', lambda state: logic.has_fire(state) and state.can_reach('Upper Lake Serene', 'Region', player), "Upper Lake Serene")
connect(world, player, 'Library', 'Eastern lake desolation')
connect(world, player, 'Library', 'Library top', lambda state: logic.has_doublejump(state) or state.has('Talaria Attachment', player))
connect(world, player, 'Library', 'Library top', lambda state: logic.has_doublejump(state) or state.has('Talaria Attachment', player))
connect(world, player, 'Library', 'Varndagroth tower left', logic.has_keycard_D)
connect(world, player, 'Library', 'Space time continuum', logic.has_teleport)
connect(world, player, 'Library top', 'Library')
Expand Down Expand Up @@ -112,9 +112,9 @@ def create_regions_and_locations(world: MultiWorld, player: int, options: Timesp
connect(world, player, 'Military Fortress (hangar)', 'The lab', lambda state: logic.has_keycard_B(state) and (state.has('Water Mask', player) if flooded.flood_lab else logic.has_doublejump(state)))
connect(world, player, 'Temporal Gyre', 'Military Fortress')
connect(world, player, 'The lab', 'Military Fortress')
connect(world, player, 'The lab', 'The lab (power off)', lambda state: options.lock_key_amadeus or logic.has_doublejump_of_npc(state))
connect(world, player, 'The lab', 'The lab (power off)', lambda state: logic.lock_key_amadeus_enabled() or logic.has_doublejump_of_npc(state))
connect(world, player, 'The lab (power off)', 'The lab', lambda state: not flooded.flood_lab or state.has('Water Mask', player))
connect(world, player, 'The lab (power off)', 'The lab (upper)', lambda state: logic.has_forwarddash_doublejump(state) and ((not options.lock_key_amadeus) or state.has('Lab Access Genza', player)))
connect(world, player, 'The lab (power off)', 'The lab (upper)', lambda state: logic.has_forwarddash_doublejump(state) and ((not logic.lock_key_amadeus_enabled()) or state.has('Lab Access Genza', player)))
connect(world, player, 'The lab (upper)', 'The lab (power off)')
connect(world, player, 'The lab (upper)', 'Emperors tower', logic.has_forwarddash_doublejump)
connect(world, player, 'The lab (upper)', 'Ancient Pyramid (entrance)', lambda state: state.has_all({'Timespinner Wheel', 'Timespinner Spindle', 'Timespinner Gear 1', 'Timespinner Gear 2', 'Timespinner Gear 3'}, player))
Expand All @@ -125,12 +125,12 @@ def create_regions_and_locations(world: MultiWorld, player: int, options: Timesp
connect(world, player, 'Sealed Caves (Xarion)', 'Skeleton Shaft')
connect(world, player, 'Sealed Caves (Xarion)', 'Space time continuum', logic.has_teleport)
connect(world, player, 'Refugee Camp', 'Forest')
connect(world, player, 'Refugee Camp', 'Library', lambda state: (options.pyramid_start or options.inverted) and options.back_to_the_future and state.has_all({'Timespinner Wheel', 'Timespinner Spindle'}, player))
connect(world, player, 'Refugee Camp', 'Library', logic.has_present_access_from_refugee_camp)
connect(world, player, 'Refugee Camp', 'Space time continuum', logic.has_teleport)
connect(world, player, 'Forest', 'Refugee Camp')
connect(world, player, 'Forest', 'Left Side forest Caves', lambda state: flooded.flood_lake_serene_bridge or state.has('Talaria Attachment', player) or logic.has_timestop(state))
connect(world, player, 'Forest', 'Caves of Banishment (Sirens)')
connect(world, player, 'Forest', 'Castle Ramparts', lambda state: not options.gate_keep or state.has('Drawbridge Key', player) or logic.has_upwarddash(state))
connect(world, player, 'Forest', 'Castle Ramparts', logic.can_traverse_drawbridge)
connect(world, player, 'Left Side forest Caves', 'Forest')
connect(world, player, 'Left Side forest Caves', 'Upper Lake Serene', logic.has_timestop)
connect(world, player, 'Left Side forest Caves', 'Lower Lake Serene', lambda state: not flooded.flood_lake_serene or state.has('Water Mask', player))
Expand All @@ -152,7 +152,7 @@ def create_regions_and_locations(world: MultiWorld, player: int, options: Timesp
connect(world, player, 'Castle Ramparts', 'Space time continuum', logic.has_teleport)
connect(world, player, 'Castle Keep', 'Castle Ramparts')
connect(world, player, 'Castle Keep', 'Castle Basement', lambda state: not flooded.flood_basement or state.has('Water Mask', player))
connect(world, player, 'Castle Keep', 'Royal towers (lower)', lambda state: logic.has_doublejump(state) and (not options.royal_roadblock or logic.has_pink(state)))
connect(world, player, 'Castle Keep', 'Royal towers (lower)', lambda state: logic.has_doublejump(state) and logic.can_open_royal_towers_door(state))
connect(world, player, 'Castle Keep', 'Space time continuum', logic.has_teleport)
connect(world, player, 'Royal towers (lower)', 'Castle Keep')
connect(world, player, 'Royal towers (lower)', 'Royal towers', lambda state: state.has('Timespinner Wheel', player) or logic.has_forwarddash_doublejump(state))
Expand Down Expand Up @@ -185,7 +185,7 @@ def create_regions_and_locations(world: MultiWorld, player: int, options: Timesp
connect(world, player, 'Space time continuum', 'Caves of Banishment (upper)', lambda state: logic.can_teleport_to(state, "Past", "GateCavesOfBanishment"))
connect(world, player, 'Space time continuum', 'Military Fortress (hangar)', lambda state: logic.can_teleport_to(state, "Present", "GateLabEntrance"))
connect(world, player, 'Space time continuum', 'The lab (upper)', lambda state: logic.can_teleport_to(state, "Present", "GateDadsTower"))
connect(world, player, 'Space time continuum', 'Ancient Pyramid (entrance)', lambda state: logic.can_teleport_to(state, "Time", "GateGyre") or logic.can_teleport_to(state, "Time", "GateLeftPyramid") or (not options.unchained_keys and options.enter_sandman))
connect(world, player, 'Space time continuum', 'Ancient Pyramid (entrance)', logic.has_pyramid_warp)
connect(world, player, 'Space time continuum', 'Ancient Pyramid (right)', lambda state: logic.can_teleport_to(state, "Time", "GateRightPyramid"))

if options.gyre_archives:
Expand Down Expand Up @@ -231,7 +231,7 @@ def connectStartingRegion(world: MultiWorld, player: int, options: TimespinnerOp
tutorial = world.get_region('Tutorial', player)
space_time_continuum = world.get_region('Space time continuum', player)

if options.pyramid_start:
if options.pyramid_start:
starting_region = world.get_region('Ancient Pyramid (entrance)', player)
elif options.inverted:
starting_region = world.get_region('Refugee Camp', player)
Expand All @@ -249,7 +249,7 @@ def connectStartingRegion(world: MultiWorld, player: int, options: TimespinnerOp
space_time_continuum.exits.append(teleport_back_to_start)


def connect(world: MultiWorld, player: int, source: str, target: str,
def connect(world: MultiWorld, player: int, source: str, target: str,
rule: Optional[Callable[[CollectionState], bool]] = None,
indirect: str = ""):

Expand Down
Loading