Mega Man 2: Implement New Game

[Silvris]

What is this fixing or adding?

Adds Mega Man 2 as a supported game to Archipelago.

How was this tested?

5 apworld releases.
Numerous unsupported games, by myself and others.

If this makes graphical changes, please attach screenshots.

[Exempt-Medic]

All my comments were addressed. I'm a little confused/worried about the collect/remove here but, eh, core can decide about that. This worlds generates fast, even when scaled to ridiculous numbers of players like 1000. I'd be lying if I said that wasn't an insane relief after the other worlds I reviewed XD
No generation errors occurred other than with non-ASCII item names which is now fixed

[NewSoupVi]

This code is pretty nice in terms of performance & logic, I don't think I had almost any genuine issues with it except I do want collect and remove to be looked at for a possible simplification.

However, I'm not quite happy that line 77 to 250 of rules.py took me 1.5h to review. They really don't do anything crazy, but the shortened variable names, use of "magic numbers", and generally not always picking the most readable container / iterator (I'd especially like seeing .items() used more and tuples & range to be used less where possible) made it really really hard to penetrate, sometimes taking me several minutes to understand what a single comprehension is doing.

But I guess at the end of the day, as long as you understand it, it's kind of fine.

Anyway, all in all, I imagine we can get this one through pretty quick.

[NewSoupVi]

Ftr, this game gens so fast so I'm definitely not gonna force this, but even just for readability, I think this should be further "pre-reduced". Consider this:

robot_master_reqs = {
    boss: reqs for boss, reqs in boss_requirements.items() if boss in robot_masters
}
world.robot_master_to_weapon_names = {
    map(lambda x: weapons_to_name[x], reqs) for boss, reqs in robot_master_reqs
}

def can_defeat_enough_rbms(state: "CollectionState", player: int,
                           required: int, robot_master_to_weapon_names: Dict[int, List[str]]):
    can_defeat = 0
    for boss, weapon_names in robot_master_to_weapon_names.items():
        if state.has_all(weapon_names, player):
            can_defeat += 1
            if can_defeat >= required:
                return True
    return False

[NewSoupVi]

In fact, at that point, you don't even need the names of the robot masters anymore, do you! The variable boss is now unused in that for loop.

You can reduce it down to a list of only the weapon names lists that beat each robot master somewhere, but omit the information about which robot master they belong to bc that's not relevant to the logic:

world.robot_master_weapon_lists = [
    [weapons_to_name[x] for x in weapons_that_kill_it]  # "Map" is a bit wank at also causes a typing issue lol
    for boss, weapons_that_kill_it in boss_requirements.items() 
    if boss in robot_masters
]

and then your rbm function can be:

def can_defeat_enough_rbms(state: "CollectionState", player: int,
                           required: int, robot_master_weapon_lists: List[List[str]]):
    can_defeat = 0
    for weapon_list in robot_master_weapon_lists:
        if state.has_all(weapon_list, player):
            can_defeat += 1
            if can_defeat >= required:
                return True
    return False

[NewSoupVi]

Oh you could even sort it by length to make the early exit more likely to be sooner... ok maybe this is going too far now

[Silvris]

I think I'm going to keep it as is, mostly because the current structure of boss_requirements is useful for debugging should there still be another error in the rule. Being able to see the end result of the validation was already very useful in debugging issues in the past, and with the speed it generates at, changing it isn't worth losing that.

[NewSoupVi]

I'm saying you could have both, boss requirements as a "source of truth" and "robot_master_weapons" as a prepared / filtered & int->str converted version for this specific access rule

In fact, in my sample impl of this, I just had that conversion sitting right before the setting of the rbm access rules, as a local variable, not even attaching it to world at all

[NewSoupVi]

Anyway think abt it, I'll do a rereview at some point tomorrow probably

[NewSoupVi]

Oh hold up, isn't this missing a MegaMan2Client.py?

[Silvris]

Oh hold up, isn't this missing a MegaMan2Client.py?

Bizhawk Client

[NewSoupVi]

Oh hold up, isn't this missing a MegaMan2Client.py?

Bizhawk Client

🤦 my bad

[NewSoupVi]

Ok then I think this is good :) Give me a lil bit