Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Core: Plando Items "rewrite" #3046

Open
wants to merge 66 commits into
base: main
Choose a base branch
from

Conversation

Silvris
Copy link
Collaborator

@Silvris Silvris commented Mar 28, 2024

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.

Silvris added 3 commits March 27, 2024 22:30
the test is still busted, but now it actually plandos the items
@github-actions github-actions bot added the affects: core Issues/PRs that touch core and may need additional validation. label Mar 28, 2024
Options.py Outdated Show resolved Hide resolved
@alwaysintreble alwaysintreble added is: enhancement Issues requesting new features or pull requests implementing new features. is: refactor/cleanup Improvements to code/output readability or organizization. labels Mar 28, 2024
Co-authored-by: Exempt-Medic <60412657+Exempt-Medic@users.noreply.github.com>
@Exempt-Medic
Copy link
Member

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?

Fill.py Outdated Show resolved Hide resolved
@Silvris
Copy link
Collaborator Author

Silvris commented Mar 29, 2024

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
Copy link
Collaborator

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
Copy link
Collaborator

beauxq commented Mar 29, 2024

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
Copy link
Collaborator Author

Silvris commented Mar 29, 2024

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
Copy link
Collaborator Author

Silvris commented Mar 29, 2024

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
Copy link
Collaborator Author

Silvris commented May 21, 2024

List of worlds that implement pre_fill and not get_pre_fill_items:

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

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

  • A Link to the Past
  • 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.

Fill.py Outdated Show resolved Hide resolved
Options.py Show resolved Hide resolved
Fill.py Outdated Show resolved Hide resolved
Fill.py Show resolved Hide resolved
@Exempt-Medic Exempt-Medic added waiting-on: author Issue/PR is waiting for feedback or changes from its author. and removed waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer. labels Jan 1, 2025
Fill.py Outdated Show resolved Hide resolved
Fill.py Outdated Show resolved Hide resolved
@NewSoupVi
Copy link
Member

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

@Exempt-Medic Exempt-Medic dismissed their stale review January 21, 2025 02:04

Significant changes made after approval

@BadMagic100
Copy link
Collaborator

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

@Exempt-Medic Exempt-Medic added waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer. and removed waiting-on: author Issue/PR is waiting for feedback or changes from its author. labels Jan 21, 2025
Options.py Outdated Show resolved Hide resolved
Co-authored-by: Doug Hoskisson <beauxq@users.noreply.github.com>
@NewSoupVi
Copy link
Member

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

Fill.py Outdated Show resolved Hide resolved
Co-authored-by: Exempt-Medic <60412657+Exempt-Medic@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects: core Issues/PRs that touch core and may need additional validation. is: enhancement Issues requesting new features or pull requests implementing new features. is: refactor/cleanup Improvements to code/output readability or organizization. waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.