-
Notifications
You must be signed in to change notification settings - Fork 63
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
Upgradable Games #2596
base: main
Are you sure you want to change the base?
Upgradable Games #2596
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.
Partial review
src/Abstractions/NexusMods.Abstractions.GameLocators/GameInstallation.cs
Show resolved
Hide resolved
src/Abstractions/NexusMods.Abstractions.Games.FileHashes/Models/GogBuild.cs
Show resolved
Hide resolved
src/Abstractions/NexusMods.Abstractions.Games.FileHashes/IFileHashesService.cs
Outdated
Show resolved
Hide resolved
src/Abstractions/NexusMods.Abstractions.Games.FileHashes/Models/VersionDefinition.cs
Show resolved
Hide resolved
...el.Synchronizer.Tests/GeneralLoadoutManagementTests.SynchronizerIntegrationTests.verified.md
Outdated
Show resolved
Hide resolved
src/Abstractions/NexusMods.Abstractions.Loadouts.Synchronizers/ALoadoutSynchronizer.cs
Outdated
Show resolved
Hide resolved
Also implemented the backup files command in this PR. |
src/Abstractions/NexusMods.Abstractions.Loadouts.Synchronizers/SyncTreeNode.cs
Outdated
Show resolved
Hide resolved
src/Abstractions/NexusMods.Abstractions.GameLocators/Stores/Steam/SteamLocatorResultMetadata.cs
Outdated
Show resolved
Hide resolved
src/Abstractions/NexusMods.Abstractions.Loadouts.Synchronizers/ALoadoutSynchronizer.cs
Outdated
Show resolved
Hide resolved
src/Abstractions/NexusMods.Abstractions.Loadouts.Synchronizers/ALoadoutSynchronizer.cs
Outdated
Show resolved
Hide resolved
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.
GetOrCreateInitialDiskState()
is unused, so are IndexNewState()
and HashGameFile()
, are there plans for them or should we remove them?
{ | ||
// Make sure the file hashes are up to date | ||
await _fileHashService.GetFileHashesDb(); | ||
return await ReindexState(gameInstallation, Connection); |
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.
ReindexState()
is the only thing that actually is indexing game files from what I can tell now, and it isn't using the minimal hash stuff. So from what I can tell we aren't using minimal hashes anywhere at the moment.
Is the plan still to use them? If so in which situation?
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.
Removed the dead code
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.
These tests have the concept of initial state, which we don't have anymore.
They were supposed to have "bin/originalGameFile.txt"
as part of the initial state, but now it is just getting discarded on loadout creation.
We talked yesterday that we should put any unrecognized files into Overrides on initial management of the game. With that we could have these tests make behave more like they were supposed to.
Do you want to implement putting unrecognized files in override on game manage as a separate PR or as part of this one?
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.
Same as above, also counts for GeneralFileManagementTests
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.
I tried doing some basic tests with a Stardew installation:
Adding a new file in the game folder causes Apply to fail due to exception:
Steps:
- Manage clean SDV game
- Close app
- Add a new text file in the game folder (I used
test/newfile.txt
) - Open the app (this will run an ingest on startup)
- Open preview and notice the new file is marked as something to be removed
- Which shouldn't be correct
- Attempt to apply -> get exception:
System.ArgumentException: An item with the same key has already been added. Key: {Game}/test/newfile.txt
at System.Collections.Generic.Dictionary`2.TryInsert(TKey key, TValue value, InsertionBehavior behavior)
at System.Collections.Generic.Dictionary`2.Add(TKey key, TValue value)
at System.Linq.Enumerable.ToDictionary[TSource,TKey](IEnumerable`1 source, Func`2 keySelector, IEqualityComparer`1 comparer)
at System.Linq.Enumerable.ToDictionary[TSource,TKey](IEnumerable`1 source, Func`2 keySelector)
at NexusMods.Abstractions.Loadouts.Synchronizers.ALoadoutSynchronizer.ReindexState(GameInstallation installation, IConnection connection, ITransaction tx) in C:\Dev\Repos\NexusMods.App\src\Abstractions\NexusMods.Abstractions.Loadouts.Synchronizers\ALoadoutSynchronizer.cs:line 985
at NexusMods.Abstractions.Loadouts.Synchronizers.ALoadoutSynchronizer.ReindexState(GameInstallation installation, IConnection connection) in C:\Dev\Repos\NexusMods.App\src\Abstractions\NexusMods.Abstractions.Loadouts.Synchronizers\ALoadoutSynchronizer.cs:line 961
at NexusMods.Abstractions.Loadouts.Synchronizers.ALoadoutSynchronizer.BuildSyncTree(ReadOnly loadout) in C:\Dev\Repos\NexusMods.App\src\Abstractions\NexusMods.Abstractions.Loadouts.Synchronizers\ALoadoutSynchronizer.cs:line 355
at NexusMods.Abstractions.Loadouts.Synchronizers.ALoadoutSynchronizer.Synchronize(ReadOnly loadout) in C:\Dev\Repos\NexusMods.App\src\Abstractions\NexusMods.Abstractions.Loadouts.Synchronizers\ALoadoutSynchronizer.cs:line 762
at NexusMods.DataModel.Synchronizer.SynchronizerService.Synchronize(LoadoutId loadoutId) in C:\Dev\Repos\NexusMods.App\src\NexusMods.DataModel\Synchronizer\SynchronizerService.cs:line 66
at NexusMods.App.UI.LeftMenu.Items.ApplyControlViewModel.<Apply>b__28_0() in C:\Dev\Repos\NexusMods.App\src\NexusMods.App.UI\LeftMenu\Items\ApplyControl\ApplyControlViewModel.cs:line 115
at NexusMods.App.UI.LeftMenu.Items.ApplyControlViewModel.Apply() in C:\Dev\Repos\NexusMods.App\src\NexusMods.App.UI\LeftMenu\Items\ApplyControl\ApplyControlViewModel.cs:line 113
at NexusMods.App.UI.LeftMenu.Items.ApplyControlViewModel.<>c__DisplayClass27_0.<<-ctor>b__0>d.MoveNext() in C:\Dev\Repos\NexusMods.App\src\NexusMods.App.UI\LeftMenu\Items\ApplyControl\ApplyControlViewModel.cs:line 55
EDIT:
I just tested and starting with a completely clean SDV, you are asked to Apply when you create the first loadout, which is a regression. Applying caused a bunch of exceptions, but here the Apply button then got hidden.
A lot of exceptions from NoWayToSourceFilesOnDisk
diagnostic going through the synchronizer:
System.ArgumentException: An item with the same key has already been added. Key: {Game}/api-ms-win-core-console-l1-1-0.dll
at System.Collections.Generic.Dictionary`2.TryInsert(TKey key, TValue value, InsertionBehavior behavior)
at System.Collections.Generic.Dictionary`2.Add(TKey key, TValue value)
at System.Linq.Enumerable.ToDictionary[TSource,TKey](IEnumerable`1 source, Func`2 keySelector, IEqualityComparer`1 comparer)
at System.Linq.Enumerable.ToDictionary[TSource,TKey](IEnumerable`1 source, Func`2 keySelector)
at NexusMods.Abstractions.Loadouts.Synchronizers.ALoadoutSynchronizer.ReindexState(GameInstallation installation, IConnection connection, ITransaction tx) in C:\Dev\Repos\NexusMods.App\src\Abstractions\NexusMods.Abstractions.Loadouts.Synchronizers\ALoadoutSynchronizer.cs:line 985
at NexusMods.Abstractions.Loadouts.Synchronizers.ALoadoutSynchronizer.ReindexState(GameInstallation installation, IConnection connection) in C:\Dev\Repos\NexusMods.App\src\Abstractions\NexusMods.Abstractions.Loadouts.Synchronizers\ALoadoutSynchronizer.cs:line 961
at NexusMods.Abstractions.Loadouts.Synchronizers.ALoadoutSynchronizer.BuildSyncTree(ReadOnly loadout) in C:\Dev\Repos\NexusMods.App\src\Abstractions\NexusMods.Abstractions.Loadouts.Synchronizers\ALoadoutSynchronizer.cs:line 355
at NexusMods.Games.FileHashes.Emitters.NoWayToSourceFilesOnDisk.Diagnose(ReadOnly loadout, CancellationToken cancellationToken)+MoveNext() in C:\Dev\Repos\NexusMods.App\src\NexusMods.Games.FileHashes\Emitters\NoWayToSourceFilesOnDisk.cs:line 16
at NexusMods.Games.FileHashes.Emitters.NoWayToSourceFilesOnDisk.Diagnose(ReadOnly loadout, CancellationToken cancellationToken)+System.Threading.Tasks.Sources.IValueTaskSource<System.Boolean>.GetResult()
at NexusMods.DataModel.Diagnostics.DiagnosticManager.<>c__DisplayClass8_1.<<GetLoadoutDiagnostics>b__0>d.MoveNext() in C:\Dev\Repos\NexusMods.App\src\NexusMods.DataModel\Diagnostics\DiagnosticManager.cs:line 92
--- End of stack trace from previous location ---
at NexusMods.DataModel.Diagnostics.DiagnosticManager.<>c__DisplayClass8_1.<<GetLoadoutDiagnostics>b__0>d.MoveNext() in C:\Dev\Repos\NexusMods.App\src\NexusMods.DataModel\Diagnostics\DiagnosticManager.cs:line 92
…e select winning files
…eam/SteamLocatorResultMetadata.cs Co-authored-by: Al <[email protected]>
…tes and ingesting
This PR does the following:
LocatorIDs
onto each loadout. These are opaque strings that each game locator (for Stem, Gog, etc. ) can use to tie back to a game build of some sortGame Files
mod from the loadouts instead these files are injected into the synchronizer before it starts processing the diffsALoadoutSynchronizer
to use massively less memory allocation, and to support injecting game filesKnown limitations: