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

Pokemon Red/Blue: Fixed Deprecated options system #3564

Closed

Conversation

agilbert1412
Copy link
Collaborator

@agilbert1412 agilbert1412 commented Jun 18, 2024

What is this fixing or adding?

I was tired of the infinity warnings, so I did something about it. Also took care of the per_slot_randoms while I was here.

How was this tested?

Not much.

I ran the generic unit tests, but Pokemon Red/Blue does not have specific unit tests, so it is impossible to know if things are correct or not.

Plus, I don't play the game myself.

Overall, low confidence, but I figured that's better than leaving the options in limbo forever. I just recommend the reviewers pay close attention I guess.

@github-actions github-actions bot added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Jun 18, 2024
@agilbert1412 agilbert1412 added waiting-on: world-maintainer Issue/PR is waiting for feedback or approval by the maintainer of a world. is: maintenance Regular updates to requirements and utilities that do not fix bugs or change/add features. labels Jun 18, 2024
@Exempt-Medic
Copy link
Member

While you're fixing these, you may as well change "half": default / 2 to "half": default // 2 in ExpModifier since that has caused a few various bugs in the past

@Exempt-Medic
Copy link
Member

Perhaps out of scope, but since you changed per_slot_randoms, you could change things to use world.player_name instead of multiworld.player_name[world.player]

@Exempt-Medic
Copy link
Member

You missed at least two instances of the old option getters in pokemon.py on lines 367 and 378

Copy link
Contributor

@Alchav Alchav left a comment

Choose a reason for hiding this comment

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

These kinds of changes have already been made on a dev branch and this would likely lead to massive merge conflicts

@agilbert1412
Copy link
Collaborator Author

As the world maintainer, I'm pretty sure you're allowed to block the PR on these grounds, so if that's your stance, so be it.

I took initiative because I don't know the timeline on that dev branch, but what I do know is that it has been over 8 months since #993 was merged, and that these specific options not being fixed is a significant annoyance to a lot of people (myself included).

Do you have a rough timeline for the dev branch, so that we know what to expect?

@eudaimonistic
Copy link
Contributor

eudaimonistic commented Jun 19, 2024

Do you have a rough timeline for the dev branch, so that we know what to expect?

Seemingly that's what the just posted #3566 is about.

@NewSoupVi
Copy link
Member

Going to default to the PR by the world author, which has been opened now

@NewSoupVi NewSoupVi closed this Jun 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is: maintenance Regular updates to requirements and utilities that do not fix bugs or change/add features. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. waiting-on: world-maintainer Issue/PR is waiting for feedback or approval by the maintainer of a world.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants