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

Spelunker: Implement New Game #3282

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

Conversation

PinkSwitch
Copy link
Contributor

@PinkSwitch PinkSwitch commented May 9, 2024

What is this fixing or adding?

Implements 'Spelunker' for the NES into AP.

How was this tested?

Technically hasn't been run publicly yet, but I've done personal tests to make sure everything is working, and given how long it takes for PRs to get reviewed, I'm sure if any issues come up I'll be able to fix them while reviews come on. Overall, the world is very small so I don't expect there to be too many problems, if any, and I've already caught the major ones that I've found.

If this makes graphical changes, please attach screenshots.

spelunker_scrn
Sprites courtesy of Seafo

Author's Note:
tis a silly game, made a micro apworld in a 3 day timespan lol

@github-actions github-actions bot added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label May 9, 2024
Copy link
Collaborator

@Zunawe Zunawe 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 few quick client improvements while I've got a few minutes. Didn't test any of the suggested changes.

worlds/spelunker/Client.py Outdated Show resolved Hide resolved
worlds/spelunker/Client.py Outdated Show resolved Hide resolved
worlds/spelunker/Client.py Outdated Show resolved Hide resolved
worlds/spelunker/Client.py Show resolved Hide resolved
return
if "tags" not in args:
return
if "DeathLink" in args["tags"] and args["data"]["source"] != ctx.slot_info[ctx.slot].name:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This means that two players playing on the same slot won't send deathlinks to each other. If you don't care about supporting that, don't worry about 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.

I'll have to think on this one.

worlds/spelunker/Client.py Outdated Show resolved Hide resolved
worlds/spelunker/Client.py Outdated Show resolved Hide resolved
worlds/spelunker/Client.py Outdated Show resolved Hide resolved
worlds/spelunker/Client.py Outdated Show resolved Hide resolved
@Exempt-Medic Exempt-Medic added the is: new game Pull requests for implementing new games into Archipelago. label May 9, 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.

It looks like you've learned a lot from the Yoshi's Island one, every comment here is fairly minor.

worlds/spelunker/__init__.py Outdated Show resolved Hide resolved
worlds/spelunker/Items.py Outdated Show resolved Hide resolved
worlds/spelunker/Locations.py Outdated Show resolved Hide resolved
option_cave_3 = 2
option_cave_4 = 3
default = 0

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would recommend adding StartInventoryPool here too.

Add StartInventoryPool to your imports from Options, then put start_inventory_from_pool: StartInventoryPool in your options dataclass.

worlds/spelunker/Rules.py Outdated Show resolved Hide resolved
worlds/spelunker/docs/en_Spelunker.md Show resolved Hide resolved
worlds/spelunker/docs/setup_en.md Outdated Show resolved Hide resolved
worlds/spelunker/docs/setup_en.md Outdated Show resolved Hide resolved
worlds/spelunker/docs/setup_en.md Outdated Show resolved Hide resolved
worlds/spelunker/local_data.py Outdated Show resolved Hide resolved
@ScipioWright
Copy link
Collaborator

ScipioWright commented May 9, 2024

Oh yeah, also update CODEOWNERS and the AP README

@PinkSwitch
Copy link
Contributor Author

I’ll address all these tonight!

@PinkSwitch PinkSwitch requested a review from ScipioWright May 10, 2024 02:41
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 couple more things
edit: these couple more things were just resolved without comments or changes to the code

worlds/spelunker/Locations.py Show resolved Hide resolved
worlds/spelunker/Rules.py Show resolved Hide resolved
Copy link
Member

@Exempt-Medic Exempt-Medic left a comment

Choose a reason for hiding this comment

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

Various suggestions, mostly small though __init__.py has quite a few unneeded functions that could be cleaned up or commented out

worlds/spelunker/Client.py Show resolved Hide resolved
worlds/spelunker/Client.py Show resolved Hide resolved
worlds/spelunker/Client.py Show resolved Hide resolved
worlds/spelunker/Client.py Show resolved Hide resolved
worlds/spelunker/Client.py Show resolved Hide resolved
Comment on lines +91 to +93
def get_excluded_items(self) -> Set[str]:
excluded_items: Set[str] = set()
return excluded_items
Copy link
Member

Choose a reason for hiding this comment

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

So this just always returns an empty set, so it can likely be removed or just commented out

Comment on lines +105 to +114
def get_item_pool(self, excluded_items: Set[str]) -> List[Item]:
pool: List[Item] = []

for name, data in item_table.items():
if name not in excluded_items:
for _ in range(data.amount):
item = self.create_item_with_correct_settings(name)
pool.append(item)

return pool
Copy link
Member

Choose a reason for hiding this comment

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

Could use a comprehension and just do this:

Suggested change
def get_item_pool(self, excluded_items: Set[str]) -> List[Item]:
pool: List[Item] = []
for name, data in item_table.items():
if name not in excluded_items:
for _ in range(data.amount):
item = self.create_item_with_correct_settings(name)
pool.append(item)
return pool
def get_item_pool(self) -> List[Item]:
return [self.create_item(name) for name, data in item_table.items() for _ in range(data.amount)]


def generate_filler(self, pool: List[Item]) -> None:
for _ in range(len(self.multiworld.get_unfilled_locations(self.player)) - len(pool) - 1):
item = self.create_item_with_correct_settings(self.get_filler_item_name())
Copy link
Member

Choose a reason for hiding this comment

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

You can do this instead:

Suggested change
item = self.create_item_with_correct_settings(self.get_filler_item_name())
item = self.create_filler()

Comment on lines +125 to +126
world = self.multiworld
player = self.player
Copy link
Member

Choose a reason for hiding this comment

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

These are unused

Suggested change
world = self.multiworld
player = self.player

rom_name = getattr(self, "rom_name", None)
if rom_name:
new_name = base64.b64encode(bytes(self.rom_name)).decode()
multidata["connect_names"][new_name] = multidata["connect_names"][self.multiworld.player_name[self.player]]
Copy link
Member

Choose a reason for hiding this comment

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

You can use the new helper function

Suggested change
multidata["connect_names"][new_name] = multidata["connect_names"][self.multiworld.player_name[self.player]]
multidata["connect_names"][new_name] = multidata["connect_names"][self.player_name]

@Exempt-Medic
Copy link
Member

Of course I immediately noticed something else... In init you have locked_locations but this is never used, so it could be removed / commented out

@Exempt-Medic Exempt-Medic added the waiting-on: author Issue/PR is waiting for feedback or changes from its author. label Aug 21, 2024
@Exempt-Medic
Copy link
Member

Comments were addressed in some other branch or something, updates are waiting on PinkSwitch figuring out some conflicts and update stuff

@PinkSwitch PinkSwitch marked this pull request as draft December 1, 2024 04:24
@PinkSwitch
Copy link
Contributor Author

Shelving this for now, need to fix codeowner conflicts, fix some bugs and address comment

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: author Issue/PR is waiting for feedback or changes from its author. 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.

4 participants