-
Notifications
You must be signed in to change notification settings - Fork 665
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
Zillion: use useful item classification #4179
base: main
Are you sure you want to change the base?
Conversation
"Champ": 9999, | ||
"JJ": 9999, | ||
"Win": 9999, | ||
"Empty": 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are Empty and Bread included if the default is already 0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The values are here so that it's more clear when reading this code what happens with Empty and Bread.
If this "Empty": 0,
weren't here, it would take a little more mental energy for the reader to figure out what happens with it.
The default is just a safety in case something changes in the future.
""" make the item useful if the number in the item pool is below this number """ | ||
|
||
|
||
def get_classification(name: str, zz_item: ZzItem, item_counts: Counter[str]) -> IC: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this not accounting for extra items such as from start_inventory, item_plando, and item_links is intended / expected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I admit I didn't think of those when writing this.
But thinking about it now, I think it's not worth the added complexity for something that won't make very much of a difference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that it's not worth changing, especially since this'll only ever make items Useful when they shouldn't be, and not the reverse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM. Comments were addressed. Tested some generations and tested the new codepaths by adding printed messages and hosting and releasing one game to check actual item classifications as well. Multiclassification items (including Win) worked just fine.
What is this fixing or adding?
Use the
useful
item classification. (wasn't using any before)How was this tested?
play through a game