New Game Implementation: Metroid Prime #4248
Conversation
github-actions
bot
added
the
waiting-on: peer-review
ScipioWright
added
the
is: new game
worlds/metroidprime/Logic.py
Outdated
| if world.options.main_power_bomb.value: | ||
| return state.has_all( | ||
| [SuitUpgrade.Morph_Ball.value, SuitUpgrade.Main_Power_Bomb.value], | ||
| world.player, | ||
| ) |
There was a problem hiding this comment.
Edit: For clarity, I'm not requesting that these changes need to be made unless it's determined that they're causing a noticeable reduction in performance, these are just best practices.
I would prefer to see world.options entirely removed from all logic rules. Individual access rules can be called 10,000s of times over the course of generation. A world's options are effectively static from the start of generation, so checking world.options within access rules is a waste of CPU time.
I would recommend that whenever a can_power_bomb rule is to be set somewhere, that is when world.options is checked and then the appropriate rule depending on if world.options.main_power_bomb.value is set.
I recommend replacing lists in state.has_all/has_any with tuples or, better, creating the lists/tuples in advance and put them into the module's globals. CPython pre-allocates memory for small tuples so they are faster to create, but putting them into the module's globals would mean they would only need to be created once, rather than on each call, which is even faster.
If you really want to push for performance, then tuples containing only string literals are the fastest because Python can precompute them into a single constant expression, e.g. state.has_all(("Morph Ball", "Power Bomb (Main)"), world.player). But there's not much difference in performance compared to getting a list/tuple from the module's globals and you lose some ease of maintainability when using string literals.
In order of fastest to slowest (the first 3 are roughly the same performance, but are about 1.5x to 2x as fast as the last 3 when the length of the list/tuple is 2):
| Description | Code |
|---|---|
| The tuple contains only literals, so Python can constant fold it into a single constant without needing to build a new tuple. | state.has_all(("Morph Ball", "Power Bomb (Main)"), world.player) |
| The tuple has been created once in advance and stored on the module, so it has to be loaded from the module globals. | At the module level:power_bomb_main_items = (SuitUpgrade.Morph_Ball.value, SuitUpgrade.Main_Power_Bomb.value)Inside the function: state.has_all(power_bomb_main_items), world.player) |
| Same as the previous, but with a list. Exactly the same instructions inside the function, but I'm putting the list lower because lists are barely slower to iterate and use slightly more memory than tuples. In most cases, this difference compared to tuples can be ignored. | At the module level:power_bomb_main_items = [SuitUpgrade.Morph_Ball.value, SuitUpgrade.Main_Power_Bomb.value]Inside the function: state.has_all(power_bomb_main_items), world.player) |
| Now things start to get slower. Everything below here also gets noticeably slower the longer the list/tuple is. | |
| The item names are stored on the module in advance, so each one has to be loaded from the module's globals, and then a new tuple is created | At the module level:morph_ball = SuitUpgrade.Morph_Ball.valuemain_power_bomb = SuitUpgrade.Main_Power_Bomb.valueInside the function: state.has_all((morph_ball, main_power_bomb)), world.player) |
| The items names are constant string literals, so two constants are loaded and then a new list is created. NOTE: Loading constants is faster than loading globals, so as the list gets longer, this will outperform the previous option. NOTE: In some cases, Python can determine that it can replace lists containing only constants with a tuple constant expression, e.g. for x in ["first", "second"]): can be replaced with for x in ("first", "second"):, but passing the list to another function is not a case where Python can do this. |
state.has_all(["Morph Ball", "Power Bomb (Main)"], world.player) |
| The item names are stored on the module in advance, so each one has to be loaded from the module's globals , and then a new list is created | At the module level:morph_ball = SuitUpgrade.Morph_Ball.valuemain_power_bomb = SuitUpgrade.Main_Power_Bomb.valueInside the function: state.has_all([morph_ball, main_power_bomb]), world.player) |
The amount of attribute lookups necessary to get each item name feels a little silly here, especially since the lookups have to be done every time can_power_bomb and the other functions are called. To get SuitUpgrade.Morph_Ball.value, SuitUpgrade has to be looked up from the module's globals, then the Morph_Ball attribute has to be looked up on that object and finally the value attribute has to be looked up on that object.
If you don't want to put string literals in your access rules, I'd recommend putting morph_ball = SuitUpgrade.Morph_Ball.value into the module's globals in advance. In this case, the entire list can be put into the module's globals, so this is more for rules that check a single item rather than a list of items.
|
It seems like there might be some swap-related performance issues with generation using the default options Taking 20 Metroid Prime template yamls and generating with I generated 24 random seeds one after the other: Generated at commit hesto2@c39a8a2 OoT isn't exactly known for being the fastest around to generate, and 20 copies of its template yaml usually takes around 22s, filling about the same number of progression items as Metroid Prime, though only about 23% of its item pool is progression, compared to Metroid Prime's 45%. |
There was a problem hiding this comment.
Missing indirect conditions can cause unexpected fill errors or result in non-deterministic generation, so need to be fixed.
Only these two have come up so far from running generations with my missing indirect conditions checker. The entrance names can vary due to door color rando, but they're always connecting the same regions.
It's not a problem unless the logic is not supposed to be like this, but I did also come across "Blue Door from Lava Lake to Lake Tunnel" checking for being able to reach "Magmoor Caverns: Lake Tunnel", which is the same region the entrance connects to.
worlds/metroidprime/data/Tricks.py
Outdated
| and can_space_jump(world, state) | ||
| and has_energy_tanks(world, state, 4) | ||
| and state.can_reach( | ||
| "Magmoor Caverns: " + RoomName.Lake_Tunnel.value, None, world.player |
There was a problem hiding this comment.
This rule is being used on entrances, but the "Magmoor Caverns: Lake Tunnel" region is not being registered as an indirect condition for the entrances. e.g. entrance "Blue Door from Lava Lake to Pit Tunnel".
There was a problem hiding this comment.
Ah nice, I didn't know about this👍 I was able to register the indirect condition for both of these with minimal effort 🎉
(I changed the condition slightly for the lake tunnel one to use a room that actually makes a bit more sense, in case you notice that change as well)
|
I feel like there's some way to do the same changes by checking the precollected items rather than |
…o metroidprime-v1.0
|
@Mysteryem I was able to take a pass at the indirect conditions as well as resolving the long times for base starting room. Prime in general suffers from pretty restrictive starts so giving a couple more pre placement options helped with these instances 👍 |
I messed around with this a bit but was unable to find those working with |
|
| - Use the latest Dolphin | ||
|
|
||
| - Dolphin Beta (**Recommended**): [Dolphin Emulator - Download](https://dolphin-emu.org/download/#download-beta) | ||
| - PrimeHack (Not thoroughly tested with Metroid Prime AP): [Releases · shiiion_dolphin.htm](https://github.com/shiiion/dolphin/releases) |
There was a problem hiding this comment.
Should Primehack be in the troubleshooting section? If people are having trouble connecting, adding in a version that's technically more likely to have problems seems like a counterproductive step.
| ) | ||
|
|
||
|
|
||
| def can_flaahgra(world: "MetroidPrimeWorld", state: CollectionState) -> bool: |
There was a problem hiding this comment.
I'm probably missing something here, but if memory serves only Power Beam or Missiles can knock down the sun reflectors, and you need to do that for the first time to unlock the bomb slots even if you dodge the tentacles for the other phases. As best as I can tell, there's no protection against starting with Ice/Wave/Plasma and bombs and no missiles in Sunchamber Lobby, which would render Flaahgra unbeatable.
| defaultLock=DoorLockType.None_, | ||
| rule_func=can_bomb, | ||
| exclude_from_rando=True, | ||
| ), # Bombs are required to get out of here |
There was a problem hiding this comment.
I'm putting this here because it's where the thought occurred to me, but this is more relevant to the game info document. If Point of No Return checks like this are never in logic, it's probably worth noting such in a player-facing manner to stave off some of the questions that may arise. If they are sometimes in logic, then this is inconsistent with that and I'm curious as to the discrepancy.
| def _can_reach_pickup( | ||
| self, | ||
| world: "MetroidPrimeWorld", | ||
| state: CollectionState, | ||
| pickup_data: PickupData, | ||
| ) -> bool: | ||
| """Determines if the player is able to reach the pickup based on their items and selected trick difficulty""" | ||
| max_difficulty = world.options.trick_difficulty.value | ||
| allow_list = world.options.trick_allow_list | ||
| deny_list = world.options.trick_deny_list | ||
| for trick in pickup_data.tricks: | ||
| if trick.name not in allow_list and ( | ||
| trick.difficulty.value > max_difficulty or trick.name in deny_list | ||
| ): | ||
| continue | ||
| if trick.rule_func(world, state): | ||
| return True | ||
|
|
||
| if pickup_data.rule_func is None: | ||
| return True | ||
| if pickup_data.rule_func(world, state): | ||
| return True | ||
| return False |
There was a problem hiding this comment.
I see some big potential for improvement here again by precalculating this data outside the access rule.
Here's what I came up with.
I renamed _can_reach_pickup to _set_pickup_rule, and rewrote it like this:
def _set_pickup_rule(
self,
location: "MetroidPrimeLocation",
world: "MetroidPrimeWorld",
pickup_data: PickupData,
) -> bool:
"""Builds and sets a rule to determines if the player is able to reach the pickup based on their items and selected trick difficulty"""
basic_rule = pickup_data.rule_func
eligible_trick_rules = []
max_difficulty = world.options.trick_difficulty.value
allow_list = world.options.trick_allow_list
deny_list = world.options.trick_deny_list
for trick in pickup_data.tricks:
if trick.name not in allow_list and (
trick.difficulty.value > max_difficulty or trick.name in deny_list
):
continue
eligible_trick_rules.append(trick.rule_func)
if basic_rule is not None:
# nice bonus: capturing world is no longer necessary as we captured it through function scope.
add_rule(location, lambda state: basic_rule(world, state))
if eligible_trick_rules:
add_rule(location, lambda state: any(rule(world, state) for rule in eligible_trick_rules), "OR")
# I don't remember the exact syntax for an "or" add_rule but it exists
# There's also some more optimisation to be done here in case of len(eligible_trick_rules) == 1,
# where we don't need the "any" and can just add the one rule directlycreate_world_region would now just call this function, instead of setting the rule then and there.
def create_world_region(self, world: "MetroidPrimeWorld"):
# Create each room as a region
for room_name, room_data in self.rooms.items():
...
for pickup in room_data.pickups:
...
self._set_pickup_rule(location, world, pickup)
What is this fixing or adding?
Support for Metroid Prime as an archipelago world
How was this tested?
Unit tests, test gens, alpha & beta with the community since April 2024