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

Setup: new setup experience (read: torch almost all of it) #2268

Merged
merged 4 commits into from
Oct 23, 2023
Merged

Conversation

Berserker66
Copy link
Member

What is this fixing or adding?

https://discord.com/channels/731205301247803413/1158944149685411880

In detail:

  1. Less upfront decisions for the user, who might otherwise face a wall of text and confusion.
  2. All registry hooks are always registered, in the past we've seen users not install clients, assign patch files to emulator and mess up their file associations. This should help by having it always installed.
  3. Everything is available. This currently includes a bunch of .exes, which I'd like to move to the Launcher anyway as an ongoing effort.
  4. People need to use the Launcher for apworlds anyway, this gets us further into that path.
  5. A bunch of links in the start menu and desktop are removed in favor of just leaving the launcher.
  6. There is less customizability, and those who did not install everything will have a slightly larger AP on their drive. Since we're not as big as a modern game though, I consider this an ok price to pay.

How was this tested?

By running it on my computer.

If this makes graphical changes, please attach screenshots.

image

@BootsinSoots
Copy link
Contributor

As much as I think being able to pick and choose was nice, this is probably for the best. I can't count the number of times someone got confused because they thought they had to have a rom for everything, or didn't realize they could scroll further.

@alwaysintreble
Copy link
Collaborator

alwaysintreble commented Oct 5, 2023

If you disable Minecraft Forge Server Setup it still includes ArchipelagoMinecraftClient.exe, which will then let you just do the forge server setup then so unsure if we need to leave it in? You're also unable to update sprites without rerunning the installer

2023-10-05.01-15-01.mp4

@Berserker66
Copy link
Member Author

The minecraft thing is fully intentional. I don't know what's going wrong with the adjuster for you, there should be an update sprites button.

@alwaysintreble
Copy link
Collaborator

alwaysintreble commented Oct 5, 2023

fwiw i can repro it from the script too. repro steps:

  1. have no folder at data/sprites/alttpr (either through manually deleting it or just never installing sprites)
  2. run adjuster
  3. open the sprite selection dialog
    Window is completely broken whether you open it from single sprite selection or sprite pool selection

@el-u
Copy link
Collaborator

el-u commented Oct 5, 2023

The broken window usually happens when there is an unhandled exception in the adjuster.

@Berserker66
Copy link
Member Author

Fixed the unrelated bug in #2279

inno_setup.iss Outdated
Comment on lines 93 to 96
Root: HKCR; Subkey: ".aplttp"; ValueData: "{#MyAppName}patch"; Flags: uninsdeletevalue; ValueType: string; ValueName: "";
Root: HKCR; Subkey: "{#MyAppName}patch"; ValueData: "Archipelago Binary Patch"; Flags: uninsdeletekey; ValueType: string; ValueName: "";
Root: HKCR; Subkey: "{#MyAppName}patch\DefaultIcon"; ValueData: "{app}\ArchipelagoSNIClient.exe,0"; ValueType: string; ValueName: "";
Root: HKCR; Subkey: "{#MyAppName}patch\shell\open\command"; ValueData: """{app}\ArchipelagoSNIClient.exe"" ""%1"""; ValueType: string; ValueName: "";
Copy link
Member

Choose a reason for hiding this comment

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

i do not agree with the tab stops :S

Copy link
Member

@black-sliver black-sliver left a comment

Choose a reason for hiding this comment

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

Code looks good, but i can't really test.
Maybe @KonoTyran because of the minecraft checkbox.

One day ™️ we should automate compilation of the .iss, running the installer unattended and rolling a game with it.

@KonoTyran
Copy link
Contributor

So it always installs all clients now. Even minecraft if the box is unchecked? I would almost rather it not be there then. As it will do all initialization upon first run. Running the --install param is not required. Just let's one download/setup the server before first game.

@LegendaryLinux
Copy link
Member

It's probably worth including a storage space requirement on the downloads page now, and to let users know the AP installer is now an all-in-one deal. I'm cool with installing everything since I know the overall space requirement is relatively small.

My one hang-up is related to what Kono said. What does full-initialization mean? Does it install Java? I know the server is Forge, but there's a checkbox for that.

@KonoTyran
Copy link
Contributor

KonoTyran commented Oct 10, 2023

the Minecraft client does a few things when it starts up

  1. fetch minecraft_versions.json from my repo to know the proper jdk/forge versions
  2. check JDK (downloads if missing) (with --install it will always delete what it has and re-download)
  3. check forge (download and run installer if missing)
  4. copy apmc file over where it is expected
  5. launch minecraft server.

running it with --install just stops it after step 3. and instead of using the version info from the apmc it uses the first one found in minecraft_versions.json
it will also cache minecraft_versions.json incase it ever does not have internet connection it will still work in a pure local-only environment.

it also never "installs" java it uses a portable .zip version.

@LegendaryLinux
Copy link
Member

I see. If the user never runs the MC client, that process does not occur, right? If so, I think it is okay to include.

@ThePhar ThePhar added is: enhancement Issues requesting new features or pull requests implementing new features. affects: core Issues/PRs that touch core and may need additional validation. is: refactor/cleanup Improvements to code/output readability or organizization. labels Oct 17, 2023
@ThePhar
Copy link
Member

ThePhar commented Oct 23, 2023

It's probably worth including a storage space requirement on the downloads page now, and to let users know the AP installer is now an all-in-one deal. I'm cool with installing everything since I know the overall space requirement is relatively small.

My one hang-up is related to what Kono said. What does full-initialization mean? Does it install Java? I know the server is Forge, but there's a checkbox for that.

To be fair, it always installed everything in the background in lib/worlds, so this actually should take up less space because now we don't have the extra .exe files.

@Berserker66
Copy link
Member Author

@KonoTyran so remove the minecraft --install option entirely, leaving only LttP sprite setup?

@KonoTyran
Copy link
Contributor

Yea it's not needed. The client will install upon first run of an apmc file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects: core Issues/PRs that touch core and may need additional validation. is: enhancement Issues requesting new features or pull requests implementing new features. is: refactor/cleanup Improvements to code/output readability or organizization.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants