[TLOZ]: Fix determinism / Add Location Name Groups / Remove Level 9 Junk Fill #3670
Conversation
…unk Fill Axing the final uses of world.multiworld.random that were missed before, hopefully fixing the determinism issue brought up in Issue ArchipelagoMW#3664 (at least on TLOZ's end, leaving SMZ3 alone). Also adding location name groups finally, as well as axing the Level 9 Junk Fill because with the new location name groups players can choose to exclude Level 9 with exclude locations instead.
…nto tloz-fix-determinism
github-actions
bot
added
the
waiting-on: peer-review
t3hf1gm3nt
added
is: bug/fix
Kept debating back and forth on whether I wanted a group for them or not. Figured why not after you mentioned it, and threw in a bonus group for the sword caves. |
Exempt-Medic
added
waiting-on: core-review
worlds/tloz/__init__.py
Outdated
| location_name_groups = { | ||
| "Shops": shop_locations, | ||
| "Take Any": take_any_locations, | ||
| "Sword Caves": sword_cave_locations, | ||
| "Level 1": level_locations[0], | ||
| "Level 2": level_locations[1], | ||
| "Level 3": level_locations[2], | ||
| "Level 4": level_locations[3], | ||
| "Level 5": level_locations[4], | ||
| "Level 6": level_locations[5], | ||
| "Level 7": level_locations[6], | ||
| "Level 8": level_locations[7], | ||
| "Level 9": level_locations[8], | ||
| } |
There was a problem hiding this comment.
This should be wrong typing since location_name_groups is defined as ClassVar[Dict[str, Set[str]]] = {}. Does pycharm not complain about this?
Should be set(shop_location), set(take_any_locations), etc.
Or do other games do the same thing and should we update the definition in AutoWorld.py? AutoWorld's __new__ already converts it to frozenset() and frozenset(set(list)) is quite a bit slower than frozenset(list)? Maybe @Berserker66 at this point.
There was a problem hiding this comment.
FWIW, PyCharm doesn't seem to complain, at least not by default, might be some setting I have to turn on somewhere
There was a problem hiding this comment.
mypy says
worlds/tloz/__init__.py:92: error: Dict entry 0 has incompatible type "str": "list[str]"; expected "str": "set[str]" [dict-item]and similar
There was a problem hiding this comment.
Ah, it requires mypy. Well that explains it then, in which case there are many errors in that file
There was a problem hiding this comment.
I had no knowledge of what mypy even was until this conversation. Should that be a requirement if it's expected for people to follow it?
Regardless, I will update the location name groups to be sets when I get a chance.
There was a problem hiding this comment.
Should that be a requirement if it's expected for people to follow it?
Pycharm does a lot of that already, but is a lot less strict and needs to operate faster (ignoring some things), so mypy is not strictly required unless you want to get typing 100% correct.
Using mypy gives you some level of confidence that you did not misuse an API or a special case (such as ignoring the None case for an Optional[Item] ).
The question here is if the type hint in Autoworld is "good" (correct), because all code operating on it would work with a list rather than a set, and providing a list (as you did) would have had better performance.
Exempt-Medic
removed
the
is: maintenance
…unk Fill (ArchipelagoMW#3670) * [TLOZ]: Fix determinism / Add Location Name Groups / Remove Level 9 Junk Fill Axing the final uses of world.multiworld.random that were missed before, hopefully fixing the determinism issue brought up in Issue ArchipelagoMW#3664 (at least on TLOZ's end, leaving SMZ3 alone). Also adding location name groups finally, as well as axing the Level 9 Junk Fill because with the new location name groups players can choose to exclude Level 9 with exclude locations instead. * location name groups * add take any item and sword cave location name groups * use sets like you're supposed to, silly
What is this fixing or adding?
Axing the final uses of world.multiworld.random that were missed before, hopefully fixing the determinism issue brought up in Issue #3664 (at least on TLOZ's end, leaving SMZ3 alone). Also adding location name groups finally, as well as axing the Level 9 Junk Fill because with the new location name groups players can choose to exclude Level 9 with exclude locations instead.
How was this tested?
Couple of test gens of the same seed with two TLOZ yamls. Tried using the yamls provided in #3664 but I believe more would need to be done on SMZ3's end, which I will be leaving to that world's maintainer.
If this makes graphical changes, please attach screenshots.