Pokemon Crystal: Implement new game #3494
Conversation
Exempt-Medic
added
the
waiting-on: author
Exempt-Medic
removed
the
waiting-on: author
There was a problem hiding this comment.
Still just minor things that are easily addressed.
Overall, I'd appreciate more type hints when it's not fully obvious from the function/arg name, but it's not terrible. I could figure them all out with a few seconds of reading the code.
Things I haven't checked yet: trainers.py, rules.py, rom.py, docs/, test/. Feel free to bother me if you get through this and I haven't finished up yet.
| if item_code > 0 and get_item_classification(item_code) != ItemClassification.filler: | ||
| # If TMs are randomized, TM items without move names are added to the pool | ||
| if item_code in crystal_data.tm_replace_map and self.options.randomize_tm_moves: | ||
| default_itempool += [self.create_item_by_code(item_code + 256)] |
There was a problem hiding this comment.
default_itempool.append(item)
is faster and (in my opinion) more readable than
default_itempool += [item]
Same for other instances below.
| weighted_pool = [["RARE_CANDY"] * 3, ["ETHER", "ELIXER", "MAX_ETHER", "MAX_ELIXER", "MYSTERYBERRY"] * 5, | ||
| ["WATER_STONE", "FIRE_STONE", "THUNDERSTONE", "LEAF_STONE", "SUN_STONE", "MOON_STONE"] * 2, | ||
| ["ESCAPE_ROPE"] * 3, ["NUGGET", "STAR_PIECE", "STARDUST", "PEARL", "BIG_PEARL"] * 2, | ||
| ["POKE_BALL", "GREAT_BALL", "ULTRA_BALL"] * 5, | ||
| ["POTION", "SUPER_POTION", "ENERGY_ROOT", "ENERGYPOWDER"] * 12, | ||
| ["HYPER_POTION", "FULL_RESTORE"] * 2, ["REPEL", "SUPER_REPEL", "MAX_REPEL"] * 3, | ||
| ["REVIVE", "REVIVAL_HERB"] * 4 + ["MAX_REVIVE"] * 2, | ||
| ["HP_UP", "PP_UP", "PROTEIN", "CARBOS", "CALCIUM", "IRON"] * 5, | ||
| ["GUARD_SPEC", "DIRE_HIT", "X_ATTACK", "X_DEFEND", "X_SPEED", "X_SPECIAL"] * 2, | ||
| ["HEAL_POWDER", "BURN_HEAL", "PARLYZ_HEAL", "ICE_HEAL", "ANTIDOTE", "AWAKENING", "FULL_HEAL"] * 5] |
There was a problem hiding this comment.
I would put this in a constant outside the function and name it _WEIGHTED_FILLER or something instead of constructing it every time the function is called.
I was going to suggest reformatting the data and using random.choices, but it turns out to be slower, so that would only be a matter of readability really.
There was a problem hiding this comment.
Same for convert_to_ingame_text.
|
|
||
| randomize_pokemon(self) | ||
|
|
||
| if self.options.randomize_starters.value: |
There was a problem hiding this comment.
I'm always going to be a fan of checking options explicitly against named values rather than relying on the falsiness of the particular option value, but I think I may be in the minority there. I value the safety and readability, but I know others consider it verbose and less elegant.
Disregard if you like it as it is.
There was a problem hiding this comment.
I think I may have made this comment before; I'll try to remember that my opinion has been shared already and stop flagging it, lol.
There was a problem hiding this comment.
I think it should be even less characters: if self.options.randomize_starters
There was a problem hiding this comment.
Either way, everything that's been mentioned here is fine as far as core is concerned :)
| @@ -0,0 +1,99 @@ | |||
| from typing import TYPE_CHECKING | |||
There was a problem hiding this comment.
I was going to comment on the strangeness of this file and its functions, but I realized I've been reading misc as short for "miscellaneous" and not "mischief", and I think that's likely going to be true for most people reading this for the first time. I don't think that ambiguity is worth the abbreviation. If nothing else, I'd suggest renaming this file.
| ids = [item_id for item_id, item_data in data.items.items() if item_data.item_const == const_name] | ||
| if len(ids): | ||
| return ids[0] if ids[0] < 256 else ids[0] - 256 | ||
| return 0 |
There was a problem hiding this comment.
I think it's a little suspicious to return 0 if you can't find a match. Is that default case useful somewhere? I think I would rather see an exception here than get reports of missing or nonexistant or empty items and tracking it back here. Especially if it's "exceptional" to not match an item name.
There was a problem hiding this comment.
These two functions are also reasonable candidates for the @cache decorator.
| return new_learnset | ||
|
|
||
|
|
||
| def get_random_move(random, move_type=None, attacking=None): |
There was a problem hiding this comment.
Looks like attacking should be default False and just have line 43 do if attacking:
| not move_data.is_hm and move_name not in ["STRUGGLE", "BEAT_UP", "NO_MOVE", "STRUGGLE"]] | ||
| else: | ||
| move_pool = [move_name for move_name, move_data in crystal_data.moves.items() if | ||
| not move_data.is_hm and move_data.type == move_type | ||
| and move_name not in ["STRUGGLE", "BEAT_UP", "NO_MOVE", "STRUGGLE"]] |
There was a problem hiding this comment.
Extra STRUGGLEs.
While I'm here, list is (theoretically) slower than tuple is (actually) slower than set even for these ad-hoc filters where you build a blacklist inside the comprehension. Probably mostly inconsequential, but getting random moves is a very hot path in Emerald, so if there's anywhere that small notes on performance might actually matter, it's somewhere like this.
EDIT: Black Sliver made a writeup recently on this topic. I would trust his opinion/experience/measurements over my own. I think (hope) that document is planned to be PR'd to this repo somewhere, but I think for now we only have this link to the discord message.
| move_pool = [move_data for move_name, move_data in copy.deepcopy(crystal_data.moves).items() if | ||
| not move_data.is_hm and move_name not in ["ROCK_SMASH", "NO_MOVE", "STRUGGLE"]] |
There was a problem hiding this comment.
I don't think you need to make a copy of the moves list here. I could be missing something.
| move.level <= level and move.move != "NO_MOVE"] | ||
| # double learnset pool to dilute HMs slightly | ||
| # exclude beat up as it can softlock the game if an enemy trainer uses it | ||
| move_pool += move_pool + [move for move in world.generated_pokemon[pokemon].tm_hm if move != "BEAT_UP"] |
There was a problem hiding this comment.
Can Beat Up sneak in here if learnsets are vanilla?
|
|
||
|
|
||
| def generate_phone_traps(world: "PokemonCrystalWorld"): | ||
| if world.options.phone_trap_weight.value: |
There was a problem hiding this comment.
This if isn't necessary here, since the same condition is used to decide whether to call this function in the first place.
If that weren't the case, there are a couple variables that get initialized inside this if block which are used below it, which would cause exceptions if this condition were ever False while in this function.
There was a problem hiding this comment.
More nitpicks and small improvements. Finally got through every file.
Left a lot of comments, but I have no major issues with the structure of the code on the whole. Only a handful of things really need addressing, and the rest is just code quality and minor optimizations.
Generated a few 1-world and a few 100-world multis, both with entirely random yamls. And I've generated with the world before in actual real games. I haven't played it myself though.
There's a conflict with main in the current list of games in the README, but otherwise all testing was after merging this branch with main.
| ride with the S.S. Ticket | ||
| - Magnet train between Goldenrod and Saffron is availble to ride with the Pass before power is restored to Kanto | ||
| - Misty is always in Cerulean Gym | ||
| - There is a ledge above the Route 2 entry to Digglet Cave, allowing you to reach the rest of West Kanto without Cut |
There was a problem hiding this comment.
| - There is a ledge above the Route 2 entry to Digglet Cave, allowing you to reach the rest of West Kanto without Cut | |
| - There is a ledge above the Route 2 entry to Diglett Cave, allowing you to reach the rest of West Kanto without Cut |
| ## Required Software | ||
|
|
||
| - [Archipelago](https://github.com/ArchipelagoMW/Archipelago/releases) | ||
| - An English (UE) Pokémon Crystal v1.0 ROM. The Archipelago community cannot provide this. |
There was a problem hiding this comment.
You include support for v1.1 now too.
|
|
||
| - [Archipelago](https://github.com/ArchipelagoMW/Archipelago/releases) | ||
| - An English (UE) Pokémon Crystal v1.0 ROM. The Archipelago community cannot provide this. | ||
| - [BizHawk](https://tasvideos.org/BizHawk/ReleaseHistory) 2.7 or later. 2.9.1 is recommended. |
There was a problem hiding this comment.
I have a slight hesitation with recommending a particular version, because it will be outdated eventually (as it was with OoT, which is why so many people continued to use 2.7 even after 2.9.1 released). And especially when <=2.9.1 has that arbitrary code execution bug on N64 games.
| def hidden(): | ||
| return world.options.randomize_hidden_items | ||
|
|
||
| def pokegear(): | ||
| return world.options.randomize_pokegear | ||
|
|
||
| def johto_only(): | ||
| return world.options.johto_only.value | ||
|
|
||
| def trainersanity(): | ||
| return world.options.trainersanity |
There was a problem hiding this comment.
I think these can be normal variables instead of functions.
hidden = bool(world.options.randomize_hidden_items)or the value, or whatever you think makes the most sense. But it looks like they're only shortcuts, so nothing is really gained from them being functions.
| rom_bytes = bytearray(rom) | ||
| if rom_bytes[revision_address] == 1: |
There was a problem hiding this comment.
| rom_bytes = bytearray(rom) | |
| if rom_bytes[revision_address] == 1: | |
| if rom[revision_address] == 1: |
| item_flag = location.address | ||
| player_name = world.multiworld.player_name[location.item.player].upper() | ||
| item_name = location.item.name.upper() | ||
| item_texts.append([player_name, item_name, item_flag]) |
There was a problem hiding this comment.
| item_texts.append([player_name, item_name, item_flag]) | |
| item_texts.append((player_name, item_name, item_flag)) |
| # if we somehow run out of capacity in both banks, just finish the table and break, | ||
| # there is a fallback string in the ROM, so it should handle this gracefully. | ||
| write_bytes(patch, [0xFF], item_name_table_adr + table_offset_adr) | ||
| print("oopsie") |
There was a problem hiding this comment.
Lol, if leaving this in was intentional, it should probably be a logging.warning or debug or something with a clearer message.
| write_bytes(patch, [0xFF], item_name_table_adr + item_name_table_length - 1) | ||
|
|
||
| if world.options.randomize_static_pokemon: | ||
| for _static_name, pkmn_data in world.generated_static.items(): |
There was a problem hiding this comment.
| for _static_name, pkmn_data in world.generated_static.items(): | |
| for pkmn_data in world.generated_static.values(): |
| for i in range(3): # morn, day, nite | ||
| for encounter in grass_encounters: | ||
| pokemon_id = data.pokemon[encounter.pokemon].id | ||
| write_bytes(patch, [encounter.level, pokemon_id], cur_address) | ||
| cur_address += 2 |
There was a problem hiding this comment.
I don't think this for i in range(3) accomplishes anything; i is unused in here.
|
|
||
| start_inventory_address = data.rom_addresses["AP_Start_Inventory"] | ||
| start_inventory = copy.deepcopy(world.options.start_inventory.value) | ||
| for item, quantity in start_inventory.items(): |
There was a problem hiding this comment.
I'd recommend building this from world.multiworld.precollected_items[world.player] instead. If you ever decide to automatically add something into your players' starting inventories, it's usually recommended to use push_precollected instead of modifying the player's options. And you want to give the player anything that the multiworld as a whole has decided is precollected, which may be partially outside your control. The generator's relatively new panic_method setting might put unplaced progression items into precollected_items. I've not tried that setting out myself though.
|
You should look at this The overlapping location/item IDs between this game and ALTTP are causing the Rogue Legacy client to crash. That is not "your fault" as Crystal is unsupported right now, but as it stands, as long as there are supported games whose clients crash with overlapping IDs, we won't allow a game with overlapping IDs to become supported, so it is blocking for this PR. |
NewSoupVi
added
the
waiting-on: author
|
I can move this to a draft until that gets fixed if you'd prefer, I've been short on time to look at the reviews anyway. I don't want to bring back the stupid ID offsetting again, and I'm sure there will be more broken worlds found from Crystal overlapping. |
|
Drafting hurts the visibility so I wouldn't necessarily ask you to do that, since this PR is, for all other intents and purposes, fully "reviewable". But maybe we can slap the "waiting on: other" label on it (I'm on mobile rn and idk how to do it, if noone else gets to it I'll try to remember later) |
Exempt-Medic
added
the
waiting-on: other
|
One more comment, you should add an entry to |
What is this fixing or adding?
Adds Pokemon Crystal as a supported game
How was this tested?
Tested by the community over the last 7 months
If this makes graphical changes, please attach screenshots.