Mega Man X: Implement New Game #3842
Conversation
|
Part of the client code requires #3093 to be properly tested. It can work without it though. |
github-actions
bot
added
the
waiting-on: peer-review
NewSoupVi
added
the
is: new game
worlds/mmx/Client.py
Outdated
| game_state = await snes_read(ctx, MMX_GAME_STATE, 0x1) | ||
| menu_state = await snes_read(ctx, MMX_MENU_STATE, 0x1) | ||
| gameplay_state = await snes_read(ctx, MMX_GAMEPLAY_STATE, 0x1) |
There was a problem hiding this comment.
In general, all the code in this client is treating snes_read as if it's as fast and as cheap and as reliable as reading from your own ram.
The await should be a clue that that isn't the case.
I think it's better to treat snes_read like asking someone to go to the other side of town to get something for you.
In this example, you're asking someone to cross town to get MMX_GAME_STATE,
and then after they return, you immediate ask them to go back, to the same place they just came from, to get another small thing MMX_MENU_STATE that was sitting right next to the first thing.
The person would probably say "Why didn't you tell me to get that in the first trip?"
And then you ask them to go get a 3rd single byte that's right next to the first two.
I would recommend reworking most of the snes_read to just get a few bigger blocks of memory that have everything you need all at once.
I was surprised by how much faster it could be getting a block of memory at once instead of individual bytes one at a time.
https://discord.com/channels/731205301247803413/1022545979146252288/1239239492385247252
There was a problem hiding this comment.
It's even better to get some memory that you don't need to make fewer trips.
example:
If you need 101, 102, 117, and 118
it's probably better to just get all of 101-118
even though you don't need 103-116
There was a problem hiding this comment.
Another reason to do this rework to read memory in fewer bigger chunks:
The more times someone has to travel across town, the more likely they are to get in an accident and not return.
$ python -m pip install --upgrade mypy
$ python -m mypy --check-untyped-defs --follow-imports=silent --allow-untyped-def --disable-error-code=assignment --disable-error-code=operator --disable-error-code=comparison-overlap --disable-error-code=attr-defined worlds/mmx/Client.pyThis reports 104 errors and almost all of these are real bugs that will happen regularly.
I have seen from someone else an indication that MMX has one of the most unstable clients of all SNI clients.
From this, I can see why.
I recommend that you not try to handle these mypy error reports until after the rework that I suggested, because afterwards there will be fewer error reports to handle.
There was a problem hiding this comment.
Ahhh, that makes so much sense with some reports I've received before. I do assume those show up less frequently in powerful machines, as I've not seen my client(s) being unstable on my end at all.
I'm gonna rework X1's client (and the other two as well, they're pretty much the same code), I believe I can group several of those as most of the RAM reads regarding collected locations and upgrades are almost in the same block of RAM.
|
Don't know if it's actually stable as I never faced its unstability, but seems to be working fine. Also I got Scipio's requested changes for X3 ported to X1, I'm focusing on one game at the time and since all of them share almost the very same code, functions, etc. I can port the changes to X2 and X3 with ease after this one is given a thumbs up with the changes. |
|
There was some discussion that I brought up in the ap-world-dev channel regarding my usage of the hint system. Link to the conversation. I'm using the current hint system to also display boss weaknesses in case they're shuffled via the Entrance field. This lead to me placing a location-item pair which triggers the goal when the item is obtained, which then users could plando or start_inventory into their games. ALTTP (?), SMW and DKC3 do this and just that day I realized that it was actually frowned upon so I'm gonna be changing it.
I did reach to the conclusion of recreating what HintMachine does for my own Weakness Hint System (tm), I haven't started worked on it, so I'd like for future reviewers to skip comments on that if possible as it's getting a rework at some point... or in case people want to place their own opinion on the aformentioned matter. My opinion on that it's that it's on the user end if they want to screw up their yamls like that, but if it'd be a blocker for this world to be approved & merged into main, well I gotta agree with the people who use the Archipelago API more often/know it like the back of their hands :P |
| itempool += [self.create_item(ItemName.stage_sigma_fortress)] | ||
|
|
||
| # Add weapons into the pool | ||
| itempool += [self.create_item(ItemName.electric_spark)] |
There was a problem hiding this comment.
I think all of these itempool += [ would be better as itempool.append( instead.
That way it doesn't create a new list for every +=
| def create_item(self, name: str, force_classification: ItemClassification = False) -> Item: | ||
| data = item_table[name] | ||
|
|
||
| if force_classification: | ||
| classification = force_classification |
There was a problem hiding this comment.
This is a dangerous interface because it doesn't allow you to force something to be filler.
I see that you may not want to force any filler right now, but someone might be surprised by a bug if something is changed in the future.
| def create_item(self, name: str, force_classification: ItemClassification = False) -> Item: | |
| data = item_table[name] | |
| if force_classification: | |
| classification = force_classification | |
| def create_item(self, name: str, force_classification: ItemClassification = False) -> Item: | |
| data = item_table[name] | |
| if isinstance(force_classification, ItemClassification): | |
| classification = force_classification |
This change would allow forcing something to be filler.
| if hasattr(self.multiworld, "generation_is_fake"): | ||
| if hasattr(self.multiworld, "re_gen_passthrough"): | ||
| if "Mega Man X" in self.multiworld.re_gen_passthrough: | ||
| self.boss_weaknesses = self.multiworld.re_gen_passthrough["Mega Man X"]["weakness_rules"] |
There was a problem hiding this comment.
Most other games that do things like this for UT include a comment explaining that it's for UT, and maybe also a little explanation in the comment for why it's needed.
| build_rules(self) | ||
|
|
||
|
|
||
| def fill_slot_data(self) -> Dict[int, Any]: |
There was a problem hiding this comment.
| def fill_slot_data(self) -> Dict[int, Any]: | |
| def fill_slot_data(self) -> Dict[str, Any]: |
I wonder how that got there...
There was a problem hiding this comment.
oops xd
I'm too new to typing and all that stuff.
| weaknesses = "" | ||
| for i in range(len(data)): | ||
| weaknesses += f"{weapon_id[data[i][1]]}, " | ||
| weaknesses = weaknesses[:-2] |
There was a problem hiding this comment.
| weaknesses = "" | |
| for i in range(len(data)): | |
| weaknesses += f"{weapon_id[data[i][1]]}, " | |
| weaknesses = weaknesses[:-2] | |
| weaknesses = ", ".join([weapon_id[weak[1]] for weak in data]) |
I think this will give the same result. You should test to make sure.
| if amount: | ||
| try: | ||
| amount = int(amount) | ||
| except: |
There was a problem hiding this comment.
| except: | |
| except ValueError: |
What is this fixing or adding?
Implements Mega Man X in Archipelago.
How was this tested?
Months of weekly sessions in private and public games
If this makes graphical changes, please attach screenshots.