feat(migration): import NX2navi saves and Hookshot game settings #52
No reviewers
Labels
No labels
area/build
area/install
area/saves
area/ui
area/updates
good first issue
gpu/amd
gpu/intel
gpu/nvidia
help wanted
kind
bug
kind
chore
kind
docs
kind
feature
kind
performance
kind
question
platform/linux
platform/steam-deck
platform/windows
priority
critical
priority
high
priority
low
status
blocked
status
confirmed
status
duplicate
status
invalid
status
needs-info
status
wontfix
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
MagicBeanDev/LongshotLauncher!52
Loading…
Reference in a new issue
No description provided.
Delete branch "brunobaccari/LongshotLauncher:feat/launcher-save-migration"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Players coming from NX2navi have to find the import screen themselves and create a Longshot save before they can bring their progress over. Hookshot players also need to set up graphics and controls again. Offer detected NX2navi saves on the home screen, import into the player folder Longshot opens on its first launch, and provide a separate import of compatible Hookshot game settings into a chosen installed build.
Validation:
The full Windows core runner still encounters existing Linux-path assumptions in SelfUpdate and symlink privilege requirements. The signed fixture must retain its repository LF bytes when testing; Windows autocrlf changes invalidate its signature. No unrelated checks were weakened.
Screenshots use only synthetic text fixtures; no game files or real saves.
Not verified: loading a real migrated save in the game, the native Linux GUI, or every historical Hookshot 0–3 settings format. The checked Hookshot schema is version 4, with the original version marker retained for Longshot's own migrations; future versions are refused. No real save or game configuration was changed during this work.
Thanks: the NX2navi first-import path is careful (fixed user ID checked against Longshot's source, pending
save-importrefusal, ambiguity refused, good fixture coverage). A few things should change before merge, mostly in the Hookshot settings import:longshot_options.iniis replaced whole, dropping every Longshot line Hookshot doesn't carry (compile_shaders,vsync,vram_budget,memory_budget,auto_*, thenote_*bindings). Merge onto the target's lines instead.~/Gamesor another drive always fails.Core/Saves.csresolves links (Real) instead; refuse links in the files, not the path to them.Also: CONTRIBUTING asks for one topic per pull request, and this one has two (the NX2navi offer/first import, and the Hookshot settings import). Splitting them would let the first merge while the second is settled. CI hasn't run yet: run 169 is waiting for approval (fork PR), so the build/test status is still open.
Smaller notes inline.
@ -8,3 +8,2 @@- **Clean break.** The launcher reads nothing of a Hookshot install, its settings, folders, channels, versions or caches,except its saves (`Core/HookshotSaves.cs`). Don't add compatibility with Hookshot or its 2.x builders.- **Clean break.** Only Hookshot's saves (`Core/HookshotSaves.cs`) and the compatible game settings explicitly selected forThis relaxes a project rule ("the launcher reads nothing of a Hookshot install except its saves"). It may be the right call, but it's a maintainer decision rather than part of a feature PR. Please get an explicit OK on it. Also, line 3 still says docs/saves.md has "the one bridge from Hookshot", which is no longer accurate.
@ -0,0 +22,4 @@ImportCommand = new Command(Import, () => CanImport);Targets = main.Service.Registry.Builds.Where(main.Service.IsBuilt).ToList();target = Targets.FirstOrDefault(b => b.Default) ?? (Targets.Count == 1 ? Targets[0] : null);var roots = HookshotSaves.Roots(main.Service.SaveEnv)Nit: the target is preselected (the default build), so the first thing the player sees is already a ready Import that replaces that build's settings. That's fine given the warning box, but consider leaving the choice empty when there are several builds, as the PR description says ("explicit target build").
@ -0,0 +46,4 @@var info = new DirectoryInfo(path);if (mustExist && !info.Exists) throw new SaveFailure($"No game folder at {path}.");for (var at = info; at != null; at = at.Parent)if (at.LinkTarget != null || (at.Exists && at.Attributes.HasFlag(FileAttributes.ReparsePoint)))This walks every ancestor up to the root and refuses on any link or reparse point, for the source, the target build and the backup root. Builds commonly sit under a symlinked
~/Gameson another drive (Linux), and Windows folders under OneDrive or a redirected profile are reparse points. On those PCs the settings import always fails with "A settings folder is a link", even though nothing unsafe is going on.Core/Saves.cshandles the same situation by resolving the path (Real) and refusing links only inside what is copied (RefuseLinks). Please do the same here: resolve the folders, compare the resolved paths for the source/target/backup overlap checks, and refuse links only for the settings files themselves.@ -0,0 +121,4 @@valid = values.Length is 1 or 2 && values.All(v => Integer(v.Trim(), pad ? -1 : 0, pad ? 16 : 255));}else { skipped.Add(key); continue; }if (!valid) throw new SaveFailure($"Hookshot setting {key} has an invalid value. Nothing was copied.");One value outside the launcher's own copy of the ranges refuses the whole import ("Nothing was copied"). Longshot clamps every row on load (
menu_init:std::min(std::max(v, r.lo), r.hi)), so a value out of range is harmless there. Meanwhile the table at the top is a hand copy of two repositories' menus that will drift: a Hookshot release that adds a 7th frame-rate cap would then block every import. I'd suggest skipping and listing an invalid key the same way as an unknown one, keeping the hard refusal for structural problems (not UTF-8, notkey=value, neweroptions_version). If the table stays, the--longshotcheck intests/test_launcher_core.pycould also compare it with Longshot's rows, as it now does forUserId.@ -0,0 +125,4 @@kept[key] = value;}if (kept.Keys.Any(k => k != "options_version"))files.Add("longshot_options.ini", Utf8.GetBytes(string.Join('\n', kept.Select(kv => kv.Key + "=" + kv.Value)) + "\n"));This writes a fresh file from Hookshot's keys only, and
Importthen replaces the build's wholelongshot_options.ini. Every Longshot line Hookshot doesn't have is lost:compile_shaders(make_game.py'swrite_compile_levelkeeps it in this file and reads it back on update),vsync,vram_budget,memory_budget,auto_for/auto_tuned/auto_steps,hdr_contrast,field_of_view, thenote_*bindings, and so on. The confirmation text says the listed settings replace this version's, but the player also loses settings Hookshot never had.Please merge instead: start from the target's existing lines, replace or add only the keys taken from Hookshot, and keep the rest (as make_game.py's
write_compile_leveland the menu's own save do). Add a check with a target that holdscompile_shaders=1andvsync=2to confirm they survive.@ -0,0 +131,4 @@{var code = Text(language, "language.txt").Split('\n').Select(l => l.Trim()).FirstOrDefault(l => l.Length > 0 && !l.StartsWith('#'));var known = Languages.Normalize(code) ?? throw new SaveFailure("language.txt has no supported game language. Nothing was copied.");files.Add("language.txt", Utf8.GetBytes(known + "\n"));Note that
language.txthere can be undone by the next update of this build: BuildStepViewModel passesSettings.Languageto make_game.py as--language, which rewriteslanguage.txt. If the player has a language set in the launcher, the imported one lasts only until the next update. Either set the launcher'sLanguageto the imported one as well (if that's the intent), or say in the dialog that the launcher's language choice applies on update.@ -282,0 +287,4 @@public static string ImportTarget(SaveEnvironment env){var profiles = Shared(env).Profiles;return profiles.Count == 0 ? SharedSavePath(env, UserId) : Pick(profiles, "Longshot").Path;Question: the game only ever opens
save/<UserId>(the comment above says so). So when Longshot has exactly one profile and it isn'tUserId(a stray folder, a hand copy), this imports into a folder the game never reads, and the import looks successful. Would it be simpler and more correct to always targetSharedSavePath(env, UserId)? That would also make the several-profiles refusal unnecessary for imports. If there's a reason to keep "the only profile", a check for a single non-UserIdprofile would help.@ -119,0 +152,4 @@restoration, unsupported values and linked paths.Settings compatibility was compared on 2026-11-13 against Hookshot[`363038d8`](https://github.com/brunobaccari/Hookshot/tree/363038d8903009d8ef06a6f6df87c7c5bde370e1)Nit: this links a commit on a personal fork (
brunobaccari/Hookshot). Please link the same commit on upstream https://github.com/NotRexed/Hookshot if it's there, so the reference still works if the fork goes away.Fixes for the review, pushed as one commit on top of yours: new head
ee07597f. Nothing was built or tested here: no dotnet, no checks, no game. The C# was written by reading, and the checks below were updated or added the same way. Please runpython -I tests/test_launcher_core.py(or let CI run it) before relying on it.1. The import merges into
longshot_options.iniinstead of replacing it (Core/HookshotSettings.cs,Merge). The import starts from the build's existing file. Every line stays where it is:compile_shaders,vsync, the budgets,auto_*,hdr_contrast,field_of_view, thenote_*bindings, comments and unknown keys. Each imported key replaces the value on its line, and imported keys the file doesn't have yet go at the end. CRLF files keep CRLF.options_versionis imported like any other key (Hookshot's marker), so Longshot migrates the imported rows. The migrations invk_menu.cppmenu_init(v<4, v<5, v<6,!v2) only touch rows Hookshot also has, so none of the Longshot-only lines that are kept is affected. The backup is still the whole old file. New checks cover a target withcompile_shaders=1,vsync=2, anote_*line, a comment and an unknown key, comparing the exact merged text, plus directMergecases (CRLF, no file, duplicate lines, an unreadable line kept).2. Links are resolved, not refused (
HookshotSettings.Folder). The source, target and backup folders go throughSaves.Real(nowinternal, as isSaves.Inside, so both are reused rather than copied). The overlap checks compare the resolved paths. Only the settings files themselves (hookshot_options.ini,language.txt, the target'slongshot_options.ini) are refused as links, as before. One change inSaves:Realresolved only the last part of the path, which isn't enough for the alias case your own check had (target =<linked parent>/Hookshot, which is the source). It now resolves every part of the path (with a guard against loops of links). For the same reason,Saves.Copy's check that the backup isn't inside the target now resolves the backup path too, so a symlinked parent can't hide an overlap. The link checks were rewritten: a folder under a linked parent and a linked game folder are accepted; source/target/backup aliases that land in the same folder are still refused; a linked targetlongshot_options.iniis refused; and a full import through linked source, target and backup folders writes into the real folders with a backup.3. An out-of-range value is skipped and listed, not fatal. It joins the unknown keys in
SkippedKeys, and the dialog text now says "unknown to Longshot or have a value it does not take". The import still refuses outright for a file that isn't text, isn'tkey=value, or has a neweroptions_version. The checks movedvolume=999,output_res=99999x99999andpad_a=99from the refused list into a preview that lists them and a real import that keeps the build's ownvolume/pad_a.4. AGENTS.md "Clean break": not changed either way. That's for the maintainers to decide, and it still needs their explicit OK. I only fixed line 3, which your change made inaccurate: docs/saves.md now covers "what is imported from Hookshot (its saves, and its compatible game settings)" instead of "the one bridge".
5. Splitting NX2navi and the Hookshot settings import into two PRs: not done. That's your call. If you split them, the NX2navi offer and first import could merge while the settings import is still being settled.
6. Language. When the launcher has a game language set and it differs from the imported one, the import also sets the launcher's language to the imported one, so the next update (
--language) keeps it. The dialog says so before Import ("Other versions take it when they are next updated", since the launcher has one language for all versions). When the launcher has no language set,make_game.pykeeps the folder'slanguage.txt, so nothing changes. If you'd rather not touch the launcher's setting, the alternative is to drop that and keep only a note in the dialog.7.
ImportTargetalways targetssave/<UserId>. I checked this against Longshotmain:host/nn_applet.chas the oneuser_uid {0x1, 0x5a3c9e1f0b7d2468}, andhost/nn_fs.cmountssave\%016llx%016llxwith the high word first, so5a3c9e1f0b7d24680000000000000001is the only folder the game opens. Any other profile folder is left alone, and the several-profiles refusal for imports is gone (export still picks Longshot's one profile as before). The checks inSavesChecks.csandHookshotChecks.csnow put a stray profile beside it and check that the import lands in the fixed user and leaves the stray folder untouched. The Windows import check now uses theUserIdfolder. docs/saves.md is updated to match.8. docs/saves.md's Hookshot link: left as it is. Could you check whether
363038d8is on upstream https://github.com/NotRexed/Hookshot? It exists on your fork (parentf8efe057), but I couldn't confirm it upstream from here. If it is, please switch the link to upstream; if not, link the nearest upstream commit with the samevk_menu.cpp/upscaler_rows.h/nn_hid.c.9. No version is preselected in the settings dialog. Import stays disabled until a version is chosen ("Choose the Longshot version whose settings will be replaced."). The warning box now describes the merge: this version's other settings stay as they are.
The PR description's validation section predates these changes and needs a fresh run (and the "refused when a settings path passes through a link" behaviour is gone, see 2).
Light re-review of the fix commit
ee07597fonly, read, not built or run. Nothing has compiledee07597fyet: CI's Compile and test job was skipped on this head (only the title check ran), so the reading below is all there is so far.One bug in our fix, so changes are still requested (this review replaces review 1186):
options_versionnow inherits the build's marker (inline onMerge). Before the merge, such a file replaced the whole ini, so Longshot read it as a pre-marker file (!v2invk_menu.cppmenu_init:realtime = 1, plus theversion < 4/5/6moves). NowLoadaccepts it with no marker,Mergekeeps the build'soptions_version=6, and Hookshot's old rows are read as current. For example, an old file'sgame_speeddefault that Longshot's!v2rule exists to undo comes back as Per frame (the 30 fps slow motion). Fix: when the import has nooptions_version, drop the target'soptions_versionline(s) inMergeand add a check for it.Checked and fine:
Merge: an empty or missing file, no trailing newline, CRLF, duplicate keys in the target (all replaced, so the last one the game reads is right), unreadable lines kept, a BOM (already stripped byText, so it isn't written back). Every expected string in the new checks matches the code.Saves.Real: relative links (ResolveLinkTarget(true)gives a full path, and the recursion resolves the target's own parents), junctions (followed), OneDrive placeholders (cloud reparse tags aren't links, so they're left alone, which is what we want), UNC roots (GetPathRootkeeps\\server\share), a tail that doesn't exist yet (kept as typed, with the exception caught), and loops (ELOOP is caught, and recursion stops at depth 40).Real/Inside:Copy(Real(src) == Real(dst), the backup check, andtargetfor the staging and aside folders, which are still beside the real folder) andGatheronly get stricter or stay the same. Resolving the backup can only turn a missed overlap into a refusal; it can't refuse a backup that is really outside.SavesChecks.Case.RuncallsCopydirectly, so the export/import cases onc.Hsdon't depend onImportTarget.ImportTargetreturnsUserIdalways, and the tests were moved to match (SavesChecksWindowsshared→Saves.UserId, the stray-profile cases).make_game.pykeeps the folder's ownlanguage.txtwhen no--languageis given, and the launcher passes one only when a language is set, so changing the launcher's language only when it is set and different is right. It's read from the dry run and applied only when the real import brought that language.Foundrecord, the tuple lists, the nullability ofLanguageChange, and the new XAML bindings (LanguageText/HasLanguageText) match the VM.internalMerge/Realare reachable because the test project compilesCore/**itself.Minor, not blocking (inline):
menu_shortcut< 5,ultrawide< 6) that the Hookshot file lacks or that were skipped as out of range keep the build's v6 value, and it then gets migrated again under the imported v4. That's rare with a complete v4 file, but the comment onMergeclaims more than holds.longshot_options.iniin the target refuses the import (ReadFile: "empty or too large"), so the empty-file case inMergecan't be reached fromImport.Still open, unchanged: point 4 (the AGENTS.md "Clean break" relaxation needs a maintainer's explicit OK), point 5 (splitting NX2navi from the settings import is the author's call), point 8 (the Hookshot fork link). After the fix above, the PR also needs CI's compile and test and a run in the launcher.
🤖 Generated with Claude Code
@ -0,0 +135,4 @@skipped.Add(key); // (unknown, or out of range: listed and left out, as Longshot would clamp or ignore it)continue;}skipped.Remove(key); // (a later valid line of the same key, as the game reads the last one)Nit: with
volume=50and thenvolume=999, the key is imported with 50 and also listed as skipped, while Hookshot itself would have used 999 (clamped). That's harmless, but the list says something that isn't true. Either drop the key when its last line is invalid, or don't list it when an earlier line was kept.@ -0,0 +155,4 @@/// own settings, such as make_game.py's compile_shaders, vsync, the budgets, auto_*, note_* bindings, and any key it does not/// know), each imported key's value put in place of the old one, and the imported keys the file does not have yet added at/// the end, as make_game.py's write_compile_level and the menu's own save keep the lines they do not write. options_version/// is one of the imported keys: Hookshot's marker, so that Longshot migrates the imported rows; the migrations it runsNit: "the migrations it runs ... are of rows Hookshot has too, none of the Longshot-only lines kept here" holds only when the Hookshot file actually carries those rows. If
menu_shortcutorultrawideis missing, or skipped as out of range, the build's v6 value is kept and then migrated again under the imported v4 (menu_shortcut0 → 2,ultrawide0 → 1 on a narrow screen). It's rare, so either word the comment to match or leave a note in docs/saves.md.@ -0,0 +159,4 @@/// (gpu/vk/vk_menu.cpp menu_init) are of rows Hookshot has too, none of the Longshot-only lines kept hereinternal static string Merge(string? existing, IReadOnlyList<(string Key, string Value)> imported){var values = imported.ToDictionary(kv => kv.Key, kv => kv.Value, StringComparer.Ordinal);Bug: when the Hookshot file has no
options_version(from before the marker),found.Optionshas nooptions_versionkey, so the build's ownoptions_version=6line is kept and Hookshot's rows are read as v6. That skips the!v2(realtime = 1) andversion < 4/5/6migrations that the whole-file replace used to get. Suggested fix: ifvalueshas nooptions_version, remove the existingoptions_versionline(s) here, so Longshot reads the merged file as a pre-marker one, as it did before. Add a check: targetoptions_version=6\nvsync=2\n, Hookshotgame_speed=0\n→ nooptions_versionline in the result.Fixed the findings of review #52 (comment) in
b2e1f458(new head, on top ofee07597f):options_versionin the Hookshot file (#52 (comment)):Mergenow removes the build'soptions_versionline(s) when the import has none, so Longshot reads the merged file as one from before the marker and runs the!v2(realtime=1) and version < 4/5/6 migrations on Hookshot's rows, as the whole-file replace did.game_speedwith no marker,key_y/key_zlbelow 3 in host/nn_hid.c,descriptor_reusebelow 4,menu_shortcutbelow 5,ultrawidebelow 6, from gpu/vk/vk_menu.cpp menu_init, vk_draw.cpp menu_env_on and host/hos_wide_size.h) is removed when the two versions fall on different sides of that migration, so it takes Longshot's default instead of being moved again. The table isHookshotSettings.Migrated. The comment now says exactly this, and docs/saves.md says it too.longshot_options.ini: a build's empty options file is read as empty (Hookshot's files still must not be empty). It's imported into without asking to overwrite and without a backup, and it's restored as empty if a write fails.New checks in tests/launcher_core/HookshotSettingsChecks.cs cover each one: no marker → the build's
options_version=6removed; v6 build with v4 import →menu_shortcut/ultrawideremoved anddescriptor_reuse/key_ykept; no-marker build with v4 import →game_speed/key_zl/descriptor_reuseremoved;volume=50thenvolume=999→ skipped and not imported; an empty target imported into with no backup.Nothing was built or tested on this PC. The checks are written but haven't been run.
Still open:
🤖 Generated with Claude Code
Re-review of
b2e1f458only (the fix for review 1222). Read, not built or run.The four points from 1222 are fixed:
options_versionnow drops the build's marker (now < 0addsoptions_versiontodrop), so Longshot reads the file as one from before the marker.Migratedplus the "different sides ofSince" test removes them. I checked the table against Longshotmain(c0442078):vk_menu.cppmenu_init (!v2→ game_speed,version < 4/5/6→ descriptor_reuse, menu_shortcut, ultrawide),nn_hid.cbinds_load (version < 3 && had_keys→ key_y, key_zl),vk_draw.cppmenu_env_on (descriptor_reuse, since 4; sampler_cache has no since),hos_main.c/hos_wide_size.h(ultrawide, < 6). No version-gated migration is missing. The dynamic_resolution, menu_size=0 and fps_counter moves depend on whether a line is there, not on the version, so they're not affected.had/now= -1 for "no line" againstSince0 matches!v2(a lineoptions_version=0is v2, and gives 0 here too). All of them use the lastoptions_versionline, ashaddoes. The rule is right both ways. Downgrade (build 6, Hookshot 4): menu_shortcut and ultrawide are removed, descriptor_reuse and key_y are kept. Upgrade (an unmarked build, Hookshot 4): game_speed, descriptor_reuse and key_y/zl are removed, and menu_shortcut is kept, since it's read under < 5 either way. I worked the newMergechecks through by hand and they match.kept.Remove/order.Remove), and the new Import check covers both orders.ReadFile(emptyOk: true)keeps it inprevious, so it isn't counted asreplacing. That means no consent and no backup, and a failed write restores it as empty bytes. An emptylanguage.txtgets the same treatment, which is harmless. The C# reads as compiling (Line(line) is ("options_version", var v), sibling-scope locals inMerge,System.Globalizationimported).One non-blocking note is inline: removing a line is wider than the migration it avoids, for menu_shortcut and key_y/key_zl.
Still open (none of it is code):
b2e1f458(run 180, only the title check ran), so nothing has compiled this commit or run its checks yet.@ -0,0 +197,4 @@foreach (var line in lines)if (Line(line) is ("options_version", var v)) had = OptionsVersion(v);var now = values.TryGetValue("options_version", out var marker) ? OptionsVersion(marker) : -1;var drop = new HashSet<string>(Migrated.Where(m => !values.ContainsKey(m.Key) && (had < m.Since) != (now < m.Since)).Select(m => m.Key),Non-blocking: removing the line is wider than what the migration does for two of these keys. menu_shortcut's migration only moves
0(version < 5 && opts.menu_shortcut == 0). So with the common pair, a build at 6 and Hookshot at 4, a Longshot player'smenu_shortcut=1(F1 only) is removed and comes back as 2 (Insert, F1 or Esc), when kept it would have been read as 1. binds_load is similar: it only fills an empty second slot, so removing key_y/key_zl also resets a custom first key (only for imports with no marker or a marker below 3). Keeping menu_shortcut when its value isn't 0, and key_y/key_zl when they have two slots, would keep those choices. With ultrawide, game_speed and descriptor_reuse, removing the line gives the same result as the migration, so nothing is lost there. docs/saves.md already says these lines take the default, so this can stay as it is if that's preferred.Superseded by #52 (comment): the code points are fixed; what's left is listed there.
Superseded by #52 (comment): the code points are fixed; what's left is listed there.