Saving Princess: implement new game #3238

LeonarthCG · 2024-04-30T18:30:25Z

What is this fixing or adding?

Adds support for Saving Princess to Archipelago.

How was this tested?

The world was tested with an async that had 3 players run the game, together with another 6 games. Only two major issues were found during this, which have since been fixed.

Over the past month and a half I have also played seeds by myself, testing every option by running the game on its own, with other slots of the game and also with other supported and unsupported games.

If this makes graphical changes, please attach screenshots.

ScipioWright

Just a quick skim, looks pretty great so far. Will review it in more depth later.

worlds/saving_princess/__init__.py

worlds/saving_princess/docs/en_Saving Princess.md

worlds/saving_princess/docs/setup_en.md

worlds/saving_princess/Regions.py

Co-authored-by: Scipio Wright <scipiowright@gmail.com>

worlds/saving_princess/Rules.py

RegionData was only wrapping a List[str], so we can directly use List[str]

worlds/saving_princess/Options.py

ScipioWright

Small game, the apworld is written very cleanly. Unit tests passed fine, no errors when genning lots of random seeds. I did not test the client at all.

nicholassaylor

Nature of Review

Only Options.py and the documentation was reviewed
Documentation was checked through an editor and verified through WebHost

Options

Looking over the options, I did not see anything of concern, the tooltips were self-explanatory and the types made sense.

Documentation

Documentation, particularly in the game page is fairly good. I left some comments that I think will make the pages a bit more readable and condense information.

Client

Looking over the set-up guide, I see that a .bs4diff file is used to patch the data.win file of a Gamemaker game. I see that there are concerns about assets and code being included, but you may want to look at the Undertale implementation for patching or AP Procedure Patch (APPP) to see if you can integrate the patching process into either a launcher for patching or a standalone program. I don't consider this a requirement for merging, but it may make the game more approachable for randomizing.

Additional Housekeeping

The following things should also get done before merging

Add game to README.md
Add your implementation to CODEOWNERS

worlds/saving_princess/docs/en_Saving Princess.md

worlds/saving_princess/__init__.py

worlds/saving_princess/Items.py

Co-authored-by: Scipio Wright <scipiowright@gmail.com>

Co-authored-by: Nicholas Saylor <79181893+nicholassaylor@users.noreply.github.com>

nicholassaylor

Nature of Review

All aspects of previous review were re-reviewed
Client was briefly looked over

Documentation

Changes look good, very minor changes suggested, otherwise very good

Client

Client now has an automated patch feature while still retaining the option to patch manually. I only took a cursory look over the patching process and didn't see any glaring issues. I was not able to test patching as I do not own the game.

Options

No changes since last review, however with #2614 being merged after my last review, it may be a good idea to take a look at Option Groups and Presets before merging. Not a requirement, but looking at the dataclass for your options, it seems like Option Groups would serve you well.

Housekeeping

Game was added to README.md and CODEOWNERS but a merge conflict on README.md needs to be resolved. Minor inconvenience at most.

Overall, the concerns that I had previously were addressed and any remaining reservations are extremely minor and/or optional.

nicholassaylor · 2024-06-06T03:16:29Z

README.md

This has a merge conflict and needs to be resolved

nicholassaylor · 2024-06-06T03:19:20Z

SavingPrincessClient.py

I believe that in general, core is trying to shift away from clients in the root AP directory and instead having them open through the launcher.

nicholassaylor · 2024-06-06T03:28:23Z

worlds/saving_princess/docs/setup_en.md

+3. Launch `ArchipelagoLauncher` or `ArchipelagoSavingPrincessClient` from your Archipelago installation folder
+4. If you launched `ArchipelagoLauncher`, click on "Saving Princess Client"
+   * You will probably need to scroll down on the Clients column to see it


If you end up moving the client to launcher only, this would need to be changed

nicholassaylor · 2024-06-06T03:33:11Z

worlds/saving_princess/docs/setup_en.md

+
+### Where do I get a YAML file?
+
+You can customize your options by visiting the [Saving Princess Player Options Page](/games/Saving Princess/player-options).


Suggested change

You can customize your options by visiting the [Saving Princess Player Options Page](/games/Saving Princess/player-options).

You can customize your options by visiting the [Saving Princess Player Options Page](/games/Saving%20Princess/player-options).

Fix broken link

Bump (broken link)

nicholassaylor · 2024-06-06T03:38:58Z

worlds/saving_princess/Client.py

+    valid_install = is_install_valid()
+    if not valid_install:


Suggested change

valid_install = is_install_valid()

if not valid_install:

if not is_install_valid():

No need to hold onto this value since it's not used elsewhere

Exempt-Medic

A lot of minor things and mostly docs. Overall this all looks very good, though some of the helper functions, like get_location confuse me. Only major thing was that you're putting events in the name_to_id lookups. I looked at everything except Client.py and have not done any generations yet.

Something I forgot to comment on: The trap change is simply a flat chance, so if there are 10 filler items and 50% traps, there are not assuredly 5 traps, but there could be any number 0-10. Is this intended?

Exempt-Medic · 2024-07-28T23:49:54Z

worlds/saving_princess/docs/en_Saving Princess.md

+
+Some locations, such as boss kills, have no visual representation, but those that do will have the Archipelago icon.
+
+Once the item is picked up a textbox will inform you of the item that was found as well as the player that will be receiving it.


Missing comma

Suggested change

Once the item is picked up a textbox will inform you of the item that was found as well as the player that will be receiving it.

Once the item is picked up, a textbox will inform you of the item that was found as well as the player that will be receiving it.

Exempt-Medic · 2024-07-28T23:51:10Z

worlds/saving_princess/docs/en_Saving Princess.md

+This textbox shows both which item you got and which player sent it to you.
+
+If you send an item to yourself, however, the sending player will be omitted.
+If the items are cheated in, no textbox will be displayed.


Is there a reason for this? What counts as "cheated in" here? Does start_inventory count?

By cheated in I meant anything that has the server as the player that found it. So, yeah, start_inventory would count for that. I found it annoying to see all the pop-ups when using start_inventory, but perhaps this would just be confusing instead?

I don't feel strongly one way or the other, so if this is outside of the norm of what other worlds do then I'm happy to change it.

It would probably be good to show the popup always, at least so the case of /send and /get_item give immediate feedback to the player. The games I've played do show start inventory items as well, but I'm not sure if that's typical across AP.

Exempt-Medic · 2024-07-28T23:54:56Z

worlds/saving_princess/docs/setup_en.md

+
+### Where do I get a YAML file?
+
+You can customize your options by visiting the [Saving Princess Player Options Page](/games/Saving Princess/player-options).


Bump (broken link)

Exempt-Medic · 2024-07-28T23:58:19Z

worlds/saving_princess/docs/setup_en.md

+### Set up the connection details
+
+After launching the game, enter the Archipelago options menu through the in-game button with the Archipelago icon.
+From here, enter the different menus and type in the **server:port**, **slot**, as well as the **password**, if one is required.


An example of the exact connection details required could be quite helpful. Are all three of these in their own boxes and password is left empty if one doesn't exist?

This is what I came up with, I'm not sure if this is what you had in mind, so I'm asking before committing anything:

After launching the game, enter the Archipelago options menu through the in-game button with the Archipelago icon.
From here, enter the different menus and type in the following details in their respective fields:

server:port (e.g. archipelago.gg:38281)

If hosting on the website, the server and port will be shown in your created room.

slot name (e.g. Player)

This is your player name, which you chose along with your player options.

password (e.g. 123456)

If the room does not have a password, it should be left empty.

This configuration persists through launches and even updates.

That looks great to me!

Exempt-Medic · 2024-07-29T00:00:10Z

worlds/saving_princess/docs/setup_en.md

+If the game is unable to reconnect, save and restart.
+
+Although you can keep playing while disconnected, you won't get any items until you reconnect, not even items found in your own game.
+Once reconnected, however, all of your progress should sync up.


I see the use of "should" here, if there's some kind of thing to do if it doesn't sync, it would be good to list that here

By "should sync up" I was trying to say that gm-apclientpp would take care of it.
I guess I should just say "will sync up"? Because if we assume the mod, the client and the protocol all work, then it will sync up, and if doesn't I don't think there's anything the user could really do to help it anyway?

The only way I've managed to make it not sync properly in my testing has been by wiping all of the local server's data and then starting it back up, but if it comes to that I'm not sure there's much a user can do about it.

Changing to "will" makes sense to me then

Exempt-Medic · 2024-07-29T00:53:37Z

worlds/saving_princess/__init__.py

+    item_name_to_id = {key: value.code for key, value in Items.item_dict.items()}
+    location_name_to_id = {key: value.code for key, value in Locations.location_dict.items()}


These include events, which should be removed

Exempt-Medic · 2024-07-29T00:58:03Z

worlds/saving_princess/__init__.py

+    settings: ClassVar[SavingPrincessSettings]
+
+    is_pool_expanded: bool = False
+    music_table: List[int] = [i for i in range(16)]


This is faster

Suggested change

music_table: List[int] = [i for i in range(16)]

music_table: List[int] = list(range(16))

Exempt-Medic · 2024-07-29T01:04:19Z

worlds/saving_princess/__init__.py

+        # and return one of the names at random
+        return self.random.choice(filler_list)
+
+    def fill_slot_data(self) -> Dict[str, Any]:


You could use self.options.as_dict("death_link", "expanded_pool" "instant_saving", "sprint_availability", "cliff_weapon_upgrade", "ace_weapon_upgrade", "shake_intensity", "iframes_duration") instead here.

Exempt-Medic · 2024-07-29T01:05:39Z

worlds/saving_princess/__init__.py

+    def set_rules(self):
+        from .Rules import set_rules
+        set_rules(self)


This should probably be moved to line up with when it occurs in the call order, namely after create_items / before fill_slot_data

Exempt-Medic · 2024-07-29T01:06:42Z

worlds/saving_princess/Rules.py

+from .Constants import *
+
+
+def set_rules(world: "SavingPrincessWorld"):


Do you have to import SavingPrincessWorld then?

Not sure if you mean I'm missing the import (I think this is what other worlds do/what I'm missing):

from typing import TYPE_CHECKING if TYPE_CHECKING: from . import SavingPrincessWorld

Or if you are asking why I'm using it in the first place?

Yeah, I'm saying you'd want to include that exact code here

Exempt-Medic · 2024-07-29T01:41:57Z

I've now done one thousand generations (wow this game gens fast) and no errors occurred across 250 solo gens and 750 gens of increasing player size. Went over the WebHost display and everything looks fine there as well, you could potentially consider adding option groups or option presets, but it's small enough that it seems alright without.

Exempt-Medic · 2024-07-29T03:08:41Z

Having now played the game, it connects and seems to run just fine, however you're connecting with the AP tag which you likely shouldn't be doing. You also should probably add code in generate_early to check that the player name can actually be input into your game. Could do something like:

from Options import OptionError, PerGameCommonOptions

    def generate_early(self) -> None:
        if not self.player_name.isascii():
            raise OptionError(f"{self.player_name}'s name must be only ASCII.")

Exempt-Medic · 2024-07-29T10:36:51Z

worlds/saving_princess/Items.py

+            self.count = count
+            self.count_extra = count_extra
+
+    def create_item(self, player: int):


Instead of creating these every single time you create an item, why not just make lookup tables directly beforehand that are shared and then use those?

Sorry, I'm not sure I follow.
I think you mean count and count_extra? But I'm pretty sure those are only touched once per entry in my item dictionary, which is the only place ItemData instances are made (unless I made a mistake somewhere).

Or maybe it's my class naming that is confusing? ItemData holds data used to create SavingPrincessItem instances, but the SavingPrincessItem class doesn't have ItemData in it.

I'm saying that every time you call create_item the index and name variables get totally rebuilt. But since the item dict is static, you could just create these once, right after making the item dict itself instead of creating them many times

I understand now. Maybe something like this would make more sense:

def create_item(self, player: int): return SavingPrincessItem(item_data_names[self], self.item_class, self.code, player) [... all of the ItemData dict stuff ...] item_data_names: Dict[ItemData, str] = {value: key for key, value in item_dict.items()}

Looks good to me

LeonarthCG · 2024-09-28T17:07:00Z

@Exempt-Medic

Something I forgot to comment on: The trap change is simply a flat chance, so if there are 10 filler items and 50% traps, there are not assuredly 5 traps, but there could be any number 0-10. Is this intended?

This is intentional, and I believe the docstring ("Likelihood of a filler item becoming a trap.") for the option is clear enough.

Is this design choice a big deal? I figured traps should be a surprise, and knowing the exact number of them kinda goes against that in my mind.

Exempt-Medic · 2024-09-28T17:33:18Z

@Exempt-Medic

Something I forgot to comment on: The trap change is simply a flat chance, so if there are 10 filler items and 50% traps, there are not assuredly 5 traps, but there could be any number 0-10. Is this intended?

This is intentional, and I believe the docstring ("Likelihood of a filler item becoming a trap.") for the option is clear enough.

Is this design choice a big deal? I figured traps should be a surprise, and knowing the exact number of them kinda goes against that in my mind.

I don't think it's a big deal, just a design choice that different devs do wildly differently so i figured it was worth asking about just in case you hadn't considered it.

@Exempt-Medic

as suggested by @Exempt-Medic this includes the removal of events from the item/location name to id, as well as checking for the player name being ASCII

the LaunchCommand option is filled to either the executable or wine with the necessary arguments based on Utils.is_windows

now deletes possible existing files before installing the mod

items sent directly by the server (such as with starting inventory) now have pop-ups just like any other item

LeonarthCG · 2024-09-29T18:42:58Z

Unless I missed something, I believe I tackled all of the concerns.

I also removed the standalone launcher, leaving only the ArchipelagoLauncher one, and added option groups and presets.

Exempt-Medic

Recent changes LGTM, did 100 more singleplayer generations and 100 more 25-player generations. Tested playing through one game. All prior comments were addressed. The Webhost changes look good as well and the presets and option groups make sense to me.

Saving Princess: initial commit

882ff7c

github-actions bot added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Apr 30, 2024

ScipioWright reviewed Apr 30, 2024

View reviewed changes

ScipioWright added the is: new game Pull requests for implementing new games into Archipelago. label Apr 30, 2024

LeonarthCG and others added 2 commits April 30, 2024 21:08

settings -> options

28d6764

Co-authored-by: Scipio Wright <scipiowright@gmail.com>

settings -> options

b522aee

Co-authored-by: Scipio Wright <scipiowright@gmail.com>

ScipioWright reviewed Apr 30, 2024

View reviewed changes

worlds/saving_princess/Rules.py Outdated Show resolved Hide resolved

worlds/saving_princess/Rules.py Outdated Show resolved Hide resolved

LeonarthCG added 4 commits April 30, 2024 21:40

replace RegionData class with List[str]

61b9450

RegionData was only wrapping a List[str], so we can directly use List[str]

world: MultiWorld -> multiworld: MultiWorld

447fe96

use world's random instead of multiworld's

fb198f5

use state's has_any and has_all where applicable

d75e2f5

ScipioWright reviewed Apr 30, 2024

View reviewed changes

worlds/saving_princess/Options.py Outdated Show resolved Hide resolved

ScipioWright approved these changes May 1, 2024

View reviewed changes

LeonarthCG added 3 commits May 1, 2024 02:15

Merge remote-tracking branch 'upstream/main'

53970e6

remove unused StartInventory import

6e962a0

reorder PerGameCommonOptions

9fed35c

nicholassaylor suggested changes May 1, 2024

View reviewed changes

ScipioWright reviewed May 1, 2024

View reviewed changes

worlds/saving_princess/__init__.py Outdated Show resolved Hide resolved

Exempt-Medic reviewed May 1, 2024

View reviewed changes

worlds/saving_princess/Items.py Outdated Show resolved Hide resolved

LeonarthCG and others added 11 commits May 2, 2024 09:52

Merge remote-tracking branch 'upstream/main'

76d2df5

fix relative AutoWorld import

9d1e070

Co-authored-by: Scipio Wright <scipiowright@gmail.com>

clean up double spaces

80d17b1

local commands -> Local Commands

13b2da9

Co-authored-by: Nicholas Saylor <79181893+nicholassaylor@users.noreply.github.com>

Merge branch 'main' of https://github.com/LeonarthCG/Archipelago

aef8a94

remove redundant which items section

e2005ac

Co-authored-by: Nicholas Saylor <79181893+nicholassaylor@users.noreply.github.com>

game info rework

572c87c

clean up item count redundancy

1806c41

Merge remote-tracking branch 'upstream/main'

304a3fe

