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: Fix Generate's slot parsing to default unknown slot names to file name #3795

Merged
merged 3 commits into from
Sep 8, 2024
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion Generate.py
Original file line number Diff line number Diff line change
@@ -200,9 +200,11 @@ def main(args=None) -> Tuple[argparse.Namespace, int]:
except Exception as e:
raise Exception(f"Error setting {k} to {v} for player {player}") from e

if not getattr(erargs, "name", None):
setattr(erargs, "name", {}) # if the first slot does not define a name we won't have a name obj to look up
qwint marked this conversation as resolved.
Show resolved Hide resolved
if path == args.weights_file_path: # if name came from the weights file, just use base player name
erargs.name[player] = f"Player{player}"
elif not erargs.name[player]: # if name was not specified, generate it from filename
elif player not in erargs.name: # if name was not specified, generate it from filename
erargs.name[player] = os.path.splitext(os.path.split(path)[-1])[0]
erargs.name[player] = handle_name(erargs.name[player], player, name_counter)
Copy link
Member

Choose a reason for hiding this comment

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

Unsure if you'd want to add this to this PR, but this does technically change the fact that previously you could name yourself false (which would get changed to the yaml name) and now you cannot name yourself anything boolean in yaml. Could add this to fix it, but maybe there's a better way?

Suggested change
erargs.name[player] = handle_name(erargs.name[player], player, name_counter)
erargs.name[player] = handle_name(str(erargs.name[player]), player, name_counter)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what is expected / what currently works? if you name yourself "false" it currently ignores the name and runs through this code to get the filename instead?

Copy link
Member

Choose a reason for hiding this comment

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

If you name yourself false (no quotation marks) currently it ends up actually hitting the yaml name rename line because your name evaluates as, well, False. This is clearly unintended, but the name does work just fine on WebHost, so I guess it converts to a string somewhere. I think it would make sense to do the same here so that name: false, name: on etc. will gen instead of crashing