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

Inscryption: Implement new game #3621

Open
wants to merge 51 commits into
base: main
Choose a base branch
from

Conversation

DrBibop
Copy link

@DrBibop DrBibop commented Jul 5, 2024

What is this fixing or adding?

Adds new world for the game Inscryption.
https://store.steampowered.com/app/1092790/Inscryption/

Learn more about the implementation here: https://github.com/Glowbuzz/Archipelago/blob/Inscryption/worlds/inscryption/docs/en_Inscryption.md

How was this tested?

Has been available publicly and regularly tested by the community and the world developers since september of 2023 through our future-game-design thread. It has been included in multiple community asyncs as a stable unsupported game. Unit tests have also been added for logic.

Glowbuzz and others added 30 commits July 4, 2023 03:33
…). Add a rule for victory. Change the regions list to remove menu in the destination.
…ms as "progression", renamed tomb checks, added more location rules, re-added completion rule
# Conflicts:
#	README.md
… tower requirement for Mycologist Key check. Fixed missing comma in act 2 locations oopsies.
…ons + made pieces progressive + adjusted docs
# Conflicts:
#	README.md
# Conflicts:
#	README.md
…ndomize abilities to randomize sigils. Fixed generation issue with epitaph pieces randomization. Goobert's painting no longer forces filler. Removed traps option for now. Reworded some option descriptions.
# Conflicts:
#	README.md
@github-actions github-actions bot added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Jul 5, 2024
@DrBibop
Copy link
Author

DrBibop commented Jul 5, 2024

Some of the latest changes do still need some testing but I'm starting the PR anyway in case anything needs changing

@remyjette remyjette added the is: new game Pull requests for implementing new games into Archipelago. label Jul 5, 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.

Out of time to keep reviewing for now, may review more later

worlds/inscryption/Options.py Outdated Show resolved Hide resolved
worlds/inscryption/Options.py Outdated Show resolved Hide resolved
worlds/inscryption/Rules.py Outdated Show resolved Hide resolved
worlds/inscryption/Rules.py Outdated Show resolved Hide resolved
Comment on lines 140 to 156
def has_pelts(self, state: CollectionState, count: int) -> bool:
return state.has("Holo Pelt", self.player, count)

def has_one_pelt(self, state: CollectionState) -> bool:
return self.has_pelts(state, 1) and self.has_gems_and_battery(state)

def has_two_pelt(self, state: CollectionState) -> bool:
return self.has_pelts(state, 2) and self.has_gems_and_battery(state)

def has_three_pelt(self, state: CollectionState) -> bool:
return self.has_pelts(state, 3) and self.has_gems_and_battery(state)

def has_four_pelt(self, state: CollectionState) -> bool:
return self.has_pelts(state, 4) and self.has_gems_and_battery(state)

def has_five_pelt(self, state: CollectionState) -> bool:
return self.has_pelts(state, 5) and self.has_gems_and_battery(state)
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 has_pelts(self, state: CollectionState, count: int) -> bool:
return state.has("Holo Pelt", self.player, count)
def has_one_pelt(self, state: CollectionState) -> bool:
return self.has_pelts(state, 1) and self.has_gems_and_battery(state)
def has_two_pelt(self, state: CollectionState) -> bool:
return self.has_pelts(state, 2) and self.has_gems_and_battery(state)
def has_three_pelt(self, state: CollectionState) -> bool:
return self.has_pelts(state, 3) and self.has_gems_and_battery(state)
def has_four_pelt(self, state: CollectionState) -> bool:
return self.has_pelts(state, 4) and self.has_gems_and_battery(state)
def has_five_pelt(self, state: CollectionState) -> bool:
return self.has_pelts(state, 5) and self.has_gems_and_battery(state)
def has_pelts(self, state: CollectionState, count: int) -> bool:
return state.has("Holo Pelt", self.player, count) and self.has_gems_and_battery(state)

Having 5 separate has_num_pelt helpers feels excessive -- would recommend condensing them into one function.

Copy link
Author

Choose a reason for hiding this comment

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

I agree that it's excessive. If I remember correctly, I think it was because my rules list is of the type dict[str, (CollectionState) -> bool] which doesn't allow me to add parameters like the number of pelts. If you know of a way, that would be great.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, well, I have no idea then -- this is probably fine then to keep using that kind of structure. I'm definitely no stranger to having to do some hacky thing to keep everything else smooth

Copy link
Collaborator

@remyjette remyjette Jul 11, 2024

Choose a reason for hiding this comment

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

Have has_pelts only take the count, then return a lambda that captures the count and takes a CollectionState to return a bool

Then, instead, of "Act 3 - Trader 1": self.has_one_pelt you would call the function to get that lambda of type Callable[[CollectionState], bool] to put in your rules dict
"Act 3 - Trader 1": self.has_pelts(1)

Basically my thoughts are something along the lines of

    def has_pelts(self, count: int) -> Callable[[CollectionState], bool]:
        return lambda state: state.has("Holo Pelt", self.player, count)

Copy link
Author

Choose a reason for hiding this comment

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

Pushed in the latest commit. Works wonderfully!

from .Locations import act1_locations, act2_locations, act3_locations, regions_to_locations
from .Regions import inscryption_regions_all, inscryption_regions_act_1
from typing import Dict, Any
from . import 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
from . import Rules
import .Rules

Not 100% sure this is correct since I'm not in front of an IDE atm

Copy link
Author

@DrBibop DrBibop Jul 10, 2024

Choose a reason for hiding this comment

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

Simply removing "from . " seems to work. No dot needed before Rules.

Edit: Nevermind, that doesn't work either.

Copy link
Author

Choose a reason for hiding this comment

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

I'm assuming this should be okay as it is? Gonna mark this as resolved if that's the case.

…ion, cleaned up imports, renamed Death Link related options to include "death_link" instead of "deathlink", replaced numeral values for option checking into class attributes for readability, slight optimization to tower rule, fixed typo in codes option description.
worlds/inscryption/__init__.py Outdated Show resolved Hide resolved
worlds/inscryption/docs/en_Inscryption.md Outdated Show resolved Hide resolved
worlds/inscryption/docs/en_Inscryption.md Outdated Show resolved Hide resolved
@@ -0,0 +1,19 @@
# Inscryption
Copy link
Collaborator

Choose a reason for hiding this comment

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

In general, I recommend having a section detailing how many locations are in your world, since it is a common question among users. Since it varies by options, I'd recommend either saying how many locations each option has, or give a range and a default if it's more complicated.

Copy link
Author

Choose a reason for hiding this comment

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

Added a section for it. Let me know if it looks good.

worlds/inscryption/docs/setup_en.md Show resolved Hide resolved
worlds/inscryption/Options.py Show resolved Hide resolved
…ion, added items/locations count in game docs and adjusted some sections
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.

None yet

4 participants