From a45e8730cb5bb523d80be2cdcd365e350bb5b82f Mon Sep 17 00:00:00 2001 From: black-sliver <59490463+black-sliver@users.noreply.github.com> Date: Sun, 25 Jun 2023 02:55:13 +0200 Subject: [PATCH] Fill: fix fill_restrictive for mixed minimal and non-minimal and test (#1800) * Tests: add test for mixing minimal and non-minimal * Tests: minor cleanup in test_minimal_mixed_fill * fix fill_restrictive for mixed minimal/non-minimal The reason why this only happens for minimal is because it would not accept the solution it found otherwise. Tracking and releasing unreachable items would be the better solution, but that's a lot harder to do. * fix typo in fill_restrictive * fix pep8 in fill_restrictive * Fill: cleanup invalid unsafe placements, better comments * Fill: more cleanup --- Fill.py | 38 +++++++++++++++++++++++++++----------- test/general/TestFill.py | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 11 deletions(-) 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)