Core: Add has_list and count_list and improve (?) count_group #2934
Conversation
github-actions
bot
added
affects: core
There was a problem hiding this comment.
Since has_list and has_group appear to have identical implementations except for the self.multiworld.worlds[player].item_name_groups[item_name_group] expression, maybe we should deduplicate them by reimplementing has_group as a call to has_list?
Code LGTM and I agree this is useful. Methods are short enough it's fine if we leave them slightly duped.
Can't help with the concerns about frozen performance.
I try to avoid calling functions in state methods because function calls take (an admittely very tiny amount of) time, duplicating the code is faster. At least in bare python ,compiled Python might be different. But even if it does take time, in this case I think it could maybe be worth it, as this is a slower function so adding one function call would probably not be the worst thing. Would like more opinions on that if ppl have them |
Exempt-Medic
added
waiting-on: core-review
|
Addressed all the feedback Rewritten docstrings should be rereviewed |
| @@ -767,8 +767,23 @@ def item_count(self, item: str, player: int) -> int: | |||
| Utils.deprecate("Use count instead.") | |||
| return self.count(item, player) | |||
|
|
|||
| def has_from_list(self, items: Iterable[str], player: int, count: int) -> bool: | |||
There was a problem hiding this comment.
to be honest, the doc string do make it clear, but when i see state.has_from_list(["itemA", "ItemB"], 3, player) it would be unclear to me at first glance what it does
has_from_list to me from its method name looks very simular to state.has_any(["itemA", "ItemB"], player) is with the only difference is that you can specify a minimal amount rather than just 1 (any)
so my recommendation would be to name it has_atleast()
| return True | ||
| return False | ||
|
|
||
| def count_from_list(self, items: Iterable[str], player: int) -> int: |
There was a problem hiding this comment.
why not just name this count and make it an overload?
…elagoMW#2934) * Add has_list and count_list and improve (?) count_group * MESSENGER STOP * Add docstrings to has_list and count_list * Add docstrings for has_group and count_group as well * oops * Rename to has_from * docstrings improvement again * Docstring
…elagoMW#2934) * Add has_list and count_list and improve (?) count_group * MESSENGER STOP * Add docstrings to has_list and count_list * Add docstrings for has_group and count_group as well * oops * Rename to has_from * docstrings improvement again * Docstring
What was done
has_list and count_list
has_groupandcount_groupcurrently exist to be able to check for a specific overall quantity of a list of items.However, these need item groups to be defined, and also the user has no way of telling this function which items from a group actually exist. Those functions will just blindly try all items from the item group.
So, I decided to add two new functions called
has_listandcount_list, that are completely analogous tohas_groupandcount_group, except you can specify the list of items you care about. This way, you 1. don't need an item group to be defined for the items and 2. can prepare the item list yourself, first sorting out any items that don't exist because of e.g. the options the player chose.count_group refactor
This function uses a for loop for a sum despite not having any sort of early exit / lazy eval (like e.g.
has_groupdoes). This should probably just be areturn sum(...)expression.Motivation
An new world dev asked for it and me and Treble agreed that the current implementation is not really das Gelbe vom Ei, at least if it's going to be the only option for this.
Considerations
Some of these decisions might have been made because of how frozen builds are optimised. I'm not sure how to judge or test that. If the
count_grouprefactor actually makes the frozen build slower bc of some weird optimisation thing, I am happy to revert it and take it out of the scope of this PR.