Merge remote-tracking branch 'upstream/main'

8e357b6

add game to readme and codeowners

4fd6a77

Update setup_en.md

4b8d3c5

nicholassaylor approved these changes Jun 6, 2024

View reviewed changes

ScipioWright added waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer. and removed waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Jun 6, 2024

Exempt-Medic added waiting-on: author Issue/PR is waiting for feedback or changes from its author. and removed waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer. labels Jul 28, 2024

Exempt-Medic suggested changes Jul 29, 2024

View reviewed changes

Exempt-Medic reviewed Jul 29, 2024

View reviewed changes

Merge remote-tracking branch 'upstream/main'

7cd9815

LeonarthCG added 11 commits September 29, 2024 17:40

remove standalone saving princess client

a213c4a

doc fixes

3b76c40

code improvements and redundant default removal

a7983a4

as suggested by @Exempt-Medic this includes the removal of events from the item/location name to id, as well as checking for the player name being ASCII

add option to change launch coammnd

3fd22eb

the LaunchCommand option is filled to either the executable or wine with the necessary arguments based on Utils.is_windows

simplify valid install check

4f30fb8

mod installer improvements

8ef33f2

now deletes possible existing files before installing the mod

Merge remote-tracking branch 'upstream/main'

25b929f

add option groups and presets

4e4b577

add required client version

cf40ec0

update docs about cheat items pop-ups

e5fb5b2

items sent directly by the server (such as with starting inventory) now have pop-ups just like any other item

add Steam Input issue to faq

0b8854c

LeonarthCG requested review from Exempt-Medic and nicholassaylor September 30, 2024 19:44

Exempt-Medic removed the waiting-on: author Issue/PR is waiting for feedback or changes from its author. label Sep 30, 2024

Exempt-Medic approved these changes Sep 30, 2024

View reviewed changes

Exempt-Medic added the waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer. label Sep 30, 2024


		### Where do I get a YAML file?

		You can customize your options by visiting the [Saving Princess Player Options Page](/games/Saving Princess/player-options).

	valid_install = is_install_valid()
	if not valid_install:
	if not is_install_valid():


		Some locations, such as boss kills, have no visual representation, but those that do will have the Archipelago icon.

		Once the item is picked up a textbox will inform you of the item that was found as well as the player that will be receiving it.

	Once the item is picked up a textbox will inform you of the item that was found as well as the player that will be receiving it.
	Once the item is picked up, a textbox will inform you of the item that was found as well as the player that will be receiving it.

		item_name_to_id = {key: value.code for key, value in Items.item_dict.items()}
		location_name_to_id = {key: value.code for key, value in Locations.location_dict.items()}

	music_table: List[int] = [i for i in range(16)]
	music_table: List[int] = list(range(16))

		from .Constants import *


		def set_rules(world: "SavingPrincessWorld"):

Saving Princess: implement new game #3238

Are you sure you want to change the base?

Saving Princess: implement new game #3238

Conversation

LeonarthCG commented Apr 30, 2024 • edited Loading

What is this fixing or adding?

How was this tested?

If this makes graphical changes, please attach screenshots.

ScipioWright left a comment

Choose a reason for hiding this comment

ScipioWright left a comment

Choose a reason for hiding this comment

nicholassaylor left a comment

Choose a reason for hiding this comment

Nature of Review

Options

Documentation

Client

Additional Housekeeping

nicholassaylor left a comment

Choose a reason for hiding this comment

Nature of Review

Documentation

Client

Options

Housekeeping

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Exempt-Medic Jul 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Exempt-Medic left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Exempt-Medic Jul 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Exempt-Medic commented Jul 29, 2024

Exempt-Medic commented Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LeonarthCG commented Sep 28, 2024

Exempt-Medic commented Sep 28, 2024

LeonarthCG commented Sep 29, 2024

Exempt-Medic left a comment • edited Loading

Choose a reason for hiding this comment

LeonarthCG commented Apr 30, 2024 •

edited

Loading

Exempt-Medic Jul 28, 2024 •

edited

Loading

Exempt-Medic left a comment •

edited

Loading

Exempt-Medic Jul 28, 2024 •

edited

Loading

Exempt-Medic commented Jul 29, 2024 •

edited

Loading

Exempt-Medic left a comment •

edited

Loading