Core: Plando Items "rewrite"

[Silvris]

What is this fixing or adding?

This PR is the combination of three major changes to plando items.

• Moves plando items to the options api as a member of PerGameCommonOptions
• Changes the placement of items to use fill_restrictive, which means plando should be capable of avoiding placements that can never result in a playable world
• Plando now evaluates item and location groups.

How was this tested?

Manually via generating games with yamls using plando items. Opening as draft since any game using pre_fill will need thorough testing with this approach (KDL3 was already found to be bugged).

Ran unittests, but more unittests should likely be added.

[Exempt-Medic]

It seems like this removes the ability to have plando blocks without any locations listed. For example, so that specific items end up in specific worlds. Is the intended workaround to use Everywhere?

[Silvris]

It seems like this removes the ability to have plando blocks without any locations listed. For example, so that specific items end up in specific worlds. Is the intended workaround to use Everywhere?

I believe I missed this usecase (plando items guide doesn't mention it, and it's built into get_unfilled_locations_for_players so I couldn't tell it from the plando code).

[BadMagic100]

Disclaimer: I have not looked at the code yet

It might be worth considering and documenting/fixing possible negative interactions with #3032 if you haven't already

I'm also curious about how pre_fill is impacted as it may affect generic ER

[beauxq]

Some people use plando when they WANT to make something that will be seen as unbeatable according to the logic. Will this still be possible with this change?

[Silvris]

Some people use plando when they WANT to make something that will be seen as unbeatable according to the logic. Will this still be possible with this change?

It might still be possible, but current methods (such as locking the item behind a location that logically requires it) will no longer work. It should be noted that this doesn't prevent impossible combinations (that usually end in fill error) such as filling your entire sphere 1 with junk.

[Silvris]

Disclaimer: I have not looked at the code yet

It might be worth considering and documenting/fixing possible negative interactions with #3032 if you haven't already

I'm also curious about how pre_fill is impacted as it may affect generic ER

This should be fine, as we use the actual instance of the item instead of creating a new item and then removing using that. It's definitely worth testing though.

It appears that any world that uses pre_fill to place items that are not in the itempool must also define get_pre_fill_items so that the state used for plando can account for those items.

[Silvris]

List of worlds that implement pre_fill and not get_pre_fill_items:

• [X] Adventure (does not place progression in prefill)
• [X] Blasphemous
• [X] Castlevania 64 (does not place progression in prefill)
• [X] Hylics 2
• [X] KH2
• [X] KDL3 (#3323)
• [X] Pokemon Emerald
• [X] Pokemon RB
• [X] Shivers
• [X] Super Metroid (does not place progression)

Worlds that implement get_pre_fill_items but do not actually include all pre_fill items

• [X] A Link to the Past
• [X] Raft (very funny)

I will need to check all of these later to confirm if this is the proper reason for some worlds having issues.

[Exempt-Medic]

Just putting this comment here as well, I feel the proper fix for Blasphemous is to remove pre_fill entirely and have these option-based item placements happen before plando. Such as be removing the pre_fill defining line so they fall into create_items instead.

This would also fix a bug in Blasphemous since it is defining local items in pre_fill which doesn't work because it is after locality_rules and would also prevent this oddity from happening as well:

Local Items:                     Thorn Upgrade
Non-local Items:                 Thorn Upgrade

Ya know what, I'm just gonna open a PR for this instead.

Opened that now https://github.com/ArchipelagoMW/Archipelago/pull/3901

[Exempt-Medic]

I believe the Blasphemous changes can now be removed

[JaredWeakStrike]

lgtm

[Exempt-Medic]

Just a reminder that my comments have not been addressed

[Exempt-Medic]

This breaks The Witness btw @NewSoupVi

line 168, in get_early_items
    if plando_setting.get("from_pool", True):
       ^^^^^^^^^^^^^^^^^^
AttributeError: 'PlandoItem' object has no attribute 'get'

[Exempt-Medic]

Several valid configurations for locations are no longer valid and throw unhashability errors.

Update: Turns out these were barely supported beforehand, and, imo, were misleading to the player. I think removing locations as a dictionary entirely makes sense. Just gotta fix up the for it and throw an error if it is a dictionary

[NewSoupVi]

This breaks The Witness btw

I've had a PR open for a bit that removes all the plando checking code from Witness https://github.com/ArchipelagoMW/Archipelago/pull/3041, and then replaced it by another newer PR. https://github.com/ArchipelagoMW/Archipelago/pull/3804/

However, this PR has had conflicts for a bit, so I'll get on fixing that.

[Exempt-Medic]

I opened a PR including/addressing my comments and also some discussion in ap-core-dev on the item group unrolling methods

[NewSoupVi]

I'm going to put my concerns on this PR so that they're documented somewhere.

I don't like that this is trying to make "Event Plando" work.

At first, it looks like one upside and one downside. The upside is: World devs now have an easy way to make "Dungeon Reward Plando" work. The downside is: We can't do validation ahead of time using location_name_to_id and item_name_to_id, and the plando code has to be bigger.

But I don't even actually think the "upside" is a genuine upside. Any given instance of something that "wants" Event Plando can be realized through a custom option. We could even make a cute new option class like "PlandoMatchingOption" that takes two sets of valid keys to match to each other. Having it in ItemPlando looks nice because it makes ItemPlando look "powerful" and provides an out-of-the-box way to do this kind of thing immediately, it's a cute extra feature and I'm sure it was very fun to add this little cherry ontop, but I don't like it at all.

1. Breaking different behavior into multiple options is a good thing. The fact that we're talking about one set of items being able to be plandoed on one set of locations, and then a different set of items only being plandoed on a different set of locations, but we are realizing this through one option with one combined syntax and one combined docstring that now has to make sure the user isn't trying to put one type of item on the other type of location, rings an alarm bell for me. Heck, what if a game has two sets of events that can't go onto each other? Like in OoT, Local Songs and local Dungeon Rewards. Should they override the plando code somehow or realize this through item rules? My opinion is: Stop. These should be separate options
2. This is the much bigger point. In terms of how we design core features, hooking into the generic player-facing options for custom world features should be discouraged. These options should be as simple as possible and behave consistently across games, and we should be encouraging people to make their own custom options if they want custom behavior. If there is a common use case that is hard to realize, we make a new generic option class to subclass. We should not be effectively saying: One world's plando_items should work different from another's, and that's fine.

I am extremely uncomfortable with the world where a world dev says "I want to shuffle my major event items, but I also want the user to be able to plando them" and the suggested solution is "Use ItemPlando, and subclass it with a docstring to explain it", and suddenly we have 5 games that rely on this behavior and can never go back on it. And then we realize we didn't account for "world: null" or something. No no no no no :( It should be "Here's how you can make your own custom plando option for your local randomization"

This is essentially me saying: I do not approve. However, Berserker has already approved the PR, so maybe this is too late or I'm outvoted here.

[Silvris]

I don't like that this is trying to make "Event Plando" work.

The problem is that event plando is already a usecase supported by core and is being used by worlds. Removing it requires adjusting even more worlds, or removing functionality.

[Silvris]

The downside is: We can't do validation ahead of time using location_name_to_id and item_name_to_id, and the plando code has to be bigger.

This is already the case with plando, not every item/location that we can plando to are in datapackage.

[Exempt-Medic]

The downside is: We can't do validation ahead of time using location_name_to_id and item_name_to_id, and the plando code has to be bigger.

This is already the case with plando, not every item/location that we can plando to are in datapackage.

Aren't the only exceptions to this events?

[NewSoupVi]

The problem is that event plando is already a usecase supported by core and is being used by worlds. Removing it requires adjusting even more worlds, or removing functionality.

I would actually like to do this. But dropping a feature is obviously a taller ask, I get that.

For the sake of completeness, could you provide an example of a world that currently supports & uses this? Or ideally if you have the full list of supported worlds, but I don't expect you're just gonna have that on hand lol

[Silvris]

For the sake of completeness, could you provide an example of a world that currently supports & uses this? Or ideally if you have the full list of supported worlds, but I don't expect you're just gonna have that on hand lol

Admittedly, I'm pretty sure it's just A Link to the Past at the moment. But it being supported there is much stronger on the intent, given it is a world maintained by a core maintainer.

[NewSoupVi]

If there is an example of a supported world losing a currently working feature if event plando is dropped, then I'm going to revoke my disapproval, but I will still strongly discourage future use of this feature & if I have time, push for a removal of the feature in a followup PR changing those worlds. (Essentially, I will considered it "grandfathered" - The first thing I'd probably do is open a PR that makes it output a warning)

Edit: Sniped

[beauxq]

However, Berserker has already approved the PR, so maybe this is too late or I'm outvoted here.

I was seeing Berserker's review as just the a review of the LTTP changes, since that's how his review was requested - not of the PR overall.

Silvris requested a review from Berserker66 as a code owner

And that links to the LTTP line in codeowners

I could be misunderstanding though.

[Mysteryem]

It doesn't look like this rewrite fixes early_locations/non_early_locations, is that something the rewrite could/should also fix?

Targetting early_locations across more than one world of the same game gathers the names of the early locations across these worlds into a single list, but just because a location is early in one world, does not mean that location is also early in another world (e.g. entrance rando). This means that item plando targetting early_locations can place an item into a world where the location is not early, just because in one of the worlds that location is early.

[Exempt-Medic]

It doesn't look like this rewrite fixes early_locations/non_early_locations, is that something the rewrite could/should also fix?

FWIW, this is known. To me, with the number of changes already in this PR, I'm not sure how well adding more in will go. I think Silvris was waiting until after this since it would require some pretty significant changes to the way candidate locations work or adding player-specific locations for early/non-early which would also be a good bit of extra work/code

[Silvris]

It doesn't look like this rewrite fixes early_locations/non_early_locations, is that something the rewrite could/should also fix?

Targetting early_locations across more than one world of the same game gathers the names of the early locations across these worlds into a single list, but just because a location is early in one world, does not mean that location is also early in another world (e.g. entrance rando). This means that item plando targetting early_locations can place an item into a world where the location is not early, just because in one of the worlds that location is early.

While I'm not necessarily against doing this change in this PR, I think scope here has already gotten way out of hand, and the changes would be much easier to review isolated (especially with the amount of review having already gone into this PR). The idea I have to fix it effectively is a rewrite of the locations handling to be str-int pairs of location/player, which needs to be adjusted in a number of places.

[NewSoupVi]

The recent additions look pretty good at a glance. I think moving towards a world where in generate_basic, worlds can look at what a plando block is planning, and react to it. Also, this information will be very useful to ER.

I am a bit worried about the inbetween time, where plando provides the info, but GER has not been updated to do anything with it. Obviously I don't want to increase the scope of this PR any more, but if we can have some band-aid addition to GER for 1-item-1-location plando ready, I would appreciate that

[BadMagic100]

I am a bit worried about the inbetween time, where plando provides the info, but GER has not been updated to do anything with it. Obviously I don't want to increase the scope of this PR any more, but if we can have some band-aid addition to GER for 1-item-1-location plando ready, I would appreciate that

I don't think the in between time is any worse off than it would be with today's plando implementation, which we have still decided to put ER before. So I feel this is not a necessity

[NewSoupVi]

I am a bit worried about the inbetween time, where plando provides the info, but GER has not been updated to do anything with it. Obviously I don't want to increase the scope of this PR any more, but if we can have some band-aid addition to GER for 1-item-1-location plando ready, I would appreciate that

I don't think the in between time is any worse off than it would be with today's plando implementation, which we have still decided to put ER before. So I feel this is not a necessity

Now that we have connect_entrances and are already recommending people to do their ER in there, I agree. We made things "worse" for now, which now gives us the luxury of movinig towards the better solution iteratively :P

[Exempt-Medic]

Conflicts again