Landstalker: implement new game #1808
Conversation
- Improved shop pricing of remote Landstalker items - Changed item name - Updated logic
|
I think pretty much everything was solved or discussed, and I also fixed a small edge-case problem encountered by a user who tried the APWorld. |
|
|
That's weird, I don't remember having touched the security settings of my fork. I'll have a look |
|
It should be good, branch is not protected anymore 👍 |
worlds/landstalker/__init__.py
Outdated
| option_definitions = ls_options | ||
| topology_present = True | ||
| data_version = 1 | ||
| required_client_version = (0, 3, 8) |
worlds/landstalker/__init__.py
Outdated
| game = "Landstalker - The Treasures of King Nole" | ||
| option_definitions = ls_options | ||
| topology_present = True | ||
| data_version = 1 |
There was a problem hiding this comment.
This has been deprecated for a while now so can be removed
worlds/landstalker/__init__.py
Outdated
| self.can_fill_slot_data.wait() | ||
|
|
||
| # Put options, locations' contents and some additional data inside slot data | ||
| slot_data = {option_name: self.get_setting(option_name).value for option_name in ls_options} |
There was a problem hiding this comment.
we have self.options.as_dict now
|
|
||
| add_specific_paths(multiworld, player, regions_table) | ||
|
|
||
| return regions_table |
There was a problem hiding this comment.
as far as i can tell this mapping is just so you can reference the actual regions from the internal names right? it's been a while since i looked at this, sorry 😓 before i think this would've been fine, but now there's a new caching mechanism on main, so it'd be more efficient for you to just refer to your already existing mapping of internal name to actual name and call get region as needed, instead of creating a cache of them. if you're doing this specifically because you're unsure if the region itself actually exists, then it'd probably need to be tested, but my gut says it'd still be a bit faster to just try, except get those regions than create this mapping.
worlds/landstalker/Regions.py
Outdated
|
|
||
| def add_specific_paths(multiworld: MultiWorld, player: int, regions_table: Dict[str, LandstalkerRegion]): | ||
| # If Gumi boulder is removed, add a path from "route_gumi_ryuma" to "gumi" | ||
| if multiworld.remove_gumi_boulder[player].value == 1: |
There was a problem hiding this comment.
there's still some more of these comparisons to integers that don't really mean anything
worlds/landstalker/Regions.py
Outdated
| from_region = regions_table[from_id] | ||
| to_region = regions_table[to_id] | ||
|
|
||
| entrance = Entrance(player, name, from_region) |
There was a problem hiding this comment.
if the helper returned the created entrance i think this would solve this issue for you, right? currently i don't think it returns anything so that should be perfectly fine, and make this quite a bit more readable.
edit: #2406 if you want to test it for me :)
worlds/landstalker/__init__.py
Outdated
| entrance.access_rule = make_path_requirement_lambda(self.player, [], [self.regions_table['tibor']]) | ||
|
|
||
| def set_rules(self): | ||
| Rules.create_rules(self.multiworld, self.player, self.regions_table, self.dark_region_ids) |
There was a problem hiding this comment.
if typing.TYPE_CHECKING
| if location.parent_region.name in EXCLUDED_REGIONS: | ||
| location.progress_type = LocationProgressType.EXCLUDED | ||
|
|
||
| def get_starting_health(self): |
There was a problem hiding this comment.
what i meant was line 119 where this is actually called
worlds/landstalker/__init__.py
Outdated
| sphere_count = len(spheres) | ||
| for sphere in spheres: | ||
| for location in sphere: | ||
| if location.player != self.player or location.type_string != 'shop': |
There was a problem hiding this comment.
should this be and? you're calling this method per player
There was a problem hiding this comment.
alternatively, use stage_post_fill to do this for all landstalker players at once. get_spheres() isn't cached, so calling it multiple times is very expensive.
There was a problem hiding this comment.
the caching part is addressed with #2410, but i still think just looping through it a single time for all landstalker worlds should be more performant, if possible.
|
Hey, |
|
I have left a PR on your main branch which includes my remarks and most changes I would have requested anyway. Please let me know once you have reviewed them and I'll revisit this PR. |
Landstalker - Requested Changes/Remarks during Pull Request Review
|
When possible, should still increase BASE_ID, but this should be fine to at least start testing in upcoming RCs. |
Co-authored-by: Anthony Demarcy <[email protected]> Co-authored-by: Phar <[email protected]>
What is this fixing or adding?
This adds Landstalker - The Treasures of King Nole to Archipelago. This is a unique isometric ARPG with a well-built open world that fits well into Archipelago set of games.
This only contains the AP world, since the client is an external program:
https://github.com/Dinopony/randstalker-archipelago
How was this tested?
This was tested extensively with a group of friends over dozens of seeds (both LS-only and with multiple games), which allowed to squash all the bugs that could be found.
All unit tests pass.