Timespinner: New options from TS Rando v1.25 + Logic fix #2090
Conversation
ThePhar
added
is: bug/fix
|
I have 2 comments:
lambda state: state.has('Water Mask', player) or not flooded.flood_lab)would most likely be faster when written as lambda state: not flooded.flood_lab or state.has('Water Mask', player))because the flood lookup is always faster and more likely to be false (because it'd be false for default settings AND for 67% of the default random flooding) EDIT: what would be even faster is setting the rule based on |
|
I agree on swapping those parameters, I did that also in the past. Timespinner is already rather fast on generation However setting the rule based on the flag, while fasterm it likely wont make the code any more readable and be a very high risk change as it should be done for all those logic rules. I don feel comfortable to make that change atm for the very small performance gain it will likely give |
|
Iv updated the parameter order, as for when to merge this. not up to me, but this PR was already created before 0.4.2 was live, its not a bugfix for something that added in 0.4.2 its a bugfix for things added in 0.4.1 |
worlds/timespinner/Regions.py
Outdated
| regions: Dict[str, Region] = {} | ||
|
|
||
| for name in region_names: | ||
| regions[name] = create_region(world, player, locations_per_region, name) |
There was a problem hiding this comment.
locations_per_region seems to be unused?
There was a problem hiding this comment.
its used, its passed on into the create_region method that will create the locations that belong to that region inside it
| @@ -39,21 +39,19 @@ class TimespinnerWorld(World): | |||
| option_definitions = timespinner_options | |||
| game = "Timespinner" | |||
| topology_present = True | |||
| data_version = 11 | |||
| data_version = 12 | |||
There was a problem hiding this comment.
would probably be better to just remove this?
There was a problem hiding this comment.
Not sure if we already can remove that?
Well the new options have been available as an .apworld for a couple on months now. didnt get complains about that, but hard to tell how much people actually used it As for the generation and optimizations i tested those locally be generating atleast 50 times using a random yaml to hit different combinations of options |
Co-authored-by: black-sliver <[email protected]>
…_mid_2023 # Conflicts: # worlds/timespinner/Regions.py
|
Drafted becase a potentional logic error was found |
|
Fixed small logic bug, put back in ready for review |
…MW#2090) Co-authored-by: black-sliver <[email protected]>
What is this fixing or adding?
A logic fix for Sealed Caves
2 new options
2 new floodable areas:
How was this tested?
Ran 100 generations locally, checked a few spoiler logs
Is Available as .apworld and has been run by a few community members
Included #2373 & #2374