diff --git a/Fill.py b/Fill.py index 6fa5ecb00d00..c6fb5c546f38 100644 --- a/Fill.py +++ b/Fill.py @@ -39,8 +39,9 @@ def fill_restrictive(world: MultiWorld, base_state: CollectionState, locations: """ unplaced_items: typing.List[Item] = [] placements: typing.List[Location] = [] + cleanup_required = False - swapped_items: typing.Counter[typing.Tuple[int, str]] = Counter() + swapped_items: typing.Counter[typing.Tuple[int, str, bool]] = Counter() reachable_items: typing.Dict[int, typing.Deque[Item]] = {} for item in item_pool: reachable_items.setdefault(item.player, deque()).append(item) @@ -84,25 +85,28 @@ def fill_restrictive(world: MultiWorld, base_state: CollectionState, locations: else: # we filled all reachable spots. if swap: - # try swapping this item with previously placed items - for (i, location) in enumerate(placements): + # try swapping this item with previously placed items in a safe way then in an unsafe way + swap_attempts = ((i, location, unsafe) + for unsafe in (False, True) + for i, location in enumerate(placements)) + for (i, location, unsafe) in swap_attempts: 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 - swap_count = swapped_items[placed_item.player, - placed_item.name] + swap_count = swapped_items[placed_item.player, placed_item.name, unsafe] if swap_count > 1: continue location.item = None placed_item.location = None - swap_state = sweep_from_pool(base_state, [placed_item]) - # swap_state assumes we can collect placed item before item_to_place + swap_state = sweep_from_pool(base_state, [placed_item] if unsafe else []) + # unsafe means swap_state assumes we can somehow collect placed_item before item_to_place + # by continuing to swap, which is not guaranteed. This is unsafe because there is no mechanic + # to clean that up later, so there is a chance generation fails. 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, which could happen with rules - # that want to not have both items. Left in until removal is proven useful. + # Verify placing this item won't reduce available locations, which would be a useless swap. prev_state = swap_state.copy() prev_loc_count = len( world.get_reachable_locations(prev_state)) @@ -117,13 +121,15 @@ def fill_restrictive(world: MultiWorld, base_state: CollectionState, locations: spot_to_fill = placements.pop(i) swap_count += 1 - swapped_items[placed_item.player, - placed_item.name] = swap_count + swapped_items[placed_item.player, placed_item.name, unsafe] = swap_count reachable_items[placed_item.player].appendleft( placed_item) item_pool.append(placed_item) + # cleanup at the end to hopefully get better errors + cleanup_required = True + break # Item can't be placed here, restore original item @@ -144,6 +150,16 @@ def fill_restrictive(world: MultiWorld, base_state: CollectionState, locations: if on_place: on_place(spot_to_fill) + if cleanup_required: + # validate all placements and remove invalid ones + for placement in placements: + state = sweep_from_pool(base_state, []) + if world.accessibility[placement.item.player] != "minimal" and not placement.can_reach(state): + placement.item.location = None + unplaced_items.append(placement.item) + placement.item = None + locations.append(placement) + if allow_excluded: # check if partial fill is the result of excluded locations, in which case retry excluded_locations = [ diff --git a/test/general/TestFill.py b/test/general/TestFill.py index 492f3e2f8360..66fc3b2f598c 100644 --- a/test/general/TestFill.py +++ b/test/general/TestFill.py @@ -199,6 +199,41 @@ def test_minimal_fill(self): # Unnecessary unreachable Item self.assertEqual(locations[1].item, items[0]) + def test_minimal_mixed_fill(self): + """ + Test that fill for 1 minimal and 1 non-minimal player will correctly place items in a way that lets + the non-minimal player get all items. + """ + + multi_world = generate_multi_world(2) + player1 = generate_player_data(multi_world, 1, 3, 3) + player2 = generate_player_data(multi_world, 2, 3, 3) + + multi_world.accessibility[player1.id].value = multi_world.accessibility[player1.id].option_minimal + multi_world.accessibility[player2.id].value = multi_world.accessibility[player2.id].option_locations + + multi_world.completion_condition[player1.id] = lambda state: True + multi_world.completion_condition[player2.id] = lambda state: state.has(player2.prog_items[2].name, player2.id) + + set_rule(player1.locations[1], lambda state: state.has(player1.prog_items[0].name, player1.id)) + set_rule(player1.locations[2], lambda state: state.has(player1.prog_items[1].name, player1.id)) + set_rule(player2.locations[1], lambda state: state.has(player2.prog_items[0].name, player2.id)) + set_rule(player2.locations[2], lambda state: state.has(player2.prog_items[1].name, player2.id)) + + # force-place an item that makes it impossible to have all locations accessible + player1.locations[0].place_locked_item(player1.prog_items[2]) + + # fill remaining locations with remaining items + location_pool = player1.locations[1:] + player2.locations + item_pool = player1.prog_items[:-1] + player2.prog_items + fill_restrictive(multi_world, multi_world.state, location_pool, item_pool) + multi_world.state.sweep_for_events() # collect everything + + # all of player2's locations and items should be accessible (not all of player1's) + for item in player2.prog_items: + self.assertTrue(multi_world.state.has(item.name, player2.id), + f'{item} is unreachable in {item.location}') + def test_reversed_fill(self): multi_world = generate_multi_world() player1 = generate_player_data(multi_world, 1, 2, 2)