Don't swap items that reduce access (#247)

Fill.py

@@ -14,7 +14,7 @@ class FillError(RuntimeError):
     pass
 
 
-def sweep_from_pool(base_state: CollectionState, itempool):
+def sweep_from_pool(base_state: CollectionState, itempool=[]):
     new_state = base_state.copy()
     for item in itempool:
         new_state.collect(item, True)
@@ -25,7 +25,7 @@ def sweep_from_pool(base_state: CollectionState, itempool):
 def fill_restrictive(world: MultiWorld, base_state: CollectionState, locations, itempool: typing.List[Item],
                      single_player_placement=False, lock=False):
     unplaced_items = []
-    placements = []
+    placements: typing.List[Location] = []
 
     swapped_items = Counter()
     reachable_items: typing.Dict[int, deque] = {}
@@ -39,6 +39,7 @@ def fill_restrictive(world: MultiWorld, base_state: CollectionState, locations,
         for item in items_to_place:
             itempool.remove(item)
         maximum_exploration_state = sweep_from_pool(base_state, itempool)
+
         has_beaten_game = world.has_beaten_game(maximum_exploration_state)
 
         for item_to_place in items_to_place:
@@ -64,23 +65,42 @@ def fill_restrictive(world: MultiWorld, base_state: CollectionState, locations,
                     placed_item = location.item
                     # Unplaceable items can sometimes be swapped infinitely. Limit the
                     # number of times we will swap an individual item to prevent this
-                    if swapped_items[placed_item.player, placed_item.name] > 0:
+                    swap_count = swapped_items[placed_item.player,
+                                               placed_item.name]
+                    if swap_count > 1:
                         continue
+
                     location.item = None
                     placed_item.location = None
-                    swap_state = sweep_from_pool(base_state, itempool)
+                    swap_state = sweep_from_pool(base_state)
                     if (not single_player_placement or location.player == item_to_place.player) \
                             and location.can_fill(swap_state, item_to_place, perform_access_check):
+
+                        # Verify that placing this item won't reduce available locations
+                        prev_state = swap_state.copy()
+                        prev_state.collect(placed_item)
+                        prev_loc_count = len(
+                            world.get_reachable_locations(prev_state))
+
+                        swap_state.collect(item_to_place, True)
+                        new_loc_count = len(
+                            world.get_reachable_locations(swap_state))
+
+                        if new_loc_count >= prev_loc_count:
                             # Add this item to the existing placement, and
                             # add the old item to the back of the queue
                             spot_to_fill = placements.pop(i)
+
+                            swap_count += 1
                             swapped_items[placed_item.player,
-                                      placed_item.name] += 1
+                                          placed_item.name] = swap_count
+
                             reachable_items[placed_item.player].appendleft(
                                 placed_item)
                             itempool.append(placed_item)
+
                             break
-                    else:
+
                     # Item can't be placed here, restore original item
                     location.item = placed_item
                     placed_item.location = location

test/general/TestFill.py

@@ -115,6 +115,10 @@ def generate_items(count: int, player_id: int, advancement: bool = False, code:
     return items
 
 
+def names(objs: list) -> List[str]:
+    return map(lambda o: o.name, objs)
+
+
 class TestFillRestrictive(unittest.TestCase):
     def test_basic_fill(self):
         multi_world = generate_multi_world()
@@ -331,6 +335,28 @@ class TestFillRestrictive(unittest.TestCase):
         self.assertEqual(player2.locations[0].item, player1.prog_items[0])
         self.assertEqual(player2.locations[1].item, player1.prog_items[1])
 
+    def test_restrictive_progress(self):
+        multi_world = generate_multi_world()
+        player1 = generate_player_data(multi_world, 1, prog_item_count=25)
+        items = player1.prog_items.copy()
+        multi_world.completion_condition[player1.id] = lambda state: state.has_all(
+            names(player1.prog_items), player1.id)
+
+        region1 = player1.generate_region(player1.menu, 5)
+        region2 = player1.generate_region(player1.menu, 5, lambda state: state.has_all(
+            names(items[2:7]), player1.id))
+        region3 = player1.generate_region(player1.menu, 5, lambda state: state.has_all(
+            names(items[7:12]), player1.id))
+        region4 = player1.generate_region(player1.menu, 5, lambda state: state.has_all(
+            names(items[12:17]), player1.id))
+        region5 = player1.generate_region(player1.menu, 5, lambda state: state.has_all(
+            names(items[17:22]), player1.id))
+
+        locations = multi_world.get_unfilled_locations()
+
+        fill_restrictive(multi_world, multi_world.state,
+                         locations, player1.prog_items)
+
 
 class TestDistributeItemsRestrictive(unittest.TestCase):
     def test_basic_distribute(self):