Core: Speed up playthrough sphere culling #3890
Conversation
To determine if an item in a sphere is required, playthrough calculation would remove the item from its location and then use can_beat_game to sweep through locations until either the game can be beaten (the item is not required) or until the sweep runs out of reachable locations (the item is required). This patch changes how it is determined if an item is required. The advancement locations in each higher sphere that have already been culled must be reached for the game to be beatable, so iterate through the advancement locations in each higher sphere after attempting to cull an item. If one of the locations in a higher sphere is unreachable, then the game is unbeatable and the item is therefore required to beat the game. For items which are required to beat the game, this aborts faster than exhausting all reachable locations to determine that the game is unbeatable. For items which are not required to beat the game, the locations which should be reachable from the current sphere are known in advance, so this only has to check reachability of the locations in the closest higher sphere before moving onto the next closest sphere, rather than repeatedly checking all filled locations which have yet to be reached. This results in a faster playthrough calculation that also scales better than the previous implementation as the number of advancement items in the multiworld increases. This new implementation seems to not quite cull as many items on average as before, but I am unsure if this is an issue with the new implementation or an issue with the logic of the worlds being generated, e.g. missing indirect connections causing a region to not be reachable when it should be, which could cause a required location in a higher sphere to not be reachable when it should be, which would give a false positive that an item is required to beat the game. Playthrough calculation duration averaged over 5 generations of the same seed recorded before/s -> after/s. 57 unique games: 671.2 -> 133.5 (80.1% reduction) 28 unique games: 149.2 -> 34.9 (76.6% reduction) 14 unique games: 7.0 -> 3.1 (55.9% reduction) 7 unique games: 4.6 -> 2.5 (45.4% reduction)
github-actions
bot
added
affects: core
|
That's so smart god DAYUM |
|
Wait, I think I see the problem, let me think about this for one moment Edit: nvm |
|
Yeah I think this checks out Idk why there is weird behavior. Would be nice to have sth reproducible there |
NewSoupVi
added
the
is: enhancement
I think I see the reason now, and it's described by the comment in create_playthrough about how the pruning stage could have made certain items dependent on others in the same or later spheres. I generated a Hat in Time seed (with act randomizer disabled to make the spheres easier to understand) and in the initial sphere candidates, it would collect the item at Normally, reaching this location requires the In the original implementation, Edit: I should add that I modified both main and this PR to be deterministic to check this. |
|
Right, that makes sense. So the playthrough won't actually be locally minimal anymore? It could be that one of the items is merely in there to keep a location it its earliest possible sphere? I definitely think this is worth having, I'm just a bit unsure now as to whether it should be optional, and if yes, which one should be the default. I'd probably lean towards: optional (host.yaml), with the new method being the default, and the old code being called "locally minimal". Idk, I just think losing local minimal playthrough would be a shame, I think it's quite nice personally. Would love to hear more opinions though |
|
I agree with the new method being the default, and the old method kept around for legacy purposes. |
|
I think I may be able to turn this back into a 'true minimal' with some modifications. It won't be as fast the current 'minimal spheres', but it should still be faster than the original. The extra things I'm doing in the cull loop are:
Altering the spheres while culling is important to prevent the next cull attempt from having to carry along the same locations that are no longer reachable in their original spheres, due to a location being culled, but are reachable in a later sphere. I need to double check if this is correct and clean up the code if it is, but here's how it's looking by comparison: Playthrough only:
Edit: Best I can tell, this does indeed work and, when playthrough is modified to be deterministic, it produces the same results as the main branch. I've put this in its own branch for now. If the new method in this PR were to become the default, the implementation in this other branch may be able to replace the original method later on. Typically, locations don't get moved up too many spheres, but this was the worst I saw in my debugging (44 spheres required to beat this multiworld):
|
|
A version that's just faster with the same results should obviously be added so I'd definitely wanna see that!! But yeah I also still kinda like this "sphere preserving playthrough" as well |
Doesn't scale as well as before, but is still a good improvement. Because the spheres are re-arranged and new spheres are added while iterating, there is no longer a need to calculate the final spheres because the result is already in its final spheres. This should now match the original playthrough implementation. The type hint in create_paths has been changed from List[Set[Location]] to Collection[Set[Location]] to allow passing it a deque.
|
I've restored the original behaviour of allowing locations to move to later spheres and allowing new spheres to be created, and I've updated the PR description. As expected, it's a little slower and matching the original behaviour is more complicated. Because of the additional complexity, I deduplicated the code by turning it into a local function which can be used by both the sphere culling and the precollected item culling. I thought it was interesting to note that playthrough can also create entirely new spheres, so I added a debug log when new spheres get added. |
|
Nice! Ftr, there does still seem to be some big interest in the original version of this, so keep that code around too for a separate PR maybe if you want :3 |
|
It looks like playthrough is another thing that would benefit from knowing which worlds each world logically depends on. If it is assumed that each world only logically depends on its own items/locations/etc. then immediately after each cull attempt, only the locations belonging to the player the culled item belongs to need to be checked. For each of their locations that became unreachable, the item at that location also becomes unreachable, so the owning player of that item could then also have unreachable locations in a higher sphere, so their locations will also have to be checked from the next sphere onwards. So, start with a
When it is knowable which worlds each world logically depends on it would be more like:
|
This was unnecessary because locations are removed from their spheres instead. This fixes issues that arise from the fact that some worlds have logic that changes when a specific item is at a specific location.
…cull # Conflicts: # BaseClasses.py
What is this fixing or adding?
To determine if an item in a sphere is required, playthrough calculation would remove the item from its location and then use can_beat_game to sweep through locations until either the game can be beaten (the item is not required) or until the sweep runs out of reachable locations (the item is required).
This patch changes how it is determined if an item is required.
The advancement locations in each higher sphere that have already been culled must be reached for the game to be beatable, so iterate through the advancement locations in each higher sphere after attempting to cull an item. If all locations in a higher sphere and all unreachable locations carried over from a lower sphere are unreachable, then the game is unbeatable and the item is therefore required to beat the game.
For items which are required to beat the game, this aborts faster than exhausting all reachable locations to determine that the game is unbeatable.
For items which are not required to beat the game, the locations which should be reachable from the current sphere are known in advance, so this only has to check reachability of the locations in the closest higher sphere, and any unreachable locations carried over from lower spheres, before moving onto the next closest sphere, rather than repeatedly checking all filled locations which have yet to be reached.
This results in a faster playthrough calculation that also scales better than the previous implementation as the number of advancement items in the multiworld increases.
Playthrough calculation duration averaged over 5 generations of the same seed recorded before/s -> after/s.
57 unique games: 671.2 -> 179.5 (73.3% reduction)
28 unique games: 149.2 -> 47.8 (68.0% reduction)
14 unique games: 7.0 -> 4.2 (39.4% reduction)
7 unique games: 4.6 -> 3.4 (26.8% reduction)
Because the spheres are re-arranged and new spheres are added while iterating, there is no longer a need to calculate the final spheres because the result is already in its final spheres.
The type hint in create_paths has been changed from List[Set[Location]] to Collection[Set[Location]] to allow passing it a deque.
How was this tested?
Generated with
--seed 1at d65863f andspoiler: 2in host.yaml and averaged over 5 generations.Playthrough only:
Full generation duration: