-
Notifications
You must be signed in to change notification settings - Fork 703
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
Saving Princess: implement new game #3238
Conversation
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.
Just a quick skim, looks pretty great so far. Will review it in more depth later.
Co-authored-by: Scipio Wright <[email protected]>
Co-authored-by: Scipio Wright <[email protected]>
RegionData was only wrapping a List[str], so we can directly use List[str]
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.
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.
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.
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
Co-authored-by: Scipio Wright <[email protected]>
Co-authored-by: Nicholas Saylor <[email protected]>
Co-authored-by: Nicholas Saylor <[email protected]>
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
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. |
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.
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.
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.
Incredibly nice implementation. I have one important request and two minor comments that aren't important.
Previously, gm-apclientpp.dll was downloaded from its own repo With this update, the dll is instead extracted from the same zip as the game's patch
given it's past brainos
automatic connection support was added, docs now reflect this
also parses the slot, password and host:port into parameters for the game
This keeps the game from freezing the launcher while it is running
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.