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: don't log warnings for plando_items and missing lttp options #3606

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

Conversation

alwaysintreble
Copy link
Collaborator

What is this fixing or adding?

generation now logs a warning for keys within a player's submitted game options if it can't find that option while resolving them. it missed a few options that are technically valid, but don't exist on the options system so needed to be hardcoded.

How was this tested?

with yamls from https://discord.com/channels/731205301247803413/1257466982748393572 plus a messenger yaml with an invalid option key

If this makes graphical changes, please attach screenshots.

@github-actions github-actions bot added affects: core Issues/PRs that touch core and may need additional validation. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Jul 2, 2024
Generate.py Outdated
if ret.game == "A Link to the Past":
roll_alttp_settings(ret, game_weights)

# log a warning for options within a game section that aren't determined as valid
for option_key in game_weights:
if option_key in {"triggers", "plando_items", *valid_keys}:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand why "plando_items" is here after it is added to valid_keys above. Is it meant to stay here after it's removed in the line above?

And this may not be in the scope of this PR, but why is "triggers" not added to valid_keys, instead of creating this new set repeatedly in every iteration of this for loop?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

because i did it the one way, realized the other way would be better, but forgot to remove this one lmao

@NewSoupVi NewSoupVi added the is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. label Jul 2, 2024
@@ -451,7 +451,7 @@ def roll_settings(weights: dict, plando_options: PlandoOptions = PlandoOptions.b
if "linked_options" in weights:
weights = roll_linked_options(weights)

valid_keys = set()
valid_keys = {"triggers"}
Copy link
Collaborator

@beauxq beauxq Jul 2, 2024

Choose a reason for hiding this comment

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

I'm worried about side effects from putting "triggers" in this set this early (because I was thinking of just adding it to the set right before the warning for loop near the end of the function).

It might be ok, but we should make sure it's tested and thought through with everything that is done with valid_keys before that warning loop.
(For example valid_keys is passed to roll_triggers, and I'm not sure what roll_triggers does with it.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

valid_keys gets rebuilt per player, and roll_triggers just adds the names that it triggers off of (fake names people use for more complex trigger setups) to the set. it should be fine, but more testing is probably a good idea

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: 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
Development

Successfully merging this pull request may close these issues.

None yet

3 participants