-
Notifications
You must be signed in to change notification settings - Fork 992
Stick Ranger: implement new game #5115
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
base: main
Are you sure you want to change the base?
Conversation
…s sent to the front
…s sent to the front
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.
Brief look while doing other stuff
world_map_exit: Entrance = Entrance( | ||
self.player, region_name, world_map_region | ||
) | ||
if region_name != "Opening Street": | ||
world_map_exit.access_rule = make_unlock_rule(region_name) | ||
world_map_region.exits.append(world_map_exit) | ||
world_map_exit.connect(region) |
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.
world_map_exit: Entrance = Entrance( | |
self.player, region_name, world_map_region | |
) | |
if region_name != "Opening Street": | |
world_map_exit.access_rule = make_unlock_rule(region_name) | |
world_map_region.exits.append(world_map_exit) | |
world_map_exit.connect(region) | |
world_map_exit = world_map_region.connect(region) | |
if region_name != "Opening Street": | |
world_map_exit.access_rule = make_unlock_rule(region_name) |
Pretty sure this does the same thing
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.
Sadly this is not the same. world_map_region.connect(region)
has return type None, so it does not work (tested with fuzzer, 100% fails)
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 just looked in BaseClasses, Region.connect returns the entrance it created.
https://github.com/ArchipelagoMW/Archipelago/blob/main/BaseClasses.py#L1300
Is world_map_region not a Region or something?
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.
Few more things while on break.
def class_count(state, player: int) -> int: | ||
"""Return the number of ranger classes the player has unlocked.""" | ||
return sum(state.has(f"Unlock {cls} Class", player) for cls in RANGER_CLASSES) |
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.
Would recommend putting the classes in an item name group, then just using state.has_group
or similar.
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.
(or has_from_list if they don't make sense as item groups)
def reached_castle( | ||
player: int, threshold: int, req_classes: int | ||
) -> Callable[[CollectionState], bool]: | ||
"""Return whether or not the player has unlocked access to the Castle stage yet.""" | ||
return lambda state, _pl=player, _T=threshold, _keys=unlocks_by_region[ | ||
"Grassland" | ||
]: ( | ||
state.has("Unlock Castle", _pl, 1) | ||
and sum(1 for k in _keys if state.has(k, _pl, 1)) >= _T | ||
and class_count(state, _pl) >= req_classes | ||
) |
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.
Consider just using state.can_reach_region
instead of this and the similar helpers below.
class StagesReqForIceCastle(Range): | ||
""" | ||
Actual number of required pre-Ice Castle stages to beat. | ||
Only used for rules and tracking. | ||
""" | ||
|
||
display_name = "Actual number of required pre-Ice Castle stages" | ||
visibility = Visibility.none | ||
range_start = 0 | ||
range_end = 14 | ||
default = 5 |
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 a player be allowed to change this option? If not, it probably shouldn't be an option, even if it's hidden. You can store information on the world class if you need to.
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 this, visibility none options will be editable manually in the yaml by the player if they know about the key,
another alternative to saving these values on the world instance is Option instances can also have extra data stored on them if that makes more sense for your organization
### Where do I get a YAML file? | ||
|
||
You can customize your settings by visiting the | ||
[Stick Ranger Player Options Page](/games/Stick%20Ranger/player-options). |
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.
[Stick Ranger Player Options Page](/games/Stick%20Ranger/player-options). | |
[Stick Ranger Player Options Page](../player-options). |
Would recommend just using the relative path instead
|
||
### Where do I get a YAML file? | ||
|
||
You can customize your settings by visiting the |
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 can customize your settings by visiting the | |
You can customize your options by visiting the |
AP insists on calling player options options rather than settings.
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.
didn't review Rules.py heavily yet, but one more overarching comment is that module names in python should be lowercase/snake_case, and earlier is better for changing those types of things
worlds/stick_ranger/test/__init__.py
Outdated
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.
really new guidance, but test base classes should be defined in a bases.py
and the __init__.py
in the test folder shall be empty
from . import StickRangerTestBase | ||
|
||
|
||
class StickRangerTest(StickRangerTestBase): |
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.
all of these test cases should already be covered by those in the base test
folder, what is the purpose for these particular tests?
|
||
### Option 2: Run the Game Locally | ||
|
||
If the website is unavailable, or you wish to use your own copy: |
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.
probably worth mentioning that (since github.io doesn't allow for http connections and most browsers don't allow for ws on https connections) another reason you would need to run locally is to connect to non-webhost rooms (knowing that is a simplification of when ws is used for rooms)
- npm: command not found? | ||
Make sure Node.js and npm are installed and added to your system PATH. | ||
- Port in use error: | ||
If you get an error that the port is already in use, either stop the other process or use `npm run dev -- --port=YOURPORT` to specify another port. | ||
- Game not loading? | ||
Double-check you are in the correct folder and all files were extracted. |
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 properly bullet or use some other formatting, I believe on webhost this will not render like github does
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 should try to make these md docs also fit in the 120 character per line style guide, especially with how easy it is to do so in markdown
worlds/stick_ranger/__init__.py
Outdated
for _ in range(trap_count): | ||
trap: TrapItemData = self.multiworld.random.choices( | ||
traps, weights=trap_weights, k=1 | ||
)[0] | ||
itempool.append(self.create_item(trap.item_name)) |
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're already using random.choices, why not just use k=trap_count
?
(also as scipio has mentioned elsewhere you should use world.random)
worlds/stick_ranger/__init__.py
Outdated
# Add Filler | ||
while len(itempool) < self.location_count: | ||
itempool.append( | ||
self.create_item(self.multiworld.random.choice(filler).item_name) |
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.
unrelated you should define a get_filler_item_name (as otherwise any items created by core could be any item name in your entire datapackage which is often unwanted behaviour), and once you do this whole line can just be self.create_filler()
self._generate_randomness() | ||
|
||
def create_regions(self) -> None: | ||
menu_region: Region = Region("Menu", self.player, self.multiworld) |
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 weren't aware, a recent-ish change allows worlds to define an origin_region_name that is not Menu
and remove the need for that region altogether, from a quick glance it seems like you aren't using Menu for anything in particular so that change could clean up some of this boilerplate
option_boxer = "Boxer" | ||
option_gladiator = "Gladiator" | ||
option_sniper = "Sniper" | ||
option_magician = "Magician" | ||
option_priest = "Priest" | ||
option_gunner = "Gunner" | ||
option_whipper = "Whipper" | ||
option_angel = "Angel" |
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.
choice option values should be ints
class StagesReqForIceCastle(Range): | ||
""" | ||
Actual number of required pre-Ice Castle stages to beat. | ||
Only used for rules and tracking. | ||
""" | ||
|
||
display_name = "Actual number of required pre-Ice Castle stages" | ||
visibility = Visibility.none | ||
range_start = 0 | ||
range_end = 14 | ||
default = 5 |
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 this, visibility none options will be editable manually in the yaml by the player if they know about the key,
another alternative to saving these values on the world instance is Option instances can also have extra data stored on them if that makes more sense for your organization
What is this fixing or adding?
An integration of the Stick Ranger game as an APWorld.
How was this tested?
I ran all the tests, they passed (Python 3.12.10)
Also the fuzzer was ran, ~1% fail rate with more than 1 game due to accessibility settings.
If this makes graphical changes, please attach screenshots.