-
Notifications
You must be signed in to change notification settings - Fork 722
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
SM64: Move Randomizer Content Update #2569
SM64: Move Randomizer Content Update #2569
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some things I missed the first time through I guess, but nothing major.
Keep in mind my python is not really good, so I might've made some mistakes in the review
worlds/sm64ex/Options.py
Outdated
def getMoveRandomizerOption(action: str): | ||
class MoveRandomizerOption(Toggle): | ||
"""Mario is unable to perform this action until a corresponding item is picked up. | ||
If Mario 64 is built with an alternative "nomoverando" branch, this option will not affect Mario.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather this say something like This option is incompatible with builds using a 'nomoverando' branch
.
The nomoverando
branch prints huge warnings on screen if they attempt to load a game where move rando is enabled, its not like its a cheat code to have it not affect mario (Except if they can ignore that I guess).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. (1cab3ef)
worlds/sm64ex/Regions.py
Outdated
|
||
# List of all courses, including secrets, without BitS as that one is static |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment is at a weird place. Currently looks like it is just referencing sm64_level_to_paintings
, which does not include secrets.
In the first place these arent lists, but dicts from the game-specific entrance format to the name of the entrance.
Does make me realize I made the same mistake just below for sm64secrets though ^^'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed that comment and cleaned up the secrets comment. (f57ef5e)
worlds/sm64ex/Rules.py
Outdated
swapdict.pop(entrance) | ||
|
||
def set_rules(world, player: int, area_connections: dict): | ||
def set_rules(world, player: int, area_connections, star_costs, move_rando_bitvec: int): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you remove the typing annotation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Accident. (007aa81)
self.multiworld.MIPS1Cost[self.player].value, self.multiworld.MIPS2Cost[self.player].value, | ||
self.multiworld.StarsToFinish[self.player].value) | ||
self.multiworld.itempool += [self.create_item("Power Star") for i in range(0,starcount)] | ||
self.multiworld.itempool += [self.create_item("1Up Mushroom") for i in range(starcount,120 - (15 if not self.multiworld.EnableCoinStars[self.player].value else 0))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Items must be created in create_items
, per #1460
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making Starcraft 2 comply with that PR is going to be involve rewriting the whole codebase from start-to-finish. A lot easier to fix it in SM64. Done. (163a61c)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be a bit pedantic: It must be done by create_items, not in create_items - If for some reason you want to make items in generate_early for example, you can do that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you are no longer creating the ExclamationBox items
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you are no longer creating the ExclamationBox items
Why would they not be created? I haven't done much to change the code in that part.
I cannot create the cannons or exclamation box items in create_items if the options result in them getting hard-placed later, as that would be modifying the item pool after create_items.
worlds/sm64ex/Rules.py
Outdated
if randomized_entrances[91] not in sm64_paintings_to_level.keys(): # Unlucky :C (91 -> BoB Entrance) | ||
rand = world.random.choice(sm64_paintings_to_level.values()) | ||
swapdict = {entrance: level for (level, entrance) in randomized_entrances.items()} | ||
valid_first_courses = valid_move_randomizer_start_courses if move_rando_bitvec > 0 else list(sm64_paintings_to_level.keys()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the cast to list necessary here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cast to list is necessary. keys() generates a dict_keys object that behaves differently from a list, and tends to fail on any operation more complicated than iterating through all the elements once (like random.choice()).
worlds/sm64ex/Rules.py
Outdated
randomized_level_to_paintings = shuffle_dict_keys(world,sm64_level_to_paintings) | ||
if world.AreaRandomizer[player].value < 3 and move_rando_bitvec > 0: | ||
first_course = world.random.choice(valid_move_randomizer_start_courses) | ||
for entrance, painting in randomized_level_to_paintings.items(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont like this loop.
Equivalent but to me nicer looking: next(key for key, value in dd.items() if value == 'value')
.
Though tbh I found this on stackoverflow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I normally use comprehensions everywhere, I'm not sure why I didn't do so here. Changed. (29480bd)
worlds/sm64ex/Rules.py
Outdated
def fix_reg(entrance_ids, entrance, destination, swapdict, world): | ||
if entrance_ids[entrance] == destination: # Unlucky :C | ||
def fix_reg(entrance_map, entrance, destination, swapdict, world): | ||
if entrance_map[swapdict[entrance]] == destination: # Unlucky :C |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably be entrance_map[sm64_entrances_to_level[entrance]]
right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm having some trouble figuring out how fix_reg works, and it doesn't seem to be working correctly before or after that change. I'm going to have to take a look at it later; might make an enum for Entrances to make it easier to understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Second round is in
worlds/sm64ex/Rules.py
Outdated
rand = world.random.choice(valid_first_courses) | ||
rand_entrance = swapdict[rand] | ||
randomized_entrances[Entrances.BOB_OMB_BATTLEFIELD], randomized_entrances[rand_entrance] = rand, first_course | ||
swapdict.pop(rand) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
Can probably be simplified to just use fix_reg as well.
fix_reg(randomized_entrances, Entrances.BOB_OMB_BATTLEFIELD, <Courses not in valid_first_courses>, swapdict, world)
You can then also remove the if cond just above, as its checked in fix_reg
worlds/sm64ex/Rules.py
Outdated
swapdict[rand] = entrance_ids[entrance] | ||
swapdict.pop(entrance) | ||
def fix_reg(entrance_map: Dict[Entrances, str], entrance: Entrances, destination: str, | ||
swapdict: Dict[str, Entrances], world, invalid_regions: Set[str] = {}): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you are introducing invalid_regions
, no need to keep destination
parameter; just use that instead. (Put it as third parameter tho, not at the end)
worlds/sm64ex/Rules.py
Outdated
rand_region, rand_entrance = world.random.choice(replacement_regions) | ||
entrance_map[entrance], entrance_map[rand_entrance] = rand_region, destination | ||
swapdict[rand_region] = entrance | ||
swapdict.pop(destination) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about it, this needs to always pop. Otherwise, on subsequent calls an invalid region might get swapped back in (So always pop entrance_map[entrance])
So for any call to fix_reg, after the call the destination for this entrance will no longer change
worlds/sm64ex/Rules.py
Outdated
fix_reg(randomized_entrances, "Cavern of the Metal Cap", "Dire, Dire Docks", swapdict, world) # ... then dont allow COTMC to be mapped to DDD | ||
fix_reg(randomized_entrances, Entrances.BOWSER_IN_THE_FIRE_SEA, "Dire, Dire Docks", swapdict, world) | ||
if randomized_entrances[Entrances.BOWSER_IN_THE_FIRE_SEA] == "Hazy Maze Cave": # If BITFS is mapped to HMC... | ||
fix_reg(randomized_entrances, Entrances.CAVERN_OF_THE_METAL_CAP, "Dire, Dire Docks", swapdict, world, {"Hazy Maze Cave"}) # ... then dont allow COTMC to be mapped to DDD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be simplified using invalid_regions
, if you remove the destination
parameter
fix_reg for BitFS
fix_reg for COTMC (invalid regions both HMC and DDD)
So only 2 calls to fix_reg
worlds/sm64ex/Regions.py
Outdated
locVCutM_table, locBitFS_table, locWMotR_table, locBitS_table, locSS_table | ||
|
||
|
||
class Entrances(int, Enum): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a fan of the name of this enum.
First, Entrances
could also mean Entrances between regions.
Then, Entrances
could also mean the string representations of the entrances (Tiny-Huge Island (Tiny)
).
As a suggestion: The game code refers to the regions as levels (So LEVEL_THI
for example). Something like SM64Levels
?
Addressed the issues in Round 2. Also fixed an issue with the 1Up generation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last thing, otherwise LGTM!
worlds/sm64ex/__init__.py
Outdated
if action == 'Double Jump': continue | ||
if getattr(self.multiworld, f"MoveRandomizer{action.replace(' ','')}")[self.player].value: | ||
max_stars -= 1 | ||
self.multiworld.itempool.append(self.create_item(action)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move this to create_items
? Its technically allowed here, but it would be more consistent with the other items.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missed this comment. Moved item creation to create_items
using the same conventions as checking for items in rules.
I am proud to present: ✅ |
AP 0.4.4 is out so we should be out of feature freeze. @Magnemania please rebase, then this is hopefully good to go |
I, uh, hit the Rebase button in Pycharm, and I don't think it rebased correctly. |
RIP Git History... Well, this was going to be squash merged anyway, so we'll do that manually.
So in the end there's only one commit with all changes. As I've already reviewed everything it should be fine. |
Going to be completely honest with you here, I've been Googling for the past hour and have absolutely no idea how to do that. I've been using PyCharm exclusively to interface with Git, and it doesn't look like it provides a way for me to reset a remote branch; I didn't think it was even possible to reset a branch. How do I do that? |
I dont use GUI with git, and have never used pycharm... If you can use a terminal with git, |
b31694f
to
0df0955
Compare
@@ -40,30 +63,35 @@ def set_rules(world, player: int, area_connections: dict): | |||
fix_reg(randomized_entrances, SM64Levels.CAVERN_OF_THE_METAL_CAP, {"Hazy Maze Cave"}, swapdict, world) | |||
|
|||
# Destination Format: LVL | AREA with LVL = LEVEL_x, AREA = Area as used in sm64 code | |||
# Cast to int to not rely on availability of SM64Levels enum. Will cause crash in MultiServer otherwise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to restore this comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hold on, I think I've figured out why the initial rebase didn't work. Didn't realize I had to force push. Think I might be able to restore somehow...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind, it looks like I could have undid the force push from the command line, but I did it from PyCharm. Comment is back. How do I give you and https://github.com/RBmans credit, again? He assisted with some logic changes in Rules.py.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Co-authored-by: name <[email protected]>
in the commit message for RBman. You can get the mail from his original commit.
I only did minor things so no need
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nvm, he has the anonymized git email on. Ask him if he wants to be credited with or without real email, for both cases you need the address from him
9b0b2e9
to
c3052dd
Compare
Co-authored-by: RBman <[email protected]> Signed-off-by: Magnemania <[email protected]>
c3052dd
to
336a337
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Green Tick number two
Signed-off-by: Magnemania <[email protected]>
* Super Mario 64: Move Randomizer Update Co-authored-by: RBman <[email protected]> Signed-off-by: Magnemania <[email protected]> * Fixed logic for Vanish Cap Under the Moat Signed-off-by: Magnemania <[email protected]>
* Super Mario 64: Move Randomizer Update Co-authored-by: RBman <[email protected]> Signed-off-by: Magnemania <[email protected]> * Fixed logic for Vanish Cap Under the Moat Signed-off-by: Magnemania <[email protected]>
* Super Mario 64: Move Randomizer Update Co-authored-by: RBman <[email protected]> Signed-off-by: Magnemania <[email protected]> * Fixed logic for Vanish Cap Under the Moat Signed-off-by: Magnemania <[email protected]>
* Super Mario 64: Move Randomizer Update Co-authored-by: RBman <[email protected]> Signed-off-by: Magnemania <[email protected]> * Fixed logic for Vanish Cap Under the Moat Signed-off-by: Magnemania <[email protected]>
What is this fixing or adding?
How was this tested?
The Archipelago community has been openly testing with the distributed APWorld since March of this year, and logic improvements have been implemented in response to community feedback.
If this makes graphical changes, please attach screenshots.