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.

Sign up for GitHub

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

Jump to bottom

Core: Fix Fill choking on itself in minimal + full games #4225

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

Core: Fix Fill choking on itself in minimal + full games #4225

wants to merge 2 commits into from

Conversation

NewSoupVi
Copy link
Member

@NewSoupVi NewSoupVi commented Nov 22, 2024 •
edited
Loading

Fixes #4224

Basically, the "cleanup" step is too overzealous and "cleans up" cases that can actually be fixed by accessibility_corrections later, leading to failed seeds that would have generated perfectly fine otherwise

I knew this fix was gonna be like one line

@github-actions github-actions bot added the affects: core Issues/PRs that touch core and may need additional validation. label Nov 22, 2024
@NewSoupVi NewSoupVi added is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Nov 22, 2024
@NewSoupVi
Copy link
Member Author

Idk about the formatting here, if someone who actually knows PEP8 and doesn't let ruff do it for them wants to check it that would be appreciated

@NewSoupVi
Copy link
Member Author

Part of me is also wondering what this part of the code is even doing, it seems like it might be a relic of the past that isn't hurting anything but doesn't need to be here anymore? Maybe one of the core devs who worked on this part of the code can explain

Either way, this fix does fix the issue, so

Exempt-Medic
Exempt-Medic approved these changes Nov 22, 2024
View reviewed changes
Copy link
Member

@Exempt-Medic Exempt-Medic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change LGTM. Stepped through the code and ran some test generations that previously broke and now did not. Slightly concerned that this doesn't check classification, but it didn't do that earlier either.

@NewSoupVi
Copy link
Member Author

NewSoupVi commented Dec 25, 2024 •
edited
Loading

Just for the record: I think this should get looked at by one more core maintainer or someone like Em or Treble who know a lot about Fill

@Exempt-Medic
Copy link
Member

Just for the record: I think this should get looked at by one more core maintainer or someone like Em or Treble who know a lot about Fill

Very much agreed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Exempt-Medic Exempt-Medic Exempt-Medic approved these changes

Assignees
No one assigned
Labels
affects: core Issues/PRs that touch core and may need additional validation. is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fill chokes on itself on the last meters in games with both minimal players and full players
2 participants
@NewSoupVi @Exempt-Medic