Skip to content
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

Mega Man X3: Implement New Game #3844

Draft
wants to merge 114 commits into
base: main
Choose a base branch
from
Draft

Conversation

TheLX5
Copy link
Contributor

@TheLX5 TheLX5 commented Aug 24, 2024

What is this fixing or adding?

Implements Mega Man X3 in Archipelago.

How was this tested?

Months of weekly sessions in private and public games

If this makes graphical changes, please attach screenshots.

image

@TheLX5
Copy link
Contributor Author

TheLX5 commented Aug 24, 2024

Part of the client code requires #3093 to be properly tested. It can work without it though.

@github-actions github-actions bot added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Aug 24, 2024
@NewSoupVi NewSoupVi added the is: new game Pull requests for implementing new games into Archipelago. label Aug 24, 2024
Copy link
Collaborator

@ScipioWright ScipioWright left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a cursory look for now

Comment on lines +7 to +10
from worlds.AutoWorld import World


def create_regions(multiworld: MultiWorld, player: int, world: World, active_locations):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
from worlds.AutoWorld import World
def create_regions(multiworld: MultiWorld, player: int, world: World, active_locations):
if typing.TYPE_CHECKING:
from . import MMX3World
def create_regions(world: "MMX3World", active_locations):
multiworld = world.multiworld
player = world.player

If you do that weird type_checking thing, you can import your actual world class for your typing. It does need to be put in quotes when typing.
And then, you can get the multiworld and player directly from the world class.

add_event_to_region(multiworld, player, RegionName.dr_doppler_lab_2, EventName.dr_doppler_lab_2_clear)

# Dr. Doppler Lab 3
if world.options.doppler_lab_3_boss_rematch_count.value > 0:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if world.options.doppler_lab_3_boss_rematch_count.value > 0:
if world.options.doppler_lab_3_boss_rematch_count > 0:

No need for .value here -- it "knows" to compare to the int value like this.
You can also remove the > 0 if you want, but you may have it here to keep it clearer so it's up to you.

connect(world, RegionName.dr_doppler_lab_4, RegionName.dr_doppler_lab_4_boss)

# Connect checkpoints
if world.options.logic_helmet_checkpoints.value:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if world.options.logic_helmet_checkpoints.value:
if world.options.logic_helmet_checkpoints:

No need for .value here

connect(world, RegionName.dr_doppler_lab_3, RegionName.dr_doppler_lab_3_after_rematches)


def create_region(multiworld: MultiWorld, player: int, active_locations, name: str, locations=None):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def create_region(multiworld: MultiWorld, player: int, active_locations, name: str, locations=None):
def create_region(multiworld: MultiWorld, player: int, active_locations, name: str, locations=None) -> Region:

Would recommend having the return type shown where feasible

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Return types and typing stuff were something I added last minute before deciding to PR. Not too long ago I noticed that it's quite useful info to have.

from worlds.generic.Rules import add_rule, set_rule
from BaseClasses import CollectionState

from . import MMX3World
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm kind of surprised this isn't giving you a recursive import error

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised as well now that I look at it, I think this has been around for a while in the world.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, as long as it works I guess lol

Comment on lines +106 to +107
set_rule(multiworld.get_entrance(f"{RegionName.intro_stage} -> {RegionName.blizzard_buffalo}", player),
lambda state: state.has(ItemName.stage_blizzard_buffalo, player))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
set_rule(multiworld.get_entrance(f"{RegionName.intro_stage} -> {RegionName.blizzard_buffalo}", player),
lambda state: state.has(ItemName.stage_blizzard_buffalo, player))
set_rule(world.get_entrance(f"{RegionName.intro_stage} -> {RegionName.blizzard_buffalo}"),
lambda state: state.has(ItemName.stage_blizzard_buffalo, player))

There's a newer world.get_entrance which doesn't require the player number (since that's in the world already)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's useful :o

Comment on lines +180 to +182
add_rule(entrance_blizzard, lambda state: state.has_group("Weapons", player, world.options.vile_weapon_count.value))
add_rule(entrance_volt, lambda state: state.has_group("Weapons", player, world.options.vile_weapon_count.value))
add_rule(entrance_crush, lambda state: state.has_group("Weapons", player, world.options.vile_weapon_count.value))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using has_group_unique for these, which is like has_group but it ignores duplicates.
I have a feeling these will erroneously succeed if someone start_inventory's something then receives a duplicate of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I didn't consider it. I just copied this from the Ride Armor logic and didn't think about it twice.

Comment on lines 253 to 257
if hasattr(self.multiworld, "generation_is_fake"):
if hasattr(self.multiworld, "re_gen_passthrough"):
if "Mega Man X3" in self.multiworld.re_gen_passthrough:
slot_data = self.multiworld.re_gen_passthrough["Mega Man X3"]
self.boss_weaknesses = slot_data["weakness_rules"]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if hasattr(self.multiworld, "generation_is_fake"):
if hasattr(self.multiworld, "re_gen_passthrough"):
if "Mega Man X3" in self.multiworld.re_gen_passthrough:
slot_data = self.multiworld.re_gen_passthrough["Mega Man X3"]
self.boss_weaknesses = slot_data["weakness_rules"]
if hasattr(self.multiworld, "generation_is_fake"):
if hasattr(self.multiworld, "re_gen_passthrough"):
if "Mega Man X3" in self.multiworld.re_gen_passthrough:
self.boss_weaknesses = self.multiworld.re_gen_passthrough["Mega Man X3"]["weakness_rules"]

Since you're only returning the one thing, might as well one-line this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will not apply this one as I noticed I need to pass a lot more info to UT than just weaknesses which is pretty much most of the data I'm saving in slot_data.

Comment on lines +11 to +12
are randomized in the multiworld. The requirements for entering Dr. Doppler's Lab can be randomized to require different
amount of items (Medals from Mavericks, Weapon count, Upgrade count, Heart Tank and Sub Tank count).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
are randomized in the multiworld. The requirements for entering Dr. Doppler's Lab can be randomized to require different
amount of items (Medals from Mavericks, Weapon count, Upgrade count, Heart Tank and Sub Tank count).
are randomized in the multiworld. The requirements for entering Dr. Doppler's Lab can be randomized to require different
amounts of items (Medals from Mavericks, Weapon count, Upgrade count, Heart Tank and Sub Tank count).

Comment on lines +55 to +57
EnergyLink is an energy storage supported by certain games that is shared across all worlds in a multiworld. In Mega Man
X3, when enabled, deposits a certain amount of Energy to the EnergyLink pool. Only a quarter of the collected Energy is
successfully sent to the EnergyLink pool.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
EnergyLink is an energy storage supported by certain games that is shared across all worlds in a multiworld. In Mega Man
X3, when enabled, deposits a certain amount of Energy to the EnergyLink pool. Only a quarter of the collected Energy is
successfully sent to the EnergyLink pool.
EnergyLink is an energy storage supported by certain games that is shared across all worlds in a multiworld. Mega Man
X3, when enabled, deposits a certain amount of collected Energy to the EnergyLink pool. Only a quarter of the collected Energy is
successfully sent to the EnergyLink pool.

@@ -255,64 +255,126 @@ def set_rules(self):
if "Mega Man X3" in self.multiworld.re_gen_passthrough:
slot_data = self.multiworld.re_gen_passthrough["Mega Man X3"]
self.boss_weaknesses = slot_data["weakness_rules"]
print (self.boss_weaknesses)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
print (self.boss_weaknesses)

Probably not intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correct

Comment on lines 392 to 396
# Parse logic-relevant settings and save them elsewhere (needed for UT support)
self.boss_weakness_strictness = self.options.boss_weakness_strictness.value
self.pickupsanity = self.options.pickupsanity.value
self.jammed_buster = self.options.jammed_buster.value
self.logic_boss_weakness = self.options.logic_boss_weakness.value
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pycharm is probably screaming about these. I'd really recommend against this pattern

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Take a look at how TUNIC is doing it -- we're setting the option values directly based on what was in the slot_data

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, that looks much better. Assuming I'm looking at the correct thing

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup! That's the right thing there. Some of the options get set a little differently -- you can just ignore those, they're because of ER stuff and how I'm handling it.

def interpret_slot_data(self, slot_data):
local_weaknesses = {boss: entries for boss, entries in slot_data["weakness_rules"].items()}

interpreted_slot_data = {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has anything actually changed between slot_data and interpreted_slot_data? This looks like you are just returning the exact same thing you put in.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can I just return the entire slot_data in that case? 🤔
Honestly I'm way too lost at UT stuff, I just find it handy to have, especially when testing logic.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup! You can just return that directly

    @staticmethod
    def interpret_slot_data(slot_data: Dict[str, Any]) -> Dict[str, Any]:
        return slot_data

Comment on lines 259 to 262
self.boss_weakness_strictness = slot_data["boss_weakness_strictness"]
self.pickupsanity = slot_data["pickupsanity"]
self.jammed_buster = slot_data["jammed_buster"]
self.logic_boss_weakness = slot_data["logic_boss_weakness"]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd recommend putting this stuff in generate_early instead -- that way everything after generate_early can catch the option changes

@TheLX5
Copy link
Contributor Author

TheLX5 commented Sep 20, 2024

The commits should address the previous concerns/suggested changes, I'll begin porting X1's suggestions here shortly ™️ .

@TheLX5 TheLX5 marked this pull request as draft January 23, 2025 08:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is: new game Pull requests for implementing new games into Archipelago. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants